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

[Jonathan Nieder <jrnieder@gmail.com>]

Hi,

Phillip Wood wrote:
> On 11/01/2020 01:16, Elijah Newren wrote:

>> But the big question here, is what is correct behavior?  Should rebase
>> call the post-commit hook, or should it skip it?  I haven't any clue
>> what the answer to that is.
>
> It's creating a new commit so I lean towards thinking it should run the
> post-commit hook. As an example I have a post-commit hook that prints a
> warning if a commit is created on a branch that is being rewritten by one of
> my scripts in another worktree. There are pre-commit and pre-rebase hooks to
> try and prevent that, but the warning is there as a last resort if those
> hooks are by-passed.

The initial design for the post-commit hook was

    commit 89e2c5f17b901edf28a8bb778ece3f6c18bbb721
    Author: Junio C Hamano <gitster@pobox.com>
    Date:   Thu Aug 18 17:20:08 2005 -0700

    Add commit hook and make the verification customizable.

    There are three hooks:

        - 'pre-commit' is given an opportunity to inspect what is
          being committed, before we invoke the EDITOR for the
          commit message;

        - 'commit-msg' is invoked on the commit log message after
          the user prepares it;

        - 'post-commit' is run after a successful commit is made.

    The first two can interfere to stop the commit.  The last one is
    for after-the-fact notification.

The initial implementation was only in "git commit" and didn't affect
other commands, but that's an artifact of implementation, not
intention.  The intention is more murky: certainly "creating a new
commit" is not the event we want to notify about (for example, "git
commit-tree" should not invoke the hook), but if I were designing it
today then all operations that create a new commit and update the
current branch with it might qualify.

Even that definition is a bit fuzzy: when I run "git rebase <upstream>
<branch>", am I updating <branch> or the current branch?  Are
cherry-picks that carry over changes that were committed previously
new commits?

Worse, we have the body of existing post-commit hooks to contend with,
and each one may have made different assumptions about the semantics.
We do not have the luxury of designing the hook without regard to that
body of existing hooks today.

The hook that jiri installs is very simple.  It wants to check when a
user has (interactively) made a commit on a detached HEAD, to let them
know that they might want to use "git checkout -b" afterward.  With
this particular hook, the behavior is better when git rebase does not
invoke the hook, since in the context of a rebase, the user has no
need to use "git checkout -b" afterward.  This is a regression, and I
think we need to take that seriously.

It doesn't seem like jiri is doing anything weird here, so there's a
reasonably high probability that other hooks would be affected in the
same way.  How can we help authors of such hooks to handle this
change?

I don't think that starting to invoke the 'post-commit' hook even more
without going through that thought process is an acceptable fix.
Since the "right" semantics aren't obvious here, the first step is
probably to collect some typical examples of post-commit hooks, like
the example from the message I'm replying to (thank you!).  Ideas for
where we can find more?

Thanks,
Jonathan