[PATCH v2 00/19] add strbuf_set operations

[PATCH v2 00/19] add strbuf_set operations

[Jeremiah Mahler]

Version 2 of the patch set to add strbuf_set operations.

Includes suggestions from Eric Sunshine [1]:

  - New operations and their documentation placed in one patch.

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

  - Use imperative mood in log messages.

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

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

  struct strbuf mybuf = STRBUF_INIT;

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

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

Several single line replacements were made for this reason.

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

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

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

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

-- 
2.0.0.592.gf55b190

[PATCH v2 01/19] add strbuf_set operations

[Jeremiah Mahler]

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

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

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

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

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

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

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

diff --git a/Documentation/technical/api-strbuf.txt b/Documentation/technical/api-strbuf.txt
index 077a709..b7e23da 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 the buffer content with data of a given length.
+
+`strbuf_setstr`::
+
+	Replace the buffer content with data from a NUL-terminated string.
+
+`strbuf_setf`::
+
+	Replace the buffer content with a formatted string.
+
+`strbuf_setbuf`::
+
+	Replace the buffer content with data from another buffer.
+
 * Adding data to the buffer
 
 NOTE: All of the functions in this section will grow the buffer as necessary.
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.592.gf55b190

[PATCH v2 02/19] sha1_name: simplify via strbuf_set()

[Jeremiah Mahler]

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

Signed-off-by: Jeremiah Mahler <jmmahler@gmail.com>
---
 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.592.gf55b190

[PATCH v2 03/19] fast-import: simplify via strbuf_set()

[Jeremiah Mahler]

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

Signed-off-by: Jeremiah Mahler <jmmahler@gmail.com>
---
 fast-import.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/fast-import.c b/fast-import.c
index e8ec34d..cfe9404 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -2898,8 +2898,7 @@ static void cat_blob(struct object_entry *oe, unsigned char sha1[20])
 	 * Output based on batch_one_object() from cat-file.c.
 	 */
 	if (type <= 0) {
-		strbuf_reset(&line);
-		strbuf_addf(&line, "%s missing\n", sha1_to_hex(sha1));
+		strbuf_setf(&line, "%s missing\n", sha1_to_hex(sha1));
 		cat_blob_write(line.buf, line.len);
 		strbuf_release(&line);
 		free(buf);
@@ -2910,8 +2909,7 @@ static void cat_blob(struct object_entry *oe, unsigned char sha1[20])
 	if (type != OBJ_BLOB)
 		die("Object %s is a %s but a blob was expected.",
 		    sha1_to_hex(sha1), typename(type));
-	strbuf_reset(&line);
-	strbuf_addf(&line, "%s %s %lu\n", sha1_to_hex(sha1),
+	strbuf_setf(&line, "%s %s %lu\n", sha1_to_hex(sha1),
 						typename(type), size);
 	cat_blob_write(line.buf, line.len);
 	strbuf_release(&line);
-- 
2.0.0.592.gf55b190

[PATCH v2 04/19] builtin/remote: simplify via strbuf_set()

[Jeremiah Mahler]

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

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

diff --git a/builtin/remote.c b/builtin/remote.c
index c9b67ff..f2d809d 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);
@@ -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.592.gf55b190

[PATCH v2 05/19] branch: simplify via strbuf_set()

[Jeremiah Mahler]

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

Signed-off-by: Jeremiah Mahler <jmmahler@gmail.com>
---
 branch.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

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

[PATCH v2 06/19] builtin/branch: simplify via strbuf_set()

[Jeremiah Mahler]

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

Signed-off-by: Jeremiah Mahler <jmmahler@gmail.com>
---
 builtin/branch.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

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

[PATCH v2 07/19] builtin/checkout: simplify via strbuf_set()

[Jeremiah Mahler]

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

Signed-off-by: Jeremiah Mahler <jmmahler@gmail.com>
---
 builtin/checkout.c | 18 ++++++------------
 1 file changed, 6 insertions(+), 12 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index 9cbe7d1..38fc0ce 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -912,8 +912,7 @@ static int prepare_linked_checkout(const struct checkout_opts *opts,
 			  sb_git.buf);
 	junk_work_tree = path;
 
-	strbuf_reset(&sb);
-	strbuf_addf(&sb, "%s/gitdir", sb_repo.buf);
+	strbuf_setf(&sb, "%s/gitdir", sb_repo.buf);
 	write_file(sb.buf, 1, "%s\n", real_path(sb_git.buf));
 	write_file(sb_git.buf, 1, "gitdir: %s/repos/%s\n",
 		   real_path(get_git_common_dir()), name);
@@ -923,11 +922,9 @@ static int prepare_linked_checkout(const struct checkout_opts *opts,
 	 * value would do because this value will be ignored and
 	 * replaced at the next (real) checkout.
 	 */
-	strbuf_reset(&sb);
-	strbuf_addf(&sb, "%s/HEAD", sb_repo.buf);
+	strbuf_setf(&sb, "%s/HEAD", sb_repo.buf);
 	write_file(sb.buf, 1, "%s\n", sha1_to_hex(new->commit->object.sha1));
-	strbuf_reset(&sb);
-	strbuf_addf(&sb, "%s/commondir", sb_repo.buf);
+	strbuf_setf(&sb, "%s/commondir", sb_repo.buf);
 	write_file(sb.buf, 1, "../..\n");
 
 	if (!opts->quiet)
@@ -942,8 +939,7 @@ static int prepare_linked_checkout(const struct checkout_opts *opts,
 	ret = run_command(&cp);
 	if (!ret)
 		is_junk = 0;
-	strbuf_reset(&sb);
-	strbuf_addf(&sb, "%s/locked", sb_repo.buf);
+	strbuf_setf(&sb, "%s/locked", sb_repo.buf);
 	unlink_or_warn(sb.buf);
 	strbuf_release(&sb);
 	strbuf_release(&sb_repo);
@@ -1048,8 +1044,7 @@ static void check_linked_checkouts(struct branch_info *new)
 		return;
 	}
 
-	strbuf_reset(&path);
-	strbuf_addf(&path, "%s/HEAD", get_git_common_dir());
+	strbuf_setf(&path, "%s/HEAD", get_git_common_dir());
 	/*
 	 * $GIT_COMMON_DIR/HEAD is practically outside
 	 * $GIT_DIR so resolve_ref_unsafe() won't work (it
@@ -1064,8 +1059,7 @@ static void check_linked_checkouts(struct branch_info *new)
 	while ((d = readdir(dir)) != NULL) {
 		if (!strcmp(d->d_name, ".") || !strcmp(d->d_name, ".."))
 			continue;
-		strbuf_reset(&path);
-		strbuf_addf(&path, "%s/repos/%s/HEAD",
+		strbuf_setf(&path, "%s/repos/%s/HEAD",
 			    get_git_common_dir(), d->d_name);
 		if (check_linked_checkout(new, d->d_name, path.buf))
 			break;
-- 
2.0.0.592.gf55b190

[PATCH v2 08/19] builtin/mailinfo: simplify via strbuf_set()

[Jeremiah Mahler]

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

Signed-off-by: Jeremiah Mahler <jmmahler@gmail.com>
---
 builtin/mailinfo.c | 18 ++++++------------
 1 file changed, 6 insertions(+), 12 deletions(-)

diff --git a/builtin/mailinfo.c b/builtin/mailinfo.c
index cf11c8d..596071e 100644
--- a/builtin/mailinfo.c
+++ b/builtin/mailinfo.c
@@ -40,8 +40,7 @@ static void get_sane_name(struct strbuf *out, struct strbuf *name, struct strbuf
 		src = email;
 	else if (name == out)
 		return;
-	strbuf_reset(out);
-	strbuf_addbuf(out, src);
+	strbuf_setbuf(out, src);
 }
 
 static void parse_bogus_from(const struct strbuf *line)
@@ -62,11 +61,9 @@ static void parse_bogus_from(const struct strbuf *line)
 	if (!ket)
 		return;
 
-	strbuf_reset(&email);
-	strbuf_add(&email, bra + 1, ket - bra - 1);
+	strbuf_set(&email, bra + 1, ket - bra - 1);
 
-	strbuf_reset(&name);
-	strbuf_add(&name, line->buf, bra - line->buf);
+	strbuf_set(&name, line->buf, bra - line->buf);
 	strbuf_trim(&name);
 	get_sane_name(&name, &name, &email);
 }
@@ -108,8 +105,7 @@ static void handle_from(const struct strbuf *from)
 		at--;
 	}
 	el = strcspn(at, " \n\t\r\v\f>");
-	strbuf_reset(&email);
-	strbuf_add(&email, at, el);
+	strbuf_set(&email, at, el);
 	strbuf_remove(&f, at - f.buf, el + (at[el] ? 1 : 0));
 
 	/* The remainder is name.  It could be
@@ -567,8 +563,7 @@ static int decode_header_bq(struct strbuf *it)
 		in = ep + 2;
 	}
 	strbuf_addstr(&outbuf, in);
-	strbuf_reset(it);
-	strbuf_addbuf(it, &outbuf);
+	strbuf_setbuf(it, &outbuf);
 decode_header_bq_out:
 	strbuf_release(&outbuf);
 	strbuf_release(&charset_q);
@@ -602,8 +597,7 @@ static void decode_transfer_encoding(struct strbuf *line)
 	default:
 		return;
 	}
-	strbuf_reset(line);
-	strbuf_addbuf(line, ret);
+	strbuf_setbuf(line, ret);
 	strbuf_release(ret);
 	free(ret);
 }
-- 
2.0.0.592.gf55b190

[PATCH v2 09/19] builtin/tag: simplify via strbuf_set()

[Jeremiah Mahler]

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

Signed-off-by: Jeremiah Mahler <jmmahler@gmail.com>
---
 builtin/tag.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

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

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

[Jeremiah Mahler]

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

Signed-off-by: Jeremiah Mahler <jmmahler@gmail.com>
---
 date.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

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

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

[Jeremiah Mahler]

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

Signed-off-by: Jeremiah Mahler <jmmahler@gmail.com>
---
 diffcore-order.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

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

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

[Jeremiah Mahler]

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

Signed-off-by: Jeremiah Mahler <jmmahler@gmail.com>
---
 http-backend.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

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

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

[Jeremiah Mahler]

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

Signed-off-by: Jeremiah Mahler <jmmahler@gmail.com>
---
 http.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

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

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

[Jeremiah Mahler]

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

Signed-off-by: Jeremiah Mahler <jmmahler@gmail.com>
---
 ident.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/ident.c b/ident.c
index 1d9b6e7..523e249 100644
--- a/ident.c
+++ b/ident.c
@@ -397,8 +397,7 @@ int git_ident_config(const char *var, const char *value, void *data)
 	if (!strcmp(var, "user.name")) {
 		if (!value)
 			return config_error_nonbool(var);
-		strbuf_reset(&git_default_name);
-		strbuf_addstr(&git_default_name, value);
+		strbuf_setstr(&git_default_name, value);
 		committer_ident_explicitly_given |= IDENT_NAME_GIVEN;
 		author_ident_explicitly_given |= IDENT_NAME_GIVEN;
 		return 0;
@@ -407,8 +406,7 @@ int git_ident_config(const char *var, const char *value, void *data)
 	if (!strcmp(var, "user.email")) {
 		if (!value)
 			return config_error_nonbool(var);
-		strbuf_reset(&git_default_email);
-		strbuf_addstr(&git_default_email, value);
+		strbuf_setstr(&git_default_email, value);
 		committer_ident_explicitly_given |= IDENT_MAIL_GIVEN;
 		author_ident_explicitly_given |= IDENT_MAIL_GIVEN;
 		return 0;
-- 
2.0.0.592.gf55b190

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

[Jeremiah Mahler]

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

Signed-off-by: Jeremiah Mahler <jmmahler@gmail.com>
---
 remote-curl.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

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

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

[Jeremiah Mahler]

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

Signed-off-by: Jeremiah Mahler <jmmahler@gmail.com>
---
 submodule.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

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

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

[Jeremiah Mahler]

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

Signed-off-by: Jeremiah Mahler <jmmahler@gmail.com>
---
 transport.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

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

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

[Jeremiah Mahler]

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

Signed-off-by: Jeremiah Mahler <jmmahler@gmail.com>
---
 vcs-svn/svndump.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/vcs-svn/svndump.c b/vcs-svn/svndump.c
index 31d1d83..7fbf5d8 100644
--- a/vcs-svn/svndump.c
+++ b/vcs-svn/svndump.c
@@ -352,8 +352,7 @@ void svndump_read(const char *url, const char *local_ref, const char *notes_ref)
 		case sizeof("UUID"):
 			if (constcmp(t, "UUID"))
 				continue;
-			strbuf_reset(&dump_ctx.uuid);
-			strbuf_addstr(&dump_ctx.uuid, val);
+			strbuf_setstr(&dump_ctx.uuid, val);
 			break;
 		case sizeof("Revision-number"):
 			if (constcmp(t, "Revision-number"))
-- 
2.0.0.592.gf55b190

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

[Jeremiah Mahler]

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

Signed-off-by: Jeremiah Mahler <jmmahler@gmail.com>
---
 wt-status.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

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

Re: [PATCH v2 00/19] add strbuf_set operations

[Eric Sunshine]

On Mon, Jun 9, 2014 at 6:19 PM, Jeremiah Mahler <jmmahler@gmail.com> wrote:
> Version 2 of the patch set to add strbuf_set operations.
>
> Includes suggestions from Eric Sunshine [1]:
>
> [...snip...]
>
> Using strbuf_set before a sequence of adds can be confusing.  But using
> strbuf_add when nothing important was being added to can also be
> confusing.  strbuf_set can be used to make these cases more clear.
>
>   struct strbuf mybuf = STRBUF_INIT;
>
>   strbuf_add(&mybuf, ...);  /* Was something there before? */
>
>   strbuf_set(&mybuf, ...);  /* Replace everything. */
>
> Several single line replacements were made for this reason.
>
> Additional files have also been converted compared to version 1 [1].
>
> [1]: http://marc.info/?l=git&m=140230874124060&w=2

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

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

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

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

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

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

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

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

Re: [PATCH v2 01/19] add strbuf_set operations

[Eric Sunshine]

On Mon, Jun 9, 2014 at 6:19 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)
>   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);

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

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

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

