wiki:org/teams/NetworkTeam/MergePolicy

Version 4 (modified by teor, 21 months ago) (diff)

Dump the rest of the email thread

-1. Meta

Hi!

This is a draft. If we think this draft is good enough, let's try following it for a few weeks, and then deciding whether to amend it or adopt it as policy.

In general, let's have the above be our policy-adoption mechanism: let's have policies start out as "draft" like this one is now. If we think it could maybe work out, let's call them "experimental" and try them out for a limited amount. And then let's see whether we need to amend them or adopt them or discard them entirely based on network-team rough consensus.

I've written this draft from my memory of things we've discussed in the past, but I have probably missed a bunch of good ideas.

  1. Goals and General principles

Here are some goals that we'd like to achieve:

  • We should keep merging Tor patches regularly.
  • We should make sure that all patches merged are of high quality, and conform to our standards.
  • We should make the Tor development process resilient to any one or two people being unavailable.
  • We should avoid developer burn-out.

Here are some principles for achieving those goals:

  • For patch quality: We should make sure that every commit to Tor is reviewed and double-checked for conformance.
  • For patch quality: We should do as much automatic testing as possible.
  • For process resilience: We should avoid single points of failure.
  • For process resilience: Nobody should have commit privileges without using them regularly. (Otherwise, they tend to fall out of practice, or get out of sync with others in their understanding of our coding standards.)
  • To avoid burnout, and to keep old series of high quality: Older release series should have a different maintainer who makes backport decisions.
  • To keep merging patches: We should create exceptions to the above rules when it makes sense to do so.
  • To keep merging patches: We should not deadlock development because somebody is on vacation or unavailable.
  1. Proposed policies

We treat "current" and "old" release series differently. For the purposes of merging: all non-stable release series are "current", and the most recent stable release is also "current" for the first 2 months of its lifetime.

Patches that are going to get merged into a current series should, if possible, be merged by somebody who did not write the patch or review the patch. (This helps us improve our chances of noticing bugs before they get merged.) If no such merger exists, anybody with merge permissions can merge. See section 2 below for what the merger should check before merging.

Most patches that are going to be merged into an old series should first have been merged into a current series, and had a reasonable time for testing in the wild. How much testing time is "reasonable" will depend on the severity of the issue that the patch fixes, and the risk introduced by the patch. The backport merge to an old series should be done by the stable release maintainer.

For security bugs of "medium" severity or higher, and other urgent patches (see 3 below) we will want to patch them simultaneously on all supported series that are affected. The stable release maintainer should coordinate with the other mergers to make sure that the patch poses as little risk as possible.

  1. What the expectations for a merger?

The merger should make sure that the review exists, that the patch exists, that CI passes (or has only unrelated failures), and that the reviewer checked the right things.

The merger should at least skim over the patch to make sure it "looks okay" and touches reasonable parts of the codebase.

The merger think about whether there are other planned branches that will touch the same parts of the codebase, and whether the patch will likely introduce conflicts that we need to prepare for.

The merger should check formal requirements that we do not currently have automated tests for:

  • Is there a changes file?
  • Is the patch based against the correct branch?
  • Is there a spec branch corresponding to any protocol change?

The merger should have the appropriate pre-push hook script installed (from scripts/maint/pre-push.git-hook), so that they do not accidentally push any fixup! or squash! commits.

Mergers should skim the tor-commits mailing list for commits to tor.git, and keep an eye on the other mergers.

The merger should, if possible, be on IRC when they're merging things.

Only merge when you are sober, awake, and alert.

If you feel funny about something, don't merge it, and tell the other team members.

