Pull Request based development (sucks)

Does this workflow sound familiar? The “master” branch is considered sacred, and it's therefore locked so nobody can push directly to it. Before you even start coding, there has to be a task opened in JIRA, and a branch also created from JIRA and linked to that task, and you then start pushing commits to that branch. And when you're done, you submit a Pull Request via Stash, and wait for at least two colleagues to review and approve your patch, and only then can it be merged to master.

As a developer who has been around for almost two decades, I'm going to argue why this workflow sucks. You tell me to run, and then you tie my legs.

My development workflow

In my world, fixing a bug usually goes like this:

Somebody mentions a problem
I go see the problem (usually in a browser, as I do a lot of front-end work)
I open the relevant file(s) in Emacs and fix the problem
Go back to the browser and make sure the issue is gone
I press a few keybindings to commit and push via Magit

Sometimes it takes less than a minute from the moment the problem got mentioned to the moment I push the fix. The fix goes to the master branch so everybody gets it immediately. If there are problems with my fix, they will be noticed quickly.

The “desired” workflow

I partially ignore this, but here is what we “should” do, according to company policy:

Somebody mentions a problem
I go see the problem and acknowledge it
Somebody files a ticket in JIRA
I go in JIRA and click “Start progress”
I create a new branch for my fix, also via JIRA
I git fetch and git checkout the new branch
I open the relevant file(s) in Emacs and fix the problem
Go back to the browser and make sure the issue is gone
I press a few keybindings to commit and push via Magit
I go to Stash, locate the branch and click “Create pull request”
I go to JIRA and click “Ready for code review”
I paste the link to the pull request into our Skype group, just in case
... and then I wait for two other developers to review and approve my patch
only after that I can click “Merge” in Stash and my fix goes to master
I go to JIRA and click “Ready for testing”1

I highlighted in bold above the steps that are actually required to fix the problem. The rest is bureaucracy. As a developer, I hate bureaucracy.

The proponents of this workflow claim the following advantages, to which I respond:

We have a log for the development in JIRA

You also have a log in Git. The Git log is invaluable, because you can use various tools to find out why a particular line of code is there (assuming the commit messages contain useful information, which they should). JIRA won't help with this.
We know how much time was spent on an issue thanks to JIRA

In practice, people will forget to use the “Start progress” / “Stop progress” buttons. That information is irrelevant. Besides, all the extra-steps that one has to make in order to commit a simple patch increase the time considerably. It's a mental burdern to shift between the various apps and interfaces, to keep all the information properly in sync, when all you really have to do is to edit a file and commit your fix.
We can show to management what was the team working on

git log. It's all there.
All commits that go into master are reviewed

That's not true. You lie to yourself. But let's discuss this at length below.

Any other advantages that I don't know of?

Let me pause a minute to tell you that I'm not against bug trackers. But perhaps not all issues should be logged. You don't file a ticket which says “the headline font size should be increased by 0.5px”, you just go and do it. You do file tickets for more profound issues or feature requests, though. Unfortunately, the bureaucracy imposed by JIRA makes me dislike it, and I avoid it as much as I can.

Nah, just JIRA. A great bug tracker, for instance, is the one at Github.

The fallacy of “code reviews”

So, you probably have a few tickets in your backlog, and are working on something, and I suddenly ask you to review my pull request. First, let's note that I'm interrupting you from whatever you are doing, which is bad. Anyway, there are two cases here:

My branch contains a trivial fix, like adding a bit of CSS, or changing a couple of lines of JS. You quickly look at it in Stash, you assume I actually tested it, and you hit “Approve”. The value of your review is exactly zero.
I've been working on my branch for a week, implementing a new component. The patch is +1000-200 lines of JS and +50-30 lines of CSS. You gaze at its greatness in Stash, you assume I actually tested it, and you hit “Approve”. The value of your review is exactly zero.

Yes, folks, this is what really happens! If you're wondering why the second case goes like this, here's why: it took me a week to work on that code. In order to understand it, you have to basically rewrite it in your mind. You don't have the mental context that I've built over a week, because you never thought about the problem that my code is trying to solve. It's simply not possible to provide a meaningful review in a short period of time, that's why you click “Approve” without thinking. You have your own issues to work on, and my interruption already did some damage by de-focusing you. Reading code is hard. Nobody really does it.

At best, you can provide stylistic objections, like “hey why did you indent code like that?”, to which I respond “I don't indent code; that's editor's business.”

Required code reviews are not just useless, they actually incur some costs:

As mentioned, you need to interrupt what you're working on in order to take a glance at my commits. Interruptions are bad because they get you out of “the zone”.
Merging pull requests from the UI, which everyone does, pollutes the git log with useless merge commits.
It takes more time for my commit to get into master. Experience shows that master is the only branch that receives any testing (did you see the footnote?), so if my commits introduce any problems, it takes more time for those problems to be noticed.

