Re: [PATCH] am: match --signoff to the original scripted version

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 6, 2015, 17:39 UTC
Message-ID: <xmqq8u8jbg79.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <CACRoPnQvNowZGbqvRLuhS8cC1EaTQX3vsyaESSQVXpuRToWLOQ@mail.gmail.com>

Paul Tan <pyokagan@gmail.com> writes:

Show 19 quoted lines

>> +       /* Does it have any Signed-off-by: in the text */
>> +       for (cp = sb->buf;
>> +            cp && *cp && (cp = strstr(cp, sign_off_header)) != NULL;
>> +            cp = strchr(cp, '\n')) {
>> +               if (sb->buf == cp || cp[-1] == '\n')
>> +                       break;
>> +       }
>> +
>> +       strbuf_addstr(sb, mine.buf + !!cp);
>
> To add on, I wonder if the above "add a blank line if there is no
> Signed-off-by: at the beginning of a line" logic could be expressed
> more succinctly like this:
>
>     if (!starts_with(sb->buf, "Signed-off-by: ") &&
>         !strstr(sb->buf, "\nSigned-off-by: "))
>         strbuf_addch(sb, '\n');
>
>     strbuf_addstr(sb, mine.buf + 1);

That may be more obvious, but I wanted to reuse the existing sign_off_header[]; there is no variant that has a leading "end of previous line".

I actually think it is not an uncommon thing to ask "Give me the first occurrence of the string NEEDLE that appears at the beginning of lines in BUF", so the right way to address the "is it more readable" question would probably be to have a helper function to do just that, and use it here and other places that answer that question by themselves due to lack of such a helper.