Re: [PATCH v2/RFC] commit: change the meaning of an empty commit message

[Kaartic Sivaraam]

On Thu, 2017-08-31 at 19:06 +0530, Kaartic Sivaraam wrote:
> On Thu, 2017-08-24 at 13:19 -0700, Junio C Hamano wrote:
> > 
> > The latter is easier for me as we do not have to worry about 
> > breaking people's scripts and tools used in
> > their established workflows at all.
> > 
> 
> In that case, how about doing something similar to what was done to
> 'set-upstream' option of branch? We could print a warning notice when
> the commit message is found to be empty due to the presence of a sign-
> off line. As usual we could stop warning and stop identifying log
> messages consisting only signed-off lines as empty after a few years of
> doing that.
> 
> Note: I have no idea how good an idea this is. Let me know if it's a
> bad one.
> 


I was recently searching to find the patches have gone missing in to
the void for no obvious reason and found this. Should I consider this
to be "Dropped" in terms of the "What's cooking" emails or has this
just not received the required attention?

---
Kaartic

Re: [PATCH v2/RFC] commit: change the meaning of an empty commit message

[Kaartic Sivaraam]

On Thu, 2017-08-24 at 13:19 -0700, Junio C Hamano wrote:
> 
> The latter is easier for me as we do not have to worry about 
> breaking people's scripts and tools used in
> their established workflows at all.
> 

In that case, how about doing something similar to what was done to
'set-upstream' option of branch? We could print a warning notice when
the commit message is found to be empty due to the presence of a sign-
off line. As usual we could stop warning and stop identifying log
messages consisting only signed-off lines as empty after a few years of
doing that.

Note: I have no idea how good an idea this is. Let me know if it's a
bad one.

-- 
Kaartic

Re: [PATCH v2/RFC] commit: change the meaning of an empty commit message

[Junio C Hamano]

Kaartic Sivaraam  writes:

>  As has been noted by Junio, 
>
>  "It would be a backward incompatible tightening of the established
>  rule, but it may not be a bad change."
>
>  The "It" above refers to this change. Expecting comments from people to 
> ensure
>  this change isn't a bad one.

FWIW, I am fairly neutral; I do not mind accepting this change if
other people are supportive, but I do not miss this patch if we end
up not applying it at all.  The latter is easier for me as we do not
have to worry about breaking people's scripts and tools used in
their established workflows at all.

[PATCH v2/RFC] commit: change the meaning of an empty commit message

[Kaartic Sivaraam]

An "empty commit message" according to 'commit' has long been,

A message that contains only empty lines and/or whitespaces
and/or 'Signed-off-by' lines

This is biased as a commit message that contains only other trailers like
'Helped-by: ', 'Tested-by: ' etc., could equally be considered empty but
such messages are considered valid. Detecting *all* possible trailers
and aborting when a commit message contains only those trailers is not
an easy thing as the meaning of a 'trailer' is not universal.

Further, leaving the meaning unchanged has the issue that it isn't
consistent with the meaning of an empty "merge" message which is,

A message that contains only empty lines and/or whitespaces

In order to keep the implementation simple and to be consistent with
the meaning of an "empty merge message"and  to remain unbiased redefine
the meaning of an "empty commit message" as,

A message that contains only empty lines and/or whitespaces

Users who would like to have a different notion of an "empty commit message"
can do so using the 'commit-msg' hook.

As a result of this change, the following commit message which was rejected
as empty before this change is considered to be valid as a consequence
of this change.

   START : COMMIT MESSAGE 

Signed-off-by: Random J Developer 

# Please enter the commit message for your changes. Lines starting
# with '#' will be ignored, and an empty message aborts the commit.
# ...
   END : COMMIT MESSAGE   

With the default cleanup, the above message would produce a commit with the
'Signed-off-by:' line as it's subject. Eg,

[master 4a34e74] Signed-off-by: Random J Developer 

Signed-off-by: Kaartic Sivaraam 
---

 As has been noted by Junio, 

 "It would be a backward incompatible tightening of the established
 rule, but it may not be a bad change."

 The "It" above refers to this change. Expecting comments from people to ensure
 this change isn't a bad one.

 Changes in v2:

Unlike the previous patch this one "doesn't add much". Only the meaning of
the empty commit message has been changed.

Unlike the previous patch, this one doesn't touch on 'merge' because after
this patch has been applied both commit and merge seem to reject the same 
set
of messages as an empty message.

I couldn't find the meaning of an empty commit message in any part of the
documentation. Let me know if there's some doc to update.

 builtin/commit.c | 10 ++
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 8e9380251..26636aac1 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -981,7 +981,7 @@ static int rest_is_empty(struct strbuf *sb, int start)
int i, eol;
const char *nl;
 
-   /* Check if the rest is just whitespace and Signed-off-by's. */
+   /* Check if the rest is just whitespace */
for (i = start; i < sb->len; i++) {
nl = memchr(sb->buf + i, '\n', sb->len - i);
if (nl)
@@ -989,11 +989,6 @@ static int rest_is_empty(struct strbuf *sb, int start)
else
eol = sb->len;
 
-   if (strlen(sign_off_header) <= eol - i &&
-   starts_with(sb->buf + i, sign_off_header)) {
-   i = eol;
-   continue;
-   }
while (i < eol)
if (!isspace(sb->buf[i++]))
return 0;
@@ -1003,8 +998,7 @@ static int rest_is_empty(struct strbuf *sb, int start)
 }
 
 /*
- * Find out if the message in the strbuf contains only whitespace and
- * Signed-off-by lines.
+ * Find out if the message in the strbuf contains only whitespace
  */
 static int message_is_empty(struct strbuf *sb)
 {
-- 
2.14.1.656.g66e7d6d0f