15

Several teams at my company practice a code review workflow I've never seen before. I am trying to understand the thinking behind it, with the idea that there's value in making the whole company consistent. (I contribute to multiple codebases and have been tripped up by the differences in the past.)

  1. Code author submits a pull request
  2. Reviewer examines the code
    • If the reviewer approves, they leave a comment along the lines of "Looks good, feel free to merge"
    • If the reviewer has concerns, they leave a comment like "Please fix minor issues X and Y, then merge" (For major changes, return to step 2)
  3. The code author makes changes if necessary, and then merges his or her own pull request

I have the following concerns:

  • In the case of approval at step 3, this workflow creates a seemingly-unnecessary roundtrip to the pull request author. The reviewer, who is already looking at the code, could just merge it immediately.

  • In the case of changes being requested at step 3, the agency to merge the pull request now rests solely with the PR's author. No one besides the author will look at the changes prior to merging.

What are some other advantages or disadvantages to this workflow? Is this workflow common on other engineering teams?

  • 5
    Have you asked the folks in your organization why they chose this specific workflow? They're probably in a better position to illuminate the relevant merits than we are. We would merely be speculating. – Robert Harvey Oct 24 '16 at 16:58
  • 1
    What happens in your organisation when the reviewer writes "Please fix major issue X"? – Doc Brown Oct 24 '16 at 17:18
  • 8
    In my experience, it's best for the original author to be the one doing the merge in case there's a merge conflict to be resolved. The original author is usually the one who is best equipped to figure out how to resolve a merge conflict. – 17 of 26 Oct 24 '16 at 17:32
  • I'd be curious as to the logic here. You should ask your colleagues and write it up as a self-answer - there could be a very good thought process or rationale here. I'm just not able to come up with one quickly. – Thomas Owens Oct 24 '16 at 18:08
21

In the first case, it's usually a courtesy. In most organizations, merges kick off a series of automated tests which must be dealt with promptly if they fail. Especially if there was a significant delay between when a pull request was submitted and when it was reviewed, it's polite to allow it to be merged on the author's timetable, so they have time to deal with any unexpected fallout. The easiest way to do that is to let them merge it themselves.

Also, sometimes the author becomes aware of reasons later that a pull request shouldn't be merged yet. Maybe another developer's PR is higher priority and would cause conflicts. Maybe she thought of an uncovered use case. Maybe a review comment triggered a brainstorm that needs further investigation before the issue is fully satisfied. The author knows the most about the code, and it makes sense to give him or her the last word about when it gets merged.

On the second point, that's just a matter of trust. If you can't trust people to fix minor issues without being double-checked, they shouldn't be working for you. If the issue is big enough that it will need another review after the fix, then trust reviewers to ask for one.

That being said, I do occasionally merge other author's pull requests, but it's usually either very simple changes, or from external sources, where I personally take responsibility for shepherding through any test automation failures.

  • 2
    Sounds like there's more variety out there than I had imagined. My experience with automated testing has been that tests run against the branch before it's merged, not after, thus becoming a precondition for merge. I have also seen unreviewed "minor" fixes, including my own, that cause bugs. – aednichols Oct 24 '16 at 18:47
  • 2
    Tests often run as a postcondition as well as a precondition. It's completely possible for changes from multiple branches to conflict in non-obvious ways that wouldn't show up as a code conflict and cause tests to start failing. At my workplace we require a branch to be up to date with the base branch as well as all tests passing before it's a candidate for merging and merging happens automatically if those two conditions are satisfied. We didn't always have that first condition - before that we did actually have issues introduced to master infrequently despite every branch passed individually – Matthew Scharley Oct 24 '16 at 20:20
3

Having the initial author merge their own pull request is my preferred workflow in small teams. In addition to technical advantages already mentioned (in terms of resolving merge conflicts, for example), I think it adds value on a cultural level: It builds a sense of ownership.

I specified initial author for the (rare) case when another developer would add commits to the open pull request (pulling the work-in-progress feature branch and pushing back to it). This does not happen often, and would result from a conversation in person or over Slack: These additional commits (by someone else) should not land there by surprise! In this context, by initial author, I mean the one who submitted the pull request.

2

In my organization we are quite new at pull requests and your question is one I have pondered myself.

One observation I would like to add: In some tools (we use TFS) there might be a work item associated with the pull request.

If that is the case, it becomes a bit of a hassle to keep track of when the reviewer performed the merge. In that scenario the developer then has to revisit the PR, open the bug or change request and mark it as 'resolved'. If we mark it 'resolved' too early, the testers believe that the fix is already part of the current build.

TFS 2017 improved on their implementation of the pull request. Now the developer can request the Pull Request to automatically merge if all the conditions are met (no merge conflict, approval from reviewers and no broken builds). YMMV.

1

This way, the author has a chance to change his mind about his branch, maybe he's figured out something that should be done in a different way, and put the additional charges back up for review.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.