Re: [PATCH ps/stash-in-c] strbuf_vinsertf: provide the correct buffer size to vsnprintf

[Johannes Sixt <j6t@xxxxxxxx>]

Am 03.02.19 um 17:51 schrieb Johannes Sixt:
> strbuf_vinsertf inserts a formatted string in the middle of an existing
> strbuf value.

Quite frankly, this is a really unusual operation, and I'd prefer to get
rid of it. There is only one call, and it looks like it only wants to be
lazy and save one strbuf variable.

This helper adds way more code than a non-lazy caller would need. There
wouldn't even be a mental burden. Like this (except that strbuf_addstr
doesn't do what I thought it would do...).

diff --git a/builtin/stash.c b/builtin/stash.c
index 74e6ff62b5..95d202aea3 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -1101,7 +1101,7 @@ static int stash_working_tree(struct stash_info *info, struct pathspec ps)
 	return ret;
 }
 
-static int do_create_stash(struct pathspec ps, struct strbuf *stash_msg_buf,
+static int do_create_stash(struct pathspec ps, const char *stash_msg,
 			   int include_untracked, int patch_mode,
 			   struct stash_info *info, struct strbuf *patch,
 			   int quiet)
@@ -1117,6 +1117,7 @@ static int do_create_stash(struct pathspec ps, struct strbuf *stash_msg_buf,
 	struct strbuf msg = STRBUF_INIT;
 	struct strbuf commit_tree_label = STRBUF_INIT;
 	struct strbuf untracked_files = STRBUF_INIT;
+	struct strbuf stash_msg_buf = STRBUF_INIT;
 
 	prepare_fallback_ident("git stash", "git@stash");
 
@@ -1188,10 +1189,12 @@ static int do_create_stash(struct pathspec ps, struct strbuf *stash_msg_buf,
 		}
 	}
 
-	if (!stash_msg_buf->len)
-		strbuf_addf(stash_msg_buf, "WIP on %s", msg.buf);
-	else
-		strbuf_insertf(stash_msg_buf, 0, "On %s: ", branch_name);
+	if (!*stash_msg) {
+		strbuf_addf(&stash_msg_buf, "WIP on %s", msg.buf);
+	} else {
+		strbuf_addf(&stash_msg_buf, "On %s: ", branch_name);
+		strbuf_addstr(&stash_msg_buf, stash_msg);
+	}
 
 	/*
 	 * `parents` will be empty after calling `commit_tree()`, so there is
@@ -1206,7 +1209,7 @@ static int do_create_stash(struct pathspec ps, struct strbuf *stash_msg_buf,
 			   &parents);
 	commit_list_insert(head_commit, &parents);
 
-	if (commit_tree(stash_msg_buf->buf, stash_msg_buf->len, &info->w_tree,
+	if (commit_tree(stash_msg_buf.buf, stash_msg_buf.len, &info->w_tree,
 			parents, &info->w_commit, NULL, NULL)) {
 		if (!quiet)
 			fprintf_ln(stderr, _("Cannot record "
@@ -1216,6 +1219,7 @@ static int do_create_stash(struct pathspec ps, struct strbuf *stash_msg_buf,
 	}
 
 done:
+	strbuf_release(&stash_msg_buf);
 	strbuf_release(&commit_tree_label);
 	strbuf_release(&msg);
 	strbuf_release(&untracked_files);
@@ -1236,7 +1240,7 @@ static int create_stash(int argc, const char **argv, const char *prefix)
 	if (!check_changes_tracked_files(ps))
 		return 0;
 
-	if (!(ret = do_create_stash(ps, &stash_msg_buf, 0, 0, &info, NULL, 0)))
+	if (!(ret = do_create_stash(ps, stash_msg_buf.buf, 0, 0, &info, NULL, 0)))
 		printf_ln("%s", oid_to_hex(&info.w_commit));
 
 	strbuf_release(&stash_msg_buf);
@@ -1300,7 +1304,7 @@ static int do_push_stash(struct pathspec ps, const char *stash_msg, int quiet,
 
 	if (stash_msg)
 		strbuf_addstr(&stash_msg_buf, stash_msg);
-	if (do_create_stash(ps, &stash_msg_buf, include_untracked, patch_mode,
+	if (do_create_stash(ps, stash_msg_buf.buf, include_untracked, patch_mode,
 			    &info, &patch, quiet)) {
 		ret = -1;
 		goto done;