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

From: Jeff King <peff@peff.net>
Date: Sep 8, 2015, 06:25 UTC
Message-ID: <20150908062523.GE26331@sigill.intra.peff.net>
In-Reply-To: <xmqqio7ob0xw.fsf@gitster.mtv.corp.google.com>

On Sat, Sep 05, 2015 at 09:56:27PM -0700, Junio C Hamano wrote:

Show 13 quoted lines

> +static void am_signoff(struct strbuf *sb)
> +{
> +	char *cp;
> +	struct strbuf mine = STRBUF_INIT;
> +
> +	/* Does it end with our own sign-off? */
> +	strbuf_addf(&mine, "\n%s%s\n",
> +		    sign_off_header,
> +		    fmt_name(getenv("GIT_COMMITTER_NAME"),
> +			     getenv("GIT_COMMITTER_EMAIL")));
> +	if (mine.len < sb->len &&
> +	    !strcmp(mine.buf, sb->buf + sb->len - mine.len))
> +		goto exit; /* no need to duplicate */

Here you insert the "\n" directly at the start of "mine", so the test "does it contain S-o-b at the beginning of a line" does not count the first line. Probably fine, as somebody putting a S-o-b in their subject deserves whatever they get.

But...

Show 7 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;
> +	}

Here you are more careful about finding S-o-b at sb->buf.

I don't think it really matters in practice, but it's an interesting inconsistency.

Other than that (and I do not think it is worth re-rolling for this; just an interesting observation), the patch looks OK to me.

-Peff