Why doesn't merge fail if message has only sign-off?

Why doesn't merge fail if message has only sign-off?

[Kaartic Sivaraam]

While trying to merge a branch using "git merge" if a merge
message consists only of a "Sign-off" line it doesn't fail.
To be consistent with the behaviour of "git commit" shouldn't the merge
fail?

-- 
Kaartic

Re: Why doesn't merge fail if message has only sign-off?

[Junio C Hamano]

Kaartic Sivaraam <kaarticsivaraam91196@gmail.com> writes:

> While trying to merge a branch using "git merge" if a merge
> message consists only of a "Sign-off" line it doesn't fail.
> To be consistent with the behaviour of "git commit" shouldn't the merge
> fail?

I think that it is not by design that it doesn't fail.  It's not
like we decided to allow s-o-b only merge because we found a reason
why it is a good idea to do so.

So I do not think anybody minds too deeply if somebody came up a
patch to "fix" it.  It's just that nobody tried to create such a
silly merge in real life so far (I do not think you did, either--you
found this out by playing around trying to find corner cases, no?)

Re: Why doesn't merge fail if message has only sign-off?

[Kaartic Sivaraam]

On Mon, 2017-07-03 at 10:21 -0700, Junio C Hamano wrote:
> I think that it is not by design that it doesn't fail.  It's not
> like we decided to allow s-o-b only merge because we found a reason
> why it is a good idea to do so.
> 
> So I do not think anybody minds too deeply if somebody came up a
> patch to "fix" it.  It's just that nobody tried to create such a
> silly merge in real life so far (I do not think you did, either--you
> found this out by playing around trying to find corner cases, no?)
> 
Yes and no. I found this out while playing around with the "insert
notes in the commit template" patch I sent previously. I wasn't trying
to find corner cases, though.

-- 
Kaartic

[PATCH] merge-message: change meaning of "empty merge message"

[Kaartic Sivaraam]

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 changing the meaning of an empty 'merge
message' to be in line with that of an empty 'commit message'.

Signed-off-by: Kaartic Sivaraam <kaarticsivaraam91196@gmail.com>
---
 builtin/merge.c | 35 ++++++++++++++++++++++++++++++++++-
 1 file changed, 34 insertions(+), 1 deletion(-)

diff --git a/builtin/merge.c b/builtin/merge.c
index 703827f00..db4bf1c40 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -748,6 +748,39 @@ static void abort_commit(struct commit_list *remoteheads, const char *err_msg)
 	exit(1);
 }
 
+/*
+ * Find out if the message in the strbuf contains only whitespace and
+ * Signed-off-by lines.
+ *
+ * This function is the "rest_is_space" function of "commit" with the unwanted
+ * parameter removed.
+ */
+static int message_is_empty(struct strbuf *sb)
+{
+	int i, eol;
+	const char *nl;
+
+	/* Check if the rest is just whitespace and Signed-off-by's. */
+	for (i = 0; i < sb->len; i++) {
+		nl = memchr(sb->buf + i, '\n', sb->len - i);
+		if (nl)
+			eol = nl - sb->buf;
+		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;
+	}
+
+	return 1;
+}
+
 static const char merge_editor_comment[] =
 N_("Please enter a commit message to explain why this merge is necessary,\n"
    "especially if it merges an updated upstream into a topic branch.\n"
@@ -772,7 +805,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))
 		abort_commit(remoteheads, _("Empty commit message."));
 	strbuf_release(&merge_msg);
 	strbuf_addbuf(&merge_msg, &msg);
-- 
2.11.0

Re: [PATCH] merge-message: change meaning of "empty merge message"

[Kevin Daudt]

