Re: [Qemu-devel] [PATCH] e1000: allow command-line selection of card model

[Andreas Färber]

Am 21.05.2014 14:11, schrieb Michael S. Tsirkin:
> On Wed, May 21, 2014 at 12:42:13PM +0200, Andreas Färber wrote:
>> Am 21.05.2014 12:22, schrieb Michael S. Tsirkin:
>>> On Wed, May 21, 2014 at 11:48:48AM +0200, Andreas Färber wrote:
>>>> Am 21.05.2014 11:25, schrieb Michael S. Tsirkin:
>>>>> On Wed, May 21, 2014 at 11:12:42AM +0200, Andreas Färber wrote:
>>>>>> Am 21.05.2014 11:04, schrieb Michael S. Tsirkin:
>>>>>>> On Wed, May 21, 2014 at 10:40:54AM +0200, Andreas Färber wrote:
>>>>>>>> Am 20.05.2014 17:05, schrieb Gabriel L. Somlo:
>>>>>>>>> Allow selection of different card models from the qemu
>>>>>>>>> command line, to better accomodate a wider range of guests.
>>>>>>>>>
>>>>>>>>> Based-on-patch-by: Romain Dolbeau <address@hidden>
>>>>>>>>
>>>>>>>> If that patch carried a Signed-off-by line, you should retain it.
>>>>>>>
>>>>>>> Actually I think that would be confusing. Romain didn't sign off
>>>>>>> on *this* patch, he signed off on a previous one.
>>>>>>> A signature by Gabriel indicates Developer's Certificate of Origin 1.1
>>>>>>> which has an option to incorporate other's work - it
>>>>>>> does not seem to require signatures by these others.
>>>>>>
>>>>>> With the same argument you could drop anyone's Sob you get as a
>>>>>> maintainer.
>>>>>
>>>>> I could but it would not be nice to submitters, and it drops useful info
>>>>> (author's Sob).  So if someone thinks there's problematic code here and
>>>>> comes complaining, we want to be able to say "this code came from XYZ".
>>>>>
>>>>>
>>>>>> But the purpose of Sob is to track through whose hands a
>>>>>> patch went, not just who last touched it.
>>>>>
>>>>> Went untouched or mostly untouched.
>>>>> Did you bother checking?
>>>>> I looked and Romain's patch isn't very similar to this one.
>>>>>
>>>>>> My point here was that Based-on-patch-by is very unusual.
>>>>>
>>>>> What's the harm?
>>>>> Gabriel's just being nice and crediting other's work.
>>>>>
>>>>>> The alternative would be to leave the original From: Romain Dolbeau, his
>>>>>> Sob, then a [gsomlo: Dropped this, added that] followed by Sob.
>>>>>>
>>>>>> Cheers,
>>>>>> Andreas
>>>>>
>>>>> That's just asking submitter to do a lot of extra work,
>>>>> I don't see why would we place roadblocks in submitter's paths
>>>>> like this. Linux certainly does not and we didn't ask for this
>>>>> in the past.
>>>>>
>>>>> Further, the patch author in git will also be the original author then
>>>>> which is only fair if the patch is changed in very minor ways.
>>>>> In which case keeping the original Sob around *would* be right.
>>>>
>>>> Either the patch is based on the patch the submitter claims it is based
>>>> on, or it is not based on that patch.
>>>>
>>>> If it is, then the Sob should be retained because not doing so is
>>>> dropping useful information as you put it.
>>>
>>> It isn't useful if most of the patch has been changed,
>>> because Sob without the patch itself has no meaning.
>>>
>>>> You will find both ways, From
>>>> new and old+new Sob or From original and [], in git history, depending
>>>> on how much changed (which I have not checked here).
>>>
>>> Saying "Based on patch" in commit log is common practice too.
>>>
>>> You really can not redefine the meaning of Sob - it is
>>> widely understood to mean a very specific thing,
>>> Developer's certificate of origin 1.1 (which, by the way, qemu source
>>> really should include a copy of), which in particular says:
>>>
>>>         (b) The contribution is based upon previous work that, to the best
>>>             of my knowledge, is covered under an appropriate open source
>>>             license and I have the right under that license to submit that
>>>             work with modifications, whether created in whole or in part
>>>             by me, under the same open source license (unless I am
>>>             permitted to submit under a different license), as indicated
>>>             in the file; or
>>>
>>> so if you base upon legal previous work and can certify that, it is
>>> absolutely not required to track authors of that one and add their
>>> signatures. What if I copy code from virtual box's qemu? Would you make
>>> me troll their log for signatures?  When to do that and when not is a
>>> judgement call on behalf of the submitter.
>>>
>>>>
>>>> If it is not based on Romain's patch, then Suggested-by would be much
>>>> more to the point - and something the maintainer (Stefan) could easily
>>>> edit when signing off, if there were nothing else to change.
>>>
>>> Suggested-by implies there wasn't a patch, just a suggestion.
>>
>> To me, it doesn't imply that.
>>
>>> Maintainer can add any text on to the patch, that's also
>>> accepted practice.  I am merely saying it does not make sense to push
>>> back on contributors who are just trying to be nice.
>>>
>>> Really there's no rule I can see that says you should not
>>> add random tags to the commit log, and I don't see what would
>>> making up such rules gain us either.
>>>
>>> This is why I'm still responding to this thread really,
>>> you seem to make up new requirements that submitters need to
>>> meet, such things would have to get buy-in from more maintainers
>>> before we ask everyone to comply.
>>
>> I feel you are turning my words around in my mouth. Let's agree that we
>> don't agree here.
> 
> Hmm I certainly didn't intend to, so maybe there's a misunderstanding.
> 
>> * I never redefined the DCO. The DCO applies to a single Sob and by my
>> reading does not restrict having more than one Sob at all, and you
>> agreed that this is common practice.
>> * I did not reject the patch on that basis, I remarked it alongside
>> contential review.
> 
> Ah OK. It's where you said "you should retain it" that made me think
> you are saying tracking Sob from original sources is mandatory.

In case my use of "retain" was unclear, I specifically meant: Never
*drop* Signed-off-bys from a patch you are modifying and resending.

> so for the record, are you basically saying:
>       "you can replace Based-on-patch-by with Signed-off-by, it's
>       more or less equivalent, and more standard"
> 
> 
> Then I agree.
> Can you confirm please?

Confirmed, assuming that original patch has such Signed-off-by(s).

> I would add that
> 
> 
> - it's possible to include text similar to
> "Based on patch by ABC" in plain text
> in addition to, or instead of the tag.

Yep, when in doubt as explanation. Not machine-parseable of course, so
my recommendation would be "in addition to", but otherwise equivalent to
Some-newly-invented-by.

> - If the patch is mostly identical to the original, with trivial
> small modifications such as fixing typos, it's also possible to add
> From: ABC
> in front of the patch, this will then be the author recorded
> in the project history

Best, to not have it end up in the wrong place:
git commit --amend --author="name <email>"

Or actually the easiest variant:
git am original-author-s.patch; git commit --amend -a -s

> It's also possible to include the message id of the original patch if
> you really want to, services such as mid.gmane.org can later be used to
> look up messages based on the message id.

To be precise, you probably meant a Message-id: line as added by
Anthony's patches tool.
e.g.
http://git.qemu-project.org/?p=qemu.git;a=commit;h=e3da9921ebc554fad3224a9fdda9a7425ffd9ef7

Archive links OTOH can become stale after some months/years. But not
wrong. And certainly useful in cover letter or, as here, below ---.

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg