Re: [PATCH v3 2/2] rebase: set REF_HEAD_DETACH in checkout_up_to_date()

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Thu, Mar 17 2022, John Cai via GitGitGadget wrote:

> From: John Cai <johncai86@gmail.com>
>
> Fixes a bug whereby rebase updates the deferenced reference HEAD points
> to instead of HEAD directly.
>
> If HEAD is on main and if the following is a fast-forward operation,
>
> git rebase $(git rev-parse main) $(git rev-parse topic)
>
> Instead of HEAD being set to $(git rev-parse topic), rebase erroneously
> dereferences HEAD and sets main to $(git rev-parse topic). See [1] for
> the original bug report.
>
> The callstack from checkout_up_to_date() is the following:
>
> cmd_rebase() -> checkout_up_to_date() -> reset_head() -> update_refs()
>  -> update_ref()
>
> When <branch> is not a valid branch but a sha, rebase sets the head_name

..but an OID...

> of rebase_options to NULL. This value gets passed down this call chain
> through the branch member of reset_head_opts also getting set to NULL
> all the way to update_refs(). update_refs() checks ropts.branch to
> decide whether or not to switch brancheds. If ropts.branch is NULL, it

brancheds -> branches.

And maybe a new paragraph before "update_refs()"? I.e. "\n\nThen
update_refs() checks..." ?

> calls update_ref() to update HEAD. At this point however, from rebase's
> point of view, we want a detached HEAD. But, since checkout_up_to_date()
> does not set the RESET_HEAD_DETACH flag, the update_ref() call will
> deference HEAD and update the branch its pointing to, which in the above
> example is main.
>
> The correct behavior is that git rebase should update HEAD to $(git
> rev-parse topic) without dereferencing it.
>
> Fix this bug by adding the RESET_HEAD_DETACH flag in checkout_up_to_date
> if <branch> is not a valid branch. so that once reset_head() calls
> update_refs(), it calls update_ref() with REF_NO_DEREF which updates
> HEAD directly intead of deferencing it and updating the branch that HEAD
> points to.

But on the "tell" v.s. show... (more below)...
>
> Also add a test to ensure this behavior.
>
> 1. https://lore.kernel.org/git/xmqqsfrpbepd.fsf@gitster.g/
>
> Reported-by: Michael McClimon <michael@mcclimon.org>
> Signed-off-by: John Cai <johncai86@gmail.com>
> ---
>  builtin/rebase.c  | 5 ++++-
>  t/t3400-rebase.sh | 9 +++++++++
>  2 files changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index b29ad2b65e7..5ae7fa2a169 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -827,8 +827,11 @@ static int checkout_up_to_date(struct rebase_options *options)
>  		    getenv(GIT_REFLOG_ACTION_ENVIRONMENT),
>  		    options->switch_to);
>  	ropts.oid = &options->orig_head;
> -	ropts.branch = options->head_name;
>  	ropts.flags = RESET_HEAD_RUN_POST_CHECKOUT_HOOK;
> +	if (options->head_name)
> +		ropts.branch = options->head_name;
> +	else
> +		ropts.flags |=  RESET_HEAD_DETACH;
>  	ropts.head_msg = buf.buf;
>  	if (reset_head(the_repository, &ropts) < 0)
>  		ret = error(_("could not switch to %s"), options->switch_to);

In this case a smaller change of:

    if (!ropts.branch)
		ropts.flags |=  RESET_HEAD_DETACH;

will do the same.

I wonder if just converting it to a designated initializer while we're
at it (or a pre-cleanup commit) would be better, i.e.:

	
	diff --git a/builtin/rebase.c b/builtin/rebase.c
	index 5ae7fa2a169..bf4fd4d2920 100644
	--- a/builtin/rebase.c
	+++ b/builtin/rebase.c
	@@ -820,18 +820,18 @@ static int rebase_config(const char *var, const char *value, void *data)
	 static int checkout_up_to_date(struct rebase_options *options)
	 {
	 	struct strbuf buf = STRBUF_INIT;
	-	struct reset_head_opts ropts = { 0 };
	+	struct reset_head_opts ropts = {
	+		.oid = &options->orig_head,
	+		.branch = options->head_name,
	+		.flags = (RESET_HEAD_RUN_POST_CHECKOUT_HOOK |
	+			  (options->head_name ? 0 : RESET_HEAD_DETACH)),
	+	};
	 	int ret = 0;
	 
	 	strbuf_addf(&buf, "%s: checkout %s",
	 		    getenv(GIT_REFLOG_ACTION_ENVIRONMENT),
	 		    options->switch_to);
	-	ropts.oid = &options->orig_head;
	-	ropts.flags = RESET_HEAD_RUN_POST_CHECKOUT_HOOK;
	-	if (options->head_name)
	-		ropts.branch = options->head_name;
	-	else
	-		ropts.flags |=  RESET_HEAD_DETACH;
	+
	 	ropts.head_msg = buf.buf;
	 	if (reset_head(the_repository, &ropts) < 0)
	 		ret = error(_("could not switch to %s"), options->switch_to);

But in any case the functionality will be the same, so this is just
bikeshedding.
	
> diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
> index 0643d015255..d5a8ee39fc4 100755
> --- a/t/t3400-rebase.sh
> +++ b/t/t3400-rebase.sh
> @@ -394,6 +394,15 @@ test_expect_success 'switch to branch not checked out' '
>  	git rebase main other
>  '
>  
> +test_expect_success 'switch to non-branch detaches HEAD' '
> +	git checkout main &&
> +	old_main=$(git rev-parse HEAD) &&
> +	git rebase First Second^0 &&
> +	test_cmp_rev HEAD Second &&
> +	test_cmp_rev main $old_main &&
> +	test_must_fail git symbolic-ref HEAD
> +'
> +
>  test_expect_success 'refuse to switch to branch checked out elsewhere' '
>  	git checkout main &&
>  	git worktree add wt &&

I think it's *much* easier to review these sorts of changes where
there's a preceding commit that positively asserts what we do now, and
we'll then in the "fix" commit change the behavior.

So more "show" v.s. "tell".

I.e. in this case do the "test_cmp_rev" to the "wrong" tip with a TODO
comment or whatever, then in the fix just adjust it, then it's clear
what we had before/after.