Re: Aborting 'rebase main feat' removes unversioned files

[Fedor Biryukov <fedor.birjukov@gmail.com>]

Had a brief look at the code.
builtin/rebase.c is using rebase_head helper function defined in
rebase.c/h to do a checkout.
builtin/checkout.c does not use the rebase_head helper function.
I would expect both to use the same code defined in checkout.c/h, but
there is no such file.

On Sun, Sep 5, 2021 at 9:44 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
>
>
> On Sat, Sep 04 2021, Elijah Newren wrote:
>
> > On Sat, Sep 4, 2021 at 3:19 AM Jeff King <peff@peff.net> wrote:
> >>
> >> On Sat, Sep 04, 2021 at 11:51:19AM +0200, Fedor Biryukov wrote:
> >>
> >> > There is no way this could be the intended behavior, but the good news
> >> > is that I cannot reproduce it...
> >> > Looks like it occurs only in one git version (2.31.0.windows.1, IIRC).
> >> > And it does not occur in the latest git version: git version 2.33.0.windows.2.
> >>
> >> I think it is a bug, and it seems to reproduce easily for me (with both
> >> the current tip of master, and with v2.33.0). Here's the recipe you
> >> showed, with a little debugging at the end:
> >>
> >>   set -x
> >>   git init repo
> >>   cd repo
> >>   git commit -m base --allow-empty
> >>   git checkout -b feat
> >>   echo feat >readme.txt
> >>   git add readme.txt
> >>   git commit -m txt=feat
> >>   git checkout main
> >>   echo precious >readme.txt
> >>
> >>   cat readme.txt
> >>   git checkout feat
> >>   cat readme.txt
> >>   git rebase main feat
> >>   cat readme.txt
> >>
> >> This produces:
> >>
> >>   + cat readme.txt
> >>   precious
> >>   + git checkout feat
> >>   error: The following untracked working tree files would be overwritten by checkout:
> >>         readme.txt
> >>   Please move or remove them before you switch branches.
> >>   Aborting
> >>   + cat readme.txt
> >>   precious
> >>   + git rebase main feat
> >>   Current branch feat is up to date.
> >>   + cat readme.txt
> >>   feat
> >>
> >> So git-checkout was not willing to overwrite the untracked content, but
> >> rebase was happy to obliterate it.
> >>
> >> It does the right thing in very old versions. Bisecting, it looks like
> >> the problem arrived in 5541bd5b8f (rebase: default to using the builtin
> >> rebase, 2018-08-08). So the bug is in the conversion from the legacy
> >> shell script to C (which makes sense; the shell version was just calling
> >> "git checkout", which we know does the right thing).
> >>
> >> -Peff
> >
> > Turns out this is quite a mess.  It's also related to the "don't
> > remove empty working directories" discussion we had earlier this
> > week[1], because we assumed all relevant codepaths correctly avoided
> > deleting untracked files and directories in the way.  But they don't.
> > And rebase isn't the only offender, because this is buried in
> > unpack_trees.  In fact, it traces back to (and before)
> >     fcc387db9b ("read-tree -m -u: do not overwrite or remove untracked
> > working tree files.", 2006-05-17)
> > which has additional commentary over at
> > https://lore.kernel.org/git/7v8xp1jc9h.fsf_-_@assigned-by-dhcp.cox.net/.
> > It appears that before this time, git happily nuked untracked files
> > and considered them expendable, in basically all cases.  However, this
> > patch continued considering them as expendable whenever opts->reset
> > was true.  There wasn't much comment about it at the time for the
> > reasoning of how opts->reset was handled, though trying to read
> > between the lines perhaps Junio was trying to limit the backward
> > compatibility concerns of introducing new errors to fewer code paths?
> > Anyway, Junio did mention `read-tree --reset` explicitly, but this
> > opts->reset usage also occurs in am, checkout, reset -- and anything
> > that calls the reset_head() function including: rebase, stash,
> > sequencer.c, and add-patch.c.
> >
> > So, then...should we preserve untracked (and non-ignored) files in all
> > these cases?  This rebase case seems clear, but others might be less
> > clear.  For example, should "git reset --hard" nuke untracked files
> > (what if it's a directory of untracked files getting nuked just to
> > place a single file in the location of the directory)?  The
> > documentation isn't explicit, but after reading it I would assume that
> > untracked files should be preserved.  Since we've had bugs in "git
> > reset --hard" before, such as requiring two invocations in order to
> > clear out unmerged entries (see [2] and [3]), that also suggests that
> > this is just another bug in the same area.  But the bug has been
> > around so long that people might be expecting it; our testsuite has
> > several cases that incidentally do.  Granted, it's better to throw an
> > error and require explicit extra steps than to nuke potentially
> > important work, but some folks might be unhappy with a change here.
> > Similarly with "git checkout -f".
> >
> > And stash.c, which operates in that edge case area has tests with
> > files nuked from the cache without nuking it from the working tree
> > (causing the working tree file to be considered untracked), and then
> > attempts to have multiple tests operate on that kind of case.  Those
> > cases look a bit buggy to me for other reasons (I'm still digging),
> > but those bugs are kind of hidden by the untracked file nuking bugs,
> > so fixing the latter requires fixing the former.  And stash.c is a
> > mess of shelling out to subcommands.  Ick.
> >
> > I have some partial patches, but don't know if I'll have anything to
> > post until I figure out the stash mess...
>
> I'd just like to applaud this effort, and also suggest that the most
> useful result of any such findings would be for us to produce some new
> test in t/ showing these various cases of nuking/clobbering and other
> "non-precious" edge cases in this logic. See[1] and its linked [2] for
> references to some of the past discussions around these cases.
>
> 1. https://lore.kernel.org/git/87a6q9kacx.fsf@evledraar.gmail.com/
> 2. https://lore.kernel.org/git/87ftsi68ke.fsf@evledraar.gmail.com/
>
> > [1] https://lore.kernel.org/git/YS8eEtwQvF7TaLCb@coredump.intra.peff.net/
> > [2] 25c200a700 ("t1015: demonstrate directory/file conflict recovery
> > failures", 2018-07-31)
> > [3] ad3762042a ("read-cache: fix directory/file conflict handling in
> > read_index_unmerged()", 2018-07-31)
>