Creating a tag to track partial reviews: "Consulted:"

[Carlos O'Donell]

On 4/28/25 8:39 AM, Carlos O'Donell wrote:
> Community,
> 
> Joseph Myers raised a good point that someone might review all but the
> final version of a series, or patch, but the final approval and
> Reviewed-by: is given by someone else, and thus we don't end up thanking
> someone who might have done the bulk of the review work that makes the
> final RB easy.
> 
> In the vein of "Reviewed-by:" I would like to add a "Consulted:" tag to
> indicate that this developer was consulted in the creation of the patch
> and involved in review of some aspect of the work, but it did not cross
> the line into being completely acceptable.
> 
> This is different from "Acked-by:" which is acceptance of inclusion for
> a subsystem e.g. like "Reviewed-by:" but only for a part of the patch,
> and "CC:" since this is a passive "this person was notified."
> 
> The goal of "Consulted:" is to give credit to partial reviews.
> 
> The description looks like this:
> ~~~
> Consultant’s statement of oversight
> 
> By offering my Consulted: tag, I state that:
> 
> * I have carried out at least a minimal amount of technical review of this
>    patch to evaluate its appropriateness and readiness for inclusion.
> 
> * Any problems, concerns, or questions relating to the patch have been
>    communicated back to the submitter.
> 
> * I am NOT yet satisfied with the submitter’s response to my comments, or
>    the technical content of the patch.
> 
> * I would like to be consulted in future discussions about the patch, but
>    I recognize that the patch may move forward without my explicit approval
>    and follow the normative development rules of the project.

Joseph's earlier comments on this tag:
~~~
That's not true in lots of cases - if a review concerns one specific issue
with the patch, then once that issue is resolved, the reviewer may not
have ongoing interest in being consulted on subsequent revisions where the
review deals with different issues.
~~~

The granularity of notification is always going to be per patch in a series
since the tags are per-patch in the series.

I do identify here that we are mixing concerns, namely that of notification
and involvement, and that of approval, which are two distinct operations.

We have two questions:
  * To whom should revisions of the patch go to?
  * Who should I thank for partial reviews?

My opinion is this:

  * If you approve of a portion of the patch, and do not want further
    involvement then convert your Consulted: to Acked-by: since you are
    now partially approving the patch.

  * If you do not yet approve the whole patch and want to continue to be
    involved give a Consulted: and stay involved.

  * If you don't want to be further involved, respond to the thread and
    ask for the Consulted: to be dropped.

Note: The kernel uses "CC:" for weaker recording of whom to contact.

Joseph, Does using Acked-by: solve the partial approval issue?

Again, the goal is to thank partial reviewers, and Acked-by: along with
Consulted: serves that purpose.

There is a *secondary* goal here, as Consulted: allows the Monday Morning
Patch Queue review to *easily find* potential reviewers for new versions
of the patch based on old reviewers rather than tracking the mailing list
data. Yes, this adds burden to the patch submitter, but it also improves
the likelihood that we find and get reviews.
  
> A Consulted tag is a statement of opinion that the patch is not yet an
> appropriate modification of the project. Any interested reviewer
> (who has done the work) can offer a Consulted tag for a patch. This tag
> serves to give credit to partial reviews and to inform maintainers of the
> degree of review which has been done on the patch. Consulted tags, when
> supplied by reviewers known to understand the subject area and to perform
> thorough reviews, will normally increase the likelihood of your patch
> getting into the project.
> 
> If a reviewer completes a review then they should send a Reviewed-by tag
> which supersedes a Consulted tag.
> ~~~
> 
> Thoughts?
> 

DJ's earlier comments:

(1) Is it superfluous?

- No. You have to opt-in with a tag rather than just assume an email does that.

(2) It adds extra work for patch authors?

- Yes it does, and this part makes it annoying, but with tooling like b4 we should
   be able to reduce the cost? Note that the cost is balanced against our desire
   to thank reviewers and provide future reviewers for the same patch.

(3) What about opposition to consensus tag?

- This is a distinct topic. A tag for "hard no" would indeed be useful, since we
   identify that in our process e.g. Rejected-by: would require two things:
   (a) validity of the rejection (concerned interest or not) and (b) subsequently
   resolving it (you can resolve it even if (a) is not from a concerned interest).

(4) Is this discussion inconsistent with our rule to remove personal attribution?

- You raise a good question. No, the rule we enacted was to remove the
   attribution from the source files themselves in *favour* of the attribution
   existing in the source control system. Therefore I'd say we continue to be
   consistent in that we want git to contain the attribution of changes, reviews,
   and partial reviews.

(5) How do we protect against abuse?

- Process, particularly open and inclusive process like this, doesn't protect
   against abuse, particularly in this case where you would need proof of work
   (though I attempt to show my work in my reviews). We need humans to look at
   what's going on to prevent abuse. A patch submitter, or human reviewer
   would call out the bot, and we'd have a discussion.

- ... OK so we *could* invent a gpg-signed process with a web of trust, but
   I don't want to go there.

(6) Do you really just want an MR/PR forge process?

- The MR/PR forge process records assigned reviewers, their approval, rejection,
   or if they did nothing. We might be able to write logic that turns this into
   a "thank you"

	approval = thank you
	rejection = thank you
	assigned reviewer but no comments on the MR/PR = nothing
	assigned reviewer and yes comments on the MR/PR = thank you

- The MR/PR forge process keeps version to version changes in one MR/PR
   (so long as the submitter doesn't close and open a new MR/PR). This helps
   us find future reviewers since they stay engaged in one place.

- Migrating our workflow to a forge is even more complicated than the current CTI,
   Sourceware, and hosting discussions because it touches on project governance,
   infrastructure governance, and consensus decision making for what features are
   needed *before* the migration could happen. You are right though, that a forge
   could meet the requirements I set out here, but it's a much larger project.

In summary to your comments DJ:

- I'm not opposed to a "Rejected-by:" tag, but it's a distinct proposal.
- Moving towards a forge workflow is distinct from this proposal.

Did I answer, or at least review, all of your questions?

-- 
Cheers,
Carlos.