add strbuf_set operations

PATCH Superseded Jeremiah Mahler 3 participants 28 messages Jun 15, 2016 Documentation

lore

Share & tools

lore

Jeremiah Mahler

author

2016-06-15 23:01 UTC #0 lore raw

v2 cover add strbuf_set operations

Version 2 of the patch set to add strbuf_set operations.

Includes suggestions from Eric Sunshine [1]:

- New operations and their documentation placed in one patch.

- Less ambiguous documentation: "Replace the buffer content with [...]"

- Use imperative mood in log messages.

- Don't use strbuf_set operations in cases where strbuf_add is used to build up a buffer in multiple steps. Multiple adds suggest that re-ordering is possible whereas a strbuf_set at the beginning suggests that it isn't. This is confusing.

Using strbuf_set before a sequence of adds can be confusing. But using strbuf_add when nothing important was being added to can also be confusing. strbuf_set can be used to make these cases more clear.

  struct strbuf mybuf = STRBUF_INIT;

strbuf_add(&mybuf, ...); /* Was something there before? */

strbuf_set(&mybuf, ...); /* Replace everything. */

Several single line replacements were made for this reason.

Additional files have also been converted compared to version 1 [1].

[1]: http://marc.info/?l=git&m=140230874124060&w=2

Jeremiah Mahler (19):
add strbuf_set operations
sha1_name: simplify via strbuf_set()
fast-import: simplify via strbuf_set()
builtin/remote: simplify via strbuf_set()
branch: simplify via strbuf_set()
builtin/branch: simplify via strbuf_set()
builtin/checkout: simplify via strbuf_set()
builtin/mailinfo: simplify via strbuf_set()
builtin/tag: simplify via strbuf_set()
date: simplify via strbuf_set()
diffcore-order: simplify via strbuf_set()
http-backend: simplify via strbuf_set()
http: simplify via strbuf_set()
ident: simplify via strbuf_set()
remote-curl: simplify via strbuf_set()
submodule: simplify via strbuf_set()
transport: simplify via strbuf_set()
vcs-svn/svndump: simplify via strbuf_set()
wt-status: simplify via strbuf_set()

 Documentation/technical/api-strbuf.txt | 18 +++++++++++
 branch.c                               |  6 ++--
 builtin/branch.c                       |  3 +-
 builtin/checkout.c                     | 18 ++++-------
 builtin/mailinfo.c                     | 18 ++++-------
 builtin/remote.c                       | 59 ++++++++++++----------------------
 builtin/tag.c                          |  3 +-
 date.c                                 |  3 +-
 diffcore-order.c                       |  3 +-
 fast-import.c                          |  6 ++--
 http-backend.c                         |  6 ++--
 http.c                                 |  3 +-
 ident.c                                |  6 ++--
 remote-curl.c                          |  3 +-
 sha1_name.c                            | 15 +++------
 strbuf.c                               | 21 ++++++++++++
 strbuf.h                               | 14 ++++++++
 submodule.c                            |  5 ++-
 transport.c                            |  3 +-
 vcs-svn/svndump.c                      |  3 +-
 wt-status.c                            |  3 +-
 21 files changed, 110 insertions(+), 109 deletions(-)

Signature · 1 line

2.0.0.592.gf55b190

Jeremiah Mahler

author

2016-06-15 23:01 UTC #1 lore raw

PATCH v2 1/19 add strbuf_set operations

Currently, the data in a strbuf is modified using add operations. To set the buffer to some data a reset must be performed before an add.

  strbuf_reset(buf);
  strbuf_add(buf, cb.buf.buf, cb.buf.len);

And this is a common sequence of operations with 70 occurrences found in the current source code. This includes all the different variations (add, addf, addstr, addbuf, addch).

  FILES=`find ./ -name '*.c'`
  CNT=$(pcregrep -M "strbuf_reset.*\n.*strbuf_add" $FILES | wc -l)
  CNT=$(echo "$CNT / 2" | bc)
  echo $CNT
  70

These patches add strbuf_set operations which allow this common sequence to be performed in one line instead of two.

  strbuf_set(buf, cb.buf.buf, cb.buf.len);

Signed-off-by Jeremiah Mahler <[email protected]>

---
 Documentation/technical/api-strbuf.txt | 18 ++++++++++++++++++
 strbuf.c                               | 21 +++++++++++++++++++++
 strbuf.h                               | 14 ++++++++++++++
 3 files changed, 53 insertions(+)

Documentation/technical/api-strbuf.txt +18 -0

index 077a709..b7e23da 100644
@@ -149,6 +149,24 @@ Functions
 	than zero if the first buffer is found, respectively, to be less than,
 	to match, or be greater than the second buffer.
 
+* Setting the buffer
+
+`strbuf_set`::
+
+	Replace the buffer content with data of a given length.
+
+`strbuf_setstr`::
+
+	Replace the buffer content with data from a NUL-terminated string.
+
+`strbuf_setf`::
+
+	Replace the buffer content with a formatted string.
+
+`strbuf_setbuf`::
+
+	Replace the buffer content with data from another buffer.
+
 * Adding data to the buffer
 
 NOTE: All of the functions in this section will grow the buffer as necessary.

strbuf.c +21 -0

index ac62982..9d64b00 100644
@@ -189,6 +189,27 @@ void strbuf_splice(struct strbuf *sb, size_t pos, size_t len,
 	strbuf_setlen(sb, sb->len + dlen - len);
 }
 
