CONTRIBUTING: review and merge conventions

[wolfgangwalther]

While squash-merges have been discouraged from already, they become impossible to do when the Merge Queue is enabled. In certain scenarios it was tolerated to use these, however, thus we introduce an acceptable replacement instead.

By making explicit that pushing to contributors PRs is allowed by default, it should be easier for committers to do so.

Related:

• Merge Queues will be enabled via rulesets/nixpkgs: add merge queue for lints on master org#149
• Discussion about Squash and Rebase merges: nixpkgs: disable rebase and squash merges org#135

---

By making blocking comments explicit, it will be easier to discard certain nits.

Suggested in NixOS/org#122 (comment).

Things done

I put these into a single PR, because I introduce a single headline for them. I could also split these up, if either is controversial enough.

• Fits CONTRIBUTING.md, pkgs/README.md, maintainers/README.md and other READMEs.

---

Add a 👍 reaction to pull requests you find important.

[wolfgangwalther]

I addressed the first round of feedback, thanks everyone, so far! Hopefully I was able to balance everyone's requests.

fixed the link

[philiptaron]

I agree with the direction here; it's one I've long advocated, despite the real hurt feelings effect that the big ❌ produces. The alternative, being ambiguous about what's needed to move forward, is worse.

[numinit]

This is a long-awaited step toward optimistic merging in nixpkgs. 👍

(aside, looks like we have a majority of ✔️, can we get a supermajority? 😄)

[infinisil]

This is great, let's do it!

[wolfgangwalther]

Thanks for the feedback and constructive discussion everyone!

Now the question is how well the transition period works: Obviously older review comments have not been made with these rules in mind. Also, going forward, not everyone knows about this, yet.

Should we do some kind of announcement to make it easier to see? Maybe a pinned issue? Or maybe even... pinging all committers in here and then locking the thread?

[philiptaron]

Let's do something at NixCon.

[numinit]

Yes, and it has to be at least this bombastic. 😄

[emilazy]

I meant to comment on this PR before it was merged, but failed to allocate the time for it. (Not that I object to it! I think it is a definite overall improvement and any nits I might have relate to parts of our review process we don't really have strong consensus about at present.)

Instead, I ended up writing up a lot of related thoughts about the problems with our review culture and considerations about optimistic merging for Nixpkgs in NixOS/org#122 (comment). Linking that here to try and trick people into spending comparable amounts of time reading it as I did writing it :)

[philiptaron]

Thanks for writing that up, Emily @emilazy. Your position is nuanced and realistic. It's hard to hit all tradeoffs given what Nixpkgs is. The spectrum you outline from with lib (almost certainly not a bug, even it is it's pretty much fine) to overrideAttrs (brittle and better alternatives exists) to pkgsCross (few good alternatives, indicator of something substantially worse going on) is a good outline of the problem space in front of reviewers.

There's a real tone in your essay of despair at being able to find something that will cause both quick, effective reviews and merging and thorough, security-and-stability oriented review and merging. Is that what you meant to communicate emotionally or is there some nuance you'd lay on it?

[emilazy]

Hmm, I think I was aiming more for “apologetic” than “despairing”, because I do feel bad about the pain our review dynamic causes for contributors, and have thoughts about how we can improve that, while also thinking that we don’t have super obvious quick fixes that will systematically produce good review outcomes. So if despair slipped in, it was subconscious!

I would say that I can think of ways we can improve in individual cases, but can’t with confidence say I have a solution to the coordination problem of scaling that up to the entire project. Partly that’s because not everyone will agree with me on what our review process should look like and we have only informal norms around this (this PR starts to address that, which is great) – clearly some people of us value very proactive optimistic merges, some of us value reviews primarily oriented around style/best practices, and the main lever I have in convincing anyone to value what I value is writing walls of text about it :) The larger work of consensus‐building and governance around review culture is downstream of actually having written up coherent pictures of what we could do better, so it felt like a good time to put my own position on the topic out there.

I think the low-hanging fruit here are the things I talked about that can be done fairly unilaterally. Encouraging contributors to push back against requests for changes that are clearly inappropriate is a cheap thing to do that doesn’t require much coordination, but I realize that contributors may not feel empowered to do so and that it may not get their PR merged. Better automation can also have a huge effect here, which is why I am incredibly grateful for @wolfgangwalther’s work on CI. PRs like this that write up subsets of things we can get easy consensus on are also very valuable as we work towards more consistent, thoughtful norms.

I do think that review culture is something that compounds: the better we get at it, the more we encourage the kinds of contributors who will become great reviewers in future, and then maybe one day we’ll be arguing over whether we should risk two CVEs per decade instead of one to get our average time‐to‐merge down from three to two minutes or something. So even taking the easy wins and avoiding the obvious pitfalls here has a lot of value for the project.