On Thu, Feb 14, 2013 at 9:58 AM, John Keeping <[email protected]> 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 <[email protected]>
>> ---
>
> 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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