More below.

> Signed-off-by: Jeremiah Mahler <jmmahler@gmail.com>
> ---
>  Documentation/technical/api-strbuf.txt | 18 ++++++++++++++++++
>  strbuf.c                               | 21 +++++++++++++++++++++
>  strbuf.h                               | 14 ++++++++++++++
>  3 files changed, 53 insertions(+)
>
> diff --git a/Documentation/technical/api-strbuf.txt b/Documentation/technical/api-strbuf.txt
> index 077a709..b7e23da 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 the buffer content with data of a given length.

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

    Replace content with [...].

More below.

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

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

> +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.592.gf55b190

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

Re: [PATCH v2 01/19] add strbuf_set operations

[Michael Haggerty]

On 06/10/2014 12:19 AM, Jeremiah Mahler 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)
>   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>
> ---
>  Documentation/technical/api-strbuf.txt | 18 ++++++++++++++++++
>  strbuf.c                               | 21 +++++++++++++++++++++
>  strbuf.h                               | 14 ++++++++++++++
>  3 files changed, 53 insertions(+)
> 
> diff --git a/Documentation/technical/api-strbuf.txt b/Documentation/technical/api-strbuf.txt
> index 077a709..b7e23da 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 the buffer content with data of a given length.
> +
> +`strbuf_setstr`::
> +
> +	Replace the buffer content with data from a NUL-terminated string.
> +
> +`strbuf_setf`::
> +
> +	Replace the buffer content with a formatted string.
> +
> +`strbuf_setbuf`::
> +
> +	Replace the buffer content with data from another buffer.
> +
>  * Adding data to the buffer
>  
>  NOTE: All of the functions in this section will grow the buffer as necessary.
> 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);
> +}
> +

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

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

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

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

