On Thu, Feb 14, 2013 at 9:58 AM, John Keeping <john@xxxxxxxxxxxxx> wrote: > On Tue, Feb 12, 2013 at 02:33:42AM -0800, Brandon Casey wrote: >> Teach append_signoff to detect whether a blank line exists at the position >> that the signed-off-by line will be added, and refrain from adding an >> additional one if one already exists. Or, add an additional line if one >> is needed to make sure the new footer is separated from the message body >> by a blank line. >> >> Signed-off-by: Brandon Casey <bcasey@xxxxxxxxxx> >> --- > > As Jonathan Nieder wondered before [1], this changes the behaviour when > the commit message is empty. Before this commit, there is an empty line > followed by the S-O-B line; now the S-O-B is on the first line of the > commit. > > The previous behaviour seems better to me since the empty line is > hinting that the user should fill something in. It looks particularly > strange if your editor has syntax highlighting for commit messages such > that the first line is in a different colour. > > [1] http://article.gmane.org/gmane.comp.version-control.git/214796 > >> diff --git a/sequencer.c b/sequencer.c >> index 3364faa..084573b 100644 >> --- a/sequencer.c >> +++ b/sequencer.c >> @@ -1127,8 +1127,19 @@ void append_signoff(struct strbuf *msgbuf, int ignore_footer, unsigned flag) >> else >> has_footer = has_conforming_footer(msgbuf, &sob, ignore_footer); >> >> - if (!has_footer) >> - strbuf_splice(msgbuf, msgbuf->len - ignore_footer, 0, "\n", 1); >> + if (!has_footer) { >> + const char *append_newlines = NULL; >> + size_t len = msgbuf->len - ignore_footer; >> + >> + if (len && msgbuf->buf[len - 1] != '\n') >> + append_newlines = "\n\n"; >> + else if (len > 1 && msgbuf->buf[len - 2] != '\n') >> + append_newlines = "\n"; > > To restore the old behaviour this needs something like this: > > else if (!len) > append_newlines = "\n"; > >> + if (append_newlines) >> + strbuf_splice(msgbuf, msgbuf->len - ignore_footer, 0, >> + append_newlines, strlen(append_newlines)); >> + } >> >> if (has_footer != 3 && (!no_dup_sob || has_footer != 2)) >> strbuf_splice(msgbuf, msgbuf->len - ignore_footer, 0, Are you talking about the output produced by format-patch? Or are you talking about what happens when you do 'commit --amend -s' for a commit with an empty commit message. (The email that you referenced was about the behavior of format-patch). I'm thinking you must be talking about the 'commit --amend -s' behavior since you mentioned your editor. Is there another case that is affected by this? Normally, any extra blank lines that precede or follow a commit message are removed before the commit object is created. So, I guess it wouldn't hurt to insert a newline (or maybe it should be two?) before the signoff in this case. Would this provide an improvement or change for any other commands than 'commit --amend -s'? If we want to do this, then I'd probably do it like this: - if (len && msgbuf->buf[len - 1] != '\n') + if (!len || msgbuf->buf[len - 1] != '\n') append_newlines = "\n\n"; - else if (len > 1 && msgbuf->buf[len - 2] != '\n') + else if (len == 1 || msgbuf->buf[len - 2] != '\n') append_newlines = "\n"; This would ensure there were two newlines preceding the sob. The editor would place its cursor on the top line where the user should begin typing in a commit message. If an editor was not opened up (e.g. if 'git cherry-pick -s --allow-empty-message ...' was used) then the normal mechanism that removes extra blank lines would trigger to remove the extra blank lines. I think that's reasonable. It seems 'git cherry-pick -s --edit' follows a different code path, and the commit message is stripped of newlines by 'git commit' before it is passed to the editor. 'cherry-pick -s --edit' and 'commit --amend -s' should probably have the same behavior and present the same buffer to the user for editing when they encounter a commit with an empty message. Maybe something like this is enough(?): diff --git a/builtin/commit.c b/builtin/commit.c index 7b9e2ac..0796412 100644 --- a/builtin/commit.c +++ b/builtin/commit.c @@ -124,8 +124,10 @@ static int opt_parse_m(const struct option *opt, const char if (unset) strbuf_setlen(buf, 0); else { + if (buf->len) + strbuf_addch(buf, '\n'); strbuf_addstr(buf, arg); - strbuf_addstr(buf, "\n\n"); + strbuf_complete_line(buf); } return 0; } @@ -673,9 +675,6 @@ static int prepare_to_commit(const char *index_file, const c if (s->fp == NULL) die_errno(_("could not open '%s'"), git_path(commit_editmsg)); - if (clean_message_contents) - stripspace(&sb, 0); - if (signoff) { /* * See if we have a Conflicts: block at the end. If yes, count @@ -703,6 +702,9 @@ static int prepare_to_commit(const char *index_file, const c append_signoff(&sb, ignore_footer, 0); } + if (clean_message_contents) + stripspace(&sb, 0); + if (fwrite(sb.buf, 1, sb.len, s->fp) < sb.len) die_errno(_("could not write commit template")); I suspect we have a broken test at t7502.15 though that happened to work because opt_parse_m() was appending two newlines that the test expected to be there. -Brandon -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html