Re: [PATCH v3 15/15] rebase: change the default backend from "am" to "merge"

Elijah Newren wrote:

On Thu, Jan 16, 2020 at 12:35 PM Jonathan Nieder [off-list ref] wrote:

quoted

Historically "git cherry-pick" ran "git commit", causing it to run the

What do you mean "historically"?

Before 356ee4659bb (sequencer: try to commit without forking 'git
commit', 2017-11-24).  See also 4627bc777e9 (sequencer: run
post-commit hook, 2019-10-15).

                                 I'm pretty sure this piece of code
is shared in the sequencer between rebase -i, rebase --merge, and
cherry-pick, so all three currently call post-commit unless I'm
mistaken.

Yep, that's right.

[...]

quoted

 A. run post-commit hook consistently in rebase --am and --merge modes

Could you spell these out a bit more?  I think by this option you mean
"Always run post-commit when commits are created", which includes
whether those commits were created by git-commit, git-rebase (any
backend), git-merge, git-cherry-pick, git-revert, git-am, etc. (but
with a glaring exception for fast-import and things that call it, much
like the exception post-rewrite already carves out for it).

Much as I don't see a reason to make rebase --merge different than
rebase --am, I don't see a reason to differentiate between git-merge
and git-commit.  I also don't see why rebase --am should be different
than am.

Sure, that matches the general gist behind (A).

[...]

quoted

 B. stop running the post-commit hook in rebase

AND sometimes in both cherry-pick and git-revert, depending on the
number of commit(s) picked/reverted (see below).

Interesting.  Doesn't "git cherry-pick A..B" run post-commit after
each commit today?  It would be possible to do (B) and retain that
behavior.

Also, even if we go this route, I think the post-commit hook should be
added to git-merge whenever it creates a merge commit.

Agreed.

quoted

    Advantages:
    * makes --am and --merge consistent with minimal user impact

...and also makes --interactive and --exec and other cases consistent too.

Hm.  My initial thought with (B) was to stop running post-commit in
non-interactive rebase but keep running it in rebase --interactive.

I don't have a strong opinion about this, though.

quoted

    Disadvantages:
    * meaning of the post-commit hook remains a muddle

Why?  "Commands which create no more than one commit (git-commit,
git-merge, maybe single-commit git-revert or git-cherry-pick) call
post-commit, commands which create several commits derived from others
(git-am, git-rebase, git-cherry-pick, sometimes git-revert ) call
post-rewrite instead for performance reasons"

Example: if I run "git revert A..B", following that rule we'd want to
call post-rewrite instead of post-commit.  But post-rewrite gets input

	<old-sha1> SP <new-sha1> [ SP <extra-info> ] LF

so that it can be used to copy data such as notes.  What do I pass in as
<old-sha1>?

Relatedly, if I remember correctly, we don't call post-rewrite in "git
cherry-pick" because cherry-pick is considered to represent a *new*
commit instead of a modified version of the old one.  In C++ terms,
it's a copy constructor instead of a move constructor. ;-)  See
notes.rewrite.* in "git help notes" for more on that subject.

[...]

quoted

 C. stop running the post-commit hook in rebase --merge or commit --amend

    Advantages:
    * produces a consistent definition of post-commit ("it's about new
      commits only")

But cherry-pick never creates "new" commits if commit --amend doesn't,
so we'd also want to turn the post-commit hook off for cherry-pick,
right?

With the copy/modify distinction described above, "git commit --amend"
acts as a "modify" operation in a user's workflow, while "git
cherry-pick" acts as a "copy" operation.

That means that with this definition, post-commit is appropriate to
run in cherry-pick but not in commit --amend.

This matches the distinction used by note rewriting and the
post-rewrite hook.  Looking back at the discussion in
https://lore.kernel.org/git/cover.1266164150.git.trast@student.ethz.ch/ (local)
I don't see this covered, so it's possible I'm remembering wrong (a
pointer to the motivating discussion that inspired that patch series
would be welcome).  In the context of note copying, this distinction
ended up being a good decision in practice in most workflows I've
seen.

[...]

quoted

    Disadvantages:
    * the change to historical "git commit --amend" behavior is likely
      to be surprising

That is certainly one issue, but I'd say this case is riddled with
problems and is the most muddled of all.

I'm inclined to agree, based on the "git commit --amend" example.

[...]

I think it was useful to phrase it this way and list the various
advantages, disadvantages, and mitigations; thanks.

Is there anything I've missed in my additional details?  Anything
sound crazy (beyond what I said I think case C leads to)?

To sum it up, I lean toward (B), but (A) is not so bad, either
(especially with the "I need some time to adapt to the change" opt-out
config described under mitigations).

Thanks for patiently working it through with me.

My two cents,
Jonathan