* 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
* Re: Proposal: Add review tags to patch review workflow.
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
` (3 subsequent siblings)
4 siblings, 0 replies; 35+ messages in thread
From: Lancelot SIX @ 2022-09-25 22:38 UTC (permalink / raw)
To: Bruno Larsen; +Cc: gdb
Hi Bruno
Thanks for putting this together. As far as I am concerned, I find this
reasonable, and am willing to give it a try. Also, considering that the
patchwork got a fresh start, I am curious to see if this workflow can
accommodate GDB. Of course, I am no maintainer so I leave the decision
to their wisdom.
To comment on your motivations, as a still relatively new contributor
and occasional reviewer, I know that I tend to stay away from any
language that might imply that I give any sort of approval to patches.
This is not for me to do, and unless I am sure the patch’s author is
familiar with who can approve patches, I want to avoid any possible
confusion. As a consequence, I only comment on patches if I find issues
or have questions, not when I find a patch OK to me. The corollary is
that from the outside a patch I have not read, a patch I skimmed through
or a patch I spend time testing and am convinced is good all look the
same. This give little help to maintainers in their task. The system
you propose to adopt could help improve on this.
Best,
Lancelot.
On Wed, Sep 21, 2022 at 01:04:47PM +0200, Bruno Larsen via Gdb wrote:
> 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
* Re: Proposal: Add review tags to patch review workflow.
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
` (2 more replies)
2022-09-26 15:59 ` Luis Machado
` (2 subsequent siblings)
4 siblings, 3 replies; 35+ messages in thread
From: Simon Marchi @ 2022-09-26 13:55 UTC (permalink / raw)
To: Bruno Larsen, gdb
On 2022-09-21 07:04, Bruno Larsen via Gdb wrote:
> 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.
Hi Bruno,
I completely agree with the proposal. I really like the fact that it
makes communication less ambiguous. Following some process (or changing
the process) can feel a bit heavy for long-timers, but I think it makes
things much clearer for newcomers.
Assuming we will go through with this proposal, it will need to be
documented on the wiki so we can easily refer people to the procedure.
Probably the ContributionChecklist page?
https://sourceware.org/gdb/wiki/ContributionChecklist
Will you be able to take care of this when needed (do you have write
access to the wiki)?
In the mean time, message to others: please let us know if you agree
with this, it's difficult to know we have the support of the community
if everybody silently agrees!
Simon
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: Proposal: Add review tags to patch review workflow.
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 15:59 ` Luis Machado
2022-09-26 16:32 ` Elena Zannoni
2022-09-27 20:50 ` Thomas Schwinge
2022-10-07 7:49 ` Bruno Larsen
4 siblings, 1 reply; 35+ messages in thread
From: Luis Machado @ 2022-09-26 15:59 UTC (permalink / raw)
To: Bruno Larsen, gdb
Hi Bruno,
Thanks for putting this together. I think this is an improvement to the current review workflow, and I
think it would be a positive update.
On 9/21/22 12:04, Bruno Larsen via Gdb wrote:
> 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!
>
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: Proposal: Add review tags to patch review workflow.
2022-09-26 15:59 ` Luis Machado
@ 2022-09-26 16:32 ` Elena Zannoni
2022-09-27 8:30 ` Bruno Larsen
0 siblings, 1 reply; 35+ messages in thread
From: Elena Zannoni @ 2022-09-26 16:32 UTC (permalink / raw)
To: Luis Machado, Bruno Larsen, gdb
One thing to do possibly is to take a look at what the kernel does in
this area, with the tags for inspiration:
https://www.kernel.org/doc/html/latest/process/submitting-patches.html
(middle of the page).
The key-signed patch workflow is also worth looking at to implement.
elena
On 9/26/22 9:59 AM, Luis Machado via Gdb wrote:
> Hi Bruno,
>
> Thanks for putting this together. I think this is an improvement to the
> current review workflow, and I
> think it would be a positive update.
>
> On 9/21/22 12:04, Bruno Larsen via Gdb wrote:
>> 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!
>>
>
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: Proposal: Add review tags to patch review workflow.
2022-09-26 13:55 ` Simon Marchi
@ 2022-09-26 16:42 ` Joel Brobecker
2022-09-27 8:39 ` Luis Machado
2022-09-26 21:32 ` John Baldwin
2022-09-27 7:58 ` Bruno Larsen
2 siblings, 1 reply; 35+ messages in thread
From: Joel Brobecker @ 2022-09-26 16:42 UTC (permalink / raw)
To: Simon Marchi via Gdb; +Cc: Bruno Larsen, Joel Brobecker
Just thinking out loud...
> I completely agree with the proposal. I really like the fact that it
> makes communication less ambiguous. Following some process (or changing
> the process) can feel a bit heavy for long-timers, but I think it makes
> things much clearer for newcomers.
Speaking of ambiguous, one thing that we used to do well in the past
but then kind of got worse was the subject prefix we used to use
to indicate the status of a patch. In particular, we used to reserve
certain keywords for that in the subject (e.g. "RFA" vs "PATCH", or
"OB" for obvious, etc). We lost that part, not sure exactly when,
but I suspect sometime when we transitionned to Git.
Something else also that I have been feeling the last year or two
is that I'm not sure people now explicitly confirm to the list
when a patch is pushed.
The reason I mention this is to show that perhaps we're getting back
to the fact that our email reviewing system is still email-based.
One way to address the various limitations is by adding more
processes, as suggested here. This has the good property of being
fairly cheap to discuss and implement, at the cost of a small
added overhead. I don't have a strong opinion about it, either
for or against (and given the amount of time I have to contribute
anyway, I don't think I should have a say).
With that said, I have a feeling that switching to a system designed
to manage patch submissions and reviews, no matter imperfect, is going
to solve a lot of the limitations of the current email-based system.
So that's another option worth reviewing from time to time, I think.
I understand that selecting, deploying and trying new review systems
requires a fair amount of effort. But having seen the benefits of
using several different such systems, I am convinced that the gains
will be very much worth whatever the drawbacks of that system might be.
> Assuming we will go through with this proposal, it will need to be
> documented on the wiki so we can easily refer people to the procedure.
> Probably the ContributionChecklist page?
>
> https://sourceware.org/gdb/wiki/ContributionChecklist
>
> Will you be able to take care of this when needed (do you have write
> access to the wiki)?
>
> In the mean time, message to others: please let us know if you agree
> with this, it's difficult to know we have the support of the community
> if everybody silently agrees!
--
Joel
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: Proposal: Add review tags to patch review workflow.
2022-09-26 13:55 ` Simon Marchi
2022-09-26 16:42 ` Joel Brobecker
@ 2022-09-26 21:32 ` John Baldwin
2022-09-27 8:06 ` Bruno Larsen
2022-09-27 7:58 ` Bruno Larsen
2 siblings, 1 reply; 35+ messages in thread
From: John Baldwin @ 2022-09-26 21:32 UTC (permalink / raw)
To: Simon Marchi, Bruno Larsen, gdb
On 9/26/22 6:55 AM, Simon Marchi via Gdb wrote:
>
>
> On 2022-09-21 07:04, Bruno Larsen via Gdb wrote:
>> 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.
>
> Hi Bruno,
>
> I completely agree with the proposal. I really like the fact that it
> makes communication less ambiguous. Following some process (or changing
> the process) can feel a bit heavy for long-timers, but I think it makes
> things much clearer for newcomers.
>
> Assuming we will go through with this proposal, it will need to be
> documented on the wiki so we can easily refer people to the procedure.
> Probably the ContributionChecklist page?
>
> https://sourceware.org/gdb/wiki/ContributionChecklist
>
> Will you be able to take care of this when needed (do you have write
> access to the wiki)?
>
> In the mean time, message to others: please let us know if you agree
> with this, it's difficult to know we have the support of the community
> if everybody silently agrees!
I'm fine with the idea. I'm less worried about "credit" for reviewing
personally, and the suggested format seems a tad verbose perhaps vs
just formalizing "Approved", but it's probably good to have it be a bit
different from straight prose to be more explicit.
It also wasn't clear to me if the intention was for the commits to
be amended with the annotations? (I don't think it was explicitly
stated in the original mail, and I'm not sure if it was an implicit
assumption?)
--
John Baldwin
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: Proposal: Add review tags to patch review workflow.
2022-09-26 13:55 ` Simon Marchi
2022-09-26 16:42 ` Joel Brobecker
2022-09-26 21:32 ` John Baldwin
@ 2022-09-27 7:58 ` Bruno Larsen
2022-09-27 12:03 ` Simon Marchi
2 siblings, 1 reply; 35+ messages in thread
From: Bruno Larsen @ 2022-09-27 7:58 UTC (permalink / raw)
To: Simon Marchi, gdb
On 26/09/2022 15:55, Simon Marchi wrote:
>
> On 2022-09-21 07:04, Bruno Larsen via Gdb wrote:
>> 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.
> Hi Bruno,
>
> I completely agree with the proposal. I really like the fact that it
> makes communication less ambiguous. Following some process (or changing
> the process) can feel a bit heavy for long-timers, but I think it makes
> things much clearer for newcomers.
>
> Assuming we will go through with this proposal, it will need to be
> documented on the wiki so we can easily refer people to the procedure.
> Probably the ContributionChecklist page?
>
> https://sourceware.org/gdb/wiki/ContributionChecklist
>
> Will you be able to take care of this when needed (do you have write
> access to the wiki)?
Hi Simon,
Thanks for the reply! I don't have write access to the wiki (In fact, I
just created my account on it), can you give me the necessary
permissions, or do I need to ask someone else? (in case it is the
latter, who?)
Cheers,
Bruno
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: Proposal: Add review tags to patch review workflow.
2022-09-26 21:32 ` John Baldwin
@ 2022-09-27 8:06 ` Bruno Larsen
2022-09-27 12:02 ` Simon Marchi
0 siblings, 1 reply; 35+ messages in thread
From: Bruno Larsen @ 2022-09-27 8:06 UTC (permalink / raw)
To: John Baldwin, Simon Marchi, gdb
On 26/09/2022 23:32, John Baldwin wrote:
> On 9/26/22 6:55 AM, Simon Marchi via Gdb wrote:
>>
>>
>> On 2022-09-21 07:04, Bruno Larsen via Gdb wrote:
>>> 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.
>>
>> Hi Bruno,
>>
>> I completely agree with the proposal. I really like the fact that it
>> makes communication less ambiguous. Following some process (or changing
>> the process) can feel a bit heavy for long-timers, but I think it makes
>> things much clearer for newcomers.
>>
>> Assuming we will go through with this proposal, it will need to be
>> documented on the wiki so we can easily refer people to the procedure.
>> Probably the ContributionChecklist page?
>>
>> https://sourceware.org/gdb/wiki/ContributionChecklist
>>
>> Will you be able to take care of this when needed (do you have write
>> access to the wiki)?
>>
>> In the mean time, message to others: please let us know if you agree
>> with this, it's difficult to know we have the support of the community
>> if everybody silently agrees!
>
> I'm fine with the idea. I'm less worried about "credit" for reviewing
> personally, and the suggested format seems a tad verbose perhaps vs
> just formalizing "Approved", but it's probably good to have it be a bit
> different from straight prose to be more explicit.
Hi John,
Thanks for your input! While it is a bit verbose, I didn't find it to be
a problem when reading through commits that used it (on other projects)
because of the tag-like formatting. It's quick and easy to identify and
skip through it when looking through the commit history, and easy to
automate the emitting from the reviewer side.
>
> It also wasn't clear to me if the intention was for the commits to
> be amended with the annotations? (I don't think it was explicitly
> stated in the original mail, and I'm not sure if it was an implicit
> assumption?)
No, I didn't intend on amending previous commits. The main problem this
change intends to solve is fixing ambiguity, and the pushed patches
don't have that issue anymore. Thanking a reviewer is just one more
positive side (IMHO) going forward.
Cheers,
Bruno
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: Proposal: Add review tags to patch review workflow.
2022-09-26 16:32 ` Elena Zannoni
@ 2022-09-27 8:30 ` Bruno Larsen
0 siblings, 0 replies; 35+ messages in thread
From: Bruno Larsen @ 2022-09-27 8:30 UTC (permalink / raw)
To: Elena Zannoni, Luis Machado, gdb
Hi Elena,
Thanks for the link, I took a look at it. From what I could understand,
the Signed-off-by tags solve a problem that we don't really have in GDB,
as the kernel seems to have multiple layers of pushing between the
submitter and it going upstream, whereas here we manually push the
patch, so there is little need to track submissions so thoroughly. I am
indifferent to reported-by and sugested-by tags, as we tend to link the
bugs already in submissions.
Also, I don't think this proposal should include those tags, simply
because other contributor's opinions may change based on it, delaying
acceptance of R-b, T-b and A-b tags.
I will, however, use that text to inspire what I will write in the wiki
about the suggested tags. Thanks a lot!
Cheers,
Bruno
On 26/09/2022 18:32, Elena Zannoni wrote:
> One thing to do possibly is to take a look at what the kernel does in
> this area, with the tags for inspiration:
> https://www.kernel.org/doc/html/latest/process/submitting-patches.html
> (middle of the page).
>
> The key-signed patch workflow is also worth looking at to implement.
>
> elena
>
>
> On 9/26/22 9:59 AM, Luis Machado via Gdb wrote:
>> Hi Bruno,
>>
>> Thanks for putting this together. I think this is an improvement to the
>> current review workflow, and I
>> think it would be a positive update.
>>
>> On 9/21/22 12:04, Bruno Larsen via Gdb wrote:
>>> 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!
>>>
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: Proposal: Add review tags to patch review workflow.
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
0 siblings, 2 replies; 35+ messages in thread
From: Luis Machado @ 2022-09-27 8:39 UTC (permalink / raw)
To: Joel Brobecker, Simon Marchi via Gdb
Hi Joel,
Thanks for bringing this up.
On 9/26/22 17:42, Joel Brobecker via Gdb wrote:
> Just thinking out loud...
>
>> I completely agree with the proposal. I really like the fact that it
>> makes communication less ambiguous. Following some process (or changing
>> the process) can feel a bit heavy for long-timers, but I think it makes
>> things much clearer for newcomers.
>
> Speaking of ambiguous, one thing that we used to do well in the past
> but then kind of got worse was the subject prefix we used to use
> to indicate the status of a patch. In particular, we used to reserve
> certain keywords for that in the subject (e.g. "RFA" vs "PATCH", or
> "OB" for obvious, etc). We lost that part, not sure exactly when,
> but I suspect sometime when we transitionned to Git.
>
> Something else also that I have been feeling the last year or two
> is that I'm not sure people now explicitly confirm to the list
> when a patch is pushed.
I think that's been happening, yes. But the IRC bot mentions commits explicitly, and
developers tend to see updates in the git repo when they update the sources.
With that said, in general the frequent GDB contributors tend to be quite busy (with
GDB or other things), so I'm inclined to say it is positive to have less steps to
take care of to push a change.
For example, ChangeLog's were a big time sink, and we managed to get rid of that rule. I think
that was very positive.
We still have other potential improvements waiting to be discussed, like auto-formatting of code
with some tool like clang-format. Time spent correcting formatting is not very useful.
>
> The reason I mention this is to show that perhaps we're getting back
> to the fact that our email reviewing system is still email-based.
> One way to address the various limitations is by adding more
> processes, as suggested here. This has the good property of being
> fairly cheap to discuss and implement, at the cost of a small
> added overhead. I don't have a strong opinion about it, either
> for or against (and given the amount of time I have to contribute
> anyway, I don't think I should have a say).
>
> With that said, I have a feeling that switching to a system designed
> to manage patch submissions and reviews, no matter imperfect, is going
> to solve a lot of the limitations of the current email-based system.
> So that's another option worth reviewing from time to time, I think.
> I understand that selecting, deploying and trying new review systems
> requires a fair amount of effort. But having seen the benefits of
> using several different such systems, I am convinced that the gains
> will be very much worth whatever the drawbacks of that system might be.
That's a fairly good point, and I agree. We tried a patch reviewing system (gerrit)
before. For me, at the time, it was obvious that the number of reviews increased
significantly. It was just easier to do reviews that way. If you had 5 minutes, you
could scan for a small change and give some feedback. The list of patches to-be-reviewed
was never forgotten.
But back then we didn't want to risk alienating global maintainers that didn't like gerrit or
liked the e-mail system better, so we dropped that effort and put nothing back in its place. It feels
to me patch reviewing by non-contributors lost some of the traction it had gained with gerrit.
It is not a secret that some of us contributors would like to see improvements in this area, hence
my suggestion to address patch reviewing/more maintainers/CI-based testing as topics for the GDB BoF.
But at the end of the day, it's up to the global maintainers to make a decision on this topic, or to
let contributors know they are open to adopting improvements.
So, in summary, I see the proposal to add tags as a way to improve a patch reviewing system that
is not being capable of keeping up with demand. I doubt we would need such tagging if we had a
proper reviewing system in place (be it gerrit, patchworks or any other).
>
>> Assuming we will go through with this proposal, it will need to be
>> documented on the wiki so we can easily refer people to the procedure.
>> Probably the ContributionChecklist page?
>>
>> https://sourceware.org/gdb/wiki/ContributionChecklist
>>
>> Will you be able to take care of this when needed (do you have write
>> access to the wiki)?
>>
>> In the mean time, message to others: please let us know if you agree
>> with this, it's difficult to know we have the support of the community
>> if everybody silently agrees!
>
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: Proposal: Add review tags to patch review workflow.
2022-09-27 8:39 ` Luis Machado
@ 2022-09-27 8:42 ` Luis Machado
2022-09-27 9:38 ` Lancelot SIX
1 sibling, 0 replies; 35+ messages in thread
From: Luis Machado @ 2022-09-27 8:42 UTC (permalink / raw)
To: Joel Brobecker, Simon Marchi via Gdb
On 9/27/22 09:39, Luis Machado via Gdb wrote:
> Hi Joel,
>
> Thanks for bringing this up.
>
> On 9/26/22 17:42, Joel Brobecker via Gdb wrote:
>> Just thinking out loud...
>>
>>> I completely agree with the proposal. I really like the fact that it
>>> makes communication less ambiguous. Following some process (or changing
>>> the process) can feel a bit heavy for long-timers, but I think it makes
>>> things much clearer for newcomers.
>>
>> Speaking of ambiguous, one thing that we used to do well in the past
>> but then kind of got worse was the subject prefix we used to use
>> to indicate the status of a patch. In particular, we used to reserve
>> certain keywords for that in the subject (e.g. "RFA" vs "PATCH", or
>> "OB" for obvious, etc). We lost that part, not sure exactly when,
>> but I suspect sometime when we transitionned to Git.
>>
>> Something else also that I have been feeling the last year or two
>> is that I'm not sure people now explicitly confirm to the list
>> when a patch is pushed.
>
> I think that's been happening, yes. But the IRC bot mentions commits explicitly, and
> developers tend to see updates in the git repo when they update the sources.
>
> With that said, in general the frequent GDB contributors tend to be quite busy (with
> GDB or other things), so I'm inclined to say it is positive to have less steps to
> take care of to push a change.
>
> For example, ChangeLog's were a big time sink, and we managed to get rid of that rule. I think
> that was very positive.
>
> We still have other potential improvements waiting to be discussed, like auto-formatting of code
> with some tool like clang-format. Time spent correcting formatting is not very useful.
>
>>
>> The reason I mention this is to show that perhaps we're getting back
>> to the fact that our email reviewing system is still email-based.
>> One way to address the various limitations is by adding more
>> processes, as suggested here. This has the good property of being
>> fairly cheap to discuss and implement, at the cost of a small
>> added overhead. I don't have a strong opinion about it, either
>> for or against (and given the amount of time I have to contribute
>> anyway, I don't think I should have a say).
>>
>> With that said, I have a feeling that switching to a system designed
>> to manage patch submissions and reviews, no matter imperfect, is going
>> to solve a lot of the limitations of the current email-based system.
>> So that's another option worth reviewing from time to time, I think.
>> I understand that selecting, deploying and trying new review systems
>> requires a fair amount of effort. But having seen the benefits of
>> using several different such systems, I am convinced that the gains
>> will be very much worth whatever the drawbacks of that system might be.
>
> That's a fairly good point, and I agree. We tried a patch reviewing system (gerrit)
> before. For me, at the time, it was obvious that the number of reviews increased
> significantly. It was just easier to do reviews that way. If you had 5 minutes, you
> could scan for a small change and give some feedback. The list of patches to-be-reviewed
> was never forgotten.
>
> But back then we didn't want to risk alienating global maintainers that didn't like gerrit or
> liked the e-mail system better, so we dropped that effort and put nothing back in its place. It feels
> to me patch reviewing by non-contributors lost some of the traction it had gained with gerrit.
Oops. I mean non-contributors -> non-maintainers.
>
> It is not a secret that some of us contributors would like to see improvements in this area, hence
> my suggestion to address patch reviewing/more maintainers/CI-based testing as topics for the GDB BoF.
>
> But at the end of the day, it's up to the global maintainers to make a decision on this topic, or to
> let contributors know they are open to adopting improvements.
>
> So, in summary, I see the proposal to add tags as a way to improve a patch reviewing system that
> is not being capable of keeping up with demand. I doubt we would need such tagging if we had a
> proper reviewing system in place (be it gerrit, patchworks or any other).
>
>>
>>> Assuming we will go through with this proposal, it will need to be
>>> documented on the wiki so we can easily refer people to the procedure.
>>> Probably the ContributionChecklist page?
>>>
>>> https://sourceware.org/gdb/wiki/ContributionChecklist
>>>
>>> Will you be able to take care of this when needed (do you have write
>>> access to the wiki)?
>>>
>>> In the mean time, message to others: please let us know if you agree
>>> with this, it's difficult to know we have the support of the community
>>> if everybody silently agrees!
>>
>
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: Proposal: Add review tags to patch review workflow.
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
1 sibling, 1 reply; 35+ messages in thread
From: Lancelot SIX @ 2022-09-27 9:38 UTC (permalink / raw)
To: Luis Machado; +Cc: Joel Brobecker, Simon Marchi via Gdb
On Tue, Sep 27, 2022 at 09:39:02AM +0100, Luis Machado via Gdb wrote:
> Hi Joel,
>
> Thanks for bringing this up.
>
> On 9/26/22 17:42, Joel Brobecker via Gdb wrote:
> > Just thinking out loud...
> >
> > > I completely agree with the proposal. I really like the fact that it
> > > makes communication less ambiguous. Following some process (or changing
> > > the process) can feel a bit heavy for long-timers, but I think it makes
> > > things much clearer for newcomers.
> >
> > Speaking of ambiguous, one thing that we used to do well in the past
> > but then kind of got worse was the subject prefix we used to use
> > to indicate the status of a patch. In particular, we used to reserve
> > certain keywords for that in the subject (e.g. "RFA" vs "PATCH", or
> > "OB" for obvious, etc). We lost that part, not sure exactly when,
> > but I suspect sometime when we transitionned to Git.
> >
> > Something else also that I have been feeling the last year or two
> > is that I'm not sure people now explicitly confirm to the list
> > when a patch is pushed.
>
> I think that's been happening, yes. But the IRC bot mentions commits explicitly, and
> developers tend to see updates in the git repo when they update the sources.
>
> With that said, in general the frequent GDB contributors tend to be quite busy (with
> GDB or other things), so I'm inclined to say it is positive to have less steps to
> take care of to push a change.
>
> For example, ChangeLog's were a big time sink, and we managed to get rid of that rule. I think
> that was very positive.
>
> We still have other potential improvements waiting to be discussed, like auto-formatting of code
> with some tool like clang-format. Time spent correcting formatting is not very useful.
>
> >
> > The reason I mention this is to show that perhaps we're getting back
> > to the fact that our email reviewing system is still email-based.
> > One way to address the various limitations is by adding more
> > processes, as suggested here. This has the good property of being
> > fairly cheap to discuss and implement, at the cost of a small
> > added overhead. I don't have a strong opinion about it, either
> > for or against (and given the amount of time I have to contribute
> > anyway, I don't think I should have a say).
> >
> > With that said, I have a feeling that switching to a system designed
> > to manage patch submissions and reviews, no matter imperfect, is going
> > to solve a lot of the limitations of the current email-based system.
> > So that's another option worth reviewing from time to time, I think.
> > I understand that selecting, deploying and trying new review systems
> > requires a fair amount of effort. But having seen the benefits of
> > using several different such systems, I am convinced that the gains
> > will be very much worth whatever the drawbacks of that system might be.
>
> That's a fairly good point, and I agree. We tried a patch reviewing system (gerrit)
> before. For me, at the time, it was obvious that the number of reviews increased
> significantly. It was just easier to do reviews that way. If you had 5 minutes, you
> could scan for a small change and give some feedback. The list of patches to-be-reviewed
> was never forgotten.
>
> But back then we didn't want to risk alienating global maintainers that didn't like gerrit or
> liked the e-mail system better, so we dropped that effort and put nothing back in its place. It feels
> to me patch reviewing by non-contributors lost some of the traction it had gained with gerrit.
>
> It is not a secret that some of us contributors would like to see improvements in this area, hence
> my suggestion to address patch reviewing/more maintainers/CI-based testing as topics for the GDB BoF.
>
> But at the end of the day, it's up to the global maintainers to make a decision on this topic, or to
> let contributors know they are open to adopting improvements.
>
> So, in summary, I see the proposal to add tags as a way to improve a patch reviewing system that
> is not being capable of keeping up with demand. I doubt we would need such tagging if we had a
> proper reviewing system in place (be it gerrit, patchworks or any other).
>
Hi,
As far as I understand, patchwork can see the R-b and T-b tags and
update patches metadata based on that. An up-to-date patchwork instance
is available for GDB
(https://patchwork.sourceware.org/project/gdb/list/) and old patches
have been archived. It is in a clean state if we want to try using it
(or re-try, I believe this have been tried in the past but there were
too much limitations at the time).
If R-b and T-b tags are being adopted, patchwork sounds like a good
candidate tool to help to monitor the patches pending review/approval.
What is still unknown to me at the moment is how much effort does it
take to keep patchwork up-to-date with events it fails to detect (like
patches merged with minor changes). I am currently experimenting
tracking patches with it (I think Simon is too). Nothing compulsory to
anyone of course. The tool is there for those who want. The pure ML
based workflow will not be impacted with this. Having Bruno's proposal
in would certainly help.
Lancelot.
> >
> > > Assuming we will go through with this proposal, it will need to be
> > > documented on the wiki so we can easily refer people to the procedure.
> > > Probably the ContributionChecklist page?
> > >
> > > https://sourceware.org/gdb/wiki/ContributionChecklist
> > >
> > > Will you be able to take care of this when needed (do you have write
> > > access to the wiki)?
> > >
> > > In the mean time, message to others: please let us know if you agree
> > > with this, it's difficult to know we have the support of the community
> > > if everybody silently agrees!
> >
>
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: Proposal: Add review tags to patch review workflow.
2022-09-27 8:06 ` Bruno Larsen
@ 2022-09-27 12:02 ` Simon Marchi
2022-09-27 12:03 ` Bruno Larsen
0 siblings, 1 reply; 35+ messages in thread
From: Simon Marchi @ 2022-09-27 12:02 UTC (permalink / raw)
To: Bruno Larsen, John Baldwin, gdb
>> It also wasn't clear to me if the intention was for the commits to
>> be amended with the annotations? (I don't think it was explicitly
>> stated in the original mail, and I'm not sure if it was an implicit
>> assumption?)
> No, I didn't intend on amending previous commits. The main problem this change intends to solve is fixing ambiguity, and the pushed patches don't have that issue anymore. Thanking a reviewer is just one more positive side (IMHO) going forward.
Err, just to be clear, we won't amend existing commits in master
obviously (can't rewrite history) but future commits in master would
contain those tags. This means that if you give me a Reviewed-By, I
amend my local commit patch to include that trailer before pushing.
Simon
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: Proposal: Add review tags to patch review workflow.
2022-09-27 7:58 ` Bruno Larsen
@ 2022-09-27 12:03 ` Simon Marchi
0 siblings, 0 replies; 35+ messages in thread
From: Simon Marchi @ 2022-09-27 12:03 UTC (permalink / raw)
To: Bruno Larsen, gdb
On 2022-09-27 03:58, Bruno Larsen via Gdb wrote:
>
> On 26/09/2022 15:55, Simon Marchi wrote:
>>
>> On 2022-09-21 07:04, Bruno Larsen via Gdb wrote:
>>> 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.
>> Hi Bruno,
>>
>> I completely agree with the proposal. I really like the fact that it
>> makes communication less ambiguous. Following some process (or changing
>> the process) can feel a bit heavy for long-timers, but I think it makes
>> things much clearer for newcomers.
>>
>> Assuming we will go through with this proposal, it will need to be
>> documented on the wiki so we can easily refer people to the procedure.
>> Probably the ContributionChecklist page?
>>
>> https://sourceware.org/gdb/wiki/ContributionChecklist
>>
>> Will you be able to take care of this when needed (do you have write
>> access to the wiki)?
>
> Hi Simon,
>
> Thanks for the reply! I don't have write access to the wiki (In fact, I just created my account on it), can you give me the necessary permissions, or do I need to ask someone else? (in case it is the latter, who?)
Done.
Simon
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: Proposal: Add review tags to patch review workflow.
2022-09-27 12:02 ` Simon Marchi
@ 2022-09-27 12:03 ` Bruno Larsen
2022-09-27 17:11 ` John Baldwin
0 siblings, 1 reply; 35+ messages in thread
From: Bruno Larsen @ 2022-09-27 12:03 UTC (permalink / raw)
To: Simon Marchi, John Baldwin, gdb
On 27/09/2022 14:02, Simon Marchi wrote:
>
>>> It also wasn't clear to me if the intention was for the commits to
>>> be amended with the annotations? (I don't think it was explicitly
>>> stated in the original mail, and I'm not sure if it was an implicit
>>> assumption?)
>> No, I didn't intend on amending previous commits. The main problem this change intends to solve is fixing ambiguity, and the pushed patches don't have that issue anymore. Thanking a reviewer is just one more positive side (IMHO) going forward.
> Err, just to be clear, we won't amend existing commits in master
> obviously (can't rewrite history) but future commits in master would
> contain those tags. This means that if you give me a Reviewed-By, I
> amend my local commit patch to include that trailer before pushing.
Yes, thank you for clarifying what I meant!
Cheers,
Bruno
> Simon
>
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: Proposal: Add review tags to patch review workflow.
2022-09-27 12:03 ` Bruno Larsen
@ 2022-09-27 17:11 ` John Baldwin
0 siblings, 0 replies; 35+ messages in thread
From: John Baldwin @ 2022-09-27 17:11 UTC (permalink / raw)
To: Bruno Larsen, Simon Marchi, gdb
On 9/27/22 5:03 AM, Bruno Larsen wrote:
> On 27/09/2022 14:02, Simon Marchi wrote:
>>
>>>> It also wasn't clear to me if the intention was for the commits to
>>>> be amended with the annotations? (I don't think it was explicitly
>>>> stated in the original mail, and I'm not sure if it was an implicit
>>>> assumption?)
>>> No, I didn't intend on amending previous commits. The main problem this change intends to solve is fixing ambiguity, and the pushed patches don't have that issue anymore. Thanking a reviewer is just one more positive side (IMHO) going forward.
>> Err, just to be clear, we won't amend existing commits in master
>> obviously (can't rewrite history) but future commits in master would
>> contain those tags. This means that if you give me a Reviewed-By, I
>> amend my local commit patch to include that trailer before pushing.
> Yes, thank you for clarifying what I meant!
Yes, my question was about amending the in-progress commits before
pushing, not existing history.
If we weren't amending commits then you could perhaps omit the e-mail
address (it's implicit from the From), but the full tag is better to
copy/paste into a commit log or git commit --trailer
--
John Baldwin
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: Proposal: Add review tags to patch review workflow.
2022-09-21 11:04 Proposal: Add review tags to patch review workflow Bruno Larsen
` (2 preceding siblings ...)
2022-09-26 15:59 ` Luis Machado
@ 2022-09-27 20:50 ` Thomas Schwinge
2022-10-07 7:49 ` Bruno Larsen
4 siblings, 0 replies; 35+ messages in thread
From: Thomas Schwinge @ 2022-09-27 20:50 UTC (permalink / raw)
To: Bruno Larsen; +Cc: gdb
Hi!
On 2022-09-21T13:04:47+0200, Bruno Larsen via Gdb <gdb@sourceware.org> wrote:
> 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.
Nice! (I haven't been contributing to GDB in many years, so my opinion
here only means so much.)
Five years ago, I had started a similar effort:
<https://inbox.sourceware.org/gdb/87zi9oj8rl.fsf@euler.schwinge.homeip.net>
'GNU Tools Cauldron 2017 follow up: "Reviewed-by" etc.'.
As that's where I spend most of my time, I then tried establishing that
in GCC land, see <https://gcc.gnu.org/wiki/Reviewed-by>. This didn't
catch on back then, so I also again seem to have stopped in mid-2020.
(But, I'm planning to be here for a lot more years, and maybe
eventually...) ;-)
This isn't meant to sound pessimistic or discouraging -- rather the
inverse: I'm happy to see that after glibc, where it's now an established
practice, another GNU Tools project, GDB, is considering this again.
(..., and I also do sympathize with those who are suggesting to use tools
more elaborate than email for doing reviews, but that seems even more far
out. But we'll get there eventually!) :-)
Grüße
Thomas
> * 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
-----------------
Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634 München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht München, HRB 106955
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: Proposal: Add review tags to patch review workflow.
2022-09-27 9:38 ` Lancelot SIX
@ 2022-09-27 21:07 ` Thiago Jung Bauermann
0 siblings, 0 replies; 35+ messages in thread
From: Thiago Jung Bauermann @ 2022-09-27 21:07 UTC (permalink / raw)
To: Lancelot SIX; +Cc: Luis Machado, Joel Brobecker, gdb
Hello,
Lancelot SIX via Gdb <gdb@sourceware.org> writes:
> On Tue, Sep 27, 2022 at 09:39:02AM +0100, Luis Machado via Gdb wrote:
>> So, in summary, I see the proposal to add tags as a way to improve a patch reviewing
>> system that
>> is not being capable of keeping up with demand. I doubt we would need such tagging if we
>> had a
>> proper reviewing system in place (be it gerrit, patchworks or any other).
>>
>
> Hi,
>
> As far as I understand, patchwork can see the R-b and T-b tags and
> update patches metadata based on that. An up-to-date patchwork instance
> is available for GDB
> (https://patchwork.sourceware.org/project/gdb/list/) and old patches
> have been archived. It is in a clean state if we want to try using it
> (or re-try, I believe this have been tried in the past but there were
> too much limitations at the time).
>
> If R-b and T-b tags are being adopted, patchwork sounds like a good
> candidate tool to help to monitor the patches pending review/approval.
>
> What is still unknown to me at the moment is how much effort does it
> take to keep patchwork up-to-date with events it fails to detect (like
> patches merged with minor changes). I am currently experimenting
> tracking patches with it (I think Simon is too). Nothing compulsory to
> anyone of course. The tool is there for those who want. The pure ML
> based workflow will not be impacted with this. Having Bruno's proposal
> in would certainly help.
FWIW I like Bruno's proposal, it would bring clarity to the patch review
and approval process. And as Lancelot mentioned, it also helps people
using patchwork.
Carlos O'Donell mentioned at GNU Tools Cauldron that patchwork has been
very helpful for him with managing glibc patches, and that he would
still use it even if no one else in that community did since it helps
him keep track of the review state of the patches.
--
Thiago
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: Proposal: Add review tags to patch review workflow.
2022-09-21 11:04 Proposal: Add review tags to patch review workflow Bruno Larsen
` (3 preceding siblings ...)
2022-09-27 20:50 ` Thomas Schwinge
@ 2022-10-07 7:49 ` Bruno Larsen
2022-10-07 20:46 ` Simon Marchi
4 siblings, 1 reply; 35+ messages in thread
From: Bruno Larsen @ 2022-10-07 7:49 UTC (permalink / raw)
To: blarsen, gdb
It's been over a week since the last response of this proposal, and all
responses have been positive. Does this mean the proposal is accepted?
There aren't any guidelines about proposals in the wiki, so I'm a bit
lost here.
--
Cheers,
Bruno
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: Proposal: Add review tags to patch review workflow.
2022-10-07 7:49 ` Bruno Larsen
@ 2022-10-07 20:46 ` Simon Marchi
2022-10-08 6:23 ` Eli Zaretskii
2022-10-10 9:39 ` Luis Machado
0 siblings, 2 replies; 35+ messages in thread
From: Simon Marchi @ 2022-10-07 20:46 UTC (permalink / raw)
To: Bruno Larsen, gdb
On 2022-10-07 03:49, Bruno Larsen via Gdb wrote:
> It's been over a week since the last response of this proposal, and all responses have been positive. Does this mean the proposal is accepted? There aren't any guidelines about proposals in the wiki, so I'm a bit lost here.
>
We don't really have a formal process for voting / adopting proposals
like this. Given there was no negative feedback at all (from what I
remember), please go ahead and update the wiki. I will try to remember
to use the tags from now on.
Simon
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: Proposal: Add review tags to patch review workflow.
2022-10-07 20:46 ` Simon Marchi
@ 2022-10-08 6:23 ` Eli Zaretskii
2022-10-08 11:55 ` Simon Marchi
2022-10-10 9:39 ` Luis Machado
1 sibling, 1 reply; 35+ messages in thread
From: Eli Zaretskii @ 2022-10-08 6:23 UTC (permalink / raw)
To: Simon Marchi; +Cc: blarsen, gdb
> Date: Fri, 7 Oct 2022 16:46:23 -0400
> From: Simon Marchi via Gdb <gdb@sourceware.org>
>
> We don't really have a formal process for voting / adopting proposals
> like this. Given there was no negative feedback at all (from what I
> remember), please go ahead and update the wiki. I will try to remember
> to use the tags from now on.
It would help if the new rules for using the tags were posted here as
well, I think. Then, if someone has questions or wants to request
clarifications, they could do that right away, instead of waiting
until the first time they need to use the new rules.
Thanks.
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: Proposal: Add review tags to patch review workflow.
2022-10-08 6:23 ` Eli Zaretskii
@ 2022-10-08 11:55 ` Simon Marchi
2022-10-08 12:44 ` Eli Zaretskii
0 siblings, 1 reply; 35+ messages in thread
From: Simon Marchi @ 2022-10-08 11:55 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: blarsen, gdb
On 2022-10-08 02:23, Eli Zaretskii wrote:
>> Date: Fri, 7 Oct 2022 16:46:23 -0400
>> From: Simon Marchi via Gdb <gdb@sourceware.org>
>>
>> We don't really have a formal process for voting / adopting proposals
>> like this. Given there was no negative feedback at all (from what I
>> remember), please go ahead and update the wiki. I will try to remember
>> to use the tags from now on.
>
> It would help if the new rules for using the tags were posted here as
> well, I think. Then, if someone has questions or wants to request
> clarifications, they could do that right away, instead of waiting
> until the first time they need to use the new rules.
They were explained in Bruno's original message, and Bruno will add them
to the wiki. Or maybe I don't understand what you mean.
Simon
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: Proposal: Add review tags to patch review workflow.
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
0 siblings, 2 replies; 35+ messages in thread
From: Eli Zaretskii @ 2022-10-08 12:44 UTC (permalink / raw)
To: Simon Marchi; +Cc: blarsen, gdb
> Date: Sat, 8 Oct 2022 07:55:00 -0400
> Cc: blarsen@redhat.com, gdb@sourceware.org
> From: Simon Marchi <simark@simark.ca>
>
>
>
> On 2022-10-08 02:23, Eli Zaretskii wrote:
> >> Date: Fri, 7 Oct 2022 16:46:23 -0400
> >> From: Simon Marchi via Gdb <gdb@sourceware.org>
> >>
> >> We don't really have a formal process for voting / adopting proposals
> >> like this. Given there was no negative feedback at all (from what I
> >> remember), please go ahead and update the wiki. I will try to remember
> >> to use the tags from now on.
> >
> > It would help if the new rules for using the tags were posted here as
> > well, I think. Then, if someone has questions or wants to request
> > clarifications, they could do that right away, instead of waiting
> > until the first time they need to use the new rules.
>
> They were explained in Bruno's original message, and Bruno will add them
> to the wiki.
They were followed by discussion and some changes, AFAIR. Also, the
original post was quite long, so I hoped for a concise version akin a
cookbook.
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: Proposal: Add review tags to patch review workflow.
2022-10-08 12:44 ` Eli Zaretskii
@ 2022-10-09 0:29 ` Simon Marchi
2022-10-10 9:27 ` Bruno Larsen
1 sibling, 0 replies; 35+ messages in thread
From: Simon Marchi @ 2022-10-09 0:29 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: gdb
> They were followed by discussion and some changes, AFAIR. Also, the
> original post was quite long, so I hoped for a concise version akin a
> cookbook.
I just re-read the thread and I didn't find any changes to the proposal
in the original post. Maybe some precisions. In any case, Bruno will
update the wiki, it should be a short and sweet section. It doesn't
need to explain all the rationale like the original post did.
Simon
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: Proposal: Add review tags to patch review workflow.
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 13:34 ` Pedro Alves
1 sibling, 2 replies; 35+ messages in thread
From: Bruno Larsen @ 2022-10-10 9:27 UTC (permalink / raw)
To: Eli Zaretskii, Simon Marchi; +Cc: gdb
On 08/10/2022 14:44, Eli Zaretskii wrote:
>> Date: Sat, 8 Oct 2022 07:55:00 -0400
>> Cc: blarsen@redhat.com, gdb@sourceware.org
>> From: Simon Marchi <simark@simark.ca>
>>
>>
>>
>> On 2022-10-08 02:23, Eli Zaretskii wrote:
>>>> Date: Fri, 7 Oct 2022 16:46:23 -0400
>>>> From: Simon Marchi via Gdb <gdb@sourceware.org>
>>>>
>>>> We don't really have a formal process for voting / adopting proposals
>>>> like this. Given there was no negative feedback at all (from what I
>>>> remember), please go ahead and update the wiki. I will try to remember
>>>> to use the tags from now on.
>>> It would help if the new rules for using the tags were posted here as
>>> well, I think. Then, if someone has questions or wants to request
>>> clarifications, they could do that right away, instead of waiting
>>> until the first time they need to use the new rules.
>> They were explained in Bruno's original message, and Bruno will add them
>> to the wiki.
> They were followed by discussion and some changes, AFAIR. Also, the
> original post was quite long, so I hoped for a concise version akin a
> cookbook.
>
As Simon mentioned, there weren't big changes, but here's a quick cookbook:
1. If you have the authority to approve a patch and believe the patch
you are reviewing is ready to be merged, add the following line to your
e-mail (usually at the end): Approved-by: Your Name <your_email@example.com>
2. If you don't have the authority to approve patches, or aren't
convinced that you know enough about the area of code to fully approve a
patch for merging, and haven't found any technical issues (i.e.
non-nitpicks) with the patch, add the following line to your e-mail:
Reviewed-by: Your Name <your_email@example.com>
3. If you aren't sure of the quality of the technical changes, but you
have tested and verified that the patch does not add any regressions,
add the following line to your e-mail: Tested-by: Your Name
<your_name@example.com>
I hope this clears up any confusion!
Cheers,
Bruno
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: Proposal: Add review tags to patch review workflow.
2022-10-07 20:46 ` Simon Marchi
2022-10-08 6:23 ` Eli Zaretskii
@ 2022-10-10 9:39 ` Luis Machado
1 sibling, 0 replies; 35+ messages in thread
From: Luis Machado @ 2022-10-10 9:39 UTC (permalink / raw)
To: Simon Marchi, Bruno Larsen, gdb
Hi Simon,
On 10/7/22 21:46, Simon Marchi via Gdb wrote:
>
>
> On 2022-10-07 03:49, Bruno Larsen via Gdb wrote:
>> It's been over a week since the last response of this proposal, and all responses have been positive. Does this mean the proposal is accepted? There aren't any guidelines about proposals in the wiki, so I'm a bit lost here.
>>
>
> We don't really have a formal process for voting / adopting proposals
> like this. Given there was no negative feedback at all (from what I
> remember), please go ahead and update the wiki. I will try to remember
> to use the tags from now on.
>
> Simon
Bruno can correct me if I'm wrong, but I think what us contributors are looking for is some sort of message from global maintainers
stating this is acceptable and we should move forward. Even better if the global maintainers can discuss this between
themselves and provide feedback to the rest of the contributors.
Without any formal position, it is unclear whether one should proceed with some proposal/improvement or not.
Does that sound reasonable?
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: Proposal: Add review tags to patch review workflow.
2022-10-10 9:27 ` Bruno Larsen
@ 2022-10-10 9:47 ` Eli Zaretskii
2022-10-10 10:11 ` Bruno Larsen
2022-10-10 13:34 ` Pedro Alves
1 sibling, 1 reply; 35+ messages in thread
From: Eli Zaretskii @ 2022-10-10 9:47 UTC (permalink / raw)
To: Bruno Larsen; +Cc: simark, gdb
> Date: Mon, 10 Oct 2022 11:27:03 +0200
> Cc: gdb@sourceware.org
> From: Bruno Larsen <blarsen@redhat.com>
>
> As Simon mentioned, there weren't big changes, but here's a quick cookbook:
Thanks!
> 1. If you have the authority to approve a patch and believe the patch
> you are reviewing is ready to be merged, add the following line to your
> e-mail (usually at the end): Approved-by: Your Name <your_email@example.com>
>
> 2. If you don't have the authority to approve patches, or aren't
> convinced that you know enough about the area of code to fully approve a
> patch for merging, and haven't found any technical issues (i.e.
> non-nitpicks) with the patch, add the following line to your e-mail:
> Reviewed-by: Your Name <your_email@example.com>
>
> 3. If you aren't sure of the quality of the technical changes, but you
> have tested and verified that the patch does not add any regressions,
> add the following line to your e-mail: Tested-by: Your Name
> <your_name@example.com>
I'm not clear what I should do when I approve just part of a patch.
It is frequently the case that a patch includes both code and
documentation, and I'm approving just the documentation part(s). Is
that item 1 or item 2? or something else?
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: Proposal: Add review tags to patch review workflow.
2022-10-10 9:47 ` Eli Zaretskii
@ 2022-10-10 10:11 ` Bruno Larsen
2022-10-10 11:27 ` Eli Zaretskii
0 siblings, 1 reply; 35+ messages in thread
From: Bruno Larsen @ 2022-10-10 10:11 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: simark, gdb
On 10/10/2022 11:47, Eli Zaretskii wrote:
>> Date: Mon, 10 Oct 2022 11:27:03 +0200
>> Cc: gdb@sourceware.org
>> From: Bruno Larsen <blarsen@redhat.com>
>>
>> As Simon mentioned, there weren't big changes, but here's a quick cookbook:
> Thanks!
>
>> 1. If you have the authority to approve a patch and believe the patch
>> you are reviewing is ready to be merged, add the following line to your
>> e-mail (usually at the end): Approved-by: Your Name <your_email@example.com>
>>
>> 2. If you don't have the authority to approve patches, or aren't
>> convinced that you know enough about the area of code to fully approve a
>> patch for merging, and haven't found any technical issues (i.e.
>> non-nitpicks) with the patch, add the following line to your e-mail:
>> Reviewed-by: Your Name <your_email@example.com>
>>
>> 3. If you aren't sure of the quality of the technical changes, but you
>> have tested and verified that the patch does not add any regressions,
>> add the following line to your e-mail: Tested-by: Your Name
>> <your_name@example.com>
> I'm not clear what I should do when I approve just part of a patch.
> It is frequently the case that a patch includes both code and
> documentation, and I'm approving just the documentation part(s). Is
> that item 1 or item 2? or something else?
>
It's a bit up to you, if I'm honest. I would default to telling you to
use Reviewed-by, to avoid confusion, but if you want to say that the
"documentation parts are Approved-by", I am fine with it.
Just let me know if you decide to go with the second, so I can mention
in the wiki something like "make sure all of your patch is approved
before pushing".
Cheers,
Bruno
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: Proposal: Add review tags to patch review workflow.
2022-10-10 10:11 ` Bruno Larsen
@ 2022-10-10 11:27 ` Eli Zaretskii
2022-10-10 12:31 ` Bruno Larsen
0 siblings, 1 reply; 35+ messages in thread
From: Eli Zaretskii @ 2022-10-10 11:27 UTC (permalink / raw)
To: Bruno Larsen; +Cc: simark, gdb
> Date: Mon, 10 Oct 2022 12:11:46 +0200
> Cc: simark@simark.ca, gdb@sourceware.org
> From: Bruno Larsen <blarsen@redhat.com>
>
> > I'm not clear what I should do when I approve just part of a patch.
> > It is frequently the case that a patch includes both code and
> > documentation, and I'm approving just the documentation part(s). Is
> > that item 1 or item 2? or something else?
> >
> It's a bit up to you, if I'm honest. I would default to telling you to
> use Reviewed-by, to avoid confusion, but if you want to say that the
> "documentation parts are Approved-by", I am fine with it.
>
> Just let me know if you decide to go with the second, so I can mention
> in the wiki something like "make sure all of your patch is approved
> before pushing".
I don't mind either way. This whole thing is a service to others, so
I'll do whatever people prefer. Let me just point out that my
situation is not too unique: several other maintainers can approve
only parts of patches.
Thanks.
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: Proposal: Add review tags to patch review workflow.
2022-10-10 11:27 ` Eli Zaretskii
@ 2022-10-10 12:31 ` Bruno Larsen
2022-10-10 13:14 ` Eli Zaretskii
0 siblings, 1 reply; 35+ messages in thread
From: Bruno Larsen @ 2022-10-10 12:31 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: simark, gdb
On 10/10/2022 13:27, Eli Zaretskii wrote:
>> Date: Mon, 10 Oct 2022 12:11:46 +0200
>> Cc: simark@simark.ca, gdb@sourceware.org
>> From: Bruno Larsen <blarsen@redhat.com>
>>
>>> I'm not clear what I should do when I approve just part of a patch.
>>> It is frequently the case that a patch includes both code and
>>> documentation, and I'm approving just the documentation part(s). Is
>>> that item 1 or item 2? or something else?
>>>
>> It's a bit up to you, if I'm honest. I would default to telling you to
>> use Reviewed-by, to avoid confusion, but if you want to say that the
>> "documentation parts are Approved-by", I am fine with it.
>>
>> Just let me know if you decide to go with the second, so I can mention
>> in the wiki something like "make sure all of your patch is approved
>> before pushing".
> I don't mind either way. This whole thing is a service to others, so
> I'll do whatever people prefer. Let me just point out that my
> situation is not too unique: several other maintainers can approve
> only parts of patches.
Ah, so I'll suggest that you approve the documentation changes, and I'll
mention that some approvers may sometimes only approve part of the
patch, so one should make sure the whole patch is approved before pushing.
Cheers,
Bruno
>
> Thanks.
>
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: Proposal: Add review tags to patch review workflow.
2022-10-10 12:31 ` Bruno Larsen
@ 2022-10-10 13:14 ` Eli Zaretskii
2022-10-10 13:26 ` Bruno Larsen
0 siblings, 1 reply; 35+ messages in thread
From: Eli Zaretskii @ 2022-10-10 13:14 UTC (permalink / raw)
To: Bruno Larsen; +Cc: simark, gdb
> Date: Mon, 10 Oct 2022 14:31:54 +0200
> Cc: simark@simark.ca, gdb@sourceware.org
> From: Bruno Larsen <blarsen@redhat.com>
>
> On 10/10/2022 13:27, Eli Zaretskii wrote:
> >> Date: Mon, 10 Oct 2022 12:11:46 +0200
> >> Cc: simark@simark.ca, gdb@sourceware.org
> >> From: Bruno Larsen <blarsen@redhat.com>
> >>
> >>> I'm not clear what I should do when I approve just part of a patch.
> >>> It is frequently the case that a patch includes both code and
> >>> documentation, and I'm approving just the documentation part(s). Is
> >>> that item 1 or item 2? or something else?
> >>>
> >> It's a bit up to you, if I'm honest. I would default to telling you to
> >> use Reviewed-by, to avoid confusion, but if you want to say that the
> >> "documentation parts are Approved-by", I am fine with it.
> >>
> >> Just let me know if you decide to go with the second, so I can mention
> >> in the wiki something like "make sure all of your patch is approved
> >> before pushing".
> > I don't mind either way. This whole thing is a service to others, so
> > I'll do whatever people prefer. Let me just point out that my
> > situation is not too unique: several other maintainers can approve
> > only parts of patches.
> Ah, so I'll suggest that you approve the documentation changes, and I'll
> mention that some approvers may sometimes only approve part of the
> patch, so one should make sure the whole patch is approved before pushing.
I'm not sure I understand: do you mean that I should not use _any_ tag
at all, when the patch includes more than just documentation?
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: Proposal: Add review tags to patch review workflow.
2022-10-10 13:14 ` Eli Zaretskii
@ 2022-10-10 13:26 ` Bruno Larsen
2022-10-10 15:25 ` Eli Zaretskii
0 siblings, 1 reply; 35+ messages in thread
From: Bruno Larsen @ 2022-10-10 13:26 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: simark, gdb
On 10/10/2022 15:14, Eli Zaretskii wrote:
>> Date: Mon, 10 Oct 2022 14:31:54 +0200
>> Cc: simark@simark.ca, gdb@sourceware.org
>> From: Bruno Larsen <blarsen@redhat.com>
>>
>> On 10/10/2022 13:27, Eli Zaretskii wrote:
>>>> Date: Mon, 10 Oct 2022 12:11:46 +0200
>>>> Cc: simark@simark.ca, gdb@sourceware.org
>>>> From: Bruno Larsen <blarsen@redhat.com>
>>>>
>>>>> I'm not clear what I should do when I approve just part of a patch.
>>>>> It is frequently the case that a patch includes both code and
>>>>> documentation, and I'm approving just the documentation part(s). Is
>>>>> that item 1 or item 2? or something else?
>>>>>
>>>> It's a bit up to you, if I'm honest. I would default to telling you to
>>>> use Reviewed-by, to avoid confusion, but if you want to say that the
>>>> "documentation parts are Approved-by", I am fine with it.
>>>>
>>>> Just let me know if you decide to go with the second, so I can mention
>>>> in the wiki something like "make sure all of your patch is approved
>>>> before pushing".
>>> I don't mind either way. This whole thing is a service to others, so
>>> I'll do whatever people prefer. Let me just point out that my
>>> situation is not too unique: several other maintainers can approve
>>> only parts of patches.
>> Ah, so I'll suggest that you approve the documentation changes, and I'll
>> mention that some approvers may sometimes only approve part of the
>> patch, so one should make sure the whole patch is approved before pushing.
> I'm not sure I understand: do you mean that I should not use _any_ tag
> at all, when the patch includes more than just documentation?
Sorry for making things even more confusing. I meant that you use
Approved-by tags.
What I was getting at was that the wording I have been using until this
point was "if you get any Approved-by, it is ready for pushing", and
that needs to be changed to "be sure that you have received Approved-by
for all the code, in case a maintainer only approved parts of your
change", or something of the sorts. Still working on that...
Cheers,
Bruno
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: Proposal: Add review tags to patch review workflow.
2022-10-10 9:27 ` Bruno Larsen
2022-10-10 9:47 ` Eli Zaretskii
@ 2022-10-10 13:34 ` Pedro Alves
1 sibling, 0 replies; 35+ messages in thread
From: Pedro Alves @ 2022-10-10 13:34 UTC (permalink / raw)
To: gdb
Hi!
On 2022-10-10 10:27 a.m., Bruno Larsen via Gdb wrote:
> 1. If you have the authority to approve a patch and believe the patch
> you are reviewing is ready to be merged, add the following line to your
> e-mail (usually at the end): Approved-by: Your Name
> <your_email@example.com>
>
> 2. If you don't have the authority to approve patches, or aren't
> convinced that you know enough about the area of code to fully approve a
> patch for merging, and haven't found any technical issues (i.e.
> non-nitpicks) with the patch, add the following line to your e-mail:
> Reviewed-by: Your Name <your_email@example.com>
>
> 3. If you aren't sure of the quality of the technical changes, but you
> have tested and verified that the patch does not add any regressions,
> add the following line to your e-mail: Tested-by: Your Name
> <your_name@example.com>
(Should not be a surprise since I said as much at the Cauldron, but,)
FAOD, I'm for giving this a try.
Pedro Alves
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: Proposal: Add review tags to patch review workflow.
2022-10-10 13:26 ` Bruno Larsen
@ 2022-10-10 15:25 ` Eli Zaretskii
0 siblings, 0 replies; 35+ messages in thread
From: Eli Zaretskii @ 2022-10-10 15:25 UTC (permalink / raw)
To: Bruno Larsen; +Cc: simark, gdb
> Date: Mon, 10 Oct 2022 15:26:33 +0200
> Cc: simark@simark.ca, gdb@sourceware.org
> From: Bruno Larsen <blarsen@redhat.com>
>
> >> Ah, so I'll suggest that you approve the documentation changes, and I'll
> >> mention that some approvers may sometimes only approve part of the
> >> patch, so one should make sure the whole patch is approved before pushing.
> > I'm not sure I understand: do you mean that I should not use _any_ tag
> > at all, when the patch includes more than just documentation?
>
> Sorry for making things even more confusing. I meant that you use
> Approved-by tags.
OK, thanks.
^ 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).