On Wed, Jan 15, 2020 at 07:50:11PM +0000, Jonathan Nieder wrote: > 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@xxxxxxxxx> > 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. This seems like the most important point to me. I took a very cursory peek at some search results on Github (https://github.com/search?l=&q=filename%3A%2Apost-commit%2A&type=Code) and I see mostly shims - make sure certain files were included in the commit, bring a worktree up to date, stage stuff that wasn't committed - but again, that's a very brief glance at over 19000 probable post-commit hooks. Of course it's not possible for us to examine all of them, but having these 19k scripts being run lots of times when they didn't used to during an operation as common as rebase seems a little scary. > 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? This part I do disagree with. Git warns (advice-optional) when I enter detached HEAD state. When I leave my detached HEAD with some orphaned commits within (via checkout), I get a warning unconditionally, and an advice-optional tip to use 'git branch <name> <commit-id>'. The Jiri hook is warning that these commits I'm making could be lost, but in fact Git itself is telling me that (and how to save them) at the first moment when they are actually in danger. So I think this particular example is not a good one. I'm more worried about ones like this, although in the rebase case it may well be a no-op as I can't start a rebase with a dirty index: https://github.com/alphagov/digitalmarketplace-runner/blob/f8524d9ee7465caef0571d8beb8dff4b25d10fe7/hooks/post-commit Perhaps a good question is "what could be very catastrophic if it happened during the middle of a rebase, but not such a bad thing if it happened when I run 'git commit'?" > 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? That Github search query I posted above is a pretty big haystack. I suppose plenty of folks who contribute to Git have a userbase and might be familiar with some workflows in use there, right? - Emily
