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

and 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”

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

Add your comment