Re: [PATCH] commit & merge: modularize the empty message validator
From: Junio C Hamano <hidden>
Date: 2017-07-11 20:23:02
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.
quoted hunk
@@ -772,7 +773,7 @@ static void prepare_to_commit(struct commit_list *remoteheads) } read_merge_msg(&msg); strbuf_stripspace(&msg, 0 < option_edit); - if (!msg.len) + if (!msg.len || message_is_empty(&msg, 0))
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.