On Wed, 2019-03-27 at 11:20 +0000, Lorenzo Pieralisi wrote:
> On Wed, Mar 27, 2019 at 09:52:15AM +0000, David Woodhouse wrote:
> > On Tue, 2019-03-26 at 15:58 +0000, Lorenzo Pieralisi wrote:
> > > > We did that internally. You really don't want me telling engineers to
> > > > post to the list *first* without running things by me to get the basics
> > > > right. Not to start with, at least.
> > > 
> > > Hi David,
> > > 
> > > I am obviously in favour of internal review and I do not question it was
> > > carried out internally, I just kindly ask developers to drop review tags
> > > given internally when going to public mailing lists - I understand it is
> > > churn for you but I prefer them to be given explicitly.
> > 
> > Sure, I've provided mine in public now.
> > 
> > I will attempt to remember your preference, although I'm not sure I
> > think it's necessary.
> > 
> > What's the failure mode we're protecting against here? That my
> > engineers are lying and have *faked* my reviewed-by tag?
> > 
> > Don't you think I'd *eat* them if I ever found that happening?
> 
> As I wrote above, I did not question the internal review process at all,
> we do internal review at ARM too in preparation for posting publicly but
> I think the patches review should take place on public mailing lists and
> tags should be given accordingly, that's it.
> 
> You may see it as churn, fair enough, it is not a big deal either.

Understood. As I said, we will endeavour to comply.

Note that we may occasionally forget. Our internal review tooling
automatically *adds* those tags, when internal review happens. And we
have reduced the "legal" approval process to the point where myself or
a handful of others only need to give the nod in that same internal
technical review tool, for a patch to be sent upstream.

This means that engineers will have to remember to actively *remove*
those tags when they're sending PCI patches. We'll try to remember :)

> > What's next? That you only accept such tags in signed email, so that
> > the dishonest engineer in question can't *fake* an email from me to the
> > list? They know I'm afflicted by Exchange so they can always send that
> > fake message with a message-id matching another message they know is
> > already in my inbox, so Exchange will helpfully discard theirs. :)
> 
> There is nothing next :) - I just would like to see patches discussions
> and reviews taking place on linux-pci@vger.kernel.org for PCI patches,
> I do not think I am asking too much.

Not asking too much at all. We will do as you request.

I do think it's pointless — you really *don't* want to see the first
rounds of review that happened internally, and once the patch makes it
to the list, it doesn't make a lot of difference whether my Reviewed-
By: tag is already there or not; you don't get to see the previous
iterations of the patch or any of those prior review comments anyway. 

If *all* there is in my public response is that Reviewed-by: tag, there
is literally no benefit to that at all, except that you know the
engineer isn't lying. None at all.

But it's fine. If it really annoys me, I can set up an autoresponder
which sends an empty mail with a 'Reviewed-by:' tag, and my engineers
can include a header in their patch submission which triggers that. We
can even automate the tooling, to turn the normal Reviewed-by: tag into
that header which triggers the response.

We will comply with your request, even if we don't understand it :)