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

[Brandon Casey <bcasey@nvidia.com>]

On 2/9/2013 3:06 PM, Junio C Hamano wrote:
> 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.
-----------------------------------------------------------------------------------