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

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