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

[John Keeping <john@keeping.me.uk>]

On Fri, Feb 15, 2013 at 10:58:38AM -0800, Brandon Casey wrote:
> On Thu, Feb 14, 2013 at 9:58 AM, John Keeping <john@keeping.me.uk> wrote:
> > 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