Re: [PATCH] git-commit: populate the edit buffer with 2 blank lines before s-o-b

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 22, 2013, 18:35 UTC
Message-ID: <7vbobcdwo7.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1361525158-3648-1-git-send-email-drafnel@gmail.com>

Brandon Casey <drafnel@gmail.com> writes:

Show 37 quoted lines

> Before commit 33f2f9ab, 'commit -s' would populate the edit buffer with
> a blank line before the Signed-off-by line.  This provided a nice
> hint to the user that something should be filled in.  Let's restore that
> behavior, but now let's ensure that the Signed-off-by line is preceded
> by two blank lines to hint that something should be filled in, and that
> a blank line should separate it from the Signed-off-by line.
>
> Plus, add a test for this behavior.
>
> Reported-by: John Keeping <john@keeping.me.uk>
> Signed-off-by: Brandon Casey <drafnel@gmail.com>
> ---
>
> Ok.  Here's a patch on top of 959a2623 bc/append-signed-off-by.  It
> implements the "2 blank lines preceding sob" behavior.
>
> -Brandon
>
>  sequencer.c       |  5 +++--
>  t/t7502-commit.sh | 12 ++++++++++++
>  2 files changed, 15 insertions(+), 2 deletions(-)
>
> diff --git a/sequencer.c b/sequencer.c
> index 53ee49a..2dac106 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -1127,9 +1127,10 @@ void append_signoff(struct strbuf *msgbuf, int ignore_footer, unsigned flag)
>  		const char *append_newlines = NULL;
>  		size_t len = msgbuf->len - ignore_footer;
>  
> -		if (len && msgbuf->buf[len - 1] != '\n')
> +		/* ensure a blank line precedes our signoff */
> +		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";

Maybe I am getting slower with age, but it took me 5 minutes of staring the above to convince me that it is doing the right thing. The if/elseif cascade is dealing with three separate things and the logic is a bit dense:

 * Is the buffer completely empty?  We need to add two LFs to give a
   room for the title and body;

 * Otherwise:

   - Is the final line incomplete?  We need to add one LF to make it a
     complete line whatever we do.

   - Is the final line an empty line?  We need to add one more LF to
     make sure we have a blank line before we add S-o-b.

I wondered if we can rewrite it to make the logic clearer (that is where I spent most of the 5 minutes), but I did not think of a better way; probably the above is the best we could do.

Thanks.

By the way, I think we would want to introduce a symbolic constants for the possible return values from has_conforming_footer(). The check that appears after this hunk

	if (has_footer != 3 && (!no_dup_sob || has_footer != 2))
		strbuf_splice(msgbuf, msgbuf->len - ignore_footer, 0,
				sob.buf, sob.len);

is hard to grok without them.