threads / rfc / 36871

RFC patch, 5 partsadd strbuf_set operations

Subject: [PATCH/RFC v1 0/5] add strbuf_set operations

## tl;dr

12 messages between Jun 9, 2014 and Jun 9, 2014. Diffs are folded; open one to read it.

replies: 11people: 3as markdown or json

Jeremiah Mahler· Jun 9, 2014, 08:36 UTC · lore
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);
Only the first few files have been converted in this preliminary patch set.
Jeremiah Mahler (5):
  add strbuf_set operations
  add strbuf_set operations documentation
  sha1_name.c: cleanup using strbuf_set operations
  fast-import.c: cleanup using strbuf_set operations
  builtin/remote.c: cleanup using strbuf_set operations
 Documentation/technical/api-strbuf.txt | 18 ++++++++++++
 builtin/remote.c                       | 51 ++++++++++++----------------------
 fast-import.c                          | 19 ++++---------
 sha1_name.c                            | 15 ++++------
 strbuf.c                               | 21 ++++++++++++++
 strbuf.h                               | 14 ++++++++++
 6 files changed, 81 insertions(+), 57 deletions(-)
-- 
2.0.0.573.ged771ce.dirty
Jeremiah Mahler· Jun 9, 2014, 08:36 UTC · re: Jeremiah Mahler · lore

[PATCH/RFC v1 1/5] 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 <jmmahler@gmail.com>
---
 strbuf.c | 21 +++++++++++++++++++++
 strbuf.h | 14 ++++++++++++++
 2 files changed, 35 insertions(+)
Show changes to 2 files +35 −0

strbuf.c, strbuf.h

diff --git a/strbuf.c b/strbuf.c
index ac62982..9d64b00 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -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);
diff --git a/strbuf.h b/strbuf.h
index e9ad03e..b339f08 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -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)
 {

-- 
2.0.0.573.ged771ce.dirty
Jeremiah Mahler· Jun 9, 2014, 08:36 UTC · re: Jeremiah Mahler · lore

[PATCH/RFC v1 2/5] add strbuf_set operations documentation

Add documentation of the strbuf_set operations to technical/api-strbuf.txt.

Signed-off-by: Jeremiah Mahler <jmmahler@gmail.com>
---
 Documentation/technical/api-strbuf.txt | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)
Show changes to Documentation/technical/api-strbuf.txt +18 −0
diff --git a/Documentation/technical/api-strbuf.txt b/Documentation/technical/api-strbuf.txt
index 077a709..ab430d9 100644
--- a/Documentation/technical/api-strbuf.txt
+++ b/Documentation/technical/api-strbuf.txt
@@ -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`::
+
+    Set the buffer to some data up to a given length.
+
+`strbuf_setstr`::
+
+	Set the buffer to a NUL-terminated string.
+
+`strbuf_setf`::
+
+	Set the buffer to a formatted string.
+
+`strbuf_setbuf`::
+
+	Set the current buffer to the contents of some other buffer.
+
 * Adding data to the buffer
 
 NOTE: All of the functions in this section will grow the buffer as necessary.

-- 
2.0.0.573.ged771ce.dirty
Eric Sunshine· Jun 9, 2014, 09:53 UTC · re: Jeremiah Mahler · lore

Re: [PATCH/RFC v1 2/5] add strbuf_set operations documentation

On Mon, Jun 9, 2014 at 4:36 AM, Jeremiah Mahler <jmmahler@gmail.com> wrote:
> Add documentation of the strbuf_set operations to
> technical/api-strbuf.txt.

Since this patch is concise and so closely related to patch 1/5, it probably should be squashed into that one.

More below.
Show 18 quoted lines
> Signed-off-by: Jeremiah Mahler <jmmahler@gmail.com>
> ---
>  Documentation/technical/api-strbuf.txt | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
>
> diff --git a/Documentation/technical/api-strbuf.txt b/Documentation/technical/api-strbuf.txt
> index 077a709..ab430d9 100644
> --- a/Documentation/technical/api-strbuf.txt
> +++ b/Documentation/technical/api-strbuf.txt
> @@ -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`::
> +
> +    Set the buffer to some data up to a given length.