Remember, above all, that Tor exists for its users, not for its developers: we all have the duty to protect user security and privacy. [This should be obvious, but it's useful to have a policy saying so.]

  1. What patches are urgent to backport?

The following items can be backported without first maturing for a while in a stable release.

Medium or higher security issues, as defined in our security problem.

Major bugs that seriously threaten network stability.

Regressions that break compilation or CI on important platforms.

  1. What patches can be merged without review?

The following items do not need a reviewer or a separate merger:

Putting out releases:

  • Creating the ChangeLog and ReleaseNotes
  • Bumping the version number
  • Creating a signed tag

Typo fixes in comments.

The following items need CI, but don't need a reviewer or separate merger:

Trivial cherry-picks and rebases for backports

Configuration changes to tests and tooling

  • Adding or removing releases from the merge scripts (for example, #29606)
  • Updating exceptions to make tests pass (for example, the final fixup in #29221)

Reverts, when the commit was merged and introduced a bug

  1. How do you do a merge?

5.1. Configuring your environment

[draft]

Make sure you have the pre-commit and pre-push hooks installed.

How do I keep the pre-commit and pre-push hooks up to date?

We are working on a post-merge (post pull) hook that logs an obvious message when the hooks are updated in master. See #29588.

Make sure you are using worktrees.

You might find it easier to use a separate git repository for merges and regular coding.

Using the git-* scripts in scripts/maint/, you need worktrees. Here is an example on how to do it properly for let say the "maint-0.3.5" branch and your worktrees are in "tor-wkt" (relative to tor.git/ you have):

 $ git worktree add ../tor-wkt/maint-0.3.5 origin/maint-0.3.5
 $ cd ../tor-wkt/maint-0.3.5
 $ git checkout -b maint-0.3.5
 $ git branch --set-upstream-to=origin/maint-0.3.5 maint-0.3.5

So then now within the worktree, you can simply do "git pull" and it will always fetch the latest from origin/maint-0.3.5 which is important because the git-pull-all.sh script requires that. These worktrees need to follow upstream at all times.

Basically, a maintainer should do the above for all maintained branches (including releases-*).

Make sure you have the git-pull-all, git-merge-forward and git-push-all scripts installed. They require a recent bash that supports arrays.

We want to make a script that sets up all the directories (#29603).

You will need to customise these scripts to your own environment.

Make sure you have merge permissions.

TODO: link to gitolite instructions for checking merge permissions

Make sure you have your git repository configured with git-rw.torproject.org:tor.git as "origin" and github.com/torproject/tor.git as "tor-github".

git remote add tor-github https://github.com/torproject/tor.git

Piece of advise here. Because git default behavior of "git push" is to use "origin", then mistake can happen where one of us can push a personal branch to our upstream tor.git.

For that I nullify the "pushurl" of origin with:

[remote "origin"]

fetch = +refs/heads/*:refs/remotes/origin/* url = https://git.torproject.org/tor.git pushurl = "Don't push origin dummy!"

To push now, I created an "upstream" branch so when I push commits upstream, I have to explicitly type in:

$ git push upstream <branch>

(The git-push-all.sh allows you to specific the name of the "upstream" branch).

To add the upstream branch properly:

$ git remote add upstream https://git.torproject.org/tor.git $ git remote set-url --push upstream ssh://git@git-rw.torproject.org/tor.git

Make sure you have PRs from tor-github set up to be named tor-github/pr/NNN, where NNN is the number of the PR.

In your gitconfig:

[remote "tor-github"]
   fetch = +refs/heads/*:refs/remotes/tor-github/*
   fetch = +refs/pull/*:refs/remotes/tor-github/pr/*

5.2. Finding out what to merge

If you are Nick, or you want to do it like Nick does, you should regularly look at the list of all merge_ready tickets in the Tor component where the Owner is not you, and the Reviewer is not you. These are tickets that you have permission to merge into newer series.

Additionally, Nick will add tags like "asn-merge" or "dgoulet-merge" to assign a merger for the tickets he shouldn't be merging himself. You should check for tickets tagged (yourname)-merge.

5.3. Merging a branch.

Make sure you have the latest version of all the stable branches checked out in your worktrees. (git-merge-forward does this automatically.)

Make sure that you have the latest version of the branch you're merging.

$ git fetch tor-github

Check all the things you're supposed to check.

Go into the earliest target directory where you are merging today, and merge the branch.

[TODO: Write a standard for merge commits]

Run "merge-forward" and verify that there are no conflicts.

The merge-forward script, at any signs of conflicts, will stop so you'll have to go in the worktree and fix the conflict yourself. It *does* happen so not to worry :).

Run "push-everything".

If you are merging a patch into the earliest series to which it applies, close the ticket after you merge, and say where you merged it. If you are merging a patch that is a candidate for backport, move the ticket into the earliest open milestone to which it applies.

  1. How do we create backport branches?

To make the merges as simple as possible, here's what we usually do:

  • when we know we want to backport, we find the earliest maint branch (N) that needs a backport
    • if we're writing the bugfix, we write the fix on branch N
    • if we have a bugfix on master, we cherry-pick or rebase to N
  • once the backport branch is done, we merge the branch into N, and then merge N+branch forward
    • if there are any significant conflicts, we find the earliest maint branch with a conflict (M)
    • we make a separate branch based on M that resolves the conflict (How? We still need to write a merge policy.)
  • then we continue to merge forward

Even if there are no conflicts, the patch may require changes to work on later versions. We deal with these changes in a similar way:

  • merge forward the changes for previous versions,
  • add a commit with the new required changes, on the earliest maint branch that needs the change.

So we should be able to merge all these branches in the right places, then merge forward without conflicts.

  1. TODO:

We should integrate this document with the stable maintainer guide.

We should describe a standard for merge commits.

We should figure out whether we can do automatic checks for our process above.