* A rebase regression in Git 2.18.0 @ 2018-08-28 12:27 Nikolay Kasyanov 2018-08-28 13:17 ` Ævar Arnfjörð Bjarmason 0 siblings, 1 reply; 20+ messages in thread From: Nikolay Kasyanov @ 2018-08-28 12:27 UTC (permalink / raw) To: git Hi, I’ve found something that may be a regression in git rebase implementation in 2.18.0. First I spotted it on macOS but I can also confirm it happening on Linux. Git 2.19.0.rc0.48.gb9dfa238d is affected too. In order to trigger it, a repo layout similar to the following is required: files/ file1 file2 file3 file4 file5 project Let’s call this state baseline. Then, in a branch, let’s edit project file and move file3 to nested/files subdirectory, here’s the final layout: files/ file1 file2 file4 file5 nested/ files/ file3 project Let’s get back to master and also edit project file to cause a conflict. After that trying to rebase the branch upon master will cause the following git status output: rebase in progress; onto baf8d2a You are currently rebasing branch 'branch' on 'baf8d2a'. (fix conflicts and then run "git rebase --continue") (use "git rebase --skip" to skip this patch) (use "git rebase --abort" to check out the original branch) Changes to be committed: (use "git reset HEAD <file>..." to unstage) renamed: files/file1 -> nested/files/file1 renamed: files/file2 -> nested/files/file2 renamed: files/file3 -> nested/files/file3 renamed: files/file4 -> nested/files/file4 renamed: files/file5 -> nested/files/file5 Unmerged paths: (use "git reset HEAD <file>..." to unstage) (use "git add <file>..." to mark resolution) both modified: project All renames except file3 are invalid and shouldn’t be here. Here’s how the output looks like produced by an older Git version (git version 2.15.1): rebase in progress; onto baf8d2a You are currently rebasing branch 'branch' on 'baf8d2a'. (fix conflicts and then run "git rebase --continue") (use "git rebase --skip" to skip this patch) (use "git rebase --abort" to check out the original branch) Changes to be committed: (use "git reset HEAD <file>..." to unstage) renamed: files/file3 -> nested/files/file3 Unmerged paths: (use "git reset HEAD <file>..." to unstage) (use "git add <file>..." to mark resolution) both modified: project Here’s a ready-to-use repository: https://github.com/nikolaykasyanov/git-rebase-bug. Regards, Nikolay Kasyanov ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: A rebase regression in Git 2.18.0 2018-08-28 12:27 A rebase regression in Git 2.18.0 Nikolay Kasyanov @ 2018-08-28 13:17 ` Ævar Arnfjörð Bjarmason 2018-08-28 13:33 ` Johannes Schindelin 0 siblings, 1 reply; 20+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2018-08-28 13:17 UTC (permalink / raw) To: Nikolay Kasyanov; +Cc: git, Elijah Newren, Stefan Beller, Junio C Hamano On Tue, Aug 28 2018, Nikolay Kasyanov wrote: > Hi, > > I’ve found something that may be a regression in git rebase implementation in 2.18.0. > First I spotted it on macOS but I can also confirm it happening on Linux. > Git 2.19.0.rc0.48.gb9dfa238d is affected too. > > In order to trigger it, a repo layout similar to the following is required: > > files/ > file1 > file2 > file3 > file4 > file5 > project > > Let’s call this state baseline. Then, in a branch, let’s edit project file and move file3 to nested/files subdirectory, here’s the final layout: > > files/ > file1 > file2 > file4 > file5 > nested/ > files/ > file3 > project > > Let’s get back to master and also edit project file to cause a conflict. After that trying to rebase the branch upon master will cause the following git status output: > > rebase in progress; onto baf8d2a > You are currently rebasing branch 'branch' on 'baf8d2a'. > (fix conflicts and then run "git rebase --continue") > (use "git rebase --skip" to skip this patch) > (use "git rebase --abort" to check out the original branch) > > Changes to be committed: > (use "git reset HEAD <file>..." to unstage) > > renamed: files/file1 -> nested/files/file1 > renamed: files/file2 -> nested/files/file2 > renamed: files/file3 -> nested/files/file3 > renamed: files/file4 -> nested/files/file4 > renamed: files/file5 -> nested/files/file5 > > Unmerged paths: > (use "git reset HEAD <file>..." to unstage) > (use "git add <file>..." to mark resolution) > > both modified: project > > All renames except file3 are invalid and shouldn’t be here. > Here’s how the output looks like produced by an older Git version (git version 2.15.1): > > rebase in progress; onto baf8d2a > You are currently rebasing branch 'branch' on 'baf8d2a'. > (fix conflicts and then run "git rebase --continue") > (use "git rebase --skip" to skip this patch) > (use "git rebase --abort" to check out the original branch) > > Changes to be committed: > (use "git reset HEAD <file>..." to unstage) > > renamed: files/file3 -> nested/files/file3 > > Unmerged paths: > (use "git reset HEAD <file>..." to unstage) > (use "git add <file>..." to mark resolution) > > both modified: project > > Here’s a ready-to-use repository: https://github.com/nikolaykasyanov/git-rebase-bug. Thanks for the test case. This bisects down to 9c0743fe1e ("merge-recursive: apply necessary modifications for directory renames", 2018-04-19) first released as part of 2.18.0. I have not dug to see if the behavior change is desired or not, that commit changed the results of a bunch of test cases, maybe it was intended. Elijah? ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: A rebase regression in Git 2.18.0 2018-08-28 13:17 ` Ævar Arnfjörð Bjarmason @ 2018-08-28 13:33 ` Johannes Schindelin 2018-08-28 13:46 ` Nikolay Kasyanov 2018-08-28 15:35 ` Elijah Newren 0 siblings, 2 replies; 20+ messages in thread From: Johannes Schindelin @ 2018-08-28 13:33 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason Cc: Nikolay Kasyanov, git, Elijah Newren, Stefan Beller, Junio C Hamano [-- Attachment #1: Type: text/plain, Size: 3138 bytes --] Hi, On Tue, 28 Aug 2018, Ævar Arnfjörð Bjarmason wrote: > On Tue, Aug 28 2018, Nikolay Kasyanov wrote: > > > I’ve found something that may be a regression in git rebase implementation in 2.18.0. > > First I spotted it on macOS but I can also confirm it happening on Linux. > > Git 2.19.0.rc0.48.gb9dfa238d is affected too. > > > > In order to trigger it, a repo layout similar to the following is required: > > > > files/ > > file1 > > file2 > > file3 > > file4 > > file5 > > project > > > > Let’s call this state baseline. Then, in a branch, let’s edit project file and move file3 to nested/files subdirectory, here’s the final layout: > > > > files/ > > file1 > > file2 > > file4 > > file5 > > nested/ > > files/ > > file3 > > project > > > > Let’s get back to master and also edit project file to cause a conflict. After that trying to rebase the branch upon master will cause the following git status output: > > > > rebase in progress; onto baf8d2a > > You are currently rebasing branch 'branch' on 'baf8d2a'. > > (fix conflicts and then run "git rebase --continue") > > (use "git rebase --skip" to skip this patch) > > (use "git rebase --abort" to check out the original branch) > > > > Changes to be committed: > > (use "git reset HEAD <file>..." to unstage) > > > > renamed: files/file1 -> nested/files/file1 > > renamed: files/file2 -> nested/files/file2 > > renamed: files/file3 -> nested/files/file3 > > renamed: files/file4 -> nested/files/file4 > > renamed: files/file5 -> nested/files/file5 > > > > Unmerged paths: > > (use "git reset HEAD <file>..." to unstage) > > (use "git add <file>..." to mark resolution) > > > > both modified: project > > > > All renames except file3 are invalid and shouldn’t be here. > > Here’s how the output looks like produced by an older Git version (git version 2.15.1): > > > > rebase in progress; onto baf8d2a > > You are currently rebasing branch 'branch' on 'baf8d2a'. > > (fix conflicts and then run "git rebase --continue") > > (use "git rebase --skip" to skip this patch) > > (use "git rebase --abort" to check out the original branch) > > > > Changes to be committed: > > (use "git reset HEAD <file>..." to unstage) > > > > renamed: files/file3 -> nested/files/file3 > > > > Unmerged paths: > > (use "git reset HEAD <file>..." to unstage) > > (use "git add <file>..." to mark resolution) > > > > both modified: project > > > > Here’s a ready-to-use repository: https://github.com/nikolaykasyanov/git-rebase-bug. > > Thanks for the test case. This bisects down to 9c0743fe1e > ("merge-recursive: apply necessary modifications for directory renames", > 2018-04-19) first released as part of 2.18.0. > > I have not dug to see if the behavior change is desired or not, that > commit changed the results of a bunch of test cases, maybe it was > intended. Elijah? I think this was already mentioned before, in a different mail thread: have you tried whether `git rebase -m` fixes that behavior? Ciao, Johannes ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: A rebase regression in Git 2.18.0 2018-08-28 13:33 ` Johannes Schindelin @ 2018-08-28 13:46 ` Nikolay Kasyanov 2018-08-28 15:35 ` Elijah Newren 1 sibling, 0 replies; 20+ messages in thread From: Nikolay Kasyanov @ 2018-08-28 13:46 UTC (permalink / raw) To: Johannes Schindelin Cc: Ævar Arnfjörð Bjarmason, git, Elijah Newren, Stefan Beller, Junio C Hamano Hi, Yes, it does fix this behavior. Could you please point me to the thread? Best, Nikolay > On 28. Aug 2018, at 15:33, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote: > > Hi, > > On Tue, 28 Aug 2018, Ævar Arnfjörð Bjarmason wrote: > >> On Tue, Aug 28 2018, Nikolay Kasyanov wrote: >> >>> I’ve found something that may be a regression in git rebase implementation in 2.18.0. >>> First I spotted it on macOS but I can also confirm it happening on Linux. >>> Git 2.19.0.rc0.48.gb9dfa238d is affected too. >>> >>> In order to trigger it, a repo layout similar to the following is required: >>> >>> files/ >>> file1 >>> file2 >>> file3 >>> file4 >>> file5 >>> project >>> >>> Let’s call this state baseline. Then, in a branch, let’s edit project file and move file3 to nested/files subdirectory, here’s the final layout: >>> >>> files/ >>> file1 >>> file2 >>> file4 >>> file5 >>> nested/ >>> files/ >>> file3 >>> project >>> >>> Let’s get back to master and also edit project file to cause a conflict. After that trying to rebase the branch upon master will cause the following git status output: >>> >>> rebase in progress; onto baf8d2a >>> You are currently rebasing branch 'branch' on 'baf8d2a'. >>> (fix conflicts and then run "git rebase --continue") >>> (use "git rebase --skip" to skip this patch) >>> (use "git rebase --abort" to check out the original branch) >>> >>> Changes to be committed: >>> (use "git reset HEAD <file>..." to unstage) >>> >>> renamed: files/file1 -> nested/files/file1 >>> renamed: files/file2 -> nested/files/file2 >>> renamed: files/file3 -> nested/files/file3 >>> renamed: files/file4 -> nested/files/file4 >>> renamed: files/file5 -> nested/files/file5 >>> >>> Unmerged paths: >>> (use "git reset HEAD <file>..." to unstage) >>> (use "git add <file>..." to mark resolution) >>> >>> both modified: project >>> >>> All renames except file3 are invalid and shouldn’t be here. >>> Here’s how the output looks like produced by an older Git version (git version 2.15.1): >>> >>> rebase in progress; onto baf8d2a >>> You are currently rebasing branch 'branch' on 'baf8d2a'. >>> (fix conflicts and then run "git rebase --continue") >>> (use "git rebase --skip" to skip this patch) >>> (use "git rebase --abort" to check out the original branch) >>> >>> Changes to be committed: >>> (use "git reset HEAD <file>..." to unstage) >>> >>> renamed: files/file3 -> nested/files/file3 >>> >>> Unmerged paths: >>> (use "git reset HEAD <file>..." to unstage) >>> (use "git add <file>..." to mark resolution) >>> >>> both modified: project >>> >>> Here’s a ready-to-use repository: https://github.com/nikolaykasyanov/git-rebase-bug. >> >> Thanks for the test case. This bisects down to 9c0743fe1e >> ("merge-recursive: apply necessary modifications for directory renames", >> 2018-04-19) first released as part of 2.18.0. >> >> I have not dug to see if the behavior change is desired or not, that >> commit changed the results of a bunch of test cases, maybe it was >> intended. Elijah? > > I think this was already mentioned before, in a different mail thread: > have you tried whether `git rebase -m` fixes that behavior? > > Ciao, > Johannes ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: A rebase regression in Git 2.18.0 2018-08-28 13:33 ` Johannes Schindelin 2018-08-28 13:46 ` Nikolay Kasyanov @ 2018-08-28 15:35 ` Elijah Newren 2018-08-28 16:58 ` Junio C Hamano 1 sibling, 1 reply; 20+ messages in thread From: Elijah Newren @ 2018-08-28 15:35 UTC (permalink / raw) To: Johannes Schindelin Cc: Ævar Arnfjörð, corrmage, Git Mailing List, Stefan Beller, Junio C Hamano On Tue, Aug 28, 2018 at 6:33 AM Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote: > On Tue, 28 Aug 2018, Ævar Arnfjörð Bjarmason wrote: > > On Tue, Aug 28 2018, Nikolay Kasyanov wrote: > > > > > I’ve found something that may be a regression in git rebase implementation in 2.18.0. > > > First I spotted it on macOS but I can also confirm it happening on Linux. > > > Git 2.19.0.rc0.48.gb9dfa238d is affected too. > > > > > > In order to trigger it, a repo layout similar to the following is required: > > > > > > files/ > > > file1 > > > file2 > > > file3 > > > file4 > > > file5 > > > project > > > > > > Let’s call this state baseline. Then, in a branch, let’s edit project file and move file3 to nested/files subdirectory, here’s the final layout: > > > > > > files/ > > > file1 > > > file2 > > > file4 > > > file5 > > > nested/ > > > files/ > > > file3 > > > project > > > > > > Let’s get back to master and also edit project file to cause a conflict. After that trying to rebase the branch upon master will cause the following git status output: > > > > > > rebase in progress; onto baf8d2a > > > You are currently rebasing branch 'branch' on 'baf8d2a'. > > > (fix conflicts and then run "git rebase --continue") > > > (use "git rebase --skip" to skip this patch) > > > (use "git rebase --abort" to check out the original branch) > > > > > > Changes to be committed: > > > (use "git reset HEAD <file>..." to unstage) > > > > > > renamed: files/file1 -> nested/files/file1 > > > renamed: files/file2 -> nested/files/file2 > > > renamed: files/file3 -> nested/files/file3 > > > renamed: files/file4 -> nested/files/file4 > > > renamed: files/file5 -> nested/files/file5 > > > > > > Unmerged paths: > > > (use "git reset HEAD <file>..." to unstage) > > > (use "git add <file>..." to mark resolution) > > > > > > both modified: project ... > > > > > > Here’s a ready-to-use repository: https://github.com/nikolaykasyanov/git-rebase-bug. > > > > Thanks for the test case. This bisects down to 9c0743fe1e > > ("merge-recursive: apply necessary modifications for directory renames", > > 2018-04-19) first released as part of 2.18.0. > > > > I have not dug to see if the behavior change is desired or not, that > > commit changed the results of a bunch of test cases, maybe it was > > intended. Elijah? > > I think this was already mentioned before, in a different mail thread: > have you tried whether `git rebase -m` fixes that behavior? I'm not aware of a previous mention, but yes, using a rebase type other than the default am one (either -m or -i) will fix this. (I did previously bring up that am-based rebase would fail to detect directory renames, due to not even calling in to the recursive merge machinery in many cases[1]. But this is an example of am-based rebase doing the opposite -- detecting a directory rename where there is none, which had never occurred to me until seeing this report.) I'm pretty sure this is a bad interaction between the build_fake_ancestor() stuff and directory rename detection. You see, you *think* the following three commits are being merged: Base: files/{file1,file2,file3,file4,file5}, project_v1 branch: files/{file1,file2,file4,file5}, nested/files/file3, project_v2 master: files/{file1,file2,file3,file4,file5}, project_v3 But the default rebase (via builtin/am) does NOT do that. It instead merges the following trees: Base: files/file3, project_v1 branch: nested/files/file3, project_v2 master: files/{file1,file2,file3,file4,file5}, project_v3 To the recursive machinery, that looks an awful lot like "branch" renamed files/ -> nested/files/, and that master just added a bunch of paths (file[1245]) into the files/ directory. From this view, what merge-recursive did was correct, it's just that rebase/am fed it information that doesn't quite match what should really be merged. Possible fixes: - Change builtin/am.c:fall_back_threeway() to use actual commit trees when available instead of building fake minimal ones. (One of the problems with am, is that the "base" commit may not exist in the current repo, so there's an issue here with threading information from rebase down to am.) - Add a flag to turn off directory rename detection, and set the flag for every call from am.c in order to avoid problems like this. The first option might be a bit nicer for the end-user, but would only help when am is called from rebase; when running `git am` directly, things would get pretty messy. So we might need the second option anyway. Since we're in -rc for 2.19, we should probably just go for the second option. I'll try to put together some patches this evening. [1] https://public-inbox.org/git/20180607171344.23331-4-newren@gmail.com/ ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: A rebase regression in Git 2.18.0 2018-08-28 15:35 ` Elijah Newren @ 2018-08-28 16:58 ` Junio C Hamano 2018-08-29 7:06 ` [PATCH 0/3] Turn off directory rename detection in am -3 Elijah Newren 2018-08-30 16:41 ` A rebase regression in Git 2.18.0 Elijah Newren 0 siblings, 2 replies; 20+ messages in thread From: Junio C Hamano @ 2018-08-28 16:58 UTC (permalink / raw) To: Elijah Newren Cc: Johannes Schindelin, Ævar Arnfjörð, corrmage, Git Mailing List, Stefan Beller Elijah Newren <newren@gmail.com> writes: > - Add a flag to turn off directory rename detection, and set the > flag for every call from am.c in order to avoid problems like this. I'd say this is the only practical solution, before you deprecate the "pipe format-patch output to am -3" style of "git rebase" (and optionally replace with something else). The whole point of "am -3" is to do _better_ than just "patch" with minimum amount of information available on the pre- and post- image blobs, without knowing the remainder of the tree that the patch did not touch. It is not surprising that the heuristics that look at the unchanging part of the tree to infer renames that may or may not exist guesses incorrectly, either with false positive or negative. In the context of "rebase", we always have all the trees that are involved. We should be able to do better than "am -3". ^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 0/3] Turn off directory rename detection in am -3 2018-08-28 16:58 ` Junio C Hamano @ 2018-08-29 7:06 ` Elijah Newren 2018-08-29 7:06 ` [PATCH 1/3] t3401: add another directory rename testcase for rebase and am Elijah Newren ` (2 more replies) 2018-08-30 16:41 ` A rebase regression in Git 2.18.0 Elijah Newren 1 sibling, 3 replies; 20+ messages in thread From: Elijah Newren @ 2018-08-29 7:06 UTC (permalink / raw) To: git; +Cc: gitster, corrmage, avarab, Johannes.Schindelin, sbeller, Elijah Newren On Tue, Aug 28, 2018 at 9:58 AM Junio C Hamano <gitster@pobox.com> wrote: > Elijah Newren <newren@gmail.com> writes: > > > - Add a flag to turn off directory rename detection, and set the > > flag for every call from am.c in order to avoid problems like this. > > I'd say this is the only practical solution, before you deprecate > the "pipe format-patch output to am -3" style of "git rebase" (and > optionally replace with something else). > > The whole point of "am -3" is to do _better_ than just "patch" with > minimum amount of information available on the pre- and post- image > blobs, without knowing the remainder of the tree that the patch did > not touch. It is not surprising that the heuristics that look at > the unchanging part of the tree to infer renames that may or may not > exist guesses incorrectly, either with false positive or negative. > In the context of "rebase", we always have all the trees that are > involved. We should be able to do better than "am -3". Here are patches to do so; they are built on the top of en/rebase-consistency, since I wanted to re-use some test code and the testfile introduced in that series, and to keep similar tests together. Elijah Newren (3): t3401: add another directory rename testcase for rebase and am merge-recursive: add ability to turn off directory rename detection am: avoid directory rename detection when calling recursive merge machinery builtin/am.c | 1 + merge-recursive.c | 18 ++++-- merge-recursive.h | 1 + t/t3401-rebase-and-am-rename.sh | 110 +++++++++++++++++++++++++++++++- 4 files changed, 124 insertions(+), 6 deletions(-) -- 2.18.0.12.g97a29da30a ^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 1/3] t3401: add another directory rename testcase for rebase and am 2018-08-29 7:06 ` [PATCH 0/3] Turn off directory rename detection in am -3 Elijah Newren @ 2018-08-29 7:06 ` Elijah Newren 2018-08-29 22:12 ` Junio C Hamano 2018-08-29 7:06 ` [PATCH 2/3] merge-recursive: add ability to turn off directory rename detection Elijah Newren 2018-08-29 7:06 ` [PATCH 3/3] am: avoid directory rename detection when calling recursive merge machinery Elijah Newren 2 siblings, 1 reply; 20+ messages in thread From: Elijah Newren @ 2018-08-29 7:06 UTC (permalink / raw) To: git; +Cc: gitster, corrmage, avarab, Johannes.Schindelin, sbeller, Elijah Newren Similar to commit 16346883ab ("t3401: add directory rename testcases for rebase and am", 2018-06-27), add another testcase for directory rename detection. This new testcase differs in that it showcases a situation where no directory rename was performed, but which some backends incorrectly detect. As with the other testcase, run this in conjunction with each of the types of rebases: git-rebase--interactive git-rebase--am git-rebase--merge and also use the same testcase for git am --3way Reported-by: Nikolay Kasyanov <corrmage@gmail.com> Signed-off-by: Elijah Newren <newren@gmail.com> --- t/t3401-rebase-and-am-rename.sh | 110 +++++++++++++++++++++++++++++++- 1 file changed, 109 insertions(+), 1 deletion(-) diff --git a/t/t3401-rebase-and-am-rename.sh b/t/t3401-rebase-and-am-rename.sh index 8f832957fc..a87df9e675 100755 --- a/t/t3401-rebase-and-am-rename.sh +++ b/t/t3401-rebase-and-am-rename.sh @@ -5,7 +5,7 @@ test_description='git rebase + directory rename tests' . ./test-lib.sh . "$TEST_DIRECTORY"/lib-rebase.sh -test_expect_success 'setup testcase' ' +test_expect_success 'setup testcase where directory rename should be detected' ' test_create_repo dir-rename && ( cd dir-rename && @@ -102,4 +102,112 @@ test_expect_failure 'am: directory rename detected' ' ) ' +test_expect_success 'setup testcase where directory rename should NOT be detected' ' + test_create_repo no-dir-rename && + ( + cd no-dir-rename && + + mkdir x && + test_seq 1 10 >x/a && + test_seq 11 20 >x/b && + test_seq 21 30 >x/c && + echo original >project_info && + git add x project_info && + git commit -m "Initial" && + + git branch O && + git branch A && + git branch B && + + git checkout A && + echo v2 >project_info && + git add project_info && + git commit -m "Modify project_info" && + + git checkout B && + mkdir y && + git mv x/c y/c && + echo v1 >project_info && + git add project_info && + git commit -m "Rename x/c to y/c, modify project_info" + ) +' + +test_expect_success 'rebase --interactive: NO directory rename' ' + test_when_finished "git -C no-dir-rename rebase --abort" && + ( + cd no-dir-rename && + + git checkout B^0 && + + set_fake_editor && + FAKE_LINES="1" test_must_fail git rebase --interactive A && + + git ls-files -s >out && + test_line_count = 6 out && + + test_path_is_file x/a && + test_path_is_file x/b && + test_path_is_missing x/c + ) +' + +test_expect_failure 'rebase (am): NO directory rename' ' + test_when_finished "git -C no-dir-rename rebase --abort" && + ( + cd no-dir-rename && + + git checkout B^0 && + + set_fake_editor && + FAKE_LINES="1" test_must_fail git rebase A && + + git ls-files -s >out && + test_line_count = 6 out && + + test_path_is_file x/a && + test_path_is_file x/b && + test_path_is_missing x/c + ) +' + +test_expect_success 'rebase --merge: NO directory rename' ' + test_when_finished "git -C no-dir-rename rebase --abort" && + ( + cd no-dir-rename && + + git checkout B^0 && + + set_fake_editor && + FAKE_LINES="1" test_must_fail git rebase --merge A && + + git ls-files -s >out && + test_line_count = 6 out && + + test_path_is_file x/a && + test_path_is_file x/b && + test_path_is_missing x/c + ) +' + +test_expect_failure 'am: NO directory rename' ' + test_when_finished "git -C no-dir-rename am --abort" && + ( + cd no-dir-rename && + + git checkout A^0 && + + git format-patch -1 B && + + test_must_fail git am --3way 0001*.patch && + + git ls-files -s >out && + test_line_count = 6 out && + + test_path_is_file x/a && + test_path_is_file x/b && + test_path_is_missing x/c + ) +' + test_done -- 2.18.0.12.g97a29da30a ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH 1/3] t3401: add another directory rename testcase for rebase and am 2018-08-29 7:06 ` [PATCH 1/3] t3401: add another directory rename testcase for rebase and am Elijah Newren @ 2018-08-29 22:12 ` Junio C Hamano 2018-08-29 23:47 ` Elijah Newren 0 siblings, 1 reply; 20+ messages in thread From: Junio C Hamano @ 2018-08-29 22:12 UTC (permalink / raw) To: Elijah Newren; +Cc: git, corrmage, avarab, Johannes.Schindelin, sbeller Elijah Newren <newren@gmail.com> writes: > +test_expect_success 'rebase --interactive: NO directory rename' ' > + test_when_finished "git -C no-dir-rename rebase --abort" && > + ( > + cd no-dir-rename && > + > + git checkout B^0 && > + > + set_fake_editor && > + FAKE_LINES="1" test_must_fail git rebase --interactive A && Is this a single-shot environment assignment? That would have been caught with the test linter. Perhaps squshing this in would be sufficient fix? t/t3401-rebase-and-am-rename.sh | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/t/t3401-rebase-and-am-rename.sh b/t/t3401-rebase-and-am-rename.sh index 94bdfbd69c..13e09afec0 100755 --- a/t/t3401-rebase-and-am-rename.sh +++ b/t/t3401-rebase-and-am-rename.sh @@ -141,7 +141,7 @@ test_expect_success 'rebase --interactive: NO directory rename' ' git checkout B^0 && set_fake_editor && - FAKE_LINES="1" test_must_fail git rebase --interactive A && + test_must_fail env FAKE_LINES="1" git rebase --interactive A && git ls-files -s >out && test_line_count = 6 out && @@ -160,7 +160,7 @@ test_expect_success 'rebase (am): NO directory rename' ' git checkout B^0 && set_fake_editor && - FAKE_LINES="1" test_must_fail git rebase A && + test_must_fail env FAKE_LINES="1" git rebase A && git ls-files -s >out && test_line_count = 6 out && @@ -179,7 +179,7 @@ test_expect_success 'rebase --merge: NO directory rename' ' git checkout B^0 && set_fake_editor && - FAKE_LINES="1" test_must_fail git rebase --merge A && + test_must_fail env FAKE_LINES="1" git rebase --merge A && git ls-files -s >out && test_line_count = 6 out && ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH 1/3] t3401: add another directory rename testcase for rebase and am 2018-08-29 22:12 ` Junio C Hamano @ 2018-08-29 23:47 ` Elijah Newren 2018-08-30 16:01 ` Junio C Hamano 0 siblings, 1 reply; 20+ messages in thread From: Elijah Newren @ 2018-08-29 23:47 UTC (permalink / raw) To: Junio C Hamano Cc: Git Mailing List, corrmage, Ævar Arnfjörð, Johannes Schindelin, Stefan Beller Hi Junio, On Wed, Aug 29, 2018 at 3:12 PM Junio C Hamano <gitster@pobox.com> wrote: > > Elijah Newren <newren@gmail.com> writes: > > > +test_expect_success 'rebase --interactive: NO directory rename' ' > > + test_when_finished "git -C no-dir-rename rebase --abort" && > > + ( > > + cd no-dir-rename && > > + > > + git checkout B^0 && > > + > > + set_fake_editor && > > + FAKE_LINES="1" test_must_fail git rebase --interactive A && > > Is this a single-shot environment assignment? That would have been > caught with the test linter. Ugh, yes. Sorry. I was trying to allow backporting to 2.18, so tried to build my series on that...but moved forward slightly to en/rebase-consistency in order to re-use the same testfile (as mentioned in my cover letter). But that meant my branch was missing a0a630192dca ("t/check-non-portable-shell: detect "FOO=bar shell_func"", 2018-07-13), so the test-linter couldn't catch this for me. > Perhaps squashing this in would be sufficient fix? Thanks for fixing it up. That works...although now I've spotted another minor issue. The FAKE_LINES setting is only needed for the interactive rebase case and can be removed for the other two. Oops. It doesn't hurt, but might confuse readers of the testcase. Would you like me to resend a fixup on top of your fixup, resend the whole series with the fixes squashed, or just ignore this final (cosmetic) issue since it doesn't affect correctness and I've caused you enough extra work already? > t/t3401-rebase-and-am-rename.sh | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/t/t3401-rebase-and-am-rename.sh b/t/t3401-rebase-and-am-rename.sh > index 94bdfbd69c..13e09afec0 100755 > --- a/t/t3401-rebase-and-am-rename.sh > +++ b/t/t3401-rebase-and-am-rename.sh > @@ -141,7 +141,7 @@ test_expect_success 'rebase --interactive: NO directory rename' ' > git checkout B^0 && > > set_fake_editor && > - FAKE_LINES="1" test_must_fail git rebase --interactive A && > + test_must_fail env FAKE_LINES="1" git rebase --interactive A && > > git ls-files -s >out && > test_line_count = 6 out && > @@ -160,7 +160,7 @@ test_expect_success 'rebase (am): NO directory rename' ' > git checkout B^0 && > > set_fake_editor && > - FAKE_LINES="1" test_must_fail git rebase A && > + test_must_fail env FAKE_LINES="1" git rebase A && > > git ls-files -s >out && > test_line_count = 6 out && > @@ -179,7 +179,7 @@ test_expect_success 'rebase --merge: NO directory rename' ' > git checkout B^0 && > > set_fake_editor && > - FAKE_LINES="1" test_must_fail git rebase --merge A && > + test_must_fail env FAKE_LINES="1" git rebase --merge A && > > git ls-files -s >out && > test_line_count = 6 out && ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/3] t3401: add another directory rename testcase for rebase and am 2018-08-29 23:47 ` Elijah Newren @ 2018-08-30 16:01 ` Junio C Hamano 2018-08-30 16:26 ` Elijah Newren 0 siblings, 1 reply; 20+ messages in thread From: Junio C Hamano @ 2018-08-30 16:01 UTC (permalink / raw) To: Elijah Newren Cc: Git Mailing List, corrmage, Ævar Arnfjörð, Johannes Schindelin, Stefan Beller Elijah Newren <newren@gmail.com> writes: >> Is this a single-shot environment assignment? That would have been >> caught with the test linter. > > Ugh, yes. Sorry. > > I was trying to allow backporting to 2.18, so tried to build my series > on that...but moved forward slightly to en/rebase-consistency in order > to re-use the same testfile (as mentioned in my cover letter). But > that meant my branch was missing a0a630192dca > ("t/check-non-portable-shell: detect "FOO=bar shell_func"", > 2018-07-13), so the test-linter couldn't catch this for me. True, I also only caught this during my integration cycle, not during the review of the patches. >> Perhaps squashing this in would be sufficient fix? > > Thanks for fixing it up. That works...although now I've spotted > another minor issue. The FAKE_LINES setting is only needed for the > interactive rebase case and can be removed for the other two. Oops. > It doesn't hurt, but might confuse readers of the testcase. Ah, OK. So the squashable fix would now become like this, all fixing the ones from the first patch. > Would you like me to resend a fixup on top of your fixup, resend the > whole series with the fixes squashed, or just ignore this final > (cosmetic) issue since it doesn't affect correctness and I've caused > you enough extra work already? No worries. This is what the maintainer does (when s/he is not playing other roles like a reviewer or an individual contributor). I'll squash in the following to the 1/3 patch and queue the topic with the other two patches. Thanks. diff --git a/t/t3401-rebase-and-am-rename.sh b/t/t3401-rebase-and-am-rename.sh index 94bdfbd69c..e0b5111993 100755 --- a/t/t3401-rebase-and-am-rename.sh +++ b/t/t3401-rebase-and-am-rename.sh @@ -141,7 +141,7 @@ test_expect_success 'rebase --interactive: NO directory rename' ' git checkout B^0 && set_fake_editor && - FAKE_LINES="1" test_must_fail git rebase --interactive A && + test_must_fail env FAKE_LINES="1" git rebase --interactive A && git ls-files -s >out && test_line_count = 6 out && @@ -160,7 +160,7 @@ test_expect_success 'rebase (am): NO directory rename' ' git checkout B^0 && set_fake_editor && - FAKE_LINES="1" test_must_fail git rebase A && + test_must_fail git rebase A && git ls-files -s >out && test_line_count = 6 out && @@ -179,7 +179,7 @@ test_expect_success 'rebase --merge: NO directory rename' ' git checkout B^0 && set_fake_editor && - FAKE_LINES="1" test_must_fail git rebase --merge A && + test_must_fail git rebase --merge A && git ls-files -s >out && test_line_count = 6 out && ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH 1/3] t3401: add another directory rename testcase for rebase and am 2018-08-30 16:01 ` Junio C Hamano @ 2018-08-30 16:26 ` Elijah Newren 0 siblings, 0 replies; 20+ messages in thread From: Elijah Newren @ 2018-08-30 16:26 UTC (permalink / raw) To: Junio C Hamano Cc: Git Mailing List, corrmage, Ævar Arnfjörð, Johannes Schindelin, Stefan Beller On Thu, Aug 30, 2018 at 9:01 AM Junio C Hamano <gitster@pobox.com> wrote: > > Elijah Newren <newren@gmail.com> writes: > > >> Is this a single-shot environment assignment? That would have been > >> caught with the test linter. > > > > Ugh, yes. Sorry. > > > > I was trying to allow backporting to 2.18, so tried to build my series > > on that...but moved forward slightly to en/rebase-consistency in order > > to re-use the same testfile (as mentioned in my cover letter). But > > that meant my branch was missing a0a630192dca > > ("t/check-non-portable-shell: detect "FOO=bar shell_func"", > > 2018-07-13), so the test-linter couldn't catch this for me. > > True, I also only caught this during my integration cycle, not > during the review of the patches. > > >> Perhaps squashing this in would be sufficient fix? > > > > Thanks for fixing it up. That works...although now I've spotted > > another minor issue. The FAKE_LINES setting is only needed for the > > interactive rebase case and can be removed for the other two. Oops. > > It doesn't hurt, but might confuse readers of the testcase. > > Ah, OK. So the squashable fix would now become like this, all > fixing the ones from the first patch. > > > Would you like me to resend a fixup on top of your fixup, resend the > > whole series with the fixes squashed, or just ignore this final > > (cosmetic) issue since it doesn't affect correctness and I've caused > > you enough extra work already? > > No worries. This is what the maintainer does (when s/he is not > playing other roles like a reviewer or an individual contributor). > > I'll squash in the following to the 1/3 patch and queue the topic > with the other two patches. Thanks, looks great. > diff --git a/t/t3401-rebase-and-am-rename.sh b/t/t3401-rebase-and-am-rename.sh > index 94bdfbd69c..e0b5111993 100755 > --- a/t/t3401-rebase-and-am-rename.sh > +++ b/t/t3401-rebase-and-am-rename.sh > @@ -141,7 +141,7 @@ test_expect_success 'rebase --interactive: NO directory rename' ' > git checkout B^0 && > > set_fake_editor && > - FAKE_LINES="1" test_must_fail git rebase --interactive A && > + test_must_fail env FAKE_LINES="1" git rebase --interactive A && > > git ls-files -s >out && > test_line_count = 6 out && > @@ -160,7 +160,7 @@ test_expect_success 'rebase (am): NO directory rename' ' > git checkout B^0 && > > set_fake_editor && > - FAKE_LINES="1" test_must_fail git rebase A && > + test_must_fail git rebase A && > > git ls-files -s >out && > test_line_count = 6 out && > @@ -179,7 +179,7 @@ test_expect_success 'rebase --merge: NO directory rename' ' > git checkout B^0 && > > set_fake_editor && > - FAKE_LINES="1" test_must_fail git rebase --merge A && > + test_must_fail git rebase --merge A && > > git ls-files -s >out && > test_line_count = 6 out && ^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 2/3] merge-recursive: add ability to turn off directory rename detection 2018-08-29 7:06 ` [PATCH 0/3] Turn off directory rename detection in am -3 Elijah Newren 2018-08-29 7:06 ` [PATCH 1/3] t3401: add another directory rename testcase for rebase and am Elijah Newren @ 2018-08-29 7:06 ` Elijah Newren 2018-08-29 12:54 ` Johannes Schindelin 2018-08-29 7:06 ` [PATCH 3/3] am: avoid directory rename detection when calling recursive merge machinery Elijah Newren 2 siblings, 1 reply; 20+ messages in thread From: Elijah Newren @ 2018-08-29 7:06 UTC (permalink / raw) To: git; +Cc: gitster, corrmage, avarab, Johannes.Schindelin, sbeller, Elijah Newren Signed-off-by: Elijah Newren <newren@gmail.com> --- merge-recursive.c | 18 +++++++++++++----- merge-recursive.h | 1 + 2 files changed, 14 insertions(+), 5 deletions(-) diff --git a/merge-recursive.c b/merge-recursive.c index f110e1c5ec..bf3cb03d3a 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -2843,12 +2843,19 @@ static int handle_renames(struct merge_options *o, head_pairs = get_diffpairs(o, common, head); merge_pairs = get_diffpairs(o, common, merge); - dir_re_head = get_directory_renames(head_pairs, head); - dir_re_merge = get_directory_renames(merge_pairs, merge); + if (o->detect_directory_renames) { + dir_re_head = get_directory_renames(head_pairs, head); + dir_re_merge = get_directory_renames(merge_pairs, merge); - handle_directory_level_conflicts(o, - dir_re_head, head, - dir_re_merge, merge); + handle_directory_level_conflicts(o, + dir_re_head, head, + dir_re_merge, merge); + } else { + dir_re_head = xmalloc(sizeof(*dir_re_head)); + dir_re_merge = xmalloc(sizeof(*dir_re_merge)); + dir_rename_init(dir_re_head); + dir_rename_init(dir_re_merge); + } ri->head_renames = get_renames(o, head_pairs, dir_re_merge, dir_re_head, head, @@ -3541,6 +3548,7 @@ void init_merge_options(struct merge_options *o) o->renormalize = 0; o->diff_detect_rename = -1; o->merge_detect_rename = -1; + o->detect_directory_renames = 1; merge_recursive_config(o); merge_verbosity = getenv("GIT_MERGE_VERBOSITY"); if (merge_verbosity) diff --git a/merge-recursive.h b/merge-recursive.h index fa7bc6b683..e39ee5d78b 100644 --- a/merge-recursive.h +++ b/merge-recursive.h @@ -18,6 +18,7 @@ struct merge_options { unsigned renormalize : 1; long xdl_opts; int verbosity; + int detect_directory_renames; int diff_detect_rename; int merge_detect_rename; int diff_rename_limit; -- 2.18.0.12.g97a29da30a ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH 2/3] merge-recursive: add ability to turn off directory rename detection 2018-08-29 7:06 ` [PATCH 2/3] merge-recursive: add ability to turn off directory rename detection Elijah Newren @ 2018-08-29 12:54 ` Johannes Schindelin 2018-08-29 23:00 ` Elijah Newren 0 siblings, 1 reply; 20+ messages in thread From: Johannes Schindelin @ 2018-08-29 12:54 UTC (permalink / raw) To: Elijah Newren; +Cc: git, gitster, corrmage, avarab, sbeller Hi Elijah, On Wed, 29 Aug 2018, Elijah Newren wrote: > Signed-off-by: Elijah Newren <newren@gmail.com> > --- > merge-recursive.c | 18 +++++++++++++----- > merge-recursive.h | 1 + > 2 files changed, 14 insertions(+), 5 deletions(-) > > diff --git a/merge-recursive.c b/merge-recursive.c > index f110e1c5ec..bf3cb03d3a 100644 > --- a/merge-recursive.c > +++ b/merge-recursive.c > @@ -2843,12 +2843,19 @@ static int handle_renames(struct merge_options *o, > head_pairs = get_diffpairs(o, common, head); > merge_pairs = get_diffpairs(o, common, merge); > > - dir_re_head = get_directory_renames(head_pairs, head); > - dir_re_merge = get_directory_renames(merge_pairs, merge); > + if (o->detect_directory_renames) { > + dir_re_head = get_directory_renames(head_pairs, head); > + dir_re_merge = get_directory_renames(merge_pairs, merge); > > - handle_directory_level_conflicts(o, > - dir_re_head, head, > - dir_re_merge, merge); > + handle_directory_level_conflicts(o, > + dir_re_head, head, > + dir_re_merge, merge); > + } else { > + dir_re_head = xmalloc(sizeof(*dir_re_head)); > + dir_re_merge = xmalloc(sizeof(*dir_re_merge)); This is not a suggestion to change anything, but a genuine question out of curiosity: would it make sense to put the `dir_re_head` and `dir_re_merge` structures into `struct merge_options` to avoid these extra `malloc()`s? Or would that cause issues with the recursive nature of the recursive merge? Ciao, Dscho > + dir_rename_init(dir_re_head); > + dir_rename_init(dir_re_merge); > + } > > ri->head_renames = get_renames(o, head_pairs, > dir_re_merge, dir_re_head, head, > @@ -3541,6 +3548,7 @@ void init_merge_options(struct merge_options *o) > o->renormalize = 0; > o->diff_detect_rename = -1; > o->merge_detect_rename = -1; > + o->detect_directory_renames = 1; > merge_recursive_config(o); > merge_verbosity = getenv("GIT_MERGE_VERBOSITY"); > if (merge_verbosity) > diff --git a/merge-recursive.h b/merge-recursive.h > index fa7bc6b683..e39ee5d78b 100644 > --- a/merge-recursive.h > +++ b/merge-recursive.h > @@ -18,6 +18,7 @@ struct merge_options { > unsigned renormalize : 1; > long xdl_opts; > int verbosity; > + int detect_directory_renames; > int diff_detect_rename; > int merge_detect_rename; > int diff_rename_limit; > -- > 2.18.0.12.g97a29da30a > > ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/3] merge-recursive: add ability to turn off directory rename detection 2018-08-29 12:54 ` Johannes Schindelin @ 2018-08-29 23:00 ` Elijah Newren 0 siblings, 0 replies; 20+ messages in thread From: Elijah Newren @ 2018-08-29 23:00 UTC (permalink / raw) To: Johannes Schindelin Cc: Git Mailing List, Junio C Hamano, corrmage, Ævar Arnfjörð, Stefan Beller On Wed, Aug 29, 2018 at 5:54 AM Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote: > > Hi Elijah, > > On Wed, 29 Aug 2018, Elijah Newren wrote: > > > Signed-off-by: Elijah Newren <newren@gmail.com> > > --- > > merge-recursive.c | 18 +++++++++++++----- > > merge-recursive.h | 1 + > > 2 files changed, 14 insertions(+), 5 deletions(-) > > > > diff --git a/merge-recursive.c b/merge-recursive.c > > index f110e1c5ec..bf3cb03d3a 100644 > > --- a/merge-recursive.c > > +++ b/merge-recursive.c > > @@ -2843,12 +2843,19 @@ static int handle_renames(struct merge_options *o, > > head_pairs = get_diffpairs(o, common, head); > > merge_pairs = get_diffpairs(o, common, merge); > > > > - dir_re_head = get_directory_renames(head_pairs, head); > > - dir_re_merge = get_directory_renames(merge_pairs, merge); > > + if (o->detect_directory_renames) { > > + dir_re_head = get_directory_renames(head_pairs, head); > > + dir_re_merge = get_directory_renames(merge_pairs, merge); > > > > - handle_directory_level_conflicts(o, > > - dir_re_head, head, > > - dir_re_merge, merge); > > + handle_directory_level_conflicts(o, > > + dir_re_head, head, > > + dir_re_merge, merge); > > + } else { > > + dir_re_head = xmalloc(sizeof(*dir_re_head)); > > + dir_re_merge = xmalloc(sizeof(*dir_re_merge)); > > This is not a suggestion to change anything, but a genuine question out of > curiosity: would it make sense to put the `dir_re_head` and `dir_re_merge` > structures into `struct merge_options` to avoid these extra `malloc()`s? > Or would that cause issues with the recursive nature of the recursive > merge? That would work to avoid the extra `malloc()`s, and be inline with the current usage of merge_options. However, I'm not sure I like the current usage of merge_options. That struct is supposed to be public API, but it's got a lot of private internal-only use stuff (and putting dir_re_head and dir_re_merge there would add more). I'm tempted to go the other way and eject some of the other internal-only stuff from merge_options (or wrap it inside an opaque struct merge_options_internal* internal field, or something like that). ^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 3/3] am: avoid directory rename detection when calling recursive merge machinery 2018-08-29 7:06 ` [PATCH 0/3] Turn off directory rename detection in am -3 Elijah Newren 2018-08-29 7:06 ` [PATCH 1/3] t3401: add another directory rename testcase for rebase and am Elijah Newren 2018-08-29 7:06 ` [PATCH 2/3] merge-recursive: add ability to turn off directory rename detection Elijah Newren @ 2018-08-29 7:06 ` Elijah Newren 2018-08-29 12:51 ` Johannes Schindelin 2 siblings, 1 reply; 20+ messages in thread From: Elijah Newren @ 2018-08-29 7:06 UTC (permalink / raw) To: git; +Cc: gitster, corrmage, avarab, Johannes.Schindelin, sbeller, Elijah Newren Let's say you have the following three trees, where Base is from one commit behind either master or branch: Base : bar_v1, foo/{file1, file2, file3} branch: bar_v2, foo/{file1, file2}, goo/file3 master: bar_v3, foo/{file1, file2, file3} Using git-am (or am-based rebase) to apply the changes from branch onto master results in the following tree: Result: bar_merged, goo/{file1, file2, file3} This is not what users want; they did not rename foo/ -> goo/, they only renamed one file within that directory. The reason this happens is am constructs fake trees (via build_fake_ancestor()) of the following form: Base_bfa : bar_v1, foo/file3 branch_bfa: bar_v2, goo/file3 Combining these two trees with master's tree: master: bar_v3, foo/{file1, file2, file3}, You can see that merge_recursive_generic() would see branch_bfa as renaming foo/ -> goo/, and master as just adding both foo/file1 and foo/file2. As such, it ends up with goo/{file1, file2, file3} The core problem is that am does not have access to the original trees; it can only construct trees using the blobs involved in the patch. As such, it is not safe to perform directory rename detection within am -3. Signed-off-by: Elijah Newren <newren@gmail.com> --- builtin/am.c | 1 + t/t3401-rebase-and-am-rename.sh | 4 ++-- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/builtin/am.c b/builtin/am.c index 2fc2d1e82c..1494a9be84 100644 --- a/builtin/am.c +++ b/builtin/am.c @@ -1596,6 +1596,7 @@ static int fall_back_threeway(const struct am_state *state, const char *index_pa o.branch1 = "HEAD"; their_tree_name = xstrfmt("%.*s", linelen(state->msg), state->msg); o.branch2 = their_tree_name; + o.detect_directory_renames = 0; if (state->quiet) o.verbosity = 0; diff --git a/t/t3401-rebase-and-am-rename.sh b/t/t3401-rebase-and-am-rename.sh index a87df9e675..94bdfbd69c 100755 --- a/t/t3401-rebase-and-am-rename.sh +++ b/t/t3401-rebase-and-am-rename.sh @@ -152,7 +152,7 @@ test_expect_success 'rebase --interactive: NO directory rename' ' ) ' -test_expect_failure 'rebase (am): NO directory rename' ' +test_expect_success 'rebase (am): NO directory rename' ' test_when_finished "git -C no-dir-rename rebase --abort" && ( cd no-dir-rename && @@ -190,7 +190,7 @@ test_expect_success 'rebase --merge: NO directory rename' ' ) ' -test_expect_failure 'am: NO directory rename' ' +test_expect_success 'am: NO directory rename' ' test_when_finished "git -C no-dir-rename am --abort" && ( cd no-dir-rename && -- 2.18.0.12.g97a29da30a ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH 3/3] am: avoid directory rename detection when calling recursive merge machinery 2018-08-29 7:06 ` [PATCH 3/3] am: avoid directory rename detection when calling recursive merge machinery Elijah Newren @ 2018-08-29 12:51 ` Johannes Schindelin 0 siblings, 0 replies; 20+ messages in thread From: Johannes Schindelin @ 2018-08-29 12:51 UTC (permalink / raw) To: Elijah Newren; +Cc: git, gitster, corrmage, avarab, sbeller Hi Elijah, On Wed, 29 Aug 2018, Elijah Newren wrote: > Let's say you have the following three trees, where Base is from one commit > behind either master or branch: > > Base : bar_v1, foo/{file1, file2, file3} > branch: bar_v2, foo/{file1, file2}, goo/file3 > master: bar_v3, foo/{file1, file2, file3} > > Using git-am (or am-based rebase) to apply the changes from branch onto > master results in the following tree: > > Result: bar_merged, goo/{file1, file2, file3} > > This is not what users want; they did not rename foo/ -> goo/, they only > renamed one file within that directory. The reason this happens is am > constructs fake trees (via build_fake_ancestor()) of the following form: > > Base_bfa : bar_v1, foo/file3 > branch_bfa: bar_v2, goo/file3 > > Combining these two trees with master's tree: > > master: bar_v3, foo/{file1, file2, file3}, > > You can see that merge_recursive_generic() would see branch_bfa as renaming > foo/ -> goo/, and master as just adding both foo/file1 and foo/file2. As > such, it ends up with goo/{file1, file2, file3} > > The core problem is that am does not have access to the original trees; it > can only construct trees using the blobs involved in the patch. As such, > it is not safe to perform directory rename detection within am -3. I read through all three patches, and they look fine to me! Ciao, Dscho > > Signed-off-by: Elijah Newren <newren@gmail.com> > --- > builtin/am.c | 1 + > t/t3401-rebase-and-am-rename.sh | 4 ++-- > 2 files changed, 3 insertions(+), 2 deletions(-) > > diff --git a/builtin/am.c b/builtin/am.c > index 2fc2d1e82c..1494a9be84 100644 > --- a/builtin/am.c > +++ b/builtin/am.c > @@ -1596,6 +1596,7 @@ static int fall_back_threeway(const struct am_state *state, const char *index_pa > o.branch1 = "HEAD"; > their_tree_name = xstrfmt("%.*s", linelen(state->msg), state->msg); > o.branch2 = their_tree_name; > + o.detect_directory_renames = 0; > > if (state->quiet) > o.verbosity = 0; > diff --git a/t/t3401-rebase-and-am-rename.sh b/t/t3401-rebase-and-am-rename.sh > index a87df9e675..94bdfbd69c 100755 > --- a/t/t3401-rebase-and-am-rename.sh > +++ b/t/t3401-rebase-and-am-rename.sh > @@ -152,7 +152,7 @@ test_expect_success 'rebase --interactive: NO directory rename' ' > ) > ' > > -test_expect_failure 'rebase (am): NO directory rename' ' > +test_expect_success 'rebase (am): NO directory rename' ' > test_when_finished "git -C no-dir-rename rebase --abort" && > ( > cd no-dir-rename && > @@ -190,7 +190,7 @@ test_expect_success 'rebase --merge: NO directory rename' ' > ) > ' > > -test_expect_failure 'am: NO directory rename' ' > +test_expect_success 'am: NO directory rename' ' > test_when_finished "git -C no-dir-rename am --abort" && > ( > cd no-dir-rename && > -- > 2.18.0.12.g97a29da30a > > ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: A rebase regression in Git 2.18.0 2018-08-28 16:58 ` Junio C Hamano 2018-08-29 7:06 ` [PATCH 0/3] Turn off directory rename detection in am -3 Elijah Newren @ 2018-08-30 16:41 ` Elijah Newren 2018-08-31 10:11 ` Johannes Schindelin 1 sibling, 1 reply; 20+ messages in thread From: Elijah Newren @ 2018-08-30 16:41 UTC (permalink / raw) To: Junio C Hamano Cc: Johannes Schindelin, Ævar Arnfjörð, corrmage, Git Mailing List, Stefan Beller On Tue, Aug 28, 2018 at 9:58 AM Junio C Hamano <gitster@pobox.com> wrote: > > Elijah Newren <newren@gmail.com> writes: > > > - Add a flag to turn off directory rename detection, and set the > > flag for every call from am.c in order to avoid problems like this. > > I'd say this is the only practical solution, before you deprecate > the "pipe format-patch output to am -3" style of "git rebase" (and > optionally replace with something else). I posted a patch a while back to add an --am flag to "git rebase", make "--am" be implied by options which are still am-specific (--whitespace, --committer-date-is-author-date, and -C), and change --merge to be the default. I'll post it as an RFC again after the various rebase-rewrite series have settled and merged down...along with my other rebase cleanups that I was waiting on to avoid conflicts with GSoC stuff. > The whole point of "am -3" is to do _better_ than just "patch" with > minimum amount of information available on the pre- and post- image > blobs, without knowing the remainder of the tree that the patch did > not touch. It is not surprising that the heuristics that look at > the unchanging part of the tree to infer renames that may or may not > exist guesses incorrectly, either with false positive or negative. > In the context of "rebase", we always have all the trees that are > involved. We should be able to do better than "am -3". ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: A rebase regression in Git 2.18.0 2018-08-30 16:41 ` A rebase regression in Git 2.18.0 Elijah Newren @ 2018-08-31 10:11 ` Johannes Schindelin 2018-08-31 19:37 ` Elijah Newren 0 siblings, 1 reply; 20+ messages in thread From: Johannes Schindelin @ 2018-08-31 10:11 UTC (permalink / raw) To: Elijah Newren Cc: Junio C Hamano, Ævar Arnfjörð, corrmage, Git Mailing List, Stefan Beller Hi Elijah, On Thu, 30 Aug 2018, Elijah Newren wrote: > On Tue, Aug 28, 2018 at 9:58 AM Junio C Hamano <gitster@pobox.com> wrote: > > > > Elijah Newren <newren@gmail.com> writes: > > > > > - Add a flag to turn off directory rename detection, and set the > > > flag for every call from am.c in order to avoid problems like this. > > > > I'd say this is the only practical solution, before you deprecate > > the "pipe format-patch output to am -3" style of "git rebase" (and > > optionally replace with something else). > > I posted a patch a while back to add an --am flag to "git rebase", > make "--am" be implied by options which are still am-specific > (--whitespace, --committer-date-is-author-date, and -C), and change > --merge to be the default. Didn't you also post a patch to fold --merge into the --interactive backend? What's your current state of thinking about this? As to switching from --am as the default: I still think that --am has serious speed advantages over --merge (or for that matter, --interactive). I have no numbers to back that up, though, and I am currently really busy with working on the CI, so I won't be able to measure these numbers, either... Also please note: I converted the `am` backend to pure C (it is waiting at https://github.com/gitgitgadget/git/pull/24, to be submitted after the v2.19.0 RC period). Switching to `--merge` as the default would force me to convert that backend, too ;-) > I'll post it as an RFC again after the various rebase-rewrite series > have settled and merged down...along with my other rebase cleanups > that I was waiting on to avoid conflicts with GSoC stuff. Thanks for waiting! Please note that I am interested, yet I will be on vacation for a couple of weeks in September. Don't let that stop you, though! > > The whole point of "am -3" is to do _better_ than just "patch" with > > minimum amount of information available on the pre- and post- image > > blobs, without knowing the remainder of the tree that the patch did > > not touch. It is not surprising that the heuristics that look at > > the unchanging part of the tree to infer renames that may or may not > > exist guesses incorrectly, either with false positive or negative. > > In the context of "rebase", we always have all the trees that are > > involved. We should be able to do better than "am -3". Right. I think that Elijah's right, and --merge is that "do better" solution. Ciao, Dscho ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: A rebase regression in Git 2.18.0 2018-08-31 10:11 ` Johannes Schindelin @ 2018-08-31 19:37 ` Elijah Newren 0 siblings, 0 replies; 20+ messages in thread From: Elijah Newren @ 2018-08-31 19:37 UTC (permalink / raw) To: Johannes Schindelin Cc: Junio C Hamano, Ævar Arnfjörð, corrmage, Git Mailing List, Stefan Beller Hi Dscho, On Fri, Aug 31, 2018 at 3:12 AM Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote: > On Thu, 30 Aug 2018, Elijah Newren wrote: > > On Tue, Aug 28, 2018 at 9:58 AM Junio C Hamano <gitster@pobox.com> wrote: > > > Elijah Newren <newren@gmail.com> writes: ... > > > I'd say this is the only practical solution, before you deprecate > > > the "pipe format-patch output to am -3" style of "git rebase" (and > > > optionally replace with something else). > > > > I posted a patch a while back to add an --am flag to "git rebase", > > make "--am" be implied by options which are still am-specific > > (--whitespace, --committer-date-is-author-date, and -C), and change > > --merge to be the default. > > Didn't you also post a patch to fold --merge into the --interactive > backend? What's your current state of thinking about this? Yes. I updated it once or twice, but it had conflicts with the GSoC projects each time, so I decided to just hold off on it a bit longer. I'm still planning to resubmit this once the GSoC projects merge down. > As to switching from --am as the default: I still think that --am has > serious speed advantages over --merge (or for that matter, --interactive). > I have no numbers to back that up, though, and I am currently really busy > with working on the CI, so I won't be able to measure these numbers, > either... Yep, we talked about this before and you mentioned that the rewrite in C should bring some performance improvements, and we agreed that merge-recursive is probably the next issue performance-wise. I think it's at least worth measuring what the approximate performance differences are with the rewrite of rebase in C, and posting an RFC with that info. If the answer comes back that we need to do more optimization before we switch the default, that's fine. > Also please note: I converted the `am` backend to pure C (it is waiting at > https://github.com/gitgitgadget/git/pull/24, to be submitted after the > v2.19.0 RC period). Switching to `--merge` as the default would force me > to convert that backend, too ;-) Not if git-rebase--merge is deleted and --merge is implemented on top of the interactive backend as an implicitly_interactive case. In fact, that's probably the simplest way to "convert" that backend to C. Anyway, since I plan to submit that change first, we should be good. > > I'll post it as an RFC again after the various rebase-rewrite series > > have settled and merged down...along with my other rebase cleanups > > that I was waiting on to avoid conflicts with GSoC stuff. > > Thanks for waiting! Please note that I am interested, yet I will be on > vacation for a couple of weeks in September. Don't let that stop you, > though! Enjoy your vacation! > > > The whole point of "am -3" is to do _better_ than just "patch" with > > > minimum amount of information available on the pre- and post- image > > > blobs, without knowing the remainder of the tree that the patch did > > > not touch. It is not surprising that the heuristics that look at > > > the unchanging part of the tree to infer renames that may or may not > > > exist guesses incorrectly, either with false positive or negative. > > > In the context of "rebase", we always have all the trees that are > > > involved. We should be able to do better than "am -3". > > Right. I think that Elijah's right, and --merge is that "do better" > solution. Cool, good to see others seem to agree on the direction I'd like to see things move. ^ permalink raw reply [flat|nested] 20+ messages in thread
end of thread, other threads:[~2018-08-31 19:38 UTC | newest] Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-08-28 12:27 A rebase regression in Git 2.18.0 Nikolay Kasyanov 2018-08-28 13:17 ` Ævar Arnfjörð Bjarmason 2018-08-28 13:33 ` Johannes Schindelin 2018-08-28 13:46 ` Nikolay Kasyanov 2018-08-28 15:35 ` Elijah Newren 2018-08-28 16:58 ` Junio C Hamano 2018-08-29 7:06 ` [PATCH 0/3] Turn off directory rename detection in am -3 Elijah Newren 2018-08-29 7:06 ` [PATCH 1/3] t3401: add another directory rename testcase for rebase and am Elijah Newren 2018-08-29 22:12 ` Junio C Hamano 2018-08-29 23:47 ` Elijah Newren 2018-08-30 16:01 ` Junio C Hamano 2018-08-30 16:26 ` Elijah Newren 2018-08-29 7:06 ` [PATCH 2/3] merge-recursive: add ability to turn off directory rename detection Elijah Newren 2018-08-29 12:54 ` Johannes Schindelin 2018-08-29 23:00 ` Elijah Newren 2018-08-29 7:06 ` [PATCH 3/3] am: avoid directory rename detection when calling recursive merge machinery Elijah Newren 2018-08-29 12:51 ` Johannes Schindelin 2018-08-30 16:41 ` A rebase regression in Git 2.18.0 Elijah Newren 2018-08-31 10:11 ` Johannes Schindelin 2018-08-31 19:37 ` Elijah Newren
Code repositories for project(s) associated with this public inbox https://80x24.org/mirrors/git.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).