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

From: Elijah Newren <newren@gmail.com>
Date: Nov 30, 2021, 00:14 UTC
Message-ID: <CABPp-BFRE2=Owf15WzkacNfdNKbkd2n4GZh7HqDokKzeviBWRw@mail.gmail.com>
In-Reply-To: <211129.868rx7gnd5.gmgdl@evledraar.gmail.com>

On Mon, Nov 29, 2021 at 2:25 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

Show 11 quoted lines

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

Show 7 quoted lines

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

Show 15 quoted lines

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