[PATCH v3 0/2] add strbuf_set operations

[PATCH v3 0/2] add strbuf_set operations

[Jeremiah Mahler]

Addition of strbuf_set operations, version 3.

Includes suggestions from Eric Sunshine [1]:

  - Revise log message to better argue why this patch is worthwhile.

  - Avoid documentation redundancy: "Setting buffer", "Replace buffer".

  - Remove unnecessary changes which didn't have a significant benefit
	to avoid unnecessary "code churn".  builtin/remote.c was the one
	file which showed a significant benefit.  Others with negligible
	benefits have been left as is.

The possible performance improvements using a strbuf_grow_to() operation
as suggested by Michael Haggerty [2] has been left for a later patch.

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

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

Jeremiah Mahler (2):
  add strbuf_set operations
  builtin/remote: improve readability via strbuf_set()

 Documentation/technical/api-strbuf.txt | 18 ++++++++++
 builtin/remote.c                       | 63 +++++++++++++---------------------
 strbuf.c                               | 21 ++++++++++++
 strbuf.h                               | 13 +++++++
 4 files changed, 75 insertions(+), 40 deletions(-)

-- 
2.0.0

[PATCH v3 1/2] add strbuf_set operations

[Jeremiah Mahler]

A common use case with strubfs is to set the buffer to a new value.
This must be done in two steps: a reset followed by an add.

  strbuf_reset(buf);
  strbuf_add(buf, new_buf, len);

In cases where the buffer is being built up in steps, these operations
make sense and correctly convey what is being performed.

  strbuf_reset(buf);
  strbuf_add(buf, data1, len1);
  strbuf_add(buf, data2, len2);
  strbuf_add(buf, data3, len3);

However, in other cases, it can be confusing and is not very concise.

  strbuf_reset(buf);
  strbuf_add(buf, default, len1);

  if (cond1) {
    strbuf_reset(buf);
    strbuf_add(buf, data2, len2);
  }

  if (cond2) {
    strbuf_reset(buf);
    strbuf_add(buf, data3, len3);
  }

Add strbuf_set operations so that it can be re-written in a clear and
concise way.

  strbuf_set(buf, default len1);

  if (cond1) {
    strbuf_set(buf, data2, len2);
  }

  if (cond2) {
    strbuf_set(buf, data3, len3);
  }

Signed-off-by: Jeremiah Mahler <jmmahler@gmail.com>
---
 Documentation/technical/api-strbuf.txt | 18 ++++++++++++++++++
 strbuf.c                               | 21 +++++++++++++++++++++
 strbuf.h                               | 13 +++++++++++++
 3 files changed, 52 insertions(+)

