Re: [PATCH v4.1 09/12] sequencer.c: teach append_signoff to avoid adding a duplicate newline

On Fri, Feb 15, 2013 at 10:58:38AM -0800, Brandon Casey wrote:

On Thu, Feb 14, 2013 at 9:58 AM, John Keeping [off-list ref] wrote:

quoted

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.

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 talking about plain 'commit -s' which seems to use the same code
path.

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.

Two blank lines seems like an improvement to me, FWIW.


John