threads / discuss / 61743

Should commit-msg hook receive the washed message?

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

## tl;dr

3 messages between Jul 5, 2024 and Jul 6, 2024.

replies: 2people: 3as markdown or json

Sean Allred· Jul 5, 2024, 20:12 UTC · lore
From githooks.txt:
Show 5 quoted lines
> 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
Eric Sunshine· Jul 5, 2024, 21:35 UTC · re: Sean Allred · lore

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

[cc:+peff +philip]
On Fri, Jul 5, 2024 at 4:12 PM Sean Allred <allred.sean@gmail.com> wrote:
Show 19 quoted lines
> 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/

Jeff King· Jul 6, 2024, 06:37 UTC · re: Eric Sunshine · lore

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

On Fri, Jul 05, 2024 at 05:35:25PM -0400, Eric Sunshine wrote:
Show 12 quoted lines
> > 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

← back to recent threads