diff --git a/Documentation/technical/api-strbuf.txt b/Documentation/technical/api-strbuf.txt
index f9c06a7..ae9c9cc 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`::
+
+	Replace content with data of a given length.
+
+`strbuf_setstr`::
+
+	Replace content with data from a NUL-terminated string.
+
+`strbuf_setf`::
+
+	Replace content with a formatted string.
+
+`strbuf_setbuf`::
+
+	Replace 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.
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..5041c35 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -101,6 +101,19 @@ 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

[PATCH v3 2/2] builtin/remote: improve readability via strbuf_set()

[Jeremiah Mahler]

builtin/remote.c has many cases where a strbuf was being set to a new
value.  Improve its readability by using strbuf_set() operations instead
of reset + add.

Signed-off-by: Jeremiah Mahler <jmmahler@gmail.com>
---
 builtin/remote.c | 63 +++++++++++++++++++++-----------------------------------
 1 file changed, 23 insertions(+), 40 deletions(-)

diff --git a/builtin/remote.c b/builtin/remote.c
index c9b67ff..94f03e2 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -184,17 +184,16 @@ static int add(int argc, const char **argv)
 			remote->fetch_refspec_nr))
 		die(_("remote %s already exists."), name);
 
-	strbuf_addf(&buf2, "refs/heads/test:refs/remotes/%s/test", name);
+	strbuf_setf(&buf2, "refs/heads/test:refs/remotes/%s/test", name);
 	if (!valid_fetch_refspec(buf2.buf))
 		die(_("'%s' is not a valid remote name"), name);
 
-	strbuf_addf(&buf, "remote.%s.url", name);
+	strbuf_setf(&buf, "remote.%s.url", name);
 	if (git_config_set(buf.buf, url))
 		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);
-- 
2.0.0

Re: [PATCH v3 1/2] add strbuf_set operations

[Thomas Braun]

Am 12.06.2014 09:29, schrieb Jeremiah Mahler:
> A common use case with strubfs is to set the buffer to a new value.
> This must be done in two steps: a reset followed by an add.
> 
>   strbuf_reset(buf);
>   strbuf_add(buf, new_buf, len);
> 
> In cases where the buffer is being built up in steps, these operations
> make sense and correctly convey what is being performed.
> 
>   strbuf_reset(buf);
>   strbuf_add(buf, data1, len1);
>   strbuf_add(buf, data2, len2);
>   strbuf_add(buf, data3, len3);
> 
> However, in other cases, it can be confusing and is not very concise.
> 
>   strbuf_reset(buf);
>   strbuf_add(buf, default, len1);
> 
>   if (cond1) {
>     strbuf_reset(buf);
>     strbuf_add(buf, data2, len2);
>   }
> 
>   if (cond2) {
>     strbuf_reset(buf);
>     strbuf_add(buf, data3, len3);
>   }
> 
> Add strbuf_set operations so that it can be re-written in a clear and
> concise way.
> 
>   strbuf_set(buf, default len1);
very minor nit: missing comma between default and len1.

Re: [PATCH v3 2/2] builtin/remote: improve readability via strbuf_set()

[Eric Sunshine]

On Thu, Jun 12, 2014 at 3:29 AM, Jeremiah Mahler <jmmahler@gmail.com> wrote:
> builtin/remote.c has many cases where a strbuf was being set to a new
> value.  Improve its readability by using strbuf_set() operations instead
> of reset + add.
>
> Signed-off-by: Jeremiah Mahler <jmmahler@gmail.com>
> ---
>  builtin/remote.c | 63 +++++++++++++++++++++-----------------------------------
>  1 file changed, 23 insertions(+), 40 deletions(-)
>
> diff --git a/builtin/remote.c b/builtin/remote.c
> index c9b67ff..94f03e2 100644
> --- a/builtin/remote.c
> +++ b/builtin/remote.c
> @@ -184,17 +184,16 @@ static int add(int argc, const char **argv)
>                         remote->fetch_refspec_nr))
>                 die(_("remote %s already exists."), name);
>
> -       strbuf_addf(&buf2, "refs/heads/test:refs/remotes/%s/test", name);
> +       strbuf_setf(&buf2, "refs/heads/test:refs/remotes/%s/test", name);
>         if (!valid_fetch_refspec(buf2.buf))
>                 die(_("'%s' is not a valid remote name"), name);
>
> -       strbuf_addf(&buf, "remote.%s.url", name);
> +       strbuf_setf(&buf, "remote.%s.url", name);
>         if (git_config_set(buf.buf, url))
>                 return 1;
>
>         if (!mirror || mirror & MIRROR_FETCH) {
> -               strbuf_reset(&buf);
> -               strbuf_addf(&buf, "remote.%s.fetch", name);
> +               strbuf_setf(&buf, "remote.%s.fetch", name);

This hunk nicely illustrates an important maintenance benefit of
strbuf_set() not mentioned as justification in patch 1/2. The original
programmer knew that 'buf' and 'buf2' had not yet been used, even
though their declarations are quite distant from this bit of code, so
he omitted the call to strbuf_reset(). Although the omission is valid,
it also is potentially dangerous and a maintenance burden. The person
who next inserts code which uses 'buf' or 'buf2' between here and the
declarations must remember or know to add strbuf_reset()s here, but
that's easily overlooked. strbuf_set() eliminates that burden.

This is the strongest argument I've thus far seen in favor of
strbuf_set() (though I'm not, in any way, suggesting you reroll the
series merely to mention this).

>                 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);
> --
> 2.0.0
>

Re: [PATCH v3 1/2] add strbuf_set operations

[Jeremiah Mahler]

Thomas,

On Thu, Jun 12, 2014 at 10:11:36AM +0200, Thomas Braun wrote:
> Am 12.06.2014 09:29, schrieb Jeremiah Mahler:
> > A common use case with strubfs is to set the buffer to a new value.
> > This must be done in two steps: a reset followed by an add.
> > 
> >   strbuf_reset(buf);
> >   strbuf_add(buf, new_buf, len);
> > 
> > In cases where the buffer is being built up in steps, these operations
> > make sense and correctly convey what is being performed.
> > 
> >   strbuf_reset(buf);
> >   strbuf_add(buf, data1, len1);
> >   strbuf_add(buf, data2, len2);
> >   strbuf_add(buf, data3, len3);
> > 
> > However, in other cases, it can be confusing and is not very concise.
> > 
> >   strbuf_reset(buf);
> >   strbuf_add(buf, default, len1);
> > 
> >   if (cond1) {
> >     strbuf_reset(buf);
> >     strbuf_add(buf, data2, len2);
> >   }
> > 
> >   if (cond2) {
> >     strbuf_reset(buf);
> >     strbuf_add(buf, data3, len3);
> >   }
> > 
> > Add strbuf_set operations so that it can be re-written in a clear and
> > concise way.
> > 
> >   strbuf_set(buf, default len1);
> very minor nit: missing comma between default and len1.

I can't believe I missed that.  Good catch ;-)

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

Re: [PATCH v3 1/2] add strbuf_set operations

[Junio C Hamano]

Jeremiah Mahler <jmmahler@gmail.com> writes:

> A common use case with strubfs is to set the buffer to a new value.
> This must be done in two steps: a reset followed by an add.
>
>   strbuf_reset(buf);
>   strbuf_add(buf, new_buf, len);
>
> In cases where the buffer is being built up in steps, these operations
> make sense and correctly convey what is being performed.
>
>   strbuf_reset(buf);
>   strbuf_add(buf, data1, len1);
>   strbuf_add(buf, data2, len2);
>   strbuf_add(buf, data3, len3);
>
> However, in other cases, it can be confusing and is not very concise.
>
>   strbuf_reset(buf);
>   strbuf_add(buf, default, len1);
>
>   if (cond1) {
>     strbuf_reset(buf);
>     strbuf_add(buf, data2, len2);
>   }
>
>   if (cond2) {
>     strbuf_reset(buf);
>     strbuf_add(buf, data3, len3);
>   }
>
> Add strbuf_set operations so that it can be re-written in a clear and
> concise way.
>
>   strbuf_set(buf, default len1);
>
>   if (cond1) {
>     strbuf_set(buf, data2, len2);
>   }
>
>   if (cond2) {
>     strbuf_set(buf, data3, len3);
>   }

Or even more concisely without making unnecessary internal calls to
strbuf_reset():

	strbuf_reset(buf);
        if (cond2)
        	strbuf_add(buf, data3, len3);
	else if (cond1)
        	strbuf_add(buf, data2, len2);
	else
        	strbuf_add(buf, default, len2);

;-)

I am on the fence.

I have this suspicion that the addition of strbuf_set() would *only*
help when the original written with reset-and-then-add sequence was
suboptimal to begin with, and it helps *only* how the code reads,
without correcting the fact that it is still doing unnecessary
"first set to a value to be discarded and then reset to set the
right value", sweeping the issue under the rug.

Repeated reset-and-then-add on the same strbuf used to be something
that may indicate that the code is doing unnecessary work.  Now,
repeated uses of strbuf_set on the same strbuf replaced that pattern
to be watched for to spot wasteful code paths.

I dunno...

> Signed-off-by: Jeremiah Mahler <jmmahler@gmail.com>
> ---
>  Documentation/technical/api-strbuf.txt | 18 ++++++++++++++++++
>  strbuf.c                               | 21 +++++++++++++++++++++
>  strbuf.h                               | 13 +++++++++++++
>  3 files changed, 52 insertions(+)
>
> diff --git a/Documentation/technical/api-strbuf.txt b/Documentation/technical/api-strbuf.txt
> index f9c06a7..ae9c9cc 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`::
> +
> +	Replace content with data of a given length.
> +
> +`strbuf_setstr`::
> +
> +	Replace content with data from a NUL-terminated string.
> +
> +`strbuf_setf`::
> +
> +	Replace content with a formatted string.
> +
> +`strbuf_setbuf`::
> +
> +	Replace 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.
> 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..5041c35 100644
> --- a/strbuf.h
> +++ b/strbuf.h
> @@ -101,6 +101,19 @@ 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)
>  {

Re: [PATCH v3 1/2] add strbuf_set operations

[Junio C Hamano]

Jeremiah Mahler <jmmahler@gmail.com> writes:

> Thomas,
>
> On Thu, Jun 12, 2014 at 10:11:36AM +0200, Thomas Braun wrote:
>> Am 12.06.2014 09:29, schrieb Jeremiah Mahler:
>> > A common use case with strubfs is to set the buffer to a new value.

strubfs???

>> > This must be done in two steps: a reset followed by an add.
>> > 
>> >   strbuf_reset(buf);
>> >   strbuf_add(buf, new_buf, len);
>> > 
>> > In cases where the buffer is being built up in steps, these operations
>> > make sense and correctly convey what is being performed.
>> > 
>> >   strbuf_reset(buf);
>> >   strbuf_add(buf, data1, len1);
>> >   strbuf_add(buf, data2, len2);
>> >   strbuf_add(buf, data3, len3);
>> > 
>> > However, in other cases, it can be confusing and is not very concise.
>> > 
>> >   strbuf_reset(buf);
>> >   strbuf_add(buf, default, len1);
>> > 
>> >   if (cond1) {
>> >     strbuf_reset(buf);
>> >     strbuf_add(buf, data2, len2);
>> >   }
>> > 
>> >   if (cond2) {
>> >     strbuf_reset(buf);
>> >     strbuf_add(buf, data3, len3);
>> >   }
>> > 
>> > Add strbuf_set operations so that it can be re-written in a clear and
>> > concise way.
>> > 
>> >   strbuf_set(buf, default len1);
>> very minor nit: missing comma between default and len1.
>
> I can't believe I missed that.  Good catch ;-)

Re: [PATCH v3 1/2] add strbuf_set operations

[Jeremiah Mahler]

Junio,

Comments below...

On Thu, Jun 12, 2014 at 11:50:41AM -0700, Junio C Hamano wrote:
> Jeremiah Mahler <jmmahler@gmail.com> writes:
> 
> > A common use case with strubfs is to set the buffer to a new value.
> > This must be done in two steps: a reset followed by an add.
> >
> >   strbuf_reset(buf);
> >   strbuf_add(buf, new_buf, len);
> >
> > In cases where the buffer is being built up in steps, these operations
> > make sense and correctly convey what is being performed.
> >
> >   strbuf_reset(buf);
> >   strbuf_add(buf, data1, len1);
> >   strbuf_add(buf, data2, len2);
> >   strbuf_add(buf, data3, len3);
> >
> > However, in other cases, it can be confusing and is not very concise.
> >
> >   strbuf_reset(buf);
> >   strbuf_add(buf, default, len1);
> >
> >   if (cond1) {
> >     strbuf_reset(buf);
> >     strbuf_add(buf, data2, len2);
> >   }
> >
> >   if (cond2) {
> >     strbuf_reset(buf);
> >     strbuf_add(buf, data3, len3);
> >   }
> >
> > Add strbuf_set operations so that it can be re-written in a clear and
> > concise way.
> >
> >   strbuf_set(buf, default len1);
> >
> >   if (cond1) {
> >     strbuf_set(buf, data2, len2);
> >   }
> >
> >   if (cond2) {
> >     strbuf_set(buf, data3, len3);
> >   }
> 
> Or even more concisely without making unnecessary internal calls to
> strbuf_reset():
> 
> 	strbuf_reset(buf);
>         if (cond2)
>         	strbuf_add(buf, data3, len3);
> 	else if (cond1)
>         	strbuf_add(buf, data2, len2);
> 	else
>         	strbuf_add(buf, default, len2);
> 
> ;-)

_add would work in your example.  strbuf_set would also work and it
would perform the same number of operations (one set and one add).

However your example is different from mine in which cond1 and cond2 are
not necessarily mutually exclusive.

> 
> I am on the fence.
> 
> I have this suspicion that the addition of strbuf_set() would *only*
> help when the original written with reset-and-then-add sequence was
> suboptimal to begin with, and it helps *only* how the code reads,
> without correcting the fact that it is still doing unnecessary
> "first set to a value to be discarded and then reset to set the
> right value", sweeping the issue under the rug.
> 
It is certainly possible that builtin/remote.c (PATCH 2/2) saw the most
benefit from this operation because it is so badly designed.  But this
seems unlikely given the review process around here ;-)

The one case where it would be doing extra work is when a strbuf_set
replaces a strbuf_add that didn't previously have a strbuf_reset.
strbuf_set is not appropriate for all cases, as I mentioned in the
patch, but in some cases I think it makes it more readable.  And in this
case it would be doing a reset on an empty strbuf.  Is avoiding this
expense worth the reduction in readability?

Also, as Eric Sunshine pointed out, being able to easily re-order
operations can make the code easier to maintain.

> Repeated reset-and-then-add on the same strbuf used to be something
> that may indicate that the code is doing unnecessary work.  Now,
> repeated uses of strbuf_set on the same strbuf replaced that pattern
> to be watched for to spot wasteful code paths.
> 
If a reset followed by and add was a rare occurrence I would tend to
agree more.

> I dunno...
> 
> > Signed-off-by: Jeremiah Mahler <jmmahler@gmail.com>
> > ---
> >  Documentation/technical/api-strbuf.txt | 18 ++++++++++++++++++
> >  strbuf.c                               | 21 +++++++++++++++++++++
> >  strbuf.h                               | 13 +++++++++++++
> >  3 files changed, 52 insertions(+)
> >
> > diff --git a/Documentation/technical/api-strbuf.txt b/Documentation/technical/api-strbuf.txt
> > index f9c06a7..ae9c9cc 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.
> >  /*----- add data in your buffer -----*/
...
> >  static inline void strbuf_addch(struct strbuf *sb, int c)
> >  {

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

Re: [PATCH v3 1/2] add strbuf_set operations

[Jeremiah Mahler]

Junio,

On Thu, Jun 12, 2014 at 11:51:19AM -0700, Junio C Hamano wrote:
> Jeremiah Mahler <jmmahler@gmail.com> writes:
> 
> > Thomas,
> >
> > On Thu, Jun 12, 2014 at 10:11:36AM +0200, Thomas Braun wrote:
> >> Am 12.06.2014 09:29, schrieb Jeremiah Mahler:
> >> > A common use case with strubfs is to set the buffer to a new value.
> 
> strubfs???
> 
I was trying to make it plural.  Perhaps strbuf's?

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

Re: [PATCH v3 1/2] add strbuf_set operations

[Eric Sunshine]

On Thu, Jun 12, 2014 at 3:36 PM, Jeremiah Mahler <jmmahler@gmail.com> wrote:
> On Thu, Jun 12, 2014 at 11:51:19AM -0700, Junio C Hamano wrote:
>> Jeremiah Mahler <jmmahler@gmail.com> writes:
>>
>> > Thomas,
>> >
>> > On Thu, Jun 12, 2014 at 10:11:36AM +0200, Thomas Braun wrote:
>> >> Am 12.06.2014 09:29, schrieb Jeremiah Mahler:
>> >> > A common use case with strubfs is to set the buffer to a new value.
>>
>> strubfs???
>>
> I was trying to make it plural.  Perhaps strbuf's?

Junio was pointing out your misspelling, not your intention to pluralize.

Re: [PATCH v3 1/2] add strbuf_set operations

[Eric Sunshine]

On Thu, Jun 12, 2014 at 3:31 PM, Jeremiah Mahler <jmmahler@gmail.com> wrote:
> On Thu, Jun 12, 2014 at 11:50:41AM -0700, Junio C Hamano wrote:
>> I am on the fence.
>>
>> I have this suspicion that the addition of strbuf_set() would *only*
>> help when the original written with reset-and-then-add sequence was
>> suboptimal to begin with, and it helps *only* how the code reads,
>> without correcting the fact that it is still doing unnecessary
>> "first set to a value to be discarded and then reset to set the
>> right value", sweeping the issue under the rug.
>>
> It is certainly possible that builtin/remote.c (PATCH 2/2) saw the most
> benefit from this operation because it is so badly designed.  But this
> seems unlikely given the review process around here ;-)
>
> The one case where it would be doing extra work is when a strbuf_set
> replaces a strbuf_add that didn't previously have a strbuf_reset.
> strbuf_set is not appropriate for all cases, as I mentioned in the
> patch, but in some cases I think it makes it more readable.  And in this
> case it would be doing a reset on an empty strbuf.  Is avoiding this
> expense worth the reduction in readability?
>
> Also, as Eric Sunshine pointed out, being able to easily re-order
> operations can make the code easier to maintain.
>
>> Repeated reset-and-then-add on the same strbuf used to be something
>> that may indicate that the code is doing unnecessary work.  Now,
>> repeated uses of strbuf_set on the same strbuf replaced that pattern
>> to be watched for to spot wasteful code paths.
>>
> If a reset followed by and add was a rare occurrence I would tend to
> agree more.

