revise -c doesn't run git hooks on new commit

[yaahc]

Not sure if this is intentional or not or something that can be fixed with revise's design, but we use gerrit at work so each commit has to have a change-id inserted into the commit message before it can be pushed. I tried splitting up a commit with git revise -c (awesome btw) and it worked perfectly but did not run the commit hook so I was left without the change id in the new commit.

This isn't a huge issue, I just immediately ran git commit --amend after the revise and it added the change id no problem, but I figured I'd mention it in case its a feature you can / would like to add.

[Manishearth]

Note that git-revise doesn't ever check out the commits it creates, which would be necessary for running normal git hooks.

[yaahc]

I was afraid of something like that :(

[Manishearth]

What kind of git hook is this?

[Manishearth]

If it's just a commit-msg hook we could potentially run them though I'd rather make that a config.

[yaahc]

It seems like it is in fact commit-msg, if you want a copy of it I can probably upload it, let me make sure it's got nothing sensitive.

[mystor]

The commit-related hooks appear to be:

• pre-commit: Can't run this one - I believe it requires the index to be in a valid state, which would be too expensive.
• prepare-commit-msg: This is probably runnable - it allows modifying the commit message before it is presented to the user. Not entirely sure if this is run if the message isn't going to be presented in an editor.
• commit-msg: Probably runnable as well, would modify the commit message at the end. Not sure if it's OK to run this repeatedly on the same commit, potentially?
• post-commit: Probably shouldn't run this one - it likely expects HEAD to be updated.
• post-rewrite: We should probably run this, although interestingly git filter-branch currently doesn't. We'd probably want to either pass in rebase (for compatibility), or revise (which might also work out?)

Other than post-rewrite I don't know if these hooks are run normally during a rebase.

[yaahc]

Yea, I can't really say what my normal workflow for splitting up a commit was before revise, its always a hard / annoying task and I don't remember how I normally end up managing it, but it definitely wasn't just a rebase so I can't really compare the hooks that would get run normally vs what revise runs :(

I think my old strategy was always to branch off of the commit before the one i want to split, cherry pick, reset soft, add, make 1st commit, add the rest, make second commit, then rebase the remaining commits from the old branch to the new one and then move the head of the old branch to the new one. Bleh

[mystor]

It might be possible to run commit hooks only when performing a cut operations? I think that's one of the few situations where git revise fabricates a new commit. I suppose the other would be in interactive mode if the "index" commit gets changed into a "pick" command.

[Manishearth]

The "maybe"s are why i want this to be prefgated, it should only be used with hooks that don't read from git state.

[totph]

PR #82 adds opt-in[1] support for the commit-msg hook. My use case is also the addition of a Change-Id: I.. field. Since these message-altering hooks should be idempotent already (--amend also runs them), it should be fine to run them on any existing message.

Opt-in because as mentioned the index usually does not match.

Should this be also / alternatively configurable via a --[no-]hooks flag, similar to the --no-verify one of git-commit?

Support for prepare-commit-msg would be a bit more involved due to the 2nd and 3rd parameters it expects, see githooks(5).

1: git -c revise.run-hooks.commit-msg=true revise ...

[arxanas]

Other than post-rewrite I don't know if these hooks are run normally during a rebase.

The rules are really arbitrary for git rebase. For example, if you reword a commit, it will run the pre-commit hook, but if you fixup or squash a commit, it won't. I don't think Git's actual decision of when to run these hooks should be taken too seriously.

For what it's worth, from among these, git-branchless only runs the post-rewrite hook, as pre-commit/post-commit don't make sense in an in-memory context (and it doesn't have commit rewording facilities, so it doesn't run the commit-msg hooks).

I think if a user really wanted to be informed of in-memory rewrites, it would be best to make a new kind of hook that would be invoked for such cases, which would be given the commit hash to operate on, instead of being expected to operate on the working copy.

We'd probably want to either pass in rebase (for compatibility), or revise (which might also work out?)

FWIW again, git-branchless only looks for amend as the type:

https://github.com/arxanas/git-branchless/blob/6d76356a6bca1f040a1b7665d4549a2bd06ea6ce/src/core/rewrite/hooks.rs#L127

This is to distinguish the case of amends running during a rebase versus amends running individually. In the former case, we want to suppress output until the entire rebase operation is done.

git-branchless passes rebase to the post-rewrite hook, for no particular reason other than that it's not amend:

https://github.com/arxanas/git-branchless/blob/850ca44e069d4fb35eb068dd68a688493014c64e/src/core/rewrite/execute.rs#L657

I think in-memory rebases are sufficiently different that it would be reasonable for us to come up with a new parameter to pass there, if the user wanted to be better-informed.

[anordal]

The "maybe"s are why i want this to be prefgated, it should only be used with hooks that don't read from git state.

I hope users don't need to set this differently depending on compatibility with other tools. Configuration is a failure. As such, I would worry about removing the preference later.

I think if a user really wanted to be informed of in-memory rewrites, it would be best to make a new kind of hook that would be invoked for such cases, which would be given the commit hash to operate on, instead of being expected to operate on the working copy.

Sounds like a better idea. If the right interface doesn't exist, someone has to make it. And since the user can always symlink the new hook to the old, it's a superset of a preference.

I don't know if specific hooks like commit-msg and post-rewrite warrant this, though. Maybe it depends on whether it's the interface or the possible assumptions (that the interface doesn't promise) that we worry about.

[anordal]

How about the hermetic option – run commit-msg, but not in the repository? An empty directory in $XDG_RUNTIME_DIR should do. Then, we don't really need to disable it, at least by default: If a script expects to find itself in a git repository, it won't. And it's a detectable condition, so the user can always check it in a wrapper and disable as necessary.

It is indeed the commit-msg hook that Gerrit instructs users to set up:
https://gerrit-review.googlesource.com/Documentation/user-changeid.html

If I'm not mistaken, this is the script you get. I tried it, and it indeed works outside the context of a repository:
https://gerrit.googlesource.com/git-repo/+/master/hooks/commit-msg

The irony of making a new hook for this purpose is that it would do exactly as the documentation for commit-msg already says: The script takes one argument – the path to the commit message file – and that's it. The hermetic approach would then be to deny any implicit context that the documentation doesn't say that it has.

I've used Gerrit before, and it was fantastic: If you've thought you wanted dependent pull requests in GitHub or GitLab, with dependent feature branches that need constant rebasing, Gerrit lets you just force-push one big feature branch all day, and it understands that your commits are born dependent pull requests, with their own intact histories, without letting rebases, cherry-picks and stale base branches distract your peers. So that's what a change-id can give you, I guess.