(strbuf_grow() could call it:

static inline void strbuf_grow(struct strbuf *sb, size_t extra)
{
	if (unsigned_add_overflows(sb->len, extra))
		die("you want to use way too much memory");
	strbuf_grow_to(sb, sb->len + extra);
}

), then your function could be minimized to

void strbuf_set(struct strbuf *sb, const void *data, size_t len)
{
	strbuf_grow_to(sb, len);
	memcpy(sb->buf, data, len);
	strbuf_setlen(sb, len);
}

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

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

> [...]

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/

Re: [PATCH v2 00/19] add strbuf_set operations

[Jeremiah Mahler]

Eric,

On Wed, Jun 11, 2014 at 04:09:17AM -0400, Eric Sunshine wrote:
> On Mon, Jun 9, 2014 at 6:19 PM, Jeremiah Mahler <jmmahler@gmail.com> wrote:
> > Version 2 of the patch set to add strbuf_set operations.
> >
> > Includes suggestions from Eric Sunshine [1]:
> >
> > [...snip...]
> >
...
> 
> Food for thought: This patch series has now likely entered the
> territory of unnecessary code churn. Quoting from
> Documentation/CodingGuidelines:
> 
>     Fixing style violations while working on a real change as a
>     preparatory clean-up step is good, but otherwise avoid useless
>     code churn for the sake of conforming to the style.
> 
>     "Once it _is_ in the tree, it's not really worth the patch noise
>     to go and fix it up."
>     Cf. http://article.gmane.org/gmane.linux.kernel/943020
> 
> Rewriting code merely for the sake of replacing strbuf_reset() +
> strbuf_add() with strbuf_set(), while not exactly a style cleanup,
> falls into the same camp. (The patch which introduces strbuf_set(),
> however, is not churn.) Each change you make can potentially conflict
> with other topics already in-flight (see [1], for example), thus
> making more work for Junio.
> 
> Also, these sorts of apparently innocuous "mechanical" changes can
> have hidden costs [2], so it's useful to weigh carefully how lengthy
> you want to make this patch series.
> 
> Having said that, patch 4/19, particularly the changes to
> builtin/remote.c:mv(), in which multiple strbufs are reused
> repeatedly, does seem to make the code a bit easier to read and likely
> reduces cognitive load.
> 
> [1]: http://thread.gmane.org/gmane.comp.version-control.git/245133/focus=245147
> [2]: http://thread.gmane.org/gmane.comp.version-control.git/245133/focus=245144
> 

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

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

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