When composing my review of the builtin/remote.c changes, I wrote
something like this:

    Although strbuf_set() does make the code a bit easier to read
    when strbufs are repeatedly re-used, re-using a variable for
    different purposes is generally considered poor programming
    practice. It's likely that heavy re-use of strbufs has been
    tolerated to avoid multiple heap allocations, but that may be a
    case of premature (allocation) optimization, rather than good
    programming. A different ("better") way to make the code more
    readable and maintainable may be to ban re-use of strbufs for
    different purposes.

But I deleted it before sending because it's a somewhat tangential
issue not introduced by your changes. However, I do see strbuf_set()
as a Band-Aid for the problem described above, rather than as a useful
feature on its own. If the practice of re-using strbufs (as a
premature optimization) ever becomes taboo, then strbuf_set() loses
its value.

>> I dunno...
>>
>> > Signed-off-by: Jeremiah Mahler <jmmahler@gmail.com>
>> > ---
>> >  Documentation/technical/api-strbuf.txt | 18 ++++++++++++++++++
>> >  strbuf.c                               | 21 +++++++++++++++++++++
>> >  strbuf.h                               | 13 +++++++++++++
>> >  3 files changed, 52 insertions(+)
>> >
>> > diff --git a/Documentation/technical/api-strbuf.txt b/Documentation/technical/api-strbuf.txt
>> > index f9c06a7..ae9c9cc 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.
>> >  /*----- add data in your buffer -----*/
> ...
>> >  static inline void strbuf_addch(struct strbuf *sb, int c)
>> >  {
>
> --
> Jeremiah Mahler
> jmmahler@gmail.com
> http://github.com/jmahler

Re: [PATCH v3 1/2] add strbuf_set operations

[Jeremiah Mahler]

On Thu, Jun 12, 2014 at 05:18:29PM -0400, Eric Sunshine wrote:
> On Thu, Jun 12, 2014 at 3:36 PM, Jeremiah Mahler <jmmahler@gmail.com> wrote:
> > On Thu, Jun 12, 2014 at 11:51:19AM -0700, Junio C Hamano wrote:
> >> Jeremiah Mahler <jmmahler@gmail.com> writes:
> >>
> >> > Thomas,
> >> >
> >> > On Thu, Jun 12, 2014 at 10:11:36AM +0200, Thomas Braun wrote:
> >> >> Am 12.06.2014 09:29, schrieb Jeremiah Mahler:
> >> >> > A common use case with strubfs is to set the buffer to a new value.
> >>
> >> strubfs???
> >>
> > I was trying to make it plural.  Perhaps strbuf's?
> 
> Junio was pointing out your misspelling, not your intention to pluralize.

OK, got it.

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

Re: [PATCH v3 1/2] add strbuf_set operations

[Jeremiah Mahler]

Eric,

On Thu, Jun 12, 2014 at 05:48:52PM -0400, Eric Sunshine wrote:
> On Thu, Jun 12, 2014 at 3:31 PM, Jeremiah Mahler <jmmahler@gmail.com> wrote:
> > On Thu, Jun 12, 2014 at 11:50:41AM -0700, Junio C Hamano wrote:
> >> I am on the fence.
> >>
> >> I have this suspicion that the addition of strbuf_set() would *only*
> >> help when the original written with reset-and-then-add sequence was
> >> suboptimal to begin with, and it helps *only* how the code reads,
> >> without correcting the fact that it is still doing unnecessary
> >> "first set to a value to be discarded and then reset to set the
> >> right value", sweeping the issue under the rug.
> >>
> > It is certainly possible that builtin/remote.c (PATCH 2/2) saw the most
> > benefit from this operation because it is so badly designed.  But this
> > seems unlikely given the review process around here ;-)
> >
> > The one case where it would be doing extra work is when a strbuf_set
> > replaces a strbuf_add that didn't previously have a strbuf_reset.
> > strbuf_set is not appropriate for all cases, as I mentioned in the
> > patch, but in some cases I think it makes it more readable.  And in this
> > case it would be doing a reset on an empty strbuf.  Is avoiding this
> > expense worth the reduction in readability?
> >
> > Also, as Eric Sunshine pointed out, being able to easily re-order
> > operations can make the code easier to maintain.
> >
> >> Repeated reset-and-then-add on the same strbuf used to be something
> >> that may indicate that the code is doing unnecessary work.  Now,
> >> repeated uses of strbuf_set on the same strbuf replaced that pattern
> >> to be watched for to spot wasteful code paths.
> >>
> > If a reset followed by and add was a rare occurrence I would tend to
> > agree more.
> 
> When composing my review of the builtin/remote.c changes, I wrote
> something like this:
> 
>     Although strbuf_set() does make the code a bit easier to read
>     when strbufs are repeatedly re-used, re-using a variable for
>     different purposes is generally considered poor programming
>     practice. It's likely that heavy re-use of strbufs has been
>     tolerated to avoid multiple heap allocations, but that may be a
>     case of premature (allocation) optimization, rather than good
>     programming. A different ("better") way to make the code more
>     readable and maintainable may be to ban re-use of strbufs for
>     different purposes.
> 
> But I deleted it before sending because it's a somewhat tangential
> issue not introduced by your changes. However, I do see strbuf_set()
> as a Band-Aid for the problem described above, rather than as a useful
> feature on its own. If the practice of re-using strbufs (as a
> premature optimization) ever becomes taboo, then strbuf_set() loses
> its value.
> 

