next-node is the new merge (ends PLT-558)

CONTRIBUTING.adoc

@@ -307,17 +307,43 @@ Code review is great (see below), but it can slow you down if you don't take the
 
 The amount of time it's worth spending doing this is probably much more than you think.
 
-==== What changes to include
+==== What branch to target
 
-Having a sensible and comprehensible set of changes makes your reviewer's life much easier.
+There are two protected branches, `main` and `next-node`.
+PRs should generally target the `main` branch, unless the change is only applicable to `next-node`.
 
-* Keep commits to a single logical change where possible.
-The reviewer will be happier, and you'll be happier if you ever have to revert it.
-If you *can't* do this (say because you have a huge mess), best to just have one commit with everything in it.
-* Keep your PRs to a single topic.
-Including unrelated changes makes things harder for your reviewers, slowing them down, and makes it harder to integrate new changes.
-* If you're working on something that's likely to conflict with someone else, talk to them.
-It's not a race.
+==== What changes to include, and pull request sizes
+
+When developing a non-trivial new feature, usually the best way to get the code reviewed is to break the implementation down to a chain of small diffs, each representing a meaningful, logical and reviewable step.
+Unfortunately GitHub doesn't have good support for this.
+You basically have three options:
+
+- Open the first PR against `main`, the second PR against the first PR's branch, and so on.
+  Merging a stack of PRs created this way into `main` can be non-trivial.
+- Wait until one PR is merged before opening the next PR.
+- Use a single PR for the whole feature that contains multiple small commits.
+  The problem is that Github doesn't support approving, rejecting or merging individual commits in a PR.
+  You can look at each individual commit, but it's not necessarily useful or even appropriate - many PRs have quite messy commits, and commits are sometimes overwritten via force push.
+
+The first two options are often referred to as ["trunk-based development"](https://trunkbaseddevelopment.com/), while the third "long-lived feature branches".
+There is no single best option for all cases, although in general we encourage adopting trunk-based development styles.
+Long-lived feature branches with too many commits are harmful because
+
+1. they are difficult to review - the PR can be quite large, and it is hard to review it incrementally;
+2. it can be difficult to resolve merge conflicts;
+3. they make it more likely that other people need to depend on your unmerged changes.
+
+It is fine to have partially implemented features or not well-tested features in `main`.
+You can simply not turn them on until they are ready, or guard them with conditinal flags.
+
+But this is not a hard rule and should be determined on a case-by-case basis.
+Sometimes for a small or medium-sized piece of work, you may not want to break it into multiple PRs, and wait till each PR is merged before creating the next one.
+You'd rather put all your code out quickly in a single PR for review.
+And that's fine.
+Or maybe it's a piece of performance improvement work, and you don't know whether or not it actually improves the performance, until you finish implementing and testing the whole thing.
+
+Whichever option you choose, please keep each of your PR to a single topic.
+Do not mix business logic with such things as reformatting and refactoring in a single PR.
 
 ==== Pull request descriptions
 
@@ -336,25 +362,34 @@ It's amazing how many obvious things you spot here, and it stops the reviewer ha
 * It's fine to make WIP PRs if you just want to show your code to someone else or have the CI check it.
 Use the Github "draft" feature for this.
 
-=== Rebasing, force-pushing, and history
+=== Rebasing and force pushing
+
+Force pushing to `main` or `next-node` is never allowed.
+There is no exception to this rule.
 
-Until a PR is merged, the branch is yours to do with as you will.
-In particular, rebasing and force-pushing is fine.
+Rebasing and force pushing to other branches you own is fine, even when you have an open PR on the branch.
 Indeed, if you need to update your branch with changes from main, rebasing is typically better than merging.
 
-So please do use this ability where it helps, for example:
+Some projects do not allow force pushing to any remote branch.
+This is not a popular policy and we do not adopt it, because
+
+- This means you must only ever use the "merge commit" merge method (or occasionally, fast forward merge, which GitHub doesn't support).
+- This means you aren't even allowed to clean up commits in your own PR, and must eventually merge everything into `main`.
+  It discourages people from pushing commits frequently when developing.
+  We should instead _encourage_ cleaning up commits in PRs, at least before merging.
+- The argument that this will cause massive pain for those who merge other people's PR branch into their branch is questionable.
+  This should be rare to begin with, if we adopt trunk-based development in general, instead of long-lived feature branches.
+  And even if you do need to depend on other people's unmerged work, you can instead rebase your branch on theirs, and if their branch changes, just rebase again.
+
+Rebasing and force pushing can be used to your advantage, for example:
 
 * Add low-effort or WIP commits to fix review comments, and then squash them away before merging the PR.
 * If you have already had a PR review, don't rebase away the old commits until the PR is ready to merge, so that the reviewer only has to look at the "new" commits.
 * Rewrite the commits to make the story clearer where possible.
-* Always prefer `git push --force-with-lease` to just `git push --force` to ensure that no work gets accidentally deleted.
-
-Don't be obsessive about history though: a little bit of effort making the history clear is nice, but you can rapidly hit diminishing returns.
-Use your judgement, but probably don't merge a PR that has commits called "WIP" or "fix"!
 
-If a PR is just a total mess, consider using Github's squash-merge feature.
+It is advisable to always prefer `git push --force-with-lease` instead of `git push --force` to ensure that no work gets accidentally deleted.
 
-=== Code review and merging
+=== Code review
 
 All pull-requests should be approved by at least one other person.
 We don't enforce this, though: a PR fixing a typo is fine to self-merge, beyond that use your judgement.
@@ -389,6 +424,37 @@ Read these blog posts for more good tips:
 - https://mtlynch.io/human-code-reviews-1/
 - https://mtlynch.io/human-code-reviews-2/
 
+=== Merging PRs
+
+==== Merge method and commit history
+
+This repo allows two merge methods: squash and merge, and rebase and merge.
+Use the one you deem appropriate.
+As said before, sometimes people use a single PR with multiple commits for their work; other times they create multiple small PRs.
+The best merge method is different for different cases.
+
+The "rebase and merge" method should be used with caution.
+If you use this method, your PR must have a clean commit history: every commit should have a meaningful message, and should be buildable.
+You don't want to have commits like "fix a typo", "this may work" or "wip, done for the day" in `main` with a linear history.
+And if some of these commits are non-buildable, it can create problems for "git bisect".
+
+==== Beware divergence of `main` and PR branch
+
+Merging a PR can break `main`, if the PR branch has diverged from `main`, even if CI on the PR is green.
+This happens because the PRs conflict in a way that isn’t obvious to git, e.g. one adds a usage of a function and the other removes that function.
+The problems with a broken `main` include inconveniencing other developers, and causing problems for "git bisect".
+There are ways to guarantee `main` never breaks, such as GitHub's [merge queue](https://docs.github.com/en/repositories/configuring-branches-and-merges-in-your-repository/configuring-pull-request-merges/managing-a-merge-queue).
+
+We don't use the merge queue because
+
+- A broken `main` has historically been quite infrequent.
+- The merge queue increases the time it takes to merge a PR, which causes productivity loss if you are waiting to create the next PR after merging the current one (which happens often).
+
+However, if your PR branch has diverged too much from `main`, it is recommended that you rebase or merge `main` into the PR branch before merging.
+And whenever you notice a broken `main`, please fix it ASAP.
+
+The same holds for `next-node` if your PR targets `next-node`.
+
 == Supporting systems
 
 === Continuous integration
@@ -406,11 +472,6 @@ Pull requests cannot be merged without at least the Hydra CI check being green.
 NOTE: This isn't strictly true: repository admins (notably Michael) can force-merge PRs without the checks being green
 If you really need this, ask.
 
-CI checks are run on the tip of the PR branch, not on the merge commit that is created with main.
-As a result, it's possible to create a "semantic" merge commit where the CI passes on commits C1 and C2, but not on the merge of C1 and C2.
-In this circumstance we can end up with the CI checks being broken on main.
-However, this is sufficiently infrequent that we just live with the possibility, since eliminating it is quite awkward.
-
 ==== Hydra
 
 Hydra is the "standard" CI builder for Nix-based projects.