[docs] Update docs on code-review process

[banach-space]

Clarify what to do in cases where multiple reviewers commented on a
patch, but only one reviewer explicitly accepted.

This is mostly to standardize what the expectations are as folks tend to
approach this differently.

[banach-space]

Follow-on from a discussion here: #111533

My assumption is that this is documenting what many of us already practice, so hopefully this is nothing particularly new. Please let me know if you feel otherwise.

Any suggestion who else to include in the review?

[nikic]

This document is just an overview, we have detailed documentation of the code review policy in https://llvm.org/docs/CodeReview.html, which is also already linked.

In particular, https://llvm.org/docs/CodeReview.html#lgtm-how-a-patch-is-accepted explains that it is the responsibility of the approving reviewer to consider other feedback on the patch and indicate if their approval is conditional.

[joker-eph]

Instead of adding more text here, I think we should trim it down and link to the dedicated page: https://llvm.org/docs/CodeReview.html

Right now you're evolving the text in a direction that contradicts what this other page is saying I believe.

[banach-space]

I think we should trim it down and link to the dedicated page

Agreed, I think that repeating similar information in multiple places might be confusing. I went ahead and deleted a paragraph from Contributing.rst to avoid that.

[banach-space]

My bad for not noticing https://llvm.org/docs/CodeReview.html, I didn't realise there were two documents (though https://llvm.org/docs/Contributing.html#how-to-submit-a-patch felt weirdly unfamiliar).

I've reverted the changes in Contributing.rst and instead added a paragraph to CodeReview.rst.

[joker-eph]

The second part of the sentence is a bit strongly worded, what do you mean to cover here?

[kuhar]

+1, I don't think we want to effectively require a re-approval after every push.
For example, when a new reviewer comes and add a typo/nit comment, I'd expect that the author can fix that and merge based on the previous approval given how insignificant the comment (and subsequent fix) is. I think this is a standard practice today. Could we use a wording that's less prescriptive to allow for this?

[banach-space]

I've expanded this paragraph. @jakub, hopefully that meets your criteria (which I mostly agree with).

what do you mean to cover here?

What should happen when there's a "LGTM", but then another reviewer leaves new comments afterwards? I wanted to clarify that the original "LGTM" wouldn't apply to the new comments (unless, like Jakub points out, those would be basic nits etc).

For example, when a new reviewer comes and add a typo/nit comment, I'd expect that the author can fix that and merge based on the previous approval given how insignificant the comment (and subsequent fix) is.

This is assuming that there won't be more substantial comments following the nits - is it a valid assumption? I guess the reviewer should make that clear, e.g. "Just some nits, no need to wait for my approval".

[joker-eph]

What should happen when there's a "LGTM", but then another reviewer leaves new comments afterwards? I wanted to clarify that the original "LGTM" wouldn't apply to the new comments (unless, like Jakub points out, those would be basic nits etc).

OK, that's the first part of the sentence "these need to be addressed ", but I don't see how "all new changes have to be reviewed " is addressing what you describe here ("another reviewer leaves new comments afterwards").

[banach-space]

but I don't see how "all new changes have to be reviewed " is addressing what you describe here ("another reviewer leaves new comments afterwards").

That's been updated since :)

[joker-eph]

LG overall, I have one comment though, thanks!

[kuhar]

To follow the style above

Suggested change

	Exceptions apply - e.g. there's no need to confirm that a comment requesting a
	Exceptions apply, e.g., there's no need to confirm that a comment requesting a

[kuhar]

I'd make it less specific to allow for some wiggle room we've traditionally had

Suggested change

	typo to be fixed has been addressed (this should be evident from the code).
	minor issue (like a typo or other 'nit') to be fixed has been addressed (this should be evident from the code).

[preames]

This advise talks about action the reviewer should take, that seems backwards here. Unless I'm misreading, the problematic situation here is a prior LGTM followed by comments by a different reviewer and the author landing the patch without addressing those comments. (edit: for typo fix)

I'd suggest something along the lines of the following:
Only approval from a single reviewer is required; however the patch author is always responsible for making sure that all feedback on a review has been properly addressed or responded to. If additional feedback is provided after acceptance (by the same reviewer or another), the author should use their best judgement in deciding whether that feedback can be incorporated into the change without comment (say a typo) or requires further review discussion. Similarly, the patch author is responsible for incorporating post commit feedback on the change.

I'd put this paragraph just above the one talking about unqualified LGTM

[banach-space]

Unless I'm misreading, the problematic situation here is a prior LGTM followed by comments by a different reviewer and the author landing the patch without addressing those comments.

Yes

(edit: for typo fix)

No, for more substantial changes (edit: comments). For "typos", I added an exception.

If additional feedback is provided after acceptance (by the same reviewer or another), the author should use their best judgement in deciding whether that feedback can be incorporated into the change without comment (say a typo) or requires further review discussion.

To me this sounds that any comments "post LGTM" are "second category" (i.e. "for the author to decide whether to address them or not" vs "need to be addressed"). I'm suggesting that we should treat such comments as any other comments prior to LGTM (i.e., "need to be addressed"). I feel that that otherwise we are discouraging people from reading/commenting on patches post LGTM.

[preames]

Unless I'm misreading, the problematic situation here is a prior LGTM followed by comments by a different reviewer and the author landing the patch without addressing those comments.

Yes

