[PATCH] D89995: Make the post-commit review expectations more explicit with respect to revert

Mon Oct 26 11:08:27 PDT 2020

rengolin added a comment.

In D89995#2354061 <https://reviews.llvm.org/D89995#2354061>, @nhaehnle wrote:

> Not everybody is necessarily aware of a 4 year old email thread;

That email isn't policy. It's the attempt to encode behaviour that was "common" at the time. The LLVM community doesn't create policy on emails, we only encode how we behave after a consensus is formed if that's a good idea to promote further or not.

> where in reality, things weren't actually done according to what's written in the policy documents.

Don't conflate your experiences with "reality". Reality is what the thousands of us who work on LLVM have experienced, from different groups, companies, countries, cultures. LLVM is so large these days that it's entirely possible that people only interact with a small subset of the community that experiences it in a similar way as themselves.

I don't know why Sean's proposal didn't go through. It could have been because there was no consensus (which would promote your view) or just that someone drop the ball and it hasn't happened since.

The important thing, however, is that:

1. LLVM behaviours are not *all* encoded in policy documents. Some of them are just things we do and no one has problems with it so we just let it be.
2. We only encode into policy things that become contentious (like this review), and for that to be encoded, needs consensus (which we still don't seem to have reached).
3. Every policy, every document, every behaviour will invariably encode the majority, not the exceptions. That's why we don't like strong policies, because that makes it hard for us to accept worthy exceptions.

> What's written in the documents needs to matter, if only as part of making LLVM a welcoming and inclusive community. Hidden tribal knowledge is the opposite of those ideals.

I absolutely agree with this statement. I personally know Mehdi for a while and Sean for even longer and I'm sure neither of them want to propagate hidden knowledge. The very reason for Sean's original email and Mehdi's review proposal is to do exactly the opposite: to elucidate what our behaviour is, and reach consensus on its merits.

> Back to the proposal itself: The problem I'm seeing (with post-commit review in general, but especially with this change) is that it exacerbates the dysfunction and arbitrariness of the LLVM review process. That's a practical problem because erring too much on the side of stasis blocks progress on dependent work, especially if that dependent work requires collaboration between multiple developers.

Post-commit review isn't going away, nor should be discussed here in this review. This is a fact of the size and scope of LLVM and the alternatives are far worse. Let's just accept it as a fact for now and discuss this review.

Having an arbitrary review process is *NOT* dysfunctional. It's just arbitrary. I would even say it's evolutionary. We do this because that's what the constraints have led us, and we have discussed alternatives and that's the best we could come up with.

Working upstream is *a lot* slower than downstream, and this is another fact that we cannot change, nor we should discuss here. It's a side-effect of having far too many people involved with widely different points of views, goals and responsibilities.

This means that you are responsible for pushing your changes with a lot more effort than if it was just a local project, but it also means that, once you do, it has a much larger, industry wide impact.

> I think this needs a solution. That solution can be a separate discussion, but I think it ought to be adopted before making the problem worse. I will try to find some time to write this up.

You have exposed many issues, not all of them "problems". I'd advise caution before you start a long unwinding thread about many things. We can change small things much faster than big ones.

More importantly, I don't think the facts that upstream development are slower and messier than downstream or that LLVM's policies are arbitrary or that LLVM has strong post-commit review process will change soon, if at all, and no amount of discussions or policies will speed that up.

Not because it's the status-quo, not because the maintainers "said-so", not because "I know better". But because this is where we (all) got after discussing those topics non-stop for (almost?) two decades...

================
Comment at: llvm/docs/CodeReview.rst:54-55
+patch author, this is inherent also to our post-commit review practices.
+Reverting a patch ensures that design discussions can happen without blocking
+other development; it's entirely possible the patch will end up being reapplied
+essentially as is once concerns have been resolved.
----------------
nhaehnle wrote:
> This is incorrect. Reverting a patch can in practice block other development more than not reverting it. Please just remove this statement.
What this means to convey is that there are far more people working on other stuff than any one of us are working on our stuff. Perhaps it needs to be rewritten, but the idea is valid.

If a patch is still in discussion, even if it remains in the tree, than it would be disingenuous for people to commit other depending patches on top of that, so essentially, the discussion process will effectively lock your progress no matter where the patch is.

But this is the process that all code reviews follow, upstream or downstream. It's the responsibility of the authors to show the value in their patches, not the other way round.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89995