Re: [edk2-devel] [edk2-rfc] GitHub Pull Request based Code Review Process

Laszlo Ersek

On 05/19/20 09:21, Desimone, Nathaniel L wrote:

However, I would like to register my general endorsement for pull
requests or some other web based system of code review... and I don't
have an Instagram account by the way :) Personally, I prefer Gerrit as
I use it a lot with coreboot and other projects. But since we are
using Github for hosting, pull requests are an easy switch and a
logical choice. My main reason for being excited about pull requests
mostly has to do with the amount of manual effort required to be a
TianoCore maintainer right now.
My understanding is that, at this point, we're inevitably going to
migrate the contribution/review workflow to GitHub. I believe the switch
is going to happen once the email webhook has been deemed functional and
stable enough by the community.

Digression starts:

Implementing the logic to parse the contents of emails to categorize
them like this required me to define no less than 12 email filter
rules in Microsoft Outlook, and I have to change my filtering logic
every time I am added/removed from a Maintainers.txt file.
That seems strange. I have one rule per edk2-* list, for moving such
incoming email into the appropriate list folder. That's all.

While I read all the subject lines (skim all the threads) on edk2-devel,
generally, if you share reviewer or maintainer responsibilities for some
subsystem, then people posting patches for that subsystem are supposed
to CC you explicitly, in addition to messaging the list.

How you handle messages from then on may be a personal matter of course.
I simply tag ("star") such messages (patches / series pending my
review), and I revisit my "set of starred messages" every day (sometimes
multiple times per day).

I'm sure every other maintainer has spent a time separately
implementing filtering logic like I have. This helps, but still for
every thread, I have to go and check if one of the other maintainers
has already reviewed/pushed that patch series yet, and if not
review/push it.
Checking whether others have commented is near trivial if your MUA
supports a threaded view.

Checking whether a co-maintainer of yours has pushed a given series is
also simple if they diligently report the fact of merging on the list
(in the subject patch threads).

If I have feedback on a patch series, I have to categorize it as
awaiting response from author and check up on it from time to time,
sometimes I ping the author directly and remind them to send a new
patch series.
I think this is not your job, as a reviewer/maintainer. Once your review
is complete, or blocked on a question you need an answer to, the ball is
back in the contributor's court. They can answer, or post the next
version, whenever they see fit. Until then, the most they can expect of
you is answering any further questions they might have for understanding
your previous feedback better. You need not push contributors to
complete their contributions.

Implementing this state machine is a lot of manual work and it kind of
feels like I'm a telephone operator in the 1950s. I greatly welcome
automation here as I am sure it will increase the number of patch
series I am able to review per hour.
"State machine" is a very good analogy! Personally, I don't find it
tiresome. Yes, it's important to recognize the events (= new emails)
that trigger transitions between states. (For example: when I complete a
review, when I get a new version of a series or a brand new series, when
I get asked a question.) Once I recognize those events correctly, I just
diligently massage said tags ("stars").

And I keep iterating over my set of "starred" messages; I do actual work
(e.g., reviews) in "bottom halves"; detached from new emails.

I don't find this a burden as I have to manage my "real life" with task
lists anyway. Without them, my real life would collapse in a week; so
it's nothing unusual for me. (And no, I don't allow shady cloud-based
automatisms to manage my life for me; I value my privacy way above my


Join to automatically receive all group messages.