Hello Sandrine,

IMO, the code review process has now evolved from the previous version. I am not too clear on the final outcome.

Does it make sense to add a "lifecycle of a review" section to the initial proposal?

-Varun

-----Original Message----- From: TF-A tf-a-bounces@lists.trustedfirmware.org On Behalf Of Sandrine Bailleux via TF-A Sent: Monday, July 6, 2020 1:53 AM To: Julius Werner jwerner@chromium.org Cc: tf-a tf-a@lists.trustedfirmware.org Subject: Re: [TF-A] Announcing some changes around the code review label in Gerrit

External email: Use caution opening links or attachments

Hi Julius and all,

As agreed and announced last week, I've now reintroduced the Code-Review label in addition to the Code-Owner-Review and Maintainer-Review ones.

The Code-Review label is purely informational and won't influence whether a patch is submittable in Gerrit. Anyone should be able to vote on this label, if you face any issue please let me know.

On 7/1/20 11:12 PM, Julius Werner wrote:

...

BTW, any help for the tooling is welcome! If you've got plugin configuration files or scripts we could reuse (with an appropriate license) or even tips on how to best set this up, please feel free to share these.

The Chromium Gerrit has an owners-enforcement system but I'm not familiar with how exactly they set it up. I think they're using this plugin https://gerrit.googlesource.com/plugins/find-owners/+/refs/heads/maste r and this is where it's integrated into Gerrit submission rules: https://chromium.googlesource.com/chromiumos/+/refs/meta/config/rules. pl . It works by having a file called OWNERS in certain directories which sets the owners for the subtree below it, so we would have to rewrite the current code owner list in that format if we wanted to use it.

This "find-owners" plugin sounds promising! Thanks for the pointers.

I notice the "reviewers" plugin [1] is installed on review.tf.org but it does not sound as configurable as "find-owners".

Rewriting the current code owners list to use the OWNERS format does not sound like a big task to me (just a dull one... could be scripted, though) so this approach definitely sounds worth investigating to me.

[1] https://review.trustedfirmware.org/plugins/reviewers/Documentation/index.htm...

There also seems to be this completely separate plugin that claims to do roughly the same thing, I have no idea where the difference is between the two: https://gerrit.googlesource.com/plugins/owners/+/refs/heads/master

I notice the "find-owners" plugin documentation mentions that "this plugin works with Gerrit projects that use Android or Chromium compatible OWNERS files" so maybe they started off from the generic "owners" plugin and customized it for Android/Chromium's needs? Just a guess.

Anyway, worth a look as well, thanks!

[2] https://gerrit.googlesource.com/plugins/find-owners/+/refs/heads/master/src/...

...

One option as you say would be to allow code owners to self-review their
patches but I am not convinced we would gain anything out of this. It
sounds like a tick-box exercise to me, an admin overhead just to get the
patch through the system and I would like to avoid that as much as
possible. It is likely that a patch submitter has already self-reviewed
his code before posting a patch anyway.

The alternative we've been discussing in the team is to call out another
reviewer in these situations. I think that there is still value in
having a second fresh pair of eyes on a patch. Even if the reviewer has
no particular expertise on this specific module, they can still catch
potential logic problems or structural issues in the code.

I definitely did not mean to suggest that these patches should not be reviewed at all -- I was just talking about the Code-Owner-Review label. There would still be a maintainer review, of course, and it seems like a good idea to get additional reviews from other people too. They just couldn't set the COR+1 bit once we start ACLing it.

Right, I see (and agree with you).

Basically, each of these bits have their own purpose, and I would see the purpose of the COR+1 bit to be that the person most familiar with that particular piece of code has made sure that it fits in and doesn't cause unexpected problems with certain quirky configurations or something like that. That's important when, say, I make a generic refactoring that touches a platform I'm unfamiliar with, but if the code owner adds to their own platform that's probably already a given and they are likely still the best person to judge that, even for their own code.

Yes, that makes sense to me.

That all code gets reviewed by people other than the author I would see as a somewhat orthogonal concern that should be checked independently. So maybe the rule could be that if the code owner sets COR+1 for themselves, there needs to be at least one Code-Review+1 (if we reintroduce that label) from another person (who is also not the one setting Maintainer-Review+1) to make sure the minimum amount of reviews stays the same. Or maybe these should all be completely independent checks so every patch needs a Code-Owner-Review+1 (can be from the author), a Maintainer-Review+1 (not from the author) and at least two Code-Review+1 from people other than the author -- with the normal expectation that when a maintainer or code owner reviews a patch, they would also set Code-Review+1 (as a general sign that they reviewed the patch) in addition to their special meaning flag.

I still don't see the benefit of code owners setting COR+1 for themselves. I would think that a patch owner already reviews his own patch before posting it for review so a self COR+1 is just reinstating the obvious in my eyes.

Your first suggestion (i.e. having at least one CR+1 from another person when a patch author cannot find another code owner to review their patches) sounds aligned with what I proposed earlier.

Your second suggestion sounds a bit too heavy to me. The fact that people would have to replay their vote on several labels sounds like an admin overhead to me. I guess this would simplify the ACL configuration but I would prefer to avoid this if we can.

So I suggest the following. We could have 2 ways to get a patch approved.

1) For patches that have multiple code owners: * COR+1 (not from patch owner) * MR+1 (not from patch owner, not from the person setting COR+1)

2) For patches that a have single code owner: * CR+1 (not from patch owner) * MR+1 (not from patch owner, not from the person setting CR+1)

In 2), I like your suggestion of mandating 2 CR+1 but I fear this might be difficult to achieve in practice (simply because we might not have enough reviewers for this).

Note that in some cases, we would need to mix both policies. If a patch modifies several modules, some of them having multiple code owners and others having a single code owner, we would apply one or the other policy for each module.

How does that sound?

Also, 1) assumes we always want different individuals doing the code-owner review and the maintainer review. I personally don't feel strongly about this split and I would be fine with the same person doing both types of review, as they are typically looking at different criteria.

What do others think?

Regards, Sandrine -- TF-A mailing list TF-A@lists.trustedfirmware.org https://lists.trustedfirmware.org/mailman/listinfo/tf-a