Kaartic SivaraamWhile 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
threads / discuss / 46304
Why doesn't merge fail if message has only sign-off?
Subject: Why doesn't merge fail if message has only sign-off?
Kaartic Sivaraam
Junio C HamanoRe: Why doesn't merge fail if message has only sign-off?
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?)
Kaartic SivaraamRe: Why doesn't merge fail if message has only sign-off?
On Mon, 2017-07-03 at 10:21 -0700, Junio C Hamano wrote:
Show 9 quoted lines
> 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
Kaartic Sivaraam[PATCH] merge-message: change meaning of "empty merge message"
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
Kevin DaudtRe: [PATCH] merge-message: change meaning of "empty merge message"
On Thu, Jul 06, 2017 at 09:01:49AM +0530, Kaartic Sivaraam wrote:
Show 29 quoted lines
> 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
Kaartic SivaraamRe: [PATCH] merge-message: change meaning of "empty merge message"
On Thu, 2017-07-06 at 06:46 +0200, Kevin Daudt wrote:
> > The function is called "rest_is_empty". >
Thanks for correcting that!
Show 6 quoted lines
> 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
Kaartic Sivaraam[PATCH] commit & merge: modularize the empty message validator
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
Kaartic SivaraamRe: [PATCH/RFC] commit & merge: modularize the empty message validator
Sorry, forgot to add the RFC suffix to [PATCH]. Please consider it's presence.
-- Kaartic
Junio C HamanoRe: [PATCH] commit & merge: modularize the empty message validator
Kaartic Sivaraam <kaarticsivaraam91196@gmail.com> writes:
Show 19 quoted lines
> 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.
Show 6 quoted lines
> @@ -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.
Kaartic SivaraamRe: [PATCH] commit & merge: modularize the empty message validator
On Tue, 2017-07-11 at 13:22 -0700, Junio C Hamano wrote:
Show 10 quoted lines
> 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
Junio C HamanoRe: [PATCH] commit & merge: modularize the empty message validator
Kaartic Sivaraam <kaarticsivaraam91196@gmail.com> writes:
Show 7 quoted lines
> 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.
Kaartic SivaraamRe: [PATCH] commit & merge: modularize the empty message validator
On Thu, 2017-07-13 at 10:58 -0700, Junio C Hamano wrote:
Show 6 quoted lines
> 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.
Christian BrabandtRe: [PATCH] commit & merge: modularize the empty message validator
On Do, 13 Jul 2017, Junio C Hamano wrote:
Show 5 quoted lines
> 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.
Junio C HamanoRe: [PATCH] commit & merge: modularize the empty message validator
Christian Brabandt <cb@256bit.org> writes:
Show 10 quoted lines
> 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.
Kaartic SivaraamRe: [PATCH] commit & merge: modularize the empty message validator
On Tue, 2017-07-11 at 13:22 -0700, Junio C Hamano wrote:
Show 12 quoted lines
> 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
Junio C HamanoRe: [PATCH] commit & merge: modularize the empty message validator
Kaartic Sivaraam <kaarticsivaraam91196@gmail.com> writes:
Show 24 quoted lines
> 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.
Kaartic SivaraamRe: [PATCH] commit & merge: modularize the empty message validator
On Thu, 2017-07-13 at 12:23 -0700, Junio C Hamano wrote:
Show 5 quoted lines
> 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
Kaartic SivaraamRe: [PATCH] commit & merge: modularize the empty message validator
On Fri, 2017-07-14 at 23:19 +0530, Kaartic Sivaraam wrote:
Show 10 quoted lines
> * 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
Kaartic Sivaraam[PATCH v2] branch: change the error messages to be more meaningful
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 operationChange 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
Kaartic SivaraamRe: [PATCH v2] branch: change the error messages to be more meaningful
Sorry, wrong thread :( Please ignore this.
--- Kaartic
Kaartic Sivaraam[PATCH v2/RFC] commit: change the meaning of an empty commit message
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' linesThis 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
Junio C HamanoRe: [PATCH v2/RFC] commit: change the meaning of an empty commit message
Kaartic Sivaraam <kaarticsivaraam91196@gmail.com> writes:
Show 7 quoted lines
> 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.
Kaartic SivaraamRe: [PATCH v2/RFC] commit: change the meaning of an empty commit message
On Thu, 2017-08-24 at 13:19 -0700, Junio C Hamano wrote:
Show 5 quoted lines
> > 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
Kaartic SivaraamRe: [PATCH v2/RFC] commit: change the meaning of an empty commit message
On Thu, 2017-08-31 at 19:06 +0530, Kaartic Sivaraam wrote:
Show 17 quoted lines
> 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