Don't Call commit-msg Hooks With Empty Commit Messages

Don't Call commit-msg Hooks With Empty Commit Messages

[Kurt von Laven]

Hello,

The most common reason commit messages are left empty is to abort
them. commit-msg hooks that replace empty commit messages with
non-empty ones (i) make it impossible to abort commits, (ii) are
startling to developers joining a project configured in this manner,
and (iii) can offer no value that wouldn't be equally or better
offered another way. For instance, a default commit message would be
better implemented as a commit message template or prepare-commit-msg
hook. I propose that Git eventually cease calling commit-msg hooks
when the commit-message is empty, but I would understand if backwards
compatibility were the overriding concern. On the other hand, the
empty commit message case is easy to overlook when crafting a
commit-msg hook. One consequence of this behavior is that running the
popular pre-commit tool (https://pre-commit.com/) tends to lead to a
spew of false positives to the console on an aborted commit when
configured with commit-msg hooks.

Be well,
Kurt von Laven

Re: Don't Call commit-msg Hooks With Empty Commit Messages

[Junio C Hamano]

Kurt von Laven <kurt.von.laven@gmail.com> writes:

> The most common reason commit messages are left empty is to abort
> them. commit-msg hooks that replace empty commit messages with
> non-empty ones (i) make it impossible to abort commits, (ii) are
> startling to developers joining a project configured in this manner,
> and (iii) can offer no value that wouldn't be equally or better
> offered another way. For instance, a default commit message would be
> better implemented as a commit message template or prepare-commit-msg
> hook. I propose that Git eventually cease calling commit-msg hooks
> when the commit-message is empty, but I would understand if backwards
> compatibility were the overriding concern. On the other hand, the
> empty commit message case is easy to overlook when crafting a
> commit-msg hook. One consequence of this behavior is that running the
> popular pre-commit tool (https://pre-commit.com/) tends to lead to a
> spew of false positives to the console on an aborted commit when
> configured with commit-msg hooks.

The primary reason commit-msg hook is there is *not* because we need
a way to tweak the log message.  As you said, prepare-commit-msg and
templates are much better way to give some sort of default.  

The purpose of the hook is to serve as the gatekeeper to cause an
attempt with a bad commit message to fail.  And a properly written
commit-msg hook would be rejecting an empty message, instead of
inserting cruft into an empty message file.

So, from that point of view, if we were to change anything, a more
useful thing to do might be to forbid commit-msg hook from modifying
the file and make sure it would only verify but not modify, I
suspect.  Doing so would have a side effect of making sure that no
commit-msg hook will turn an empty message file into a non-empty
message file ;-).

Re: Don't Call commit-msg Hooks With Empty Commit Messages

[Ævar Arnfjörð Bjarmason]

On Fri, Sep 17 2021, Junio C Hamano wrote:

> Kurt von Laven <kurt.von.laven@gmail.com> writes:
>
>> The most common reason commit messages are left empty is to abort
>> them. commit-msg hooks that replace empty commit messages with
>> non-empty ones (i) make it impossible to abort commits, (ii) are
>> startling to developers joining a project configured in this manner,
>> and (iii) can offer no value that wouldn't be equally or better
>> offered another way. For instance, a default commit message would be
>> better implemented as a commit message template or prepare-commit-msg
>> hook. I propose that Git eventually cease calling commit-msg hooks
>> when the commit-message is empty, but I would understand if backwards
>> compatibility were the overriding concern. On the other hand, the
>> empty commit message case is easy to overlook when crafting a
>> commit-msg hook. One consequence of this behavior is that running the
>> popular pre-commit tool (https://pre-commit.com/) tends to lead to a
>> spew of false positives to the console on an aborted commit when
>> configured with commit-msg hooks.
>
> The primary reason commit-msg hook is there is *not* because we need
> a way to tweak the log message.  As you said, prepare-commit-msg and
> templates are much better way to give some sort of default.  
>
> The purpose of the hook is to serve as the gatekeeper to cause an
> attempt with a bad commit message to fail.  And a properly written
> commit-msg hook would be rejecting an empty message, instead of
> inserting cruft into an empty message file.
>
> So, from that point of view, if we were to change anything, a more
> useful thing to do might be to forbid commit-msg hook from modifying
> the file and make sure it would only verify but not modify, I
> suspect.  Doing so would have a side effect of making sure that no
> commit-msg hook will turn an empty message file into a non-empty
> message file ;-).

I'd think we'd want to call it on an empty message, e.g. maybe someone
depends on that with empty message = auto-generate a message for me.

But for those that don't, doesn't the default behavior of "git commit"
catch this in either case, i.e. it wouldn't let it pass without
--allow-empty-message.

I understood this report as the hook taking the empty message (e.g. the
user using it as a shorthand to abort), and their hook "helpfully"
inserting some "default" message or template.

Re: Don't Call commit-msg Hooks With Empty Commit Messages

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> I'd think we'd want to call it on an empty message, e.g. maybe someone
> depends on that with empty message = auto-generate a message for me.

I tend to agree with you that, especially if we are to keep the
"commit-msg hook is allowed to change the message, even though it is
a job for other hooks", we should invoke it even on an empty file.

> But for those that don't, doesn't the default behavior of "git commit"
> catch this in either case, i.e. it wouldn't let it pass without
> --allow-empty-message.
>
> I understood this report as the hook taking the empty message (e.g. the
> user using it as a shorthand to abort), and their hook "helpfully"
> inserting some "default" message or template.

My understanding largely overlaps with yours but with some
differences.

They do not fall into either of the two camps.  Their hook does futz
with the message indiscriminately and adds some boilerplate material
blindly, even to an empty file.

The complaint is "the message is otherwise without any substance,
but because the hook blindly added some boilerplate material into
the empty original, it appears to be non-empty and fails to trigger
the --no-allow-empty-message machinery."

Re: Don't Call commit-msg Hooks With Empty Commit Messages

[Kurt von Laven]

> The primary reason commit-msg hook is there is *not* because we need
> a way to tweak the log message.  As you said, prepare-commit-msg and
> templates are much better way to give some sort of default.

Yes, I completely agree. I meant my original message to refer only to
the case of an empty commit message.

> The purpose of the hook is to serve as the gatekeeper to cause an
> attempt with a bad commit message to fail.  And a properly written
> commit-msg hook would be rejecting an empty message, instead of
> inserting cruft into an empty message file.

It doesn't conceptually make sense for Git to be calling the
commit-msg hook in this case to begin with though. The developer has
already expressed an intent to abort the commit, and that intuitively
feels like it should be the end of the process. When someone sits down
to write a commit-msg hook, it's very natural to think: "how should I
handle this commit message?" It's less natural to think: "How should I
handle the case where the developer aborted the commit?" This after
all has nothing to do with the intention of the commit-msg hook being
written, so it will tend not to be on peoples' minds. If they do think
about it without investigating thoroughly, they are liable to assume
that Git wouldn't call their hook in this case since the commit has no
message and is being aborted. Worse still, even if they do recognize
that their hook will be called, they are most likely to assume that
they should reject an empty commit message by exiting with an error
code in the typical guard-against-bad-input sense, but this is also
wrong. The correct action is in a sense doubly inverted: you want to
explicitly always allow an empty commit message by exiting
successfully without output. In essence, your script must pretend that
nothing happened, because it shouldn't have, which is rarely the
correct action to take as a programmer. Every single commit-msg hook
author is presently being relied on to get this right, which
automatically means more mistakes will be made. I would be curious if
anyone is aware of any hook using the pre-commit framework that
implements this correctly by default (without relying on the end
developer to pass additional arguments). My experience using the
pre-commit framework has been a spew of erroneous errors from
commit-msg hooks in the aborted commit case, because none of them made
it even to the first step of considering the possibility that the
commit may have already been aborted.

> So, from that point of view, if we were to change anything, a more
> useful thing to do might be to forbid commit-msg hook from modifying
> the file and make sure it would only verify but not modify, I
> suspect.  Doing so would have a side effect of making sure that no
> commit-msg hook will turn an empty message file into a non-empty
> message file ;-).