“So what is the solution?”, you ask. “How do we keep the master branch stable?” The answer is: you don't. As has been seen time after time, things happen exactly as I described above and sometimes a bug finds its way into the master branch, and you have to deal with it. By not allowing the fix to be pushed quickly, you're making it worse! The master branch enables your developers to collaborate, to share code. Don't take that away from them; allow direct push to master! What you should do instead of locking master, is to have a “stable” branch, and periodically reset it to master when master was tested extensively and has passed the tests. And that's when you do a new release.

But wait, speaking of mandatory code reviews, did you think about this? So you don't trust your own developers, and require attention of at least 3 people for a commit to get in. But, do you use any frameworks or libraries? Do you use Nginx? Do you use Linux? You trust tens of thousands of random developers all over the Internet; your business runs on their work. Your node_modules has 247.5MB and I'm telling you, most of it is crap; yet you trust it to run the infrastructure that you depend on daily. Did you review any of that code?

Please, people, kill the pull request! Open the development branch!

Footnotes

1. Did you notice that testing comes after merging? Doesn't this kinda defeat the goal of having the master branch stable at all times?

11 comments. This is HOT!

Add your comment

# Yevgen

2017-03-19 15:27

I think the gains of more formal PR review process (a la bureaucracy ) are the following: - the bigger team -> more valuable the formal process will be. - PR based approach allows running tests/lint in CI before it will be merged into master. How often we face a problem "works on my machine"? - not necessarily all PR will be review by all engineers in the team. In most cases will be 'aria experts' that could give valuable input into PR changes - PR allows better knowledge sharing across the team, especially for newcomers.

# anonymous

2017-03-23 16:18

Generally I agree that receiving 500+ patch and staring at it for some time is pretty much useless. However, code review + "white box" testing (when code reviewers actually run _their_ own test cases against your new changes in order to break them) really changes the quality upward by far. Not all code is that critical obviously.

# Brandon

2017-03-23 16:31

Reading this I noticed some issues, and its with how the bureaucracy around you functions. First off, tests should come first not last, and a piece of functionality that is properly tested should never break master. Not to mention that a full build of your branch(which is off of master) should work properly with all tests passing. Therefore your master branch stays the same. As far as your comment on "noone reads code" I would challenge that with everyone that is a good developer reads code regularly. In fact code is read far more than it is written. I'd say a solid 2 hours of each day goes to reading code. This is a normal part of being a good developer. lastly, if your argument is that code reviews are pointless bureaucracy you shouldn't contradict yourself by referring to open source projects that have heavy amounts of bureaucracy and require lots of code reviews before any code makes it into the code base. The truth is, that the bureaucracy exists because people are error prone. we make mistakes all the time, but by making a team come to consensus on a change then breaking changes are less likely to ever happen. My suggestion is, take a step back, have a moment of reflection on why you are wrong and work to be better. Your team may just benefit from it.

# Mishoo ✔ ↰

2017-03-23 17:59

Right, I should have mentioned that my rant does not apply to open-source. Indeed, you can't open the repository for anyone to push directly; for open-source (or, yeah, large distributed teams) PR may be the way to go. I'm talking about a closed product and a relatively small team.

# sbarzowski

2017-03-24 14:12

There's more to code review than just looking for bugs, that's just a side-effect. It's mostly about code quality and going in the right direction with the architecture. There are things that other developers might have a better chance to catch, for example: * Mixing abstraction layers * Overcomplicated code * Bad names * Obsolete comments * Missing tests * Backwards compatibility issues

# TRB ↰

2017-03-24 18:02

One major difference as I see it is that OSS projects are not a team of hired individuals whom you should trust but literally anyone on the Internet - whom you should definitely not implicitly trust. Also I think the scale and scope of the code base and change matters. Changing the color of text of moving an image around a bit is not the same thing as refactoring a series of conditional operations.

# TRB ↰

2017-03-24 18:05

I would assert that PRs are precisely the wrong place to be doing "architecture" work. I'd even go so far as to say that the only architecture related work in a PR is validating that what was written implements what was designed. Architecture work is done in advance, agreed upon, and then the PR implements it (or a subset of it). If you're deign system design review/changes in PRs you are not doing system design or architecture, you're doing organic growth of a system.

# Dali

2017-04-13 14:10

Perhaps it is just companies trying to imitate the bureaucracy of open-source pull-requests, badly. Hired individuals should be people you trust to push directly to master. I have a suggestion: tag releases instead of branching.

# Jonathan

2018-03-29 22:09

