[Docs] Make code review policy clearer about requested pre-commit reviews

[jdoerfert]

Summary

The current wording already urges for pre-commit code reviews in case of
uncertainty. In addition, we now make it clear that the bar to assume
likely-community-consensus, and to do unreviewed commits in general,
is significantly higher if pre-commit reviews were requested in a
certain area.

[jdoerfert]

Add comma

[hubert.reinterpretcast]

llvm/docs/CodeReview.rst

33

If I understand this correctly, this is meant to cover situations where reviewers are active in an area and indicated an interest in reviewing basically everything. This seems redundant to likely-community-consensus. If someone is active in an area of the code, they would already know which reviewers are active and the area-specific culture. If someone is not active in an area of the code, the likely-community-consensus requirement should be enough to cause them to request a review.

Perhaps a clarification that "the likely-community-consensus requirement may vary depending on the specific area of the code" is enough. Or perhaps, "likely-community-consensus is not restricted to the content of the patch, but also includes the review process and culture".

[jdoerfert]

llvm/docs/CodeReview.rst

33

If I understand this correctly, this is meant to cover situations where reviewers are active in an area and indicated an interest in reviewing basically everything.

Pretty much, yes.

This seems redundant to likely-community-consensus. If someone is active in an area of the code, they would already know which reviewers are active and the area-specific culture. If someone is not active in an area of the code, the likely-community-consensus requirement should be enough to cause them to request a review.

I would have agreed with you but this is in practice unfortunately not true.

Perhaps a clarification that "the likely-community-consensus requirement may vary depending on the specific area of the code" is enough. Or perhaps, "likely-community-consensus is not restricted to the content of the patch, but also includes the review process and culture".

I'm fine with either. At the end of the day this is supposed to make it clear that reviews cannot be circumvented while pointing to this rule if it is clear reviews were requested.

[arsenm]

llvm/docs/CodeReview.rst

33

Typo reviewes, and commited

[hubert.reinterpretcast]

llvm/docs/CodeReview.rst

33

I'd suggest a process of checking for Herald rules if I knew of an easy way to know how Herald would react for a commit without posting a review. I don't know of such a way though.

Suggestion for wording:
The likely-community-consensus is not restricted to the content of the patch but also includes the review process and culture, which can vary by context. A strong pre-commit culture in an area of the code should be respected.

[lattner]

I don't really understand what this is getting at, so I'd recommend rewording this a bit for clarity. Would something along the lines of this capture the intended meaning?:

If there is likely to be uncertainty, you should default to getting a patch reviewed prior to commit, particularly if someone has asked for extra review of a specific area.

[mehdi_amini]

This seems like both over-fitting the general rule for a specific case and fuzzy/imprecise enough that it isn't clear to me how much useful it is in practice?

Maybe we could step back: what is the concrete problem that this intends to address? Is there an area where it has been problematic at the moment?

[jhenderson]

My impression is that you shouldn't be making changes in code you are not already familiar with without a review, even if the fix seems fairly obvious (it's surprising how often an "obvious" fix isn't actually the right thing to do). I'd have assumed this was covered by "any uncertainty" already. Presumably to get familiarity with said area of code, you'd already have prior art in getting/giving reviews of a given area, so would know the local culture?

[rengolin]

+1 to Chris, Mehdi and John comments. I think I get the idea, but the text is certainly not conveying that, for me.

[jdoerfert]

Use @lattner 's wording which is more specific and has less spelling errors.

[jdoerfert]

I did like @lattner 's solution so I use that now. Wdyt?

[mehdi_amini]

I am still not sure what "if someone has asked for extra review of a specific area" refers to?

[jdoerfert]

In D77683#1970826, @mehdi_amini wrote:

I am still not sure what "if someone has asked for extra review of a specific area" refers to?

As said earlier

If I understand this correctly, this is meant to cover situations where reviewers are active in an area and indicated an interest in reviewing basically everything.

Pretty much, yes.

this should mean that, if requested, all non-trivial patches should go through review. The current wording is very lenient, especially wrt. code owners.
While most people I talked to don't see owners as special per se (but just assume they have more responsibility), this paragraph says they have special rights.
Given the murky ways owners are "selected", I think we should have a well defined way to limit these rights without revoking the status.

[mehdi_amini]

But this sentence alone at the end of this paragraph does not solve anything to me: how is one know that someone in an "area" (whatever that means) is expected to review all the "non-trivial" patches pre-commit?

I don't necessarily disagree with your underlying intent, I just feel that adding this sentence here won't help and does not fit in an existing "process".