On Thu, Jul 06, 2017 at 09:01:49AM +0530, Kaartic Sivaraam wrote:
> 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 changing the meaning of an empty 'merge
> message' to be in line with that of an empty 'commit message'.
> 
> Signed-off-by: Kaartic Sivaraam <kaarticsivaraam91196@gmail.com>
> ---
>  builtin/merge.c | 35 ++++++++++++++++++++++++++++++++++-
>  1 file changed, 34 insertions(+), 1 deletion(-)
> 
> diff --git a/builtin/merge.c b/builtin/merge.c
> index 703827f00..db4bf1c40 100644
> --- a/builtin/merge.c
> +++ b/builtin/merge.c
> @@ -748,6 +748,39 @@ static void abort_commit(struct commit_list *remoteheads, const char *err_msg)
>  	exit(1);
>  }
>  
> +/*
> + * Find out if the message in the strbuf contains only whitespace and
> + * Signed-off-by lines.
> + *
> + * This function is the "rest_is_space" function of "commit" with the unwanted
> + * parameter removed.

The function is called "rest_is_empty".

But isn't it better that commit and merge use the same code, instead of
duplicating it again? Otherwise one may be updated, and the other
forgotten, getting differences in behaviur, which is what you want to
solve.

Kevin

Re: [PATCH] merge-message: change meaning of "empty merge message"

[Kaartic Sivaraam]

On Thu, 2017-07-06 at 06:46 +0200, Kevin Daudt wrote:
> 
> The function is called "rest_is_empty".
> 
Thanks for correcting that!

> But isn't it better that commit and merge use the same code, instead
> of
> duplicating it again? Otherwise one may be updated, and the other
> forgotten, getting differences in behaviur, which is what you want to
> solve.
> 
Yes, I did think of that. It *seems* that neither "message_is_empty" or
the "rest_is_empty" are exposed to other files. Have to work on that.

-- 
Kaartic

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

[Kaartic Sivaraam]

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 <kaarticsivaraam91196@gmail.com>
---
 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.
 
 Makefile            |  1 +
 builtin/commit.c    | 39 +++++----------------------------------
 builtin/merge.c     |  3 ++-
 message-validator.c | 34 ++++++++++++++++++++++++++++++++++
 message-validator.h |  6 ++++++
 5 files changed, 48 insertions(+), 35 deletions(-)
 create mode 100644 message-validator.c
 create mode 100644 message-validator.h

diff --git a/Makefile b/Makefile
index ffa6da71b..c1c26e434 100644
--- a/Makefile
+++ b/Makefile
@@ -783,6 +783,7 @@ LIB_OBJS += merge.o
 LIB_OBJS += merge-blobs.o
 LIB_OBJS += merge-recursive.o
 LIB_OBJS += mergesort.o
+LIB_OBJS += message-validator.o
 LIB_OBJS += mru.o
 LIB_OBJS += name-hash.o
 LIB_OBJS += notes.o
diff --git a/builtin/commit.c b/builtin/commit.c
index 8d1cac062..4c3112bb4 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -33,6 +33,7 @@
 #include "notes-utils.h"
 #include "mailmap.h"
 #include "sigchain.h"
+#include "message-validator.h"
 
 static const char * const builtin_commit_usage[] = {
 	N_("git commit [<options>] [--] <pathspec>..."),
@@ -979,41 +980,11 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 	return 1;
 }
 
-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-of-by's. */
-	for (i = start; i < sb->len; i++) {
-		nl = memchr(sb->buf + i, '\n', sb->len - i);
-		if (nl)
-			eol = nl - sb->buf;
-		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;
-	}
-
-	return 1;
-}
-
-/*
- * Find out if the message in the strbuf contains only whitespace and
- * Signed-off-by lines.
- */
-static int message_is_empty(struct strbuf *sb)
+static int is_empty(struct strbuf *sb)
 {
 	if (cleanup_mode == CLEANUP_NONE && sb->len)
 		return 0;
-	return rest_is_empty(sb, 0);
+	return message_is_empty(sb, 0);
 }
 
 /*
@@ -1035,7 +1006,7 @@ static int template_untouched(struct strbuf *sb)
 	if (!skip_prefix(sb->buf, tmpl.buf, &start))
 		start = sb->buf;
 	strbuf_release(&tmpl);
-	return rest_is_empty(sb, start - sb->buf);
+	return message_is_empty(sb, start - sb->buf);
 }
 
 static const char *find_author_by_nickname(const char *name)
@@ -1744,7 +1715,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 		fprintf(stderr, _("Aborting commit; you did not edit the message.\n"));
 		exit(1);
 	}
-	if (message_is_empty(&sb) && !allow_empty_message) {
+	if (is_empty(&sb) && !allow_empty_message) {
 		rollback_index_files();
 		fprintf(stderr, _("Aborting commit due to empty commit message.\n"));
 		exit(1);
diff --git a/builtin/merge.c b/builtin/merge.c
index 703827f00..625cfb848 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -31,6 +31,7 @@
 #include "gpg-interface.h"
 #include "sequencer.h"
 #include "string-list.h"
+#include "message-validator.h"
 
 #define DEFAULT_TWOHEAD (1<<0)
 #define DEFAULT_OCTOPUS (1<<1)
@@ -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))
 		abort_commit(remoteheads, _("Empty commit message."));
 	strbuf_release(&merge_msg);
 	strbuf_addbuf(&merge_msg, &msg);
diff --git a/message-validator.c b/message-validator.c
new file mode 100644
index 000000000..32feb4e26
--- /dev/null
+++ b/message-validator.c
@@ -0,0 +1,34 @@
+#include "git-compat-util.h"
+#include "sequencer.h"
+#include "strbuf.h"
+#include "message-validator.h"
+
+/*
+ * Find out if the message in the strbuf contains only whitespace and
+ * Signed-off-by lines.
+ */
+int message_is_empty(struct strbuf *sb, int start)
+{
+	int i, eol;
+	const char *nl;
+
+	/* Check if the rest is just whitespace and Signed-of-by's. */
+	for (i = start; i < sb->len; i++) {
+		nl = memchr(sb->buf + i, '\n', sb->len - i);
+		if (nl)
+			eol = nl - sb->buf;
+		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;
+	}
+
+	return 1;
+}
diff --git a/message-validator.h b/message-validator.h
new file mode 100644
index 000000000..4caea499c
--- /dev/null
+++ b/message-validator.h
@@ -0,0 +1,6 @@
+#ifndef MESSAGE_VALIDATOR_H
+#define MESSAGE_VALIDATOR_H
+
+extern int message_is_empty(struct strbuf *sb, int start);
+
+#endif
-- 
2.13.2.957.g457671ade

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