I personally find this slightly ambiguous. Upon reading it, the first question that pops into my mind is whether or not the existing strbuf content is replaced (even though "set" should imply that it is). I wonder if it would make sense to rewrite as:

    Set the buffer to [...], replacing the old content
    of the buffer.
Alternately:
    Replace the buffer content with [...].
Ditto for the others.
Show 17 quoted lines
> +`strbuf_setstr`::
> +
> +       Set the buffer to a NUL-terminated string.
> +
> +`strbuf_setf`::
> +
> +       Set the buffer to a formatted string.
> +
> +`strbuf_setbuf`::
> +
> +       Set the current buffer to the contents of some other buffer.
> +
>  * Adding data to the buffer
>
>  NOTE: All of the functions in this section will grow the buffer as necessary.
> --
> 2.0.0.573.ged771ce.dirty
Jeremiah Mahler· Jun 9, 2014, 21:49 UTC · re: Eric Sunshine · lore

Re: [PATCH/RFC v1 2/5] add strbuf_set operations documentation

Eric,
On Mon, Jun 09, 2014 at 05:53:49AM -0400, Eric Sunshine wrote:
Show 7 quoted lines
> On Mon, Jun 9, 2014 at 4:36 AM, Jeremiah Mahler <jmmahler@gmail.com> wrote:
> > Add documentation of the strbuf_set operations to
> > technical/api-strbuf.txt.
> 
> Since this patch is concise and so closely related to patch 1/5, it
> probably should be squashed into that one.
> 
Fixed.
Show 33 quoted lines
> More below.
> 
> > Signed-off-by: Jeremiah Mahler <jmmahler@gmail.com>
> > ---
> >  Documentation/technical/api-strbuf.txt | 18 ++++++++++++++++++
> >  1 file changed, 18 insertions(+)
> >
> > diff --git a/Documentation/technical/api-strbuf.txt b/Documentation/technical/api-strbuf.txt
> > index 077a709..ab430d9 100644
> > --- a/Documentation/technical/api-strbuf.txt
> > +++ b/Documentation/technical/api-strbuf.txt
> > @@ -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`::
> > +
> > +    Set the buffer to some data up to a given length.
> 
> I personally find this slightly ambiguous. Upon reading it, the first
> question that pops into my mind is whether or not the existing strbuf
> content is replaced (even though "set" should imply that it is). I
> wonder if it would make sense to rewrite as:
> 
>     Set the buffer to [...], replacing the old content
>     of the buffer.
> 
> Alternately:
> 
>     Replace the buffer content with [...].
>

On a second reading, I agree that it is ambigous. 'Replace' is much more clear. Great suggestion.

Show 18 quoted lines
> Ditto for the others.
> 
> > +`strbuf_setstr`::
> > +
> > +       Set the buffer to a NUL-terminated string.
> > +
> > +`strbuf_setf`::
> > +
> > +       Set the buffer to a formatted string.
> > +
> > +`strbuf_setbuf`::
> > +
> > +       Set the current buffer to the contents of some other buffer.
> > +
> >  * Adding data to the buffer
> >
> >  NOTE: All of the functions in this section will grow the buffer as necessary.
> > --
-- 
Jeremiah Mahler
jmmahler@gmail.com
http://github.com/jmahler
Jeremiah Mahler· Jun 9, 2014, 08:36 UTC · re: Jeremiah Mahler · lore

[PATCH/RFC v1 3/5] sha1_name.c: cleanup using strbuf_set operations

Simplified cases where a strbuf_reset was immediately followed by a strbuf_add using the new strbuf_set operations.

Signed-off-by: Jeremiah Mahler <jmmahler@gmail.com>
---
 sha1_name.c | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)
Show changes to sha1_name.c +5 −10
diff --git a/sha1_name.c b/sha1_name.c
index 2b6322f..f88b66c 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -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);
 }
 

-- 
2.0.0.573.ged771ce.dirty
Jeremiah Mahler· Jun 9, 2014, 08:36 UTC · re: Jeremiah Mahler · lore

[PATCH/RFC v1 4/5] fast-import.c: cleanup using strbuf_set operations

Simplified cases where a strbuf_reset was immediately followed by a strbuf_add using the new strbuf_set operations.

Signed-off-by: Jeremiah Mahler <jmmahler@gmail.com>
---
 fast-import.c | 19 ++++++-------------
 1 file changed, 6 insertions(+), 13 deletions(-)
Show changes to fast-import.c +6 −13
diff --git a/fast-import.c b/fast-import.c
index e8ec34d..c23935c 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -2741,8 +2741,7 @@ static void parse_new_commit(void)
 	hashcpy(b->branch_tree.versions[0].sha1,
 		b->branch_tree.versions[1].sha1);
 
-	strbuf_reset(&new_data);
-	strbuf_addf(&new_data, "tree %s\n",
+	strbuf_setf(&new_data, "tree %s\n",
 		sha1_to_hex(b->branch_tree.versions[1].sha1));
 	if (!is_null_sha1(b->sha1))
 		strbuf_addf(&new_data, "parent %s\n", sha1_to_hex(b->sha1));
@@ -2829,9 +2828,7 @@ static void parse_new_tag(void)
 	parse_data(&msg, 0, NULL);
 
 	/* build the tag object */
-	strbuf_reset(&new_data);
-
-	strbuf_addf(&new_data,
+	strbuf_setf(&new_data,
 		    "object %s\n"
 		    "type %s\n"
 		    "tag %s\n",
@@ -2898,8 +2895,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 +2906,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);
@@ -3034,14 +3029,12 @@ static void print_ls(int mode, const unsigned char *sha1, const char *path)
 
 	if (!mode) {
 		/* missing SP path LF */
-		strbuf_reset(&line);
-		strbuf_addstr(&line, "missing ");
+		strbuf_setstr(&line, "missing ");
 		quote_c_style(path, &line, NULL, 0);
 		strbuf_addch(&line, '\n');
 	} else {
 		/* mode SP type SP object_name TAB path LF */
-		strbuf_reset(&line);
-		strbuf_addf(&line, "%06o %s %s\t",
+		strbuf_setf(&line, "%06o %s %s\t",
 				mode & ~NO_DELTA, type, sha1_to_hex(sha1));
 		quote_c_style(path, &line, NULL, 0);
 		strbuf_addch(&line, '\n');

-- 
2.0.0.573.ged771ce.dirty
Eric Sunshine· Jun 9, 2014, 10:12 UTC · re: Jeremiah Mahler · lore

Re: [PATCH/RFC v1 4/5] fast-import.c: cleanup using strbuf_set operations

On Mon, Jun 9, 2014 at 4:36 AM, Jeremiah Mahler <jmmahler@gmail.com> wrote:
> Subject: fast-import.c: cleanup using strbuf_set operations
This might read more clearly if written:
    fast-import: simplify via strbuf_set()
> Simplified cases where a strbuf_reset was immediately followed by a
> strbuf_add using the new strbuf_set operations.
On this project, commit messages are written in imperative mood:
    Simplify cases where ... is immediately ...
More below.
Show 19 quoted lines
> Signed-off-by: Jeremiah Mahler <jmmahler@gmail.com>
> ---
>  fast-import.c | 19 ++++++-------------
>  1 file changed, 6 insertions(+), 13 deletions(-)
>
> diff --git a/fast-import.c b/fast-import.c
> index e8ec34d..c23935c 100644
> --- a/fast-import.c
> +++ b/fast-import.c
> @@ -2741,8 +2741,7 @@ static void parse_new_commit(void)
>         hashcpy(b->branch_tree.versions[0].sha1,
>                 b->branch_tree.versions[1].sha1);
>
> -       strbuf_reset(&new_data);
> -       strbuf_addf(&new_data, "tree %s\n",
> +       strbuf_setf(&new_data, "tree %s\n",
>                 sha1_to_hex(b->branch_tree.versions[1].sha1));
>         if (!is_null_sha1(b->sha1))
>                 strbuf_addf(&new_data, "parent %s\n", sha1_to_hex(b->sha1));

Unlike the cases in patches 3/5 and 5/5 where the strbuf is used or returned immediately following the strbuf_set() call, I am not convinced that this change is an improvement. This code has the general form:

    strbuf_reset(...);
    strbuf_add(...);
    if (condition)
        strbuf_add(...);
    strbuf_add(...);

in which it is clear that the string is being built piecemeal, and it's easy for a programmer to insert, remove, or re-order strbuf_add() calls.

Replacing the first two lines with strbuf_set() somewhat obscures the fact that the string is going to be built up piecemeal. Plus, the change makes it more difficult to insert, remove, or re-order the strbuf_add() invocations.

This isn't a strong objection, but the benefit of the change seems minimal or non-existent.

Ditto for several remaining cases in this patch.
Show 50 quoted lines
> @@ -2829,9 +2828,7 @@ static void parse_new_tag(void)
>         parse_data(&msg, 0, NULL);
>
>         /* build the tag object */
> -       strbuf_reset(&new_data);
> -
> -       strbuf_addf(&new_data,
> +       strbuf_setf(&new_data,
>                     "object %s\n"
>                     "type %s\n"
>                     "tag %s\n",
> @@ -2898,8 +2895,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 +2906,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);
> @@ -3034,14 +3029,12 @@ static void print_ls(int mode, const unsigned char *sha1, const char *path)
>
>         if (!mode) {
>                 /* missing SP path LF */
> -               strbuf_reset(&line);
> -               strbuf_addstr(&line, "missing ");
> +               strbuf_setstr(&line, "missing ");
>                 quote_c_style(path, &line, NULL, 0);
>                 strbuf_addch(&line, '\n');
>         } else {
>                 /* mode SP type SP object_name TAB path LF */
> -               strbuf_reset(&line);
> -               strbuf_addf(&line, "%06o %s %s\t",
> +               strbuf_setf(&line, "%06o %s %s\t",
>                                 mode & ~NO_DELTA, type, sha1_to_hex(sha1));
>                 quote_c_style(path, &line, NULL, 0);
>                 strbuf_addch(&line, '\n');
> --
> 2.0.0.573.ged771ce.dirty
Jeremiah Mahler· Jun 9, 2014, 22:00 UTC · re: Eric Sunshine · lore

Re: [PATCH/RFC v1 4/5] fast-import.c: cleanup using strbuf_set operations

Eric,
On Mon, Jun 09, 2014 at 06:12:12AM -0400, Eric Sunshine wrote:
> On Mon, Jun 9, 2014 at 4:36 AM, Jeremiah Mahler <jmmahler@gmail.com> wrote:
> > Subject: fast-import.c: cleanup using strbuf_set operations
> 
...
Show 46 quoted lines
> 
> > Signed-off-by: Jeremiah Mahler <jmmahler@gmail.com>
> > ---
> >  fast-import.c | 19 ++++++-------------
> >  1 file changed, 6 insertions(+), 13 deletions(-)
> >
> > diff --git a/fast-import.c b/fast-import.c
> > index e8ec34d..c23935c 100644
> > --- a/fast-import.c
> > +++ b/fast-import.c
> > @@ -2741,8 +2741,7 @@ static void parse_new_commit(void)
> >         hashcpy(b->branch_tree.versions[0].sha1,
> >                 b->branch_tree.versions[1].sha1);
> >
> > -       strbuf_reset(&new_data);
> > -       strbuf_addf(&new_data, "tree %s\n",
> > +       strbuf_setf(&new_data, "tree %s\n",
> >                 sha1_to_hex(b->branch_tree.versions[1].sha1));
> >         if (!is_null_sha1(b->sha1))
> >                 strbuf_addf(&new_data, "parent %s\n", sha1_to_hex(b->sha1));
> 
> Unlike the cases in patches 3/5 and 5/5 where the strbuf is used or
> returned immediately following the strbuf_set() call, I am not
> convinced that this change is an improvement. This code has the
> general form:
> 
>     strbuf_reset(...);
>     strbuf_add(...);
>     if (condition)
>         strbuf_add(...);
>     strbuf_add(...);
> 
> in which it is clear that the string is being built piecemeal, and
> it's easy for a programmer to insert, remove, or re-order strbuf_add()
> calls.
> 
> Replacing the first two lines with strbuf_set() somewhat obscures the
> fact that the string is going to be built up piecemeal. Plus, the
> change makes it more difficult to insert, remove, or re-order the
> strbuf_add() invocations.
> 
> This isn't a strong objection, but the benefit of the change seems
> minimal or non-existent.
> 
> Ditto for several remaining cases in this patch.
> 
...

This is a great observation that I certainly did overlook. Using strbuf_add or strbuf_set to help make it more obvious what the code is doing.

By the same token, strbuf_set can be used to replace strbuf_add to make it clear that nothing important was being added to and that the entire buffer is being replaced.

  struct strbuf mybuf = STRBUF_INIT;
  strbuf_add(&mybuf, ...);  /* Was something there before? */
  strbuf_set(&mybuf, ...);  /* Replace everything. */
-- 
Jeremiah Mahler
jmmahler@gmail.com
http://github.com/jmahler
Jeremiah Mahler· Jun 9, 2014, 08:36 UTC · re: Jeremiah Mahler · lore

[PATCH/RFC v1 5/5] builtin/remote.c: cleanup using strbuf_set operations

Simplified cases where a strbuf_reset was immediately followed by a strbuf_add using the new strbuf_set operations.

Signed-off-by: Jeremiah Mahler <jmmahler@gmail.com>
---
 builtin/remote.c | 51 +++++++++++++++++----------------------------------
 1 file changed, 17 insertions(+), 34 deletions(-)
Show changes to builtin/remote.c +17 −34
diff --git a/builtin/remote.c b/builtin/remote.c
index c9b67ff..b059852 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -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);
@@ -589,14 +584,12 @@ static int migrate_file(struct remote *remote)
 		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'"),
@@ -644,23 +637,20 @@ static int mv(int argc, const char **argv)
 	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_setf(&buf, "remote.%s", rename.old);
 	strbuf_addf(&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);
 	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);

-- 
2.0.0.573.ged771ce.dirty
Duy Nguyen· Jun 9, 2014, 10:39 UTC · re: Jeremiah Mahler · lore

Re: [PATCH/RFC v1 0/5] add strbuf_set operations

On Mon, Jun 9, 2014 at 3:36 PM, Jeremiah Mahler <jmmahler@gmail.com> wrote:
Show 12 quoted lines
> 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)

Hmm.. I wonder if git-grep could do this.. There's pcre support but I never tried.

>   CNT=$(echo "$CNT / 2" | bc)
>   echo $CNT
>   70

The change in this series looks nice. There's another pattern, save strbuf length, then strbuf_setlen() at the beginning or the end of a loop. But I think it's less often.

-- 
Duy
Jeremiah Mahler· Jun 9, 2014, 22:06 UTC · re: Duy Nguyen · lore

Re: [PATCH/RFC v1 0/5] add strbuf_set operations

Duy,
On Mon, Jun 09, 2014 at 05:39:12PM +0700, Duy Nguyen wrote:
Show 17 quoted lines
> On Mon, Jun 9, 2014 at 3:36 PM, Jeremiah Mahler <jmmahler@gmail.com> wrote:
> > 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)
> 
> Hmm.. I wonder if git-grep could do this.. There's pcre support but I
> never tried.
>

Not sure if git-grep does this. The multi-line (-M) support was the thing I needed.

Show 7 quoted lines
> >   CNT=$(echo "$CNT / 2" | bc)
> >   echo $CNT
> >   70
> 
> The change in this series looks nice. There's another pattern, save
> strbuf length, then strbuf_setlen() at the beginning or the end of a
> loop. But I think it's less often.

A quick look did not see any obvious patterns for this. I think you are right, there may be fewer cases.

> -- 
> Duy
-- 
Jeremiah Mahler
jmmahler@gmail.com
http://github.com/jmahler

← back to recent threads