# [BUG REPORT] `git rebase --exec` shouldn't run the exec command when there is nothing to rebase 6 messages from 2021-11-26 to 2021-11-30. Participants: Nikita Bobko, Ævar Arnfjörð Bjarmason, Elijah Newren, Taylor Blau, Phillip Wood. Thread: https://gitlist.dev/t/56985 ## Nikita Bobko, 2021-11-26 12:44 Subject: [BUG REPORT] `git rebase --exec` shouldn't run the exec command when there is nothing to rebase Message-ID: URL: https://gitlist.dev/e/CAMJzOtyw78-8gt3oGscN7KUzpzRRWtAQuGfcJ%2BR_Fjoom9Lexw%40mail.gmail.com ``` Steps: git rebase HEAD --exec "echo foo" EXPECTED: since 0 commits are going to be rebased then I expect "foo" NOT to be printed ACTUAL: "foo" is printed A real-world example where such behavior is really unexpected - I have a script that signs a series of commits like this: ˋˋˋbash head="$(git rev-parse HEAD)" # Generate some commits (in my case it's cherry-picking commits from public GitHub subtree repo to internal monorepo) file="$(mktemp)" git rebase $head --exec "git log -1 --pretty='%B' > $file" \ --exec "echo 'closes 'https://github.com/JetBrains/intellij-community/pull/$1' >> $file" \ --exec "git commit --amend -F $file" ˋˋˋ But if none of the commits were generated then `git rebase --exec` will amend the HEAD which is not expected. It means that I have to process the case when 0 zero commits are generated separately. If you agree that it's a bug then, most likely, it won't be possible to fix it because it would break compatibility. Well, yes then this bug report is JFYI. [System Info] git version: git version 2.33.0 cpu: x86_64 no commit associated with this build sizeof-long: 8 sizeof-size_t: 8 shell-path: /bin/sh uname: Linux 5.4.98-1-lts #1 SMP Sat, 13 Feb 2021 19:22:14 +0000 x86_64 compiler info: gnuc: 11.1 libc info: glibc: 2.33 $SHELL (typically, interactive shell): /bin/zsh [Enabled Hooks] ``` ## Ævar Arnfjörð Bjarmason, 2021-11-29 12:07 Subject: Re: [BUG REPORT] `git rebase --exec` shouldn't run the exec command when there is nothing to rebase Message-ID: <211129.868rx7gnd5.gmgdl@evledraar.gmail.com> URL: https://gitlist.dev/e/211129.868rx7gnd5.gmgdl%40evledraar.gmail.com In-Reply-To: ``` On Fri, Nov 26 2021, Nikita Bobko wrote: > Steps: > git rebase HEAD --exec "echo foo" > > EXPECTED: since 0 commits are going to be rebased then I expect "foo" > NOT to be printed > ACTUAL: "foo" is printed I don't think this is a bug, but explicitly desired behavior. When you do: git rebase -x 'make test' BASE You expect to run 'make test' for all of BASE..HEAD inclusive of "base". E.g. for HEAD~1 we'll run 'make test' twice, and you know both your HEAD~ and HEAD passed tests. So why wouldn't doing the same for HEAD make sense? That being said perhaps some users would think an option or configuration to skip the injection of "exec" after "noop" would make sense in that case. But does this really have anything per-se to do with --exec? Wouldn't such an option/configuration be the same as rebase in general dying if there's no work to do? And wouldn't such a thing be more useful than a narrow change to make --exec a NOOP in these cases? E.g. if I've got a "topic" that has commit "A", that's since been integrated into my upstream and I have a script to "make test" on my topics, won't simply dying (and thus indicating that the topic is dead/integrated) be better than noop-ing? ``` ## Elijah Newren, 2021-11-30 00:14 Subject: Re: [BUG REPORT] `git rebase --exec` shouldn't run the exec command when there is nothing to rebase Message-ID: URL: https://gitlist.dev/e/CABPp-BFRE2%3DOwf15WzkacNfdNKbkd2n4GZh7HqDokKzeviBWRw%40mail.gmail.com In-Reply-To: <211129.868rx7gnd5.gmgdl@evledraar.gmail.com> ``` On Mon, Nov 29, 2021 at 2:25 PM Ævar Arnfjörð Bjarmason wrote: > > On Fri, Nov 26 2021, Nikita Bobko wrote: > > > Steps: > > git rebase HEAD --exec "echo foo" > > > > EXPECTED: since 0 commits are going to be rebased then I expect "foo" > > NOT to be printed > > ACTUAL: "foo" is printed > > I don't think this is a bug, but explicitly desired behavior. My reading of the docs are such that I'd expect the same as Nikita here: Append "exec " after each line creating a commit in the final history. ... If --autosquash is used, "exec" lines will not be appended for the intermediate commits, and will only appear at the end of each squash/fixup series. There is no line creating a commit in the final history when you do a git rebase -i --exec "echo foo" HEAD (there is only a noop line), so there should be no exec line. > When you do: > > git rebase -x 'make test' BASE > > You expect to run 'make test' for all of BASE..HEAD inclusive of > "base". E.g. for HEAD~1 we'll run 'make test' twice, and you know both > your HEAD~ and HEAD passed tests. This is not true. Try `git rebase -i --exec HEAD~$N` for various values of N>0. base is not included. > So why wouldn't doing the same for HEAD make sense? Indeed; HEAD is weirdly inconsistent and should be brought in line with the others. > That being said perhaps some users would think an option or > configuration to skip the injection of "exec" after "noop" would make > sense in that case. > > But does this really have anything per-se to do with --exec? Wouldn't > such an option/configuration be the same as rebase in general dying if > there's no work to do? > > And wouldn't such a thing be more useful than a narrow change to make > --exec a NOOP in these cases? > > E.g. if I've got a "topic" that has commit "A", that's since been > integrated into my upstream and I have a script to "make test" on my > topics, won't simply dying (and thus indicating that the topic is > dead/integrated) be better than noop-ing? Why do you suggest "dying" rather than early completion with success? Anyway, rebase does early exit in non-interactive mode when there is nothing to do, it's just that interactive mode suggests users might want to do something special, so they get a TODO list containing only "noop". Since --exec was written in terms of interactive rebase by editing the TODO list and inserting an exec command after each of the picks, and it accidentally always added one at the end of the todo list even if the last instruction (group) was not a pick/fixup/squash, we hit this bug. Anyway, I've got a patch I'll send in. ``` ## Taylor Blau, 2021-11-30 00:43 Subject: Re: [BUG REPORT] `git rebase --exec` shouldn't run the exec command when there is nothing to rebase Message-ID: URL: https://gitlist.dev/e/YaVzufpKcC0t%2Bq%2BL%40nand.local In-Reply-To: ``` On Mon, Nov 29, 2021 at 04:14:33PM -0800, Elijah Newren wrote: > On Mon, Nov 29, 2021 at 2:25 PM Ævar Arnfjörð Bjarmason > wrote: > > > > On Fri, Nov 26 2021, Nikita Bobko wrote: > > > > > Steps: > > > git rebase HEAD --exec "echo foo" > > > > > > EXPECTED: since 0 commits are going to be rebased then I expect "foo" > > > NOT to be printed > > > ACTUAL: "foo" is printed > > > > I don't think this is a bug, but explicitly desired behavior. > > My reading of the docs are such that I'd expect the same as Nikita here: > > Append "exec " after each line creating a commit in the final > history. > ... > If --autosquash is used, "exec" lines will not be appended for the > intermediate commits, and will only appear at the end of each > squash/fixup series. > > There is no line creating a commit in the final history when you do a > git rebase -i --exec "echo foo" HEAD (there is only a noop line), so > there should be no exec line. Thanks for quoting the docs here. When I ran this myself, I thought that the docs must say something like "after every line" and not further specify "... creating a commit". But they do, so I agree with the original report from Nikita that git rebase -x 'echo foo' HEAD should be silent in order to be consistent with the docs. > > When you do: > > > > git rebase -x 'make test' BASE > > > > You expect to run 'make test' for all of BASE..HEAD inclusive of > > "base". E.g. for HEAD~1 we'll run 'make test' twice, and you know both > > your HEAD~ and HEAD passed tests. > > This is not true. Try `git rebase -i --exec HEAD~$N` for various > values of N>0. base is not included. > > > So why wouldn't doing the same for HEAD make sense? > > Indeed; HEAD is weirdly inconsistent and should be brought in line > with the others. Yep. Thanks, Taylor ``` ## Elijah Newren, 2021-11-30 04:01 Subject: Re: [BUG REPORT] `git rebase --exec` shouldn't run the exec command when there is nothing to rebase Message-ID: URL: https://gitlist.dev/e/CABPp-BFdw7wL40WcM7_pfLCRx5CrF30cOgu3mL5EB6DysEqZWQ%40mail.gmail.com In-Reply-To: ``` On Mon, Nov 29, 2021 at 4:14 PM Elijah Newren wrote: > > On Mon, Nov 29, 2021 at 2:25 PM Ævar Arnfjörð Bjarmason > wrote: > > > > On Fri, Nov 26 2021, Nikita Bobko wrote: > > > > > Steps: > > > git rebase HEAD --exec "echo foo" > > > > > > EXPECTED: since 0 commits are going to be rebased then I expect "foo" > > > NOT to be printed > > > ACTUAL: "foo" is printed > > > > I don't think this is a bug, but explicitly desired behavior. > > My reading of the docs are such that I'd expect the same as Nikita here: > > Append "exec " after each line creating a commit in the final > history. > ... > If --autosquash is used, "exec" lines will not be appended for the > intermediate commits, and will only appear at the end of each > squash/fixup series. > > There is no line creating a commit in the final history when you do a > git rebase -i --exec "echo foo" HEAD (there is only a noop line), so > there should be no exec line. > > > When you do: > > > > git rebase -x 'make test' BASE > > > > You expect to run 'make test' for all of BASE..HEAD inclusive of > > "base". E.g. for HEAD~1 we'll run 'make test' twice, and you know both > > your HEAD~ and HEAD passed tests. > > This is not true. Try `git rebase -i --exec HEAD~$N` for various > values of N>0. base is not included. > > > So why wouldn't doing the same for HEAD make sense? > > Indeed; HEAD is weirdly inconsistent and should be brought in line > with the others. > > > That being said perhaps some users would think an option or > > configuration to skip the injection of "exec" after "noop" would make > > sense in that case. > > > > But does this really have anything per-se to do with --exec? Wouldn't > > such an option/configuration be the same as rebase in general dying if > > there's no work to do? > > > > And wouldn't such a thing be more useful than a narrow change to make > > --exec a NOOP in these cases? > > > > E.g. if I've got a "topic" that has commit "A", that's since been > > integrated into my upstream and I have a script to "make test" on my > > topics, won't simply dying (and thus indicating that the topic is > > dead/integrated) be better than noop-ing? > > Why do you suggest "dying" rather than early completion with success? > > Anyway, rebase does early exit in non-interactive mode when there is > nothing to do, it's just that interactive mode suggests users might > want to do something special, so they get a TODO list containing only > "noop". Since --exec was written in terms of interactive rebase by > editing the TODO list and inserting an exec command after each of the > picks, and it accidentally always added one at the end of the todo > list even if the last instruction (group) was not a pick/fixup/squash, > we hit this bug. > > Anyway, I've got a patch I'll send in. Found over here: https://lore.kernel.org/git/pull.1149.git.git.1638244719381.gitgitgadget@gmail.com/ ``` ## Phillip Wood, 2021-11-30 11:09 Subject: Re: [BUG REPORT] `git rebase --exec` shouldn't run the exec command when there is nothing to rebase Message-ID: URL: https://gitlist.dev/e/e22ef3c3-ee3e-d700-720c-71339abc19c0%40gmail.com In-Reply-To: <211129.868rx7gnd5.gmgdl@evledraar.gmail.com> ``` On 29/11/2021 12:07, Ævar Arnfjörð Bjarmason wrote: > > On Fri, Nov 26 2021, Nikita Bobko wrote: > >> Steps: >> git rebase HEAD --exec "echo foo" >> >> EXPECTED: since 0 commits are going to be rebased then I expect "foo" >> NOT to be printed >> ACTUAL: "foo" is printed > > I don't think this is a bug, but explicitly desired behavior. When you > do: > > git rebase -x 'make test' BASE > > You expect to run 'make test' for all of BASE..HEAD inclusive of > "base". E.g. for HEAD~1 we'll run 'make test' twice, and you know both > your HEAD~ and HEAD passed tests. I don't think we run 'make test' for base in that case, only after each pick and base is not picked by the rebase. Best Wishes Phillip > So why wouldn't doing the same for HEAD make sense? > > That being said perhaps some users would think an option or > configuration to skip the injection of "exec" after "noop" would make > sense in that case. > > But does this really have anything per-se to do with --exec? Wouldn't > such an option/configuration be the same as rebase in general dying if > there's no work to do? > > And wouldn't such a thing be more useful than a narrow change to make > --exec a NOOP in these cases? > > E.g. if I've got a "topic" that has commit "A", that's since been > integrated into my upstream and I have a script to "make test" on my > topics, won't simply dying (and thus indicating that the topic is > dead/integrated) be better than noop-ing? > ```