Should commit-msg hook receive the washed message?

Should commit-msg hook receive the washed message?

[Sean Allred]

From githooks.txt:
> This hook is invoked by linkgit:git-commit[1] and
> linkgit:git-merge[1], and can be bypassed with the `--no-verify`
> option. It takes a single parameter, the name of the file that holds
> the proposed commit log message. Exiting with a non-zero status causes
> the command to abort.

Of course the actual 'proposed commit log message' doesn't include the
comments included when running a commit, e.g.

    git -c commit.status=true commit

but the execution of the `commit-msg` happens before `cleanup_message`
is called on COMMIT_EDITMSG.

This seems like a bug to me; is there something I'm missing? I would
propose adding a call to `cleanup_message` (with the appropriate
arguments) inside `prepare_to_commit` right before `commit-msg` is
invoked.

It's causing us quite a bit of grief (e.g. with external tools that
invoke hooks incorrectly [1] + some other internal workarounds for
things like patch scissors).

Thanks,
-Sean

[1]: https://lore.kernel.org/git/17df67804ef7a3c8.df629cdadcf4ea15.524a056283063601@EPIC94403/

-- 
Sean Allred

Re: Should commit-msg hook receive the washed message?

[Eric Sunshine]

[cc:+peff +philip]

On Fri, Jul 5, 2024 at 4:12 PM Sean Allred <allred.sean@gmail.com> wrote:
> From githooks.txt:
> > This hook is invoked by linkgit:git-commit[1] and
> > linkgit:git-merge[1], and can be bypassed with the `--no-verify`
> > option. It takes a single parameter, the name of the file that holds
> > the proposed commit log message. Exiting with a non-zero status causes
> > the command to abort.
>
> Of course the actual 'proposed commit log message' doesn't include the
> comments included when running a commit, e.g.
>
>     git -c commit.status=true commit
>
> but the execution of the `commit-msg` happens before `cleanup_message`
> is called on COMMIT_EDITMSG.
>
> This seems like a bug to me; is there something I'm missing? I would
> propose adding a call to `cleanup_message` (with the appropriate
> arguments) inside `prepare_to_commit` right before `commit-msg` is
> invoked.

The idea of calling cleanup_message() has been discussed before[1]. My
takeaway from reading that message is that calling cleanup_message()
unconditionally before invoking the hook could potentially throw away
information that the hook might want to consult. It's possible to
imagine a workflow in which a specialized comment is inserted in a
commit message to control/augment behavior of the hook in some
fashion.

The idea you proposed in a different thread[2] of exposing
cleanup_message() functionality as a user-facing utility which a hook
can call on an as-needed basis may make more sense(?).

[1]: https://lore.kernel.org/git/693954a7-af64-67c5-41b9-b648a9fe3ef2@gmail.com/
[2]: https://lore.kernel.org/git/m034onpng4.fsf@epic96565.epic.com/

Re: Should commit-msg hook receive the washed message?

[Jeff King]

On Fri, Jul 05, 2024 at 05:35:25PM -0400, Eric Sunshine wrote:

> > This seems like a bug to me; is there something I'm missing? I would
> > propose adding a call to `cleanup_message` (with the appropriate
> > arguments) inside `prepare_to_commit` right before `commit-msg` is
> > invoked.
> 
> The idea of calling cleanup_message() has been discussed before[1]. My
> takeaway from reading that message is that calling cleanup_message()
> unconditionally before invoking the hook could potentially throw away
> information that the hook might want to consult. It's possible to
> imagine a workflow in which a specialized comment is inserted in a
> commit message to control/augment behavior of the hook in some
> fashion.

Yeah, looking over that earlier discussion, I think the main takeaway is
that the unsanitized version might have useful information for the hook.
I don't know of any real workflow that relies on that, but it does seem
possible that somebody has one.

> The idea you proposed in a different thread[2] of exposing
> cleanup_message() functionality as a user-facing utility which a hook
> can call on an as-needed basis may make more sense(?).

So yes, I like that approach much better. But as noted elsewhere, the
hook has to understand which cleanup mechanism is going to be used.
Which could get complicated.

It would be nice if we could just provide _both_ forms to the hook. It
looks like commit-msg just takes the filename as the first parameter.
Perhaps we could extend it by passing a second one? It does mean
sanitizing and writing out the message twice, even if the hook might not
look at it, but I doubt the overhead is all that high.

-Peff

Re: Should commit-msg hook receive the washed message?

[brianmlyles]

> Eric Sunshine <sunshine@sunshineco.com> wrote:
> 
> The idea you proposed in a different thread[2] of exposing
> cleanup_message() functionality as a user-facing utility which a hook
> can call on an as-needed basis may make more sense(?).
> 
> [2]: https://lore.kernel.org/git/m034onpng4.fsf@epic96565.epic.com/

I don't think that would suffice for our use case. Specifically, the
commit-msg hook has no way to know if it's looking at a message that
will be washed, or will not. For example, let's say a commit-msg hook
aims to enforce a 72-character hard-wrap policy. If a line starts with
`#`, what is the hook supposed to do?

- If the commit was created using `git commit` and the user's normal
  editor is invoked, the hook should *not* evaluate any line starting
  with `#` because that line will be removed prior to creating the
  commit as part of the message washing process.
- If the commit was created using `git commit -m`, the hook *should*
  evaluate any line starting with `#` because that line will not be
  removed by washing.

This challenge also exists for the patch scissors and any content
following it. That could have been added by git because the user called
`git commit -v` (most likely), but it also could in theory have been
added by the user, in which case the patch (or patch-like content) ends
up in the final commit message as well. The hook has no way to know
whether the commit was initiated via `git commit -v` or not. The
real-world use case here could be a commit-msg hook that adds a trailer:
if the patch will be removed during washing, then using `git
interpret-trailers` to add the trailer (which places it *before* the
patch) is fine. If the patch will not be removed during washing, then 
`git interpret-trailers` ends up putting the trailer in the middle of
the final message between the subject/body and the patch, then the patch
isn't removed and thus it's not a valid trailer.

Ultimately, the hook doesn't know what will happen to the message after
it runs, and thus doesn't know if a given line is guaranteed to be part
of the final commit message. This presents these edge case challenges
when attempting to implement the hook.

If the hook either were always called with the final proposed commit
message, or if it knew that the message would be washed after being
executed, then we'd have options for handling these edge cases.

-- 
Thanks,
Brian Lyles