Elijah Newren wrote:
> On Thu, Jan 16, 2020 at 12:35 PM Jonathan Nieder <jrnieder@xxxxxxxxx> wrote:
>> 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.
[...]
>> 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).
[...]
>> 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.
>> 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.
>> 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.
[...]
>> 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@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.
[...]
>> 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
