* Why doesn't merge fail if message has only sign-off? @ 2017-07-02 12:03 Kaartic Sivaraam 2017-07-03 17:21 ` Junio C Hamano 0 siblings, 1 reply; 24+ messages in thread From: Kaartic Sivaraam @ 2017-07-02 12:03 UTC (permalink / raw) To: git 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 ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: Why doesn't merge fail if message has only sign-off? 2017-07-02 12:03 Why doesn't merge fail if message has only sign-off? Kaartic Sivaraam @ 2017-07-03 17:21 ` Junio C Hamano 2017-07-04 20:03 ` Kaartic Sivaraam 2017-07-06 3:31 ` [PATCH] merge-message: change meaning of "empty merge message" Kaartic Sivaraam 0 siblings, 2 replies; 24+ messages in thread From: Junio C Hamano @ 2017-07-03 17:21 UTC (permalink / raw) To: Kaartic Sivaraam; +Cc: git 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?) ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: Why doesn't merge fail if message has only sign-off? 2017-07-03 17:21 ` Junio C Hamano @ 2017-07-04 20:03 ` Kaartic Sivaraam 2017-07-06 3:31 ` [PATCH] merge-message: change meaning of "empty merge message" Kaartic Sivaraam 1 sibling, 0 replies; 24+ messages in thread From: Kaartic Sivaraam @ 2017-07-04 20:03 UTC (permalink / raw) To: Junio C Hamano; +Cc: git 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 ^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH] merge-message: change meaning of "empty merge message" 2017-07-03 17:21 ` Junio C Hamano 2017-07-04 20:03 ` Kaartic Sivaraam @ 2017-07-06 3:31 ` Kaartic Sivaraam 2017-07-06 4:46 ` Kevin Daudt 1 sibling, 1 reply; 24+ messages in thread From: Kaartic Sivaraam @ 2017-07-06 3:31 UTC (permalink / raw) To: gitster; +Cc: git 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 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH] merge-message: change meaning of "empty merge message" 2017-07-06 3:31 ` [PATCH] merge-message: change meaning of "empty merge message" Kaartic Sivaraam @ 2017-07-06 4:46 ` Kevin Daudt 2017-07-06 12:20 ` Kaartic Sivaraam 2017-07-11 14:12 ` [PATCH] commit & merge: modularize the empty message validator Kaartic Sivaraam 0 siblings, 2 replies; 24+ messages in thread From: Kevin Daudt @ 2017-07-06 4:46 UTC (permalink / raw) To: Kaartic Sivaraam; +Cc: gitster, git 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 ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] merge-message: change meaning of "empty merge message" 2017-07-06 4:46 ` Kevin Daudt @ 2017-07-06 12:20 ` Kaartic Sivaraam 2017-07-11 14:12 ` [PATCH] commit & merge: modularize the empty message validator Kaartic Sivaraam 1 sibling, 0 replies; 24+ messages in thread From: Kaartic Sivaraam @ 2017-07-06 12:20 UTC (permalink / raw) To: Kevin Daudt; +Cc: gitster, git 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 ^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH] commit & merge: modularize the empty message validator 2017-07-06 4:46 ` Kevin Daudt 2017-07-06 12:20 ` Kaartic Sivaraam @ 2017-07-11 14:12 ` Kaartic Sivaraam 2017-07-11 14:41 ` [PATCH/RFC] " Kaartic Sivaraam 2017-07-11 20:22 ` [PATCH] " Junio C Hamano 1 sibling, 2 replies; 24+ messages in thread From: Kaartic Sivaraam @ 2017-07-11 14:12 UTC (permalink / raw) To: gitster; +Cc: git, me 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 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH/RFC] commit & merge: modularize the empty message validator 2017-07-11 14:12 ` [PATCH] commit & merge: modularize the empty message validator Kaartic Sivaraam @ 2017-07-11 14:41 ` Kaartic Sivaraam 2017-07-11 20:22 ` [PATCH] " Junio C Hamano 1 sibling, 0 replies; 24+ messages in thread From: Kaartic Sivaraam @ 2017-07-11 14:41 UTC (permalink / raw) To: gitster; +Cc: git, me Sorry, forgot to add the RFC suffix to [PATCH]. Please consider it's presence. -- Kaartic ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] commit & merge: modularize the empty message validator 2017-07-11 14:12 ` [PATCH] commit & merge: modularize the empty message validator Kaartic Sivaraam 2017-07-11 14:41 ` [PATCH/RFC] " Kaartic Sivaraam @ 2017-07-11 20:22 ` Junio C Hamano 2017-07-13 13:00 ` Kaartic Sivaraam 2017-07-13 18:15 ` Kaartic Sivaraam 1 sibling, 2 replies; 24+ messages in thread From: Junio C Hamano @ 2017-07-11 20:22 UTC (permalink / raw) To: Kaartic Sivaraam; +Cc: git, me 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. ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] commit & merge: modularize the empty message validator 2017-07-11 20:22 ` [PATCH] " Junio C Hamano @ 2017-07-13 13:00 ` Kaartic Sivaraam 2017-07-13 17:58 ` Junio C Hamano 2017-07-13 18:15 ` Kaartic Sivaraam 1 sibling, 1 reply; 24+ messages in thread From: Kaartic Sivaraam @ 2017-07-13 13:00 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, me 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 ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] commit & merge: modularize the empty message validator 2017-07-13 13:00 ` Kaartic Sivaraam @ 2017-07-13 17:58 ` Junio C Hamano 2017-07-14 13:31 ` Kaartic Sivaraam 2017-07-17 9:08 ` Christian Brabandt 0 siblings, 2 replies; 24+ messages in thread From: Junio C Hamano @ 2017-07-13 17:58 UTC (permalink / raw) To: Kaartic Sivaraam; +Cc: git, me 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. ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] commit & merge: modularize the empty message validator 2017-07-13 17:58 ` Junio C Hamano @ 2017-07-14 13:31 ` Kaartic Sivaraam 2017-07-17 9:08 ` Christian Brabandt 1 sibling, 0 replies; 24+ messages in thread From: Kaartic Sivaraam @ 2017-07-14 13:31 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, me 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. ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] commit & merge: modularize the empty message validator 2017-07-13 17:58 ` Junio C Hamano 2017-07-14 13:31 ` Kaartic Sivaraam @ 2017-07-17 9:08 ` Christian Brabandt 2017-07-17 17:16 ` Junio C Hamano 1 sibling, 1 reply; 24+ messages in thread From: Christian Brabandt @ 2017-07-17 9:08 UTC (permalink / raw) To: git 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. ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] commit & merge: modularize the empty message validator 2017-07-17 9:08 ` Christian Brabandt @ 2017-07-17 17:16 ` Junio C Hamano 0 siblings, 0 replies; 24+ messages in thread From: Junio C Hamano @ 2017-07-17 17:16 UTC (permalink / raw) To: Christian Brabandt; +Cc: git 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. ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] commit & merge: modularize the empty message validator 2017-07-11 20:22 ` [PATCH] " Junio C Hamano 2017-07-13 13:00 ` Kaartic Sivaraam @ 2017-07-13 18:15 ` Kaartic Sivaraam 2017-07-13 19:23 ` Junio C Hamano 1 sibling, 1 reply; 24+ messages in thread From: Kaartic Sivaraam @ 2017-07-13 18:15 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, me 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 ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] commit & merge: modularize the empty message validator 2017-07-13 18:15 ` Kaartic Sivaraam @ 2017-07-13 19:23 ` Junio C Hamano 2017-07-14 17:49 ` Kaartic Sivaraam 0 siblings, 1 reply; 24+ messages in thread From: Junio C Hamano @ 2017-07-13 19:23 UTC (permalink / raw) To: Kaartic Sivaraam; +Cc: git, me 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. ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] commit & merge: modularize the empty message validator 2017-07-13 19:23 ` Junio C Hamano @ 2017-07-14 17:49 ` Kaartic Sivaraam 2017-07-15 8:33 ` Kaartic Sivaraam 0 siblings, 1 reply; 24+ messages in thread From: Kaartic Sivaraam @ 2017-07-14 17:49 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, me 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 ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] commit & merge: modularize the empty message validator 2017-07-14 17:49 ` Kaartic Sivaraam @ 2017-07-15 8:33 ` Kaartic Sivaraam 2017-08-21 13:34 ` [PATCH v2] branch: change the error messages to be more meaningful Kaartic Sivaraam 2017-08-21 14:05 ` [PATCH v2/RFC] commit: change the meaning of an empty commit message Kaartic Sivaraam 0 siblings, 2 replies; 24+ messages in thread From: Kaartic Sivaraam @ 2017-07-15 8:33 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, me 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 ^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v2] branch: change the error messages to be more meaningful 2017-07-15 8:33 ` Kaartic Sivaraam @ 2017-08-21 13:34 ` Kaartic Sivaraam 2017-08-21 13:52 ` Kaartic Sivaraam 2017-08-21 14:05 ` [PATCH v2/RFC] commit: change the meaning of an empty commit message Kaartic Sivaraam 1 sibling, 1 reply; 24+ messages in thread From: Kaartic Sivaraam @ 2017-08-21 13:34 UTC (permalink / raw) To: gitster; +Cc: git 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 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH v2] branch: change the error messages to be more meaningful 2017-08-21 13:34 ` [PATCH v2] branch: change the error messages to be more meaningful Kaartic Sivaraam @ 2017-08-21 13:52 ` Kaartic Sivaraam 0 siblings, 0 replies; 24+ messages in thread From: Kaartic Sivaraam @ 2017-08-21 13:52 UTC (permalink / raw) To: gitster; +Cc: git Sorry, wrong thread :( Please ignore this. --- Kaartic ^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v2/RFC] commit: change the meaning of an empty commit message 2017-07-15 8:33 ` Kaartic Sivaraam 2017-08-21 13:34 ` [PATCH v2] branch: change the error messages to be more meaningful Kaartic Sivaraam @ 2017-08-21 14:05 ` Kaartic Sivaraam 2017-08-24 20:19 ` Junio C Hamano 1 sibling, 1 reply; 24+ messages in thread From: Kaartic Sivaraam @ 2017-08-21 14:05 UTC (permalink / raw) To: gitster; +Cc: git 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 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH v2/RFC] commit: change the meaning of an empty commit message 2017-08-21 14:05 ` [PATCH v2/RFC] commit: change the meaning of an empty commit message Kaartic Sivaraam @ 2017-08-24 20:19 ` Junio C Hamano 2017-08-31 13:36 ` Kaartic Sivaraam 0 siblings, 1 reply; 24+ messages in thread From: Junio C Hamano @ 2017-08-24 20:19 UTC (permalink / raw) To: Kaartic Sivaraam; +Cc: git 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. ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2/RFC] commit: change the meaning of an empty commit message 2017-08-24 20:19 ` Junio C Hamano @ 2017-08-31 13:36 ` Kaartic Sivaraam 2017-10-02 17:20 ` Kaartic Sivaraam 0 siblings, 1 reply; 24+ messages in thread From: Kaartic Sivaraam @ 2017-08-31 13:36 UTC (permalink / raw) To: Junio C Hamano; +Cc: git 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 ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2/RFC] commit: change the meaning of an empty commit message 2017-08-31 13:36 ` Kaartic Sivaraam @ 2017-10-02 17:20 ` Kaartic Sivaraam 0 siblings, 0 replies; 24+ messages in thread From: Kaartic Sivaraam @ 2017-10-02 17:20 UTC (permalink / raw) To: Junio C Hamano; +Cc: git 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 ^ permalink raw reply [flat|nested] 24+ messages in thread
end of thread, other threads:[~2017-10-02 17:20 UTC | newest] Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-07-02 12:03 Why doesn't merge fail if message has only sign-off? Kaartic Sivaraam 2017-07-03 17:21 ` Junio C Hamano 2017-07-04 20:03 ` Kaartic Sivaraam 2017-07-06 3:31 ` [PATCH] merge-message: change meaning of "empty merge message" Kaartic Sivaraam 2017-07-06 4:46 ` Kevin Daudt 2017-07-06 12:20 ` Kaartic Sivaraam 2017-07-11 14:12 ` [PATCH] commit & merge: modularize the empty message validator Kaartic Sivaraam 2017-07-11 14:41 ` [PATCH/RFC] " Kaartic Sivaraam 2017-07-11 20:22 ` [PATCH] " Junio C Hamano 2017-07-13 13:00 ` Kaartic Sivaraam 2017-07-13 17:58 ` Junio C Hamano 2017-07-14 13:31 ` Kaartic Sivaraam 2017-07-17 9:08 ` Christian Brabandt 2017-07-17 17:16 ` Junio C Hamano 2017-07-13 18:15 ` Kaartic Sivaraam 2017-07-13 19:23 ` Junio C Hamano 2017-07-14 17:49 ` Kaartic Sivaraam 2017-07-15 8:33 ` Kaartic Sivaraam 2017-08-21 13:34 ` [PATCH v2] branch: change the error messages to be more meaningful Kaartic Sivaraam 2017-08-21 13:52 ` Kaartic Sivaraam 2017-08-21 14:05 ` [PATCH v2/RFC] commit: change the meaning of an empty commit message Kaartic Sivaraam 2017-08-24 20:19 ` Junio C Hamano 2017-08-31 13:36 ` Kaartic Sivaraam 2017-10-02 17:20 ` Kaartic Sivaraam
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).