I am getting the feeling that I have mis-understood the purpose of
strbufs.  It is not just a library to use in place of char*.

If strbufs should only be added to and never reset a good test would be
to re-write builtin/remote.c without the use of strbuf_reset.

builtin/remote.c does re-use the buffers.  But it seems if a buffer is
used N times then to avoid a reset you would need N buffers.

But on the other hand I agree with your comment that re-using a variable
for different purposes is poor practice.

Now I am not even sure if I want my own patch :-)

> >> I dunno...
> >>
> >> > Signed-off-by: Jeremiah Mahler <jmmahler@gmail.com>
> >> > ---
> >> >  Documentation/technical/api-strbuf.txt | 18 ++++++++++++++++++
> >> >  strbuf.c                               | 21 +++++++++++++++++++++
> >> >  strbuf.h                               | 13 +++++++++++++
> >> >  3 files changed, 52 insertions(+)
> >> >
> >> > diff --git a/Documentation/technical/api-strbuf.txt b/Documentation/technical/api-strbuf.txt
> >> > index f9c06a7..ae9c9cc 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.
> >> >  /*----- add data in your buffer -----*/
> > ...
> >> >  static inline void strbuf_addch(struct strbuf *sb, int c)
> >> >  {
> >
> > --
> > Jeremiah Mahler
> > jmmahler@gmail.com
> > http://github.com/jmahler

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

Re: [PATCH v3 1/2] add strbuf_set operations

[Jeff King]

On Thu, Jun 12, 2014 at 04:46:37PM -0700, Jeremiah Mahler wrote:

> >     Although strbuf_set() does make the code a bit easier to read
> >     when strbufs are repeatedly re-used, re-using a variable for
> >     different purposes is generally considered poor programming
> >     practice. It's likely that heavy re-use of strbufs has been
> >     tolerated to avoid multiple heap allocations, but that may be a
> >     case of premature (allocation) optimization, rather than good
> >     programming. A different ("better") way to make the code more
> >     readable and maintainable may be to ban re-use of strbufs for
> >     different purposes.
> > 
> > But I deleted it before sending because it's a somewhat tangential
> > issue not introduced by your changes. However, I do see strbuf_set()
> > as a Band-Aid for the problem described above, rather than as a useful
> > feature on its own. If the practice of re-using strbufs (as a
> > premature optimization) ever becomes taboo, then strbuf_set() loses
> > its value.
> > 
> 
> I am getting the feeling that I have mis-understood the purpose of
> strbufs.  It is not just a library to use in place of char*.
> 
> If strbufs should only be added to and never reset a good test would be
> to re-write builtin/remote.c without the use of strbuf_reset.
> 
> builtin/remote.c does re-use the buffers.  But it seems if a buffer is
> used N times then to avoid a reset you would need N buffers.
> 
> But on the other hand I agree with your comment that re-using a variable
> for different purposes is poor practice.
> 
> Now I am not even sure if I want my own patch :-)

