Looks like a bug in git rebase main feat.
To reproduce:
git init
git commit -m 'init' --allow-empty
git checkout -b feat
echo 123 > readme.txt
git add readme.txt
git commit -m 'txt=123'
git checkout main
echo 012 > readme.txt
git rebase main feat
git rebase --abort
Expected result:
readme.txt contains 012
Actual result:
readme.txt contains 123
According to the docs, git rebase main feat is a shorthand for git checkout feat followed by git rebase main. I have checked that doing checkout and rebase separately instead of using the shorthand does not have the same issue.
Cheers!
Fedor
Looks like a bug in git rebase main feat.
To reproduce:
git init
git commit -m 'init' --allow-empty
git checkout -b feat
echo 123 > readme.txt
git add readme.txt
git commit -m 'txt=123'
git checkout main
echo 012 > readme.txt
git rebase main feat
git rebase --abort
Did you forget committing?
Expected result:
readme.txt contains 012
Actual result:
readme.txt contains 123
According to the docs, git rebase main feat is a shorthand for git checkout feat followed by git rebase main. I have checked that doing checkout and rebase separately instead of using the shorthand does not have the same issue.
I think this is non-issue (behavior as intended).
So when you say `git rebase main feat`, Git will rebase your commits in
feat on top of main. If any conflicts occur and you abort rebasing (`git
rebase --abort`), your feat branch just looks like before rebasing.
--
An old man doll... just what I always wanted! - Clara
From: Jeff King <hidden> Date: 2021-09-04 09:48:31
On Sat, Sep 04, 2021 at 01:57:11PM +0700, Bagas Sanjaya wrote:
On 04/09/21 03.33, Fedor Biryukov wrote:
quoted
Looks like a bug in git rebase main feat.
To reproduce:
git init
git commit -m 'init' --allow-empty
git checkout -b feat
echo 123 > readme.txt
git add readme.txt
git commit -m 'txt=123'
git checkout main
echo 012 > readme.txt
git rebase main feat
git rebase --abort
Did you forget committing?
I don't think so.
The point is that "readme.txt" is not a tracked file on the main branch,
and thus Git should consider it precious.
I don't think the "rebase --abort" is the problem here, though. It's the
command before:
git rebase main feat
The "feat" branch is already ahead of "main" (which has no new commits),
and so it just says:
Current branch feat is up to date.
and leaves us on the "feat" branch. But in doing so, it overwrites the
precious untracked content in the working tree.
The "git rebase --abort" command then does nothing, because there's no
rebase in progress.
-Peff
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.
-Fedor
On Sat, Sep 4, 2021 at 11:48 AM Jeff King [off-list ref] wrote:
On Sat, Sep 04, 2021 at 01:57:11PM +0700, Bagas Sanjaya wrote:
quoted
On 04/09/21 03.33, Fedor Biryukov wrote:
quoted
Looks like a bug in git rebase main feat.
To reproduce:
git init
git commit -m 'init' --allow-empty
git checkout -b feat
echo 123 > readme.txt
git add readme.txt
git commit -m 'txt=123'
git checkout main
echo 012 > readme.txt
git rebase main feat
git rebase --abort
Did you forget committing?
I don't think so.
The point is that "readme.txt" is not a tracked file on the main branch,
and thus Git should consider it precious.
I don't think the "rebase --abort" is the problem here, though. It's the
command before:
git rebase main feat
The "feat" branch is already ahead of "main" (which has no new commits),
and so it just says:
Current branch feat is up to date.
and leaves us on the "feat" branch. But in doing so, it overwrites the
precious untracked content in the working tree.
The "git rebase --abort" command then does nothing, because there's no
rebase in progress.
-Peff
Just tested it on my MacBook. To my surprise, the file got deleted by
'git rebase main feat':
550$ git --version
git version 2.33.0
551$ git init -b main
Initialized empty Git repository in
/Users/ted/workspace/git-abort-bug/test/.git/
552$ git commit -m 'init' --allow-empty
[main (root-commit) ede7bdf] init
553$ git checkout -b feat
Switched to a new branch 'feat'
554$ echo 123 > readme.txt
555$ git add readme.txt
556$ git commit -m 'txt=123'
[feat 29ac3de] txt=123
1 file changed, 1 insertion(+)
create mode 100644 readme.txt
557$ git checkout main
Switched to branch 'main'
558$ echo 012 > readme.txt
559$ git rebase main feat
Current branch feat is up to date.
560$ git rebase --abort
fatal: No rebase in progress?
On Sat, Sep 4, 2021 at 11:51 AM Fedor Biryukov [off-list ref] 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.
-Fedor
On Sat, Sep 4, 2021 at 11:48 AM Jeff King [off-list ref] wrote:
quoted
On Sat, Sep 04, 2021 at 01:57:11PM +0700, Bagas Sanjaya wrote:
quoted
On 04/09/21 03.33, Fedor Biryukov wrote:
quoted
Looks like a bug in git rebase main feat.
To reproduce:
git init
git commit -m 'init' --allow-empty
git checkout -b feat
echo 123 > readme.txt
git add readme.txt
git commit -m 'txt=123'
git checkout main
echo 012 > readme.txt
git rebase main feat
git rebase --abort
Did you forget committing?
I don't think so.
The point is that "readme.txt" is not a tracked file on the main branch,
and thus Git should consider it precious.
I don't think the "rebase --abort" is the problem here, though. It's the
command before:
git rebase main feat
The "feat" branch is already ahead of "main" (which has no new commits),
and so it just says:
Current branch feat is up to date.
and leaves us on the "feat" branch. But in doing so, it overwrites the
precious untracked content in the working tree.
The "git rebase --abort" command then does nothing, because there's no
rebase in progress.
-Peff
Here's the output from Windows, where everything works as expected.
PS> git --version
git version 2.33.0.windows.2
PS> rm -re -fo *
PS> git init -b main
Initialized empty Git repository in C:/Users/ted/test/.git/
PS> git commit -m 'init' --allow-empty
[main (root-commit) ea4422d] init
PS> git checkout -b feat
Switched to a new branch 'feat'
PS> echo 123 > readme.txt
PS> git add readme.txt
PS> git commit -m 'txt=123'
[feat 82fe34b] txt=123
1 file changed, 0 insertions(+), 0 deletions(-)
create mode 100644 readme.txt
PS> git checkout main
Switched to branch 'main'
PS> echo 012 > readme.txt
PS> git status
On branch main
Untracked files:
(use "git add <file>..." to include in what will be committed)
readme.txt
nothing added to commit but untracked files present (use "git add" to track)
PS> git rebase main 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
error: could not detach HEAD
PS> git rebase --abort
fatal: No rebase in progress?
On Sat, Sep 4, 2021 at 11:58 AM Fedor Biryukov [off-list ref] wrote:
Just tested it on my MacBook. To my surprise, the file got deleted by
'git rebase main feat':
550$ git --version
git version 2.33.0
551$ git init -b main
Initialized empty Git repository in
/Users/ted/workspace/git-abort-bug/test/.git/
552$ git commit -m 'init' --allow-empty
[main (root-commit) ede7bdf] init
553$ git checkout -b feat
Switched to a new branch 'feat'
554$ echo 123 > readme.txt
555$ git add readme.txt
556$ git commit -m 'txt=123'
[feat 29ac3de] txt=123
1 file changed, 1 insertion(+)
create mode 100644 readme.txt
557$ git checkout main
Switched to branch 'main'
558$ echo 012 > readme.txt
559$ git rebase main feat
Current branch feat is up to date.
560$ git rebase --abort
fatal: No rebase in progress?
On Sat, Sep 4, 2021 at 11:51 AM Fedor Biryukov [off-list ref] 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.
-Fedor
On Sat, Sep 4, 2021 at 11:48 AM Jeff King [off-list ref] wrote:
quoted
On Sat, Sep 04, 2021 at 01:57:11PM +0700, Bagas Sanjaya wrote:
quoted
On 04/09/21 03.33, Fedor Biryukov wrote:
quoted
Looks like a bug in git rebase main feat.
To reproduce:
git init
git commit -m 'init' --allow-empty
git checkout -b feat
echo 123 > readme.txt
git add readme.txt
git commit -m 'txt=123'
git checkout main
echo 012 > readme.txt
git rebase main feat
git rebase --abort
Did you forget committing?
I don't think so.
The point is that "readme.txt" is not a tracked file on the main branch,
and thus Git should consider it precious.
I don't think the "rebase --abort" is the problem here, though. It's the
command before:
git rebase main feat
The "feat" branch is already ahead of "main" (which has no new commits),
and so it just says:
Current branch feat is up to date.
and leaves us on the "feat" branch. But in doing so, it overwrites the
precious untracked content in the working tree.
The "git rebase --abort" command then does nothing, because there's no
rebase in progress.
-Peff
From: Jeff King <hidden> Date: 2021-09-04 10:18:04
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
From: Jeff King <hidden> Date: 2021-09-04 10:25:00
On Sat, Sep 04, 2021 at 12:03:49PM +0200, Fedor Biryukov wrote:
Here's the output from Windows, where everything works as expected.
[...]
PS> git rebase main 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
error: could not detach HEAD
Interesting that it behaves differently than the mac and linux versions.
There are some changes between Git for Windows and regular upstream Git,
but looking over the diff between the v2.33.0 releases, I don't see
anything that might cause this discrepancy.
If the problem used to occur on v2.31 and now doesn't, it might be
interesting to bisect it.
-Peff
Actually it's the same on all operating systems. I just forgot to add
a new base commit in main in the reproduction scenario - there was
nothing to rebase...
Here is the correct scenario along with the output.
+ git init -b main repo
Initialized empty Git repository in
/Users/ted/workspace/git-abort-bug/repo/.git/
+ cd repo
+ git commit -m base --allow-empty
[main (root-commit) 3299694] base
+ git checkout -b feat
Switched to a new branch 'feat'
+ echo feat >readme.txt
+ git add readme.txt
+ git commit -m txt=feat
[feat c9dc9b4] txt=feat
1 file changed, 1 insertion(+)
create mode 100644 readme.txt
+ git checkout main
Switched to branch 'main'
+ git commit -m new-base --allow-empty
[main c5c292c] new-base
+ echo precious > readme.txt
+ git rebase main feat
error: The following untracked working tree files would be overwritten by merge:
readme.txt
Please move or remove them before you merge.
Aborting
hint: Could not execute the todo command
hint:
hint: pick c9dc9b4d5aca461f1a1bedd3b99b5c8533d5ef10 txt=feat
hint:
hint: It has been rescheduled; To edit the command before continuing, please
hint: edit the todo list first:
hint:
hint: git rebase --edit-todo
hint: git rebase --continue
Could not apply c9dc9b4... txt=feat
+ cat readme.txt
precious
+ git rebase --abort
+ cat readme.txt
feat
-Fedor
On Sat, Sep 4, 2021 at 12:24 PM Jeff King [off-list ref] wrote:
On Sat, Sep 04, 2021 at 12:03:49PM +0200, Fedor Biryukov wrote:
quoted
Here's the output from Windows, where everything works as expected.
[...]
PS> git rebase main 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
error: could not detach HEAD
Interesting that it behaves differently than the mac and linux versions.
There are some changes between Git for Windows and regular upstream Git,
but looking over the diff between the v2.33.0 releases, I don't see
anything that might cause this discrepancy.
If the problem used to occur on v2.31 and now doesn't, it might be
interesting to bisect it.
-Peff
On Sat, Sep 4, 2021 at 3:19 AM Jeff King [off-list ref] wrote:
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/.
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...
[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)
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/.
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...
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/.
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...
On Sun, Sep 5, 2021 at 12: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/.
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.
Yeah, I discovered the problems by making some testcases for the
avoid-removing-current-working-directory case, and then when I was
surprised by some behaviors I saw I started adding tests for the
(mistaken?) nuking of untracked files...and then saw this bug report.
While I've also been trying to add fixes for the issues I've found,
making the testcases (trying to figure out which commands had bugs and
how to trigger them) was definitely the hardest part of the effort so
far, and likely the most valuable.
The first item, preserving ignored files, is related but different (I
was specifically discussing non-ignored untracked files above).
However on the topic of precious ignored files...both checkout and
merge have a --[no-]overwriite-ignore flag to select this behavior.
Unfortunately, the merge command only passes that flag along to the
fast-forwarding code path and ignores it for all other merge backends.
But merge-ort could trivially be made to handle it (search for the
line of code that reads "if (1/* FIXME: opts->overwrite_ignore*/)" and
note opts->overwrite_ignore isn't a flag that exists, yet), and then
it'd just need to be threaded through merge/rebase/cherry-pick/revert
toplevel commands down to that line of code. Also, my preliminary
patches touch on the ignore handling for other codepaths and make it
clear how one could support such a flag for those other cases; I'll
leave some comments in the cover letter when I submit it. Anyway, if
we were to get those other codepaths all hooked up, perhaps we could
add a core.overwrite_ignored option defaulting to true and giving
people a place to just configure this once. I would much rather a
global option than attempting to provide another class of file that
users have to configure (ignored but precious), especially as we have
had plenty of bugs and problems just dealing with the two classes of
'untracked' and 'ignored' already.