Re: [PATCH v3 05/11] sequencer.c: recognize "(cherry picked from ..." as part of s-o-b footer

[Brandon Casey <drafnel@gmail.com>]

On Sun, Jan 27, 2013 at 6:51 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Jonathan Nieder wrote:
>
>> Here's the tweak I suggested last time.  I think its behavior is
>> slightly better in the "ends with incomplete line" case because it
>> limits the characters examined by is_rfc2822_line() and
>> is_cherry_picked_from_line() not to include buf[len] (which would
>> presumably sometimes be '\0').
>
> Whoops, that revealed a subtlety --- the '\n' or '\0' is what prevents
> exiting the loop in is_rfc2822_line when the line does not contain a
> colon.  Here's a corrected version of the tweak, that should actually
> work. :)
>
> diff --git i/sequencer.c w/sequencer.c
> index 0b5cd18c..108ea27b 100644
> --- i/sequencer.c
> +++ w/sequencer.c
> @@ -1029,13 +1029,11 @@ static int is_rfc2822_line(const char *buf, int len)
>         for (i = 0; i < len; i++) {
>                 int ch = buf[i];
>                 if (ch == ':')
> +                       return 1;
> +               if (!isalnum(ch) && ch != '-')
>                         break;
> -               if (isalnum(ch) || (ch == '-'))
> -                       continue;
> -               return 0;
>         }
> -
> -       return 1;
> +       return 0;
>  }
>
>  static int is_cherry_picked_from_line(const char *buf, int len)
> @@ -1043,9 +1041,7 @@ static int is_cherry_picked_from_line(const char *buf, int len)
>         /*
>          * We only care that it looks roughly like (cherry picked from ...)
>          */
> -       return !prefixcmp(buf, cherry_picked_prefix) &&
> -               (buf[len - 1] == ')' ||
> -                (buf[len - 1] == '\n' && buf[len - 2] == ')'));
> +       return !prefixcmp(buf, cherry_picked_prefix) && buf[len - 1] == ')';
>  }
>
>  static int has_conforming_footer(struct strbuf *sb, int ignore_footer)
> @@ -1072,8 +1068,8 @@ static int has_conforming_footer(struct strbuf *sb, int ignore_footer)
>                         ; /* do nothing */
>                 k++;
>
> -               if (!(is_rfc2822_line(buf + i, k - i) ||
> -                       is_cherry_picked_from_line(buf + i, k - i)))
> +               if (!is_rfc2822_line(buf + i, k - i - 1) &&
> +                   !is_cherry_picked_from_line(buf + i, k - i - 1))
>                         return 0;
>         }
>         return 1;

Looks good to me.

-Brandon