Hi Sandrine,

If my understanding is right, Code-Owner-Review is the equivalent to the old Code-Review+1 and therefore anyone can add a vote there, owner or not. If that is so, I think the current name might be misleading. Wouldn't it be better just "Code-Review", for instance?

As an example, I am not code owner for Measure Boot, but sometimes I am requested to review the patches for it, so in that case it might lead to confusion if I give Code-Owner-Review score.

What do you think?

Best regards,
Javier


-----Original Message-----
From: Sandrine Bailleux via TF-A <tf-a@lists.trustedfirmware.org>
Reply-To: Sandrine Bailleux <sandrine.bailleux@arm.com>
To: tf-a <tf-a@lists.trustedfirmware.org>
Subject: [TF-A] Announcing some changes around the code review label in Gerrit
Date: Tue, 30 Jun 2020 13:29:00 +0000

Hello all,

If you recall, the tf.org Project Maintenance Process [1] advocates a
code review model where both code owners and maintainers need to review
and approve a patch before it gets merged.

Until now, the way we would record this in Gerrit was a bit cumbersome
and ambiguous.
* Code-Review+1 was for code owners to approve a patch.
* Code-Review+2 was for maintainers to approve a patch.

The submission rules in Gerrit were such that a patch needed a
Code-Review+2 to be merged. This meant that if a maintainer was first to
take a look at a patch and approved it (Code-Review+2), Gerrit would
allow the patch to be merged without any code owners review.

To align better with the Project Maintenance Process, I've done some
changes in our Gerrit configuration. You will notice that the
Code-Review label is gone and has been replaced by 2 new labels:
* Code-Owner-Review
* Maintainer-Review

Stating the obvious but code owners are expected to vote on the
Code-Owner-Review label and maintainers on the Maintainer-Review.

Note that maintainers might also be code owners for some modules. If
they are doing a "code owner" type of review on a patch, they would vote
on the Code-Owner-Review label in this case.

Any doubt, please ask. I hope I didn't break anything but if you notice
any permissions problems (things you used to be able to do and cannot do
anymore, or vice versa!) please let me know.

One annoying consequence of this change is that if you've got some
patches in review right now and got some votes on the former
'Code-Review' label, these have been partially lost. The history of
review comments in Gerrit will still show that somebody had voted
Code-Review -1/+1 on your patch but this will no longer appear in the
labels summary frame and won't count towards the approvals required to
get the patch merged.

I could not think of a way to avoid this issue... This should only be
temporary, while we transition all in-flight patches to the new code
review model.

Roughly:
* Existing "Code-Review -1/+1" votes will have to be replayed as
"Code-Owner-Review -1/+1".
* Existing "Code-Review -2/+2" votes will have to be replayed as
"Maintainer-Review -1/+1".

Sorry for the inconvenience. I hope, once we've gone through the
transition period, these changes will make the code review process
clearer to everybody.

I will update the TF-A documentation to explain all of this in the
coming future.

Best regards,
Sandrine

[1]
https://developer.trustedfirmware.org/w/collaboration/project-maintenance-process/