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

[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:
> >>>>>> Hi,
> >>>>>>
> >>>>>> 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.
> >>
> >> Andreas
> > 
> > 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.

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?

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.

- 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

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.

> What the contributor makes of it depends on him, and
> he hasn't spoken up so far, so you're not his lawyer either.
> * People are free to invent new *-bys, but they shouldn't when there
> exists one of different name and precise meaning, in particular Sob.
> This is not against random *-by, it's specifically about
> Based-on-patch-by which sounded redundant to Sob for me.
> * We are having GSoC students around, and they shouldn't be picking up
> bad examples just because no one else cares to comment on or fix commit
> messages like I do. Linux also lets a lot of typos slip through, doesn't
> mean we need to when we notice them.

Right.  It's exactly for the benefit of the new people that I am
responding here, we should try to make the rules explicit, to reduce the
barrier of entry.

> CC'ing PMM, who has stated opinions on how to do these things too, so
> it's unfair to pretend that it were just me making this up.
> 
> Andreas
> 
> -- 
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg