{"thread":{"id":"61743","subject":"Should commit-msg hook receive the washed message?","startedAt":"2024-07-05T20:12:31Z","lastAt":"2024-07-06T06:37:38Z","messageCount":3,"participants":["Sean Allred","Eric Sunshine","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"498120","messageId":"m0h6d3pphu.fsf@epic96565.epic.com","threadId":"61743","inReplyTo":null,"subject":"Should commit-msg hook receive the washed message?","fromName":"Sean Allred","fromEmail":"allred.sean@gmail.com","sentAt":"2024-07-05T20:12:29Z","receivedAt":"2024-07-05T20:12:31Z","isPatch":false,"sender":{"key":"allred.sean@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2082195?v=4"},"body":"From githooks.txt:\n> This hook is invoked by linkgit:git-commit[1] and\n> linkgit:git-merge[1], and can be bypassed with the `--no-verify`\n> option. It takes a single parameter, the name of the file that holds\n> the proposed commit log message. Exiting with a non-zero status causes\n> the command to abort.\n\nOf course the actual 'proposed commit log message' doesn't include the\ncomments included when running a commit, e.g.\n\n    git -c commit.status=true commit\n\nbut the execution of the `commit-msg` happens before `cleanup_message`\nis called on COMMIT_EDITMSG.\n\nThis seems like a bug to me; is there something I'm missing? I would\npropose adding a call to `cleanup_message` (with the appropriate\narguments) inside `prepare_to_commit` right before `commit-msg` is\ninvoked.\n\nIt's causing us quite a bit of grief (e.g. with external tools that\ninvoke hooks incorrectly [1] + some other internal workarounds for\nthings like patch scissors).\n\nThanks,\n-Sean\n\n[1]: https://lore.kernel.org/git/17df67804ef7a3c8.df629cdadcf4ea15.524a056283063601@EPIC94403/\n\n-- \nSean Allred\n"},{"id":"498127","messageId":"CAPig+cTpxXNwy8MYWjcDTa5QPoq5Mod3_LZ=+F16-gF5QVbrkg@mail.gmail.com","threadId":"61743","inReplyTo":"m0h6d3pphu.fsf@epic96565.epic.com","subject":"Re: Should commit-msg hook receive the washed message?","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2024-07-05T21:35:25Z","receivedAt":"2024-07-05T21:35:37Z","isPatch":false,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"[cc:+peff +philip]\n\nOn Fri, Jul 5, 2024 at 4:12 PM Sean Allred <allred.sean@gmail.com> wrote:\n> From githooks.txt:\n> > This hook is invoked by linkgit:git-commit[1] and\n> > linkgit:git-merge[1], and can be bypassed with the `--no-verify`\n> > option. It takes a single parameter, the name of the file that holds\n> > the proposed commit log message. Exiting with a non-zero status causes\n> > the command to abort.\n>\n> Of course the actual 'proposed commit log message' doesn't include the\n> comments included when running a commit, e.g.\n>\n>     git -c commit.status=true commit\n>\n> but the execution of the `commit-msg` happens before `cleanup_message`\n> is called on COMMIT_EDITMSG.\n>\n> This seems like a bug to me; is there something I'm missing? I would\n> propose adding a call to `cleanup_message` (with the appropriate\n> arguments) inside `prepare_to_commit` right before `commit-msg` is\n> invoked.\n\nThe idea of calling cleanup_message() has been discussed before[1]. My\ntakeaway from reading that message is that calling cleanup_message()\nunconditionally before invoking the hook could potentially throw away\ninformation that the hook might want to consult. It's possible to\nimagine a workflow in which a specialized comment is inserted in a\ncommit message to control/augment behavior of the hook in some\nfashion.\n\nThe idea you proposed in a different thread[2] of exposing\ncleanup_message() functionality as a user-facing utility which a hook\ncan call on an as-needed basis may make more sense(?).\n\n[1]: https://lore.kernel.org/git/693954a7-af64-67c5-41b9-b648a9fe3ef2@gmail.com/\n[2]: https://lore.kernel.org/git/m034onpng4.fsf@epic96565.epic.com/\n"},{"id":"498152","messageId":"20240706063737.GF700645@coredump.intra.peff.net","threadId":"61743","inReplyTo":"CAPig+cTpxXNwy8MYWjcDTa5QPoq5Mod3_LZ=+F16-gF5QVbrkg@mail.gmail.com","subject":"Re: Should commit-msg hook receive the washed message?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-07-06T06:37:37Z","receivedAt":"2024-07-06T06:37:38Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jul 05, 2024 at 05:35:25PM -0400, Eric Sunshine wrote:\n\n> > This seems like a bug to me; is there something I'm missing? I would\n> > propose adding a call to `cleanup_message` (with the appropriate\n> > arguments) inside `prepare_to_commit` right before `commit-msg` is\n> > invoked.\n> \n> The idea of calling cleanup_message() has been discussed before[1]. My\n> takeaway from reading that message is that calling cleanup_message()\n> unconditionally before invoking the hook could potentially throw away\n> information that the hook might want to consult. It's possible to\n> imagine a workflow in which a specialized comment is inserted in a\n> commit message to control/augment behavior of the hook in some\n> fashion.\n\nYeah, looking over that earlier discussion, I think the main takeaway is\nthat the unsanitized version might have useful information for the hook.\nI don't know of any real workflow that relies on that, but it does seem\npossible that somebody has one.\n\n> The idea you proposed in a different thread[2] of exposing\n> cleanup_message() functionality as a user-facing utility which a hook\n> can call on an as-needed basis may make more sense(?).\n\nSo yes, I like that approach much better. But as noted elsewhere, the\nhook has to understand which cleanup mechanism is going to be used.\nWhich could get complicated.\n\nIt would be nice if we could just provide _both_ forms to the hook. It\nlooks like commit-msg just takes the filename as the first parameter.\nPerhaps we could extend it by passing a second one? It does mean\nsanitizing and writing out the message twice, even if the hook might not\nlook at it, but I doubt the overhead is all that high.\n\n-Peff\n"}]}