Re: [PATCH v2 04/14] rebase: do not remove untracked files on checkout

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 9, 2021, 19:09 UTC
Message-ID: <xmqqwnkdr3xb.fsf@gitster.g>
In-Reply-To: <2b499704c8f132f173c7328d5f020330481b24dc.1638975482.git.gitgitgadget@gmail.com>

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

Show 7 quoted lines

> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> If "git rebase [--apply|--merge] <upstream> <branch>" detects that
> <upstream> is an ancestor of <branch> then it will fast-forward and
> checkout <branch>. Normally a checkout or picking a commit during a
> rebase will refuse to overwrite untracked files, however rebase does
> overwrite untracked files when checking <branch>.

"when checking <branch> out", you mean?

Show 6 quoted lines

> The fix is to only set reset in `unpack_tree_opts` if flags contains
> `RESET_HEAD_HARD`. t5403 may seem like an odd home for the new test
> but it will be extended in the next commit to check that the
> post-checkout hook is not run when the checkout fails.
>
> The test for `!deatch_head` dates back to the

"detach_head"?

Show 6 quoted lines

> original implementation of reset_head() in
> ac7f467fef ("builtin/rebase: support running "git rebase <upstream>"",
> 2018-08-07) and was correct until e65123a71d
> ("builtin rebase: support `git rebase <upstream> <switch-to>`",
> 2018-09-04) started using reset_head() to checkout <switch-to> when
> fast-forwarding.

In other words, before e64123a71d, there was only one caller of reset_head(), and the caller always gave detach_head==1, so we never had .reset member set to true. So what this patch fixes was a piece of dead code that did a wrong thing but hurted nobody only because nobody called it back then. e64123a71d started to exercise the code and immediately got affected by it.

OK, and this fixes it.  Good.

Show 5 quoted lines

> Note that 480d3d6bf9 ("Change unpack_trees' 'reset' flag into an
> enum", 2021-09-27) also fixes this bug as it changes reset_head() to
> never remove untracked files. I think this fix is still worthwhile as
> it makes it clear that the same settings are used for detached and
> non-detached checkouts.

If detached-ness has nothing to do with PROTECT_UNTRACKED but the true logic is tied to "are we doing a hard reset?" (e.g. "we are doing a hard reset so we need not to care about untracked files, we are allowed to nuke them"), this change makes quite a lot of sense.

Show 11 quoted lines

> diff --git a/reset.c b/reset.c
> index 315fef91d33..3e7b9e2e131 100644
> --- a/reset.c
> +++ b/reset.c
> @@ -59,7 +59,7 @@ int reset_head(struct repository *r, struct object_id *oid, const char *action,
>  	unpack_tree_opts.merge = 1;
>  	unpack_tree_opts.preserve_ignored = 0; /* FIXME: !overwrite_ignore */
>  	init_checkout_metadata(&unpack_tree_opts.meta, switch_to_branch, oid, NULL);
> -	if (!detach_head)
> +	if (reset_hard)
>  		unpack_tree_opts.reset = UNPACK_RESET_PROTECT_UNTRACKED;

The polarity of the check goes against my intuition, though. If we are resetting hard, shouldn't we be allowed to nuke untracked paths? Perhaps it is just the naming of variables that confuses me. I dunno.

Show 22 quoted lines

>  	if (repo_read_index_unmerged(r) < 0) {
> diff --git a/t/t5403-post-checkout-hook.sh b/t/t5403-post-checkout-hook.sh
> index 17ab518f268..fd2817b4068 100755
> --- a/t/t5403-post-checkout-hook.sh
> +++ b/t/t5403-post-checkout-hook.sh
> @@ -85,6 +85,16 @@ test_rebase () {
>  		test_cmp_rev three $new &&
>  		test $flag = 1
>  	'
> +
> +	test_expect_success "rebase $args checkout does not remove untracked files" '
> +		test_when_finished "test_might_fail git rebase --abort" &&
> +		git update-ref refs/heads/rebase-fast-forward three &&
> +		git checkout two &&
> +		echo untracked >three.t &&
> +		test_when_finished "rm three.t" &&
> +		test_must_fail git rebase $args HEAD rebase-fast-forward 2>err &&
> +		grep "untracked working tree files would be overwritten by checkout" err
> +'
>  }
>  
>  test_rebase --apply &&