{"thread":{"id":"56985","subject":"[BUG REPORT] `git rebase --exec` shouldn't run the exec command when there is nothing to rebase","startedAt":"2021-11-26T13:35:47Z","lastAt":"2021-11-30T11:09:55Z","messageCount":6,"participants":["Nikita Bobko","Ævar Arnfjörð Bjarmason","Elijah Newren","Taylor Blau","Phillip Wood"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"442361","messageId":"CAMJzOtyw78-8gt3oGscN7KUzpzRRWtAQuGfcJ+R_Fjoom9Lexw@mail.gmail.com","threadId":"56985","inReplyTo":null,"subject":"[BUG REPORT] `git rebase --exec` shouldn't run the exec command when there is nothing to rebase","fromName":"Nikita Bobko","fromEmail":"nikitabobko@gmail.com","sentAt":"2021-11-26T12:44:14Z","receivedAt":"2021-11-26T13:35:47Z","isPatch":false,"sender":{"key":"nikitabobko@gmail.com","avatar":null},"body":"Steps:\ngit rebase HEAD --exec \"echo foo\"\n\nEXPECTED: since 0 commits are going to be rebased then I expect \"foo\"\nNOT to be printed\nACTUAL: \"foo\" is printed\n\nA real-world example where such behavior is really unexpected - I have\na script that signs a series of commits like this:\n```bash\nhead=\"$(git rev-parse HEAD)\"\n\n# Generate some commits (in my case it's cherry-picking commits from\npublic GitHub subtree repo to internal monorepo)\n\nfile=\"$(mktemp)\"\ngit rebase $head --exec \"git log -1 --pretty='%B' > $file\" \\\n--exec \"echo 'closes\n'https://github.com/JetBrains/intellij-community/pull/$1' >> $file\" \\\n--exec \"git commit --amend -F $file\"\n```\nBut if none of the commits were generated then `git rebase --exec`\nwill amend the HEAD which is not expected. It means that I have to\nprocess the case when 0 zero commits are generated separately.\n\nIf you agree that it's a bug then, most likely, it won't be possible\nto fix it because it would break compatibility. Well, yes then this\nbug report is JFYI.\n\n[System Info]\ngit version:\ngit version 2.33.0\ncpu: x86_64\nno commit associated with this build\nsizeof-long: 8\nsizeof-size_t: 8\nshell-path: /bin/sh\nuname: Linux 5.4.98-1-lts #1 SMP Sat, 13 Feb 2021 19:22:14 +0000 x86_64\ncompiler info: gnuc: 11.1\nlibc info: glibc: 2.33\n$SHELL (typically, interactive shell): /bin/zsh\n\n\n[Enabled Hooks]\n"},{"id":"442439","messageId":"211129.868rx7gnd5.gmgdl@evledraar.gmail.com","threadId":"56985","inReplyTo":"CAMJzOtyw78-8gt3oGscN7KUzpzRRWtAQuGfcJ+R_Fjoom9Lexw@mail.gmail.com","subject":"Re: [BUG REPORT] `git rebase --exec` shouldn't run the exec command when there is nothing to rebase","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-11-29T12:07:46Z","receivedAt":"2021-11-29T13:43:13Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Nov 26 2021, Nikita Bobko wrote:\n\n> Steps:\n> git rebase HEAD --exec \"echo foo\"\n>\n> EXPECTED: since 0 commits are going to be rebased then I expect \"foo\"\n> NOT to be printed\n> ACTUAL: \"foo\" is printed\n\nI don't think this is a bug, but explicitly desired behavior. When you\ndo:\n\n git rebase -x 'make test' BASE\n\nYou expect to run 'make test' for all of BASE..HEAD inclusive of\n\"base\". E.g. for HEAD~1 we'll run 'make test' twice, and you know both\nyour HEAD~ and HEAD passed tests.\n\nSo why wouldn't doing the same for HEAD make sense?\n\nThat being said perhaps some users would think an option or\nconfiguration to skip the injection of \"exec\" after \"noop\" would make\nsense in that case.\n\nBut does this really have anything per-se to do with --exec? Wouldn't\nsuch an option/configuration be the same as rebase in general dying if\nthere's no work to do?\n\nAnd wouldn't such a thing be more useful than a narrow change to make\n--exec a NOOP in these cases?\n\nE.g. if I've got a \"topic\" that has commit \"A\", that's since been\nintegrated into my upstream and I have a script to \"make test\" on my\ntopics, won't simply dying (and thus indicating that the topic is\ndead/integrated) be better than noop-ing?\n"},{"id":"442588","messageId":"CABPp-BFRE2=Owf15WzkacNfdNKbkd2n4GZh7HqDokKzeviBWRw@mail.gmail.com","threadId":"56985","inReplyTo":"211129.868rx7gnd5.gmgdl@evledraar.gmail.com","subject":"Re: [BUG REPORT] `git rebase --exec` shouldn't run the exec command when there is nothing to rebase","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-11-30T00:14:33Z","receivedAt":"2021-11-30T00:14:47Z","isPatch":false,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Mon, Nov 29, 2021 at 2:25 PM Ævar Arnfjörð Bjarmason\n wrote:\n>\n> On Fri, Nov 26 2021, Nikita Bobko wrote:\n>\n> > Steps:\n> > git rebase HEAD --exec \"echo foo\"\n> >\n> > EXPECTED: since 0 commits are going to be rebased then I expect \"foo\"\n> > NOT to be printed\n> > ACTUAL: \"foo\" is printed\n>\n> I don't think this is a bug, but explicitly desired behavior.\n\nMy reading of the docs are such that I'd expect the same as Nikita here:\n\n Append \"exec \" after each line creating a commit in the final\n history.\n ...\n If --autosquash is used, \"exec\" lines will not be appended for the\n intermediate commits, and will only appear at the end of each\n squash/fixup series.\n\nThere is no line creating a commit in the final history when you do a\ngit rebase -i --exec \"echo foo\" HEAD (there is only a noop line), so\nthere should be no exec line.\n\n> When you do:\n>\n> git rebase -x 'make test' BASE\n>\n> You expect to run 'make test' for all of BASE..HEAD inclusive of\n> \"base\". E.g. for HEAD~1 we'll run 'make test' twice, and you know both\n> your HEAD~ and HEAD passed tests.\n\nThis is not true. Try `git rebase -i --exec HEAD~$N` for various\nvalues of N>0. base is not included.\n\n> So why wouldn't doing the same for HEAD make sense?\n\nIndeed; HEAD is weirdly inconsistent and should be brought in line\nwith the others.\n\n> That being said perhaps some users would think an option or\n> configuration to skip the injection of \"exec\" after \"noop\" would make\n> sense in that case.\n>\n> But does this really have anything per-se to do with --exec? Wouldn't\n> such an option/configuration be the same as rebase in general dying if\n> there's no work to do?\n>\n> And wouldn't such a thing be more useful than a narrow change to make\n> --exec a NOOP in these cases?\n>\n> E.g. if I've got a \"topic\" that has commit \"A\", that's since been\n> integrated into my upstream and I have a script to \"make test\" on my\n> topics, won't simply dying (and thus indicating that the topic is\n> dead/integrated) be better than noop-ing?\n\nWhy do you suggest \"dying\" rather than early completion with success?\n\nAnyway, rebase does early exit in non-interactive mode when there is\nnothing to do, it's just that interactive mode suggests users might\nwant to do something special, so they get a TODO list containing only\n\"noop\". Since --exec was written in terms of interactive rebase by\nediting the TODO list and inserting an exec command after each of the\npicks, and it accidentally always added one at the end of the todo\nlist even if the last instruction (group) was not a pick/fixup/squash,\nwe hit this bug.\n\nAnyway, I've got a patch I'll send in.\n"},{"id":"442592","messageId":"YaVzufpKcC0t+q+L@nand.local","threadId":"56985","inReplyTo":"CABPp-BFRE2=Owf15WzkacNfdNKbkd2n4GZh7HqDokKzeviBWRw@mail.gmail.com","subject":"Re: [BUG REPORT] `git rebase --exec` shouldn't run the exec command when there is nothing to rebase","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-11-30T00:43:37Z","receivedAt":"2021-11-30T00:43:40Z","isPatch":false,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Nov 29, 2021 at 04:14:33PM -0800, Elijah Newren wrote:\n> On Mon, Nov 29, 2021 at 2:25 PM Ævar Arnfjörð Bjarmason\n> wrote:\n> >\n> > On Fri, Nov 26 2021, Nikita Bobko wrote:\n> >\n> > > Steps:\n> > > git rebase HEAD --exec \"echo foo\"\n> > >\n> > > EXPECTED: since 0 commits are going to be rebased then I expect \"foo\"\n> > > NOT to be printed\n> > > ACTUAL: \"foo\" is printed\n> >\n> > I don't think this is a bug, but explicitly desired behavior.\n>\n> My reading of the docs are such that I'd expect the same as Nikita here:\n>\n> Append \"exec \" after each line creating a commit in the final\n> history.\n> ...\n> If --autosquash is used, \"exec\" lines will not be appended for the\n> intermediate commits, and will only appear at the end of each\n> squash/fixup series.\n>\n> There is no line creating a commit in the final history when you do a\n> git rebase -i --exec \"echo foo\" HEAD (there is only a noop line), so\n> there should be no exec line.\n\nThanks for quoting the docs here. When I ran this myself, I thought that\nthe docs must say something like \"after every line\" and not further\nspecify \"... creating a commit\".\n\nBut they do, so I agree with the original report from Nikita that\n\n git rebase -x 'echo foo' HEAD\n\nshould be silent in order to be consistent with the docs.\n\n> > When you do:\n> >\n> > git rebase -x 'make test' BASE\n> >\n> > You expect to run 'make test' for all of BASE..HEAD inclusive of\n> > \"base\". E.g. for HEAD~1 we'll run 'make test' twice, and you know both\n> > your HEAD~ and HEAD passed tests.\n>\n> This is not true. Try `git rebase -i --exec HEAD~$N` for various\n> values of N>0. base is not included.\n>\n> > So why wouldn't doing the same for HEAD make sense?\n>\n> Indeed; HEAD is weirdly inconsistent and should be brought in line\n> with the others.\n\nYep.\n\nThanks,\nTaylor\n"},{"id":"442598","messageId":"CABPp-BFdw7wL40WcM7_pfLCRx5CrF30cOgu3mL5EB6DysEqZWQ@mail.gmail.com","threadId":"56985","inReplyTo":"CABPp-BFRE2=Owf15WzkacNfdNKbkd2n4GZh7HqDokKzeviBWRw@mail.gmail.com","subject":"Re: [BUG REPORT] `git rebase --exec` shouldn't run the exec command when there is nothing to rebase","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-11-30T04:01:10Z","receivedAt":"2021-11-30T04:01:24Z","isPatch":false,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Mon, Nov 29, 2021 at 4:14 PM Elijah Newren wrote:\n>\n> On Mon, Nov 29, 2021 at 2:25 PM Ævar Arnfjörð Bjarmason\n> wrote:\n> >\n> > On Fri, Nov 26 2021, Nikita Bobko wrote:\n> >\n> > > Steps:\n> > > git rebase HEAD --exec \"echo foo\"\n> > >\n> > > EXPECTED: since 0 commits are going to be rebased then I expect \"foo\"\n> > > NOT to be printed\n> > > ACTUAL: \"foo\" is printed\n> >\n> > I don't think this is a bug, but explicitly desired behavior.\n>\n> My reading of the docs are such that I'd expect the same as Nikita here:\n>\n> Append \"exec \" after each line creating a commit in the final\n> history.\n> ...\n> If --autosquash is used, \"exec\" lines will not be appended for the\n> intermediate commits, and will only appear at the end of each\n> squash/fixup series.\n>\n> There is no line creating a commit in the final history when you do a\n> git rebase -i --exec \"echo foo\" HEAD (there is only a noop line), so\n> there should be no exec line.\n>\n> > When you do:\n> >\n> > git rebase -x 'make test' BASE\n> >\n> > You expect to run 'make test' for all of BASE..HEAD inclusive of\n> > \"base\". E.g. for HEAD~1 we'll run 'make test' twice, and you know both\n> > your HEAD~ and HEAD passed tests.\n>\n> This is not true. Try `git rebase -i --exec HEAD~$N` for various\n> values of N>0. base is not included.\n>\n> > So why wouldn't doing the same for HEAD make sense?\n>\n> Indeed; HEAD is weirdly inconsistent and should be brought in line\n> with the others.\n>\n> > That being said perhaps some users would think an option or\n> > configuration to skip the injection of \"exec\" after \"noop\" would make\n> > sense in that case.\n> >\n> > But does this really have anything per-se to do with --exec? Wouldn't\n> > such an option/configuration be the same as rebase in general dying if\n> > there's no work to do?\n> >\n> > And wouldn't such a thing be more useful than a narrow change to make\n> > --exec a NOOP in these cases?\n> >\n> > E.g. if I've got a \"topic\" that has commit \"A\", that's since been\n> > integrated into my upstream and I have a script to \"make test\" on my\n> > topics, won't simply dying (and thus indicating that the topic is\n> > dead/integrated) be better than noop-ing?\n>\n> Why do you suggest \"dying\" rather than early completion with success?\n>\n> Anyway, rebase does early exit in non-interactive mode when there is\n> nothing to do, it's just that interactive mode suggests users might\n> want to do something special, so they get a TODO list containing only\n> \"noop\". Since --exec was written in terms of interactive rebase by\n> editing the TODO list and inserting an exec command after each of the\n> picks, and it accidentally always added one at the end of the todo\n> list even if the last instruction (group) was not a pick/fixup/squash,\n> we hit this bug.\n>\n> Anyway, I've got a patch I'll send in.\n\nFound over here:\nhttps://lore.kernel.org/git/pull.1149.git.git.1638244719381.gitgitgadget@gmail.com/\n"},{"id":"442626","messageId":"e22ef3c3-ee3e-d700-720c-71339abc19c0@gmail.com","threadId":"56985","inReplyTo":"211129.868rx7gnd5.gmgdl@evledraar.gmail.com","subject":"Re: [BUG REPORT] `git rebase --exec` shouldn't run the exec command when there is nothing to rebase","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2021-11-30T11:09:34Z","receivedAt":"2021-11-30T11:09:55Z","isPatch":false,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 29/11/2021 12:07, Ævar Arnfjörð Bjarmason wrote:\n> \n> On Fri, Nov 26 2021, Nikita Bobko wrote:\n> \n>> Steps:\n>> git rebase HEAD --exec \"echo foo\"\n>>\n>> EXPECTED: since 0 commits are going to be rebased then I expect \"foo\"\n>> NOT to be printed\n>> ACTUAL: \"foo\" is printed\n> \n> I don't think this is a bug, but explicitly desired behavior. When you\n> do:\n> \n> git rebase -x 'make test' BASE\n> \n> You expect to run 'make test' for all of BASE..HEAD inclusive of\n> \"base\". E.g. for HEAD~1 we'll run 'make test' twice, and you know both\n> your HEAD~ and HEAD passed tests.\n\nI don't think we run 'make test' for base in that case, only after each \npick and base is not picked by the rebase.\n\nBest Wishes\n\nPhillip\n\n> So why wouldn't doing the same for HEAD make sense?\n> \n> That being said perhaps some users would think an option or\n> configuration to skip the injection of \"exec\" after \"noop\" would make\n> sense in that case.\n> \n> But does this really have anything per-se to do with --exec? Wouldn't\n> such an option/configuration be the same as rebase in general dying if\n> there's no work to do?\n> \n> And wouldn't such a thing be more useful than a narrow change to make\n> --exec a NOOP in these cases?\n> \n> E.g. if I've got a \"topic\" that has commit \"A\", that's since been\n> integrated into my upstream and I have a script to \"make test\" on my\n> topics, won't simply dying (and thus indicating that the topic is\n> dead/integrated) be better than noop-ing?\n> \n\n"}]}