And you're adding this after "If there is likely to be uncertainty, you should default to getting a patch reviewed prior to commit", I don't see why the addition is useful here.

[dblaikie]

In D77683#1973757, @jdoerfert wrote:

In D77683#1970826, @mehdi_amini wrote:

I am still not sure what "if someone has asked for extra review of a specific area" refers to?

As said earlier

If I understand this correctly, this is meant to cover situations where reviewers are active in an area and indicated an interest in reviewing basically everything.

Pretty much, yes.

this should mean that, if requested, all non-trivial patches should go through review. The current wording is very lenient, especially wrt. code owners.
While most people I talked to don't see owners as special per se (but just assume they have more responsibility), this paragraph says they have special rights.
Given the murky ways owners are "selected", I think we should have a well defined way to limit these rights without revoking the status.

I disagree here. I think it's suitable (within the way the LLVM project works) for code owners to commit without review - given they are the arbiters of what's acceptable in that subcomponent - ultimately they can veto anyone else (short of a broader project/code owner). Doesn't usually come to that, but that's what the ownership role means.

I don't think it's correct for arbitrary contributors to say "you need to/this component needs review-before-commit" - the code owner could say that if they really don't trust any of the contributors to conform to the direction they have in mind for the component (that's not to discredit the contributors - but that's the point of pre-commit review: to ensure it conforms to the code owners vision for the component).

(yes, code owners aren't meant to be dictators - but they're ultimately the final decider)

[hfinkel]

In D77683#1973762, @dblaikie wrote:

In D77683#1973757, @jdoerfert wrote:

In D77683#1970826, @mehdi_amini wrote:

I am still not sure what "if someone has asked for extra review of a specific area" refers to?

As said earlier

If I understand this correctly, this is meant to cover situations where reviewers are active in an area and indicated an interest in reviewing basically everything.

Pretty much, yes.

this should mean that, if requested, all non-trivial patches should go through review. The current wording is very lenient, especially wrt. code owners.
While most people I talked to don't see owners as special per se (but just assume they have more responsibility), this paragraph says they have special rights.
Given the murky ways owners are "selected", I think we should have a well defined way to limit these rights without revoking the status.

I disagree here. I think it's suitable (within the way the LLVM project works) for code owners to commit without review - given they are the arbiters of what's acceptable in that subcomponent - ultimately they can veto anyone else (short of a broader project/code owner). Doesn't usually come to that, but that's what the ownership role means.

I don't think it's correct for arbitrary contributors to say "you need to/this component needs review-before-commit" - the code owner could say that if they really don't trust any of the contributors to conform to the direction they have in mind for the component (that's not to discredit the contributors - but that's the point of pre-commit review: to ensure it conforms to the code owners vision for the component).
privledges
(yes, code owners aren't meant to be dictators - but they're ultimately the final decider)

I disagree that this characterizes our conception of the role of code owner within the project, but there is some mitigating context. In my experience, we describe code owners as "reviewers of last resort", and their primary responsibility is to prevent review stagnation with their component (including from new/infrequent contributors whose patches might otherwise go unreviewed). The code owner is not the final decider in all cases, but can serve as a tie breaker when community consensus is unclear. That's an important role, because it ensures our ability to make forward progress, but is different in character from someone with overriding final authority. The code owners vision matters only slightly if the community consensus is for a different vision. There certainly are parts of LLVM that are developed by one person, and that person is the code owner, and thus excessive a lot of influence over the future direction of the component. Patches are often contributed without pre-commit review, in this case, and the code owner is often the most-skilled reviewer for any other patches as well. However, this state exists only at the pleasure of the rest of the community - we all see these patches on the commits list, and should more people become involved and request a more-inclusive pre-commit review cycle, that's what should happen. For components actively developed by many people, code owners have relatively minor privileges in this regard.

I suppose that we never wrote this down anywhere, but when I became code owner for the AA subsystem, we had an understanding that, because AA is so pervasive and subtle, even as code owner I would never commit anything (aside from reverts or similar) without pre-commit review. IMHO, this is the only responsible way to handle this subsystem. Not all subsystems are so risky, so there's appropriately a spectrum of development practices (some favoring velocity over stability), but I still view this as being more democratic than hierarchically authoritative.

[dblaikie]

In D77683#1978070, @hfinkel wrote:

In D77683#1973762, @dblaikie wrote:

In D77683#1973757, @jdoerfert wrote:

In D77683#1970826, @mehdi_amini wrote:

I am still not sure what "if someone has asked for extra review of a specific area" refers to?

As said earlier

If I understand this correctly, this is meant to cover situations where reviewers are active in an area and indicated an interest in reviewing basically everything.

