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

[Kurt von Laven <kurt.von.laven@gmail.com>]

> 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."
>
>
>