On Fri, Feb 22, 2013 at 10:35 AM, Junio C Hamano <gits...@pobox.com> wrote:
> Brandon Casey <draf...@gmail.com> writes:

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

> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to