+void strbuf_set(struct strbuf *sb, const void *data, size_t len)
+{
+	strbuf_reset(sb);
+	strbuf_add(sb, data, len);
+}
+
+void strbuf_setf(struct strbuf *sb, const char *fmt, ...)
+{
+	va_list ap;
+	strbuf_reset(sb);
+	va_start(ap, fmt);
+	strbuf_vaddf(sb, fmt, ap);
+	va_end(ap);
+}
+
+void strbuf_setbuf(struct strbuf *sb, const struct strbuf *sb2)
+{
+	strbuf_reset(sb);
+	strbuf_add(sb, sb2->buf, sb2->len);
+}
+
 void strbuf_insert(struct strbuf *sb, size_t pos, const void *data, size_t len)
 {
 	strbuf_splice(sb, pos, 0, data, len);

strbuf.h +14 -0

index e9ad03e..b339f08 100644
@@ -101,6 +101,20 @@ static inline struct strbuf **strbuf_split(const struct strbuf *sb,
  */
 extern void strbuf_list_free(struct strbuf **);
 
+/*----- set buffer to data -----*/
+
+extern void strbuf_set(struct strbuf *sb, const void *data, size_t len);
+
+static inline void strbuf_setstr(struct strbuf *sb, const char *s)
+{
+	strbuf_set(sb, s, strlen(s));
+}
+
+__attribute__((format (printf,2,3)))
+extern void strbuf_setf(struct strbuf *sb, const char *fmt, ...);
+
+extern void strbuf_setbuf(struct strbuf *sb, const struct strbuf *sb2);
+
 /*----- add data in your buffer -----*/
 static inline void strbuf_addch(struct strbuf *sb, int c)
 {

Signature · 1 line

2.0.0.592.gf55b190

Michael Haggerty

2016-06-15 23:01 UTC #2 lore raw

Quoted 62 lines · Jeremiah Mahler

Currently, the data in a strbuf is modified using add operations. To set the buffer to some data a reset must be performed before an add.

  strbuf_reset(buf);
  strbuf_add(buf, cb.buf.buf, cb.buf.len);

And this is a common sequence of operations with 70 occurrences found in the current source code. This includes all the different variations (add, addf, addstr, addbuf, addch).

  FILES=`find ./ -name '*.c'`
  CNT=$(pcregrep -M "strbuf_reset.*\n.*strbuf_add" $FILES | wc -l)
  CNT=$(echo "$CNT / 2" | bc)
  echo $CNT
  70

These patches add strbuf_set operations which allow this common sequence to be performed in one line instead of two.

  strbuf_set(buf, cb.buf.buf, cb.buf.len);

Signed-off-by Jeremiah Mahler <[email protected]>

---
 Documentation/technical/api-strbuf.txt | 18 ++++++++++++++++++
 strbuf.c                               | 21 +++++++++++++++++++++
 strbuf.h                               | 14 ++++++++++++++
 3 files changed, 53 insertions(+)

Documentation/technical/api-strbuf.txt +18 -0

index 077a709..b7e23da 100644
@@ -149,6 +149,24 @@ Functions
 	than zero if the first buffer is found, respectively, to be less than,
 	to match, or be greater than the second buffer.
 
+* Setting the buffer
+
+`strbuf_set`::
+
+	Replace the buffer content with data of a given length.
+
+`strbuf_setstr`::
+
+	Replace the buffer content with data from a NUL-terminated string.
+
+`strbuf_setf`::
+
+	Replace the buffer content with a formatted string.
+
+`strbuf_setbuf`::
+
+	Replace the buffer content with data from another buffer.
+
 * Adding data to the buffer
 
 NOTE: All of the functions in this section will grow the buffer as necessary.

strbuf.c +0 -0

index ac62982..9d64b00 100644

strbuf.c void strbuf_splice(struct strbuf *sb, size_t pos, size_t len, · line 197

Quoted 9 lines

strbuf.c +6 -0

@@ -189,6 +189,27 @@ void strbuf_splice(struct strbuf *sb, size_t pos, size_t len,
 	strbuf_setlen(sb, sb->len + dlen - len);
 }
 
+void strbuf_set(struct strbuf *sb, const void *data, size_t len)
+{
+	strbuf_reset(sb);
+	strbuf_add(sb, data, len);
+}
+

I never know how much intelligence to attribute to modern compilers, but it seems like even after optimization this function will be doing more work than necessary:

    strbuf_reset(sb)
    -> strbuf_setlen(sb, 0)
       -> sb->len = 0
       -> sb->buf[len] = '\0'
    -> strbuf_add(sb, data, len)
       -> strbuf_grow(sb, len)
          -> ...lots of stuff...
       -> memcpy(sb->buf + sb->len, data, len)
       -> strbuf_setlen(sb, sb->len + len)
          -> sb->len = len
          -> sb->buf[len] = '\0'

If there were a function like strbuf_grow_to(sb, len):

void strbuf_grow_to(struct strbuf *sb, size_t len)

{
	int new_buf = !sb->alloc;
	if (unsigned_add_overflows(len, 1))
		die("you want to use way too much memory");
	if (new_buf)
		sb->buf = NULL;
	ALLOC_GROW(sb->buf, len + 1, sb->alloc);
	if (new_buf)
		sb->buf[0] = '\0';
}

(strbuf_grow() could call it:

static inline void strbuf_grow(struct strbuf *sb, size_t extra)

{
	if (unsigned_add_overflows(sb->len, extra))
		die("you want to use way too much memory");
	strbuf_grow_to(sb, sb->len + extra);
}

), then your function could be minimized to

void strbuf_set(struct strbuf *sb, const void *data, size_t len)

{
	strbuf_grow_to(sb, len);
	memcpy(sb->buf, data, len);
	strbuf_setlen(sb, len);
}

I think strbuf_grow_to() would be useful in other situations too.

This is just an idea; I'm not saying that you have to make this change.

Quoted 1 line

[...]

Michael

Signature · 3 lines

Michael Haggerty
[email protected]
http://softwareswirl.blogspot.com/

Jeremiah Mahler

author

2016-06-15 23:01 UTC #3 lore raw

Michael,

Quoted 4 lines · Michael Haggerty

Quoted 3 lines · Jeremiah Mahler
Currently, the data in a strbuf is modified using add operations. To set the buffer to some data a reset must be performed before an add.

...

Quoted 34 lines

Quoted 5 lines
strbuf.c +0 -0
index ac62982..9d64b00 100644
strbuf.c void strbuf_splice(struct strbuf *sb, size_t pos, size_t len, · line 197
Quoted 9 lines
strbuf.c +6 -0
@@ -189,6 +189,27 @@ void strbuf_splice(struct strbuf *sb, size_t pos, size_t len,
 	strbuf_setlen(sb, sb->len + dlen - len);
 }
 
+void strbuf_set(struct strbuf *sb, const void *data, size_t len)
+{
+	strbuf_reset(sb);
+	strbuf_add(sb, data, len);
+}
+
I never know how much intelligence to attribute to modern compilers, but it seems like even after optimization this function will be doing more work than necessary:
    strbuf_reset(sb)
    -> strbuf_setlen(sb, 0)
       -> sb->len = 0
       -> sb->buf[len] = '\0'
    -> strbuf_add(sb, data, len)
       -> strbuf_grow(sb, len)
          -> ...lots of stuff...
       -> memcpy(sb->buf + sb->len, data, len)
       -> strbuf_setlen(sb, sb->len + len)
          -> sb->len = len
          -> sb->buf[len] = '\0'
If there were a function like strbuf_grow_to(sb, len):
void strbuf_grow_to(struct strbuf *sb, size_t len)

Quoted 11 lines

{
	int new_buf = !sb->alloc;
	if (unsigned_add_overflows(len, 1))
		die("you want to use way too much memory");
	if (new_buf)
		sb->buf = NULL;
	ALLOC_GROW(sb->buf, len + 1, sb->alloc);
	if (new_buf)
		sb->buf[0] = '\0';
}

grow_to() which could reduce in size, interesting.

Quoted 18 lines

(strbuf_grow() could call it:
static inline void strbuf_grow(struct strbuf *sb, size_t extra)
{
	if (unsigned_add_overflows(sb->len, extra))
		die("you want to use way too much memory");
	strbuf_grow_to(sb, sb->len + extra);
}
), then your function could be minimized to
void strbuf_set(struct strbuf *sb, const void *data, size_t len)
{
	strbuf_grow_to(sb, len);
	memcpy(sb->buf, data, len);
	strbuf_setlen(sb, len);
}

Quoted 4 lines

I think strbuf_grow_to() would be useful in other situations too.
This is just an idea; I'm not saying that you have to make this change.

I like your idea. I am leaning towards doing the un-optimized strbuf_set operations first, then optimizing in a later patch.

Quoted 8 lines

Quoted 1 line
[...]
Michael
Signature · 3 lines
Michael Haggerty
[email protected]
http://softwareswirl.blogspot.com/

Signature · 3 lines

Jeremiah Mahler
[email protected]
http://github.com/jmahler

Michael Haggerty

2016-06-15 23:01 UTC #4 lore raw

Quoted 16 lines · Jeremiah Mahler

Quoted 4 lines · Michael Haggerty
[...]
If there were a function like strbuf_grow_to(sb, len):
void strbuf_grow_to(struct strbuf *sb, size_t len)
Quoted 11 lines
{
	int new_buf = !sb->alloc;
	if (unsigned_add_overflows(len, 1))
		die("you want to use way too much memory");
	if (new_buf)
		sb->buf = NULL;
	ALLOC_GROW(sb->buf, len + 1, sb->alloc);
	if (new_buf)
		sb->buf[0] = '\0';
}

Quoted 1 line

grow_to() which could reduce in size, interesting.

I don't understand what you mean by "could reduce in size". This function can only grow, never reduce, the size of the strbuf, because ALLOC_GROW doesn't do anything unless the requested size is larger than the currently-allocated size.

Quoted 18 lines

Quoted 18 lines
(strbuf_grow() could call it:
static inline void strbuf_grow(struct strbuf *sb, size_t extra)
{
	if (unsigned_add_overflows(sb->len, extra))
		die("you want to use way too much memory");
	strbuf_grow_to(sb, sb->len + extra);
}
), then your function could be minimized to
void strbuf_set(struct strbuf *sb, const void *data, size_t len)
{
	strbuf_grow_to(sb, len);
	memcpy(sb->buf, data, len);
	strbuf_setlen(sb, len);
}

Quoted 6 lines

Quoted 4 lines
I think strbuf_grow_to() would be useful in other situations too.
This is just an idea; I'm not saying that you have to make this change.
I like your idea. I am leaning towards doing the un-optimized strbuf_set operations first, then optimizing in a later patch.

That's a good plan.

In case you're interested, I sketched what the strbuf_grow_to() changes might look like, and also looked for other places in the code where strbuf_grow_to() could be used instead of strbuf_grow() [1]. This is only a sketch; I haven't even tested the result. Feel free to use what you want from it.

Michael

[1] Branch "strbuf-grow-to-sketch" on https://github.com/mhagger/git

Signature · 3 lines

Michael Haggerty
[email protected]
http://softwareswirl.blogspot.com/

Jeremiah Mahler

author

2016-06-15 23:01 UTC #5 lore raw

Michael,

Quoted 17 lines · Michael Haggerty

Quoted 16 lines · Jeremiah Mahler
Quoted 15 lines · Michael Haggerty
[...]
If there were a function like strbuf_grow_to(sb, len):
void strbuf_grow_to(struct strbuf *sb, size_t len)
{
	int new_buf = !sb->alloc;
	if (unsigned_add_overflows(len, 1))
		die("you want to use way too much memory");
	if (new_buf)
		sb->buf = NULL;
	ALLOC_GROW(sb->buf, len + 1, sb->alloc);
	if (new_buf)
		sb->buf[0] = '\0';
}

Quoted 7 lines

Quoted 1 line
grow_to() which could reduce in size, interesting.
I don't understand what you mean by "could reduce in size". This function can only grow, never reduce, the size of the strbuf, because ALLOC_GROW doesn't do anything unless the requested size is larger than the currently-allocated size.

I got it now. strbuf_grow() adds extra to the current buf. strbuf_grow_to() will add more if len is larger than the current length.

Quoted 25 lines

Quoted 18 lines
Quoted 18 lines
(strbuf_grow() could call it:
static inline void strbuf_grow(struct strbuf *sb, size_t extra)
{
	if (unsigned_add_overflows(sb->len, extra))
		die("you want to use way too much memory");
	strbuf_grow_to(sb, sb->len + extra);
}
), then your function could be minimized to
void strbuf_set(struct strbuf *sb, const void *data, size_t len)
{
	strbuf_grow_to(sb, len);
	memcpy(sb->buf, data, len);
	strbuf_setlen(sb, len);
}
Quoted 6 lines
Quoted 4 lines
I think strbuf_grow_to() would be useful in other situations too.
This is just an idea; I'm not saying that you have to make this change.
I like your idea. I am leaning towards doing the un-optimized strbuf_set operations first, then optimizing in a later patch.

Quoted 8 lines

That's a good plan.
In case you're interested, I sketched what the strbuf_grow_to() changes might look like, and also looked for other places in the code where strbuf_grow_to() could be used instead of strbuf_grow() [1]. This is only a sketch; I haven't even tested the result. Feel free to use what you want from it.

I started looking at it. I will let you know if I get something together.

Quoted 8 lines

Michael
[1] Branch "strbuf-grow-to-sketch" on https://github.com/mhagger/git
Signature · 3 lines
Michael Haggerty
[email protected]
http://softwareswirl.blogspot.com/

Signature · 3 lines

Jeremiah Mahler
[email protected]
http://github.com/jmahler

Eric Sunshine

2016-06-15 23:01 UTC #6 lore raw

In reply to Jeremiah Mahler

Quoted 16 lines · Jeremiah Mahler

Currently, the data in a strbuf is modified using add operations. To set the buffer to some data a reset must be performed before an add.
  strbuf_reset(buf);
  strbuf_add(buf, cb.buf.buf, cb.buf.len);
And this is a common sequence of operations with 70 occurrences found in the current source code. This includes all the different variations (add, addf, addstr, addbuf, addch).
  FILES=`find ./ -name '*.c'`
  CNT=$(pcregrep -M "strbuf_reset.*\n.*strbuf_add" $FILES | wc -l)
  CNT=$(echo "$CNT / 2" | bc)
  echo $CNT
  70

Quoted 4 lines

These patches add strbuf_set operations which allow this common sequence to be performed in one line instead of two.
  strbuf_set(buf, cb.buf.buf, cb.buf.len);

This commit message is effectively the cover letter for the entire patch series; it doesn't say specifically what _this_ patch is doing.

Justification for the change could be stronger. Rather than merely pointing out that a sequence of operations occurs N times in the project, explain why strbuf_set() is superior. For instance, you might say something about how strbuf_set() conveys the operation being performed more concisely and clearly than strbuf_reset() + strbuf_add() (and thus may reduce cognitive load, though that's subjective). The bit about performing the operation in one line instead of two is minor, at best, and may not be worth mentioning at all (since it's implied).

It's also redundant to say "this patch" in the commit message, thus should be avoided.

More below.

Quoted 12 lines

Signed-off-by Jeremiah Mahler <[email protected]>

---
 Documentation/technical/api-strbuf.txt | 18 ++++++++++++++++++
 strbuf.c                               | 21 +++++++++++++++++++++
 strbuf.h                               | 14 ++++++++++++++
 3 files changed, 53 insertions(+)

Documentation/technical/api-strbuf.txt +0 -0

index 077a709..b7e23da 100644

Documentation/technical/api-strbuf.txt Functions · line 156

Quoted 8 lines

Documentation/technical/api-strbuf.txt +5 -0

@@ -149,6 +149,24 @@ Functions
        than zero if the first buffer is found, respectively, to be less than,
        to match, or be greater than the second buffer.
 
+* Setting the buffer
+
+`strbuf_set`::
+
+       Replace the buffer content with data of a given length.

I know that you copied the wording I suggested in my v1 review, but now that I see it in context, I find the redundancy level rather high. The header already says "Setting the buffer", so repeating "the buffer" in each function description doesn't add much. It might make sense to reword as:

Replace content with [...].

More below.

Quoted 52 lines

Documentation/technical/api-strbuf.txt +12 -0

+`strbuf_setstr`::
+
+       Replace the buffer content with data from a NUL-terminated string.
+
+`strbuf_setf`::
+
+       Replace the buffer content with a formatted string.
+
+`strbuf_setbuf`::
+
+       Replace the buffer content with data from another buffer.
+
 * Adding data to the buffer
 
 NOTE: All of the functions in this section will grow the buffer as necessary.

strbuf.c +21 -0

index ac62982..9d64b00 100644
@@ -189,6 +189,27 @@ void strbuf_splice(struct strbuf *sb, size_t pos, size_t len,
        strbuf_setlen(sb, sb->len + dlen - len);
 }
 
+void strbuf_set(struct strbuf *sb, const void *data, size_t len)
+{
+       strbuf_reset(sb);
+       strbuf_add(sb, data, len);
+}
+
+void strbuf_setf(struct strbuf *sb, const char *fmt, ...)
+{
+       va_list ap;
+       strbuf_reset(sb);
+       va_start(ap, fmt);
+       strbuf_vaddf(sb, fmt, ap);
+       va_end(ap);
+}
+
+void strbuf_setbuf(struct strbuf *sb, const struct strbuf *sb2)
+{
+       strbuf_reset(sb);
+       strbuf_add(sb, sb2->buf, sb2->len);
+}
+
 void strbuf_insert(struct strbuf *sb, size_t pos, const void *data, size_t len)
 {
        strbuf_splice(sb, pos, 0, data, len);

strbuf.h +0 -0

index e9ad03e..b339f08 100644

strbuf.h static inline struct strbuf **strbuf_split(const struct strbuf *sb, · line 105

Quoted 5 lines

strbuf.h +2 -0

@@ -101,6 +101,20 @@ static inline struct strbuf **strbuf_split(const struct strbuf *sb,
  */
 extern void strbuf_list_free(struct strbuf **);
 
+/*----- set buffer to data -----*/
+

Minor: Existing divider lines in this header are not followed by a blank line.

Quoted 16 lines

strbuf.h +12 -1

+extern void strbuf_set(struct strbuf *sb, const void *data, size_t len);
+
+static inline void strbuf_setstr(struct strbuf *sb, const char *s)
+{
+       strbuf_set(sb, s, strlen(s));
+}
+
+__attribute__((format (printf,2,3)))
+extern void strbuf_setf(struct strbuf *sb, const char *fmt, ...);
+
+extern void strbuf_setbuf(struct strbuf *sb, const struct strbuf *sb2);
+
 /*----- add data in your buffer -----*/
 static inline void strbuf_addch(struct strbuf *sb, int c)
 {
--

Quoted 1 line

2.0.0.592.gf55b190

Final word: The comments in this review do not necessarily require a re-roll. Junio may or may not want to pick up the series as is. If he doesn't, or if you want to polish it further, then perhaps take the review comments into consideration when rerolling.

Jeremiah Mahler

author

2016-06-15 23:01 UTC #7 lore raw

Quoted 25 lines · Eric Sunshine

Quoted 16 lines · Jeremiah Mahler
Currently, the data in a strbuf is modified using add operations. To set the buffer to some data a reset must be performed before an add.
  strbuf_reset(buf);
  strbuf_add(buf, cb.buf.buf, cb.buf.len);
And this is a common sequence of operations with 70 occurrences found in the current source code. This includes all the different variations (add, addf, addstr, addbuf, addch).
  FILES=`find ./ -name '*.c'`
  CNT=$(pcregrep -M "strbuf_reset.*\n.*strbuf_add" $FILES | wc -l)
  CNT=$(echo "$CNT / 2" | bc)
  echo $CNT
  70
Quoted 4 lines
These patches add strbuf_set operations which allow this common sequence to be performed in one line instead of two.
  strbuf_set(buf, cb.buf.buf, cb.buf.len);
This commit message is effectively the cover letter for the entire patch series; it doesn't say specifically what _this_ patch is doing.

Quoted 10 lines

Justification for the change could be stronger. Rather than merely pointing out that a sequence of operations occurs N times in the project, explain why strbuf_set() is superior. For instance, you might say something about how strbuf_set() conveys the operation being performed more concisely and clearly than strbuf_reset() + strbuf_add() (and thus may reduce cognitive load, though that's subjective). The bit about performing the operation in one line instead of two is minor, at best, and may not be worth mentioning at all (since it's implied).

After reviewing my argument, I think the biggest benefit is that it can make the code more readable in some cases.

Quoted 2 lines

It's also redundant to say "this patch" in the commit message, thus should be avoided.

Got it.

Quoted 30 lines

More below.
Quoted 12 lines
Signed-off-by Jeremiah Mahler <[email protected]>
---
 Documentation/technical/api-strbuf.txt | 18 ++++++++++++++++++
 strbuf.c                               | 21 +++++++++++++++++++++
 strbuf.h                               | 14 ++++++++++++++
 3 files changed, 53 insertions(+)
Documentation/technical/api-strbuf.txt +0 -0
index 077a709..b7e23da 100644
Documentation/technical/api-strbuf.txt Functions · line 156
Quoted 8 lines
Documentation/technical/api-strbuf.txt +5 -0
@@ -149,6 +149,24 @@ Functions
        than zero if the first buffer is found, respectively, to be less than,
        to match, or be greater than the second buffer.
 
+* Setting the buffer
+
+`strbuf_set`::
+
+       Replace the buffer content with data of a given length.
I know that you copied the wording I suggested in my v1 review, but now that I see it in context, I find the redundancy level rather high. The header already says "Setting the buffer", so repeating "the buffer" in each function description doesn't add much. It might make sense to reword as:

Quoted 2 lines

Replace content with [...].

Fixed.

Quoted 57 lines

More below.

Quoted 55 lines

Documentation/technical/api-strbuf.txt +12 -0

+`strbuf_setstr`::
+
+       Replace the buffer content with data from a NUL-terminated string.
+
+`strbuf_setf`::
+
+       Replace the buffer content with a formatted string.
+
+`strbuf_setbuf`::
+
+       Replace the buffer content with data from another buffer.
+
 * Adding data to the buffer
 
 NOTE: All of the functions in this section will grow the buffer as necessary.

strbuf.c +21 -0

index ac62982..9d64b00 100644
@@ -189,6 +189,27 @@ void strbuf_splice(struct strbuf *sb, size_t pos, size_t len,
        strbuf_setlen(sb, sb->len + dlen - len);
 }
 
+void strbuf_set(struct strbuf *sb, const void *data, size_t len)
+{
+       strbuf_reset(sb);
+       strbuf_add(sb, data, len);
+}
+
+void strbuf_setf(struct strbuf *sb, const char *fmt, ...)
+{
+       va_list ap;
+       strbuf_reset(sb);
+       va_start(ap, fmt);
+       strbuf_vaddf(sb, fmt, ap);
+       va_end(ap);
+}
+
+void strbuf_setbuf(struct strbuf *sb, const struct strbuf *sb2)
+{
+       strbuf_reset(sb);
+       strbuf_add(sb, sb2->buf, sb2->len);
+}
+
 void strbuf_insert(struct strbuf *sb, size_t pos, const void *data, size_t len)
 {
        strbuf_splice(sb, pos, 0, data, len);

strbuf.h +0 -0

index e9ad03e..b339f08 100644
@@ -101,6 +101,20 @@ static inline struct strbuf **strbuf_split(const struct strbuf *sb,
  */
 extern void strbuf_list_free(struct strbuf **);

Quoted 4 lines

strbuf.h static inline struct strbuf **strbuf_split(const struct strbuf *sb, · line 105
Quoted 2 lines
strbuf.h +2 -0
+/*----- set buffer to data -----*/
+
Minor: Existing divider lines in this header are not followed by a blank line.

Fixed.

Quoted 15 lines

strbuf.h +12 -0

+extern void strbuf_set(struct strbuf *sb, const void *data, size_t len);
+
+static inline void strbuf_setstr(struct strbuf *sb, const char *s)
+{
+       strbuf_set(sb, s, strlen(s));
+}
+
+__attribute__((format (printf,2,3)))
+extern void strbuf_setf(struct strbuf *sb, const char *fmt, ...);
+
+extern void strbuf_setbuf(struct strbuf *sb, const struct strbuf *sb2);
+
 /*----- add data in your buffer -----*/
 static inline void strbuf_addch(struct strbuf *sb, int c)

Quoted 8 lines

Quoted 3 lines
strbuf.h +0 -1
 {
--
2.0.0.592.gf55b190
Final word: The comments in this review do not necessarily require a re-roll. Junio may or may not want to pick up the series as is. If he doesn't, or if you want to polish it further, then perhaps take the review comments into consideration when rerolling.

They are certainly changes worth making.
Thanks again for your review :-)

Signature · 3 lines

Jeremiah Mahler
[email protected]
http://github.com/jmahler

Jeremiah Mahler

author

2016-06-15 23:01 UTC #8 lore raw

PATCH v2 2/19 sha1_name: simplify via strbuf_set()

In reply to Jeremiah Mahler

Simplify cases where a strbuf_reset is immediately followed by a strbuf_add by using strbuf_set operations.

Signed-off-by Jeremiah Mahler <[email protected]>

---
 sha1_name.c | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

sha1_name.c +5 ~~-10~~

index 2b6322f..f88b66c 100644
@@ -920,8 +920,7 @@ static int grab_nth_branch_switch(unsigned char *osha1, unsigned char *nsha1,
 		return 0;
 	if (--(cb->remaining) == 0) {
 		len = target - match;
-		strbuf_reset(&cb->buf);
-		strbuf_add(&cb->buf, match, len);
+		strbuf_set(&cb->buf, match, len);
 		return 1; /* we are done */
 	}
 	return 0;

@@ -957,8 +956,7 @@ static int interpret_nth_prior_checkout(const char *name, int namelen,
 
 	retval = 0;
 	if (0 < for_each_reflog_ent_reverse("HEAD", grab_nth_branch_switch, &cb)) {
-		strbuf_reset(buf);
-		strbuf_add(buf, cb.buf.buf, cb.buf.len);
+		strbuf_set(buf, cb.buf.buf, cb.buf.len);
 		retval = brace - name + 1;
 	}

@@ -1025,8 +1023,7 @@ static int interpret_empty_at(const char *name, int namelen, int len, struct str
 	if (next != name + 1)
 		return -1;
 
-	strbuf_reset(buf);
-	strbuf_add(buf, "HEAD", 4);
+	strbuf_set(buf, "HEAD", 4);
 	return 1;
 }

@@ -1044,8 +1041,7 @@ static int reinterpret(const char *name, int namelen, int len, struct strbuf *bu
 		strbuf_setlen(buf, used);
 		return len;
 	}
-	strbuf_reset(buf);
-	strbuf_addbuf(buf, &tmp);
+	strbuf_setbuf(buf, &tmp);
 	strbuf_release(&tmp);
 	/* tweak for size of {-N} versus expanded ref name */
 	return ret - used + len;

@@ -1054,8 +1050,7 @@ static int reinterpret(const char *name, int namelen, int len, struct strbuf *bu
 static void set_shortened_ref(struct strbuf *buf, const char *ref)
 {
 	char *s = shorten_unambiguous_ref(ref, 0);
-	strbuf_reset(buf);
-	strbuf_addstr(buf, s);
+	strbuf_setstr(buf, s);
 	free(s);
 }

Signature · 1 line

2.0.0.592.gf55b190

Jeremiah Mahler

author

2016-06-15 23:01 UTC #9 lore raw

PATCH v2 3/19 fast-import: simplify via strbuf_set()

In reply to Jeremiah Mahler

Simplify cases where a strbuf_reset is immediately followed by a strbuf_add by using strbuf_set operations.

Signed-off-by Jeremiah Mahler <[email protected]>

---
 fast-import.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

fast-import.c +2 -4

index e8ec34d..cfe9404 100644
@@ -2898,8 +2898,7 @@ static void cat_blob(struct object_entry *oe, unsigned char sha1[20])
 	 * Output based on batch_one_object() from cat-file.c.
 	 */
 	if (type <= 0) {
-		strbuf_reset(&line);
-		strbuf_addf(&line, "%s missing\n", sha1_to_hex(sha1));
+		strbuf_setf(&line, "%s missing\n", sha1_to_hex(sha1));
 		cat_blob_write(line.buf, line.len);
 		strbuf_release(&line);
 		free(buf);

@@ -2910,8 +2909,7 @@ static void cat_blob(struct object_entry *oe, unsigned char sha1[20])
 	if (type != OBJ_BLOB)
 		die("Object %s is a %s but a blob was expected.",
 		    sha1_to_hex(sha1), typename(type));
-	strbuf_reset(&line);
-	strbuf_addf(&line, "%s %s %lu\n", sha1_to_hex(sha1),
+	strbuf_setf(&line, "%s %s %lu\n", sha1_to_hex(sha1),
 						typename(type), size);
 	cat_blob_write(line.buf, line.len);
 	strbuf_release(&line);

Signature · 1 line

2.0.0.592.gf55b190

Jeremiah Mahler

author

2016-06-15 23:01 UTC #10 lore raw

PATCH v2 4/19 builtin/remote: simplify via strbuf_set()

In reply to Jeremiah Mahler

Simplify cases where a strbuf_reset is immediately followed by a strbuf_add by using strbuf_set operations.

Signed-off-by Jeremiah Mahler <[email protected]>

---
 builtin/remote.c | 59 ++++++++++++++++++++------------------------------------
 1 file changed, 21 insertions(+), 38 deletions(-)

builtin/remote.c +21 ~~-38~~

index c9b67ff..f2d809d 100644
@@ -193,8 +193,7 @@ static int add(int argc, const char **argv)
 		return 1;
 
 	if (!mirror || mirror & MIRROR_FETCH) {
-		strbuf_reset(&buf);
-		strbuf_addf(&buf, "remote.%s.fetch", name);
+		strbuf_setf(&buf, "remote.%s.fetch", name);
 		if (track.nr == 0)
 			string_list_append(&track, "*");
 		for (i = 0; i < track.nr; i++) {

@@ -205,15 +204,13 @@ static int add(int argc, const char **argv)
 	}
 
 	if (mirror & MIRROR_PUSH) {
-		strbuf_reset(&buf);
-		strbuf_addf(&buf, "remote.%s.mirror", name);
+		strbuf_setf(&buf, "remote.%s.mirror", name);
 		if (git_config_set(buf.buf, "true"))
 			return 1;
 	}
 
 	if (fetch_tags != TAGS_DEFAULT) {
-		strbuf_reset(&buf);
-		strbuf_addf(&buf, "remote.%s.tagopt", name);
+		strbuf_setf(&buf, "remote.%s.tagopt", name);
 		if (git_config_set(buf.buf,
 			fetch_tags == TAGS_SET ? "--tags" : "--no-tags"))
 			return 1;

@@ -223,11 +220,9 @@ static int add(int argc, const char **argv)
 		return 1;
 
 	if (master) {
-		strbuf_reset(&buf);
-		strbuf_addf(&buf, "refs/remotes/%s/HEAD", name);
+		strbuf_setf(&buf, "refs/remotes/%s/HEAD", name);
 
-		strbuf_reset(&buf2);
-		strbuf_addf(&buf2, "refs/remotes/%s/%s", name, master);
+		strbuf_setf(&buf2, "refs/remotes/%s/%s", name, master);
 
 		if (create_symref(buf.buf, buf2.buf, "remote add"))
 			return error(_("Could not setup master '%s'"), master);

@@ -584,19 +579,17 @@ static int migrate_file(struct remote *remote)
 	int i;
 	const char *path = NULL;
 
-	strbuf_addf(&buf, "remote.%s.url", remote->name);
+	strbuf_setf(&buf, "remote.%s.url", remote->name);
 	for (i = 0; i < remote->url_nr; i++)
 		if (git_config_set_multivar(buf.buf, remote->url[i], "^$", 0))
 			return error(_("Could not append '%s' to '%s'"),
 					remote->url[i], buf.buf);
-	strbuf_reset(&buf);
-	strbuf_addf(&buf, "remote.%s.push", remote->name);
+	strbuf_setf(&buf, "remote.%s.push", remote->name);
 	for (i = 0; i < remote->push_refspec_nr; i++)
 		if (git_config_set_multivar(buf.buf, remote->push_refspec[i], "^$", 0))
 			return error(_("Could not append '%s' to '%s'"),
 					remote->push_refspec[i], buf.buf);
-	strbuf_reset(&buf);
-	strbuf_addf(&buf, "remote.%s.fetch", remote->name);
+	strbuf_setf(&buf, "remote.%s.fetch", remote->name);
 	for (i = 0; i < remote->fetch_refspec_nr; i++)
 		if (git_config_set_multivar(buf.buf, remote->fetch_refspec[i], "^$", 0))
 			return error(_("Could not append '%s' to '%s'"),

@@ -640,27 +633,24 @@ static int mv(int argc, const char **argv)
 	if (newremote && (newremote->url_nr > 1 || newremote->fetch_refspec_nr))
 		die(_("remote %s already exists."), rename.new);
 
-	strbuf_addf(&buf, "refs/heads/test:refs/remotes/%s/test", rename.new);
+	strbuf_setf(&buf, "refs/heads/test:refs/remotes/%s/test", rename.new);
 	if (!valid_fetch_refspec(buf.buf))
 		die(_("'%s' is not a valid remote name"), rename.new);
 
-	strbuf_reset(&buf);
-	strbuf_addf(&buf, "remote.%s", rename.old);
-	strbuf_addf(&buf2, "remote.%s", rename.new);
+	strbuf_setf(&buf, "remote.%s", rename.old);
+	strbuf_setf(&buf2, "remote.%s", rename.new);
 	if (git_config_rename_section(buf.buf, buf2.buf) < 1)
 		return error(_("Could not rename config section '%s' to '%s'"),
 				buf.buf, buf2.buf);
 
-	strbuf_reset(&buf);
-	strbuf_addf(&buf, "remote.%s.fetch", rename.new);
+	strbuf_setf(&buf, "remote.%s.fetch", rename.new);
 	if (git_config_set_multivar(buf.buf, NULL, NULL, 1))
 		return error(_("Could not remove config section '%s'"), buf.buf);
-	strbuf_addf(&old_remote_context, ":refs/remotes/%s/", rename.old);
+	strbuf_setf(&old_remote_context, ":refs/remotes/%s/", rename.old);
 	for (i = 0; i < oldremote->fetch_refspec_nr; i++) {
 		char *ptr;
 
-		strbuf_reset(&buf2);
-		strbuf_addstr(&buf2, oldremote->fetch_refspec[i]);
+		strbuf_setstr(&buf2, oldremote->fetch_refspec[i]);
 		ptr = strstr(buf2.buf, old_remote_context.buf);
 		if (ptr) {
 			refspec_updated = 1;

@@ -683,8 +673,7 @@ static int mv(int argc, const char **argv)
 		struct string_list_item *item = branch_list.items + i;
 		struct branch_info *info = item->util;
 		if (info->remote_name && !strcmp(info->remote_name, rename.old)) {
-			strbuf_reset(&buf);
-			strbuf_addf(&buf, "branch.%s.remote", item->string);
+			strbuf_setf(&buf, "branch.%s.remote", item->string);
 			if (git_config_set(buf.buf, rename.new)) {
 				return error(_("Could not set '%s'"), buf.buf);
 			}

@@ -715,12 +704,10 @@ static int mv(int argc, const char **argv)
 
 		if (item->util)
 			continue;
-		strbuf_reset(&buf);
-		strbuf_addstr(&buf, item->string);
+		strbuf_setstr(&buf, item->string);
 		strbuf_splice(&buf, strlen("refs/remotes/"), strlen(rename.old),
 				rename.new, strlen(rename.new));
-		strbuf_reset(&buf2);
-		strbuf_addf(&buf2, "remote: renamed %s to %s",
+		strbuf_setf(&buf2, "remote: renamed %s to %s",
 				item->string, buf.buf);
 		if (rename_ref(item->string, buf.buf, buf2.buf))
 			die(_("renaming '%s' failed"), item->string);

@@ -730,16 +717,13 @@ static int mv(int argc, const char **argv)
 
 		if (!item->util)
 			continue;
-		strbuf_reset(&buf);
-		strbuf_addstr(&buf, item->string);
+		strbuf_setstr(&buf, item->string);
 		strbuf_splice(&buf, strlen("refs/remotes/"), strlen(rename.old),
 				rename.new, strlen(rename.new));
-		strbuf_reset(&buf2);
-		strbuf_addstr(&buf2, item->util);
+		strbuf_setstr(&buf2, item->util);
 		strbuf_splice(&buf2, strlen("refs/remotes/"), strlen(rename.old),
 				rename.new, strlen(rename.new));
-		strbuf_reset(&buf3);
-		strbuf_addf(&buf3, "remote: renamed %s to %s",
+		strbuf_setf(&buf3, "remote: renamed %s to %s",
 				item->string, buf.buf);
 		if (create_symref(buf.buf, buf2.buf, buf3.buf))
 			die(_("creating '%s' failed"), buf.buf);

@@ -804,8 +788,7 @@ static int rm(int argc, const char **argv)
 		if (info->remote_name && !strcmp(info->remote_name, remote->name)) {
 			const char *keys[] = { "remote", "merge", NULL }, **k;
 			for (k = keys; *k; k++) {
-				strbuf_reset(&buf);
-				strbuf_addf(&buf, "branch.%s.%s",
+				strbuf_setf(&buf, "branch.%s.%s",
 						item->string, *k);
 				if (git_config_set(buf.buf, NULL)) {
 					strbuf_release(&buf);

Signature · 1 line

2.0.0.592.gf55b190

Jeremiah Mahler

author

2016-06-15 23:01 UTC #11 lore raw

PATCH v2 5/19 branch: simplify via strbuf_set()

In reply to Jeremiah Mahler

Simplify cases where a strbuf_reset is immediately followed by a strbuf_add by using strbuf_set operations.

Signed-off-by Jeremiah Mahler <[email protected]>

---
 branch.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

branch.c +2 -4

index 660097b..bc7cc7e 100644
@@ -65,13 +65,11 @@ void install_branch_config(int flag, const char *local, const char *origin, cons
 	strbuf_addf(&key, "branch.%s.remote", local);
 	git_config_set(key.buf, origin ? origin : ".");
 
-	strbuf_reset(&key);
-	strbuf_addf(&key, "branch.%s.merge", local);
+	strbuf_setf(&key, "branch.%s.merge", local);
 	git_config_set(key.buf, remote);
 
 	if (rebasing) {
-		strbuf_reset(&key);
-		strbuf_addf(&key, "branch.%s.rebase", local);
+		strbuf_setf(&key, "branch.%s.rebase", local);
 		git_config_set(key.buf, "true");
 	}
 	strbuf_release(&key);

Signature · 1 line

2.0.0.592.gf55b190

Jeremiah Mahler

author

2016-06-15 23:01 UTC #12 lore raw

PATCH v2 6/19 builtin/branch: simplify via strbuf_set()

In reply to Jeremiah Mahler

Simplify cases where a strbuf_reset is immediately followed by a strbuf_add by using strbuf_set operations.

Signed-off-by Jeremiah Mahler <[email protected]>

---
 builtin/branch.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

builtin/branch.c +1 -2

index 2d1c57c..ad641b6 100644
@@ -984,8 +984,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 
 		strbuf_addf(&buf, "branch.%s.remote", branch->name);
 		git_config_set_multivar(buf.buf, NULL, NULL, 1);
-		strbuf_reset(&buf);
-		strbuf_addf(&buf, "branch.%s.merge", branch->name);
+		strbuf_setf(&buf, "branch.%s.merge", branch->name);
 		git_config_set_multivar(buf.buf, NULL, NULL, 1);
 		strbuf_release(&buf);
 	} else if (argc > 0 && argc <= 2) {

Signature · 1 line

2.0.0.592.gf55b190

Jeremiah Mahler

author

2016-06-15 23:01 UTC #13 lore raw

PATCH v2 7/19 builtin/checkout: simplify via strbuf_set()

In reply to Jeremiah Mahler

Simplify cases where a strbuf_reset is immediately followed by a strbuf_add by using strbuf_set operations.

Signed-off-by Jeremiah Mahler <[email protected]>

---
 builtin/checkout.c | 18 ++++++------------
 1 file changed, 6 insertions(+), 12 deletions(-)

builtin/checkout.c +6 ~~-12~~

index 9cbe7d1..38fc0ce 100644
@@ -912,8 +912,7 @@ static int prepare_linked_checkout(const struct checkout_opts *opts,
 			  sb_git.buf);
 	junk_work_tree = path;
 
-	strbuf_reset(&sb);
-	strbuf_addf(&sb, "%s/gitdir", sb_repo.buf);
+	strbuf_setf(&sb, "%s/gitdir", sb_repo.buf);
 	write_file(sb.buf, 1, "%s\n", real_path(sb_git.buf));
 	write_file(sb_git.buf, 1, "gitdir: %s/repos/%s\n",
 		   real_path(get_git_common_dir()), name);

@@ -923,11 +922,9 @@ static int prepare_linked_checkout(const struct checkout_opts *opts,
 	 * value would do because this value will be ignored and
 	 * replaced at the next (real) checkout.
 	 */
-	strbuf_reset(&sb);
-	strbuf_addf(&sb, "%s/HEAD", sb_repo.buf);
+	strbuf_setf(&sb, "%s/HEAD", sb_repo.buf);
 	write_file(sb.buf, 1, "%s\n", sha1_to_hex(new->commit->object.sha1));
-	strbuf_reset(&sb);
-	strbuf_addf(&sb, "%s/commondir", sb_repo.buf);
+	strbuf_setf(&sb, "%s/commondir", sb_repo.buf);
 	write_file(sb.buf, 1, "../..\n");
 
 	if (!opts->quiet)

@@ -942,8 +939,7 @@ static int prepare_linked_checkout(const struct checkout_opts *opts,
 	ret = run_command(&cp);
 	if (!ret)
 		is_junk = 0;
-	strbuf_reset(&sb);
-	strbuf_addf(&sb, "%s/locked", sb_repo.buf);
+	strbuf_setf(&sb, "%s/locked", sb_repo.buf);
 	unlink_or_warn(sb.buf);
 	strbuf_release(&sb);
 	strbuf_release(&sb_repo);

@@ -1048,8 +1044,7 @@ static void check_linked_checkouts(struct branch_info *new)
 		return;
 	}
 
-	strbuf_reset(&path);
-	strbuf_addf(&path, "%s/HEAD", get_git_common_dir());
+	strbuf_setf(&path, "%s/HEAD", get_git_common_dir());
 	/*
 	 * $GIT_COMMON_DIR/HEAD is practically outside
 	 * $GIT_DIR so resolve_ref_unsafe() won't work (it

@@ -1064,8 +1059,7 @@ static void check_linked_checkouts(struct branch_info *new)
 	while ((d = readdir(dir)) != NULL) {
 		if (!strcmp(d->d_name, ".") || !strcmp(d->d_name, ".."))
 			continue;
-		strbuf_reset(&path);
-		strbuf_addf(&path, "%s/repos/%s/HEAD",
+		strbuf_setf(&path, "%s/repos/%s/HEAD",
 			    get_git_common_dir(), d->d_name);
 		if (check_linked_checkout(new, d->d_name, path.buf))
 			break;

Signature · 1 line

2.0.0.592.gf55b190

Jeremiah Mahler

author

2016-06-15 23:01 UTC #14 lore raw

PATCH v2 8/19 builtin/mailinfo: simplify via strbuf_set()

In reply to Jeremiah Mahler

Simplify cases where a strbuf_reset is immediately followed by a strbuf_add by using strbuf_set operations.

Signed-off-by Jeremiah Mahler <[email protected]>

---
 builtin/mailinfo.c | 18 ++++++------------
 1 file changed, 6 insertions(+), 12 deletions(-)

builtin/mailinfo.c +6 ~~-12~~

index cf11c8d..596071e 100644
@@ -40,8 +40,7 @@ static void get_sane_name(struct strbuf *out, struct strbuf *name, struct strbuf
 		src = email;
 	else if (name == out)
 		return;
-	strbuf_reset(out);
-	strbuf_addbuf(out, src);
+	strbuf_setbuf(out, src);
 }
 
 static void parse_bogus_from(const struct strbuf *line)

@@ -62,11 +61,9 @@ static void parse_bogus_from(const struct strbuf *line)
 	if (!ket)
 		return;
 
-	strbuf_reset(&email);
-	strbuf_add(&email, bra + 1, ket - bra - 1);
+	strbuf_set(&email, bra + 1, ket - bra - 1);
 
-	strbuf_reset(&name);
-	strbuf_add(&name, line->buf, bra - line->buf);
+	strbuf_set(&name, line->buf, bra - line->buf);
 	strbuf_trim(&name);
 	get_sane_name(&name, &name, &email);
 }

@@ -108,8 +105,7 @@ static void handle_from(const struct strbuf *from)
 		at--;
 	}
 	el = strcspn(at, " \n\t\r\v\f>");
-	strbuf_reset(&email);
-	strbuf_add(&email, at, el);
+	strbuf_set(&email, at, el);
 	strbuf_remove(&f, at - f.buf, el + (at[el] ? 1 : 0));
 
 	/* The remainder is name.  It could be

@@ -567,8 +563,7 @@ static int decode_header_bq(struct strbuf *it)
 		in = ep + 2;
 	}
 	strbuf_addstr(&outbuf, in);
-	strbuf_reset(it);
-	strbuf_addbuf(it, &outbuf);
+	strbuf_setbuf(it, &outbuf);
 decode_header_bq_out:
 	strbuf_release(&outbuf);
 	strbuf_release(&charset_q);

@@ -602,8 +597,7 @@ static void decode_transfer_encoding(struct strbuf *line)
 	default:
 		return;
 	}
-	strbuf_reset(line);
-	strbuf_addbuf(line, ret);
+	strbuf_setbuf(line, ret);
 	strbuf_release(ret);
 	free(ret);
 }

Signature · 1 line

2.0.0.592.gf55b190

Jeremiah Mahler

author

2016-06-15 23:01 UTC #15 lore raw

PATCH v2 9/19 builtin/tag: simplify via strbuf_set()

In reply to Jeremiah Mahler

Simplify cases where a strbuf_reset is immediately followed by a strbuf_add by using strbuf_set operations.

Signed-off-by Jeremiah Mahler <[email protected]>

---
 builtin/tag.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

builtin/tag.c +1 -2

index c6e8a71..b545c21 100644
@@ -496,8 +496,7 @@ static int strbuf_check_tag_ref(struct strbuf *sb, const char *name)
 	if (name[0] == '-')
 		return -1;
 
-	strbuf_reset(sb);
-	strbuf_addf(sb, "refs/tags/%s", name);
+	strbuf_setf(sb, "refs/tags/%s", name);
 
 	return check_refname_format(sb->buf, 0);
 }

Signature · 1 line

2.0.0.592.gf55b190

Jeremiah Mahler

author

2016-06-15 23:01 UTC #16 lore raw

PATCH v2 10/19 date: simplify via strbuf_set()

In reply to Jeremiah Mahler

Simplify cases where a strbuf_reset is immediately followed by a strbuf_add by using strbuf_set operations.

Signed-off-by Jeremiah Mahler <[email protected]>

---
 date.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

date.c +1 -2

index 782de95..0b723a4 100644
@@ -166,8 +166,7 @@ const char *show_date(unsigned long time, int tz, enum date_mode mode)
 	static struct strbuf timebuf = STRBUF_INIT;
 
 	if (mode == DATE_RAW) {
-		strbuf_reset(&timebuf);
-		strbuf_addf(&timebuf, "%lu %+05d", time, tz);
+		strbuf_setf(&timebuf, "%lu %+05d", time, tz);
 		return timebuf.buf;
 	}

Signature · 1 line

2.0.0.592.gf55b190

Jeremiah Mahler

author

2016-06-15 23:01 UTC #17 lore raw

PATCH v2 11/19 diffcore-order: simplify via strbuf_set()

In reply to Jeremiah Mahler

Simplify cases where a strbuf_reset is immediately followed by a strbuf_add by using strbuf_set operations.

Signed-off-by Jeremiah Mahler <[email protected]>

---
 diffcore-order.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diffcore-order.c +1 -2

index 97dd3d0..5a93971 100644
@@ -63,8 +63,7 @@ static int match_order(const char *path)
 	static struct strbuf p = STRBUF_INIT;
 
 	for (i = 0; i < order_cnt; i++) {
-		strbuf_reset(&p);
-		strbuf_addstr(&p, path);
+		strbuf_setstr(&p, path);
 		while (p.buf[0]) {
 			char *cp;
 			if (!wildmatch(order[i], p.buf, 0, NULL))

Signature · 1 line

2.0.0.592.gf55b190

Jeremiah Mahler

author

2016-06-15 23:01 UTC #18 lore raw

PATCH v2 12/19 http-backend: simplify via strbuf_set()

In reply to Jeremiah Mahler

Simplify cases where a strbuf_reset is immediately followed by a strbuf_add by using strbuf_set operations.

Signed-off-by Jeremiah Mahler <[email protected]>

---
 http-backend.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

http-backend.c +2 -4

index d2c0a62..25c7435 100644
@@ -489,14 +489,12 @@ static void service_rpc(char *service_name)
 	struct rpc_service *svc = select_service(service_name);
 	struct strbuf buf = STRBUF_INIT;
 
-	strbuf_reset(&buf);
-	strbuf_addf(&buf, "application/x-git-%s-request", svc->name);
+	strbuf_setf(&buf, "application/x-git-%s-request", svc->name);
 	check_content_type(buf.buf);
 
 	hdr_nocache();
 
-	strbuf_reset(&buf);
-	strbuf_addf(&buf, "application/x-git-%s-result", svc->name);
+	strbuf_setf(&buf, "application/x-git-%s-result", svc->name);
 	hdr_str(content_type, buf.buf);
 
 	end_headers();

Signature · 1 line

2.0.0.592.gf55b190

Jeremiah Mahler

author

2016-06-15 23:01 UTC #19 lore raw

PATCH v2 13/19 http: simplify via strbuf_set()

In reply to Jeremiah Mahler

Simplify cases where a strbuf_reset is immediately followed by a strbuf_add by using strbuf_set operations.

Signed-off-by Jeremiah Mahler <[email protected]>

---
 http.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

http.c +1 -2

index 2b4f6a3..626fed7 100644
@@ -1098,8 +1098,7 @@ static int update_url_from_redirect(struct strbuf *base,
 	    strcmp(tail, got->buf + got->len - tail_len))
 		return 0; /* insane redirect scheme */
 
-	strbuf_reset(base);
-	strbuf_add(base, got->buf, got->len - tail_len);
+	strbuf_set(base, got->buf, got->len - tail_len);
 	return 1;
 }

Signature · 1 line

2.0.0.592.gf55b190

Jeremiah Mahler

author

2016-06-15 23:01 UTC #20 lore raw

PATCH v2 14/19 ident: simplify via strbuf_set()

In reply to Jeremiah Mahler

Simplify cases where a strbuf_reset is immediately followed by a strbuf_add by using strbuf_set operations.

Signed-off-by Jeremiah Mahler <[email protected]>

---
 ident.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

ident.c +2 -4

index 1d9b6e7..523e249 100644
@@ -397,8 +397,7 @@ int git_ident_config(const char *var, const char *value, void *data)
 	if (!strcmp(var, "user.name")) {
 		if (!value)
 			return config_error_nonbool(var);
-		strbuf_reset(&git_default_name);
-		strbuf_addstr(&git_default_name, value);
+		strbuf_setstr(&git_default_name, value);
 		committer_ident_explicitly_given |= IDENT_NAME_GIVEN;
 		author_ident_explicitly_given |= IDENT_NAME_GIVEN;
 		return 0;

@@ -407,8 +406,7 @@ int git_ident_config(const char *var, const char *value, void *data)
 	if (!strcmp(var, "user.email")) {
 		if (!value)
 			return config_error_nonbool(var);
-		strbuf_reset(&git_default_email);
-		strbuf_addstr(&git_default_email, value);
+		strbuf_setstr(&git_default_email, value);
 		committer_ident_explicitly_given |= IDENT_MAIL_GIVEN;
 		author_ident_explicitly_given |= IDENT_MAIL_GIVEN;
 		return 0;

Signature · 1 line

2.0.0.592.gf55b190

Jeremiah Mahler

author

2016-06-15 23:01 UTC #21 lore raw

PATCH v2 15/19 remote-curl: simplify via strbuf_set()

In reply to Jeremiah Mahler

Simplify cases where a strbuf_reset is immediately followed by a strbuf_add by using strbuf_set operations.

Signed-off-by Jeremiah Mahler <[email protected]>

---
 remote-curl.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

remote-curl.c +1 -2

index 4493b38..49d27f5 100644
@@ -288,8 +288,7 @@ static struct discovery* discover_refs(const char *service, int for_push)
 		 */
 		line = packet_read_line_buf(&last->buf, &last->len, NULL);
 
-		strbuf_reset(&exp);
-		strbuf_addf(&exp, "# service=%s", service);
+		strbuf_setf(&exp, "# service=%s", service);
 		if (strcmp(line, exp.buf))
 			die("invalid server response; got '%s'", line);
 		strbuf_release(&exp);

Signature · 1 line

2.0.0.592.gf55b190

Jeremiah Mahler

author

2016-06-15 23:01 UTC #22 lore raw

PATCH v2 16/19 submodule: simplify via strbuf_set()

In reply to Jeremiah Mahler

Simplify cases where a strbuf_reset is immediately followed by a strbuf_add by using strbuf_set operations.

Signed-off-by Jeremiah Mahler <[email protected]>

---
 submodule.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

submodule.c +2 -3

index 3402af6..878cc48 100644
@@ -1324,13 +1324,12 @@ void connect_work_tree_and_git_dir(const char *work_tree, const char *git_dir)
 	const char *real_work_tree = xstrdup(real_path(work_tree));
 
 	/* Update gitfile */
-	strbuf_addf(&file_name, "%s/.git", work_tree);
+	strbuf_setf(&file_name, "%s/.git", work_tree);
 	write_file(file_name.buf, 1, "gitdir: %s\n",
 		   relative_path(real_git_dir, real_work_tree, &rel_path));
 
 	/* Update core.worktree setting */
-	strbuf_reset(&file_name);
-	strbuf_addf(&file_name, "%s/config", real_git_dir);
+	strbuf_setf(&file_name, "%s/config", real_git_dir);
 	if (git_config_set_in_file(file_name.buf, "core.worktree",
 				   relative_path(real_work_tree, real_git_dir,
 						 &rel_path)))

Signature · 1 line

2.0.0.592.gf55b190

Jeremiah Mahler

author

2016-06-15 23:01 UTC #23 lore raw

PATCH v2 17/19 transport: simplify via strbuf_set()

In reply to Jeremiah Mahler

Simplify cases where a strbuf_reset is immediately followed by a strbuf_add by using strbuf_set operations.

Signed-off-by Jeremiah Mahler <[email protected]>

---
 transport.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

transport.c +1 -2

index 172b3d8..e8f5dfa 100644
@@ -1092,8 +1092,7 @@ static int run_pre_push_hook(struct transport *transport,
 		if (r->status == REF_STATUS_REJECT_STALE) continue;
 		if (r->status == REF_STATUS_UPTODATE) continue;
 
-		strbuf_reset(&buf);
-		strbuf_addf( &buf, "%s %s %s %s\n",
+		strbuf_setf( &buf, "%s %s %s %s\n",
 			 r->peer_ref->name, sha1_to_hex(r->new_sha1),
 			 r->name, sha1_to_hex(r->old_sha1));

Signature · 1 line

2.0.0.592.gf55b190

Jeremiah Mahler

author

2016-06-15 23:01 UTC #24 lore raw

PATCH v2 18/19 vcs-svn/svndump: simplify via strbuf_set()

In reply to Jeremiah Mahler

Simplify cases where a strbuf_reset is immediately followed by a strbuf_add by using strbuf_set operations.

Signed-off-by Jeremiah Mahler <[email protected]>

---
 vcs-svn/svndump.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

vcs-svn/svndump.c +1 -2

index 31d1d83..7fbf5d8 100644
@@ -352,8 +352,7 @@ void svndump_read(const char *url, const char *local_ref, const char *notes_ref)
 		case sizeof("UUID"):
 			if (constcmp(t, "UUID"))
 				continue;
-			strbuf_reset(&dump_ctx.uuid);
-			strbuf_addstr(&dump_ctx.uuid, val);
+			strbuf_setstr(&dump_ctx.uuid, val);
 			break;
 		case sizeof("Revision-number"):
 			if (constcmp(t, "Revision-number"))

Signature · 1 line

2.0.0.592.gf55b190

Jeremiah Mahler

author

2016-06-15 23:01 UTC #25 lore raw

PATCH v2 19/19 wt-status: simplify via strbuf_set()

In reply to Jeremiah Mahler

Simplify cases where a strbuf_reset is immediately followed by a strbuf_add by using strbuf_set operations.

Signed-off-by Jeremiah Mahler <[email protected]>

---
 wt-status.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

wt-status.c +1 -2

index 318a191..a89cd73 100644
@@ -1168,8 +1168,7 @@ static char *read_and_strip_branch(const char *path)
 	else if (!get_sha1_hex(sb.buf, sha1)) {
 		const char *abbrev;
 		abbrev = find_unique_abbrev(sha1, DEFAULT_ABBREV);
-		strbuf_reset(&sb);
-		strbuf_addstr(&sb, abbrev);
+		strbuf_setstr(&sb, abbrev);
 	} else if (!strcmp(sb.buf, "detached HEAD")) /* rebase */
 		goto got_nothing;
 	else			/* bisect */

Signature · 1 line

2.0.0.592.gf55b190

Eric Sunshine

2016-06-15 23:01 UTC #26 lore raw

In reply to Jeremiah Mahler

Quoted 14 lines · Jeremiah Mahler

Version 2 of the patch set to add strbuf_set operations.
Includes suggestions from Eric Sunshine [1]:
[...snip...]
Using strbuf_set before a sequence of adds can be confusing. But using strbuf_add when nothing important was being added to can also be confusing. strbuf_set can be used to make these cases more clear.
  struct strbuf mybuf = STRBUF_INIT;
strbuf_add(&mybuf, ...); /* Was something there before? */

Quoted 7 lines

strbuf_set(&mybuf, ...); /* Replace everything. */
Several single line replacements were made for this reason.
Additional files have also been converted compared to version 1 [1].
[1]: http://marc.info/?l=git&m=140230874124060&w=2

Food for thought: This patch series has now likely entered the territory of unnecessary code churn. Quoting from Documentation/CodingGuidelines:

Fixing style violations while working on a real change as a preparatory clean-up step is good, but otherwise avoid useless code churn for the sake of conforming to the style.

"Once it _is_ in the tree, it's not really worth the patch noise to go and fix it up."
Cf. http://article.gmane.org/gmane.linux.kernel/943020

Rewriting code merely for the sake of replacing strbuf_reset() + strbuf_add() with strbuf_set(), while not exactly a style cleanup, falls into the same camp. (The patch which introduces strbuf_set(), however, is not churn.) Each change you make can potentially conflict with other topics already in-flight (see [1], for example), thus making more work for Junio.

Also, these sorts of apparently innocuous "mechanical" changes can have hidden costs [2], so it's useful to weigh carefully how lengthy you want to make this patch series.

Having said that, patch 4/19, particularly the changes to builtin/remote.c:mv(), in which multiple strbufs are reused repeatedly, does seem to make the code a bit easier to read and likely reduces cognitive load.

[1]: http://thread.gmane.org/gmane.comp.version-control.git/245133/focus=245147
[2]: http://thread.gmane.org/gmane.comp.version-control.git/245133/focus=245144

Quoted 46 lines

Jeremiah Mahler (19):
add strbuf_set operations
sha1_name: simplify via strbuf_set()
fast-import: simplify via strbuf_set()
builtin/remote: simplify via strbuf_set()
branch: simplify via strbuf_set()
builtin/branch: simplify via strbuf_set()
builtin/checkout: simplify via strbuf_set()
builtin/mailinfo: simplify via strbuf_set()
builtin/tag: simplify via strbuf_set()
date: simplify via strbuf_set()
diffcore-order: simplify via strbuf_set()
http-backend: simplify via strbuf_set()
http: simplify via strbuf_set()
ident: simplify via strbuf_set()
remote-curl: simplify via strbuf_set()
submodule: simplify via strbuf_set()
transport: simplify via strbuf_set()
vcs-svn/svndump: simplify via strbuf_set()
wt-status: simplify via strbuf_set()
 Documentation/technical/api-strbuf.txt | 18 +++++++++++
 branch.c                               |  6 ++--
 builtin/branch.c                       |  3 +-
 builtin/checkout.c                     | 18 ++++-------
 builtin/mailinfo.c                     | 18 ++++-------
 builtin/remote.c                       | 59 ++++++++++++----------------------
 builtin/tag.c                          |  3 +-
 date.c                                 |  3 +-
 diffcore-order.c                       |  3 +-
 fast-import.c                          |  6 ++--
 http-backend.c                         |  6 ++--
 http.c                                 |  3 +-
 ident.c                                |  6 ++--
 remote-curl.c                          |  3 +-
 sha1_name.c                            | 15 +++------
 strbuf.c                               | 21 ++++++++++++
 strbuf.h                               | 14 ++++++++
 submodule.c                            |  5 ++-
 transport.c                            |  3 +-
 vcs-svn/svndump.c                      |  3 +-
 wt-status.c                            |  3 +-
 21 files changed, 110 insertions(+), 109 deletions(-)
--
2.0.0.592.gf55b190

Jeremiah Mahler

author

2016-06-15 23:01 UTC #27 lore raw

Eric,

Quoted 7 lines · Eric Sunshine

Quoted 6 lines · Jeremiah Mahler
Version 2 of the patch set to add strbuf_set operations.
Includes suggestions from Eric Sunshine [1]:
[...snip...]

...

Quoted 24 lines

Food for thought: This patch series has now likely entered the territory of unnecessary code churn. Quoting from Documentation/CodingGuidelines:
Fixing style violations while working on a real change as a preparatory clean-up step is good, but otherwise avoid useless code churn for the sake of conforming to the style.
"Once it _is_ in the tree, it's not really worth the patch noise to go and fix it up."
Cf. http://article.gmane.org/gmane.linux.kernel/943020
Rewriting code merely for the sake of replacing strbuf_reset() + strbuf_add() with strbuf_set(), while not exactly a style cleanup, falls into the same camp. (The patch which introduces strbuf_set(), however, is not churn.) Each change you make can potentially conflict with other topics already in-flight (see [1], for example), thus making more work for Junio.
Also, these sorts of apparently innocuous "mechanical" changes can have hidden costs [2], so it's useful to weigh carefully how lengthy you want to make this patch series.

Quoted 8 lines

Having said that, patch 4/19, particularly the changes to builtin/remote.c:mv(), in which multiple strbufs are reused repeatedly, does seem to make the code a bit easier to read and likely reduces cognitive load.
[1]: http://thread.gmane.org/gmane.comp.version-control.git/245133/focus=245147
[2]: http://thread.gmane.org/gmane.comp.version-control.git/245133/focus=245144

I did not realize this "code churn" aspect at the time but it makes sense now. These "mechanical" changes can be more trouble than they are worth.

I will try to re-focus my changes to only those that have a substantial benefit.

Signature · 3 lines

Jeremiah Mahler
[email protected]
http://github.com/jmahler