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

On Mon, Jan 21, 2013 at 11:54 PM, Jonathan Nieder [off-list ref] wrote:

Hi,

Brandon Casey wrote:

quoted

--- a/sequencer.c
+++ b/sequencer.c

[...]

quoted

@@ -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:

quoted

      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.

quoted

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

quoted

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