Re: [PATCH v2 06/10] sequencer.c: teach append_signoff how to detect duplicate s-o-b

From: Brandon Casey <bcasey@nvidia.com>
Date: Feb 9, 2013, 23:49 UTC
Message-ID: <5116E07F.9020607@nvidia.com>
In-Reply-To: <7v7gmhulya.fsf@alter.siamese.dyndns.org>

On 2/9/2013 3:06 PM, Junio C Hamano wrote:

Show 32 quoted lines

> Junio C Hamano <gitster@pobox.com> writes:
> 
>> Brandon Casey <drafnel@gmail.com> writes:
>>
>>> On Tue, Jan 22, 2013 at 12:38 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
>>>> Brandon Casey wrote:
>>>>
>>>>> Teach append_signoff how to detect a duplicate s-o-b in the commit footer.
>>>>> This is in preparation to unify the append_signoff implementations in
>>>>> log-tree.c and sequencer.c.
>>>> [...]
>>>>> --- a/sequencer.c
>>>>> +++ b/sequencer.c
>>>>> @@ -1082,9 +1101,10 @@ int sequencer_pick_revisions(struct replay_opts *opts)
>>>>>       return pick_commits(todo_list, opts);
>>>>>  }
>>>>>
>>>>> -void append_signoff(struct strbuf *msgbuf, int ignore_footer)
>>>>> +void append_signoff(struct strbuf *msgbuf, int ignore_footer, int no_dup_sob)
>>>>
>>>> Isn't the behavior of passing '1' here just a bug in "format-patch -s"?
>>>
>>> I think that is an open question.
>>
>> Yes. as I said in a previous review round, I think it was a mistake
>> that format-patch chose to pay attention to any S-o-b in the patch
>> trail, not only the last one, and we may want to correcct it once
>> this series solidifies as a separate "bugfix" change on top.
> 
> This is a tangent, but I _think_ (didn't check, though) "git am -s"
> implements this incorrrectly.  Just another LHF somebody may want to
> take a look.

(I haven't checked the 'git am -s' behavior, but ...)

One distinction between commit, cherry-pick, am, and format-patch is that the first three are related to importing commits _into_ your own repository while format-patch is related to exporting patches _from_ your repository.

Not sure if an "exporting" operation should be treated differently from an "importing" operation and mean "add my sign-off only if I did not previously do so when I did commit, cherry-pick, or am" or whether it should mean the same thing i.e. "append my sign-off, unless it's already last".

Just pointing something out that may or may not be useful.

Hmm, actually, this series could provide a data point. When I submitted the first series, I used 'format-patch -s' to append my sob. When Jonathan reviewed the series, he provided Acked-by and Reviewed-by which I collected and added as I reworked the series. When I added Jonathan's Ack's/Rev-by's, I _appended_ them to the commits that I had already submitted, so they appeared _after_ my own sob. When I submitted the later iterations of the series, I probably used 'format-patch -s' again and I of course did not want an additional sob to be appended after Jonathan's Acks.

-Brandon

----------------------------------------------------------------------------------- This email message is for the sole use of the intended recipient(s) and may contain confidential information. Any unauthorized review, use, disclosure or distribution is prohibited. If you are not the intended recipient, please contact the sender by reply email and destroy all copies of the original message. -----------------------------------------------------------------------------------