Hi Hannes,
On Mon, 4 Feb 2019, Johannes Sixt wrote:
> 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.
The only reason why there are not more callers is that I did not convert
any of the appropriate places. We have quite a few places where we
allocate a new strbuf for the sole purpose of formatting something that is
then inserted into an already existing strbuf (possibly extending the
buffer, which might require a move of the buffer just because that
temporary strbuf is in the way).
It does not sound like good practice to me to allocate things left and
right, only to reallocate something that was just allocated anyway and to
copy things into that and then release things left and right.
Ciao,
Dscho
> 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;
>
