
The Case Against Pull Requests - shubhamjain
https://dev.to/shubhamjain/the-case-against-pull-requests-and-how-to-fix-it-533g
======
jacknews
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.

------
Niksko
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

------
yelirekim
He's making the same point that is made here:
[https://secure.phabricator.com/phame/post/view/766/write_rev...](https://secure.phabricator.com/phame/post/view/766/write_review_merge_publish_phabricator_review_workflow/)

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.

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

Namely?

~~~
yelirekim
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.

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

~~~
mark64
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...](https://secure.phabricator.com/book/phabricator/article/diffusion_uris/#observe-
a-repository)

~~~
majkinetor
Thanks for details.

------
karmakaze
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.

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

Oooh, thanks!!!

------
jepler
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?

~~~
Jach
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).

------
codingdave
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.

~~~
shubhamjain
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.

~~~
moltar
All of that can be automated via CLI.

Here are a few tools:

\- hub [https://github.com/github/hub](https://github.com/github/hub)

\- lab [https://github.com/zaquestion/lab](https://github.com/zaquestion/lab)

\- git-extras [https://github.com/tj/git-extras](https://github.com/tj/git-
extras)

------
anticristi
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?

~~~
shubhamjain
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.

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

~~~
Jach
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.

------
Blackthorn
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.

