[BUG REPORT] `git rebase --exec` shouldn't run the exec command when there is nothing to rebase

[BUG REPORT] `git rebase --exec` shouldn't run the exec command when there is nothing to rebase

[Nikita Bobko]

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]

Re: [BUG REPORT] `git rebase --exec` shouldn't run the exec command when there is nothing to rebase

[Ævar Arnfjörð Bjarmason]

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?

Re: [BUG REPORT] `git rebase --exec` shouldn't run the exec command when there is nothing to rebase

[Elijah Newren]

On Mon, Nov 29, 2021 at 2:25 PM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> 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 <cmd>" 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.

Re: [BUG REPORT] `git rebase --exec` shouldn't run the exec command when there is nothing to rebase

[Taylor Blau]

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
> <avarab@gmail.com> 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 <cmd>" 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

[PATCH] sequencer: avoid adding exec commands for non-commit creating commands

[Elijah Newren via GitGitGadget]

From: Elijah Newren <newren@gmail.com>

The `--exec <cmd>` is documented as

    Append "exec <cmd>" 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.

Unfortunately, it would also add exec commands after non-pick
operations, such as 'no-op', which could be seen for example with
    git rebase -i --exec true HEAD

todo_list_add_exec_commands() intent was to insert exec commands after
each logical pick, while trying to consider a chains of fixup and squash
commits to be part of the pick before it.  So it would keep an 'insert'
boolean tracking if it had seen a pick or merge, but not write the exec
command until it saw the next non-fixup/squash command.  Since that
would make it miss the final exec command, it had some code that would
check whether it still needed to insert one at the end, but instead of a
simple

    if (insert)

it had a

    if (insert || <condition that is always true>)

That's buggy; as per the docs, we should only add exec commands for
lines that create commits, i.e. only if insert is true.  Fix the
conditional.

