On Thu, Jan 16, 2020 at 2:39 PM Jonathan Nieder <jrnieder@xxxxxxxxx> wrote:
> [...]
> >> 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!
> >> 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.
> >> 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.
> [...]
> >> 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@xxxxxxxxxxxxxxx/
> 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.
> [...]
> >> 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