[Kaartic Sivaraam]

Sorry, forgot to add the RFC suffix to [PATCH]. Please consider it's
presence.

-- 
Kaartic

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

[Junio C Hamano]

Kaartic Sivaraam <kaarticsivaraam91196@gmail.com> 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 <kaarticsivaraam91196@gmail.com>
> ---
>  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.

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

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

[Kaartic Sivaraam]

On Tue, 2017-07-11 at 13:22 -0700, Junio C Hamano wrote:
> 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  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.
> 
It's little unfortunate that I haven't mentioned the reason I asked the
question that has resulted in this patch. It would explain a little
about why I thought this wasn't "meh" (I hope it stands for "who care
what ever").

Sometimes I abort an commit from from the editor by providing an empty
commit message. Then I came to know that 'git commit' considers commit
messages with just signed-off-by lines as an empty message. I tried to
take advantage of that. I once tried to abort a merge by just removing
the "Merge ..." line and leaving the "Signed-off" line and was
surprised to see the merge happen instead of an abort. The rest is
history. :)

-- 
Kaartic

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

[Junio C Hamano]

Kaartic Sivaraam <kaarticsivaraam91196@gmail.com> writes:

> Sometimes I abort an commit from from the editor by providing an empty
> commit message. Then I came to know that 'git commit' considers commit
> messages with just signed-off-by lines as an empty message. I tried to
> take advantage of that. I once tried to abort a merge by just removing
> the "Merge ..." line and leaving the "Signed-off" line and was
> surprised to see the merge happen instead of an abort. The rest is
> history. :)