(edit: for typo fix)

No, for more substantial changes. For "typos", I added an exception.

(You're responding to a note I made about editting my text to fix a typo...)

If additional feedback is provided after acceptance (by the same reviewer or another), the author should use their best judgement in deciding whether that feedback can be incorporated into the change without comment (say a typo) or requires further review discussion.

To me this sounds that any comments "post LGTM" are "second category" (i.e. "for the author to decide whether to address them or not" vs "need to be addressed"). I'm suggesting that we should treat such comments as any other comments prior to LGTM (i.e., "need to be addressed"). I feel that that otherwise we are discouraging people from reading/commenting on patches post LGTM.

If you have suggested wording, feel free to suggest. I don't read this the way you do, particular following the prior sentence that you trimmed.

[banach-space]

Hi @preames , thanks for that suggestion and sorry for the delay - I had to step away and re-think this a bit. I do want to incorporate your feedback, but also want to make sure we avoid duplication and keep this document succinct. So I trimmed your suggestion a bit as per the comments below (hope this won't come across as nit-picking).

Point 1:

[From Phillip's suggestion]
Only approval from a single reviewer is required;

This point is already mentioned earlier in the document:

llvm-project/llvm/docs/CodeReview.rst

Lines 148 to 150 in c2063de

	A patch is approved to be committed when a reviewer accepts it, and this is
	almost always associated with a message containing the text "LGTM" (which
	stands for Looks Good To Me). Only approval from a single reviewer is required.

Point 2:

[From Phillip's suggestion]
however the patch author is always responsible for making sure that all feedback on a review has been properly addressed or responded to.

Isn't this a contradicting one existing policy?

llvm-project/llvm/docs/CodeReview.rst

Lines 152 to 159 in c2063de

	When providing an unqualified LGTM (approval to commit), it is the
	responsibility of the reviewer to have reviewed all of the discussion and
	feedback from all reviewers ensuring that all feedback has been addressed and
	that all other reviewers will almost surely be satisfied with the patch being
	approved. If unsure, the reviewer should provide a qualified approval, (e.g.,
	"LGTM, but please wait for @someone, @someone_else"). You may also do this if
	you are fairly certain that a particular community member will wish to review,
	even if that person hasn't done so yet.

Specifically:

it is the responsibility of the reviewer to have reviewed all of the discussion and feedback from all reviewers ensuring that all feedback has been addressed and that all other reviewers will almost surely be satisfied with the patch being approved.

I'd just leave this out.

Point 3:

[From Phillip's suggestion]
Similarly, the patch author is responsible for incorporating post commit feedback on the change.

This is already covered here:

llvm-project/llvm/docs/CodeReview.rst

Lines 46 to 49 in c2063de

	Post-commit review is encouraged, and can be accomplished using any of the
	tools detailed below. There is a strong expectation that authors respond
	promptly to post-commit feedback and address it. Failure to do so is cause for
	the patch to be :ref:`reverted <revert_policy>`.

Point 4:

Which leaves this bit from Phillip's original suggestion (emphasis mine):

[From Phillip's suggestion]
If additional feedback is provided after acceptance (by the same reviewer or another), the author should use their best judgement in deciding whether that feedback can be incorporated into the change without comment (say a typo) or requires further review discussion.

This a bit softer requirement compared to what I added "these need to be addressed" and sounds more along the lines of what @kuhar would prefer? Let me use that, but let me add small clarification at the end. Let me know what you think.

[preames]

You have seemingly managed to complete miss the spirit of my original suggestion, and are misreading the current policy in my view. If the document implies that the author is not responsible for incorporating feedback (because that's only the reviewers responsibility), the document is simply wrong. I don't read it that way, but if you do, stop and fix that wording before working on this smaller item.

[banach-space]

You have seemingly managed to complete miss the spirit of my original suggestion

That's not intentional. Would you be able to share a diff so that I better understand what you had in mind?

If the document implies that the author is not responsible for incorporating feedback

I don’t believe the document implies that. I think it’s fairly clear that:

1. The author is responsible for addressing/incorporating the feedback, which is already covered under "Acknowledge All Reviewer Feedback" (so perhaps we can leave that as is).
2. The reviewer, when posting an unqualified LGTM, is responsible for ensuring that all feedback has been addressed (i.e., “I've reviewed all comments and confirm they’ve been addressed").

The existing wording covers those points well. The part that is unclear IMO is what happens after LGTM and before commit when new comments are posted. Hopefully, the additional paragraph I proposed clarifies that.

[nikic]

I feel like this has cut out too much. E.g. I think the "ping" convention should still be mentioned here. This is quite relevant to new contributors, who otherwise don't need to read the full scope of the code review documentation.

[banach-space]

I can leave it if you feel strongly about it, but IMO this is duplicating information and that can be confusing. Sometimes less is more and also - why not encourage people to read our policy instead? it's an important document and it's there for a reason.

[banach-space]

I'm keen to progress this, so I've just restored that note. Thanks!

[banach-space]

Gentle ping :)

[joker-eph]

Still LGTM, but we should hear back from @nikic / @preames.

[kuhar]

LGTM % nit

[banach-space]

Ping @nikic and @preames - are you OK with me landing this in its current form? Thanks!

[nikic]

LGTM

[banach-space]

Hey folks, thank you for all the reviews! If there are no new comments, I will land this tomorrow.