Re: [PATCH v2 10/14] rebase: cleanup reset_head() calls

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 9, 2021, 19:26 UTC
Message-ID: <xmqqczm5r34h.fsf@gitster.g>
In-Reply-To: <5ea636009e7858e50357f0f6f8d8fa42e056db60.1638975482.git.gitgitgadget@gmail.com>

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

Show 9 quoted lines

> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> If ORIG_HEAD is not set by passing RESET_ORIG_HEAD then there is no
> need to pass anything for reflog_orig_head. In addition to the callers
> fixed in this commit move_to_original_branch() also passes
> reflog_orig_head without setting ORIG_HEAD. That caller is mistakenly
> passing the message it wants to put in the branch reflog which is not
> currently possible so we delay fixing that caller until we can pass
> the message as the branch reflog.

As I hinted elsewhere, these rules should be spelled out in a comment before "int reset_head(...)" either in reset.[ch].

For this particular one, I wonder if

 (A) we can lose RESET_ORIG_HEAD bit and use the presence of
     reflog_orig_head to mean what that bit currently means, or

 (B) we keep the current code strucure, but make it a BUG() if
     a non-NULL reflog_orig_head is given without RESET_ORIG_HEAD
     bit set.

Show 27 quoted lines

> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> ---
>  builtin/rebase.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index 3d78b5c8bef..fdd822c470f 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -675,7 +675,7 @@ static int run_am(struct rebase_options *opts)
>  
>  		reset_head(the_repository, &opts->orig_head,
>  			   opts->head_name, 0,
> -			   "HEAD", NULL, DEFAULT_REFLOG_ACTION);
> +			   NULL, NULL, DEFAULT_REFLOG_ACTION);
>  		error(_("\ngit encountered an error while preparing the "
>  			"patches to replay\n"
>  			"these revisions:\n"
> @@ -1777,7 +1777,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>  			options.head_name ? options.head_name : "detached HEAD",
>  			oid_to_hex(&options.onto->object.oid));
>  		reset_head(the_repository, NULL, options.head_name,
> -			   RESET_HEAD_REFS_ONLY, "HEAD", msg.buf, NULL);
> +			   RESET_HEAD_REFS_ONLY, NULL, msg.buf, NULL);
>  		strbuf_release(&msg);
>  		ret = finish_rebase(&options);
>  		goto cleanup;