There was one testcase in the testsuite that we tweak for this change;
it was introduced in 54fd3243da ("rebase -i: reread the todo list if
`exec` touched it", 2017-04-26), and was merely testing that after an
exec had fired that the todo list would be re-read.  The test at the
time would have worked given any revision at all, though it would only
work with 'HEAD' as a side-effect of this bug.  Since we're fixing this
bug, choose something other than 'HEAD' for that test.

Finally, add a testcase that verifies when we have no commits to pick,
that we get no exec lines in the generated todo list.

Reported-by: Nikita Bobko <nikitabobko@gmail.com>
Signed-off-by: Elijah Newren <newren@gmail.com>
---
    sequencer: avoid adding exec commands for non-commit creating commands
    
    Original report over at
    https://lore.kernel.org/git/YaVzufpKcC0t+q+L@nand.local/T/#m13fbd7b054c06ba1f98ae66e6d1b9fcc51bb875e

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1149%2Fnewren%2Frebase-exec-empty-bug-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1149/newren/rebase-exec-empty-bug-v1
Pull-Request: https://github.com/git/git/pull/1149

 sequencer.c                 | 2 +-
 t/t3429-rebase-edit-todo.sh | 7 ++++++-
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index ea96837cde3..aa790f0bba8 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -5496,7 +5496,7 @@ static void todo_list_add_exec_commands(struct todo_list *todo_list,
 	}
 
 	/* insert or append final <commands> */
-	if (insert || nr == todo_list->nr) {
+	if (insert) {
 		ALLOC_GROW(items, nr + commands->nr, alloc);
 		COPY_ARRAY(items + nr, base_items, commands->nr);
 		nr += commands->nr;
diff --git a/t/t3429-rebase-edit-todo.sh b/t/t3429-rebase-edit-todo.sh
index 7024d49ae7b..abd66f36021 100755
--- a/t/t3429-rebase-edit-todo.sh
+++ b/t/t3429-rebase-edit-todo.sh
@@ -13,10 +13,15 @@ test_expect_success 'setup' '
 
 test_expect_success 'rebase exec modifies rebase-todo' '
 	todo=.git/rebase-merge/git-rebase-todo &&
-	git rebase HEAD -x "echo exec touch F >>$todo" &&
+	git rebase HEAD~1 -x "echo exec touch F >>$todo" &&
 	test -e F
 '
 
+test_expect_success 'rebase exec with an empty list does not exec anything' '
+	git rebase HEAD -x "true" 2>output &&
+	! grep "Executing: true" output
+'
+
 test_expect_success 'loose object cache vs re-reading todo list' '
 	GIT_REBASE_TODO=.git/rebase-merge/git-rebase-todo &&
 	export GIT_REBASE_TODO &&

base-commit: 35151cf0720460a897cde9b8039af364743240e7
-- 
gitgitgadget

Re: [BUG REPORT] `git rebase --exec` shouldn't run the exec command when there is nothing to rebase

[Elijah Newren]

On Mon, Nov 29, 2021 at 4:14 PM Elijah Newren <newren@gmail.com> wrote:
>
> On Mon, Nov 29, 2021 at 2:25 PM Ævar Arnfjörð Bjarmason
> <avarab@gmail.com> 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 <cmd>" 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/

Re: [PATCH] sequencer: avoid adding exec commands for non-commit creating commands

[Taylor Blau]

On Tue, Nov 30, 2021 at 03:58:39AM +0000, Elijah Newren via GitGitGadget wrote:
> diff --git a/sequencer.c b/sequencer.c
> index ea96837cde3..aa790f0bba8 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -5496,7 +5496,7 @@ static void todo_list_add_exec_commands(struct todo_list *todo_list,
>  	}
>
>  	/* insert or append final <commands> */
> -	if (insert || nr == todo_list->nr) {
> +	if (insert) {

Looks good. My worry after first reading this is that we wouldn't insert
an `--autosquash` rebase that ends in a fixup, e.g.:

    git commit -m foo
    git commit --fixup HEAD
    git rebase -i --autosquash -x true HEAD~2

But we're OK there, since we set insert to 1 when we see the first pick,
and leave it because we never saw another fixup. Then we'll still have
fixup as 1 when we exit the loop, and we correctly insert an exec line
at the end of the fixup chain.

So I think that having "|| nr == todo_list->nr" part of the conditional
was broken to begin with.

As far as I can tell, this behavior of always sticking an 'exec' line at
the end of the todo list has existed since the inception of the `-x`
option back in c214538416 (rebase -i: teach "--exec <cmd>", 2012-06-12).
See the unconditional `printf "%s" "$cmd"` at the end of the sub-shell
within `add_exec_commands()` from that commit.

But this is broken according to the docs, and I think that your fix and
test coverage are sensible. Thanks!

Thanks,
Taylor

Re: [BUG REPORT] `git rebase --exec` shouldn't run the exec command when there is nothing to rebase

[Phillip Wood]

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?
> 

Re: [BUG REPORT] `git rebase --exec` shouldn't run the exec command when there is nothing to rebase

[Ævar Arnfjörð Bjarmason]

On Mon, Nov 29 2021, Elijah Newren wrote:

[Moving this between threads, from
https://lore.kernel.org/git/CABPp-BFRE2=Owf15WzkacNfdNKbkd2n4GZh7HqDokKzeviBWRw@mail.gmail.com/
to the patch]

> On Mon, Nov 29, 2021 at 2:25 PM Ævar Arnfjörð Bjarmason
> <avarab@gmail.com> 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 <cmd>" 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.

Maybe you're right & we can just change it. Keep in mind that those docs
were added by a non-native speaker (or rather, I'm assuming so based on
the name / E-Mail address).

See c214538416e (rebase -i: teach "--exec <cmd>", 2012-06-12). I agree
that the reading you've got of it is the more obvious one.

The reason I thought it wasn't a bug (some of which I dug more into
afterwards):

 1. I read that "commit in the final history" as referring to the range of
    commits to be rebased. Having only one commit or zero is perfectly OK,
    since...

 2. ... with "exec" we don't know if the "commit in the final history" isn't
   affected with an argument of HEAD. I.e. yes you can also provide "HEAD~", but
   that's the difference between having a "pick" line or not. I don't think the
   sequencer cares, but maybe third party scripting via the sequence editor does?

   We already have an explicit facility to early abort the rebasing. See
   ff74126c03a (rebase -i: do not fail when there is no commit to cherry-pick,
   2008-10-10)

   So the feature that Nikita wants is already possible via GIT_SEQUENCE_EDITOR.
   Now, that's a painful UI, but perhaps if this patch is implemented as a 1=1
   mapping to that we'll discover some new edge case that wasn't considered?

 3. This isn't just a theoretical concern. It's *interactive* rebase, e.g. a
    perfectly fine use for it (which I've occasionally used is):

        # no local commits
        git checkout master
        # opens my editor with just a "noop" line
        git rebase -i

    And then adding/copying around *new* commits in the buffer and saving
    it, i.e. using it as an interactive text-based cherry-pick (this is
    particularly nice with Emacs's magit mode).

For #3 we can just say "well use HEAD~ then and ignore the one 'pick'"
line. Sure, I've probably only used this once or twice.

I just worry that we'll break thinsg for other users because we're
narrowly focusing on --exec as a way to follow-up interactive rebase
commands that we insert, and forgetting that this is a generic
templating language that others are intercepting and modifying.

So e.g. if you want to cherry-pick new commits and always use the same
10 "exec" lines to build and test those you can just rebase to HEAD with
those --exec, then copy/paste them for each new thing you insert.

You can also do that with HEAD~ and carry forward any "pick" line, but
that's *different*. I.e. we're forcing whoever relies on the current
semantics to change their GIT_SEQUENCE_EDITOR script from (pseudocode):

    if grep ^noop git-rebase-todo
    then
        for c in commits
        do
            echo "pick $c"
            # get the exec lines for each one, if any
            cat git-rebase-todo
        done
    fi

To something that'll have to handle a single "pick" 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.

Sorry, I meant "inclusive of HEAD". I.e. we'll run "make test" for HEAD,
not just HEAD~. Likewise with any "exec" commands.

>> 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.

I mean why shouldn't we run "make test" on HEAD, sorry. I agree that
running "make test" on "base" would make no sense. You can rebase to
BASE~ if you want that.

But yes, the result is the same as a rebase to HEAD~, so maybe it's fine
to change it...

>> 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?

If you do:

    git rebase -i HEAD

Comment out the "noop" line, and save you'll get:

    error: nothing to do

And an exit code of 1.

Maybe we should silently return 0 there, but it seems to me like this
behavior needs to be consistent with whatever "noop" is trying to
accomplish in general (see ff74126c03a above).

That's why I said "does this really have anything per-se to do with
--exec?". I.e. we already observe this behavior without --exec, we just
get a noop line, and if we had no line at all we'd error with nothing to
do.

If we're going to make "git rebase -i HEAD" do nothing, why would it
have behavior different from a TODO list of just a "noop" line (which is
not the same thing as "nothing to do").

That's partially a matter of consistency, but mostly the general
paranoia that if we're going to subtly change what's *probably* an
obscure feature hopefully many aren't relying on, then at least having
it die instead of silently "succeed" would be better. I.e. we'll now
silently ignore the "--exec" commands, but didn't before.

Re: [PATCH] sequencer: avoid adding exec commands for non-commit creating commands

[Phillip Wood]

Hi Elijah

On 30/11/2021 03:58, Elijah Newren via GitGitGadget wrote:
> From: Elijah Newren <newren@gmail.com>
> 
> The `--exec <cmd>` is documented as
> 
>      Append "exec <cmd>" 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.
> 
> Unfortunately, it would also add exec commands after non-pick
> operations, such as 'no-op', which could be seen for example with
>      git rebase -i --exec true HEAD
> 
> todo_list_add_exec_commands() intent was to insert exec commands after
> each logical pick, while trying to consider a chains of fixup and squash
> commits to be part of the pick before it.  So it would keep an 'insert'
> boolean tracking if it had seen a pick or merge, but not write the exec
> command until it saw the next non-fixup/squash command.  Since that
> would make it miss the final exec command, it had some code that would
> check whether it still needed to insert one at the end, but instead of a
> simple
> 
>      if (insert)
> 
> it had a
> 
>      if (insert || <condition that is always true>)
> 
> That's buggy; as per the docs, we should only add exec commands for
> lines that create commits, i.e. only if insert is true.  Fix the
> conditional.
> 
> There was one testcase in the testsuite that we tweak for this change;
> it was introduced in 54fd3243da ("rebase -i: reread the todo list if
> `exec` touched it", 2017-04-26), and was merely testing that after an
> exec had fired that the todo list would be re-read.  The test at the
> time would have worked given any revision at all, though it would only
> work with 'HEAD' as a side-effect of this bug.  Since we're fixing this
> bug, choose something other than 'HEAD' for that test.
> 
> Finally, add a testcase that verifies when we have no commits to pick,
> that we get no exec lines in the generated todo list.

Thanks for fixing this, the patch looks good and the commit message is 
excellent. I did see Ævar's concerns in another thread but I think this 
is the least surprising approach - if we're not actually rebasing 
anything it seems odd to run the exec command.

Best Wishes

Phillip

> Reported-by: Nikita Bobko <nikitabobko@gmail.com>
> Signed-off-by: Elijah Newren <newren@gmail.com>
> ---
>      sequencer: avoid adding exec commands for non-commit creating commands
>      
>      Original report over at
>      https://lore.kernel.org/git/YaVzufpKcC0t+q+L@nand.local/T/#m13fbd7b054c06ba1f98ae66e6d1b9fcc51bb875e
> 
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1149%2Fnewren%2Frebase-exec-empty-bug-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1149/newren/rebase-exec-empty-bug-v1
> Pull-Request: https://github.com/git/git/pull/1149
> 
>   sequencer.c                 | 2 +-
>   t/t3429-rebase-edit-todo.sh | 7 ++++++-
>   2 files changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/sequencer.c b/sequencer.c
> index ea96837cde3..aa790f0bba8 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -5496,7 +5496,7 @@ static void todo_list_add_exec_commands(struct todo_list *todo_list,
>   	}
>   
>   	/* insert or append final <commands> */
> -	if (insert || nr == todo_list->nr) {
> +	if (insert) {
>   		ALLOC_GROW(items, nr + commands->nr, alloc);
>   		COPY_ARRAY(items + nr, base_items, commands->nr);
>   		nr += commands->nr;
> diff --git a/t/t3429-rebase-edit-todo.sh b/t/t3429-rebase-edit-todo.sh
> index 7024d49ae7b..abd66f36021 100755
> --- a/t/t3429-rebase-edit-todo.sh
> +++ b/t/t3429-rebase-edit-todo.sh
> @@ -13,10 +13,15 @@ test_expect_success 'setup' '
>   
>   test_expect_success 'rebase exec modifies rebase-todo' '
>   	todo=.git/rebase-merge/git-rebase-todo &&
> -	git rebase HEAD -x "echo exec touch F >>$todo" &&
> +	git rebase HEAD~1 -x "echo exec touch F >>$todo" &&
>   	test -e F
>   '
>   
> +test_expect_success 'rebase exec with an empty list does not exec anything' '
> +	git rebase HEAD -x "true" 2>output &&
> +	! grep "Executing: true" output
> +'
> +
>   test_expect_success 'loose object cache vs re-reading todo list' '
>   	GIT_REBASE_TODO=.git/rebase-merge/git-rebase-todo &&
>   	export GIT_REBASE_TODO &&
> 
> base-commit: 35151cf0720460a897cde9b8039af364743240e7
> 

Re: [BUG REPORT] `git rebase --exec` shouldn't run the exec command when there is nothing to rebase

[Phillip Wood]

On 30/11/2021 14:03, Ævar Arnfjörð Bjarmason wrote:
> 
> On Mon, Nov 29 2021, Elijah Newren wrote:
> 
> [Moving this between threads, from
> https://lore.kernel.org/git/CABPp-BFRE2=Owf15WzkacNfdNKbkd2n4GZh7HqDokKzeviBWRw@mail.gmail.com/
> to the patch]
> 
>> On Mon, Nov 29, 2021 at 2:25 PM Ævar Arnfjörð Bjarmason
>> <avarab@gmail.com> 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 <cmd>" 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.
> 
> Maybe you're right & we can just change it. Keep in mind that those docs
> were added by a non-native speaker (or rather, I'm assuming so based on
> the name / E-Mail address).
> 
> See c214538416e (rebase -i: teach "--exec <cmd>", 2012-06-12). I agree
> that the reading you've got of it is the more obvious one.
> 
> The reason I thought it wasn't a bug (some of which I dug more into
> afterwards):
> 
>   1. I read that "commit in the final history" as referring to the range of
>      commits to be rebased. Having only one commit or zero is perfectly OK,
>      since...
> 
>   2. ... with "exec" we don't know if the "commit in the final history" isn't
>     affected with an argument of HEAD. I.e. yes you can also provide "HEAD~", but
>     that's the difference between having a "pick" line or not. I don't think the
>     sequencer cares, but maybe third party scripting via the sequence editor does?
> 
>     We already have an explicit facility to early abort the rebasing. See
>     ff74126c03a (rebase -i: do not fail when there is no commit to cherry-pick,
>     2008-10-10)
> 
>     So the feature that Nikita wants is already possible via GIT_SEQUENCE_EDITOR.
>     Now, that's a painful UI, but perhaps if this patch is implemented as a 1=1
>     mapping to that we'll discover some new edge case that wasn't considered?
> 
>   3. This isn't just a theoretical concern. It's *interactive* rebase, e.g. a
>      perfectly fine use for it (which I've occasionally used is):
> 
>          # no local commits
>          git checkout master
>          # opens my editor with just a "noop" line
>          git rebase -i
> 
>      And then adding/copying around *new* commits in the buffer and saving
>      it, i.e. using it as an interactive text-based cherry-pick (this is
>      particularly nice with Emacs's magit mode).
> 
> For #3 we can just say "well use HEAD~ then and ignore the one 'pick'"
> line. Sure, I've probably only used this once or twice.

I'm not sure I really follow. For #3 you can just type the exec command 
into the editor rather than passing it on the command line. You already 
have to manually add exec commands after any new pick lines anyway.

> I just worry that we'll break thinsg for other users because we're
> narrowly focusing on --exec as a way to follow-up interactive rebase
> commands that we insert, and forgetting that this is a generic
> templating language that others are intercepting and modifying.

I see what you're getting at but I think this is a small enough corner 
case that we shouldn't worry too much. I think it is simpler to say if 
we don't pick any commits we don't add any exec commands.

 >[...]
>>> 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.
> 
> Sorry, I meant "inclusive of HEAD". I.e. we'll run "make test" for HEAD,
> not just HEAD~. Likewise with any "exec" commands.

We do not run "make test" for HEAD~ when executing "git rebase -x 'make 
test' HEAD~1".

>>> 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.
> 
> I mean why shouldn't we run "make test" on HEAD, sorry. I agree that
> running "make test" on "base" would make no sense. You can rebase to
> BASE~ if you want that.
> 
> But yes, the result is the same as a rebase to HEAD~, so maybe it's fine
> to change it...
> 
>>> 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?
> 
> If you do:
> 
>      git rebase -i HEAD
> 
> Comment out the "noop" line, and save you'll get:
> 
>      error: nothing to do
> 
> And an exit code of 1.
> 
> Maybe we should silently return 0 there, but it seems to me like this
> behavior needs to be consistent with whatever "noop" is trying to
> accomplish in general (see ff74126c03a above).
> 
> That's why I said "does this really have anything per-se to do with
> --exec?". I.e. we already observe this behavior without --exec, we just
> get a noop line, and if we had no line at all we'd error with nothing to
> do.
> 
> If we're going to make "git rebase -i HEAD" do nothing, why would it
> have behavior different from a TODO list of just a "noop" line (which is
> not the same thing as "nothing to do").
> 
> That's partially a matter of consistency, but mostly the general
> paranoia that if we're going to subtly change what's *probably* an
> obscure feature hopefully many aren't relying on, then at least having
> it die instead of silently "succeed" would be better. I.e. we'll now
> silently ignore the "--exec" commands, but didn't before.

I wonder if we could print a warning if the exec command gets ignored. I 
haven't looked how hard it would be to do in general but certainly for 
'rebase -x cmd HEAD' it should be simple(ish?) to do that. We also pick 
nothing if we're already up to date. If HEAD is an ancestor of 
<upstream> then I think we avoid fast-forwarding when there is an exec 
command so we will pick commits in that case.

Best Wishes

Phillip

Re: [PATCH] sequencer: avoid adding exec commands for non-commit creating commands

[Johannes Schindelin]

Hi Elijah,

On Tue, 30 Nov 2021, Elijah Newren via GitGitGadget wrote:

> From: Elijah Newren <newren@gmail.com>
>
> The `--exec <cmd>` is documented as
>
>     Append "exec <cmd>" 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.
>
> Unfortunately, it would also add exec commands after non-pick
> operations, such as 'no-op', which could be seen for example with
>     git rebase -i --exec true HEAD
>
> todo_list_add_exec_commands() intent was to insert exec commands after
> each logical pick, while trying to consider a chains of fixup and squash
> commits to be part of the pick before it.  So it would keep an 'insert'
> boolean tracking if it had seen a pick or merge, but not write the exec
> command until it saw the next non-fixup/squash command.  Since that
> would make it miss the final exec command, it had some code that would
> check whether it still needed to insert one at the end, but instead of a
> simple
>
>     if (insert)
>
> it had a
>
>     if (insert || <condition that is always true>)
>
> That's buggy; as per the docs, we should only add exec commands for
> lines that create commits, i.e. only if insert is true.  Fix the
> conditional.
>
> There was one testcase in the testsuite that we tweak for this change;
> it was introduced in 54fd3243da ("rebase -i: reread the todo list if
> `exec` touched it", 2017-04-26), and was merely testing that after an
> exec had fired that the todo list would be re-read.  The test at the
> time would have worked given any revision at all, though it would only
> work with 'HEAD' as a side-effect of this bug.  Since we're fixing this
> bug, choose something other than 'HEAD' for that test.
>
> Finally, add a testcase that verifies when we have no commits to pick,
> that we get no exec lines in the generated todo list.
>
> Reported-by: Nikita Bobko <nikitabobko@gmail.com>
> Signed-off-by: Elijah Newren <newren@gmail.com>

This patch gets my whole-hearted ACK!

Thank you,
Dscho

> ---
>     sequencer: avoid adding exec commands for non-commit creating commands
>
>     Original report over at
>     https://lore.kernel.org/git/YaVzufpKcC0t+q+L@nand.local/T/#m13fbd7b054c06ba1f98ae66e6d1b9fcc51bb875e
>
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1149%2Fnewren%2Frebase-exec-empty-bug-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1149/newren/rebase-exec-empty-bug-v1
> Pull-Request: https://github.com/git/git/pull/1149
>
>  sequencer.c                 | 2 +-
>  t/t3429-rebase-edit-todo.sh | 7 ++++++-
>  2 files changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/sequencer.c b/sequencer.c
> index ea96837cde3..aa790f0bba8 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -5496,7 +5496,7 @@ static void todo_list_add_exec_commands(struct todo_list *todo_list,
>  	}
>
>  	/* insert or append final <commands> */
> -	if (insert || nr == todo_list->nr) {
> +	if (insert) {
>  		ALLOC_GROW(items, nr + commands->nr, alloc);
>  		COPY_ARRAY(items + nr, base_items, commands->nr);
>  		nr += commands->nr;
> diff --git a/t/t3429-rebase-edit-todo.sh b/t/t3429-rebase-edit-todo.sh
> index 7024d49ae7b..abd66f36021 100755
> --- a/t/t3429-rebase-edit-todo.sh
> +++ b/t/t3429-rebase-edit-todo.sh
> @@ -13,10 +13,15 @@ test_expect_success 'setup' '
>
>  test_expect_success 'rebase exec modifies rebase-todo' '
>  	todo=.git/rebase-merge/git-rebase-todo &&
> -	git rebase HEAD -x "echo exec touch F >>$todo" &&
> +	git rebase HEAD~1 -x "echo exec touch F >>$todo" &&
>  	test -e F
>  '
>
> +test_expect_success 'rebase exec with an empty list does not exec anything' '
> +	git rebase HEAD -x "true" 2>output &&
> +	! grep "Executing: true" output
> +'
> +
>  test_expect_success 'loose object cache vs re-reading todo list' '
>  	GIT_REBASE_TODO=.git/rebase-merge/git-rebase-todo &&
>  	export GIT_REBASE_TODO &&
>
> base-commit: 35151cf0720460a897cde9b8039af364743240e7
> --
> gitgitgadget
>