On Fri, Feb 22, 2013 at 10:35 AM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> Brandon Casey <drafnel@xxxxxxxxx> 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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html