I think reusing buffers like remote.c does makes things uglier and more
confusing than necessary, and probably doesn't have any appreciable
performance gain. We are saving a handful of allocations, and have such
wonderful variable names as "buf2" (when is it OK to reuse that one,
versus regular "buf"?).

A better reason I think is to reuse allocations across a loop, like:

  struct strbuf buf = STRBUF_INIT;

  for (i = 0; i < nr; i++) {
	strbuf_reset(&buf);
	strbuf_add(&buf, foo[i]);
	... do something with buf ...
  }
  strbuf_release(&buf);

You can write that as:

  for (i = 0; i < nr; i++) {
	struct strbuf buf = STRBUF_INIT;
	strbuf_add(&buf, foo[i]);
	... do something ...
	strbuf_release(&buf);
  }

and it is definitely still a case of premature optimization. But:

  1. "nr" here may be very large, so the amortized benefits are greater

  2. It's only one call to strbuf_reset to cover many items. Not one
     sprinkled every few lines.

You'll note that strbuf_getline uses a "set" convention (making it
different from the rest of strbuf) to handle this looping case.

I don't have a problem with strbuf_set, but just peeking at remote.c, I
think I'd rather see it cleaned up. It looks like one of the major uses
is setting config variables. I wonder how hard it would be to make a
git_config_set variant that took printf-style formats, and handled the
strbuf itself.

