"Phillip Wood via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes:
> From: Phillip Wood <phillip.wood@xxxxxxxxxxxxx>
>
> 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?
> 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"?
> 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.
> 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.
> 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.
> 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 &&
