[PATCH/RFC v1 0/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);

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

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 <redacted>
---
 strbuf.c | 21 +++++++++++++++++++++
 strbuf.h | 14 ++++++++++++++
 2 files changed, 35 insertions(+)

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

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

Signed-off-by: Jeremiah Mahler <redacted>
---
 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.
+
+`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

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

Signed-off-by: Jeremiah Mahler <redacted>
---
 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));

@@ -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

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

Signed-off-by: Jeremiah Mahler <redacted>
---
 sha1_name.c | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

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

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

Signed-off-by: Jeremiah Mahler <redacted>
---
 builtin/remote.c | 51 +++++++++++++++++----------------------------------
 1 file changed, 17 insertions(+), 34 deletions(-)

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

On Mon, Jun 9, 2014 at 4:36 AM, Jeremiah Mahler [off-list ref] 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.

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.

+`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

On Mon, Jun 9, 2014 at 4:36 AM, Jeremiah Mahler [off-list ref] 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.

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.

On Mon, Jun 9, 2014 at 3:36 PM, Jeremiah Mahler [off-list ref] 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.

  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

Eric,

On Mon, Jun 09, 2014 at 05:53:49AM -0400, Eric Sunshine wrote:

On Mon, Jun 9, 2014 at 4:36 AM, Jeremiah Mahler [off-list ref] wrote:

quoted

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.

More below.

quoted

Signed-off-by: Jeremiah Mahler <redacted>
---
 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.

Ditto for the others.

quoted

+`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

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 [off-list ref] wrote:

quoted

Subject: fast-import.c: cleanup using strbuf_set operations

...

quoted

Signed-off-by: Jeremiah Mahler <redacted>
---
 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

Duy,

On Mon, Jun 09, 2014 at 05:39:12PM +0700, Duy Nguyen wrote:

On Mon, Jun 9, 2014 at 3:36 PM, Jeremiah Mahler [off-list ref] wrote:

quoted

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.

quoted

  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