-Peff

Re: [PATCH v3 1/2] add strbuf_set operations

[Jeremiah Mahler]

Peff,

On Fri, Jun 13, 2014 at 03:15:50AM -0400, Jeff King wrote:
> On Thu, Jun 12, 2014 at 04:46:37PM -0700, Jeremiah Mahler wrote:
> 
> > >     Although strbuf_set() does make the code a bit easier to read
> > >     when strbufs are repeatedly re-used, re-using a variable for
> > >     different purposes is generally considered poor programming
> > >     practice. It's likely that heavy re-use of strbufs has been
> > >     tolerated to avoid multiple heap allocations, but that may be a
> > >     case of premature (allocation) optimization, rather than good
> > >     programming. A different ("better") way to make the code more
> > >     readable and maintainable may be to ban re-use of strbufs for
> > >     different purposes.
> > > 
> > > But I deleted it before sending because it's a somewhat tangential
> > > issue not introduced by your changes. However, I do see strbuf_set()
> > > as a Band-Aid for the problem described above, rather than as a useful
> > > feature on its own. If the practice of re-using strbufs (as a
> > > premature optimization) ever becomes taboo, then strbuf_set() loses
> > > its value.
> > > 
> > 
> > I am getting the feeling that I have mis-understood the purpose of
> > strbufs.  It is not just a library to use in place of char*.
> > 
> > If strbufs should only be added to and never reset a good test would be
> > to re-write builtin/remote.c without the use of strbuf_reset.
> > 
> > builtin/remote.c does re-use the buffers.  But it seems if a buffer is
> > used N times then to avoid a reset you would need N buffers.
> > 
> > But on the other hand I agree with your comment that re-using a variable
> > for different purposes is poor practice.
> > 
> > Now I am not even sure if I want my own patch :-)
> 
> I think reusing buffers like remote.c does makes things uglier and more
> confusing than necessary, and probably doesn't have any appreciable
> performance gain. We are saving a handful of allocations, and have such
> wonderful variable names as "buf2" (when is it OK to reuse that one,
> versus regular "buf"?).
> 
> A better reason I think is to reuse allocations across a loop, like:
> 
>   struct strbuf buf = STRBUF_INIT;
> 
>   for (i = 0; i < nr; i++) {
> 	strbuf_reset(&buf);
> 	strbuf_add(&buf, foo[i]);
> 	... do something with buf ...
>   }
>   strbuf_release(&buf);
> 
> You can write that as:
> 
>   for (i = 0; i < nr; i++) {
> 	struct strbuf buf = STRBUF_INIT;
> 	strbuf_add(&buf, foo[i]);
> 	... do something ...
> 	strbuf_release(&buf);
>   }
> 
> and it is definitely still a case of premature optimization. But:
> 
>   1. "nr" here may be very large, so the amortized benefits are greater
> 
>   2. It's only one call to strbuf_reset to cover many items. Not one
>      sprinkled every few lines.
> 
> You'll note that strbuf_getline uses a "set" convention (making it
> different from the rest of strbuf) to handle this looping case.
> 
> I don't have a problem with strbuf_set, but just peeking at remote.c, I
> think I'd rather see it cleaned up. It looks like one of the major uses
> is setting config variables. I wonder how hard it would be to make a
> git_config_set variant that took printf-style formats, and handled the
> strbuf itself.
> 
> -Peff

Improving remote.c sounds like a better direction than adding set
operations.  I will start looking in to it.

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