Git style requirements

Here we describe some hard guidelines that contributions are required to follow. They may seem nit-picky at first, but over time, adhering to these rules in collaborative projects becomes extremely valuable and important. If you create a pull request, make sure it and the commits within it, follow these rules.

Commits

A commit ideally needs to be a self-consistent change in the code base. Often you will see an even stronger requirement that for each commit the tests should pass completely, such that any commit can be checked out and run successfully. This is a little bit too strict, I find, and not always necessary. However, what is important is that the changes in a single commit tackle a single well defined issue, or at the very least extremely closely related issues, if they are inextricably intertwined. Try to avoid at all costs to lump multiple unrelated changes in a single commit. Remember, commits are not just to share code, they are there to record changes in the code base. You or someone else in the future will need to go back to the commit you created and understand why you changed things. The bottom line is that git is not just a tool to share code changes, it is a tool to communicate.

While you are working on a problem, you often find new problems along the way or it is difficult to isolate certain things without touching others. This makes it difficult to create these concise and consistent commits the first time around. But that is why git provides powerful tools to revise history. After you have completed your work, creating multiple potentially unclean commits along the way, you can then easily reorganize that work. The tool for the job is git rebase. There are a ton of tutorials out there, so I won't write another one here. In general, I think for us developers it makes a lot of sense to sit down sometime and really try to understand git and how to work with it efficiently and correctly. Just think about how often you use it every day and how much work you have really spent to understand and learn how to work with it (googling quick commands doesn't count).

Commit messages

A commit needs to come with a human-readable message. Its intention, is not to say what was changed but rather why it was changed and what potential difficulties were encountered and subsequent decisions taken. It is much like a code comment: explaining what the next line of code does is pointless, the code should do that for you. What rather is important is to convey why that particular solution was chosen, if that is at all relevant and not necessarily immediately obvious. A commit message is no different.

Therefore there are a few guidelines for content and makeup of commit messages:

• Commit messages consist of three parts:

  1. A short descriptive title
  2. A blank line
  3. A more verbose message body

• The title should be 50 and 72 characters to very shortly describe the scope

• The message body can be widely varying, from empty, if the commit title really is all there is to say, or multiple paragraphs with again a maximum line length of 72 characters.

• Note that many terminal editors, such as vim, have built in syntax highlighting to show when your title is too long, and line wrapping to automatically respect the line lengths

• The intention of the body text should be to explain the reason for the change. For complex changes, it is always a good idea to start with some context. You have probably been working on it for a while and therefore no the current situation, what and why it had to be changed. But imagine you, or better, someone else reading this commit in a month's time. What do they need to know why this change was necessary? Messages to say, for example, that you added a unit test when you added a new feature are not really all that useful. The code diff will already tell you this.

Pull requests

• Only open a PR when you think it is ready for review. Each time you push a commit to a branch with an open PR, the reviewers will receive an email and notification, but worse, it will trigger a build on Travis. Each build now takes over 30 minutes and we can only run one at a time, for the whole aiidateam organization. Unnecessary builds are blocking other builds.

• Do not manually merge the base branch to bring your PR up to date. This will also trigger another build causing the same problems described before. Another PR might be merged and you will have to update it again. The policy is that the one responsible for merging the PR will make sure that it will be brought up to date, after the review has been completed.

• A PR can consist of one or multiple commits, however, each of these commits should respect the rules outlined above. Before you open a PR, make sure to clean up your work and create the commits that you think best divide up the total work in self-consistent parts. Use git rebase and git commit --amend a lot.

• If a reviewer requests changes that are minor, try to use git commit --amend instead of creating new commits, unless you intend the PR to be squashed anyway (see below) in which case it does not matter. While you are at it, it is also good to rebase to make sure your branch is up to date. You will have to push anyway and trigger a build. If you don't bring your branch up to date, you will have to do it again.

• There are three ways of 'merging' pull requests and which one to use depends on the situation, but which method is intended in turn also puts some constraints on the commits:

  1. Merge with merge commit: will put all commits as they are on the base branch with a merge commit on top
  2. Rebase and merge: will take all commits and 'recreate' them on top of the base branch. There will be no merge commit and all commits will be recreated, which means that their hash will be different.
  3. Squash and merge: take all commits, squash them into a single one and put it on top of the base branch.

• [Squash and merge]: for most pull requests, number 3 will be the most likely candidate. Most pull requests will typically address a single issue, which then also invites a single commit. If the pull request was not already openend with a single commit, or more commits were added during review, the squash option can be easily used to turn them into a single commit, and respect the commit guidelines. Note that this won't work of course if the commits of the request are individually significant and need to be kept as such. In that rebase and merge can be used.

• [Rebase and merge]: this should be used when the PR requires more than a single commit. Examples is a PR that has two commits from two different authors. If we were to squash it, the attribution of one of those authors would be lost. This of course presumes that both commits have an existence right and constitute self-consistent and significant changes. If one of the commits are only slight fixes or touchups, it should have simply ammended the other.

• [Merge with merge commit]: this should be used for bigger pull requests. Examples are long lived collaborative branches. These are typically not single issue PRs but release branches or pull requests for big changes that often consist of the work of multiple people. Here, having the merge commit and not recreating new commits but keeping the actual one, provides actual benefits, unless in the other discusses scenarios.

• If the request is squashed, please make sure to also clean the message. Github will provide you with a textbox that contains the message of all invididual commit messages. Remove the commit messages of 'fixup' commits with messages like 'make linter happy', 'fix typo' or 'minor bug fix'.

• Ultimately, if you are not sure what to use, please do not merge. Come ask and discuss with me or someone who does know.