Re: [PATCH v2 01/14] rebase: factor out checkout for up to date branch

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 9, 2021, 21:04 UTC
Message-ID: <xmqqk0gdpk0v.fsf@gitster.g>
In-Reply-To: <0e84d00572ed9a99d68ec856b779f550b2a934de.1638975482.git.gitgitgadget@gmail.com>

"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> This code is heavily indented and it will be convenient later in the
> series to have it in its own function.

Looks good in the sense that it is straight code movement without changing any behaviour that won't hurt.

Without a clear overall direction given in the cover letter, however, it is hard to judge if "being convenient later in the series" is a good thing in the first place, though.

Show 57 quoted lines

> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> ---
>  builtin/rebase.c | 33 +++++++++++++++++++--------------
>  1 file changed, 19 insertions(+), 14 deletions(-)
>
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index 34b4744e5f3..f5c37b7d4a5 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -812,6 +812,23 @@ static int rebase_config(const char *var, const char *value, void *data)
>  	return git_default_config(var, value, data);
>  }
>  
> +static int checkout_up_to_date(struct rebase_options *options)
> +{
> +	struct strbuf buf = STRBUF_INIT;
> +	int ret = 0;
> +
> +	strbuf_addf(&buf, "%s: checkout %s",
> +		    getenv(GIT_REFLOG_ACTION_ENVIRONMENT),
> +		    options->switch_to);
> +	if (reset_head(the_repository, &options->orig_head, "checkout",
> +		       options->head_name, RESET_HEAD_RUN_POST_CHECKOUT_HOOK,
> +		       NULL, buf.buf, DEFAULT_REFLOG_ACTION) < 0)
> +		ret = error(_("could not switch to %s"), options->switch_to);
> +	strbuf_release(&buf);
> +
> +	return ret;
> +}
> +
>  /*
>   * Determines whether the commits in from..to are linear, i.e. contain
>   * no merge commits. This function *expects* `from` to be an ancestor of
> @@ -1673,21 +1690,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>  		if (!(options.flags & REBASE_FORCE)) {
>  			/* Lazily switch to the target branch if needed... */
>  			if (options.switch_to) {
> -				strbuf_reset(&buf);
> -				strbuf_addf(&buf, "%s: checkout %s",
> -					    getenv(GIT_REFLOG_ACTION_ENVIRONMENT),
> -					    options.switch_to);
> -				if (reset_head(the_repository,
> -					       &options.orig_head, "checkout",
> -					       options.head_name,
> -					       RESET_HEAD_RUN_POST_CHECKOUT_HOOK,
> -					       NULL, buf.buf,
> -					       DEFAULT_REFLOG_ACTION) < 0) {
> -					ret = error(_("could not switch to "
> -							"%s"),
> -						      options.switch_to);
> +				ret = checkout_up_to_date(&options);
> +				if (ret)
>  					goto cleanup;
> -				}
>  			}
>  
>  			if (!(options.flags & REBASE_NO_QUIET))