Re: [PATCH v2 01/19] add strbuf_set operations

[Jeremiah Mahler]

On Wed, Jun 11, 2014 at 04:42:56AM -0400, Eric Sunshine wrote:
> On Mon, Jun 9, 2014 at 6:19 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)
> >   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);
> 
> This commit message is effectively the cover letter for the entire
> patch series; it doesn't say specifically what _this_ patch is doing.
> 
> Justification for the change could be stronger. Rather than merely
> pointing out that a sequence of operations occurs N times in the
> project, explain why strbuf_set() is superior. For instance, you might
> say something about how strbuf_set() conveys the operation being
> performed more concisely and clearly than strbuf_reset() +
> strbuf_add() (and thus may reduce cognitive load, though that's
> subjective). The bit about performing the operation in one line
> instead of two is minor, at best, and may not be worth mentioning at
> all (since it's implied).
> 
After reviewing my argument, I think the biggest benefit is that it can
make the code more readable in some cases.

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

> 
> More below.
> 
> > Signed-off-by: Jeremiah Mahler <jmmahler@gmail.com>
> > ---
> >  Documentation/technical/api-strbuf.txt | 18 ++++++++++++++++++
> >  strbuf.c                               | 21 +++++++++++++++++++++
> >  strbuf.h                               | 14 ++++++++++++++
> >  3 files changed, 53 insertions(+)
> >
> > diff --git a/Documentation/technical/api-strbuf.txt b/Documentation/technical/api-strbuf.txt
> > index 077a709..b7e23da 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 the buffer content with data of a given length.
> 
> I know that you copied the wording I suggested in my v1 review, but
> now that I see it in context, I find the redundancy level rather high.
> The header already says "Setting the buffer", so repeating "the
> buffer" in each function description doesn't add much. It might make
> sense to reword as:
> 
>     Replace content with [...].
> 
Fixed.

> More below.
> 
> > +`strbuf_setstr`::
> > +
> > +       Replace the buffer content with data from a NUL-terminated string.
> > +
> > +`strbuf_setf`::
> > +
> > +       Replace the buffer content with a formatted string.
> > +
> > +`strbuf_setbuf`::
> > +
> > +       Replace the buffer content with data from another buffer.
> > +
> >  * Adding data to the buffer
> >
> >  NOTE: All of the functions in this section will grow the buffer as necessary.
> > 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 -----*/
> > +
> 
> Minor: Existing divider lines in this header are not followed by a blank line.
Fixed.
> 
> > +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.592.gf55b190
> 
> Final word: The comments in this review do not necessarily require a
> re-roll. Junio may or may not want to pick up the series as is. If he
> doesn't, or if you want to polish it further, then perhaps take the
> review comments into consideration when rerolling.

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

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

Re: [PATCH v2 01/19] add strbuf_set operations

[Jeremiah Mahler]

Michael,

On Wed, Jun 11, 2014 at 02:05:35PM +0200, Michael Haggerty wrote:
> On 06/10/2014 12:19 AM, Jeremiah Mahler 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.
> > 
...
> > 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);
> > +}
> > +
> 
> I never know how much intelligence to attribute to modern compilers, but
> it seems like even after optimization this function will be doing more
> work than necessary:
> 
>     strbuf_reset(sb)
>     -> strbuf_setlen(sb, 0)
>        -> sb->len = 0
>        -> sb->buf[len] = '\0'
>     -> strbuf_add(sb, data, len)
>        -> strbuf_grow(sb, len)
>           -> ...lots of stuff...
>        -> memcpy(sb->buf + sb->len, data, len)
>        -> strbuf_setlen(sb, sb->len + len)
>           -> sb->len = len
>           -> sb->buf[len] = '\0'
> 
> If there were a function like strbuf_grow_to(sb, len):
> 
> void strbuf_grow_to(struct strbuf *sb, size_t len)
> {
> 	int new_buf = !sb->alloc;
> 	if (unsigned_add_overflows(len, 1))
> 		die("you want to use way too much memory");
> 	if (new_buf)
> 		sb->buf = NULL;
> 	ALLOC_GROW(sb->buf, len + 1, sb->alloc);
> 	if (new_buf)
> 		sb->buf[0] = '\0';
> }
> 
grow_to() which could reduce in size, interesting.

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

