The Case Against Pull Requests | Hacker News Reader

The Case Against Pull Requests

dev.to

16 pointsshubhamjain25 comments

Just make smaller PRs

Sure there's some small overhead to create a PR, but the process proposed here is essentially treating single commits as though they ara PRs.

How does that work with CI? I think it's always better to wait until a feature is 'done' and passes tests, before asking others to spend time reviewing it.

A bigger issue is perhaps that a lot of code metadata, ie feature descriptions, design rationales, back-and-forth conversations to hone the code, etc ends up only in the pull request, which is stored on some commercial change-management system, ie github, gitlab, etc, rather than in the intrinsic git history.

I don't really see how this is an argument against pull requests. Pull requests are useful because they allow for asynchronous and distributed code review. All you're describing is trunk based development with feature flags, and encouraging small PRs

He's making the same point that is made here: https://secure.phabricator.com/phame/post/view/766/write_rev...

He's describing the workflow that Phabricator uses but not explaining the benefits very well. He also seems to be oblivious to the fact that there already exists an open source platform built to do what he's describing.

> there already exists an open source platform > to do what he's describing

Namely?

Phabricator.

I guess my last sentence there doesn't explicitly state that but I assumed if anyone clicked on that link they would get the picture.

The problem seems to be that you can't use this in front of other repository, for example Gitlab

You can use phabricator (and its command line tool Arcanist) with a repository in GitHub, Gitlab, or even your own local repo host. You just configure phabricator to use an external repo as the remote (by adding a URI to it and setting the mode to "observe"). See here: https://secure.phabricator.com/book/phabricator/article/diff...

Thanks for details.

If the following looked normal, it can be simplified:

  Create a new branch from master.
  Checkout the newly created branch.
  Create and push your fix commit.
  If everything is alright, file a pull request.
  Get it reviewed and merged.
  Switch back to master.

That seems like a lot of unnecessary branch switching. I'm hardly ever on master. Checkout with -b option, fetch --prune, rebase onto origin/master.

Edit: bonus tip `git checkout -` works like `cd -` but for branches.

"Edit: bonus tip `git checkout -` works like `cd -` but for branc"

Oooh, thanks!!!

How do you verify that everything is behind a feature flag? How do you verify that everything will work right once multiple feature flags are enabled? How much of a negative effect do feature flags have for reasoning about the software? For binary bloat? For performance? Are ordinary devs capable of doing this properly, or just google's superhumans?

Testing, and lots of it. How else? You can also expand the problem to general flags, not just WIP feature ones, and you have to deal with sellable/licensable flags, permission flags, version flags...

Big software is going to run into complexity sooner or later. You don't need superhuman skill to manage it but no single person will be able to hold the whole thing in their head.

