Re: [Qemu-devel] [PULL 0/6] target-arm queue

[Peter Maydell]

On 31 October 2013 17:27, Andreas Färber <address@hidden> wrote:
> Am 31.10.2013 18:18, schrieb Peter Maydell:
>> On 31 October 2013 17:14, Andreas Färber <address@hidden> wrote:
>>> Am 31.10.2013 16:16, schrieb Peter Maydell:
>>>> On 31 October 2013 14:36, Andreas Färber <address@hidden> wrote:
>>>>> It was clearly stated that a Reviewed-by
>>>>> needs to be explicitly sent as reply but that "looks okay" should in
>>>>> exactly such a case where sender=submaintainer should be recorded as
>>>>> Acked-by, and Sob is certainly stronger than Acked-by. Cf. minutes.
>>>>
>>>> ...but you're not the submaintainer here so I don't think this applies.
>>>
>>> It does, because you are the patch author and the ARM submaintainer
>>> sending the pull.
>>
>> Er, no, because they're ARM subsystem patches.
>
> You misunderstand. You sending an ARM patch in your ARM PULL with just
> your Sob is the same as me sending a CPU patch with just my Sob in my
> CPU PULL.

I agree with this...

> That's what I was saying.

...it's just not at all what you seemed to be saying. I think this is
related to a disagreement about whether acked-by is at all meaningful
for anybody who's not the relevant subsystem maintainer or otherwise
an "authoritative person".

> It is NOT about whether someone can veto something, it's about getting
> external review and formally recognizing that review.

No, that's what Reviewed-by is for. Acked-by is exactly a statement
that "I think this looks OK and my opinion matters", which is implicitly
making the statement that it's not a NAK, ie not a veto. It's a handy
way to avoid somebody further upstream having to make an explicit
query of that person about whether they'd seen this stuff and were
happy with it, nothing more.

So, to be clear:
 * I welcome external review
 * If I get review and people send emails to the list with reviewed-by:
   tags I'll do my best (and my workflow generally helps) to pick up
   and reflect those tags in the pull requests
 * I'm not going to attempt to infer reviewed-by tags from anything
   other than a specific reply to the list with a tag in the proper format
 * pragmatically speaking there are some patches for ARM which do
   not get any third-party review and where patches have been on list
   for a reasonable period of time I'm going to put them in pull requests,
   since we can't stop the world just because we don't have enough
   people willing to code review things
 * acked-by doesn't imply (to me) any kind of level of review beyond "I don't
   object to this", so it is irrelevant for the purposes of "try to make sure
   patches get review" (which is a goal I agree with)
 * nonetheless I'll generally reflect specifically sent acked-by tags
   where I get them, simply because my usual workflow tends to
   result in that
 * I think a general rule that all tags should be sent to the list explicitly
   and nobody should infer them will be simpler and less confusing
   for all concerned

> If Anthony is apparently making a retreat on that front

I don't recall Anthony ever saying that external review was going
to be mandatory. I think it's certainly something we should try to
do better with, but pragmatically speaking we're not going to get
to 100% reviewed overnight. I'd definitely object to any proposal
to enforce full code review by simply refusing to apply nonreviewed
patches now (and I don't think anybody's proposed that).

>, then we don't
> necessarily need external review on our own subsystems, but if we want
> to evaluate which or how many patches have been reviewed by someone else
> then we need to record that in the commit message in *some* way. I don't
> care what -by it is as long as we have and respect a clear rule.

I don't think the rules have ever changed here; they've been broadly
described in the kernel doc that our wiki page points to for at least a
year. If you've reviewed a patch you mark that with Reviewed-by.

-- PMM