{"thread":{"id":"49234","subject":"A rebase regression in Git 2.18.0","startedAt":"2018-08-28T12:27:18Z","lastAt":"2018-08-31T19:38:13Z","messageCount":20,"participants":["Nikolay Kasyanov","Ævar Arnfjörð Bjarmason","Johannes Schindelin","Elijah Newren","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"356683","messageId":"679370A8-E477-450D-96E5-6B1869D27576@gmail.com","threadId":"49234","inReplyTo":null,"subject":"A rebase regression in Git 2.18.0","fromName":"Nikolay Kasyanov","fromEmail":"corrmage@gmail.com","sentAt":"2018-08-28T12:27:13Z","receivedAt":"2018-08-28T12:27:18Z","isPatch":false,"sender":{"key":"corrmage@gmail.com","avatar":null},"body":"Hi,\n\nI’ve found something that may be a regression in git rebase implementation in 2.18.0.\nFirst I spotted it on macOS but I can also confirm it happening on Linux.\nGit 2.19.0.rc0.48.gb9dfa238d is affected too.\n\nIn order to trigger it, a repo layout similar to the following is required:\n\nfiles/\n\tfile1\n\tfile2\n\tfile3\n\tfile4\n\tfile5\nproject\n\nLet’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:\n\nfiles/\n\tfile1\n\tfile2\n\tfile4\n\tfile5\nnested/\n\tfiles/\n\t\tfile3\nproject\n\nLet’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:\n\nrebase in progress; onto baf8d2a\nYou are currently rebasing branch 'branch' on 'baf8d2a'.\n  (fix conflicts and then run \"git rebase --continue\")\n  (use \"git rebase --skip\" to skip this patch)\n  (use \"git rebase --abort\" to check out the original branch)\n\nChanges to be committed:\n  (use \"git reset HEAD <file>...\" to unstage)\n\n\trenamed:    files/file1 -> nested/files/file1\n\trenamed:    files/file2 -> nested/files/file2\n\trenamed:    files/file3 -> nested/files/file3\n\trenamed:    files/file4 -> nested/files/file4\n\trenamed:    files/file5 -> nested/files/file5\n\nUnmerged paths:\n  (use \"git reset HEAD <file>...\" to unstage)\n  (use \"git add <file>...\" to mark resolution)\n\n\tboth modified:   project\n\nAll renames except file3 are invalid and shouldn’t be here.\nHere’s how the output looks like produced by an older Git version (git version 2.15.1):\n\nrebase in progress; onto baf8d2a\nYou are currently rebasing branch 'branch' on 'baf8d2a'.\n  (fix conflicts and then run \"git rebase --continue\")\n  (use \"git rebase --skip\" to skip this patch)\n  (use \"git rebase --abort\" to check out the original branch)\n\nChanges to be committed:\n  (use \"git reset HEAD <file>...\" to unstage)\n\n\trenamed:    files/file3 -> nested/files/file3\n\nUnmerged paths:\n  (use \"git reset HEAD <file>...\" to unstage)\n  (use \"git add <file>...\" to mark resolution)\n\n\tboth modified:   project\n\nHere’s a ready-to-use repository: https://github.com/nikolaykasyanov/git-rebase-bug.\n\nRegards,\nNikolay Kasyanov\n\n"},{"id":"356689","messageId":"87in3ueiks.fsf@evledraar.gmail.com","threadId":"49234","inReplyTo":"679370A8-E477-450D-96E5-6B1869D27576@gmail.com","subject":"Re: A rebase regression in Git 2.18.0","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-08-28T13:17:39Z","receivedAt":"2018-08-28T13:17:44Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Aug 28 2018, Nikolay Kasyanov wrote:\n\n> Hi,\n>\n> I’ve found something that may be a regression in git rebase implementation in 2.18.0.\n> First I spotted it on macOS but I can also confirm it happening on Linux.\n> Git 2.19.0.rc0.48.gb9dfa238d is affected too.\n>\n> In order to trigger it, a repo layout similar to the following is required:\n>\n> files/\n> \tfile1\n> \tfile2\n> \tfile3\n> \tfile4\n> \tfile5\n> project\n>\n> 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:\n>\n> files/\n> \tfile1\n> \tfile2\n> \tfile4\n> \tfile5\n> nested/\n> \tfiles/\n> \t\tfile3\n> project\n>\n> 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:\n>\n> rebase in progress; onto baf8d2a\n> You are currently rebasing branch 'branch' on 'baf8d2a'.\n>   (fix conflicts and then run \"git rebase --continue\")\n>   (use \"git rebase --skip\" to skip this patch)\n>   (use \"git rebase --abort\" to check out the original branch)\n>\n> Changes to be committed:\n>   (use \"git reset HEAD <file>...\" to unstage)\n>\n> \trenamed:    files/file1 -> nested/files/file1\n> \trenamed:    files/file2 -> nested/files/file2\n> \trenamed:    files/file3 -> nested/files/file3\n> \trenamed:    files/file4 -> nested/files/file4\n> \trenamed:    files/file5 -> nested/files/file5\n>\n> Unmerged paths:\n>   (use \"git reset HEAD <file>...\" to unstage)\n>   (use \"git add <file>...\" to mark resolution)\n>\n> \tboth modified:   project\n>\n> All renames except file3 are invalid and shouldn’t be here.\n> Here’s how the output looks like produced by an older Git version (git version 2.15.1):\n>\n> rebase in progress; onto baf8d2a\n> You are currently rebasing branch 'branch' on 'baf8d2a'.\n>   (fix conflicts and then run \"git rebase --continue\")\n>   (use \"git rebase --skip\" to skip this patch)\n>   (use \"git rebase --abort\" to check out the original branch)\n>\n> Changes to be committed:\n>   (use \"git reset HEAD <file>...\" to unstage)\n>\n> \trenamed:    files/file3 -> nested/files/file3\n>\n> Unmerged paths:\n>   (use \"git reset HEAD <file>...\" to unstage)\n>   (use \"git add <file>...\" to mark resolution)\n>\n> \tboth modified:   project\n>\n> Here’s a ready-to-use repository: https://github.com/nikolaykasyanov/git-rebase-bug.\n\nThanks for the test case. This bisects down to 9c0743fe1e\n(\"merge-recursive: apply necessary modifications for directory renames\",\n2018-04-19) first released as part of 2.18.0.\n\nI have not dug to see if the behavior change is desired or not, that\ncommit changed the results of a bunch of test cases, maybe it was\nintended. Elijah?\n"},{"id":"356693","messageId":"nycvar.QRO.7.76.6.1808281532220.16620@tvgsbejvaqbjf.bet","threadId":"49234","inReplyTo":"87in3ueiks.fsf@evledraar.gmail.com","subject":"Re: A rebase regression in Git 2.18.0","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-08-28T13:33:24Z","receivedAt":"2018-08-28T13:33:31Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Tue, 28 Aug 2018, Ævar Arnfjörð Bjarmason wrote:\n\n> On Tue, Aug 28 2018, Nikolay Kasyanov wrote:\n> \n> > I’ve found something that may be a regression in git rebase implementation in 2.18.0.\n> > First I spotted it on macOS but I can also confirm it happening on Linux.\n> > Git 2.19.0.rc0.48.gb9dfa238d is affected too.\n> >\n> > In order to trigger it, a repo layout similar to the following is required:\n> >\n> > files/\n> > \tfile1\n> > \tfile2\n> > \tfile3\n> > \tfile4\n> > \tfile5\n> > project\n> >\n> > 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:\n> >\n> > files/\n> > \tfile1\n> > \tfile2\n> > \tfile4\n> > \tfile5\n> > nested/\n> > \tfiles/\n> > \t\tfile3\n> > project\n> >\n> > 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:\n> >\n> > rebase in progress; onto baf8d2a\n> > You are currently rebasing branch 'branch' on 'baf8d2a'.\n> >   (fix conflicts and then run \"git rebase --continue\")\n> >   (use \"git rebase --skip\" to skip this patch)\n> >   (use \"git rebase --abort\" to check out the original branch)\n> >\n> > Changes to be committed:\n> >   (use \"git reset HEAD <file>...\" to unstage)\n> >\n> > \trenamed:    files/file1 -> nested/files/file1\n> > \trenamed:    files/file2 -> nested/files/file2\n> > \trenamed:    files/file3 -> nested/files/file3\n> > \trenamed:    files/file4 -> nested/files/file4\n> > \trenamed:    files/file5 -> nested/files/file5\n> >\n> > Unmerged paths:\n> >   (use \"git reset HEAD <file>...\" to unstage)\n> >   (use \"git add <file>...\" to mark resolution)\n> >\n> > \tboth modified:   project\n> >\n> > All renames except file3 are invalid and shouldn’t be here.\n> > Here’s how the output looks like produced by an older Git version (git version 2.15.1):\n> >\n> > rebase in progress; onto baf8d2a\n> > You are currently rebasing branch 'branch' on 'baf8d2a'.\n> >   (fix conflicts and then run \"git rebase --continue\")\n> >   (use \"git rebase --skip\" to skip this patch)\n> >   (use \"git rebase --abort\" to check out the original branch)\n> >\n> > Changes to be committed:\n> >   (use \"git reset HEAD <file>...\" to unstage)\n> >\n> > \trenamed:    files/file3 -> nested/files/file3\n> >\n> > Unmerged paths:\n> >   (use \"git reset HEAD <file>...\" to unstage)\n> >   (use \"git add <file>...\" to mark resolution)\n> >\n> > \tboth modified:   project\n> >\n> > Here’s a ready-to-use repository: https://github.com/nikolaykasyanov/git-rebase-bug.\n> \n> Thanks for the test case. This bisects down to 9c0743fe1e\n> (\"merge-recursive: apply necessary modifications for directory renames\",\n> 2018-04-19) first released as part of 2.18.0.\n> \n> I have not dug to see if the behavior change is desired or not, that\n> commit changed the results of a bunch of test cases, maybe it was\n> intended. Elijah?\n\nI think this was already mentioned before, in a different mail thread:\nhave you tried whether `git rebase -m` fixes that behavior?\n\nCiao,\nJohannes"},{"id":"356694","messageId":"FDE5DF36-1E32-4FC0-A206-81FAA1A962E4@gmail.com","threadId":"49234","inReplyTo":"nycvar.QRO.7.76.6.1808281532220.16620@tvgsbejvaqbjf.bet","subject":"Re: A rebase regression in Git 2.18.0","fromName":"Nikolay Kasyanov","fromEmail":"corrmage@gmail.com","sentAt":"2018-08-28T13:46:12Z","receivedAt":"2018-08-28T13:46:18Z","isPatch":false,"sender":{"key":"corrmage@gmail.com","avatar":null},"body":"Hi,\n\nYes, it does fix this behavior. Could you please point me to the thread?\n\nBest,\nNikolay\n> On 28. Aug 2018, at 15:33, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:\n> \n> Hi,\n> \n> On Tue, 28 Aug 2018, Ævar Arnfjörð Bjarmason wrote:\n> \n>> On Tue, Aug 28 2018, Nikolay Kasyanov wrote:\n>> \n>>> I’ve found something that may be a regression in git rebase implementation in 2.18.0.\n>>> First I spotted it on macOS but I can also confirm it happening on Linux.\n>>> Git 2.19.0.rc0.48.gb9dfa238d is affected too.\n>>> \n>>> In order to trigger it, a repo layout similar to the following is required:\n>>> \n>>> files/\n>>> \tfile1\n>>> \tfile2\n>>> \tfile3\n>>> \tfile4\n>>> \tfile5\n>>> project\n>>> \n>>> 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:\n>>> \n>>> files/\n>>> \tfile1\n>>> \tfile2\n>>> \tfile4\n>>> \tfile5\n>>> nested/\n>>> \tfiles/\n>>> \t\tfile3\n>>> project\n>>> \n>>> 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:\n>>> \n>>> rebase in progress; onto baf8d2a\n>>> You are currently rebasing branch 'branch' on 'baf8d2a'.\n>>>  (fix conflicts and then run \"git rebase --continue\")\n>>>  (use \"git rebase --skip\" to skip this patch)\n>>>  (use \"git rebase --abort\" to check out the original branch)\n>>> \n>>> Changes to be committed:\n>>>  (use \"git reset HEAD <file>...\" to unstage)\n>>> \n>>> \trenamed:    files/file1 -> nested/files/file1\n>>> \trenamed:    files/file2 -> nested/files/file2\n>>> \trenamed:    files/file3 -> nested/files/file3\n>>> \trenamed:    files/file4 -> nested/files/file4\n>>> \trenamed:    files/file5 -> nested/files/file5\n>>> \n>>> Unmerged paths:\n>>>  (use \"git reset HEAD <file>...\" to unstage)\n>>>  (use \"git add <file>...\" to mark resolution)\n>>> \n>>> \tboth modified:   project\n>>> \n>>> All renames except file3 are invalid and shouldn’t be here.\n>>> Here’s how the output looks like produced by an older Git version (git version 2.15.1):\n>>> \n>>> rebase in progress; onto baf8d2a\n>>> You are currently rebasing branch 'branch' on 'baf8d2a'.\n>>>  (fix conflicts and then run \"git rebase --continue\")\n>>>  (use \"git rebase --skip\" to skip this patch)\n>>>  (use \"git rebase --abort\" to check out the original branch)\n>>> \n>>> Changes to be committed:\n>>>  (use \"git reset HEAD <file>...\" to unstage)\n>>> \n>>> \trenamed:    files/file3 -> nested/files/file3\n>>> \n>>> Unmerged paths:\n>>>  (use \"git reset HEAD <file>...\" to unstage)\n>>>  (use \"git add <file>...\" to mark resolution)\n>>> \n>>> \tboth modified:   project\n>>> \n>>> Here’s a ready-to-use repository: https://github.com/nikolaykasyanov/git-rebase-bug.\n>> \n>> Thanks for the test case. This bisects down to 9c0743fe1e\n>> (\"merge-recursive: apply necessary modifications for directory renames\",\n>> 2018-04-19) first released as part of 2.18.0.\n>> \n>> I have not dug to see if the behavior change is desired or not, that\n>> commit changed the results of a bunch of test cases, maybe it was\n>> intended. Elijah?\n> \n> I think this was already mentioned before, in a different mail thread:\n> have you tried whether `git rebase -m` fixes that behavior?\n> \n> Ciao,\n> Johannes\n\n"},{"id":"356704","messageId":"CABPp-BENB=mqfFU4FGb2OS9VDV=9VdT71HhFLZwtyxD8MpdTMQ@mail.gmail.com","threadId":"49234","inReplyTo":"nycvar.QRO.7.76.6.1808281532220.16620@tvgsbejvaqbjf.bet","subject":"Re: A rebase regression in Git 2.18.0","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-08-28T15:35:03Z","receivedAt":"2018-08-28T15:35:19Z","isPatch":false,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Tue, Aug 28, 2018 at 6:33 AM Johannes Schindelin\n<Johannes.Schindelin@gmx.de> wrote:\n> On Tue, 28 Aug 2018, Ævar Arnfjörð Bjarmason wrote:\n> > On Tue, Aug 28 2018, Nikolay Kasyanov wrote:\n> >\n> > > I’ve found something that may be a regression in git rebase implementation in 2.18.0.\n> > > First I spotted it on macOS but I can also confirm it happening on Linux.\n> > > Git 2.19.0.rc0.48.gb9dfa238d is affected too.\n> > >\n> > > In order to trigger it, a repo layout similar to the following is required:\n> > >\n> > > files/\n> > >     file1\n> > >     file2\n> > >     file3\n> > >     file4\n> > >     file5\n> > > project\n> > >\n> > > 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:\n> > >\n> > > files/\n> > >     file1\n> > >     file2\n> > >     file4\n> > >     file5\n> > > nested/\n> > >     files/\n> > >             file3\n> > > project\n> > >\n> > > 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:\n> > >\n> > > rebase in progress; onto baf8d2a\n> > > You are currently rebasing branch 'branch' on 'baf8d2a'.\n> > >   (fix conflicts and then run \"git rebase --continue\")\n> > >   (use \"git rebase --skip\" to skip this patch)\n> > >   (use \"git rebase --abort\" to check out the original branch)\n> > >\n> > > Changes to be committed:\n> > >   (use \"git reset HEAD <file>...\" to unstage)\n> > >\n> > >     renamed:    files/file1 -> nested/files/file1\n> > >     renamed:    files/file2 -> nested/files/file2\n> > >     renamed:    files/file3 -> nested/files/file3\n> > >     renamed:    files/file4 -> nested/files/file4\n> > >     renamed:    files/file5 -> nested/files/file5\n> > >\n> > > Unmerged paths:\n> > >   (use \"git reset HEAD <file>...\" to unstage)\n> > >   (use \"git add <file>...\" to mark resolution)\n> > >\n> > >     both modified:   project\n...\n> > >\n> > > Here’s a ready-to-use repository: https://github.com/nikolaykasyanov/git-rebase-bug.\n> >\n> > Thanks for the test case. This bisects down to 9c0743fe1e\n> > (\"merge-recursive: apply necessary modifications for directory renames\",\n> > 2018-04-19) first released as part of 2.18.0.\n> >\n> > I have not dug to see if the behavior change is desired or not, that\n> > commit changed the results of a bunch of test cases, maybe it was\n> > intended. Elijah?\n>\n> I think this was already mentioned before, in a different mail thread:\n> have you tried whether `git rebase -m` fixes that behavior?\n\nI'm not aware of a previous mention, but yes, using a rebase type\nother than the default am one (either -m or -i) will fix this.  (I did\npreviously bring up that am-based rebase would fail to detect\ndirectory renames, due to not even calling in to the recursive merge\nmachinery in many cases[1].  But this is an example of am-based rebase\ndoing the opposite -- detecting a directory rename where there is\nnone, which had never occurred to me until seeing this report.)\n\n\nI'm pretty sure this is a bad interaction between the\nbuild_fake_ancestor() stuff and directory rename detection.  You see,\nyou *think* the following three commits are being merged:\n\nBase: files/{file1,file2,file3,file4,file5}, project_v1\nbranch: files/{file1,file2,file4,file5}, nested/files/file3, project_v2\nmaster: files/{file1,file2,file3,file4,file5}, project_v3\n\nBut the default rebase (via builtin/am) does NOT do that.  It instead\nmerges the following trees:\n\nBase: files/file3, project_v1\nbranch: nested/files/file3, project_v2\nmaster: files/{file1,file2,file3,file4,file5}, project_v3\n\n\nTo the recursive machinery, that looks an awful lot like \"branch\"\nrenamed files/ -> nested/files/, and that master just added a bunch of\npaths (file[1245]) into the files/ directory.  From this view, what\nmerge-recursive did was correct, it's just that rebase/am fed it\ninformation that doesn't quite match what should really be merged.\n\nPossible fixes:\n  - Change builtin/am.c:fall_back_threeway() to use actual commit\ntrees when available instead of building fake minimal ones.  (One of\nthe problems with am, is that the \"base\" commit may not exist in the\ncurrent repo, so there's an issue here with threading information from\nrebase down to am.)\n  - Add a flag to turn off directory rename detection, and set the\nflag for every call from am.c in order to avoid problems like this.\n\nThe first option might be a bit nicer for the end-user, but would only\nhelp when am is called from rebase; when running `git am` directly,\nthings would get pretty messy.  So we might need the second option\nanyway.  Since we're in -rc for 2.19, we should probably just go for\nthe second option.  I'll try to put together some patches this\nevening.\n\n\n[1] https://public-inbox.org/git/20180607171344.23331-4-newren@gmail.com/\n"},{"id":"356708","messageId":"xmqqh8jeh1id.fsf@gitster-ct.c.googlers.com","threadId":"49234","inReplyTo":"CABPp-BENB=mqfFU4FGb2OS9VDV=9VdT71HhFLZwtyxD8MpdTMQ@mail.gmail.com","subject":"Re: A rebase regression in Git 2.18.0","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-08-28T16:58:02Z","receivedAt":"2018-08-28T16:58:07Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren <newren@gmail.com> writes:\n\n>   - Add a flag to turn off directory rename detection, and set the\n> flag for every call from am.c in order to avoid problems like this.\n\nI'd say this is the only practical solution, before you deprecate\nthe \"pipe format-patch output to am -3\" style of \"git rebase\" (and\noptionally replace with something else).\n\nThe whole point of \"am -3\" is to do _better_ than just \"patch\" with\nminimum amount of information available on the pre- and post- image\nblobs, without knowing the remainder of the tree that the patch did\nnot touch.  It is not surprising that the heuristics that look at\nthe unchanging part of the tree to infer renames that may or may not\nexist guesses incorrectly, either with false positive or negative.\nIn the context of \"rebase\", we always have all the trees that are\ninvolved.  We should be able to do better than \"am -3\".\n"},{"id":"356814","messageId":"20180829070613.11793-1-newren@gmail.com","threadId":"49234","inReplyTo":"xmqqh8jeh1id.fsf@gitster-ct.c.googlers.com","subject":"[PATCH 0/3] Turn off directory rename detection in am -3","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-08-29T07:06:10Z","receivedAt":"2018-08-29T07:06:24Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Tue, Aug 28, 2018 at 9:58 AM Junio C Hamano <gitster@pobox.com> wrote:\n> Elijah Newren <newren@gmail.com> writes:\n>\n> >   - Add a flag to turn off directory rename detection, and set the\n> > flag for every call from am.c in order to avoid problems like this.\n>\n> I'd say this is the only practical solution, before you deprecate\n> the \"pipe format-patch output to am -3\" style of \"git rebase\" (and\n> optionally replace with something else).\n>\n> The whole point of \"am -3\" is to do _better_ than just \"patch\" with\n> minimum amount of information available on the pre- and post- image\n> blobs, without knowing the remainder of the tree that the patch did\n> not touch.  It is not surprising that the heuristics that look at\n> the unchanging part of the tree to infer renames that may or may not\n> exist guesses incorrectly, either with false positive or negative.\n> In the context of \"rebase\", we always have all the trees that are\n> involved.  We should be able to do better than \"am -3\".\n\nHere are patches to do so; they are built on the top of\nen/rebase-consistency, since I wanted to re-use some test code and the\ntestfile introduced in that series, and to keep similar tests\ntogether.\n\nElijah Newren (3):\n  t3401: add another directory rename testcase for rebase and am\n  merge-recursive: add ability to turn off directory rename detection\n  am: avoid directory rename detection when calling recursive merge\n    machinery\n\n builtin/am.c                    |   1 +\n merge-recursive.c               |  18 ++++--\n merge-recursive.h               |   1 +\n t/t3401-rebase-and-am-rename.sh | 110 +++++++++++++++++++++++++++++++-\n 4 files changed, 124 insertions(+), 6 deletions(-)\n\n-- \n2.18.0.12.g97a29da30a\n"},{"id":"356815","messageId":"20180829070613.11793-2-newren@gmail.com","threadId":"49234","inReplyTo":"20180829070613.11793-1-newren@gmail.com","subject":"[PATCH 1/3] t3401: add another directory rename testcase for rebase and am","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-08-29T07:06:11Z","receivedAt":"2018-08-29T07:06:24Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Similar to commit 16346883ab (\"t3401: add directory rename testcases for\nrebase and am\", 2018-06-27), add another testcase for directory rename\ndetection.  This new testcase differs in that it showcases a situation\nwhere no directory rename was performed, but which some backends\nincorrectly detect.\n\nAs with the other testcase, run this in conjunction with each of the\ntypes of rebases:\n  git-rebase--interactive\n  git-rebase--am\n  git-rebase--merge\nand also use the same testcase for\n  git am --3way\n\nReported-by: Nikolay Kasyanov <corrmage@gmail.com>\nSigned-off-by: Elijah Newren <newren@gmail.com>\n---\n t/t3401-rebase-and-am-rename.sh | 110 +++++++++++++++++++++++++++++++-\n 1 file changed, 109 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t3401-rebase-and-am-rename.sh b/t/t3401-rebase-and-am-rename.sh\nindex 8f832957fc..a87df9e675 100755\n--- a/t/t3401-rebase-and-am-rename.sh\n+++ b/t/t3401-rebase-and-am-rename.sh\n@@ -5,7 +5,7 @@ test_description='git rebase + directory rename tests'\n . ./test-lib.sh\n . \"$TEST_DIRECTORY\"/lib-rebase.sh\n \n-test_expect_success 'setup testcase' '\n+test_expect_success 'setup testcase where directory rename should be detected' '\n \ttest_create_repo dir-rename &&\n \t(\n \t\tcd dir-rename &&\n@@ -102,4 +102,112 @@ test_expect_failure 'am: directory rename detected' '\n \t)\n '\n \n+test_expect_success 'setup testcase where directory rename should NOT be detected' '\n+\ttest_create_repo no-dir-rename &&\n+\t(\n+\t\tcd no-dir-rename &&\n+\n+\t\tmkdir x &&\n+\t\ttest_seq  1 10 >x/a &&\n+\t\ttest_seq 11 20 >x/b &&\n+\t\ttest_seq 21 30 >x/c &&\n+\t\techo original >project_info &&\n+\t\tgit add x project_info &&\n+\t\tgit commit -m \"Initial\" &&\n+\n+\t\tgit branch O &&\n+\t\tgit branch A &&\n+\t\tgit branch B &&\n+\n+\t\tgit checkout A &&\n+\t\techo v2 >project_info &&\n+\t\tgit add project_info &&\n+\t\tgit commit -m \"Modify project_info\" &&\n+\n+\t\tgit checkout B &&\n+\t\tmkdir y &&\n+\t\tgit mv x/c y/c &&\n+\t\techo v1 >project_info &&\n+\t\tgit add project_info &&\n+\t\tgit commit -m \"Rename x/c to y/c, modify project_info\"\n+\t)\n+'\n+\n+test_expect_success 'rebase --interactive: NO directory rename' '\n+\ttest_when_finished \"git -C no-dir-rename rebase --abort\" &&\n+\t(\n+\t\tcd no-dir-rename &&\n+\n+\t\tgit checkout B^0 &&\n+\n+\t\tset_fake_editor &&\n+\t\tFAKE_LINES=\"1\" test_must_fail git rebase --interactive A &&\n+\n+\t\tgit ls-files -s >out &&\n+\t\ttest_line_count = 6 out &&\n+\n+\t\ttest_path_is_file x/a &&\n+\t\ttest_path_is_file x/b &&\n+\t\ttest_path_is_missing x/c\n+\t)\n+'\n+\n+test_expect_failure 'rebase (am): NO directory rename' '\n+\ttest_when_finished \"git -C no-dir-rename rebase --abort\" &&\n+\t(\n+\t\tcd no-dir-rename &&\n+\n+\t\tgit checkout B^0 &&\n+\n+\t\tset_fake_editor &&\n+\t\tFAKE_LINES=\"1\" test_must_fail git rebase A &&\n+\n+\t\tgit ls-files -s >out &&\n+\t\ttest_line_count = 6 out &&\n+\n+\t\ttest_path_is_file x/a &&\n+\t\ttest_path_is_file x/b &&\n+\t\ttest_path_is_missing x/c\n+\t)\n+'\n+\n+test_expect_success 'rebase --merge: NO directory rename' '\n+\ttest_when_finished \"git -C no-dir-rename rebase --abort\" &&\n+\t(\n+\t\tcd no-dir-rename &&\n+\n+\t\tgit checkout B^0 &&\n+\n+\t\tset_fake_editor &&\n+\t\tFAKE_LINES=\"1\" test_must_fail git rebase --merge A &&\n+\n+\t\tgit ls-files -s >out &&\n+\t\ttest_line_count = 6 out &&\n+\n+\t\ttest_path_is_file x/a &&\n+\t\ttest_path_is_file x/b &&\n+\t\ttest_path_is_missing x/c\n+\t)\n+'\n+\n+test_expect_failure 'am: NO directory rename' '\n+\ttest_when_finished \"git -C no-dir-rename am --abort\" &&\n+\t(\n+\t\tcd no-dir-rename &&\n+\n+\t\tgit checkout A^0 &&\n+\n+\t\tgit format-patch -1 B &&\n+\n+\t\ttest_must_fail git am --3way 0001*.patch &&\n+\n+\t\tgit ls-files -s >out &&\n+\t\ttest_line_count = 6 out &&\n+\n+\t\ttest_path_is_file x/a &&\n+\t\ttest_path_is_file x/b &&\n+\t\ttest_path_is_missing x/c\n+\t)\n+'\n+\n test_done\n-- \n2.18.0.12.g97a29da30a\n\n"},{"id":"356816","messageId":"20180829070613.11793-3-newren@gmail.com","threadId":"49234","inReplyTo":"20180829070613.11793-1-newren@gmail.com","subject":"[PATCH 2/3] merge-recursive: add ability to turn off directory rename detection","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-08-29T07:06:12Z","receivedAt":"2018-08-29T07:06:26Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Signed-off-by: Elijah Newren <newren@gmail.com>\n---\n merge-recursive.c | 18 +++++++++++++-----\n merge-recursive.h |  1 +\n 2 files changed, 14 insertions(+), 5 deletions(-)\n\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex f110e1c5ec..bf3cb03d3a 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -2843,12 +2843,19 @@ static int handle_renames(struct merge_options *o,\n \thead_pairs = get_diffpairs(o, common, head);\n \tmerge_pairs = get_diffpairs(o, common, merge);\n \n-\tdir_re_head = get_directory_renames(head_pairs, head);\n-\tdir_re_merge = get_directory_renames(merge_pairs, merge);\n+\tif (o->detect_directory_renames) {\n+\t\tdir_re_head = get_directory_renames(head_pairs, head);\n+\t\tdir_re_merge = get_directory_renames(merge_pairs, merge);\n \n-\thandle_directory_level_conflicts(o,\n-\t\t\t\t\t dir_re_head, head,\n-\t\t\t\t\t dir_re_merge, merge);\n+\t\thandle_directory_level_conflicts(o,\n+\t\t\t\t\t\t dir_re_head, head,\n+\t\t\t\t\t\t dir_re_merge, merge);\n+\t} else {\n+\t\tdir_re_head  = xmalloc(sizeof(*dir_re_head));\n+\t\tdir_re_merge = xmalloc(sizeof(*dir_re_merge));\n+\t\tdir_rename_init(dir_re_head);\n+\t\tdir_rename_init(dir_re_merge);\n+\t}\n \n \tri->head_renames  = get_renames(o, head_pairs,\n \t\t\t\t\tdir_re_merge, dir_re_head, head,\n@@ -3541,6 +3548,7 @@ void init_merge_options(struct merge_options *o)\n \to->renormalize = 0;\n \to->diff_detect_rename = -1;\n \to->merge_detect_rename = -1;\n+\to->detect_directory_renames = 1;\n \tmerge_recursive_config(o);\n \tmerge_verbosity = getenv(\"GIT_MERGE_VERBOSITY\");\n \tif (merge_verbosity)\ndiff --git a/merge-recursive.h b/merge-recursive.h\nindex fa7bc6b683..e39ee5d78b 100644\n--- a/merge-recursive.h\n+++ b/merge-recursive.h\n@@ -18,6 +18,7 @@ struct merge_options {\n \tunsigned renormalize : 1;\n \tlong xdl_opts;\n \tint verbosity;\n+\tint detect_directory_renames;\n \tint diff_detect_rename;\n \tint merge_detect_rename;\n \tint diff_rename_limit;\n-- \n2.18.0.12.g97a29da30a\n\n"},{"id":"356817","messageId":"20180829070613.11793-4-newren@gmail.com","threadId":"49234","inReplyTo":"20180829070613.11793-1-newren@gmail.com","subject":"[PATCH 3/3] am: avoid directory rename detection when calling recursive merge machinery","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-08-29T07:06:13Z","receivedAt":"2018-08-29T07:06:27Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Let's say you have the following three trees, where Base is from one commit\nbehind either master or branch:\n\n   Base  : bar_v1, foo/{file1, file2, file3}\n   branch: bar_v2, foo/{file1, file2},       goo/file3\n   master: bar_v3, foo/{file1, file2, file3}\n\nUsing git-am (or am-based rebase) to apply the changes from branch onto\nmaster results in the following tree:\n\n   Result: bar_merged, goo/{file1, file2, file3}\n\nThis is not what users want; they did not rename foo/ -> goo/, they only\nrenamed one file within that directory.  The reason this happens is am\nconstructs fake trees (via build_fake_ancestor()) of the following form:\n\n   Base_bfa  : bar_v1, foo/file3\n   branch_bfa: bar_v2, goo/file3\n\nCombining these two trees with master's tree:\n\n   master: bar_v3, foo/{file1, file2, file3},\n\nYou can see that merge_recursive_generic() would see branch_bfa as renaming\nfoo/ -> goo/, and master as just adding both foo/file1 and foo/file2.  As\nsuch, it ends up with goo/{file1, file2, file3}\n\nThe core problem is that am does not have access to the original trees; it\ncan only construct trees using the blobs involved in the patch.  As such,\nit is not safe to perform directory rename detection within am -3.\n\nSigned-off-by: Elijah Newren <newren@gmail.com>\n---\n builtin/am.c                    | 1 +\n t/t3401-rebase-and-am-rename.sh | 4 ++--\n 2 files changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex 2fc2d1e82c..1494a9be84 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -1596,6 +1596,7 @@ static int fall_back_threeway(const struct am_state *state, const char *index_pa\n \to.branch1 = \"HEAD\";\n \ttheir_tree_name = xstrfmt(\"%.*s\", linelen(state->msg), state->msg);\n \to.branch2 = their_tree_name;\n+\to.detect_directory_renames = 0;\n \n \tif (state->quiet)\n \t\to.verbosity = 0;\ndiff --git a/t/t3401-rebase-and-am-rename.sh b/t/t3401-rebase-and-am-rename.sh\nindex a87df9e675..94bdfbd69c 100755\n--- a/t/t3401-rebase-and-am-rename.sh\n+++ b/t/t3401-rebase-and-am-rename.sh\n@@ -152,7 +152,7 @@ test_expect_success 'rebase --interactive: NO directory rename' '\n \t)\n '\n \n-test_expect_failure 'rebase (am): NO directory rename' '\n+test_expect_success 'rebase (am): NO directory rename' '\n \ttest_when_finished \"git -C no-dir-rename rebase --abort\" &&\n \t(\n \t\tcd no-dir-rename &&\n@@ -190,7 +190,7 @@ test_expect_success 'rebase --merge: NO directory rename' '\n \t)\n '\n \n-test_expect_failure 'am: NO directory rename' '\n+test_expect_success 'am: NO directory rename' '\n \ttest_when_finished \"git -C no-dir-rename am --abort\" &&\n \t(\n \t\tcd no-dir-rename &&\n-- \n2.18.0.12.g97a29da30a\n\n"},{"id":"356830","messageId":"nycvar.QRO.7.76.6.1808291451030.71@tvgsbejvaqbjf.bet","threadId":"49234","inReplyTo":"20180829070613.11793-4-newren@gmail.com","subject":"Re: [PATCH 3/3] am: avoid directory rename detection when calling recursive merge machinery","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-08-29T12:51:35Z","receivedAt":"2018-08-29T12:51:44Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Elijah,\n\nOn Wed, 29 Aug 2018, Elijah Newren wrote:\n\n> Let's say you have the following three trees, where Base is from one commit\n> behind either master or branch:\n> \n>    Base  : bar_v1, foo/{file1, file2, file3}\n>    branch: bar_v2, foo/{file1, file2},       goo/file3\n>    master: bar_v3, foo/{file1, file2, file3}\n> \n> Using git-am (or am-based rebase) to apply the changes from branch onto\n> master results in the following tree:\n> \n>    Result: bar_merged, goo/{file1, file2, file3}\n> \n> This is not what users want; they did not rename foo/ -> goo/, they only\n> renamed one file within that directory.  The reason this happens is am\n> constructs fake trees (via build_fake_ancestor()) of the following form:\n> \n>    Base_bfa  : bar_v1, foo/file3\n>    branch_bfa: bar_v2, goo/file3\n> \n> Combining these two trees with master's tree:\n> \n>    master: bar_v3, foo/{file1, file2, file3},\n> \n> You can see that merge_recursive_generic() would see branch_bfa as renaming\n> foo/ -> goo/, and master as just adding both foo/file1 and foo/file2.  As\n> such, it ends up with goo/{file1, file2, file3}\n> \n> The core problem is that am does not have access to the original trees; it\n> can only construct trees using the blobs involved in the patch.  As such,\n> it is not safe to perform directory rename detection within am -3.\n\nI read through all three patches, and they look fine to me!\n\nCiao,\nDscho\n\n> \n> Signed-off-by: Elijah Newren <newren@gmail.com>\n> ---\n>  builtin/am.c                    | 1 +\n>  t/t3401-rebase-and-am-rename.sh | 4 ++--\n>  2 files changed, 3 insertions(+), 2 deletions(-)\n> \n> diff --git a/builtin/am.c b/builtin/am.c\n> index 2fc2d1e82c..1494a9be84 100644\n> --- a/builtin/am.c\n> +++ b/builtin/am.c\n> @@ -1596,6 +1596,7 @@ static int fall_back_threeway(const struct am_state *state, const char *index_pa\n>  \to.branch1 = \"HEAD\";\n>  \ttheir_tree_name = xstrfmt(\"%.*s\", linelen(state->msg), state->msg);\n>  \to.branch2 = their_tree_name;\n> +\to.detect_directory_renames = 0;\n>  \n>  \tif (state->quiet)\n>  \t\to.verbosity = 0;\n> diff --git a/t/t3401-rebase-and-am-rename.sh b/t/t3401-rebase-and-am-rename.sh\n> index a87df9e675..94bdfbd69c 100755\n> --- a/t/t3401-rebase-and-am-rename.sh\n> +++ b/t/t3401-rebase-and-am-rename.sh\n> @@ -152,7 +152,7 @@ test_expect_success 'rebase --interactive: NO directory rename' '\n>  \t)\n>  '\n>  \n> -test_expect_failure 'rebase (am): NO directory rename' '\n> +test_expect_success 'rebase (am): NO directory rename' '\n>  \ttest_when_finished \"git -C no-dir-rename rebase --abort\" &&\n>  \t(\n>  \t\tcd no-dir-rename &&\n> @@ -190,7 +190,7 @@ test_expect_success 'rebase --merge: NO directory rename' '\n>  \t)\n>  '\n>  \n> -test_expect_failure 'am: NO directory rename' '\n> +test_expect_success 'am: NO directory rename' '\n>  \ttest_when_finished \"git -C no-dir-rename am --abort\" &&\n>  \t(\n>  \t\tcd no-dir-rename &&\n> -- \n> 2.18.0.12.g97a29da30a\n> \n> \n"},{"id":"356831","messageId":"nycvar.QRO.7.76.6.1808291452070.71@tvgsbejvaqbjf.bet","threadId":"49234","inReplyTo":"20180829070613.11793-3-newren@gmail.com","subject":"Re: [PATCH 2/3] merge-recursive: add ability to turn off directory rename detection","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-08-29T12:54:20Z","receivedAt":"2018-08-29T12:54:28Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Elijah,\n\nOn Wed, 29 Aug 2018, Elijah Newren wrote:\n\n> Signed-off-by: Elijah Newren <newren@gmail.com>\n> ---\n>  merge-recursive.c | 18 +++++++++++++-----\n>  merge-recursive.h |  1 +\n>  2 files changed, 14 insertions(+), 5 deletions(-)\n> \n> diff --git a/merge-recursive.c b/merge-recursive.c\n> index f110e1c5ec..bf3cb03d3a 100644\n> --- a/merge-recursive.c\n> +++ b/merge-recursive.c\n> @@ -2843,12 +2843,19 @@ static int handle_renames(struct merge_options *o,\n>  \thead_pairs = get_diffpairs(o, common, head);\n>  \tmerge_pairs = get_diffpairs(o, common, merge);\n>  \n> -\tdir_re_head = get_directory_renames(head_pairs, head);\n> -\tdir_re_merge = get_directory_renames(merge_pairs, merge);\n> +\tif (o->detect_directory_renames) {\n> +\t\tdir_re_head = get_directory_renames(head_pairs, head);\n> +\t\tdir_re_merge = get_directory_renames(merge_pairs, merge);\n>  \n> -\thandle_directory_level_conflicts(o,\n> -\t\t\t\t\t dir_re_head, head,\n> -\t\t\t\t\t dir_re_merge, merge);\n> +\t\thandle_directory_level_conflicts(o,\n> +\t\t\t\t\t\t dir_re_head, head,\n> +\t\t\t\t\t\t dir_re_merge, merge);\n> +\t} else {\n> +\t\tdir_re_head  = xmalloc(sizeof(*dir_re_head));\n> +\t\tdir_re_merge = xmalloc(sizeof(*dir_re_merge));\n\nThis is not a suggestion to change anything, but a genuine question out of\ncuriosity: would it make sense to put the `dir_re_head` and `dir_re_merge`\nstructures into `struct merge_options` to avoid these extra `malloc()`s?\nOr would that cause issues with the recursive nature of the recursive\nmerge?\n\nCiao,\nDscho\n\n> +\t\tdir_rename_init(dir_re_head);\n> +\t\tdir_rename_init(dir_re_merge);\n> +\t}\n>  \n>  \tri->head_renames  = get_renames(o, head_pairs,\n>  \t\t\t\t\tdir_re_merge, dir_re_head, head,\n> @@ -3541,6 +3548,7 @@ void init_merge_options(struct merge_options *o)\n>  \to->renormalize = 0;\n>  \to->diff_detect_rename = -1;\n>  \to->merge_detect_rename = -1;\n> +\to->detect_directory_renames = 1;\n>  \tmerge_recursive_config(o);\n>  \tmerge_verbosity = getenv(\"GIT_MERGE_VERBOSITY\");\n>  \tif (merge_verbosity)\n> diff --git a/merge-recursive.h b/merge-recursive.h\n> index fa7bc6b683..e39ee5d78b 100644\n> --- a/merge-recursive.h\n> +++ b/merge-recursive.h\n> @@ -18,6 +18,7 @@ struct merge_options {\n>  \tunsigned renormalize : 1;\n>  \tlong xdl_opts;\n>  \tint verbosity;\n> +\tint detect_directory_renames;\n>  \tint diff_detect_rename;\n>  \tint merge_detect_rename;\n>  \tint diff_rename_limit;\n> -- \n> 2.18.0.12.g97a29da30a\n> \n> \n"},{"id":"356901","messageId":"xmqqo9dkakl0.fsf@gitster-ct.c.googlers.com","threadId":"49234","inReplyTo":"20180829070613.11793-2-newren@gmail.com","subject":"Re: [PATCH 1/3] t3401: add another directory rename testcase for rebase and am","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-08-29T22:12:27Z","receivedAt":"2018-08-29T22:12:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren <newren@gmail.com> writes:\n\n> +test_expect_success 'rebase --interactive: NO directory rename' '\n> +\ttest_when_finished \"git -C no-dir-rename rebase --abort\" &&\n> +\t(\n> +\t\tcd no-dir-rename &&\n> +\n> +\t\tgit checkout B^0 &&\n> +\n> +\t\tset_fake_editor &&\n> +\t\tFAKE_LINES=\"1\" test_must_fail git rebase --interactive A &&\n\nIs this a single-shot environment assignment?  That would have been\ncaught with the test linter.\n\nPerhaps squshing this in would be sufficient fix?\n\n t/t3401-rebase-and-am-rename.sh | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/t/t3401-rebase-and-am-rename.sh b/t/t3401-rebase-and-am-rename.sh\nindex 94bdfbd69c..13e09afec0 100755\n--- a/t/t3401-rebase-and-am-rename.sh\n+++ b/t/t3401-rebase-and-am-rename.sh\n@@ -141,7 +141,7 @@ test_expect_success 'rebase --interactive: NO directory rename' '\n \t\tgit checkout B^0 &&\n \n \t\tset_fake_editor &&\n-\t\tFAKE_LINES=\"1\" test_must_fail git rebase --interactive A &&\n+\t\ttest_must_fail env FAKE_LINES=\"1\" git rebase --interactive A &&\n \n \t\tgit ls-files -s >out &&\n \t\ttest_line_count = 6 out &&\n@@ -160,7 +160,7 @@ test_expect_success 'rebase (am): NO directory rename' '\n \t\tgit checkout B^0 &&\n \n \t\tset_fake_editor &&\n-\t\tFAKE_LINES=\"1\" test_must_fail git rebase A &&\n+\t\ttest_must_fail env FAKE_LINES=\"1\" git rebase A &&\n \n \t\tgit ls-files -s >out &&\n \t\ttest_line_count = 6 out &&\n@@ -179,7 +179,7 @@ test_expect_success 'rebase --merge: NO directory rename' '\n \t\tgit checkout B^0 &&\n \n \t\tset_fake_editor &&\n-\t\tFAKE_LINES=\"1\" test_must_fail git rebase --merge A &&\n+\t\ttest_must_fail env FAKE_LINES=\"1\" git rebase --merge A &&\n \n \t\tgit ls-files -s >out &&\n \t\ttest_line_count = 6 out &&\n"},{"id":"356913","messageId":"CABPp-BHFrk026R5VZLGwVvRYq_d82GTFqAyAeyTD=iQX4df2Bw@mail.gmail.com","threadId":"49234","inReplyTo":"nycvar.QRO.7.76.6.1808291452070.71@tvgsbejvaqbjf.bet","subject":"Re: [PATCH 2/3] merge-recursive: add ability to turn off directory rename detection","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-08-29T23:00:48Z","receivedAt":"2018-08-29T23:01:03Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Wed, Aug 29, 2018 at 5:54 AM Johannes Schindelin\n<Johannes.Schindelin@gmx.de> wrote:\n>\n> Hi Elijah,\n>\n> On Wed, 29 Aug 2018, Elijah Newren wrote:\n>\n> > Signed-off-by: Elijah Newren <newren@gmail.com>\n> > ---\n> >  merge-recursive.c | 18 +++++++++++++-----\n> >  merge-recursive.h |  1 +\n> >  2 files changed, 14 insertions(+), 5 deletions(-)\n> >\n> > diff --git a/merge-recursive.c b/merge-recursive.c\n> > index f110e1c5ec..bf3cb03d3a 100644\n> > --- a/merge-recursive.c\n> > +++ b/merge-recursive.c\n> > @@ -2843,12 +2843,19 @@ static int handle_renames(struct merge_options *o,\n> >       head_pairs = get_diffpairs(o, common, head);\n> >       merge_pairs = get_diffpairs(o, common, merge);\n> >\n> > -     dir_re_head = get_directory_renames(head_pairs, head);\n> > -     dir_re_merge = get_directory_renames(merge_pairs, merge);\n> > +     if (o->detect_directory_renames) {\n> > +             dir_re_head = get_directory_renames(head_pairs, head);\n> > +             dir_re_merge = get_directory_renames(merge_pairs, merge);\n> >\n> > -     handle_directory_level_conflicts(o,\n> > -                                      dir_re_head, head,\n> > -                                      dir_re_merge, merge);\n> > +             handle_directory_level_conflicts(o,\n> > +                                              dir_re_head, head,\n> > +                                              dir_re_merge, merge);\n> > +     } else {\n> > +             dir_re_head  = xmalloc(sizeof(*dir_re_head));\n> > +             dir_re_merge = xmalloc(sizeof(*dir_re_merge));\n>\n> This is not a suggestion to change anything, but a genuine question out of\n> curiosity: would it make sense to put the `dir_re_head` and `dir_re_merge`\n> structures into `struct merge_options` to avoid these extra `malloc()`s?\n> Or would that cause issues with the recursive nature of the recursive\n> merge?\n\nThat would work to avoid the extra `malloc()`s, and be inline with the\ncurrent usage of merge_options.  However, I'm not sure I like the\ncurrent usage of merge_options. That struct is supposed to be public\nAPI, but it's got a lot of private internal-only use stuff (and\nputting dir_re_head and dir_re_merge there would add more).  I'm\ntempted to go the other way and eject some of the other internal-only\nstuff from merge_options (or wrap it inside an opaque struct\nmerge_options_internal* internal field, or something like that).\n"},{"id":"356918","messageId":"CABPp-BFYhw9iU=jxt-1uY75GSt-_JtLnYQFf0kfjbqtK--xi3w@mail.gmail.com","threadId":"49234","inReplyTo":"xmqqo9dkakl0.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 1/3] t3401: add another directory rename testcase for rebase and am","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-08-29T23:47:26Z","receivedAt":"2018-08-29T23:47:41Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Hi Junio,\n\nOn Wed, Aug 29, 2018 at 3:12 PM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Elijah Newren <newren@gmail.com> writes:\n>\n> > +test_expect_success 'rebase --interactive: NO directory rename' '\n> > +     test_when_finished \"git -C no-dir-rename rebase --abort\" &&\n> > +     (\n> > +             cd no-dir-rename &&\n> > +\n> > +             git checkout B^0 &&\n> > +\n> > +             set_fake_editor &&\n> > +             FAKE_LINES=\"1\" test_must_fail git rebase --interactive A &&\n>\n> Is this a single-shot environment assignment?  That would have been\n> caught with the test linter.\n\nUgh, yes.  Sorry.\n\nI was trying to allow backporting to 2.18, so tried to build my series\non that...but moved forward slightly to en/rebase-consistency in order\nto re-use the same testfile (as mentioned in my cover letter).  But\nthat meant my branch was missing a0a630192dca\n(\"t/check-non-portable-shell: detect \"FOO=bar shell_func\"\",\n2018-07-13), so the test-linter couldn't catch this for me.\n\n> Perhaps squashing this in would be sufficient fix?\n\nThanks for fixing it up.  That works...although now I've spotted\nanother minor issue.  The FAKE_LINES setting is only needed for the\ninteractive rebase case and can be removed for the other two.  Oops.\nIt doesn't hurt, but might confuse readers of the testcase.\n\nWould you like me to resend a fixup on top of your fixup, resend the\nwhole series with the fixes squashed, or just ignore this final\n(cosmetic) issue since it doesn't affect correctness and I've caused\nyou enough extra work already?\n\n>  t/t3401-rebase-and-am-rename.sh | 6 +++---\n>  1 file changed, 3 insertions(+), 3 deletions(-)\n>\n> diff --git a/t/t3401-rebase-and-am-rename.sh b/t/t3401-rebase-and-am-rename.sh\n> index 94bdfbd69c..13e09afec0 100755\n> --- a/t/t3401-rebase-and-am-rename.sh\n> +++ b/t/t3401-rebase-and-am-rename.sh\n> @@ -141,7 +141,7 @@ test_expect_success 'rebase --interactive: NO directory rename' '\n>                 git checkout B^0 &&\n>\n>                 set_fake_editor &&\n> -               FAKE_LINES=\"1\" test_must_fail git rebase --interactive A &&\n> +               test_must_fail env FAKE_LINES=\"1\" git rebase --interactive A &&\n>\n>                 git ls-files -s >out &&\n>                 test_line_count = 6 out &&\n> @@ -160,7 +160,7 @@ test_expect_success 'rebase (am): NO directory rename' '\n>                 git checkout B^0 &&\n>\n>                 set_fake_editor &&\n> -               FAKE_LINES=\"1\" test_must_fail git rebase A &&\n> +               test_must_fail env FAKE_LINES=\"1\" git rebase A &&\n>\n>                 git ls-files -s >out &&\n>                 test_line_count = 6 out &&\n> @@ -179,7 +179,7 @@ test_expect_success 'rebase --merge: NO directory rename' '\n>                 git checkout B^0 &&\n>\n>                 set_fake_editor &&\n> -               FAKE_LINES=\"1\" test_must_fail git rebase --merge A &&\n> +               test_must_fail env FAKE_LINES=\"1\" git rebase --merge A &&\n>\n>                 git ls-files -s >out &&\n>                 test_line_count = 6 out &&\n"},{"id":"356987","messageId":"xmqqlg8n6dy4.fsf@gitster-ct.c.googlers.com","threadId":"49234","inReplyTo":"CABPp-BFYhw9iU=jxt-1uY75GSt-_JtLnYQFf0kfjbqtK--xi3w@mail.gmail.com","subject":"Re: [PATCH 1/3] t3401: add another directory rename testcase for rebase and am","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-08-30T16:01:39Z","receivedAt":"2018-08-30T16:01:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren <newren@gmail.com> writes:\n\n>> Is this a single-shot environment assignment?  That would have been\n>> caught with the test linter.\n>\n> Ugh, yes.  Sorry.\n>\n> I was trying to allow backporting to 2.18, so tried to build my series\n> on that...but moved forward slightly to en/rebase-consistency in order\n> to re-use the same testfile (as mentioned in my cover letter).  But\n> that meant my branch was missing a0a630192dca\n> (\"t/check-non-portable-shell: detect \"FOO=bar shell_func\"\",\n> 2018-07-13), so the test-linter couldn't catch this for me.\n\nTrue, I also only caught this during my integration cycle, not\nduring the review of the patches.\n\n>> Perhaps squashing this in would be sufficient fix?\n>\n> Thanks for fixing it up.  That works...although now I've spotted\n> another minor issue.  The FAKE_LINES setting is only needed for the\n> interactive rebase case and can be removed for the other two.  Oops.\n> It doesn't hurt, but might confuse readers of the testcase.\n\nAh, OK.  So the squashable fix would now become like this, all\nfixing the ones from the first patch.\n\n> Would you like me to resend a fixup on top of your fixup, resend the\n> whole series with the fixes squashed, or just ignore this final\n> (cosmetic) issue since it doesn't affect correctness and I've caused\n> you enough extra work already?\n\nNo worries.  This is what the maintainer does (when s/he is not\nplaying other roles like a reviewer or an individual contributor).\n\nI'll squash in the following to the 1/3 patch and queue the topic\nwith the other two patches.\n\nThanks.\n\ndiff --git a/t/t3401-rebase-and-am-rename.sh b/t/t3401-rebase-and-am-rename.sh\nindex 94bdfbd69c..e0b5111993 100755\n--- a/t/t3401-rebase-and-am-rename.sh\n+++ b/t/t3401-rebase-and-am-rename.sh\n@@ -141,7 +141,7 @@ test_expect_success 'rebase --interactive: NO directory rename' '\n \t\tgit checkout B^0 &&\n \n \t\tset_fake_editor &&\n-\t\tFAKE_LINES=\"1\" test_must_fail git rebase --interactive A &&\n+\t\ttest_must_fail env FAKE_LINES=\"1\" git rebase --interactive A &&\n \n \t\tgit ls-files -s >out &&\n \t\ttest_line_count = 6 out &&\n@@ -160,7 +160,7 @@ test_expect_success 'rebase (am): NO directory rename' '\n \t\tgit checkout B^0 &&\n \n \t\tset_fake_editor &&\n-\t\tFAKE_LINES=\"1\" test_must_fail git rebase A &&\n+\t\ttest_must_fail git rebase A &&\n \n \t\tgit ls-files -s >out &&\n \t\ttest_line_count = 6 out &&\n@@ -179,7 +179,7 @@ test_expect_success 'rebase --merge: NO directory rename' '\n \t\tgit checkout B^0 &&\n \n \t\tset_fake_editor &&\n-\t\tFAKE_LINES=\"1\" test_must_fail git rebase --merge A &&\n+\t\ttest_must_fail git rebase --merge A &&\n \n \t\tgit ls-files -s >out &&\n \t\ttest_line_count = 6 out &&\n"},{"id":"356989","messageId":"CABPp-BErSY+284vqD6xiLrcsbMOghzM7vSp6THXkYuCbymUqKA@mail.gmail.com","threadId":"49234","inReplyTo":"xmqqlg8n6dy4.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 1/3] t3401: add another directory rename testcase for rebase and am","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-08-30T16:26:59Z","receivedAt":"2018-08-30T16:27:14Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Thu, Aug 30, 2018 at 9:01 AM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Elijah Newren <newren@gmail.com> writes:\n>\n> >> Is this a single-shot environment assignment?  That would have been\n> >> caught with the test linter.\n> >\n> > Ugh, yes.  Sorry.\n> >\n> > I was trying to allow backporting to 2.18, so tried to build my series\n> > on that...but moved forward slightly to en/rebase-consistency in order\n> > to re-use the same testfile (as mentioned in my cover letter).  But\n> > that meant my branch was missing a0a630192dca\n> > (\"t/check-non-portable-shell: detect \"FOO=bar shell_func\"\",\n> > 2018-07-13), so the test-linter couldn't catch this for me.\n>\n> True, I also only caught this during my integration cycle, not\n> during the review of the patches.\n>\n> >> Perhaps squashing this in would be sufficient fix?\n> >\n> > Thanks for fixing it up.  That works...although now I've spotted\n> > another minor issue.  The FAKE_LINES setting is only needed for the\n> > interactive rebase case and can be removed for the other two.  Oops.\n> > It doesn't hurt, but might confuse readers of the testcase.\n>\n> Ah, OK.  So the squashable fix would now become like this, all\n> fixing the ones from the first patch.\n>\n> > Would you like me to resend a fixup on top of your fixup, resend the\n> > whole series with the fixes squashed, or just ignore this final\n> > (cosmetic) issue since it doesn't affect correctness and I've caused\n> > you enough extra work already?\n>\n> No worries.  This is what the maintainer does (when s/he is not\n> playing other roles like a reviewer or an individual contributor).\n>\n> I'll squash in the following to the 1/3 patch and queue the topic\n> with the other two patches.\n\nThanks, looks great.\n\n\n> diff --git a/t/t3401-rebase-and-am-rename.sh b/t/t3401-rebase-and-am-rename.sh\n> index 94bdfbd69c..e0b5111993 100755\n> --- a/t/t3401-rebase-and-am-rename.sh\n> +++ b/t/t3401-rebase-and-am-rename.sh\n> @@ -141,7 +141,7 @@ test_expect_success 'rebase --interactive: NO directory rename' '\n>                 git checkout B^0 &&\n>\n>                 set_fake_editor &&\n> -               FAKE_LINES=\"1\" test_must_fail git rebase --interactive A &&\n> +               test_must_fail env FAKE_LINES=\"1\" git rebase --interactive A &&\n>\n>                 git ls-files -s >out &&\n>                 test_line_count = 6 out &&\n> @@ -160,7 +160,7 @@ test_expect_success 'rebase (am): NO directory rename' '\n>                 git checkout B^0 &&\n>\n>                 set_fake_editor &&\n> -               FAKE_LINES=\"1\" test_must_fail git rebase A &&\n> +               test_must_fail git rebase A &&\n>\n>                 git ls-files -s >out &&\n>                 test_line_count = 6 out &&\n> @@ -179,7 +179,7 @@ test_expect_success 'rebase --merge: NO directory rename' '\n>                 git checkout B^0 &&\n>\n>                 set_fake_editor &&\n> -               FAKE_LINES=\"1\" test_must_fail git rebase --merge A &&\n> +               test_must_fail git rebase --merge A &&\n>\n>                 git ls-files -s >out &&\n>                 test_line_count = 6 out &&\n"},{"id":"356990","messageId":"CABPp-BH8-1YQ49aaM-dXkT9ukKO92MF-FZ2kLwd=KHe4TVbjqQ@mail.gmail.com","threadId":"49234","inReplyTo":"xmqqh8jeh1id.fsf@gitster-ct.c.googlers.com","subject":"Re: A rebase regression in Git 2.18.0","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-08-30T16:41:16Z","receivedAt":"2018-08-30T16:41:30Z","isPatch":false,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Tue, Aug 28, 2018 at 9:58 AM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Elijah Newren <newren@gmail.com> writes:\n>\n> >   - Add a flag to turn off directory rename detection, and set the\n> > flag for every call from am.c in order to avoid problems like this.\n>\n> I'd say this is the only practical solution, before you deprecate\n> the \"pipe format-patch output to am -3\" style of \"git rebase\" (and\n> optionally replace with something else).\n\nI posted a patch a while back to add an --am flag to \"git rebase\",\nmake \"--am\" be implied by options which are still am-specific\n(--whitespace, --committer-date-is-author-date, and -C), and change\n--merge to be the default.\n\nI'll post it as an RFC again after the various rebase-rewrite series\nhave settled and merged down...along with my other rebase cleanups\nthat I was waiting on to avoid conflicts with GSoC stuff.\n\n> The whole point of \"am -3\" is to do _better_ than just \"patch\" with\n> minimum amount of information available on the pre- and post- image\n> blobs, without knowing the remainder of the tree that the patch did\n> not touch.  It is not surprising that the heuristics that look at\n> the unchanging part of the tree to infer renames that may or may not\n> exist guesses incorrectly, either with false positive or negative.\n> In the context of \"rebase\", we always have all the trees that are\n> involved.  We should be able to do better than \"am -3\".\n"},{"id":"357079","messageId":"nycvar.QRO.7.76.6.1808311158540.71@tvgsbejvaqbjf.bet","threadId":"49234","inReplyTo":"CABPp-BH8-1YQ49aaM-dXkT9ukKO92MF-FZ2kLwd=KHe4TVbjqQ@mail.gmail.com","subject":"Re: A rebase regression in Git 2.18.0","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-08-31T10:11:56Z","receivedAt":"2018-08-31T10:12:06Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Elijah,\n\nOn Thu, 30 Aug 2018, Elijah Newren wrote:\n\n> On Tue, Aug 28, 2018 at 9:58 AM Junio C Hamano <gitster@pobox.com> wrote:\n> >\n> > Elijah Newren <newren@gmail.com> writes:\n> >\n> > >   - Add a flag to turn off directory rename detection, and set the\n> > > flag for every call from am.c in order to avoid problems like this.\n> >\n> > I'd say this is the only practical solution, before you deprecate\n> > the \"pipe format-patch output to am -3\" style of \"git rebase\" (and\n> > optionally replace with something else).\n> \n> I posted a patch a while back to add an --am flag to \"git rebase\",\n> make \"--am\" be implied by options which are still am-specific\n> (--whitespace, --committer-date-is-author-date, and -C), and change\n> --merge to be the default.\n\nDidn't you also post a patch to fold --merge into the --interactive\nbackend? What's your current state of thinking about this?\n\nAs to switching from --am as the default: I still think that --am has\nserious speed advantages over --merge (or for that matter, --interactive).\nI have no numbers to back that up, though, and I am currently really busy\nwith working on the CI, so I won't be able to measure these numbers,\neither...\n\nAlso please note: I converted the `am` backend to pure C (it is waiting at\nhttps://github.com/gitgitgadget/git/pull/24, to be submitted after the\nv2.19.0 RC period). Switching to `--merge` as the default would force me\nto convert that backend, too ;-)\n\n> I'll post it as an RFC again after the various rebase-rewrite series\n> have settled and merged down...along with my other rebase cleanups\n> that I was waiting on to avoid conflicts with GSoC stuff.\n\nThanks for waiting! Please note that I am interested, yet I will be on\nvacation for a couple of weeks in September. Don't let that stop you,\nthough!\n\n> > The whole point of \"am -3\" is to do _better_ than just \"patch\" with\n> > minimum amount of information available on the pre- and post- image\n> > blobs, without knowing the remainder of the tree that the patch did\n> > not touch.  It is not surprising that the heuristics that look at\n> > the unchanging part of the tree to infer renames that may or may not\n> > exist guesses incorrectly, either with false positive or negative.\n> > In the context of \"rebase\", we always have all the trees that are\n> > involved.  We should be able to do better than \"am -3\".\n\nRight. I think that Elijah's right, and --merge is that \"do better\"\nsolution.\n\nCiao,\nDscho\n"},{"id":"357112","messageId":"CABPp-BEJ-JPa8g8G1ELsjqf3EDxe+aCBdwERBi5hr5ukrDjJJg@mail.gmail.com","threadId":"49234","inReplyTo":"nycvar.QRO.7.76.6.1808311158540.71@tvgsbejvaqbjf.bet","subject":"Re: A rebase regression in Git 2.18.0","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-08-31T19:37:59Z","receivedAt":"2018-08-31T19:38:13Z","isPatch":false,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Hi Dscho,\n\nOn Fri, Aug 31, 2018 at 3:12 AM Johannes Schindelin\n<Johannes.Schindelin@gmx.de> wrote:\n> On Thu, 30 Aug 2018, Elijah Newren wrote:\n> > On Tue, Aug 28, 2018 at 9:58 AM Junio C Hamano <gitster@pobox.com> wrote:\n> > > Elijah Newren <newren@gmail.com> writes:\n...\n> > > I'd say this is the only practical solution, before you deprecate\n> > > the \"pipe format-patch output to am -3\" style of \"git rebase\" (and\n> > > optionally replace with something else).\n> >\n> > I posted a patch a while back to add an --am flag to \"git rebase\",\n> > make \"--am\" be implied by options which are still am-specific\n> > (--whitespace, --committer-date-is-author-date, and -C), and change\n> > --merge to be the default.\n>\n> Didn't you also post a patch to fold --merge into the --interactive\n> backend? What's your current state of thinking about this?\n\nYes.  I updated it once or twice, but it had conflicts with the GSoC\nprojects each time, so I decided to just hold off on it a bit longer.\nI'm still planning to resubmit this once the GSoC projects merge down.\n\n> As to switching from --am as the default: I still think that --am has\n> serious speed advantages over --merge (or for that matter, --interactive).\n> I have no numbers to back that up, though, and I am currently really busy\n> with working on the CI, so I won't be able to measure these numbers,\n> either...\n\nYep, we talked about this before and you mentioned that the rewrite in\nC should bring some performance improvements, and we agreed that\nmerge-recursive is probably the next issue performance-wise.  I think\nit's at least worth measuring what the approximate performance\ndifferences are with the rewrite of rebase in C, and posting an RFC\nwith that info.  If the answer comes back that we need to do more\noptimization before we switch the default, that's fine.\n\n> Also please note: I converted the `am` backend to pure C (it is waiting at\n> https://github.com/gitgitgadget/git/pull/24, to be submitted after the\n> v2.19.0 RC period). Switching to `--merge` as the default would force me\n> to convert that backend, too ;-)\n\nNot if git-rebase--merge is deleted and --merge is implemented on top\nof the interactive backend as an implicitly_interactive case.  In\nfact, that's probably the simplest way to \"convert\" that backend to C.\nAnyway, since I plan to submit that change first, we should be good.\n\n> > I'll post it as an RFC again after the various rebase-rewrite series\n> > have settled and merged down...along with my other rebase cleanups\n> > that I was waiting on to avoid conflicts with GSoC stuff.\n>\n> Thanks for waiting! Please note that I am interested, yet I will be on\n> vacation for a couple of weeks in September. Don't let that stop you,\n> though!\n\nEnjoy your vacation!\n\n> > > The whole point of \"am -3\" is to do _better_ than just \"patch\" with\n> > > minimum amount of information available on the pre- and post- image\n> > > blobs, without knowing the remainder of the tree that the patch did\n> > > not touch.  It is not surprising that the heuristics that look at\n> > > the unchanging part of the tree to infer renames that may or may not\n> > > exist guesses incorrectly, either with false positive or negative.\n> > > In the context of \"rebase\", we always have all the trees that are\n> > > involved.  We should be able to do better than \"am -3\".\n>\n> Right. I think that Elijah's right, and --merge is that \"do better\"\n> solution.\n\nCool, good to see others seem to agree on the direction I'd like to\nsee things move.\n"}]}