{"thread":{"id":"42421","subject":"Small rerere in rebase regression","startedAt":"2016-05-23T18:01:22Z","lastAt":"2016-05-28T06:39:48Z","messageCount":11,"participants":["Johannes Sixt","Junio C Hamano","Johannes Schindelin"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"287264","messageId":"57434572.6030306@kdbg.org","threadId":"42421","inReplyTo":null,"subject":"Small rerere in rebase regression","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2016-05-23T18:01:22Z","receivedAt":"2016-05-23T18:01:22Z","isPatch":false,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Even though I operate with jc/rerere-multi for some time now,\nI noticed this regression only recently. Watch out for the\nduplicated \"Recorded preimage\" message:\n\ngittest@side:1078> strace -f -o /tmp/git-rebase-conflict.strace git rebase -i master \nerror: could not apply 59f92e0... side\n\nWhen you have resolved this problem, run \"git rebase --continue\".\nIf you prefer to skip this patch, run \"git rebase --skip\" instead.\nTo check out the original branch and stop rebasing, run \"git rebase --abort\".\n\nRecorded preimage for 'a'\nRecorded preimage for 'a'\nCould not apply 59f92e09c890c08442eb1df91eaecec3d1656c86... side\n\nAccording to the strace, the first \"Recorded preimage\" is from\ngit cherry-pick, the second is from \"git rerere\" in this section\nin git-rebase--interactive.sh:\n\ndie_with_patch () {\n\techo \"$1\" > \"$state_dir\"/stopped-sha\n\tmake_patch \"$1\"\n\tgit rerere\n\tdie \"$2\"\n}\n\nThe buglet bisects to\n\n629716d256a792179325c2cc7945bb2d81dda8c2 is the first bad commit\ncommit 629716d256a792179325c2cc7945bb2d81dda8c2\nAuthor: Junio C Hamano \nDate: Thu Jul 30 15:49:18 2015 -0700\n\n rerere: do use multiple variants\n \n This enables the multiple-variant support for real. Multiple\n conflicts of the same shape can have differences in contexts where\n they appear, interfering the replaying of recorded resolution of one\n conflict to another, and in such a case, their resolutions are\n recorded as different variants under the same conflict ID.\n \n We still need to adjust garbage collection codepaths for this\n change, but the basic \"replay\" functionality is functional with\n this change.\n \n Signed-off-by: Junio C Hamano \n\nI'm not sure whether the new behavior is a defect in rerere.c or a\nconsequence of the extra rerere call in interactive rebase...\n\n-- Hannes\n"},{"id":"287303","messageId":"xmqqy4708ss0.fsf@gitster.mtv.corp.google.com","threadId":"42421","inReplyTo":"57434572.6030306@kdbg.org","subject":"Re: Small rerere in rebase regression","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-23T19:43:11Z","receivedAt":"2016-05-23T19:43:11Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt writes:\n\n> I'm not sure whether the new behavior is a defect in rerere.c or a\n> consequence of the extra rerere call in interactive rebase...\n\nInteresting. When running an unnecessary \"git rerere\" (because it\nalready was run and recorded the preimage), we used to be silent.\nWith the updated code, we say something.\n\nAt least it seems that we do not record the same preimage as a\ndifferent variant, so the regression is hopefully merely cosmetic.\n\nI do not think it is a crime to say \"git rerere\" repeatedly without\nchanging anything, so I do not think \"rebase -i\"'s call to rerere\nin die_with_patch is wrong, even though some calls to it seem to be\nmade after seeing a \"git merge\" fail (which means we know 'rerere'\nhas been run already).\n\nI think all plumbing that can turn an index into unmerged state has\na call to git_rerere() at the end (the only obvious exception being\n\"update-index\" that lets you stuff higher stage entries to the\nindex), so as long as \"rebase -i\" uses them correctly, I suspect\nthat die_with_patch shouldn't have to have a call to \"git rerere\".\n"},{"id":"287318","messageId":"57437693.3030106@kdbg.org","threadId":"42421","inReplyTo":"xmqqy4708ss0.fsf@gitster.mtv.corp.google.com","subject":"Re: Small rerere in rebase regression","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2016-05-23T21:30:59Z","receivedAt":"2016-05-23T21:30:59Z","isPatch":false,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 23.05.2016 um 21:43 schrieb Junio C Hamano:\n> Johannes Sixt writes:\n>\n>> I'm not sure whether the new behavior is a defect in rerere.c or a\n>> consequence of the extra rerere call in interactive rebase...\n>\n> Interesting. When running an unnecessary \"git rerere\" (because it\n> already was run and recorded the preimage), we used to be silent.\n> With the updated code, we say something.\n>\n> At least it seems that we do not record the same preimage as a\n> different variant, so the regression is hopefully merely cosmetic.\n>\n> I do not think it is a crime to say \"git rerere\" repeatedly without\n> changing anything, so I do not think \"rebase -i\"'s call to rerere\n> in die_with_patch is wrong, even though some calls to it seem to be\n> made after seeing a \"git merge\" fail (which means we know 'rerere'\n> has been run already).\n>\n> I think all plumbing that can turn an index into unmerged state has\n> a call to git_rerere() at the end (the only obvious exception being\n> \"update-index\" that lets you stuff higher stage entries to the\n> index), so as long as \"rebase -i\" uses them correctly, I suspect\n> that die_with_patch shouldn't have to have a call to \"git rerere\".\n\nI also come to the conclusion that die_with_patch shouldn't have to have \na call to \"git rerere\". die_with_patch can be called after \"git \ncherry-pick\", \"git merge\", \"git commit\", all of which have their own \nrerere() invocation.\n\nHowever, calling \"git rerere\" after a failed \"git commit\" may be \ndestructive: it would record a resolution even though the commit has not \nbe completed. Think of an squash commit being aborted because the user \nnotices an error in the last minute. If that error is in a conflict \nresolution, that wrong resolution would be recorded.\n\n-- Hannes\n"},{"id":"287324","messageId":"xmqqk2ik77cr.fsf@gitster.mtv.corp.google.com","threadId":"42421","inReplyTo":"57437693.3030106@kdbg.org","subject":"Re: Small rerere in rebase regression","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-23T22:11:16Z","receivedAt":"2016-05-23T22:11:16Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt writes:\n\n> I also come to the conclusion that die_with_patch shouldn't have to\n> have a call to \"git rerere\". die_with_patch can be called after \"git\n> cherry-pick\", \"git merge\", \"git commit\", all of which have their own\n> rerere() invocation.\n>\n> However, calling \"git rerere\" after a failed \"git commit\" may be\n> destructive: it would record a resolution even though the commit has\n> not be completed. Think of an squash commit being aborted because the\n> user notices an error in the last minute. If that error is in a\n> conflict resolution, that wrong resolution would be recorded.\n\nSo, the behaviour change you observed uncovered a small bug in\n\"rebase -i\" that was covered by the old limitation of \"rerere\" that\nrefrained from creating preimage when there already is one?\n\nI think removing the call to \"git rerere\" there is a safe and\nsensible thing regardless, but perhaps authors of \"rebase -i\" had\ntheir own reasons. I dunno (it is unlikely I'll have a chance to do\nblame and digging today).\n"},{"id":"287415","messageId":"alpine.DEB.2.20.1605241510370.4449@virtualbox","threadId":"42421","inReplyTo":"xmqqk2ik77cr.fsf@gitster.mtv.corp.google.com","subject":"Re: Small rerere in rebase regression","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-05-24T13:18:15Z","receivedAt":"2016-05-24T13:18:15Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Mon, 23 May 2016, Junio C Hamano wrote:\n\n> Johannes Sixt writes:\n> \n> > I also come to the conclusion that die_with_patch shouldn't have to\n> > have a call to \"git rerere\". die_with_patch can be called after \"git\n> > cherry-pick\", \"git merge\", \"git commit\", all of which have their own\n> > rerere() invocation.\n> >\n> > However, calling \"git rerere\" after a failed \"git commit\" may be\n> > destructive: it would record a resolution even though the commit has\n> > not be completed. Think of an squash commit being aborted because the\n> > user notices an error in the last minute. If that error is in a\n> > conflict resolution, that wrong resolution would be recorded.\n> \n> So, the behaviour change you observed uncovered a small bug in \"rebase\n> -i\" that was covered by the old limitation of \"rerere\" that refrained\n> from creating preimage when there already is one?\n> \n> I think removing the call to \"git rerere\" there is a safe and sensible\n> thing regardless, but perhaps authors of \"rebase -i\" had their own\n> reasons. I dunno (it is unlikely I'll have a chance to do blame and\n> digging today).\n\nI introduced this in ecfe72ff658124ea301ec7b2bb8b987689303685, as a (late)\nanswer to f131dd492f098f9f565df93df13e35c734284590.\n\nHannes, could you quickly test whether\nhttps://github.com/dscho/git/tree/interactive-rebase calls rerere twice,\ntoo? (Please call interactive rebase with the GIT_USE_REBASE_HELPER=true\nto avoid running the interactive rebase twice.)\n\nI have a hunch that it does not call rerere twice, which would be a nice\nbonus in that patch thicket (so far, I split the patches into seven patch\nseries, in addition to the ones I already sent, still have to do one\nthorough review before I'll continue submitting them).\n\nCiao,\nDscho\n"},{"id":"287449","messageId":"5744B00D.4020006@kdbg.org","threadId":"42421","inReplyTo":"alpine.DEB.2.20.1605241510370.4449@virtualbox","subject":"Re: Small rerere in rebase regression","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2016-05-24T19:48:29Z","receivedAt":"2016-05-24T19:48:29Z","isPatch":false,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 24.05.2016 um 15:18 schrieb Johannes Schindelin:\n> Hannes, could you quickly test whether\n> https://github.com/dscho/git/tree/interactive-rebase calls rerere twice,\n> too? (Please call interactive rebase with the GIT_USE_REBASE_HELPER=true\n> to avoid running the interactive rebase twice.)\n>\n> I have a hunch that it does not call rerere twice, which would be a nice\n> bonus in that patch thicket\n\nIt prints the message only once:\n\ngittest@master:1007> GIT_USE_REBASE_HELPER=true git rebase -i side\nerror: could not apply fa62fea... master\n\nWhen you have resolved this problem, run \"git rebase --continue\".\nIf you prefer to skip this patch, run \"git rebase --skip\" instead.\nTo check out the original branch and stop rebasing, run \"git rebase \n--abort\".\n\nRecorded preimage for 'a'\nCould not apply fa62fea... mastergittest@master|REBASE-i 1/1:1008> 5~\n\n(Take note of the missing LF at the end of the message.)\n\nCan this result be interpreted as another indication that the \"git \nrerere\" call in die_with_patch can be removed, or are the two git-rebase \nimplementations too different to be comparable?\n\n-- Hannes\n"},{"id":"287452","messageId":"5744BD97.4070902@kdbg.org","threadId":"42421","inReplyTo":"xmqqk2ik77cr.fsf@gitster.mtv.corp.google.com","subject":"Re: Small rerere in rebase regression","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2016-05-24T20:46:15Z","receivedAt":"2016-05-24T20:46:15Z","isPatch":false,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 24.05.2016 um 00:11 schrieb Junio C Hamano:\n> Johannes Sixt writes:\n>> However, calling \"git rerere\" after a failed \"git commit\" may be\n>> destructive: it would record a resolution even though the commit has\n>> not be completed. Think of an squash commit being aborted because the\n>> user notices an error in the last minute. If that error is in a\n>> conflict resolution, that wrong resolution would be recorded.\n>\n> So, the behaviour change you observed uncovered a small bug in\n> \"rebase -i\" that was covered by the old limitation of \"rerere\" that\n> refrained from creating preimage when there already is one?\n\nI had a closer look, and found that the failure mode I described does \nnot occur: after having resolved a commit, the user can call \"git rebase \n--continue\", but the \"git commit\" invocation that occurs there is \nguarded with a mere 'die', not 'die_with_patch'.\n\nAll other \"git commit\" guarded with 'die_with_patch' are either not \ninteractive or are after a non-conflicting merge or cherry-pick (and so \nthere was no opportunity to resolve a conflict).\n\n-- Hannes\n"},{"id":"287481","messageId":"alpine.DEB.2.20.1605250710340.4449@virtualbox","threadId":"42421","inReplyTo":"5744B00D.4020006@kdbg.org","subject":"Re: Small rerere in rebase regression","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-05-25T05:38:12Z","receivedAt":"2016-05-25T05:38:12Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Hannes,\n\nOn Tue, 24 May 2016, Johannes Sixt wrote:\n\n> Am 24.05.2016 um 15:18 schrieb Johannes Schindelin:\n> > Hannes, could you quickly test whether\n> > https://github.com/dscho/git/tree/interactive-rebase calls rerere twice,\n> > too? (Please call interactive rebase with the GIT_USE_REBASE_HELPER=true\n> > to avoid running the interactive rebase twice.)\n> >\n> > I have a hunch that it does not call rerere twice, which would be a nice\n> > bonus in that patch thicket\n> \n> It prints the message only once:\n\nThanks for confirming.\n\n> Could not apply fa62fea... mastergittest@master|REBASE-i 1/1:1008> 5~\n> \n> (Take note of the missing LF at the end of the message.)\n\nOops. Good catch, thank you! I just fixed this:\n\n\thttps://github.com/dscho/git/commit/0393d7bb2d\n\n> Can this result be interpreted as another indication that the \"git rerere\"\n> call in die_with_patch can be removed, or are the two git-rebase\n> implementations too different to be comparable?\n\nThe code used by the rebase--helper is not *quite* the same as the entire\ncherry-pick code path. For one, we run the sequencer directly, without any\nof cherry-pick's option parsing.\n\nHaving said that, yes, we inherit sequencer's rerere() call that was part\nof cherry-pick originally ever since aa1a011 (Make cherry-pick use rerere\nfor conflict resolution., 2008-08-10) (amd which was moved to\nbuiltin/revert.c in 81b50f3 (Move 'builtin-*' into a 'builtin/'\nsubdirectory, 2010-02-22) and refactored out into the sequencer in 043a449\n(sequencer: factor code out of revert builtin, 2012-01-11)).\n\nI did notice that rerere() call when implementing rebase -i's\nfunctionality in the sequencer, and dropped the extra call in\nerror_with_patch() (the libified version of rebase -i's die_with_patch\nfunction).\n\nIn short: yes, the explicit `git rerere` call can be dropped, that is\nessentially what I did in the rebase--helper branch.\n\nCiao,\nDscho\n"},{"id":"287482","messageId":"alpine.DEB.2.20.1605250747040.4449@virtualbox","threadId":"42421","inReplyTo":"alpine.DEB.2.20.1605250710340.4449@virtualbox","subject":"Re: Small rerere in rebase regression","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-05-25T05:47:54Z","receivedAt":"2016-05-25T05:47:54Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Hannes,\n\nOn Wed, 25 May 2016, Johannes Schindelin wrote:\n\n> On Tue, 24 May 2016, Johannes Sixt wrote:\n> \n> > Could not apply fa62fea... mastergittest@master|REBASE-i 1/1:1008> 5~\n> > \n> > (Take note of the missing LF at the end of the message.)\n> \n> Oops. Good catch, thank you! I just fixed this:\n> \n> \thttps://github.com/dscho/git/commit/0393d7bb2d\n\nArgh. I fixed it in https://github.com/dscho/git/commit/0704d6f425 of\ncourse.\n\nCiao,\nDscho\n"},{"id":"287687","messageId":"574875A5.9040303@kdbg.org","threadId":"42421","inReplyTo":"alpine.DEB.2.20.1605250710340.4449@virtualbox","subject":"Re: Small rerere in rebase regression","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2016-05-27T16:28:21Z","receivedAt":"2016-05-27T16:28:21Z","isPatch":false,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 25.05.2016 um 07:38 schrieb Johannes Schindelin:\n> In short: yes, the explicit `git rerere` call can be dropped, that is\n> essentially what I did in the rebase--helper branch.\n\nHere's a patch to do that.\n\n--- 8< ---\nSubject: [PATCH] rebase -i: remove an unnecessary 'rerere' invocation\n\nInteractive rebase uses 'git cherry-pick' and 'git merge' to replay\ncommits. Both invoke the 'rerere' machinery when they fail due to merge\nconflicts. Note that all code paths with these two commands also invoke\nthe shell function die_with_patch when the commands fail.\n\nSince commit 629716d2 (\"rerere: do use multiple variants\") the second\noperation of the rerere machinery can be observed by a duplicated\nmessage \"Recorded preimage for 'file'\". This second operation records\nthe same preimage as the first one and, hence, only wastes cycles.\nRemove the 'git rerere' invocation from die_with_patch.\n\nShell function die_with_patch can be called after the failure of\n\"git commit\", too, which also calls into the rerere machinery, but it\ndoes so only after a successful commit to record the resolution.\nTherefore, it is wrong to call 'git rerere' from die_with_patch after\n\"git commit\" fails.\n\nSigned-off-by: Johannes Sixt \n---\n git-rebase--interactive.sh | 1 -\n 1 file changed, 1 deletion(-)\n\ndiff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh\nindex 9d2bfb7..6e96abc 100644\n--- a/git-rebase--interactive.sh\n+++ b/git-rebase--interactive.sh\n@@ -192,7 +192,6 @@ make_patch () {\n die_with_patch () {\n \techo \"$1\" > \"$state_dir\"/stopped-sha\n \tmake_patch \"$1\"\n-\tgit rerere\n \tdie \"$2\"\n }\n \n-- \n2.9.0.rc0.40.gb3c1388\n"},{"id":"287724","messageId":"alpine.DEB.2.20.1605280839170.4449@virtualbox","threadId":"42421","inReplyTo":"574875A5.9040303@kdbg.org","subject":"Re: Small rerere in rebase regression","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-05-28T06:39:48Z","receivedAt":"2016-05-28T06:39:48Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Hannes,\n\nOn Fri, 27 May 2016, Johannes Sixt wrote:\n\n> Subject: [PATCH] rebase -i: remove an unnecessary 'rerere' invocation\n> \n> Interactive rebase uses 'git cherry-pick' and 'git merge' to replay\n> commits. Both invoke the 'rerere' machinery when they fail due to merge\n> conflicts. Note that all code paths with these two commands also invoke\n> the shell function die_with_patch when the commands fail.\n> \n> Since commit 629716d2 (\"rerere: do use multiple variants\") the second\n> operation of the rerere machinery can be observed by a duplicated\n> message \"Recorded preimage for 'file'\". This second operation records\n> the same preimage as the first one and, hence, only wastes cycles.\n> Remove the 'git rerere' invocation from die_with_patch.\n> \n> Shell function die_with_patch can be called after the failure of\n> \"git commit\", too, which also calls into the rerere machinery, but it\n> does so only after a successful commit to record the resolution.\n> Therefore, it is wrong to call 'git rerere' from die_with_patch after\n> \"git commit\" fails.\n> \n> Signed-off-by: Johannes Sixt \n\nACK\n\nCiao,\nDscho\n"}]}