Re: [PATCH v2 08/11] rebase: remove redundant strbuf

["René Scharfe" <l.s.r@web.de>]

Am 13.09.21 um 17:19 schrieb Phillip Wood via GitGitGadget:
> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> There is already an strbuf that can be reused for creating messages.

Reminds me of a terrible joke from elementary school: In Peter's class
everybody is called Klaus, except Franz -- his name is Michael.

Why would we want to use the same variable for multiple purposes?  This
makes the code harder to read.  And the allocation overhead for these
few cases should be negligible.

The most important question: Is this patch really needed to support
tags (the purpose of this series)?

> msg is not freed if there is an error and there is a logic error where
> we call strbuf_release(&msg) followed by strbuf_reset(&msg) and
> strbuf_addf(&msg).

strbuf_reset() after strbuf_release() is redundant but legal.

>
> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> ---
>  builtin/rebase.c | 14 ++++++--------
>  1 file changed, 6 insertions(+), 8 deletions(-)
>
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index 6138009d6e4..69a67ab1252 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -1299,7 +1299,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>  	int ret, flags, total_argc, in_progress = 0;
>  	int keep_base = 0;
>  	int ok_to_skip_pre_rebase = 0;
> -	struct strbuf msg = STRBUF_INIT;
>  	struct strbuf revisions = STRBUF_INIT;
>  	struct strbuf buf = STRBUF_INIT;
>  	struct object_id merge_base;
> @@ -2063,30 +2062,29 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>  		printf(_("First, rewinding head to replay your work on top of "
>  			 "it...\n"));
>
> -	strbuf_addf(&msg, "%s: checkout %s",
> +	strbuf_reset(&buf);
> +	strbuf_addf(&buf, "%s: checkout %s",
>  		    getenv(GIT_REFLOG_ACTION_ENVIRONMENT), options.onto_name);
>  	if (reset_head(the_repository, &options.onto->object.oid, "checkout", NULL,
>  		       RESET_HEAD_DETACH | RESET_ORIG_HEAD |
>  		       RESET_HEAD_RUN_POST_CHECKOUT_HOOK,
> -		       NULL, msg.buf, DEFAULT_REFLOG_ACTION))
> +		       NULL, buf.buf, DEFAULT_REFLOG_ACTION))
>  		die(_("Could not detach HEAD"));
> -	strbuf_release(&msg);
>
>  	/*
>  	 * If the onto is a proper descendant of the tip of the branch, then
>  	 * we just fast-forwarded.
>  	 */
> -	strbuf_reset(&msg);
> +	strbuf_reset(&buf);
>  	if (oideq(&merge_base, &options.orig_head)) {
>  		printf(_("Fast-forwarded %s to %s.\n"),
>  			branch_name, options.onto_name);
> -		strbuf_addf(&msg, "rebase finished: %s onto %s",
> +		strbuf_addf(&buf, "rebase finished: %s onto %s",
>  			options.head_name ? options.head_name : "detached HEAD",
>  			oid_to_hex(&options.onto->object.oid));
>  		reset_head(the_repository, NULL, "Fast-forwarded", options.head_name,
> -			   RESET_HEAD_REFS_ONLY, "HEAD", msg.buf,
> +			   RESET_HEAD_REFS_ONLY, "HEAD", buf.buf,
>  			   DEFAULT_REFLOG_ACTION);
> -		strbuf_release(&msg);
>  		ret = !!finish_rebase(&options);
>  		goto cleanup;
>  	}
>

msg is not released if die() is called, but that's OK; in all other
cases it _is_ released in the old code.

I'd rather see the use of that multi-purpose "buf" reduced, e.g. we
could simplify path-building like this in a few cases:

-       strbuf_reset(&buf);
-       strbuf_addf(&buf, "%s/applying", apply_dir());
-       if(file_exists(buf.buf))
+       if (file_exists(mkpath("%s/applying", apply_dir())))

Sure, this looks a bit lispy, but still better than the old code
because there is no state to carry around and reset when "buf" is
repurposed.

René