{"thread":{"id":"51630","subject":"minor interactive rebase regression: HEAD points to wrong commit while rewording","startedAt":"2019-08-12T17:50:52Z","lastAt":"2019-08-15T16:54:56Z","messageCount":10,"participants":["SZEDER Gábor","Junio C Hamano","Phillip Wood","Johannes Schindelin"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"380322","messageId":"20190812175046.GM20404@szeder.dev","threadId":"51630","inReplyTo":null,"subject":"minor interactive rebase regression: HEAD points to wrong commit while rewording","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-08-12T17:50:46Z","receivedAt":"2019-08-12T17:50:52Z","isPatch":false,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"\nWhen running interactive rebase to reword a commit message, I would\nexpect that the commit whose message I'm rewording is checked out.\nThis is not quite the case when rewording multiple subsequent commit\nmessages.\n\nLet's start with four commits, and start an interactive rebase from\nthe first commit:\n\n  $ git log --oneline\n  5835aa1 (HEAD -> master) fourth\n  64ecc64 third\n  d5fad83 second\n  384b86f first\n  $ git rebase -i 384b86f\n\nUpdate the instruction sheet to edit the log messages of two\nsubsequent commits:\n\n  r d5fad83 second\n  r 64ecc64 third\n  pick 5835aa1 fourth\n\nNow, after the editor opens up the second commit's log message, start\na new terminal and check where HEAD is pointing to:\n\n  ~/tmp/reword (master|REBASE-i 1/3)$ head -n1 .git/COMMIT_EDITMSG \n  second\n  ~/tmp/reword (master|REBASE-i 1/3)$ git log --oneline -1\n  d5fad83 (HEAD) second\n\nSo far so good.\nSave the updated commit message, and after the editor opens up the\nthird commit's log message, check again where HEAD is pointing to now:\n\n  ~/tmp/reword (master +|REBASE-i 2/3)$ head -n1 .git/COMMIT_EDITMSG \n  third\n  ~/tmp/reword (master +|REBASE-i 2/3)$ git log --oneline -1\n  c3db735 (HEAD) second - updated\n\nAs you can see, HEAD still points to the (now rewritten) second\ncommit.\n\nIt's only HEAD, though: notice the '+' in the git prompt, indicating\nthat both the worktree and index are dirty.  And indeed, they both\nalready match the state of the currently reworded, i.e. third, commit:\n\n  ~/tmp/reword (master +|REBASE-i 2/3)$ cat file\n  third\n\nThis is good, because even though HEAD has not been updated yet, it\nalready allows users to take a look at the \"big picture\", i.e. actual\nfile contents, in case the diff included in the commit message\ntemplate doesn't show enough context.\n\nThis behavior changed in commit 18633e1a22 (rebase -i: use the\nrebase--helper builtin, 2017-02-09); prior to that HEAD pointed to the\nthird commit while editing its log message.\n\nIt's important to reword subsequent commits.  When rewording multiple,\nbut non subsequent commits (e.g. reword, pick, reword in the\ninstruction sheet), then HEAD is pointing to the right commits during\nboth rewords.\n\n\n"},{"id":"380324","messageId":"xmqqsgq69rky.fsf@gitster-ct.c.googlers.com","threadId":"51630","inReplyTo":"20190812175046.GM20404@szeder.dev","subject":"Re: minor interactive rebase regression: HEAD points to wrong commit while rewording","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-08-12T18:17:01Z","receivedAt":"2019-08-12T18:17:09Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"SZEDER Gábor <szeder.dev@gmail.com> writes:\n\n> ...\n> So far so good.\n> Save the updated commit message, and after the editor opens up the\n> third commit's log message, check again where HEAD is pointing to now:\n>\n>   ~/tmp/reword (master +|REBASE-i 2/3)$ head -n1 .git/COMMIT_EDITMSG \n>   third\n>   ~/tmp/reword (master +|REBASE-i 2/3)$ git log --oneline -1\n>   c3db735 (HEAD) second - updated\n>\n> As you can see, HEAD still points to the (now rewritten) second\n> commit.\n>\n> It's only HEAD,...\n\nYuck.\n\nThat would still be annoying to some people and outright buggy to\nothers, if their workflow relies on HEAD (e.g. compare with HEAD\nwhile reviewing the log messsage) and then now they instead need to\nadjust (e.g. compare with the index instead).  Perhaps you are one\nof them (apparently I am not, as I did not notice the behaviour\nchange until you pointed it out here).\n\n"},{"id":"380327","messageId":"20190812185618.GN20404@szeder.dev","threadId":"51630","inReplyTo":"xmqqsgq69rky.fsf@gitster-ct.c.googlers.com","subject":"Re: minor interactive rebase regression: HEAD points to wrong commit while rewording","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-08-12T18:56:18Z","receivedAt":"2019-08-12T18:56:25Z","isPatch":false,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Mon, Aug 12, 2019 at 11:17:01AM -0700, Junio C Hamano wrote:\n> SZEDER Gábor <szeder.dev@gmail.com> writes:\n> \n> > ...\n> > So far so good.\n> > Save the updated commit message, and after the editor opens up the\n> > third commit's log message, check again where HEAD is pointing to now:\n> >\n> >   ~/tmp/reword (master +|REBASE-i 2/3)$ head -n1 .git/COMMIT_EDITMSG \n> >   third\n> >   ~/tmp/reword (master +|REBASE-i 2/3)$ git log --oneline -1\n> >   c3db735 (HEAD) second - updated\n> >\n> > As you can see, HEAD still points to the (now rewritten) second\n> > commit.\n> >\n> > It's only HEAD,...\n> \n> Yuck.\n> \n> That would still be annoying to some people and outright buggy to\n> others, if their workflow relies on HEAD (e.g. compare with HEAD\n> while reviewing the log messsage) and then now they instead need to\n> adjust (e.g. compare with the index instead).\n\nWell, it's caused by a 2.5 year old commit, so either all those people\nhave been lucky so far (or just don't do subsequent rewords?), or\nthose who stumbled upon it already adjusted without complaining to us.\n\n> Perhaps you are one of them\n\nNo; during the second reword I ran 'git log -2' in the other terminal\n(wanted to copy a sentence from the previous, just reworded commit),\nand noticed that the two listed commits were \"off by one\".\n\n\nBTW, rewriting the first commit appears to be important.  When doing\nan edit-reword of two subsequent commits, then this issue only happens\nif the edit actually rewrites the commit.  However, when, instead of\nediting anything, I do a 'git rebase --continue' right away, then HEAD\npoints to where it should during the subsequent reword.\n\nAnyway, Cc:-ing Dscho for his rebase expertise.\n\n"},{"id":"380340","messageId":"2a7d69a9-cb3e-eb84-188f-5713876f6d78@gmail.com","threadId":"51630","inReplyTo":"20190812175046.GM20404@szeder.dev","subject":"Re: minor interactive rebase regression: HEAD points to wrong commit while rewording","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2019-08-12T20:28:52Z","receivedAt":"2019-08-12T20:28:58Z","isPatch":false,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 12/08/2019 18:50, SZEDER Gábor wrote:\n> \n> When running interactive rebase to reword a commit message, I would\n> expect that the commit whose message I'm rewording is checked out.\n> This is not quite the case when rewording multiple subsequent commit\n> messages.\n> \n> Let's start with four commits, and start an interactive rebase from\n> the first commit:\n> \n>    $ git log --oneline\n>    5835aa1 (HEAD -> master) fourth\n>    64ecc64 third\n>    d5fad83 second\n>    384b86f first\n>    $ git rebase -i 384b86f\n> \n> Update the instruction sheet to edit the log messages of two\n> subsequent commits:\n> \n>    r d5fad83 second\n>    r 64ecc64 third\n>    pick 5835aa1 fourth\n> \n> Now, after the editor opens up the second commit's log message, start\n> a new terminal and check where HEAD is pointing to:\n> \n>    ~/tmp/reword (master|REBASE-i 1/3)$ head -n1 .git/COMMIT_EDITMSG\n>    second\n>    ~/tmp/reword (master|REBASE-i 1/3)$ git log --oneline -1\n>    d5fad83 (HEAD) second\n> \n> So far so good.\n\nBecause the sequencer can fast-forwarded to second from first it does \nthat and then run 'commit --amend' to do the reword.\n\n> Save the updated commit message, and after the editor opens up the\n> third commit's log message, check again where HEAD is pointing to now:\n> \n>    ~/tmp/reword (master +|REBASE-i 2/3)$ head -n1 .git/COMMIT_EDITMSG\n>    third\n>    ~/tmp/reword (master +|REBASE-i 2/3)$ git log --oneline -1\n>    c3db735 (HEAD) second - updated\n\nAs second has been updated the sequencer cannot fast-forward to third so \nit cherry-picks third and then passes --edit when it runs 'git commit' \nto commit the cherry-pick. HEAD is updated once the reworded commit has \nbeen created.\n\nI think the scripted rebase always ran cherry-pick and then ran 'commit \n--amend' afterwards if the commit was being reworded. The C \nimplementation is more efficient as it avoids creating an redundant \ncommit but has the side effect that HEAD is not updated before the \nreword which was surprising here.\n\nI don't think I've ever looked at HEAD while rewording, my shell prompt \ngets the current pick from .git/rebase-merge/done so does not look at \nHEAD. While it might seem odd if the user looks at HEAD it's quite nice \nnot to create a new commit only to amend it straight away when it's \nreworded. We have REBASE_HEAD which always points to the current pick - \nHEAD is also an unreliable indicator of the current pick if there are \nconflicts.\n\nBest Wishes\n\nPhillip\n\n> As you can see, HEAD still points to the (now rewritten) second\n> commit.\n> \n> It's only HEAD, though: notice the '+' in the git prompt, indicating\n> that both the worktree and index are dirty.  And indeed, they both\n> already match the state of the currently reworded, i.e. third, commit:\n> \n>    ~/tmp/reword (master +|REBASE-i 2/3)$ cat file\n>    third\n> \n> This is good, because even though HEAD has not been updated yet, it\n> already allows users to take a look at the \"big picture\", i.e. actual\n> file contents, in case the diff included in the commit message\n> template doesn't show enough context.\n> \n> This behavior changed in commit 18633e1a22 (rebase -i: use the\n> rebase--helper builtin, 2017-02-09); prior to that HEAD pointed to the\n> third commit while editing its log message.\n> \n> It's important to reword subsequent commits.  When rewording multiple,\n> but non subsequent commits (e.g. reword, pick, reword in the\n> instruction sheet), then HEAD is pointing to the right commits during\n> both rewords.\n> \n> \n"},{"id":"380459","messageId":"nycvar.QRO.7.76.6.1908142241080.656@tvgsbejvaqbjf.bet","threadId":"51630","inReplyTo":"2a7d69a9-cb3e-eb84-188f-5713876f6d78@gmail.com","subject":"Re: minor interactive rebase regression: HEAD points to wrong commit while rewording","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-08-14T20:45:22Z","receivedAt":"2019-08-14T20:46:08Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Mon, 12 Aug 2019, Phillip Wood wrote:\n\n> On 12/08/2019 18:50, SZEDER Gábor wrote:\n> >\n> > When running interactive rebase to reword a commit message, I would\n> > expect that the commit whose message I'm rewording is checked out.\n> > This is not quite the case when rewording multiple subsequent commit\n> > messages.\n> >\n> > Let's start with four commits, and start an interactive rebase from\n> > the first commit:\n> >\n> >    $ git log --oneline\n> >    5835aa1 (HEAD -> master) fourth\n> >    64ecc64 third\n> >    d5fad83 second\n> >    384b86f first\n> >    $ git rebase -i 384b86f\n> >\n> > Update the instruction sheet to edit the log messages of two\n> > subsequent commits:\n> >\n> >    r d5fad83 second\n> >    r 64ecc64 third\n> >    pick 5835aa1 fourth\n> >\n> > Now, after the editor opens up the second commit's log message, start\n> > a new terminal and check where HEAD is pointing to:\n> >\n> >    ~/tmp/reword (master|REBASE-i 1/3)$ head -n1 .git/COMMIT_EDITMSG\n> >    second\n> >    ~/tmp/reword (master|REBASE-i 1/3)$ git log --oneline -1\n> >    d5fad83 (HEAD) second\n> >\n> > So far so good.\n>\n> Because the sequencer can fast-forwarded to second from first it does that and\n> then run 'commit --amend' to do the reword.\n>\n> > Save the updated commit message, and after the editor opens up the\n> > third commit's log message, check again where HEAD is pointing to now:\n> >\n> >    ~/tmp/reword (master +|REBASE-i 2/3)$ head -n1 .git/COMMIT_EDITMSG\n> >    third\n> >    ~/tmp/reword (master +|REBASE-i 2/3)$ git log --oneline -1\n> >    c3db735 (HEAD) second - updated\n>\n> As second has been updated the sequencer cannot fast-forward to third so it\n> cherry-picks third and then passes --edit when it runs 'git commit' to commit\n> the cherry-pick. HEAD is updated once the reworded commit has been created.\n>\n> I think the scripted rebase always ran cherry-pick and then ran 'commit\n> --amend' afterwards if the commit was being reworded. The C implementation is\n> more efficient as it avoids creating an redundant commit but has the side\n> effect that HEAD is not updated before the reword which was surprising here.\n\nIndeed, that was even intentional.\n\n> I don't think I've ever looked at HEAD while rewording, my shell prompt gets\n> the current pick from .git/rebase-merge/done so does not look at HEAD. While\n> it might seem odd if the user looks at HEAD it's quite nice not to create a\n> new commit only to amend it straight away when it's reworded. We have\n> REBASE_HEAD which always points to the current pick - HEAD is also an\n> unreliable indicator of the current pick if there are conflicts.\n\nThat is interesting; I would never have thought about scripting around\n`reword`.\n\nHowever, I am reluctant to accept the performance impact: in the long\nrun, I would love to have an interactive rebase that actually only\nupdates `HEAD` (and the worktree) when interrupting the rebase (via\n`break` or `edit`), and `reword` does not qualify for \"interrupting\" in\nmy mind.\n\nCiao,\nDscho\n"},{"id":"380460","messageId":"20190814212036.GQ20404@szeder.dev","threadId":"51630","inReplyTo":"2a7d69a9-cb3e-eb84-188f-5713876f6d78@gmail.com","subject":"Re: minor interactive rebase regression: HEAD points to wrong commit while rewording","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-08-14T21:20:36Z","receivedAt":"2019-08-14T21:20:44Z","isPatch":false,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Mon, Aug 12, 2019 at 09:28:52PM +0100, Phillip Wood wrote:\n> >Save the updated commit message, and after the editor opens up the\n> >third commit's log message, check again where HEAD is pointing to now:\n> >\n> >   ~/tmp/reword (master +|REBASE-i 2/3)$ head -n1 .git/COMMIT_EDITMSG\n> >   third\n> >   ~/tmp/reword (master +|REBASE-i 2/3)$ git log --oneline -1\n> >   c3db735 (HEAD) second - updated\n> \n> As second has been updated the sequencer cannot fast-forward to third so it\n> cherry-picks third and then passes --edit when it runs 'git commit' to\n> commit the cherry-pick. HEAD is updated once the reworded commit has been\n> created.\n> \n> I think the scripted rebase always ran cherry-pick and then ran 'commit\n> --amend' afterwards if the commit was being reworded. The C implementation\n> is more efficient as it avoids creating an redundant commit but has the side\n> effect that HEAD is not updated before the reword which was surprising here.\n\nI'm not sure about this more efficient thing.  I mean, 'git rebase' is\nabout to launch the editor to let the user type something...  Compared\nto that the time spent in creating an extra commit object is surely\nnegligible.\n\n"},{"id":"380462","messageId":"20190814214047.GR20404@szeder.dev","threadId":"51630","inReplyTo":"nycvar.QRO.7.76.6.1908142241080.656@tvgsbejvaqbjf.bet","subject":"Re: minor interactive rebase regression: HEAD points to wrong commit while rewording","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-08-14T21:40:48Z","receivedAt":"2019-08-14T21:40:54Z","isPatch":false,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Wed, Aug 14, 2019 at 10:45:22PM +0200, Johannes Schindelin wrote:\n> However, I am reluctant to accept the performance impact: in the long\n> run, I would love to have an interactive rebase that actually only\n> updates `HEAD` (and the worktree) when interrupting the rebase (via\n> `break` or `edit`), and `reword` does not qualify for \"interrupting\" in\n> my mind.\n\nIt's crucial that users can look at the files in the currently\nreworded commit while it is being reworded.\n\n"},{"id":"380471","messageId":"026d6615-3420-eb31-8c93-a6245f3be503@gmail.com","threadId":"51630","inReplyTo":"20190814212036.GQ20404@szeder.dev","subject":"Re: minor interactive rebase regression: HEAD points to wrong commit while rewording","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2019-08-15T13:47:17Z","receivedAt":"2019-08-15T13:47:23Z","isPatch":false,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 14/08/2019 22:20, SZEDER Gábor wrote:\n> On Mon, Aug 12, 2019 at 09:28:52PM +0100, Phillip Wood wrote:\n>>> Save the updated commit message, and after the editor opens up the\n>>> third commit's log message, check again where HEAD is pointing to now:\n>>>\n>>>    ~/tmp/reword (master +|REBASE-i 2/3)$ head -n1 .git/COMMIT_EDITMSG\n>>>    third\n>>>    ~/tmp/reword (master +|REBASE-i 2/3)$ git log --oneline -1\n>>>    c3db735 (HEAD) second - updated\n>>\n>> As second has been updated the sequencer cannot fast-forward to third so it\n>> cherry-picks third and then passes --edit when it runs 'git commit' to\n>> commit the cherry-pick. HEAD is updated once the reworded commit has been\n>> created.\n>>\n>> I think the scripted rebase always ran cherry-pick and then ran 'commit\n>> --amend' afterwards if the commit was being reworded. The C implementation\n>> is more efficient as it avoids creating an redundant commit but has the side\n>> effect that HEAD is not updated before the reword which was surprising here.\n> \n> I'm not sure about this more efficient thing.  I mean, 'git rebase' is\n> about to launch the editor to let the user type something...  Compared\n> to that the time spent in creating an extra commit object is surely\n> negligible.\n\nI changed the sequencer to always commit the cherry-pick and then run \n'git commit --amend' for rewords [1]. Running\n\n\ttime env GIT_EDITOR=true GIT_SEQUENCE_EDITOR='sed -i s/pick/reword/' \n../bin-wrappers/git rebase -i --root\n\nover 100 commits I cannot see any real difference in the timings between \nmaster and that branch. Any difference is within the variation of the \ntimes of multiple runs. The change also fixes a bug when rewording a \nre-arranged root commit.\n\nBest Wishes\n\nPhillip\n\n[1] https://github.com/phillipwood/git/commits/wip/rebase-reword-update-head\n"},{"id":"380480","messageId":"xmqqpnl64c94.fsf@gitster-ct.c.googlers.com","threadId":"51630","inReplyTo":"026d6615-3420-eb31-8c93-a6245f3be503@gmail.com","subject":"Re: minor interactive rebase regression: HEAD points to wrong commit while rewording","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-08-15T16:36:07Z","receivedAt":"2019-08-15T16:36:12Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood <phillip.wood123@gmail.com> writes:\n\n> On 14/08/2019 22:20, SZEDER Gábor wrote:\n>\n> I changed the sequencer to always commit the cherry-pick and then run\n> 'git commit --amend' for rewords [1]. Running\n>\n> \ttime env GIT_EDITOR=true GIT_SEQUENCE_EDITOR='sed -i\n> s/pick/reword/' ../bin-wrappers/git rebase -i --root\n>\n> over 100 commits I cannot see any real difference in the timings\n> between master and that branch. Any difference is within the variation\n> of the times of multiple runs. The change also fixes a bug when\n> rewording a re-arranged root commit.\n\nI guess that settles the \"efficiency\" story; besides, showing a\nwrong intermediate state to users and scripts efficiently is just as\nbad as showing a wrong state less efficiently.\n\n"},{"id":"380482","messageId":"xmqqlfvu4be3.fsf@gitster-ct.c.googlers.com","threadId":"51630","inReplyTo":"xmqqpnl64c94.fsf@gitster-ct.c.googlers.com","subject":"Re: minor interactive rebase regression: HEAD points to wrong commit while rewording","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-08-15T16:54:44Z","receivedAt":"2019-08-15T16:54:56Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Phillip Wood <phillip.wood123@gmail.com> writes:\n>\n>> On 14/08/2019 22:20, SZEDER Gábor wrote:\n>>\n>> I changed the sequencer to always commit the cherry-pick and then run\n>> 'git commit --amend' for rewords [1]. Running\n>>\n>> \ttime env GIT_EDITOR=true GIT_SEQUENCE_EDITOR='sed -i\n>> s/pick/reword/' ../bin-wrappers/git rebase -i --root\n>>\n>> over 100 commits I cannot see any real difference in the timings\n>> between master and that branch. Any difference is within the variation\n>> of the times of multiple runs. The change also fixes a bug when\n>> rewording a re-arranged root commit.\n>\n> I guess that settles the \"efficiency\" story; besides, showing a\n> wrong intermediate state to users and scripts efficiently is just as\n> bad as showing a wrong state less efficiently.\n\nNeedless to say, there is no need for us to make things less\nefficient when we do not need to.  If there are two back-to-back\n\"pick\"s, and there is no hook and the like that gives an end-user\nand/or a script a reliable trigger point that can be used to\n\"observe\" and \"utilize\" the state immediately after the first\n\"pick\", we do not have to update the HEAD (or update the working\ntree for that matter) between these two \"pick\"s.\n\nThe external process being able to observe alone is not an enough\nreason to force us go inefficient here---if the triggering event is\nlike \"spawn an editor and wait for it to exit\", that gives the\nexternal process to not just \"observe\" the updated state after the\nfirst pick, but also \"utilize\" that state (e.g. build, compare with\nsome other tree, etc.) knowing that the second \"pick\" does not start\nchanging the state until it relinquishes the control.\n\nBut unless the external process can reliably utilize the updated\nstate, it is of not much use to just be able to notice the change.\nFor example, an argument like \"constantly monitoring changes in the\n$GIT_DIR/rebase-merge directory, we can notice that each 'pick' gets\ndone\" is not a reason to force us to update HEAD every time any\ninstruction is consumed in the todo file, as the next \"pick\" cannot\nbe reliably delayed by somebody who is merely observing the\ndirectory to utilize the updated state.\n\n\n"}]}