v2.25 regression: cherry-pick alters patch and leaves working tree dirty

v2.25 regression: cherry-pick alters patch and leaves working tree dirty

[Anders Kaseorg]

I ran into a problem where git cherry-pick incorrectly alters a patch that should apply cleanly, and leaves the working tree dirty despite claiming to finish successfully. I minimized the problem to the reproduction recipe below. Bisecting git.git shows the problem was introduced by 49b8133a9ece199a17db8bb2545202c6eac67485 (v2.25.0-rc0~144^2~1) “merge-recursive: fix merging a subdirectory into the root directory”, and it remains a problem in v2.32.0.

To reproduce:

git init
echo foo >foo
echo bar >bar
echo baz >baz
git add foo bar baz
git commit -m 'Initial commit'
mkdir dir
git mv foo dir/foo
git commit -am 'Move foo'
git mv bar dir/bar
echo baz >>baz
git commit -am 'Move bar'
git tag move-bar
git reset --hard move-bar~2
git cherry-pick move-bar

The rename part of the patch has been unexpectedly lost, and ‘bar’ has been unexpectedly deleted from the working tree:

$ git show
commit f06592b45db70540983a6c3e8bcf62712c694257
Author: Anders Kaseorg <andersk@mit.edu>
Date:   Sat Jun 12 00:20:13 2021 -0700

    Move bar

diff --git a/baz b/baz
index 7601807..1f55335 100644
--- a/baz
+++ b/baz
@@ -1 +1,2 @@
 baz
+baz

$ git status
On branch master
Changes not staged for commit:
  (use "git add/rm <file>..." to update what will be committed)
  (use "git restore <file>..." to discard changes in working directory)
	deleted:    bar

no changes added to commit (use "git add" and/or "git commit -a")

Before 49b8133a9e, the results are as expected:

$ git show
commit b7a2a3505dae3589203ca4469cb49e8a8e2727c3
Author: Anders Kaseorg <andersk@mit.edu>
Date:   Sat Jun 12 00:23:07 2021 -0700

    Move bar

diff --git a/baz b/baz
index 7601807..1f55335 100644
--- a/baz
+++ b/baz
@@ -1 +1,2 @@
 baz
+baz
diff --git a/bar b/dir/bar
similarity index 100%
rename from bar
rename to dir/bar

$ git status
On branch master
nothing to commit, working tree clean

Anders

Re: v2.25 regression: cherry-pick alters patch and leaves working tree dirty

[Elijah Newren]

On Sat, Jun 12, 2021 at 12:34 AM Anders Kaseorg <andersk@mit.edu> wrote:
>
> I ran into a problem where git cherry-pick incorrectly alters a patch that should apply cleanly, and leaves the working tree dirty despite claiming to finish successfully. I minimized the problem to the reproduction recipe below. Bisecting git.git shows the problem was introduced by 49b8133a9ece199a17db8bb2545202c6eac67485 (v2.25.0-rc0~144^2~1) “merge-recursive: fix merging a subdirectory into the root directory”, and it remains a problem in v2.32.0.
>
> To reproduce:
>
> git init
> echo foo >foo
> echo bar >bar
> echo baz >baz
> git add foo bar baz
> git commit -m 'Initial commit'
> mkdir dir
> git mv foo dir/foo
> git commit -am 'Move foo'
> git mv bar dir/bar
> echo baz >>baz
> git commit -am 'Move bar'
> git tag move-bar
> git reset --hard move-bar~2
> git cherry-pick move-bar
>
> The rename part of the patch has been unexpectedly lost, and ‘bar’ has been unexpectedly deleted from the working tree:
>
> $ git show
> commit f06592b45db70540983a6c3e8bcf62712c694257
> Author: Anders Kaseorg <andersk@mit.edu>
> Date:   Sat Jun 12 00:20:13 2021 -0700
>
>     Move bar
>
> diff --git a/baz b/baz
> index 7601807..1f55335 100644
> --- a/baz
> +++ b/baz
> @@ -1 +1,2 @@
>  baz
> +baz
>
> $ git status
> On branch master
> Changes not staged for commit:
>   (use "git add/rm <file>..." to update what will be committed)
>   (use "git restore <file>..." to discard changes in working directory)
>         deleted:    bar
>
> no changes added to commit (use "git add" and/or "git commit -a")
>
> Before 49b8133a9e, the results are as expected:
>
> $ git show
> commit b7a2a3505dae3589203ca4469cb49e8a8e2727c3
> Author: Anders Kaseorg <andersk@mit.edu>
> Date:   Sat Jun 12 00:23:07 2021 -0700
>
>     Move bar
>
> diff --git a/baz b/baz
> index 7601807..1f55335 100644
> --- a/baz
> +++ b/baz
> @@ -1 +1,2 @@
>  baz
> +baz
> diff --git a/bar b/dir/bar
> similarity index 100%
> rename from bar
> rename to dir/bar
>
> $ git status
> On branch master
> nothing to commit, working tree clean
>
> Anders

Thanks for the detailed report, and going the extra mile to come up
with a simple testcase.  Very helpful.

There's two bugs here, and they actually predate v2.25.  If you put
all your files in a subdirectory (e.g. 'subdir/bar' instead of 'bar'
and 'subdir/dir/bar' instead of 'dir/bar'), then the same bug
reproduces going back many more versions.  The changes in v2.25 to
support directory rename detection of a subdirectory to the root just
allowed this bug to happen at the toplevel as well.

Also, one of the two bugs affects merge-ort (in addition to merge-recursive).

I've got a one-liner fix to merge-ort.c, plus two additional lines
changed to audit for similar bugs in merge-ort and make sure they just
can't happen.  I was hoping to also have patches to fix
merge-recursive as well, but despite spending more time on it than on
merge-ort, I wasn't yet able to track down either of those two bugs.
I'll try again next week.

Re: v2.25 regression: cherry-pick alters patch and leaves working tree dirty

[Anders Kaseorg]

On 6/12/21 11:57 PM, Elijah Newren wrote:
> There's two bugs here, and they actually predate v2.25.  If you put
> all your files in a subdirectory (e.g. 'subdir/bar' instead of 'bar'
> and 'subdir/dir/bar' instead of 'dir/bar'), then the same bug
> reproduces going back many more versions.  The changes in v2.25 to
> support directory rename detection of a subdirectory to the root just
> allowed this bug to happen at the toplevel as well.

You’re right.  I’m sure you’ve already done this bisect, but for 
posterity, after that modification to the test case (‘mkdir subdir; cd 
subdir’ after ‘git init’), it reproduces back as far as 
9c0743fe1e45b3a0ffe166ac949a27f95a3e5c34 (v2.18.0-rc0~2^2~20) 
“merge-recursive: apply necessary modifications for directory renames”.

Thanks for the quick investigation!

Anders

Re: v2.25 regression: cherry-pick alters patch and leaves working tree dirty

[Elijah Newren]

On Sun, Jun 13, 2021 at 12:45 AM Anders Kaseorg <andersk@mit.edu> wrote:
>
> On 6/12/21 11:57 PM, Elijah Newren wrote:
> > There's two bugs here, and they actually predate v2.25.  If you put
> > all your files in a subdirectory (e.g. 'subdir/bar' instead of 'bar'
> > and 'subdir/dir/bar' instead of 'dir/bar'), then the same bug
> > reproduces going back many more versions.  The changes in v2.25 to
> > support directory rename detection of a subdirectory to the root just
> > allowed this bug to happen at the toplevel as well.
>
> You’re right.  I’m sure you’ve already done this bisect, but for
> posterity, after that modification to the test case (‘mkdir subdir; cd
> subdir’ after ‘git init’), it reproduces back as far as
> 9c0743fe1e45b3a0ffe166ac949a27f95a3e5c34 (v2.18.0-rc0~2^2~20)
> “merge-recursive: apply necessary modifications for directory renames”.
>
> Thanks for the quick investigation!
>
> Anders

Patches posted here:
https://lore.kernel.org/git/pull.1039.git.git.1624727121.gitgitgadget@gmail.com/