Workflow

These are the rules we try to follow when working on Cockpit.

Review Criteria

• Each commit should be easy to read.

  Commits are for people to read, so try to tell the story of a new feature clearly. For example, refactor the code in a preparatory commit to make the actual change in the next commit easier to understand. Try to separate changes to separate pieces of the code base.

  Historical accuracy of how you figured out the final form of a change is ususally not very interesting, but it doesn't need to be totally hidden of course. If rewriting historical commits is tricky and has a high risk of introducing bugs, don't do it.

• Each commit should adhere to the Cockpit Coding Guidelines

• The tip of master must always pass the test suites

  A fleet of robots run the test suites for each pull request. This includes unit tests, integration tests, and browser-compatability tests.

  The integration tests performed are slow and brittle, and not all failures are caused by bugs in the pull request, but don't just blame every failure on the crappy tests.

• Whenever a pull request changes the API or makes other significant changes, the documentation needs to be updated. Documentation locations that require manual updating include:

  • https://github.com/cockpit-project/cockpit/blob/master/README.md
  • https://github.com/cockpit-project/cockpit/blob/master/HACKING.md
  • https://github.com/cockpit-project/cockpit/blob/master/test/README
  • the documentation tree, used for the guide

Git-related / Merging Conventions

• No merge commits on master.

  See https://sandofsky.com/blog/git-workflow.html for the motivation. In brief, merge commits are confusing when rolling back history to find the commit that introduced a particular bug/feature.

  Unfortunately, github does not seem to agree to the points made in the article above. This means that we can't let github merge our pull requests.

• Each commit on master should have been reviewed. (Almost each.)

  And have a Reviewed-by line at the bottom.

  The commits made during a release to bump the version number etc don't need to be formally reviewed.

• The subject of a commit should start with a short <topic>: prefix.

  This is usually the package name for frontend code, such as shell, base, or server-systemd, or some other suitable directory name. Check the existing commits for examples.

• If a commit fixes an issue, it should have a Fixes #NNN line.

  At the bottom, before the Reviewed-by line.

• Force pushing to master is allowed, BUT...

  ...write an email to [email protected] to explain what has been done and why.

  Don't force push master lightly of course. One reason would be to remove an accidental merge commit, or to quickly correct wrong or missing Closes or Fixes lines. When in doubt, don't force push master, close/reopen issues manually as needed, and live with the shame.

• The main cockpit-project/cockpit repository should not have any work-in-progress branches.

  Otherwise, there will be a huge number of these branches over time. We could delete them, but that throws away information, and people would still have them in their local clones.

  Instead, each developer (including the core developers) should make his/her own clone and submit pull requests from there. This makes it slightly harder to take over a pull request from another developer, but it can be done.

• A pull request with a WIP prefix in the name is not yet ready for serious review.

  You can make those pull requests to more visibly share some of your work with the rest of the team.

• A pull request that has been reviewed and needs action from the submitter has the needswork label.

  This includes changes to the code, or just replies to comments. Once you have done all that work, remove the needswork label again.

• A pull request is updated with copious rewrites and rebases until it has a small number of 'perfect' commits.

  These commits should be fit for master and the pull request is merged by rebasing these commits onto master.

  The Reviewed-by and Closes lines are added by the reviewer during the rebase.

• A temporary commit with a change for a specific review comment is added to a pull request with a FIXUP prefix in its subject.

  This might make the re-review easier. It's best to agree with the reviewer how to handle this in detail. Usually it doesn't matter much.

  Of course, these FIXUP commits need to be merged into the 'real' commits before the pull request can be merged into master.

• The top commit of a pull request should have a Closes #NNN line.

  At the bottom, before the Reviewed-by line.

  This is our usual way to signal to github that a pull request has been merged. Unfortunately, github labels them as "closed" which looks like they have been rejected.

• A pull request that depends on other pull requests declares that in its description.

  When the commits of a pull request sit on top of the commits of another pull request, it's not easy to see from github where one pull request ends and the other begins. Thus, it is import to note dependencies explicitly so that the reviewer is less likely to get confused.

Merge Workflow

We don't merge requests from the GitHub Web UI. Make sure the Pull request meets above criteria and has passed tests. Review each commit properly.

$ git fetch origin master
$ git fetch origin pull/<PR-ID>/head
$ git rebase -i origin/master FETCH_HEAD
# In case of multiple commits, 'reword' each commit and add
#  'Reviewed-by: Me <[email protected]>' to each commit at the bottom of log message
# ...
$ git commit --amend
# Add 'Closes #NNNN'
$ git log
# Check if everything looks good
$ git push origin HEAD:master
$ git checkout master