Code Contributions

Overview

To ensure a proper code review, every change to the code base (master branch) must go through a GitHub pull request. This rule applies to all members, including those who have push access to the repo.

The exceptions to this rule are exclusive to member made branches that were made during the SVN days of the repository (such as the zookeeper branch), and the wxfred2 branch which is still in its infancy. These branches may Eventually (TM) be phased out.

Contributions include new features, bug fixes, enhancements to existing features, and refactors. When making a new Pull Request, please use the tags we have provided to help reviewers quickly identify what your pull request is about.

Branching Strategy

• The master branch is not worked on directly, instead all contributions are done via Pull Requests (PR).
• Releases are tags on master, they do not get their own branch.
• Contributions get their own branch and this branch is used to create a PR. Once the PR is successfully reviewed and the branch is merged, that branch may not be reused.
  • These branches are known as "feature branches" or "topic branches," and is widely practiced by git users.
  • The master branch on forks is never worked on by contributors. Instead, it is used as a local clone of the master branch in the main repository and all feature branches made by the contributor will be based on/created from the master branch of their fork.
  • The master branch of the fork must be kept up to date with the master branch of the main repository so as to minimize potential conflicts in the contributions. To do this, pull master from the main repository (often referenced as "upstream") and then push master to your fork (often referenced as "origin").

If Contributions depend on another branch that's still in the PR review stage:

• If the contribution is logically similar to the dependent branch:
  1. The contribution gets merged into that branch while its in the PR review stage
• If the contribution is not logically similar to the dependent branch:
  1. The contribution is a turned into PR draft until the dependent is merged into master.
  2. Once the dependent is merged, master is then merged into the contribution's branch and the PR draft is then turned into a PR review.

Reasoning

This strategy is to be used to hopefully promote a smooth workflow with the diverse and chaotic nature of open source contributions. Here are a number of keynotes:

There is no dedicated release branch because our release phases have a feature freeze which permit only certain features to be added and bugfixes. During this time, additional feature PR's may be requested but they will not be considered for merging until the Release phase is complete. Therefore, it doesn't make sense to have a branch for release builds and a branch for development builds when you can just put a tag on the development branch of where the release builds are made off of.

Branches that have successfully been through a PR stage may not be reused for another PR. This is because we use the branch head to reference changes in that branch instead of making a new tag for it, or relying on the merge commit into master. It also makes the commit history easy to distinguish what commits where in what PR, because the latter PR's may end up including all commits up to the branch's original root commit.

The branching strategy for branches dependent on branches within the PR stages was selected due to the nature of the PR review stage. During this stage, additional commits may be required to address issues flagged by the CI tools as well as changes requested by reviewers. "PR stacking" where a string of PR branches that depend on each other is an example where this rule comes into play. When commits are added to dependent PR's to address the CI issues, etc. all child PR's must be re-based or re-merged with their dependents, in order to get those commits into their string. Re-basing offers a clean commit history, but requires all child branches to be also re-based (which is bad if multiple authors are in the string). Merging avoids this issue, but is not recommended for logically similar contributions because it clutters the commit history and makes it difficult to pick out which commits are from this branch and which branch is from the parents. Merging with logically different contributions is recommended, however, because the merged commits should still theoretically be easy to distinguish.

Recommended Practices

• Don't rebase a branch that's in PR review, unless the new root commit is the current head of master
  • If you want to tidy up your branch with rebase, do it before sending it into the PR review.
• Don't force-push your local branch to the remote branch while it is in PR review, unless necessary.
  • Example: An old PR passed the CI tests, but was not reviewed or merged in before another PR which may conflict with it. A rebase on master plus a force-push is allowed here in order to check for conflicts and restart the CI tests.
• Every commit must be buildable and can run using retail data. (Golden Rule)
• Be wary of "cleaning" the codebase of invisible behavior in retail data, some mods may depend on it. It's generally safer (albeit uglier) to only ever add new functionality.

Acknowledgements and Special Thanks

SelfGamer for branching strategy tips.