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

[Junio C Hamano <gitster@pobox.com>]

Brandon Casey <drafnel@gmail.com> writes:

> -static int ends_rfc2822_footer(struct strbuf *sb, int ignore_footer)
> +static int is_rfc2822_line(const char *buf, int len)
> +{
> +	int i;
> +
> +	for (i = 0; i < len; i++) {
> +		int ch = buf[i];
> +		if (ch == ':')
> +			break;
> +		if (isalnum(ch) || (ch == '-'))
> +			continue;
> +		return 0;
> +	}
> +
> +	return 1;
> +}
> +
> +static int is_cherry_pick_from_line(const char *buf, int len)
> +{
> +	return (strlen(cherry_picked_prefix) + 41) <= len &&
> +		!prefixcmp(buf, cherry_picked_prefix);
> +}
> +
> +static int has_conforming_footer(struct strbuf *sb, int ignore_footer)
>  {
> -	int ch;
>  	int hit = 0;
> -	int i, j, k;
> +	int i, k;
>  	int len = sb->len - ignore_footer;
>  	const char *buf = sb->buf;
>  
> @@ -1039,15 +1061,9 @@ static int ends_rfc2822_footer(struct strbuf *sb, int ignore_footer)
>  			; /* do nothing */
>  		k++;
>  
> -		for (j = 0; i + j < len; j++) {
> -			ch = buf[i + j];
> -			if (ch == ':')
> -				break;
> -			if (isalnum(ch) ||
> -			    (ch == '-'))
> -				continue;
> +		if (!(is_rfc2822_line(buf+i, k-i) ||
> +			is_cherry_pick_from_line(buf+i, k-i)))
>  			return 0;
> -		}
>  	}
>  	return 1;
>  }

Refactored code looks vastly more readable, but I think the
is_cherry_pick_from_line() function (by the way, shouldn't it be
named is_cherry_picked_from_line()?) shows its ambivalence.  It
insists that the line has to be long enough to hold 40-hex object
name plus a closing parenthesis, but it only makes sure that the
prefix matches, without checking if the line has 40-hex object name,
or the object name is immediately followed by a closing parenthesis.
It also does not care if there are other garbage after it.

If the code is trying to be strict to avoid misidentification, then
the check should be tightened (i.e. require the known fixed length,
make sure get_sha1_hex() is happy, 41st byte is a close parenthesis
that is followed by the end of line).  If the code is trying to be
more lenient to allow people hand-editing the cherry-picked-from
line that was generated, the check could be loosened (people may
truncate the 40-hex down to 12-hex or something).

I cannot read from this code which one was intended; the code must
make up its mind, whichever way (I do not have a strong preference).

> +test_expect_success 'cherry-pick -x -s adds sob when last sob doesnt match committer' '

Is the title of this test appropriate?  It looks like it is making
sure we do not add an extra blank line after the existing footer to
me.

> +	pristine_detach initial &&
> +	sha1=`git rev-parse mesg-with-footer^0` &&
> +	git cherry-pick -x -s mesg-with-footer &&
> +	cat <<-EOF >expect &&
> +		$mesg_with_footer
> +		(cherry picked from commit $sha1)
> +		Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>
> +	EOF
> +	git log -1 --pretty=format:%B >actual &&
> +	test_cmp expect actual
> +'
> +

> +test_expect_success 'cherry-pick -x -s adds sob even when trailing sob exists for committer' '
> +	pristine_detach initial &&
> +	sha1=`git rev-parse mesg-with-footer-sob^0` &&
> +	git cherry-pick -x -s mesg-with-footer-sob &&
> +	cat <<-EOF >expect &&
> +		$mesg_with_footer_sob
> +		(cherry picked from commit $sha1)
> +		Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>
> +	EOF
> +	git log -1 --pretty=format:%B >actual &&
> +	test_cmp expect actual
> +'

For people on the sideline, $mesg_with_footer_sob ends with s-o-b by
the same "$GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>" we are adding
here.  This is probably a sensible thing to do.

One thing I am not so happy about this whole "(cherry picked from"
thing (and I am again agreeing with Linus who made me change the
default long time ago not to add this line by default) is this.  If
you use "cherry-pick -s -x $commit" to transplant a commit from an
unrelated part of the history, you will get the object name in the
resulting commit.  But you can do the same in at least two different
ways.  The easiest is:

    git format-patch -1 --stdout $commit | git am -s3

and a bit closer to what "cherry-pick" does is:

    git checkout $commit^0 &&
    git rebase --onto @{-1} HEAD^ &&
    git checkout -B @{-1}

i.e. rebase the single commit on top of the branch you were on.

In either way, you don't get "cherry picked from", even though you
just did the moral equivalent of it.  Also we need to realize that
the first one is essentially what happens all the time here; the "|"
is implemented by the mailing list.  And nobody misses "cherry
picked from" to point at the original commit object, which is
useless for the recipient of the resulting commit.

But that is an orthogonal issue.