> > [...]
> 
> Michael
> 
> -- 
> Michael Haggerty
> mhagger@alum.mit.edu
> http://softwareswirl.blogspot.com/

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

Re: [PATCH v2 01/19] add strbuf_set operations

[Michael Haggerty]

On 06/12/2014 09:10 AM, Jeremiah Mahler wrote:
> On Wed, Jun 11, 2014 at 02:05:35PM +0200, Michael Haggerty wrote:
>> [...]
>> If there were a function like strbuf_grow_to(sb, len):
>>
>> void strbuf_grow_to(struct strbuf *sb, size_t len)
>> {
>> 	int new_buf = !sb->alloc;
>> 	if (unsigned_add_overflows(len, 1))
>> 		die("you want to use way too much memory");
>> 	if (new_buf)
>> 		sb->buf = NULL;
>> 	ALLOC_GROW(sb->buf, len + 1, sb->alloc);
>> 	if (new_buf)
>> 		sb->buf[0] = '\0';
>> }
>>
> grow_to() which could reduce in size, interesting.

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

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

That's a good plan.

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

Michael

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

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/

Re: [PATCH v2 01/19] add strbuf_set operations

[Jeremiah Mahler]

Michael,

On Thu, Jun 12, 2014 at 12:21:48PM +0200, Michael Haggerty wrote:
> On 06/12/2014 09:10 AM, Jeremiah Mahler wrote:
> > On Wed, Jun 11, 2014 at 02:05:35PM +0200, Michael Haggerty wrote:
> >> [...]
> >> If there were a function like strbuf_grow_to(sb, len):
> >>
> >> void strbuf_grow_to(struct strbuf *sb, size_t len)
> >> {
> >> 	int new_buf = !sb->alloc;
> >> 	if (unsigned_add_overflows(len, 1))
> >> 		die("you want to use way too much memory");
> >> 	if (new_buf)
> >> 		sb->buf = NULL;
> >> 	ALLOC_GROW(sb->buf, len + 1, sb->alloc);
> >> 	if (new_buf)
> >> 		sb->buf[0] = '\0';
> >> }
> >>
> > grow_to() which could reduce in size, interesting.
> 
> I don't understand what you mean by "could reduce in size".  This
> function can only grow, never reduce, the size of the strbuf, because
> ALLOC_GROW doesn't do anything unless the requested size is larger than
> the currently-allocated size.
> 
I got it now.  strbuf_grow() adds extra to the current buf.
strbuf_grow_to() will add more if len is larger than the current length.