I think many people know about and do use the "delete all lines"
(i.e. ":1,$d" in vi, or \M-< \C-SPC \M-> \C-w in Emacs) to abort out
of a commit or a merge.  I just do not think it is likely for them
to leave Sign-off lines and remove everything else, which is more
work than to delete everything, hence my reaction.

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

[Kaartic Sivaraam]

On Thu, 2017-07-13 at 10:58 -0700, Junio C Hamano wrote:
> I think many people know about and do use the "delete all lines"
> (i.e. ":1,$d" in vi, or \M-< \C-SPC \M-> \C-w in Emacs) to abort out
> of a commit or a merge.  I just do not think it is likely for them
> to leave Sign-off lines and remove everything else, which is more
> work than to delete everything, hence my reaction.
> 
Thanks! Didn't know this before.

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

[Christian Brabandt]

On Do, 13 Jul 2017, Junio C Hamano wrote:

> I think many people know about and do use the "delete all lines"
> (i.e. ":1,$d" in vi, or \M-< \C-SPC \M-> \C-w in Emacs) to abort out
> of a commit or a merge.  I just do not think it is likely for them
> to leave Sign-off lines and remove everything else, which is more
> work than to delete everything, hence my reaction.

In Vim you can also abort the commit message using :cq which exits the 
editor with an error code.

Best,
Christian
-- 
Das Werk soll den Meister loben.

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

[Junio C Hamano]

Christian Brabandt <cb@256bit.org> writes:

> On Do, 13 Jul 2017, Junio C Hamano wrote:
>
>> I think many people know about and do use the "delete all lines"
>> (i.e. ":1,$d" in vi, or \M-< \C-SPC \M-> \C-w in Emacs) to abort out
>> of a commit or a merge.  I just do not think it is likely for them
>> to leave Sign-off lines and remove everything else, which is more
>> work than to delete everything, hence my reaction.
>
> In Vim you can also abort the commit message using :cq which exits the 
> editor with an error code.

Sure, but it's not like we are trying to come up with an education
material to teach people how to abort their commit in progress in
this discussion, so I do not quite see a relevance of your comment
to the topic at hand here.

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

[Kaartic Sivaraam]

