{"thread":{"id":"53118","subject":"Re: git-rebase ignores or squashes GIT_REFLOG_ACTION","startedAt":"2020-03-28T02:34:47Z","lastAt":"2020-04-01T04:18:34Z","messageCount":3,"participants":["Jonathan Nieder","Ian Jackson","Elijah Newren"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"394212","messageId":"20200328023438.GA202996@google.com","threadId":"53118","inReplyTo":"24190.33234.108820.211871@chiark.greenend.org.uk","subject":"Re: git-rebase ignores or squashes GIT_REFLOG_ACTION","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2020-03-28T02:34:38Z","receivedAt":"2020-03-28T02:34:47Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nIan Jackson wrote[1]:\n\n> [ some git-debrebase invocation etc. ]\n> + git reflog\n> + egrep 'debrebase new-upstream.*checkout'\n> + rc=1\n>\n> I have looked at the log and repro'd the bug.\n>\n> git-debrebase (which lives in src:dgit but does not depend on dgit)\n> must invoke git-rebase. It sets GIT_REFLOG_ACTION so that the reflog\n> is comprehensible to the user. In previous versions of git this works\n> as expected. In 2.26.0-1 it does not.\n>\n> This is easy to reproduce by running\n> GIT_REFLOG_ACTION='zeeks!' git rebase --onto \n> with arguments implying a nontrivial rebase.\n>\n> The test suite in src:dgit, which is the checks that its\n> GIT_REFLOG_ACTION manipulation is effective, and it is this test which\n> has now failed and which is blocking the migration of git.\n>\n> git-rebrebase in sid produces quite different looking reflog entries.\n> I guess the code for generating the messages has changed and that\n> git-rebase is now *setting* GIT_REFLOG_ACTION (or the equivalent\n> internal variable) rather than adding to it.\n>\n> ISTM that to preserve the documented semantics, it is basically always\n> wrong of anything to unconditionally set GIT_REFLOG_ACTION. In\n> src:dgit I have a small bit of code to arrange to always *add* to\n> GIT_REFLOG_ACTION, if it is already set. There might be several\n> precise ways to add to GIT_REFLOG_ACTION but the failing test case\n> here should be happy with any reasonable choice.\n\nThanks for reporting.\n\nThe main relevant change is that \"git rebase\" switched its default\nbackend from \"apply\" to \"merge\". This makes it more robust by using\nthree-way merges in a similar way to \"git cherry-pick\". The \"merge\"\nbackend was historically already used for interactive rebases.\n\nI believe some reflog behavior changes were noticed in\n\n commit 980b482d28482c307648c3abbcb16ae60843c7ae\n Author: Elijah Newren \n Date: Sat Feb 15 21:36:37 2020 +0000\n\n rebase tests: mark tests specific to the am-backend with --am\n\nbut we didn't investigate at the time (shame on me). Anyway, we have\na chance to improve things now.\n\nMy first thought is to look at when rebase prepares its msg, in\nbuiltin/rebase.c#set_reflog_action:\n\n\tif (!is_merge(options))\n\t\treturn;\n\n\tenv = getenv(GIT_REFLOG_ACTION_ENVIRONMENT);\n\tif (env && strcmp(\"rebase\", env))\n\t\treturn; /* only override it if it is \"rebase\" */\n\n\tstrbuf_addf(&buf, \"rebase (%s)\", options->action);\n\tsetenv(GIT_REFLOG_ACTION_ENVIRONMENT, buf.buf, 1);\n\tstrbuf_release(&buf);\n\nIn the --am case, is_merge is false and this code isn't run. But in\nour case, GIT_REFLOG_ACTION is not rebase, so this code still\nshouldn't be run.\n\nMy next thought is to look at when this function was changed:\n\n commit c2417d3af7574adc1fb14f7df31b862aa9551e2e\n Author: Elijah Newren \n Date: Sat Feb 15 21:36:36 2020 +0000\n\n rebase: drop '-i' from the reflog for interactive-based rebases\n\nIf I am reading\n\n commit 13a5a9f0fdcf36270dcc2dcb7752c281bbea06f1\n Author: Johannes Schindelin \n Date: Thu Nov 29 11:09:21 2018 -0800\n\n rebase: fix GIT_REFLOG_ACTION regression\n\ncorrectly, then dgit requires that to be '-i' for interactive rebases.\nAre we sure that that's not the issue here?\n\nThanks,\nJonathan\n\n[1] https://bugs.debian.org/955152\n"},{"id":"394221","messageId":"24191.15179.253629.878217@chiark.greenend.org.uk","threadId":"53118","inReplyTo":"20200328023438.GA202996@google.com","subject":"Re: git-rebase ignores or squashes GIT_REFLOG_ACTION","fromName":"Ian Jackson","fromEmail":"ijackson@chiark.greenend.org.uk","sentAt":"2020-03-28T11:55:55Z","receivedAt":"2020-03-28T12:24:51Z","isPatch":false,"sender":{"key":"ijackson@chiark.greenend.org.uk","avatar":null},"body":"Jonathan Nieder writes (\"Re: git-rebase ignores or squashes GIT_REFLOG_ACTION\"):\n> The main relevant change is that \"git rebase\" switched its default\n> backend from \"apply\" to \"merge\". This makes it more robust by using\n> three-way merges in a similar way to \"git cherry-pick\". The \"merge\"\n> backend was historically already used for interactive rebases.\n\nAh. Interesting. A co-worker had a case recently where \"merge\" did\nthe wrong thing (silently misapplied a hunk!) but \"apply\" did the\nright thing and I remmber thinking that \"merge\" ought to be the\ndefault (since it can do a better job by using all of the available\ninformation). So I applaud that change.\n\n> If I am reading\n> \n> commit 13a5a9f0fdcf36270dcc2dcb7752c281bbea06f1\n> Author: Johannes Schindelin \n> Date: Thu Nov 29 11:09:21 2018 -0800\n> \n> rebase: fix GIT_REFLOG_ACTION regression\n> \n> correctly, then dgit requires that to be '-i' for interactive rebases.\n> Are we sure that that's not the issue here?\n\ngit-debrebase invokes git-rebase both with and without -i, depending\non the user's own choice. In this case, git-debrebase is invoking\ngit-rebase *without* -i.\n\nBut maybe I have misunderstood. Maybe you mean `are you sure the test\ncase doesn't demand that \"-i\" appears in the reflog' ? In which case,\nyes. The failing line in the test case (which is a shell script) is:\n git reflog | egrep 'debrebase new-upstream.*checkout'\n\nHere is a repro for the problem. Run the attached script, optionally\nwith \"-i\" as a single argument. The script will \"rm -rf d\" and\nrecreate it. It sets GIT_REFLOG_ACTION=\"plim\" and does a nontrivial\nrebase in a fresh tree, and prints the resulting reflog.\n\nOn older git, without -i, quoting only the relevant bits:\n fc9165e HEAD@{0}: rebase finished: returning to refs/heads/master\n fc9165e HEAD@{1}: plim: 3\n ebf6515 HEAD@{2}: plim: checkout HEAD~2\nThis is right except for the final finish message.\n\nOn older git, with -i:\n 4f6cff0 HEAD@{0}: plim: checkout HEAD~2: returning to refs/heads/master\n 4f6cff0 HEAD@{1}: plim: checkout HEAD~2: 3\n 9f5e72d HEAD@{2}: plim: checkout HEAD~2\nThis seems to be wrong for all but the initial checkout.\n\nOn newer git, I get this output both with and without -i:\n 30067e1 HEAD@{0}: rebase (finish): returning to refs/heads/master\n 30067e1 HEAD@{1}: rebase (pick): 3\n 8a9a5fd HEAD@{2}: rebase (start): checkout HEAD~2\nThis is wrong because it doesn't mention \"plim\" at all. That's\nwhat is spotted by my test case.\n\nI think the best output would be this:\n 30067e1 HEAD@{0}: plim (finish): returning to refs/heads/master\n 30067e1 HEAD@{1}: plim (pick): 3\n 8a9a5fd HEAD@{2}: plim (start): checkout HEAD~2\nor this:\n 30067e1 HEAD@{0}: plim: rebase (finish): returning to refs/heads/master\n 30067e1 HEAD@{1}: plim: rebase (pick): 3\n 8a9a5fd HEAD@{2}: plim: rebase (start): checkout HEAD~2\n\nWhich of those two is better depends on whether you think callers\nought to put something to do with \"rebase\" in GIT_REFLOG_ACTION\nsomewhere. In this particular case, git-debrebase sets it to\nsomething like\n debrebase new-upstream $new_version: rebase\nbecause it thinks that its GIT_REFLOG_ACTION value will replace\nthe usual \"rebase\", rather than supplementing it.\n\nI think either choice on git-rebase's part would be reasonable and the\noutput with current git-debrebase behaviour is good enough either way.\nThe former choice on git-rebase's part would produce the prettiest\noutput with my current setting of GIT_REFLOG_ACTION. And it allows\nthe caller the most control over the reflog messages. Unconditionally\nadding the top-level command being invoked could be simulated by the\ncaller (whereas adding things like \"(finish)\" cannot). So if it were\nup to me I would have git-rebase produce the first of my two expected\noutputs, ie\n 30067e1 HEAD@{0}: plim (finish): returning to refs/heads/master\netc.\n\nBut my test case merely demands that \"plim\" appears *somewhere*.\n\n\nAside:\n\nObviously with an interactive rebase, it might well stop, and the user\nwill then later presumably git-rebase --continue (or maybe --abort)\netc. Those commands will no longer (necessarily) have\nGIT_REFLOG_ACTION in their environment. So, with the obvious\nimplementation, depending on the circumstances the reflog for later\nparts of the rebase might not contain all the relevant information.\n\nThis is unavoidable unless git-rebase were to make a note of the value\nof GIT_REFLOG_ACTION somewhere - and even in that case, it wouldn't\naffect git commit (which is often a thing that is run in the middle of\nan interactive rebase) unless this note had a global effect. It seems\nto me that it would be a bad idea for git-rebase to do anything like\nthis. Not only would it have to squirrel away GIT_REFLOG_ACTION and\n(perhaps surprisingly) honour it later, it would presumably have to\ntry to combine it with the value in force during later commands.\n\nThis would all be complicated and provide plenty of opportunity for\nweird corner case bugs, in order to do something which (to say the\nleast) it's not clear is desirable.\n\nInstead, I think that the GIT_REFLOG_ACTION in the environment of\ngit-rebase should take effect for all the things that are done\nby or on behalf of that git-rebase command until that git-rebase\ncommand has exited. Future commands should honour the\nGIT_REFLOG_ACTION in force at that later time. I hope you\nagree :-), and I'm giving this aside for completeness.\n\n\nI hope this is helpful.\n\nIan.\n\n\n\n#!/bin/bash\nset -e\nrm -rf d\nmkdir d\ncd d\n\nexport EDITOR=true\nexport VISUAL=true\n\ngit init\n\nfor x in 1 2 3; do\n echo $x >$x\n git add $x\n git commit -m $x\ndone\n\ngit branch old\n\nGIT_REFLOG_ACTION=plim git rebase \"$@\" --onto HEAD~2 HEAD~1\n\ngit reflog |cat\n\n\n\n-- \nIan Jackson These opinions are my own.\n\nIf I emailed you from an address @fyvzl.net or @evade.org.uk, that is\na private address which bypasses my fierce spamfilter.\n"},{"id":"394465","messageId":"CABPp-BEiRtG=8xpXy-HYLDGdh9v7uwREuiCvwr--8UqrZ_pz2w@mail.gmail.com","threadId":"53118","inReplyTo":"24191.15179.253629.878217@chiark.greenend.org.uk","subject":"Re: git-rebase ignores or squashes GIT_REFLOG_ACTION","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2020-04-01T04:18:20Z","receivedAt":"2020-04-01T04:18:34Z","isPatch":false,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Sat, Mar 28, 2020 at 4:55 AM Ian Jackson\n wrote:\n>\n> Jonathan Nieder writes (\"Re: git-rebase ignores or squashes GIT_REFLOG_ACTION\"):\n> > The main relevant change is that \"git rebase\" switched its default\n> > backend from \"apply\" to \"merge\". This makes it more robust by using\n> > three-way merges in a similar way to \"git cherry-pick\". The \"merge\"\n> > backend was historically already used for interactive rebases.\n>\n> Ah. Interesting. A co-worker had a case recently where \"merge\" did\n> the wrong thing (silently misapplied a hunk!) but \"apply\" did the\n> right thing and I remmber thinking that \"merge\" ought to be the\n> default (since it can do a better job by using all of the available\n> information). So I applaud that change.\n>\n> > If I am reading\n> >\n> > commit 13a5a9f0fdcf36270dcc2dcb7752c281bbea06f1\n> > Author: Johannes Schindelin \n> > Date: Thu Nov 29 11:09:21 2018 -0800\n> >\n> > rebase: fix GIT_REFLOG_ACTION regression\n> >\n> > correctly, then dgit requires that to be '-i' for interactive rebases.\n> > Are we sure that that's not the issue here?\n>\n> git-debrebase invokes git-rebase both with and without -i, depending\n> on the user's own choice. In this case, git-debrebase is invoking\n> git-rebase *without* -i.\n>\n> But maybe I have misunderstood. Maybe you mean `are you sure the test\n> case doesn't demand that \"-i\" appears in the reflog' ? In which case,\n> yes. The failing line in the test case (which is a shell script) is:\n> git reflog | egrep 'debrebase new-upstream.*checkout'\n>\n> Here is a repro for the problem. Run the attached script, optionally\n> with \"-i\" as a single argument. The script will \"rm -rf d\" and\n> recreate it. It sets GIT_REFLOG_ACTION=\"plim\" and does a nontrivial\n> rebase in a fresh tree, and prints the resulting reflog.\n>\n> On older git, without -i, quoting only the relevant bits:\n> fc9165e HEAD@{0}: rebase finished: returning to refs/heads/master\n> fc9165e HEAD@{1}: plim: 3\n> ebf6515 HEAD@{2}: plim: checkout HEAD~2\n> This is right except for the final finish message.\n>\n> On older git, with -i:\n> 4f6cff0 HEAD@{0}: plim: checkout HEAD~2: returning to refs/heads/master\n> 4f6cff0 HEAD@{1}: plim: checkout HEAD~2: 3\n> 9f5e72d HEAD@{2}: plim: checkout HEAD~2\n> This seems to be wrong for all but the initial checkout.\n>\n> On newer git, I get this output both with and without -i:\n> 30067e1 HEAD@{0}: rebase (finish): returning to refs/heads/master\n> 30067e1 HEAD@{1}: rebase (pick): 3\n> 8a9a5fd HEAD@{2}: rebase (start): checkout HEAD~2\n> This is wrong because it doesn't mention \"plim\" at all. That's\n> what is spotted by my test case.\n>\n> I think the best output would be this:\n> 30067e1 HEAD@{0}: plim (finish): returning to refs/heads/master\n> 30067e1 HEAD@{1}: plim (pick): 3\n> 8a9a5fd HEAD@{2}: plim (start): checkout HEAD~2\n> or this:\n> 30067e1 HEAD@{0}: plim: rebase (finish): returning to refs/heads/master\n> 30067e1 HEAD@{1}: plim: rebase (pick): 3\n> 8a9a5fd HEAD@{2}: plim: rebase (start): checkout HEAD~2\n>\n> Which of those two is better depends on whether you think callers\n> ought to put something to do with \"rebase\" in GIT_REFLOG_ACTION\n> somewhere. In this particular case, git-debrebase sets it to\n> something like\n> debrebase new-upstream $new_version: rebase\n> because it thinks that its GIT_REFLOG_ACTION value will replace\n> the usual \"rebase\", rather than supplementing it.\n>\n> I think either choice on git-rebase's part would be reasonable and the\n> output with current git-debrebase behaviour is good enough either way.\n> The former choice on git-rebase's part would produce the prettiest\n> output with my current setting of GIT_REFLOG_ACTION. And it allows\n> the caller the most control over the reflog messages. Unconditionally\n> adding the top-level command being invoked could be simulated by the\n> caller (whereas adding things like \"(finish)\" cannot). So if it were\n> up to me I would have git-rebase produce the first of my two expected\n> outputs, ie\n> 30067e1 HEAD@{0}: plim (finish): returning to refs/heads/master\n> etc.\n>\n> But my test case merely demands that \"plim\" appears *somewhere*.\n>\n>\n> Aside:\n>\n> Obviously with an interactive rebase, it might well stop, and the user\n> will then later presumably git-rebase --continue (or maybe --abort)\n> etc. Those commands will no longer (necessarily) have\n> GIT_REFLOG_ACTION in their environment. So, with the obvious\n> implementation, depending on the circumstances the reflog for later\n> parts of the rebase might not contain all the relevant information.\n>\n> This is unavoidable unless git-rebase were to make a note of the value\n> of GIT_REFLOG_ACTION somewhere - and even in that case, it wouldn't\n> affect git commit (which is often a thing that is run in the middle of\n> an interactive rebase) unless this note had a global effect. It seems\n> to me that it would be a bad idea for git-rebase to do anything like\n> this. Not only would it have to squirrel away GIT_REFLOG_ACTION and\n> (perhaps surprisingly) honour it later, it would presumably have to\n> try to combine it with the value in force during later commands.\n>\n> This would all be complicated and provide plenty of opportunity for\n> weird corner case bugs, in order to do something which (to say the\n> least) it's not clear is desirable.\n>\n> Instead, I think that the GIT_REFLOG_ACTION in the environment of\n> git-rebase should take effect for all the things that are done\n> by or on behalf of that git-rebase command until that git-rebase\n> command has exited. Future commands should honour the\n> GIT_REFLOG_ACTION in force at that later time. I hope you\n> agree :-), and I'm giving this aside for completeness.\n>\n>\n> I hope this is helpful.\n\nThanks for the report and details. Sorry for my slow response; I've\ngot this thread marked as important, just haven't gotten back to it\nwith the other things in the queue yet.\n"}]}