Re: Aborting 'rebase main feat' removes unversioned files

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 [off-list ref] wrote:

On Sat, Sep 04 2021, Elijah Newren wrote:

quoted

On Sat, Sep 4, 2021 at 3:19 AM Jeff King [off-list ref] wrote:

quoted

On Sat, Sep 04, 2021 at 11:51:19AM +0200, Fedor Biryukov wrote:

quoted

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/ (local).
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/ (local)
2. https://lore.kernel.org/git/87ftsi68ke.fsf@evledraar.gmail.com/ (local)

quoted

[1] https://lore.kernel.org/git/YS8eEtwQvF7TaLCb@coredump.intra.peff.net/ (local)
[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)