I would not recommend this, no. What other hook would be better suited
to programmatically modify a non-empty commit message? Such a
restriction would break, for instance, the witty sign-commit
pre-commit hook: https://github.com/mattlqx/pre-commit-sign.
Implementing sign-commit without the power to change the commit
message from the commit-msg hook boxes you into using a
prepare-commit-msg hook, which is a worse user experience. It puts the
signature in a human-editable field where they can accidentally mangle
or delete the signature, put their message beneath it instead of above
it, delimit it from their commit message inconsistently, etc. Also,
this would not solve the problem of preventing the commit-msg hook
from being run in the first place when the commit message is empty.

> I'd think we'd want to call it on an empty message, e.g. maybe someone
> depends on that with empty message = auto-generate a message for me.

This is the backwards compatibility point I was referring to.
Definitely understand if this is a show stopper since I consider this
more of a quirk than a serious issue. I would think some amount of
warning would be in order if the behavior of commit-msg hooks were to
change to avoid unnecessary chaos, and I would be curious if anyone is
specifically aware of any commit-msg hooks that intend to consume
empty commit message. The better way to implement the auto-generated
message in my opinion would be with a commit message template or a
prepare-commit-msg hook. In theory, one could potentially also offer a
new hook that is similar to a commit-msg hook but that didn't get
called if a commit was aborted, but that feels like overkill.

