Re: [PATCH] commit & merge: modularize the empty message validator

Kaartic Sivaraam [off-list ref] writes:

In the context of "git merge" the meaning of an "empty message"
is one that contains no line of text. This is not in line with
"git commit" where an "empty message" is one that contains only
whitespaces and/or signed-off-by lines. This could cause surprises
to users who are accustomed to the meaning of an "empty message"
of "git commit".

Prevent such surprises by ensuring the meaning of an empty 'merge
message' to be in line with that of an empty 'commit message'. This
is done by separating the empty message validator from 'commit' and
making it stand-alone.

Signed-off-by: Kaartic Sivaraam <redacted>
---
 I have made an attempt to solve the issue by separating the concerned
 function as I found no reason against it.

 I've tried to name them with what felt appropriate and concise to me.
 Let me know if it's alright.

I probably would have avoided a pair of new files just to house a
single function.  I anticipate that the last helper function in
commit.c at the top-level would become relevant to this topic, and
because of that, I would have added this function at the end of the
file if I were doing this patch.

I do not see much point in checking !msg.len here.  The function
immediately returns by hitting the termination condition of the
outermost loop and this is not a performance-critical codepath.

I think the "validation" done with the rest_is_empty() is somewhat
bogus.  Why should we reject a commit without a message and a
trailer block with only signed-off-by lines, while accepting a
commit without a message and a trailer block as long as the trailer
block has something equally meaningless by itself, like
"Helped-by:"?  I think we should inspect the proposed commit log
message taken from the editor, find its tail ignoring the trailing
comment using ignore_non_trailer, and further separate the result
into (<message>, <trailers>, <junk at the tail>) using the same
logic used by the interpret-trailers tool, and then complain when
<message> turns out to be empty, to be truly useful and consistent.

And for that eventual future, merging the logic used in commit and
merge might be a good first step.

Having said all that, I am not sure "Prevent such surprises" is a
problem that is realistic to begin with.  When a user sees the
editor buffer in "git merge", it is pre-populated with at least a
single line of message "Merge branch 'foo'", possibly followed by
the summary of the side branch being merged, so unless the user
deliberately removes everything and then add a sign-off line
(because we do not usually add one), there is no room for "such
surprises" in the first place.  It does not _hurt_ to diagnose such
a crazy case, but it feels a bit lower priority.

So from the point of "let's improve what merge does", this change
looks to me a borderline "Meh"; but to improve the "why sign-off is
so special and behave differently from helped-by when deciding if
there is any log?" situation, having a separate helper function that
is shared across multiple codepaths that accept edited result may be
a good idea.