public inbox for gdb@sourceware.org
 help / color / mirror / Atom feed
* Proposal: Add review tags to patch review workflow.
@ 2022-09-21 11:04 Bruno Larsen
  2022-09-25 22:38 ` Lancelot SIX
                   ` (4 more replies)
  0 siblings, 5 replies; 35+ messages in thread
From: Bruno Larsen @ 2022-09-21 11:04 UTC (permalink / raw)
  To: gdb

TL;DR: I want to introduce the usage of 3 new review tags to the GDB 
patch review workflow. They are: Reviewed-by, Approved-by and Tested-by.

* Reviewed-by would be used by reviewers such as myself, who may look 
over the code but don't have the authority to approve it for pushing, 
essentially working as a +1.

* Approved-by would be used by maintainers who have the authority to 
approve code for pushing when they want to do so. Referenced as a +2 
later in this email.

* Tested-by would be used by new contributors or reviewers who aren't 
familiar with the code touched by a given patch, but who are able to 
verify that the test doesn't introduce regressions.

I will now dive into motivation and specifics of the proposal.

----

Motivation: As a new contributor, I see 4 main issues with the current 
patch review workflow:

1. It is sometimes hard to be sure that the reviewer has approved your 
patch, exemplified by a few recent patches which were already approved 
and were still pinged by their authors, such as 
https://sourceware.org/pipermail/gdb-patches/2022-August/191474.html
2. Patch review as it is now looks rather thankless. Sometimes a patch 
may go through various rounds of review, taking a lot of time from the 
contributor and reviewer, but if the latter never suggested enough code 
to warrant a Co-Authored-By tag, the only person mentioned at the final 
commit will be the original writer. I have personally wanted to thank 
reviewers who were especially helpful in understanding issues.
3. As a new contributor, it is not always obvious when a LGTM means the 
patch can be pushed. While one can always check the maintainers file, it 
would be nice if this information was baked into a review.
4. On the other hand, it's not always obvious to new reviewers 
(non-approvers) that their LGTM makes any difference, possibly 
discouraging them from giving positive feedback and making the community 
feel less lively than it is.

Adding Reviewed-by (R-b) tags by itself would solve the first 2 issues, 
since it only is given once the review is finished and the patch is good 
to go from the reviewer's perspective. Approved-by (A-b) tags were 
mentioned during the GDB BoF at Cauldron 2022, as a way to fix issues 3 
and 4, along with allowing for maintainers to give only +1, instead of 
+2, when they are not sure about a certain change. Tested-by (T-b) were 
also mentioned at the BoF as a way to give another option for new 
reviewers, especially if they have different hardware or setups.


The workflow: The basic usage is as simple as I explained above, if all 
a reviewer is comfortable with doing is confirming that the error is 
fixed and testing for regression, they can reply with a T-b tag; if they 
are comfortable sharing a positive opinion on the proposed code, but 
can't approve or are not sure if the patch should be approved, they can 
reply with an R-b tag; and if they wish to approve a patch, they reply 
with A-p.

Questions arose when thinking of new versions of a given patch. The 
workflow used by Glibc, which inspired great part of this proposal, 
makes it so the tags are dropped only when a patch has been 
"sufficiently changed". Cosmetic things like fixing typos or moving a 
proposed function from one place to another would not invalidate R-b 
tags, while reworking the solution would. The submitter has some space 
to decide that they think the patch has changed enough to warrant a new 
review (and invalidate the previous R-b), or the reviewer may ask their 
tag to be removed if they disagree with a change.

In case the explanation is not clear, the follow example shows how these 
would be used in a fictional patch that requires 3 versions before it is 
ready.

---

From: new<at>contributor<dot>com
Subject: [PATCH] Fix GDB's behavior

I fixed bug number PR/XXXXX by making GDB kick and scream instead of 
failing silently.

---

From: new<at>reviewer<dot>com
Subject: Re: [PATCH] Fix GDB's behavior