I agree with #Brandon and #sbarzowski but I have a few things to add. I understand that as a developer with decades under our belt, our time is very important and we can go in and fix a defect or knock out a feature fairly quickly and our PRs will likely only contain a thumbs up. I make sure to look through every line of code in every PR. I am looking for possible defects, complicated code, code that is not formatted correctly (which makes the code harder to read when you have SCSS or JS files incorrectly indented). I am making comments about where I see the developer can improve or where I feel they need to clarify their code. To me, a PR is not only a code quality review, but a teaching lesson. Not only does the developer get instruction from us more senior guys, but the newer guys can see how the experienced developers write code. The key about code reviews is not to criticize, but to educate. Never say "This is wrong, fix it" but say "This will not work as it is written because A, B, C.". Lastly, as coders, we tend to work in 4-hour chunks. Putting meetings inside those 4 hours tends to disrupt the entire chunk of development. Same thing with code reviews. If you make it a team policy to look through the open code reviews first thing in the morning and first thing after lunch, it puts that "meeting" at the beginning of a 4-hour chunk and doesn't disrupt it by breaking stride 2 hours into our development.

# Gunnar

2018-04-03 14:58

I totally disagree with you that code reviews are pointless. My company has been using code review for years and no it is not that hard. The team size is between 2-10 developers per project and the more experienced developers know most of the code base. I have found hundreds of bugs during code review and it is a good platform to suggest refactoring to less experienced developers. You mention a trivial fix. Well those trivial fixes aren't always that trivial and many times I have found that someone is trying to fix something and no it doesn't quite fix the problem it just fixes 95% of the cases. Code reviews also tackle the points Sbarzowski listed. I have also repeatedly found duplicate code during reviews, where the developer simply didn't that a specific function existed in some library.

# Scott

2018-10-30 19:36

I also hate our PR process and find it extremely frustrating for some of the same and maybe some of the opposite reasons to you. I have three outstanding pull requests that are for medium to minor-size things and have been sitting for more than a week because our other developers are lazy as fcuk. I'm supposedly the lead on the project and have asked the other developers to review PRs within 1 to 2 work days of them being submitted. It should be a maintenance task like checking and replying to important emails on a daily basis or brushing your teeth or showering, but they won't do it and I have no power to force them to actually do anything because they are from a "partner organization" that I have no control of. One of the annoying devs claims it interrupts her flow. I mean, yes, you don't need to drop everything to do a PR review a minute after it comes in, but holding up incorporating code for weeks wastes everyone else's time and in some cases holds up further progress since that code is a dependency for future work I need to get done. I could probably get nearly as much done on the project by myself if I weren't being hamstrung by them all the time. This is _The Mythical Man Month_ territory here. By the time they get around to reviewing PRs (usually at the end of a two week sprint), the merges are much more complicated and create merge conflicts and add extra time to the process vs if they had done it within a day or two. You should feel lucky if people are rubber stamping PRs in a timely way instead of holding up all progress for weeks. One of our developers at least tests things a bit even if it takes her weeks to do anything. The other one just rubber stamps and it still takes him weeks to move his lazy ass into gear. In a more cohesive, experienced and productive team the PR process can be OK and keep developers more honest because someone else will go through a kind of checklist and it keeps developers doing the stuff that they know they should do and not taking shortcuts. Stuff like: 1. Did you write at least basic unit tests and do they pass in the continuous integration system? 2. Does most of the code look more or less comprehensible and is there anything big that the other developer might have missed? 3. The reviewer knows what you're working on an have some idea where some of your code is and what it looks like in case they ever need to work with it. I find the nit-picky stuff that is easiest for people who just glance at code give the least useful kind of feedback. For a team with some less experienced or less conscientious people on it, it gives others a chance to catch some problems. This kind of heavy gatekeeping of PRs with code review sucks for everyone involved since it turns into micromanagement and I don't want to work someplace like that, either. Personally, I think that if your team actually needs to engage in such hard-core, vigilant gatekeeping then you need to get some of the others on the team to improve or to fire them if it goes on for a long time. If it's a matter of management imposing too much bureaucracy and using PRs as a bureaucratic weapon then it seems as though this isn't an ideal place to work. I know that some management likes to "cargo cult" things because someone told them they needed to be "agile" or PRs are a best practice and they don't have a useful understanding of the process or why they're doing it, so they implement it in a horrific or heavy way. It seems to me that a lot of problems with PRs are based on management or cultural problems. If you can't change the company culture and it makes you really crazy then maybe it's a good time to find a company culture that fits better and (hopefully) managers that do things for better reasons than going through the motions to jump on a trend or "best practice" to say they're doing whatever is cool at the moment.

Mar

2017

Pull Request based development (sucks)

Published: 2017-03-18
Modified: 2018-02-24 15:17
By: Mishoo
Comments: 11 (add)