Merging

From Archivematica
Revision as of 16:26, 31 March 2017 by Hbecker (talk | contribs) (→‎Code review & approval: Update code review link)
Jump to navigation Jump to search

Main Page > Development > Merging & Branches

Branching guidelines

Guidelines on when to create branches for new development in Archivematica projects. Short answer: always create branches!

  • All development work happens in branches - nothing is committed directly to master, qa/ or stable/ branches
  • Development branches should follow the naming format: dev/issue-####-short-description
    • dev/ marks it as being a development branch, not for QA integration or a stable release
    • issue-#### is the Redmine ticket that the work is mostly related to. If the branch expands in scope to include multiple tickets later, that's fine, and don't worry about renaming branches
    • short-description is a couple word description of the branch, to make it easier to remember what the topic of the branch is.
    • Example: dev/issue-8161-dashboard-i18n or dev/issue-8693-dataverse
  • Development branches should generally be create from the branch they will be merged into. In most cases, this is master, qa/1.x (Archivematica) or qa/0.x (Storage Service). If something is a patch for an old release, it may be more appropriate to branch from the old stable branch (e.g. stable/1.5.x)
  • All branches should undergo code review before being merged into the target branch. If something was developed for an old release, it should be cherry-picked to the current development branch, and a comment with the commit hash should be left in the pull request for tracking.

Code review & approval

All code changes merged into a stable or qa branch must go through code review. We use Github's pull request feature for code review. Every change being submitted to an Artefactual project should be submitted as a pull request to the appropriate repository.

Another core contributor (usually an Artefactual employee) will review the code for content, style and suitability in the project. A more extensive guideline to code review is also available. After the reviewer is satisfied with the code, they will approve it on github. If at least one reviewer approves, and there are no disapprovals, the branch can be merged.

In general, the original contributor will merge the branch. For external contributions, the reviewer can merge or cherry-pick the commits. This will leave the Author correct in git, but update the committer.

Merge process

We use a rebase-based workflow, where branches are brought up to date with the latest stable by rebasing them. (This is compared to a merge-based workflow, where branches are merged, and conflicts resolved in the merge commit). This shows the process for merging development branch dev/test into target branch master. The target branch is usually master, qa/0.x or qa/1.x.

The instructions are formatted: (current branch) $ <command to run>

  1. Check your remote is set to be gitolite
    • $ git remote -v should show origin git@git.artefactual.com:<repository-name>.git
    • Not all repositories have gitolite as the canonical repo - ask if you're unsure
  2. Ensure your copy of the target branch is up to date
    • $ git checkout master
    • (master) $ git pull --rebase
  3. Ensure your development branch is based off the latest target branch
    • $ git checkout dev/test
    • (dev/test) $ git rebase master
  4. Check that your changes still work after rebasing
  5. Check tests pass (see https://wiki.archivematica.org/Getting_started#Tests for more details)
    • (dev/test) $ py.test
  6. Push your newly-rebased branch to remote. Because this is a rebase, which changes history, you'll need --force. The rebased dev branch has to be pushed before the master branch is or github gets confused.
    • Never force push on a stable/, qa/ or master branch!
    • (dev/test) $ git push -f
  7. Fast-forward merge
    • $ git checkout master
    • (master) $ git merge dev/test
  8. Push merge!
    • (master) $ git push
  9. Delete dev branch
    • Locally: (master) $ git branch -d dev/test
    • Remote: (master) $ git push origin --delete dev/test
  10. Update the related ticket, if needed
  11. You're done!

Proposed changes

Forks

  • Current status: All development branches are created in the artefactual (or artefactual-labs) owned repository, and core contributors (usually Artefactual employees) commit directly to the official repository
  • Proposed change: Development happens in contributor's forks, and the official repo contains only stable & QA branches
  • Pros:
    • Fewer branches in the official repository, so there is less clutter and noise for people watching activity
    • Handles more contributors, since there won't be dozens or hundreds of branches of uncertain development status
    • Contribution process is the same for external and internal contributors
    • Contributors can handle their repository branches however they want - naming conventions, deleting old branches or not, etc
  • Cons:
    • Potential confusion about which repository is the correct official one
    • Potential to push changes to the wrong repo (e.g. push a development branch to the official repo instead of the contributor's fork)
    • Developers have to keep 2 repositories up to date: upstream and their fork
    • Harder for analysts to check out development code, since they would have to set their remote to the contributor's fork

Hybrid merge/rebase model

  • Current status: All branches are rebased onto their target branch before being (fast-forward) merged
  • Proposed change: Small or minor branches are rebased, larger feature branches are true merged
  • Pros:
    • Commit history clearly shows 'feature', and which commits came in together as a unit
    • Rebasing small branches preserves clean history for minor changes
    • Better reflects nature of development - shows minor changes as happening inline, and major changes as happening in parallel
    • Merge commits are clear places to look for integration issues
  • Cons:
    • Confusion about when to merged vs rebase
    • Developers have to learn two workflows