Inside Adaptavist's Stash pull request workflow

Many organisations use code reviews as an aid to ensuring code quality. Teams will typically use various different methods of code review including Crucible reviews, pull requests (branching or forking workflows) and patches sent to mailing lists.

Adaptavist moved to Git a few years ago and immediately adopted a pull request workflow based around branches. More recently, our Stash pull request workflow is a massive improvement but we still experience many problems that our customers have including:

• We didn't have a naming standard around branches. Some got the issue key put in them, others the feature or bug name and some were completely meaningless,
• Selecting reviews for pull requests is optional. Although Stash lets you define a minimum number of approvers, you can pick whoever you like to review, and
• The review workflow wasn't that efficient for us. Sometimes you'd go to review a pull request and find it was already conflicted. In that case, approving doesn't seem like the sensible thing to do.

Stuff just wasn't getting merged. Several people might review and approve, but there was uncertainty on who had the responsibility to press the Merge button. Should it be the author or project lead? Or someone else entirely?

Enter ScriptRunner for Stash!

Naming Standards Enforcement

We tackled these problems using various ScriptRunner for Stash features. The naming standards were straightforward. We use the Naming Standard Enforcement to make sure the branches have an issue key and at least some text afterwards. So an acceptable branch name might be:

bugfix/SR-266_send_mail_hardcoded_urls.

The issue key alone wasn't sufficient, as we want to have a clue what the branch is for when using the command line or SourceTree.

(If you create the branch from JIRA it will come up with a sensible branch name but we don't always do that, of course.)

Default and Mandatory Reviewers

The project lead is set as mandatory reviewer for every pull request in the relevant repositories. The resident front-end expert is set to be a mandatory reviewer if the changed files contained front-end stuff, like .js, .css, .less etc.

Notice the padlock icon, indicating this reviewer can't be removed.

It makes sense to minimise the time between creating the pull request and merging the longer the gap, the more opportunity there is for conflicts or for the author to forgets what they were trying to do. We use the Auto Merge event handler to automatically merge if the project lead approves (this is boss mode) more than 60% of the reviewers have approved.

The other process efficiency we have is blocking pull requests that lag behind their target branch when they are created. If this happens they could be immediately conflicted on creation, plus the merge result is not a known quantity. Check the documentation for a longer discussion on this.

Pull Request Advisor

Finally, we use the pull request advisor to give people up- to-date information about the state of their pull requests, when they push:

remote: The branch 'topic' is 2 commit(s) behind master.
remote:   The first commit on the target that you're missing was at Fri Jun 05 14:12:53 BST 2015 (server's timezone).
remote:   Please consider rebasing.
remote:
remote: In addition the following files have conflicts:
remote:
remote:   include/ap_compat.h
remote:   include/mod_request.h
remote:

Pull requests that lag their target branch by a long way are more likely to have merge conflicts and take longer to merge. Also, I'd rather that everyone has access to everyone else's delivered features when they're developing. It means that they have more time to root out incompatibilities and can be engaged as testers for new features (even if they are not aware of it).

Cleaning up

For a while we probably weren't doing this quite right. We had a lot of branches that had been merged but not deleted and were just hanging around. There were also a lot of branches that had commits that weren't merged back to master but hadn't been updated for a long time. With branches like these, it's often unclear whether they're ever going to be finished.

This script can be run from the Script Console to remove dead branches: https://bitbucket.org/snippets/Adaptavist/kyEpn

The first part is easy enough. If a branch is not behind master then it's probably safe to delete. However, the crucial line is commented out run once and view the log output. If they all look like they can go then uncomment the line and rerun.

Make sure everyone then fetches with

git fetch --prune

otherwise there is a risk of someone pushing them back if they are using

git push --all

(but why would anyone do that?).

Dealing with branches that are not completely merged but are way out of date is trickier. I settled for listing branches that are behind master but haven't been updated for more than six weeks. You need to make a decision on them on a case by case basis, or just stop worrying about them cluttering up your branches.

What still doesn't work?

There are still some aspects of our pull request workflow that don't work well at all. We use pull requests not just for quality review, but as a way to discuss changes, and to get help on an issue. As such, pull requests are sometimes created very early in the process.

The problem then is that reviewers get notified of every future change, and I personally am never clear whether to add my comments, and instead just keep asking my colleagues: Is it ready? One thing that would work, of course, is to decline the pull request, but that seems just a little bit rude!

Visit the ScriptRunner for Stash page for more information or try it free today from the Atlassian Marketplace.