I'm not sure how good this solution is, but I verified that in my setup 
this doesn't regress anything and fixes the issue.
Tested-by: New Reviewer <new<at>reviewer<dot>com>

---

From: other<at>reviewer<dot>com
Subject: Re: [PATCH] Fix GDB's behavior

The solution looks good, but I have some style nits, see below.

---

From: new<at>contributor<dot>com
Subject: [PATCHv2] Fix GDB's behavior

I fixed bug number PR/XXXXX by making GDB kick and scream instead of 
failing silently.
Tested-by: New Reviewer <new<at>reviewer<dot>com>

---

From: other<at>reviewer<dot>com
Subject: Re: [PATCHv2] Fix GDB's behavior

Thanks, this looks good now!

Reviewed-by: Other Contributor <other<at>contributor<dot>com>

---

From: approver<at>gdb<dot>com
Subject: [PATCHv2] Fix GDB's behavior

I don't think this is a good solution, because you haven't fixed the 
problem, just complained about it. Please ensure that the patch makes 
GDB behave correctly with this input, though I'm not opposed to keeping 
the warning for other unexpected errors.

---

From: new<at>contributor<dot>com
Subject: [PATCHv3] Fix GDB's behavior

I fixed bug number PR/XXXXX by making GDB work as expected, and also 
added a warning for unexpected inputs.

---

From: other<at>maintainer<dot>com
Subject: Re: [PATCHv3] Fix GDB's behavior

Hi! This patch still looks good.
R-b: Other Maintainer <other<at>maintainer<dot>com>

---

From: approver<at>gdb<dot>com
Subject: Re: [PATCHv3] Fix GDB's behavior

I like this solution much better, thank you!
Approved-by: Approver <approver<at>gdb<dot>com>

---

From: new<at>contributor<dot>com
Subject: Re: Re: [PATCHv3] Fix GDB's behavior

Thank you, I pushed it!

-- 
Cheers,
Bruno


^ permalink raw reply	[flat|nested] 35+ messages in thread

end of thread, other threads:[~2022-10-10 15:25 UTC | newest]

Thread overview: 35+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-09-21 11:04 Proposal: Add review tags to patch review workflow Bruno Larsen
2022-09-25 22:38 ` Lancelot SIX
2022-09-26 13:55 ` Simon Marchi
2022-09-26 16:42   ` Joel Brobecker
2022-09-27  8:39     ` Luis Machado
2022-09-27  8:42       ` Luis Machado
2022-09-27  9:38       ` Lancelot SIX
2022-09-27 21:07         ` Thiago Jung Bauermann
2022-09-26 21:32   ` John Baldwin
2022-09-27  8:06     ` Bruno Larsen
2022-09-27 12:02       ` Simon Marchi
2022-09-27 12:03         ` Bruno Larsen
2022-09-27 17:11           ` John Baldwin
2022-09-27  7:58   ` Bruno Larsen
2022-09-27 12:03     ` Simon Marchi
2022-09-26 15:59 ` Luis Machado
2022-09-26 16:32   ` Elena Zannoni
2022-09-27  8:30     ` Bruno Larsen
2022-09-27 20:50 ` Thomas Schwinge
2022-10-07  7:49 ` Bruno Larsen
2022-10-07 20:46   ` Simon Marchi
2022-10-08  6:23     ` Eli Zaretskii
2022-10-08 11:55       ` Simon Marchi
2022-10-08 12:44         ` Eli Zaretskii
2022-10-09  0:29           ` Simon Marchi
2022-10-10  9:27           ` Bruno Larsen
2022-10-10  9:47             ` Eli Zaretskii
2022-10-10 10:11               ` Bruno Larsen
2022-10-10 11:27                 ` Eli Zaretskii
2022-10-10 12:31                   ` Bruno Larsen
2022-10-10 13:14                     ` Eli Zaretskii
2022-10-10 13:26                       ` Bruno Larsen
2022-10-10 15:25                         ` Eli Zaretskii
2022-10-10 13:34             ` Pedro Alves
2022-10-10  9:39     ` Luis Machado

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).