Re: [PATCH v2 01/10] sequencer.c: remove broken support for rfc2822 continuation in footer

From: Brandon Casey <drafnel@gmail.com>
Date: Jan 22, 2013, 09:35 UTC
Message-ID: <CA+sFfMfERgGbf58LjOfAhhO_-YLu+yo+L-vYMuAuOUaP90yvgA@mail.gmail.com>
In-Reply-To: <20130122075413.GB6085@elie.Belkin>

On Mon, Jan 21, 2013 at 11:54 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:

Show 16 quoted lines

> Hi,
>
> Brandon Casey wrote:
>
>> --- a/sequencer.c
>> +++ b/sequencer.c
> [...]
>> @@ -1042,13 +1041,8 @@ static int ends_rfc2822_footer(struct strbuf *sb, int ignore_footer)
>
> Git is checking if (sb->buf) ends with a "Signed-off-by:" style
> line.  If it doesn't, it will need to add an extra blank line
> before adding a new sign-off.
>
> First (snipped), it seeks back two newlines from the end and then
> forward to the next non-newline character, so (buf + i) is at the
> start of the last line of (the interesting part of) sb.

Did you catch that the two newlines have to be adjacent to each other?
 i.e. it seeks back until it finds a blank line.  Then it seeks
forward so that buf + i points at the first line of the last paragraph
of sb.

Show 8 quoted lines

> Now:
>
>>       for (; i < len; i = k) {
>>               for (k = i; k < len && buf[k] != '\n'; k++)
>>                       ; /* do nothing */
>>               k++;
>
> (buf + k) points to the end of this line.

buf + k points to the start of the next line. Not sure if that is the same thing as what you said.

Show 5 quoted lines

>> -             if ((buf[k] == ' ' || buf[k] == '\t') && !first)
>> -                     continue;
>
> This is always the first line examined, so this "continue" never
> triggers.

This is just totally broken and always has been. The index variable should be 'i' not 'k'. It is supposed to check whether the current line is a continuation of the previously inspected line. An rfc2822 continuation line begins with space or tab. The first line of the paragraph obviously can't be a continuation line, so the /first/ variable is used as a guard against matching the first line.

Show 9 quoted lines

>> -
>> -             first = 0;
>> -
>>               for (j = 0; i + j < len; j++) {
>
> If the line matches /^[[:alnum:]-]*:/, it passes and git moves on to
> the (nonexistent) next line.  Otherwise, it fails.
>
> Do I understand correctly?

No, I think you misread the for loop that you snipped out. It seeks back to the beginning of the last paragraph, not the last line. The for loop that you included above, inspects each line of that paragraph. If any line does not match /^[[:alnum:]-]*:/, then the function returns false (0). If every line matches, it returns true (1).

> If so, this patch should be a no-op, which
> is good, I guess.

You're correct here though. The patch is a no-op. This patch removes the code that was supposed to parse rfc2822 continuation lines, but it was broken. Thankfully it was broken utterly and completely so it never allowed anything through that wasn't /^[[:alnum:]-]*:/.

-Brandon