From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: Elijah Newren <newren@gmail.com>
Cc: Nikita Bobko <nikitabobko@gmail.com>,
Git Mailing List <git@vger.kernel.org>,
Lucien Kong <Lucien.Kong@ensimag.imag.fr>,
Taylor Blau <me@ttaylorr.com>,
Phillip Wood <phillip.wood@dunelm.org.uk>,
Johannes Schindelin <johannes.schindelin@gmx.de>
Subject: Re: [BUG REPORT] `git rebase --exec` shouldn't run the exec command when there is nothing to rebase
Date: Tue, 30 Nov 2021 15:03:57 +0100 [thread overview]
Message-ID: <211130.865ys9em75.gmgdl@evledraar.gmail.com> (raw)
In-Reply-To: <pull.1149.git.git.1638244719381.gitgitgadget@gmail.com>
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.
next prev parent reply other threads:[~2021-11-30 14:45 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-11-26 12:44 [BUG REPORT] `git rebase --exec` shouldn't run the exec command when there is nothing to rebase Nikita Bobko
2021-11-29 12:07 ` Ævar Arnfjörð Bjarmason
2021-11-30 0:14 ` Elijah Newren
2021-11-30 0:43 ` Taylor Blau
2021-11-30 3:58 ` [PATCH] sequencer: avoid adding exec commands for non-commit creating commands Elijah Newren via GitGitGadget
2021-11-30 5:13 ` Taylor Blau
2021-11-30 14:03 ` Ævar Arnfjörð Bjarmason [this message]
2021-12-01 11:45 ` [BUG REPORT] `git rebase --exec` shouldn't run the exec command when there is nothing to rebase Phillip Wood
2021-12-01 11:24 ` [PATCH] sequencer: avoid adding exec commands for non-commit creating commands Phillip Wood
2021-12-03 22:22 ` Johannes Schindelin
2021-11-30 4:01 ` [BUG REPORT] `git rebase --exec` shouldn't run the exec command when there is nothing to rebase Elijah Newren
2021-11-30 11:09 ` Phillip Wood
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=211130.865ys9em75.gmgdl@evledraar.gmail.com \
--to=avarab@gmail.com \
--cc=Lucien.Kong@ensimag.imag.fr \
--cc=git@vger.kernel.org \
--cc=johannes.schindelin@gmx.de \
--cc=me@ttaylorr.com \
--cc=newren@gmail.com \
--cc=nikitabobko@gmail.com \
--cc=phillip.wood@dunelm.org.uk \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).