An interesting approach to handling the combinatorial explosion of flags, if you desire to test all possible interactions (even though you don't typically need to) is instead limit yourself to the subset of all pairs of interactions (all-pairs testing).

Or, take a middle ground. PR early, PR often, and also feature flag new work. The main branch stays fairly close to synched with everyone, you don't get huge PRs, and you still get the benefits of code reviews and documentation of closed PRs.

My team tends to run like this. Many WIP PRs, and if nobody reviews and merges it in a timely fashion, we tend to go ahead and merge it anyway, after asking in slack if anyone wants to take a look before merging. PRs are an add-on to the process to help communication, not a gatekeeper to moving the codebase forward.

In my opinion, the cost of creating branches, checking out, creating PRs, and ensuring they are part of the appropriate release can't be ignored. I have seen it happen. The backlog is high but no one wants to pick an issue (say, a styling bug) even if they have time, just because the number of steps involved in getting it to production is too damn high. If you only had to worry about your change, there would be far fewer of those bugs.

If you only had to worry about your change, your work would be easy and you would be paid much less.

Is this high cost inherent to PRs or are there other processes around them (e.g., create a ticket with a long description) that raises the cost? I feel a certain costs for PRs is good: You better leave that styling bug unfixed, rather than breaking the whole website in production.

I don't feel that pain - it takes seconds to make a new branch, seconds to push it up, maybe 30 seconds to turn into a pr, add a description and some comments, and after it is review, merge and let CI/CD take over.

Now, without CI/CD, I could see it as far more painful. But I'm not sure that has anything to do with PRs. Even taking PRs out of the mix, you're still having to get it to production.

All of that can be automated via CLI.

Here are a few tools:

- hub https://github.com/github/hub

- lab https://github.com/zaquestion/lab

- git-extras https://github.com/tj/git-extras

I'm not sure to fully understand the alternative. Your commit still gets reviewed. Where does it sit while it is being reviewed? (a) on trunk, (b) on a separate "place" (just to avoid the word "branch").

If (a), then what happens if that commit needs to be reworked on? Will it lead to another commit on trunk?

If (b), then what happens if the trunk has moved by the time your commit got approved?

OP here. Apologies for not being clear. This is very similar to how Gerrit works. There's an additional stage before the commit lands on the trunk (where commits get reviewed). You rework your commit by amending/interactive rebasing and pushing again.

When you get approval, you can submit your commits and Gerrit will try to merge them (even if the trunk has moved; you can say it's sort of cherry-picking on the trunk). If it can't merge the commit, you'll have to take a pull and push your commit again by resolving conflicts.

Edit: Tweaked the post a little to make it more clear.

So the ‘additional stage’ works exactly like a branch then, right?

Except without all the benefits of having real branches that might be desirable.

To me OP's flow just reminds me of a particular way of doing development with perforce. You make your changes against one branch everyone works from, move them to a new 'pending changelist', then send the diff to another system for review. (You might even use perforce's own swarm tool by making a 'shelf' of your pending changes and starting a review from that.) Rework is done, you send over the new diff/overwrite the shelf, reviewers can see diff-of-diffs (tracked only by the review tool) and overall-diff. Eventually if all is well (or if your review has been in limbo and no one's going to argue if you merge without review anyway) you commit the pending changelist. (Which might involve in reality sending your diff again to another service, and that service tries to integrate and build/run tests using a new changelist in your name and only finalizing a commit to the master branch with that.)

I wouldn't recommend it. Not that I think the popular pull request model is much better, but at least you have options, though that's more to do with using a DVCS. I'd like to sometime try Fossil with a team, which aims to have the technical benefits of DVCS but with the tradeoff of cathedral vs bazaar being given back to the cathedral style, which is what I see pull requests as popularly done on github as clumsily trying to accomplish.

If I understand your post correctly, I agree with its substance: keep unreviewed changes small, probably a subclass of Kanban's "reduce WIP". This should be already known industry-wide. The Kubernetes Contributor guide states: "break large changes into a logical series of smaller patches which individually make easily understandable changes, and in aggregate solve a broader issue".

However, I feel that your technological choice is a bit nitpicking. I believe that creating an alias for 'git checkout master; git pull; git checkout my-branch' and another alias for 'git push; curl .../review' would essentially implement the same workflow as you suggest. (Granted, in some organizations, creating a branch is a heavy process: It requires opening a JIRA ticket, the branch needs to be well-named, etc. but these are not technological issues.)

It's quite possible to follow this or a similar workflow with merge / pull requests. In fact, this describes pretty closely what I do. I work off master, amending a single commit, periodically squashing or fixing up newer commits, and periodically rebasing. Then when I'm ready to deploy I push my local master to a remote staging branch (git push origin master:some_new_remote_branch_issue_xxxxx) and open a MR on Gitlab. Then, when the MR gets merged and deployed, I rebase my local master against remote master and my commit gets "eaten", thus putting me back on a clean master branch. I never leave master locally. Rinse, repeat.

How is this different from what you are suggesting?

This is a distinction without a difference. The "trunk based development" has exactly the same issue with merging when it comes time to submit: what you're submitting onto may have moved. All you've done is change the name.

Reply on news.ycombinator.com