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

[Hal Finkel via Phabricator via llvm-commits]

hfinkel added a comment.

In D77683#1973762 <https://reviews.llvm.org/D77683#1973762>, @dblaikie wrote:

> In D77683#1973757 <https://reviews.llvm.org/D77683#1973757>, @jdoerfert wrote:
>
> > In D77683#1970826 <https://reviews.llvm.org/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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77683/new/

https://reviews.llvm.org/D77683