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

From: Elijah Newren <newren@gmail.com>
Date: Jan 16, 2020, 23:19 UTC
Message-ID: <CABPp-BE9XqOCG0VqA5XgDbXm4apw0-_mOqW-JSO+PDZhisO0Rw@mail.gmail.com>
In-Reply-To: <20200116223912.GB71299@google.com>

On Thu, Jan 16, 2020 at 2:39 PM Jonathan Nieder <jrnieder@gmail.com> wrote:

Show 9 quoted lines

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

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

Show 9 quoted lines

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

Show 22 quoted lines

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

Show 17 quoted lines

> [...]
> >>  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?

Show 8 quoted lines

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

Show 20 quoted lines

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

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