Notes on using git (#7496)

* general updates to CONTRIBUTING.md
* notes on updating your PR
* Notes on squash-merging or otherwise
* document git branching model

CONTRIBUTING.md

@@ -1,19 +1,20 @@
-# Contributing code to Matrix
+# Contributing code to Synapse
 
-Everyone is welcome to contribute code to Matrix
+Everyone is welcome to contribute code to [matrix.org
-(https://github.com/matrix-org), provided that they are willing to license
+projects](https://github.com/matrix-org), provided that they are willing to
-their contributions under the same license as the project itself. We follow a
+license their contributions under the same license as the project itself. We
-simple 'inbound=outbound' model for contributions: the act of submitting an
+follow a simple 'inbound=outbound' model for contributions: the act of
-'inbound' contribution means that the contributor agrees to license the code
+submitting an 'inbound' contribution means that the contributor agrees to
-under the same terms as the project's overall 'outbound' license - in our
+license the code under the same terms as the project's overall 'outbound'
-case, this is almost always Apache Software License v2 (see [LICENSE](LICENSE)).
+license - in our case, this is almost always Apache Software License v2 (see
+[LICENSE](LICENSE)).
 
 ## How to contribute
 
-The preferred and easiest way to contribute changes to Matrix is to fork the
+The preferred and easiest way to contribute changes is to fork the relevant
-relevant project on github, and then [create a pull request](
+project on github, and then [create a pull request](
-https://help.github.com/articles/using-pull-requests/) to ask us to pull
+https://help.github.com/articles/using-pull-requests/) to ask us to pull your
-your changes into our repo.
+changes into our repo.
 
 **The single biggest thing you need to know is: please base your changes on
 the develop branch - *not* master.**
@@ -28,35 +29,31 @@ use github's pull request workflow to review the contribution, and either ask
 you to make any refinements needed or merge it and make them ourselves. The
 changes will then land on master when we next do a release.
 
-We use [Buildkite](https://buildkite.com/matrix-dot-org/synapse) for continuous
+Some other things you will need to know when contributing to Synapse:
-integration. If your change breaks the build, this will be shown in GitHub, so
-please keep an eye on the pull request for feedback.
 
-To run unit tests in a local development environment, you can use:
+ * Please follow the [code style requirements](#code-style).
 
-- ``tox -e py35`` (requires tox to be installed by ``pip install tox``)
+ * Please include a [changelog entry](#changelog) with each PR.
-  for SQLite-backed Synapse on Python 3.5.
-- ``tox -e py36`` for SQLite-backed Synapse on Python 3.6.
-- ``tox -e py36-postgres`` for PostgreSQL-backed Synapse on Python 3.6
-  (requires a running local PostgreSQL with access to create databases).
-- ``./test_postgresql.sh`` for PostgreSQL-backed Synapse on Python 3.5
-  (requires Docker). Entirely self-contained, recommended if you don't want to
-  set up PostgreSQL yourself.
 
-Docker images are available for running the integration tests (SyTest) locally,
+ * Please [sign off](#sign-off) your contribution.
-see the [documentation in the SyTest repo](
+
-https://github.com/matrix-org/sytest/blob/develop/docker/README.md) for more
+ * Please keep an eye on the pull request for feedback from the [continuous
-information.
+   integration system](#continuous-integration-and-testing) and try to fix any
+   errors that come up.
+
+ * If you need to [update your PR](#updating-your-pull-request), just add new
+   commits to your branch rather than rebasing.
 
 ## Code style
 
-All Matrix projects have a well-defined code-style - and sometimes we've even
+Synapse's code style is documented [here](docs/code_style.md). Please follow
-got as far as documenting it... For instance, synapse's code style doc lives
+it, including the conventions for the [sample configuration
-[here](docs/code_style.md).
+file](docs/code_style.md#configuration-file-format).
 
-To facilitate meeting these criteria you can run `scripts-dev/lint.sh`
+Many of the conventions are enforced by scripts which are run as part of the
-locally. Since this runs the tools listed in the above document, you'll need
+[continuous integration system](#continuous-integration-and-testing). To help
-python 3.6 and to install each tool:
+check if you have followed the code style, you can run `scripts-dev/lint.sh`
+locally. You'll need python 3.6 or later, and to install a number of tools:
 
 ```
 # Install the dependencies
@@ -67,9 +64,11 @@ pip install -U black flake8 flake8-comprehensions isort
 ```
 
 **Note that the script does not just test/check, but also reformats code, so you
-may wish to ensure any new code is committed first**. By default this script
+may wish to ensure any new code is committed first**.
-checks all files and can take some time; if you alter only certain files, you
+
-might wish to specify paths as arguments to reduce the run-time:
+By default, this script checks all files and can take some time; if you alter
+only certain files, you might wish to specify paths as arguments to reduce the
+run-time:
 
 ```
 ./scripts-dev/lint.sh path/to/file1.py path/to/file2.py path/to/folder
@@ -82,7 +81,6 @@ Please ensure your changes match the cosmetic style of the existing project,
 and **never** mix cosmetic and functional changes in the same commit, as it
 makes it horribly hard to review otherwise.
 
-
 ## Changelog
 
 All changes, even minor ones, need a corresponding changelog / newsfragment
@@ -98,24 +96,55 @@ in the format of `PRnumber.type`. The type can be one of the following:
 * `removal` (also used for deprecations)
 * `misc` (for internal-only changes)
 
-The content of the file is your changelog entry, which should be a short
+This file will become part of our [changelog](
-description of your change in the same style as the rest of our [changelog](
+https://github.com/matrix-org/synapse/blob/master/CHANGES.md) at the next
-https://github.com/matrix-org/synapse/blob/master/CHANGES.md). The file can
+release, so the content of the file should be a short description of your
-contain Markdown formatting, and should end with a full stop (.) or an
+change in the same style as the rest of the changelog. The file can contain Markdown
-exclamation mark (!) for consistency.
+formatting, and should end with a full stop (.) or an exclamation mark (!) for
+consistency.
 
 Adding credits to the changelog is encouraged, we value your
 contributions and would like to have you shouted out in the release notes!
 
 For example, a fix in PR #1234 would have its changelog entry in
-`changelog.d/1234.bugfix`, and contain content like "The security levels of
+`changelog.d/1234.bugfix`, and contain content like:
-Florbs are now validated when received over federation. Contributed by Jane
-Matrix.".
 
-## Debian changelog
+> The security levels of Florbs are now validated when received
+> via the `/federation/florb` endpoint. Contributed by Jane Matrix.
+
+If there are multiple pull requests involved in a single bugfix/feature/etc,
+then the content for each `changelog.d` file should be the same. Towncrier will
+merge the matching files together into a single changelog entry when we come to
+release.
+
+### How do I know what to call the changelog file before I create the PR?
+
+Obviously, you don't know if you should call your newsfile
+`1234.bugfix` or `5678.bugfix` until you create the PR, which leads to a
+chicken-and-egg problem.
+
+There are two options for solving this:
+
+ 1. Open the PR without a changelog file, see what number you got, and *then*
+    add the changelog file to your branch (see [Updating your pull
+    request](#updating-your-pull-request)), or:
+
+ 1. Look at the [list of all
+    issues/PRs](https://github.com/matrix-org/synapse/issues?q=), add one to the
+    highest number you see, and quickly open the PR before somebody else claims
+    your number.
+
+    [This
+    script](https://github.com/richvdh/scripts/blob/master/next_github_number.sh)
+    might be helpful if you find yourself doing this a lot.
+
+Sorry, we know it's a bit fiddly, but it's *really* helpful for us when we come
+to put together a release!
+
+### Debian changelog
 
 Changes which affect the debian packaging files (in `debian`) are an
-exception.
+exception to the rule that all changes require a `changelog.d` file.
 
 In this case, you will need to add an entry to the debian changelog for the
 next release. For this, run the following command:
@@ -200,19 +229,45 @@ Git allows you to add this signoff automatically when using the `-s`
 flag to `git commit`, which uses the name and email set in your
 `user.name` and `user.email` git configs.
 
-## Merge Strategy
+## Continuous integration and testing
 
-We use the commit history of develop/master extensively to identify
+[Buildkite](https://buildkite.com/matrix-dot-org/synapse) will automatically
-when regressions were introduced and what changes have been made.
+run a series of checks and tests against any PR which is opened against the
+project; if your change breaks the build, this will be shown in GitHub, with
+links to the build results. If your build fails, please try to fix the errors
+and update your branch.
 
-We aim to have a clean merge history, which means we normally squash-merge
+To run unit tests in a local development environment, you can use:
-changes into develop. For small changes this means there is no need to rebase
-to clean up your PR before merging. Larger changes with an organised set of
-commits may be merged as-is, if the history is judged to be useful.
 
-This use of squash-merging will mean PRs built on each other will be hard to 
+- ``tox -e py35`` (requires tox to be installed by ``pip install tox``)
-merge. We suggest avoiding these where possible, and if required, ensuring
+  for SQLite-backed Synapse on Python 3.5.
-each PR has a tidy set of commits to ease merging.
+- ``tox -e py36`` for SQLite-backed Synapse on Python 3.6.
+- ``tox -e py36-postgres`` for PostgreSQL-backed Synapse on Python 3.6
+  (requires a running local PostgreSQL with access to create databases).
+- ``./test_postgresql.sh`` for PostgreSQL-backed Synapse on Python 3.5
+  (requires Docker). Entirely self-contained, recommended if you don't want to
+  set up PostgreSQL yourself.
+
+Docker images are available for running the integration tests (SyTest) locally,
+see the [documentation in the SyTest repo](
+https://github.com/matrix-org/sytest/blob/develop/docker/README.md) for more
+information.
+
+## Updating your pull request
+
+If you decide to make changes to your pull request - perhaps to address issues
+raised in a review, or to fix problems highlighted by [continuous
+integration](#continuous-integration-and-testing) - just add new commits to your
+branch, and push to GitHub. The pull request will automatically be updated.
+
+Please **avoid** rebasing your branch, especially once the PR has been
+reviewed: doing so makes it very difficult for a reviewer to see what has
+changed since a previous review.
+
+## Notes for maintainers on merging PRs etc
+
+There are some notes for those with commit access to the project on how we
+manage git [here](docs/dev/git.md).
 
 ## Conclusion
 

docs/dev/git.md

@@ -0,0 +1,148 @@
+Some notes on how we use git
+============================
+
+On keeping the commit history clean
+-----------------------------------
+
+In an ideal world, our git commit history would be a linear progression of
+commits each of which contains a single change building on what came
+before. Here, by way of an arbitrary example, is the top of `git log --graph
+b2dba0607`:
+
+<img src="git/clean.png" alt="clean git graph" width="500px">
+
+Note how the commit comment explains clearly what is changing and why. Also
+note the *absence* of merge commits, as well as the absence of commits called
+things like (to pick a few culprits):
+[“pep8”](https://github.com/matrix-org/synapse/commit/84691da6c), [“fix broken
+test”](https://github.com/matrix-org/synapse/commit/474810d9d),
+[“oops”](https://github.com/matrix-org/synapse/commit/c9d72e457),
+[“typo”](https://github.com/matrix-org/synapse/commit/836358823), or [“Who's
+the president?”](https://github.com/matrix-org/synapse/commit/707374d5d).
+
+There are a number of reasons why keeping a clean commit history is a good
+thing:
+
+ * From time to time, after a change lands, it turns out to be necessary to
+   revert it, or to backport it to a release branch. Those operations are
+   *much* easier when the change is contained in a single commit.
+
+ * Similarly, it's much easier to answer questions like “is the fix for
+   `/publicRooms` on the release branch?” if that change consists of a single
+   commit.
+
+ * Likewise: “what has changed on this branch in the last week?” is much
+   clearer without merges and “pep8” commits everywhere.
+
+ * Sometimes we need to figure out where a bug got introduced, or some
+   behaviour changed. One way of doing that is with `git bisect`: pick an
+   arbitrary commit between the known good point and the known bad point, and
+   see how the code behaves. However, that strategy fails if the commit you
+   chose is the middle of someone's epic branch in which they broke the world
+   before putting it back together again.
+
+One counterargument is that it is sometimes useful to see how a PR evolved as
+it went through review cycles. This is true, but that information is always
+available via the GitHub UI (or via the little-known [refs/pull
+namespace](https://help.github.com/en/github/collaborating-with-issues-and-pull-requests/checking-out-pull-requests-locally)).
+
+
+Of course, in reality, things are more complicated than that. We have release
+branches as well as `develop` and `master`, and we deliberately merge changes
+between them. Bugs often slip through and have to be fixed later. That's all
+fine: this not a cast-iron rule which must be obeyed, but an ideal to aim
+towards.
+
+Merges, squashes, rebases: wtf?
+-------------------------------
+
+Ok, so that's what we'd like to achieve. How do we achieve it?
+
+The TL;DR is: when you come to merge a pull request, you *probably* want to
+“squash and merge”:
+
+![squash and merge](git/squash.png).
+
+(This applies whether you are merging your own PR, or that of another
+contributor.)
+
+“Squash and merge”<sup id="a1">[1](#f1)</sup> takes all of the changes in the
+PR, and bundles them into a single commit. GitHub gives you the opportunity to
+edit the commit message before you confirm, and normally you should do so,
+because the default will be useless (again: `* woops typo` is not a useful
+thing to keep in the historical record).
+
+The main problem with this approach comes when you have a series of pull
+requests which build on top of one another: as soon as you squash-merge the
+first PR, you'll end up with a stack of conflicts to resolve in all of the
+others. In general, it's best to avoid this situation in the first place by
+trying not to have multiple related PRs in flight at the same time. Still,
+sometimes that's not possible and doing a regular merge is the lesser evil.
+
+Another occasion in which a regular merge makes more sense is a PR where you've
+deliberately created a series of commits each of which makes sense in its own
+right. For example: [a PR which gradually propagates a refactoring operation
+through the codebase](https://github.com/matrix-org/synapse/pull/6837), or [a
+PR which is the culmination of several other
+PRs](https://github.com/matrix-org/synapse/pull/5987). In this case the ability
+to figure out when a particular change/bug was introduced could be very useful.
+
+Ultimately: **this is not a hard-and-fast-rule**. If in doubt, ask yourself “do
+each of the commits I am about to merge make sense in their own right”, but
+remember that we're just doing our best to balance “keeping the commit history
+clean” with other factors.
+
+Git branching model
+-------------------
+
+A [lot](https://nvie.com/posts/a-successful-git-branching-model/)
+[of](http://scottchacon.com/2011/08/31/github-flow.html)
+[words](https://www.endoflineblog.com/gitflow-considered-harmful) have been
+written in the past about git branching models (no really, [a
+lot](https://martinfowler.com/articles/branching-patterns.html)). I tend to
+think the whole thing is overblown. Fundamentally, it's not that
+complicated. Here's how we do it.
+
+Let's start with a picture:
+
+![branching model](git/branches.jpg)
+
+It looks complicated, but it's really not. There's one basic rule: *anyone* is
+free to merge from *any* more-stable branch to *any* less-stable branch at
+*any* time<sup id="a2">[2](#f2)</sup>. (The principle behind this is that if a
+change is good enough for the more-stable branch, then it's also good enough go
+put in a less-stable branch.)
+
+Meanwhile, merging (or squashing, as per the above) from a less-stable to a
+more-stable branch is a deliberate action in which you want to publish a change
+or a set of changes to (some subset of) the world: for example, this happens
+when a PR is landed, or as part of our release process.
+
+So, what counts as a more- or less-stable branch? A little reflection will show
+that our active branches are ordered thus, from more-stable to less-stable:
+
+ * `master` (tracks our last release).
+ * `release-vX.Y.Z` (the branch where we prepare the next release)<sup
+   id="a3">[3](#f3)</sup>.
+ * PR branches which are targeting the release.
+ * `develop` (our "mainline" branch containing our bleeding-edge).
+ * regular PR branches.
+
+The corollary is: if you have a bugfix that needs to land in both
+`release-vX.Y.Z` *and* `develop`, then you should base your PR on
+`release-vX.Y.Z`, get it merged there, and then merge from `release-vX.Y.Z` to
+`develop`. (If a fix lands in `develop` and we later need it in a
+release-branch, we can of course cherry-pick it, but landing it in the release
+branch first helps reduce the chance of annoying conflicts.)
+
+---
+
+<b id="f1">[1]</b>: “Squash and merge” is GitHub's term for this
+operation. Given that there is no merge involved, I'm not convinced it's the
+most intuitive name. [^](#a1)
+
+<b id="f2">[2]</b>: Well, anyone with commit access.[^](#a2)
+
+<b id="f3">[3]</b>: Very, very occasionally (I think this has happened once in
+the history of Synapse), we've had two releases in flight at once. Obviously,
+`release-v1.2.3` is more-stable than `release-v1.3.0`. [^](#a3)