> But for those that don't, doesn't the default behavior of "git commit"
> catch this in either case, i.e. it wouldn't let it pass without
> --allow-empty-message.

I agree with your intuition in the sense that that would be preferable
behavior, but no, this is not how Git behaves presently. The
commit-msg hook still gets called, and a maintainer of the pre-commit
framework has assured me that they follow Git's behavior to the tee in
this regard.

> I understood this report as the hook taking the empty message (e.g. the
> user using it as a shorthand to abort), and their hook "helpfully"
> inserting some "default" message or template.

I believe that sign-commit does exactly what you suggest, yes. The
cases I personally have encountered were simply misleading validation
errors (e.g., the commit message doesn't follow conventional commit or
isn't long enough).

> I tend to agree with you that, especially if we are to keep the
> "commit-msg hook is allowed to change the message, even though it is
> a job for other hooks", we should invoke it even on an empty file.

Do you have a use case in mind for a commit-msg hook that runs on an
empty commit message that wouldn't be better implemented some other
way? More to the point, why continue with the commit process at all at
this point? It's vaguely analogous to a program allowing third-party
extensions to automatically relaunch a program after the user closed
it. Presently commit hooks are given the power to overrule what is
most often a developer's intention to abort a commit, and the most
common case in practice that this behavior manifests is in incorrectly
implemented commit-msg hooks that either spew misleading error
messages or abort the abort process unintentionally.

> They do not fall into either of the two camps.  Their hook does futz
> with the message indiscriminately and adds some boilerplate material
> blindly, even to an empty file.

> The complaint is "the message is otherwise without any substance,
> but because the hook blindly added some boilerplate material into
> the empty original, it appears to be non-empty and fails to trigger
> the --no-allow-empty-message machinery."

None of the hooks I have personally used do this, but such hooks exist
and are problematic.

Be well,
Kurt

El vie, 17 sept 2021 a las 12:44, Junio C Hamano (<gitster@pobox.com>) escribió:
>
> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
> > I'd think we'd want to call it on an empty message, e.g. maybe someone
> > depends on that with empty message = auto-generate a message for me.
>
> I tend to agree with you that, especially if we are to keep the
> "commit-msg hook is allowed to change the message, even though it is
> a job for other hooks", we should invoke it even on an empty file.
>
> > But for those that don't, doesn't the default behavior of "git commit"
> > catch this in either case, i.e. it wouldn't let it pass without
> > --allow-empty-message.
> >
> > I understood this report as the hook taking the empty message (e.g. the
> > user using it as a shorthand to abort), and their hook "helpfully"
> > inserting some "default" message or template.
>
> My understanding largely overlaps with yours but with some
> differences.
>
> They do not fall into either of the two camps.  Their hook does futz
> with the message indiscriminately and adds some boilerplate material
> blindly, even to an empty file.
>
> The complaint is "the message is otherwise without any substance,
> but because the hook blindly added some boilerplate material into
> the empty original, it appears to be non-empty and fails to trigger
> the --no-allow-empty-message machinery."
>
>
>

Re: Don't Call commit-msg Hooks With Empty Commit Messages

[Kurt von Laven]

P.S. The maintainer of gitlint, a commit-msg hook, pointed out to me
that handling the case where the commit message is empty is trickier
than what I described. Presently there is no way to discern whether
--allow-empty-message was passed, so the hook doesn't even know
whether the commit is being aborted and is forced to handle both
cases the same way. I view this as a deeper issue with the present
behavior than what I initially brought up since it fundamentally limits
commit-msg hooks. My point here isn't that commit-msg hooks should
be told whether the commit is being aborted explicitly, but rather
that aborted commits should abort as soon as the developer enters an
empty commit message (with the one exception being the confirmation
prompt verifying that the developer intended to abort the commit),
before commit-msg hooks are called.

El sáb, 18 sept 2021 a las 2:58, Kurt von Laven
(<kurt.von.laven@gmail.com>) escribió:

>
> > The primary reason commit-msg hook is there is *not* because we need
> > a way to tweak the log message.  As you said, prepare-commit-msg and
> > templates are much better way to give some sort of default.
>
> Yes, I completely agree. I meant my original message to refer only to
> the case of an empty commit message.
>
> > The purpose of the hook is to serve as the gatekeeper to cause an
> > attempt with a bad commit message to fail.  And a properly written
> > commit-msg hook would be rejecting an empty message, instead of
> > inserting cruft into an empty message file.
>
> It doesn't conceptually make sense for Git to be calling the
> commit-msg hook in this case to begin with though. The developer has
> already expressed an intent to abort the commit, and that intuitively
> feels like it should be the end of the process. When someone sits down
> to write a commit-msg hook, it's very natural to think: "how should I
> handle this commit message?" It's less natural to think: "How should I
> handle the case where the developer aborted the commit?" This after
> all has nothing to do with the intention of the commit-msg hook being
> written, so it will tend not to be on peoples' minds. If they do think
> about it without investigating thoroughly, they are liable to assume
> that Git wouldn't call their hook in this case since the commit has no
> message and is being aborted. Worse still, even if they do recognize
> that their hook will be called, they are most likely to assume that
> they should reject an empty commit message by exiting with an error
> code in the typical guard-against-bad-input sense, but this is also
> wrong. The correct action is in a sense doubly inverted: you want to
> explicitly always allow an empty commit message by exiting
> successfully without output. In essence, your script must pretend that
> nothing happened, because it shouldn't have, which is rarely the
> correct action to take as a programmer. Every single commit-msg hook
> author is presently being relied on to get this right, which
> automatically means more mistakes will be made. I would be curious if
> anyone is aware of any hook using the pre-commit framework that
> implements this correctly by default (without relying on the end
> developer to pass additional arguments). My experience using the
> pre-commit framework has been a spew of erroneous errors from
> commit-msg hooks in the aborted commit case, because none of them made
> it even to the first step of considering the possibility that the
> commit may have already been aborted.
>
> > So, from that point of view, if we were to change anything, a more
> > useful thing to do might be to forbid commit-msg hook from modifying
> > the file and make sure it would only verify but not modify, I
> > suspect.  Doing so would have a side effect of making sure that no
> > commit-msg hook will turn an empty message file into a non-empty
> > message file ;-).
>
> I would not recommend this, no. What other hook would be better suited
> to programmatically modify a non-empty commit message? Such a
> restriction would break, for instance, the witty sign-commit
> pre-commit hook: https://github.com/mattlqx/pre-commit-sign.
> Implementing sign-commit without the power to change the commit
> message from the commit-msg hook boxes you into using a
> prepare-commit-msg hook, which is a worse user experience. It puts the
> signature in a human-editable field where they can accidentally mangle
> or delete the signature, put their message beneath it instead of above
> it, delimit it from their commit message inconsistently, etc. Also,
> this would not solve the problem of preventing the commit-msg hook
> from being run in the first place when the commit message is empty.
>
> > I'd think we'd want to call it on an empty message, e.g. maybe someone
> > depends on that with empty message = auto-generate a message for me.
>
> This is the backwards compatibility point I was referring to.
> Definitely understand if this is a show stopper since I consider this
> more of a quirk than a serious issue. I would think some amount of
> warning would be in order if the behavior of commit-msg hooks were to
> change to avoid unnecessary chaos, and I would be curious if anyone is
> specifically aware of any commit-msg hooks that intend to consume
> empty commit message. The better way to implement the auto-generated
> message in my opinion would be with a commit message template or a
> prepare-commit-msg hook. In theory, one could potentially also offer a
> new hook that is similar to a commit-msg hook but that didn't get
> called if a commit was aborted, but that feels like overkill.
>
> > But for those that don't, doesn't the default behavior of "git commit"
> > catch this in either case, i.e. it wouldn't let it pass without
> > --allow-empty-message.
>
> I agree with your intuition in the sense that that would be preferable
> behavior, but no, this is not how Git behaves presently. The
> commit-msg hook still gets called, and a maintainer of the pre-commit
> framework has assured me that they follow Git's behavior to the tee in
> this regard.
>
> > I understood this report as the hook taking the empty message (e.g. the
> > user using it as a shorthand to abort), and their hook "helpfully"
> > inserting some "default" message or template.
>
> I believe that sign-commit does exactly what you suggest, yes. The
> cases I personally have encountered were simply misleading validation
> errors (e.g., the commit message doesn't follow conventional commit or
> isn't long enough).
>
> > I tend to agree with you that, especially if we are to keep the
> > "commit-msg hook is allowed to change the message, even though it is
> > a job for other hooks", we should invoke it even on an empty file.
>
> Do you have a use case in mind for a commit-msg hook that runs on an
> empty commit message that wouldn't be better implemented some other
> way? More to the point, why continue with the commit process at all at
> this point? It's vaguely analogous to a program allowing third-party
> extensions to automatically relaunch a program after the user closed
> it. Presently commit hooks are given the power to overrule what is
> most often a developer's intention to abort a commit, and the most
> common case in practice that this behavior manifests is in incorrectly
> implemented commit-msg hooks that either spew misleading error
> messages or abort the abort process unintentionally.
>
> > They do not fall into either of the two camps.  Their hook does futz
> > with the message indiscriminately and adds some boilerplate material
> > blindly, even to an empty file.
>
> > The complaint is "the message is otherwise without any substance,
> > but because the hook blindly added some boilerplate material into
> > the empty original, it appears to be non-empty and fails to trigger
> > the --no-allow-empty-message machinery."
>
> None of the hooks I have personally used do this, but such hooks exist
> and are problematic.
>
> Be well,
> Kurt
>
> El vie, 17 sept 2021 a las 12:44, Junio C Hamano (<gitster@pobox.com>) escribió:
> >
> > Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
> >
> > > I'd think we'd want to call it on an empty message, e.g. maybe someone
> > > depends on that with empty message = auto-generate a message for me.
> >
> > I tend to agree with you that, especially if we are to keep the
> > "commit-msg hook is allowed to change the message, even though it is
> > a job for other hooks", we should invoke it even on an empty file.
> >
> > > But for those that don't, doesn't the default behavior of "git commit"
> > > catch this in either case, i.e. it wouldn't let it pass without
> > > --allow-empty-message.
> > >
> > > I understood this report as the hook taking the empty message (e.g. the
> > > user using it as a shorthand to abort), and their hook "helpfully"
> > > inserting some "default" message or template.
> >
> > My understanding largely overlaps with yours but with some
> > differences.
> >
> > They do not fall into either of the two camps.  Their hook does futz
> > with the message indiscriminately and adds some boilerplate material
> > blindly, even to an empty file.
> >
> > The complaint is "the message is otherwise without any substance,
> > but because the hook blindly added some boilerplate material into
> > the empty original, it appears to be non-empty and fails to trigger
> > the --no-allow-empty-message machinery."
> >
> >
> >