Pretty much, yes.

this should mean that, if requested, all non-trivial patches should go through review. The current wording is very lenient, especially wrt. code owners.
While most people I talked to don't see owners as special per se (but just assume they have more responsibility), this paragraph says they have special rights.
Given the murky ways owners are "selected", I think we should have a well defined way to limit these rights without revoking the status.

I disagree here. I think it's suitable (within the way the LLVM project works) for code owners to commit without review - given they are the arbiters of what's acceptable in that subcomponent - ultimately they can veto anyone else (short of a broader project/code owner). Doesn't usually come to that, but that's what the ownership role means.

I don't think it's correct for arbitrary contributors to say "you need to/this component needs review-before-commit" - the code owner could say that if they really don't trust any of the contributors to conform to the direction they have in mind for the component (that's not to discredit the contributors - but that's the point of pre-commit review: to ensure it conforms to the code owners vision for the component).
privledges
(yes, code owners aren't meant to be dictators - but they're ultimately the final decider)

I disagree that this characterizes our conception of the role of code owner within the project, but there is some mitigating context. In my experience, we describe code owners as "reviewers of last resort", and their primary responsibility is to prevent review stagnation with their component (including from new/infrequent contributors whose patches might otherwise go unreviewed). The code owner is not the final decider in all cases, but can serve as a tie breaker when community consensus is unclear. That's an important role, because it ensures our ability to make forward progress, but is different in character from someone with overriding final authority. The code owners vision matters only slightly if the community consensus is for a different vision. There certainly are parts of LLVM that are developed by one person, and that person is the code owner, and thus excessive a lot of influence over the future direction of the component. Patches are often contributed without pre-commit review, in this case, and the code owner is often the most-skilled reviewer for any other patches as well. However, this state exists only at the pleasure of the rest of the community - we all see these patches on the commits list, and should more people become involved and request a more-inclusive pre-commit review cycle, that's what should happen. For components actively developed by many people, code owners have relatively minor privileges in this regard.

I suppose that we never wrote this down anywhere, but when I became code owner for the AA subsystem, we had an understanding that, because AA is so pervasive and subtle, even as code owner I would never commit anything (aside from reverts or similar) without pre-commit review. IMHO, this is the only responsible way to handle this subsystem. Not all subsystems are so risky, so there's appropriately a spectrum of development practices (some favoring velocity over stability), but I still view this as being more democratic than hierarchically authoritative.

Fair enough - it sounded like the instigating incident in this case was maybe the other end of the spectrum - one developer telling another developer that they had to send more things for review when neither were code owner & they seem to disagree on that line. I'm not sure that's something we could/should codify. I think if there's ongoing disagreement about the suitable norms in part of the project it'd probably be useful for a code owner (or at least some precedence from a collection of developers in that space) to help codify practices/norms in that component.

[jdoerfert]

The problem I want to address is with code owners. TBH, I would like our overall description of that role to be (more) like @hfinkel described it. In case there is no controversy, that is not really different from @dblaikie vision (I think). If there is controversy, a growing community in which there might not be "the one active person" anymore, or some other "interesting" situations arises, I don't think we should not have these "special rights" without checks. This is particularly important as there is no process of changing the code owner (that I know of). If I play devils advocate, I'd say the situation described by @dblaikie prevents a growing community to change directions as the code owner has final say and the rest has to follow. I don't think hat is the intend, neither of @dblaikie nor the community.

@mehdi_amini I understand your concerns. I actually think you might be right, this sentence might not be the way to go. As mentioned above, I'd like us to have more documentation on the role of code owners in general. What does this mean, how are things changed, ... if we get those, and they address the situation I try to resolve via our review policy, I'd like that far better than this "solution".

llvm/docs/CodeReview.rst

 
 Must Code Be Reviewed Prior to Being Committed?
 -----------------------------------------------
 
 Code can be reviewed either before it is committed or after. We expect
 significant patches to be reviewed before being committed. Smaller patches
 (or patches where the developer owns the component) that meet
 likely-community-consensus requirements (as apply to all patch approvals) can
-be committed prior to an explicit review. In situations where there is any
-uncertainty, a patch should be reviewed prior to being committed.
+be committed prior to an explicit review. If there is likely to be uncertainty,
+you should default to getting a patch reviewed prior to commit, particularly if
+someone has asked for extra review of a specific area.
 
 Please note that the developer responsible for a patch is also
 responsible for making all necessary review-related changes, including
 those requested during any post-commit review.
 
 Can Code Be Reviewed After It Is Committed?
 -------------------------------------------