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

From: Brandon Casey <drafnel@gmail.com>
Date: Feb 22, 2013, 22:03 UTC
Message-ID: <CA+sFfMdok7wRDhgq7i=b3cu3LB+poExvxLBxYkg8L3pN92bEYg@mail.gmail.com>
In-Reply-To: <7vbobcdwo7.fsf@alter.siamese.dyndns.org>

On Fri, Feb 22, 2013 at 10:35 AM, Junio C Hamano <gitster@pobox.com> wrote:

> Brandon Casey <drafnel@gmail.com> writes:

Show 36 quoted lines

>> 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.

We could unroll the conditionals into individual blocks and add your comments from above like:

   if (!len) {
      /* The buffer is completely empty.  Leave room for the title and body. */
      append_newlines = "\n\n";
   } else if (msgbuf->buf[len - 1] != '\n') {
      /* Incomplete line.  Complete the line and add a blank one */
      append_newlines = "\n\n";
   } else if (len == 1) {
      /*
       * Buffer contains a single newline.  Add another so that we leave
       * room for the title and body.
       */
      append_newlines = "\n";
   } ...

Not sure that it will reduce the amount of time needed to understand what's going on, but at least it describes the expectations made by each block.

Show 11 quoted lines

> 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.

Yeah, Jonathan said the same thing and I agree. I was hoping someone else would beat me to it.

-Brandon