Re: [PATCH 6/5] sequencer.c: refrain from adding duplicate s-o-b lines

From: Brandon Casey <drafnel@gmail.com>
Date: Nov 16, 2012, 23:55 UTC
Message-ID: <CA+sFfMfQz1qWpVZ4GCJdN8rOQNn+zkh9cd5wd7ouZL9xyqZRJQ@mail.gmail.com>
In-Reply-To: <7v4nkq8fsn.fsf@alter.siamese.dyndns.org>

On Thu, Nov 15, 2012 at 6:03 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 21 quoted lines

> Brandon Casey <drafnel@gmail.com> writes:
>
>> Detect whether the s-o-b already exists in the commit footer and refrain
>> from adding a duplicate.
>
> If you are trying to forbid
>
>         git cherry-pick -s $other
>
> from adding s-o-b: A when $other ends with these two existing s-o-b:
>
>         s-o-b: A
>         s-o-b: B
>
> then I think that is a wrong thing to do.
>
> In such a case, the resulting commit should gain another s-o-b from
> A to record the provenance as a chain of events.  A originally wrote
> the patch, B forwarded it (possibly with his own tweaks), and then A
> picked it up and recorded the result to the history, possibly with a
> final tweak or two.

Hmm. I've never thought that it was necessary to add an additional sob for the patches that I've cherry picked that I had previously signed-off-on. I considered one sign-off to be enough. In your example, A is the committer and the patch set already contains A's sign-off. For me that indicates that A still considers the commit to comply with whatever s-o-b implies.

We also seem to have a few tools to help people avoid adding duplicate sob's, like the current behavior of format-patch and the sample commit-msg hook.

I did some quick searching through the kernel commits to try to find some examples that could set a precedence. I didn't find anything that supported either argument. I didn't see any commits that were cherry-picked _and_ had an existing sob that was not the last sob. I didn't see any that had duplicate sob lines either. I'm not mentioning this to say that lack of a prior use should mean we should actively disallow the practice of adding duplicate sob's, I'm just providing it as a data point.

I've always thought that the reason that 'commit -s' and 'cherry-pick -s' checked only the last line of the commit message was simply that a full scan of the footer had not been implemented.

Whichever behavior is determined to be the right way for git to do it, format-patch should be brought in-line with the others and be built on top of the code in sequencer.c. So, if git _should_ create duplicate sob's then this patch should just be dropped. Duy's unification patch can just be built on top of sequencer.c:append_signoff() without bringing over any of the duplicate sob detection from log-tree.c.

-Brandon