> >> (strbuf_grow() could call it:
> >>
> >> static inline void strbuf_grow(struct strbuf *sb, size_t extra)
> >> {
> >> 	if (unsigned_add_overflows(sb->len, extra))
> >> 		die("you want to use way too much memory");
> >> 	strbuf_grow_to(sb, sb->len + extra);
> >> }
> >>
> >> ), then your function could be minimized to
> >>
> >> void strbuf_set(struct strbuf *sb, const void *data, size_t len)
> >> {
> >> 	strbuf_grow_to(sb, len);
> >> 	memcpy(sb->buf, data, len);
> >> 	strbuf_setlen(sb, len);
> >> }
> >>
> >> I think strbuf_grow_to() would be useful in other situations too.
> >>
> >> This is just an idea; I'm not saying that you have to make this change.
> >>
> > I like your idea.  I am leaning towards doing the un-optimized
> > strbuf_set operations first, then optimizing in a later patch.
> 
> That's a good plan.
> 
> In case you're interested, I sketched what the strbuf_grow_to() changes
> might look like, and also looked for other places in the code where
> strbuf_grow_to() could be used instead of strbuf_grow() [1].  This is
> only a sketch; I haven't even tested the result.  Feel free to use what
> you want from it.
> 
I started looking at it.  I will let you know if I get something
together.

> Michael
> 
> [1] Branch "strbuf-grow-to-sketch" on https://github.com/mhagger/git
> 
> -- 
> Michael Haggerty
> mhagger@alum.mit.edu
> http://softwareswirl.blogspot.com/

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