On Tue, 2017-07-11 at 13:22 -0700, Junio C Hamano wrote:
> 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.
> 
I have a few doubts for which I need clarification to move on with
this. 

    1. If we abort when the <message> part is empty wouldn't it be too
    restrictive ?

    IOW, Wouldn't it affect users of "git commit -‍-cleanup=verbatim"
    who wish to commit only the comments or parts of it ?
    (I'm not sure if someone would find that useful)

    2. Is it ok to use the "find_trailer_start" function of "trailer.c"
    to locate the trailer? 

    Note: It has a little issue that it wouldn't detect the trailer if
    the message comprises of one trailer alone and no other text. This
    case occurs while aborting a commit started using "git commit -s".
    Any possibilities to overcome the issue?

    3. Ignoring point 1 for now, What other helper methods except the
    ones listed below could be helpful in the separating the cleaned up
    commit message into the <message>, <trailer>, <junk-at-tail> ?

        * ignore_non_trailer
        * find_trailer_start

-- 
Kaartic

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

[Junio C Hamano]

Kaartic Sivaraam <kaarticsivaraam91196@gmail.com> writes:

> I have a few doubts for which I need clarification to move on with
> this.
>
>     1. If we abort when the <message> part is empty wouldn't it be too
>     restrictive ?
>
>     IOW, Wouldn't it affect users of "git commit -‍-cleanup=verbatim"
>     who wish to commit only the comments or parts of it ?
>     (I'm not sure if someone would find that useful)
>
>     2. Is it ok to use the "find_trailer_start" function of "trailer.c"
>     to locate the trailer? 
>
>     Note: It has a little issue that it wouldn't detect the trailer if
>     the message comprises of one trailer alone and no other text. This
>     case occurs while aborting a commit started using "git commit -s".
>     Any possibilities to overcome the issue?
>
>     3. Ignoring point 1 for now, What other helper methods except the
>     ones listed below could be helpful in the separating the cleaned up
>     commit message into the <message>, <trailer>, <junk-at-tail> ?
>
>         * ignore_non_trailer
>         * find_trailer_start

All good points; if it bothers you that "commit" and "merge" define
"emptyness" of the buffer differently too much, I think you could
persuade me to unify them to "the buffer _must_ contain no bytes",
i.e. not special-casing sign-off lines only "commit".

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

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

[Kaartic Sivaraam]

On Thu, 2017-07-13 at 12:23 -0700, Junio C Hamano wrote:
> All good points; if it bothers you that "commit" and "merge" define
> "emptyness" of the buffer differently too much, I think you could
> persuade me to unify them to "the buffer _must_ contain no bytes",
> i.e. not special-casing sign-off lines only "commit".
> 
Intereseting, let me give it a try.

To persuade you with this, I have to convince you that the current
behaviour (special-casing of sign-off lines) is defective and/or
biased. It really is for quite a few reasons,

            * Though it's not apparent, it indirectly seems to be hindering
            (to some extent) the idea of including the sign-off (or) other
            trailers which *can't be modified* by the user.

            IOW, the current behaviour seems make the contributors/users
            falsely believe (at least to some extent) that git *does* have
            trailers for commit messages and thus preventing them from coming
            up with ideas that could make "untouchable trailers" a reality.

            Thus, consider "the buffer _must_ contain no bytes" hoping this
            would initiate a "Butterfly effect" :)


        * Looking from an implementation perspective, it's biased in that
        it checks only for sign-offs. Making it work in general is
        difficult as there's no standard definition for the term
        <trailer>. That's because it varies with respect to usage, I
        think. Different people/projects may consider different lines to
        be trailer lines. A few examples are,

            * Bug:
            * Fixes:
            * Change-id:
            * Helped-by:

        Moreover, some people may wish to have commit messages that only
        have such trailers (e.g. "Fixes:"). So, it's difficult to do a
        generalized implementation that aborts when the message is empty
        or consists only of trailers.

        Thus, consider "the buffer _must_ contain no bytes" because it's
        not easy to define what a <trailer> means and special casing
        "sign-off" is biased.


        * Imagine a hypothetical version of git that aborts when the
        <message> is empty though a <trailer> is present. This would
        quite possibly instigate controversies as the "hypothetical git"
        reduces the "valid commit messages" and would quite possibly
        reject a commit message as "empty" (which is uncommunicative)
        though a previous version (which did not have this change)
        accepted a similar message.

        SO, bringing in the Occam's razor, let's choose the option that's
        the simplest and makes the fewest assumptions.


Thus, I conclude that the considering a commit message consisting only
of <trailer>s as empty isn't a good idea and should be dropped.


-- 
Kaartic

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

[Kaartic Sivaraam]

On Fri, 2017-07-14 at 23:19 +0530, Kaartic Sivaraam wrote:
>         * Imagine a hypothetical version of git that aborts when the
>         <message> is empty though a <trailer> is present. This would
>         quite possibly instigate controversies as the "hypothetical git"
>         reduces the "valid commit messages" and would quite possibly
>         reject a commit message as "empty" (which is uncommunicative)
>         though a previous version (which did not have this change)
>         accepted a similar message.
> 
>         SO, bringing in the Occam's razor, let's choose the option that's
>         the simplest and makes the fewest assumptions.
I would like to add a little to the "making fewer assumptions" point.
If we make the fewest assumptions possible, it has quite a few
advantages,

* It would make the implementation that checks for an empty message,
trivial. Thus reducing the complexity of the code.

* It would not overload the meaning of the error message,

    Aborting due to empty commit message.

Thus making the sentence stand for what it means "literally". 
(BTW, I guess an "an" is missing in the message)

* It allows for others to have more freedom in defining what a commit
message should have using the appropriate hook(s). IOW, let us do the
minimal check(message consisting only of whitespaces) and let the
others define what a commit message should have using the "commit-msg"
hook.

-- 
Kaartic

[PATCH v2] branch: change the error messages to be more meaningful

[Kaartic Sivaraam]

The error messages shown when the branch command is misused
by supplying it wrong number of parameters wasn't meaningful.
That's because it used the the phrase "too many branches"
assuming all parameters to be "valid" branch names. It's not
always the case as exemplified below,

        $ git branch
          foo
        * master

        $ git branch -m foo foo old
        fatal: too many branches for a rename operation

Change the messages to be more general thus making no assumptions
about the "parameters".

Signed-off-by: Kaartic Sivaraam <kaarticsivaraam91196@gmail.com>
---
 Changes in v2:

    - changed the wordings of the error message

 builtin/branch.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index a3bd2262b..62981d358 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -707,12 +707,12 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 		else if (argc == 2)
 			rename_branch(argv[0], argv[1], rename > 1);
 		else
-			die(_("too many branches for a rename operation"));
+			die(_("too many arguments for a rename operation"));
 	} else if (new_upstream) {
 		struct branch *branch = branch_get(argv[0]);
 
 		if (argc > 1)
-			die(_("too many branches to set new upstream"));
+			die(_("too many arguments to set new upstream"));
 
 		if (!branch) {
 			if (!argc || !strcmp(argv[0], "HEAD"))
@@ -735,7 +735,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 		struct strbuf buf = STRBUF_INIT;
 
 		if (argc > 1)
-			die(_("too many branches to unset upstream"));
+			die(_("too many arguments to unset upstream"));
 
 		if (!branch) {
 			if (!argc || !strcmp(argv[0], "HEAD"))
-- 
2.14.0.rc1.434.g6eded367a

Re: [PATCH v2] branch: change the error messages to be more meaningful

[Kaartic Sivaraam]

Sorry, wrong thread :( Please ignore this.

---
Kaartic

[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 <developer@example.org>

    # 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 <developer@example.org>

Signed-off-by: Kaartic Sivaraam <kaarticsivaraam91196@gmail.com>
---

 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

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

[Junio C Hamano]

Kaartic Sivaraam <kaarticsivaraam91196@gmail.com> 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.

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

[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

[PATCH] branch: change the error messages to be more meaningful

[Kaartic Sivaraam]

The error messages shown when the branch command is misused
by supplying it wrong number of parameters wasn't meaningful.
That's because it used the the phrase "too many branches"
assuming all parameters to be "valid" branch names. It's not
always the case as exemplified below,

        $ git branch
          foo
        * master

        $ git branch -m foo foo old
        fatal: too many branches for a rename operation

Change the messages to be more general thus making no assumptions
about the "parameters".

Signed-off-by: Kaartic Sivaraam <kaarticsivaraam91196@gmail.com>
---
 Sending this patch to the correct thread. I apologise if this
 seems reduntant. I'm having hard time keeping up with the patch-flow
 without 'send-email'!

 builtin/branch.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index a3bd2262b..59fedf085 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -707,12 +707,12 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 		else if (argc == 2)
 			rename_branch(argv[0], argv[1], rename > 1);
 		else
-			die(_("too many branches for a rename operation"));
+			die(_("too many parameters for a rename operation"));
 	} else if (new_upstream) {
 		struct branch *branch = branch_get(argv[0]);
 
 		if (argc > 1)
-			die(_("too many branches to set new upstream"));
+			die(_("too many parameters to set new upstream"));
 
 		if (!branch) {
 			if (!argc || !strcmp(argv[0], "HEAD"))
@@ -735,7 +735,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 		struct strbuf buf = STRBUF_INIT;
 
 		if (argc > 1)
-			die(_("too many branches to unset upstream"));
+			die(_("too many parameters to unset upstream"));
 
 		if (!branch) {
 			if (!argc || !strcmp(argv[0], "HEAD"))
-- 
2.14.0.rc1.434.g6eded367a

[PATCH v2] branch: change the error messages to be more meaningful

[Kaartic Sivaraam]

The error messages shown when the branch command is misused
by supplying it wrong number of parameters wasn't meaningful.
That's because it used the the phrase "too many branches"
assuming all parameters to be "valid" branch names. It's not
always the case as exemplified below,

        $ git branch
          foo
        * master

        $ git branch -m foo foo old
        fatal: too many branches for a rename operation

Change the messages to be more general thus making no assumptions
about the "parameters".

Signed-off-by: Kaartic Sivaraam <kaarticsivaraam91196@gmail.com>
---
 Changes in v2:

    - changed the wordings of the error message

 builtin/branch.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index a3bd2262b..62981d358 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -707,12 +707,12 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 		else if (argc == 2)
 			rename_branch(argv[0], argv[1], rename > 1);
 		else
-			die(_("too many branches for a rename operation"));
+			die(_("too many arguments for a rename operation"));
 	} else if (new_upstream) {
 		struct branch *branch = branch_get(argv[0]);
 
 		if (argc > 1)
-			die(_("too many branches to set new upstream"));
+			die(_("too many arguments to set new upstream"));
 
 		if (!branch) {
 			if (!argc || !strcmp(argv[0], "HEAD"))
@@ -735,7 +735,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 		struct strbuf buf = STRBUF_INIT;
 
 		if (argc > 1)
-			die(_("too many branches to unset upstream"));
+			die(_("too many arguments to unset upstream"));
 
 		if (!branch) {
 			if (!argc || !strcmp(argv[0], "HEAD"))
-- 
2.14.0.rc1.434.g6eded367a

Re: [PATCH v2] branch: change the error messages to be more meaningful

[Kaartic Sivaraam]

On Mon, 2017-08-21 at 19:06 +0530, Kaartic Sivaraam wrote:
> The error messages shown when the branch command is misused
> by supplying it wrong number of parameters wasn't meaningful.
> That's because it used the the phrase "too many branches"
> assuming all parameters to be "valid" branch names. It's not
> always the case as exemplified below,
> 
>         $ git branch
>           foo
>         * master
> 
>         $ git branch -m foo foo old
>         fatal: too many branches for a rename operation
> 
> Change the messages to be more general thus making no assumptions
> about the "parameters".
> 
> Signed-off-by: Kaartic Sivaraam <kaarticsivaraam91196@gmail.com>
> ---
>  Changes in v2:
> 
>     - changed the wordings of the error message
> 
>  builtin/branch.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/builtin/branch.c b/builtin/branch.c
> index a3bd2262b..62981d358 100644
> --- a/builtin/branch.c
> +++ b/builtin/branch.c
> @@ -707,12 +707,12 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
>  		else if (argc == 2)
>  			rename_branch(argv[0], argv[1], rename > 1);
>  		else
> -			die(_("too many branches for a rename operation"));
> +			die(_("too many arguments for a rename operation"));
>  	} else if (new_upstream) {
>  		struct branch *branch = branch_get(argv[0]);
>  
>  		if (argc > 1)
> -			die(_("too many branches to set new upstream"));
> +			die(_("too many arguments to set new upstream"));
>  
>  		if (!branch) {
>  			if (!argc || !strcmp(argv[0], "HEAD"))
> @@ -735,7 +735,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
>  		struct strbuf buf = STRBUF_INIT;
>  
>  		if (argc > 1)
> -			die(_("too many branches to unset upstream"));
> +			die(_("too many arguments to unset upstream"));
>  
>  		if (!branch) {
>  			if (!argc || !strcmp(argv[0], "HEAD"))


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] branch: change the error messages to be more meaningful

[Junio C Hamano]

Kaartic Sivaraam <kaarticsivaraam91196@gmail.com> writes:

> 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?

I do not even recall what the patches did and if I thought what they
wanted to do made sense, so I wouldn't be surprised if I did not
pick them up, after seeing nobody's commenting on it.

Re: [PATCH v2] branch: change the error messages to be more meaningful

[Kaartic Sivaraam]

On Tue, 2017-10-03 at 09:21 +0900, Junio C Hamano wrote:
> Kaartic Sivaraam <kaarticsivaraam91196@gmail.com> writes:
> 
> I do not even recall what the patches did and if I thought what they
> wanted to do made sense, 


I thought you did or may be I misinterpreted the following statement,


On Thursday 17 August 2017 12:58 AM, Junio C Hamano wrote:
> I do not find the s/branch/parameter/ too bad (although I would have
> said "arguments" instead).
>

I interpreted the "not .. too bad" as a "it makes little sense". So,
pinged the thread.

---
Kaartic

Re: [PATCH v2] branch: change the error messages to be more meaningful

[Junio C Hamano]

Kaartic Sivaraam <kaarticsivaraam91196@gmail.com> writes:

> I interpreted the "not .. too bad" as a "it makes little sense". So,
> pinged the thread.

Thanks.  I think what the patch does (sort of) makes sense.  

It is a bit dissapointing that we do not need to touch tests, as it
indicates that the logic to diagnose extra arguments as an error has
no coverage.

Re: [PATCH v2] branch: change the error messages to be more meaningful

[Kaartic Sivaraam]

On Wed, 2017-10-04 at 13:11 +0900, Junio C Hamano wrote:
> 
> It is a bit dissapointing that we do not need to touch tests, as it
> indicates that the logic to diagnose extra arguments as an error has
> no coverage.

Even if there were tests I don't think they would have needed any
updation as most of the tests (at least those that I came across) that
check for failure seem not to be checking for what error message gets
printed. They seem to test only if the command fails (using
test_must_fail in most cases, I guess).

Moreover, as a consequence of my assumption that the tests don't check
for the error messages themselves; I haven't even thought of checking
whether the tests or the travis-ci build succeeded as a consequence of
my patches that touch "only" the error messages!

---
Kaartic

Re: [PATCH v2] branch: change the error messages to be more meaningful

[Junio C Hamano]

Kaartic Sivaraam <kaarticsivaraam91196@gmail.com> writes:

> Moreover, as a consequence of my assumption that the tests don't check
> for the error messages themselves; I haven't even thought of checking
> whether the tests or the travis-ci build succeeded as a consequence of
> my patches that touch "only" the error messages!

That's a bad thing, right?

Re: [PATCH v2] branch: change the error messages to be more meaningful

[Kaartic Sivaraam]

On Thu, 2017-10-05 at 10:13 +0900, Junio C Hamano wrote:
> Kaartic Sivaraam <kaarticsivaraam91196@gmail.com> writes:
> 
> > Moreover, as a consequence of my assumption that the tests don't check
> > for the error messages themselves; I haven't even thought of checking
> > whether the tests or the travis-ci build succeeded as a consequence of
> > my patches that touch "only" the error messages!
> 
> That's a bad thing, right?

Might be, but my assumption seems to held at least until now for the
two or three patches that I have sent that touch "only" the error
messages (I haven't received emails from anyone shouting at me that the
build fails as a consequence of one such patches).

The build status for this patch can be found at [1]. Supporting my
assumption, it passed too.

That said, you don't have to worry a lot about this. I'll come over my
laziness and check if the travis-ci build passes for such patches in
the future :-)

[1]: https://travis-ci.org/sivaraam/git/builds/276222235

---
Kaartic