26

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?

4
  • 10
    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. Oct 24, 2016 at 16:58
  • 1
    What happens in your organisation when the reviewer writes "Please fix major issue X"?
    – Doc Brown
    Oct 24, 2016 at 17:18
  • 12
    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, 2016 at 17:32
  • 1
    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, 2016 at 18:08

5 Answers 5

38

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
  • 7
    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, 2016 at 18:47
  • 3
    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 Oct 24, 2016 at 20:20
5

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
  • Care to specify why encouraging the initial author to merge their own pull requests would build a sense of ownership? I believe in collective ownership and consider open PRs as statements of shared ownership between initial author and reviewers/community. I respect your opinion, would like to hear more about how the control over who hits merge translates to sense of ownership. Being able to open a pr and have others review and discuss already brings enough ownership, but I had another person tell me about ownership the same way you did, so I would like to understand that point.
    – leosteffen
    Mar 3, 2021 at 17:49
  • 4
    @leosteffen in a team project, ownership is shared and collective. By definition. I'm not calling this into question. The author of a given PR knows their PR better than anybody else; even if they provide a great PR description, as they should, the consequences of merging the PR cannot typically be clearer and more comprehensive to other contributors. By merging your own PR, you know exactly when it lands into master (now, main) and you take responsibility for it from start to finish.
    – mkcor
    Apr 23, 2021 at 21:40
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.

Added much later: You should only do a merge that can be done without merge conflicts. To achieve this, I merge the baseline into my code first - the result of that would later become the new baseline by merging it to the baseline without merge conflicts.

If there are merge conflicts then I fix them and the merged result goes into the pull request. So the pull request contains only the differences between my branch and the latest base line, and when the merge is eventual done, the merged result is exactly what has been on my machine.

0

The author, in most cases, knows more about the code than the reviewer. They are also the ones responsible for that piece of code. If something goes wrong, the author, who is familiar with that part of the code, can jump in and fix it. It creates a sense of responsibility and ownership for the authors.

Also, at my job, we have CI/CD set up for testing and deploys, but we don't really have it set up for DB migrations. That falls to the developer to run migrations when it gets deployed.

It's important to allow them to merge at their preferred time in order to run migrations or fix bugs/tests.

Edit: (thanks FluidCode)
The reviewer is responsible for checking the quality of the code i.e. is it readable, follows a standard approach, catch obvious inefficiencies, ensure tests were added, and in some high profile companies (like banks) ensure there aren't any attempted fraud.

1
  • Right. But you forgot to add the role of the reviewer. Checking the code quality, organise access to the master by different developers, keep up to date the overall status, etc. The seemingly redundant round trip between two roles has a purpose.
    – FluidCode
    May 16 at 12:41

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.