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

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

[...]

quoted

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.

Sure, it'd be possible to retain the post-commit calls for cherry-pick
but should we?  Isn't the primary reason the issue came up that having
batch-commit operations calling post-commit for each intermediate
commit is ugly/unexpected overhead?  If we're ripping it out of rebase
for that reason, it seems that cherry-pick should get the same
treatment.  I don't want to just move the inconsistencies around, I
want to fix them.

quoted

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.

Yay!

quoted

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.

I'm okay with treating them differently if there's a defining reason
why, but otherwise I think they should all behave the same.  The only
reasons I've seen for differences all fall in category C so far, which
gives me a strong opinion against treating them differently.

quoted

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.

Ah, I see.  Yeah, there's some impedance mismatch between the
purpose/definition of the post-rewrite hook and what I think is needed
for B.  Perhaps create a new post-batch-commit hook
("batch-post-commit"?) and have git-rebase, git-am, git-cherry-pick,
git-revert, etc. call it while git-commit and git-merge call
post-commit.  (Or maybe even make git-commit and git-merge also call
post-batch-commit/batch-post-commit but do so with just one commit --
and just note that post-commit's design of not taking any parameters
was a mistake and any place that happens to call it beyond
"git-commit/git-merge" is solely by accident of past implementation
and is not guaranteed to continue.)

I think the realm of "copy constructor vs. move constructor" may make
sense for post-rewrite-hook, but I'm worried that trying to apply that
logic to notification of new commits may lead us into category C
craziness.

[...]

quoted

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.

What if there are conflicts?  Because with conflicts, aren't
cherry-picks more like the 'commit --amend' case since they behave as
a "modify"?  Conflict-dependent invoking of the post-commit hook?

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.

Yeah, for note copying that distinction makes a lot of sense to me.
For "notification of new commits", it doesn't seem to jive at all for
me.

[...]

quoted

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.

[...]

quoted

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

Sounds like we might be starting to converge towards a similar
opinion.  I roughly feel the same with leaning towards something like
(B) _if_ we can address the inconsistencies rather than just shuffle
them around (meaning we also turn off post-commit for interactive
rebase and cherry-pick) and move all of them over to some kind of
batch-commit-notification mechanism.

Thanks for patiently working it through with me.

My two cents,
Jonathan

Likewise.  :-)
Elijah