On Mon, Jan 21, 2013 at 11:54 PM, Jonathan Nieder <jrnieder@xxxxxxxxx> wrote:
> 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.
> 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.
>> - 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.
>> -
>> - 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
--
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