
Branches and PRs: institutionalised distrust? - wheresvic1
https://www.jimkinsey.com/blog/feature-branching-institutionalised-distrust
======
Yen
I use branches and PRs when developing _solo_ , nevermind developing on a
team.

It's incredibly freeing to be able to say "This branch is treated as a
sandbox. The code is in flux, history is mutable." and "This (master) branch
is stable. Every single commit in it is good, passes all known tests and
linting, should be safe to rollback to, should be amenable to `git bisect`".

When developing on a team, even if I _can_ push directly to master, I usually
shouldn't and won't. I want a second pair of eyes on that code.

Does this "process-heavy" workflow mean I distrust myself? Sure. I expect that
I am human, make mistakes and oversights. I also expect that my coworkers are
human, and I'm not going to trust their solo efforts any more than my own.

If branches and PRs are institutionalized distrust, well, remember that a
healthy amount of distrust is good for you.

~~~
aequitas
I can only agree to this.

Personally I have stopped seeing branches as part of a distrust system and
PR's being some sort of gate-keeping/control mechanism. And more and more see
them as a healthy self caution.

I no longer see making a PR as something for others to judge my work and
disapprove of it, but more as an opening for collaboration and discussion.
More of a friendly "can we look at this together so we can make it even
better", as opposed to an authoritarian "I've done the best I can, I hope it
meets all requirements and I don't get complaints". Of course this depends on
how people view it at the other end of the line as well, and some people never
change. But overall having this look at things made my life as a (open source)
developer a lot less stressful.

------
ndiscussion
With Git, creating a branch and merging into master takes mere seconds.

It's not about distrust - it's about making systems reliable. People make
mistakes. Having things branched out from master means that those mistakes
aren't as damaging.

Using other branches does not require you to institute onerous restrictions. I
don't understand what this author is arguing against - it reminds me of a
comment on another recent submission:

> In his book, The Checklist Manifesto, Gawande reports that at the end of a
> big checklist pilot project for the World Health Organization (IIRC),
> participating surgeons were surveyed.

> • One of the survey questions was, in essence: Would the use of checklists
> improve your surgical practice? Most of the surgeons responded no. This was
> consistent with the resistance that Gawande encountered: I don't need no
> stinking checklist; I'm a good surgeon!

> • But the more-telling survey question was this: Would you want another
> surgeon to use a checklist if operating on you or one of your family
> members? Something like 93% of the surgeons surveyed responded ... yes.

[https://news.ycombinator.com/item?id=17358827](https://news.ycombinator.com/item?id=17358827)

~~~
aequitas
"Pilots don't use a checklist because they are dumb, they use a checklist
because they are smart".

------
tylertreat
There are several things wrong with this IMO.

First, branching/PRs are not automatically process-heavy. If it is for your
company, there's probably something else wrong.

Second, this process—if done right—dramatically INCREASES code quality and
DECREASES defects. This can be backed up with data. Code reviews, if they're
not rubber stamps, are extremely valuable.

Third, the trunk-based model described in the article often leads to big-bang
release branches. Master should be gold. This facilitates rapid and small
releases, which directly impacts the business. This can also be backed up by
data (e.g. see the Accelerate book).

Fourth, this model (again if done right), plays an instrumental role in
compliance. The article doesn't even mention compliance, which is a HUGE thing
bogging down a lot of companies trying to adopt DevOps practices right now. It
has nothing to do with discipline or trust.

------
alexandercrohde
I just don't think this makes its case well, though there is a case to be
made.

I have worked at places where one person should _never_ have the ability to
push unreviewed code into master because the company was huge and already
successful and it'd be a security risk.

I have also worked with coworkers who fetishized the process part of stuff and
wanted to create all sorts of rules with high overhead to solve entirely
hypothetical problems from their imagination.

The hard part is differentiating the two.

------
Raphmedia
There's nothing holding you back from setting up a process in which developers
can approve their own pull requests.

I worked with such a workflow before and it worked like a charm. You only ask
for a third-party approval when you know the feature you worked on is big and
might break something. It is all about trust.

The pull request interface is very good to show you all the code you worked on
and review everything before committing to master.

~~~
dtech
Why require PR approval if people can do it themselves? Then it just becomes
useless ceremony over allowing people to merge without approvals or PRs

~~~
wrs
PRs are a useful artifact that can trigger CI builds, provide an organized way
to discuss changes, record reasoning behind multi-commit changes, etc. etc.

------
jedbrown
Nonsense. Code review increases quality and test suites take time to run.
Topic branches and PRs create parallelism and accommodate asynchronous review.

~~~
mping
Also creates a huge backlog and a possible bottleneck in reviewing. Each
process must accommodate the maturity of the team, not the other way around.

I find that trunk based works extremely well combined with pairing and/or
ci/CD, it's the fastest way to deliver value if you have the checks in place
(mostly good tests and review through pairing).

------
microtherion
It's not about trust. It's about:

* Catching errors more quickly.

* Increasing the bus factor my having each bit of code read by at least two developers.

* Getting everybody on the same page, both by disseminating information, and by holding each other to a consistent coding style.

~~~
adrianmonk
To me, the coordination thing is the biggest unexpected benefit.

Sure, you can do coordination offline as an asynchronous thing. But making it
a blocking part of the commit process just gives you so much higher confidence
that it's going to actually happen.

Though really, you're even better off if, as much as possible, you do the
coordination and communication before the code review. If you have agreement
on basic approach before then, you can reduce the amount of last-minute stuff
to minor style changes, fixing errors, and cleanup.

Code review, as a business process, tends to push things this direction. If
you don't do reviews, you might skip coordination and communication. If you do
reviews, you know it's happening at some point, and it only takes a few bad
experiences of having to redo all your work to clue in that maybe those
discussions (which you are now definitely having) should be started earlier.

------
Scaevolus
> "People can break the build by pushing to master without running the tests
> first" \- is the problem here that people could break the build? Or is it
> the lack of trust in other devs to act with professional discipline?

> To me the latter issue is much bigger. If there isn't trust and discipline
> within a team, then that team is unlikely to be highly effective and may
> even slide into dysfunctionality.

I trust coworkers' discipline 95% of the time, but everyone makes mistakes.
Automated testing and code reviews catch bugs, and allow codebases to scale
from tiny teams to large teams.

------
whorleater
The goals of branches and PR is that programmers are _inherently_
untrustworthy. Large portions of code can easily contain hidden bugs and
complexity, and we've all written our fair share of code that would only ever
make sense to the person who wrote it. We haven't institutionalized the
distrust, we've simply formalized it.

------
al2o3cr
> not just pushing completed features to master, but every last _well-crafted_
> commit

(emphasis mine) If you're making "well-crafted" commits, you're staging them
somewhere. If it's not on a branch, then it's not sharable - but it's still
HAPPENING.

------
mdekkers
This is nonsense, and comes across as exactly the case the author is talking
about, a solution (no branches and PR's) looking for a problem (It must be
trust, who can argue against increasing trust in the team). In our team, we
have a huge amount of mutual trust and respect. We still PR, because it
catches issues early, and improves overall quality.

------
Michielvv
I wonder if he actually tried it. I always thought the same about PR's for
internal process, until I actually tried it. It made working with feature
branches much more convenient.

I mainly use it to test if features meet their functional description before
merging, but also reviewing the code at the same time does help to spot subtle
misunderstandings in the requirements.

I also disagree that it would hold back development of inexperienced members
of the team. Having the structure with PRs allows them to work on things that
they otherwise may not have been trusted with yet to work on at all.

------
jack9
> If there isn't trust and discipline within a team, then that team is
> unlikely to be highly effective and may even slide into dysfunctionality.

> building a process into a team which signifies a lack of trust is unlikely
> to ever improve matters

This is one of those "I can't help fix problems because you're just hopeless
if you have problems, but I can make perfect better" articles that come up
time to time about why you should use X (that nobody does in practice, or not
for very long).

------
acjohnson55
> The introduction of controlling processes is often a way of consolidating
> power structures or building them from scratch for later (ab)use - and I
> would always question people's motives in these cases. In such highly
> bureaucratic hierarchies, those on the lower levels often feel absolved of
> responsibility and that is not likely to end well, either.

This is a really great insight, and I'm glad I read the article twice, because
on the whole, I think it's a bit hysterical.

------
eyelidlessness
Normal brain: commit directly to master.

Expanding brain: give up distributed version control entirely.

Galaxy brain: write directly to the deployment server.

------
andymoe
Yes, they are a giant waste of time and in general slow things down and don’t
improve your ability to add more features over time. Bigger win from having
good tests and properly factored code encourage by shared owership (no
disincentives to change any code you see fit)

~~~
01100011
Depends on the team. Sometimes they're great, sometimes people just start
rubber-stamping the reviews because of time pressures, laziness, or lack of
familiarity with the code.

Right now, I'm on a team of 3. 2 embedded devs and 1 web dev. The web dev
keeps asking me to PR their web framework changes, but I'm not a web guy. The
other embedded dev has ADHD and can only rubberstamp my reviews. Basically I'm
just wasting time complying with the branch/PR/merge/tag cycle.

~~~
Yen
I've had a similar problem in the past. The things I've found to be very
helpful are:

1) Don't look at the PR diff as a whole, look at individual commits, and judge
each one, then read the PR diff.

2) If the commits are too big to keep in your head, don't make individual
sense, etc., ask your coworker to rewrite their history until you can do 1)

2b) If the individual commits make sense, but the PR still is too big to
comprehend, this PR should probably be split into multiple smaller PRs which
can be reviewed, approved, and released individually (or at least in
sequence).

3) If you lack still lack enough context to make a good review, ask your
coworker to walk you through the commit, and just explain out loud why they're
making the changes they're making. Sometimes, a "rubber-duck code review" is
enough. Even if it's not, any programmer can read and understand a single line
of code at a time, and you can likely tell if that line of code is complex or
elegant.

Plus, even if you don't know the full technical context of a proposed change,
people are fairly good at picking up whether another person has an explanation
that makes sense, or if they seem to be making it up as they go.

4) Spend the time and do the reviews, and become more familiar with that part
of the code. It sounds like right now, that part of the code has a bus factor
of 1.

5) Track time spent doing code reviews, and track bugs caught during code
review. Code review has notable benefits for your team/company, so justify and
sell that. You have fewer bugs & a better bus factor. When estimating your
next sprint (or other unit of work), know that you'll spend X hours per week
doing code review, and integrate that.

~~~
andymoe
I hear you, I’m just one of those pair programming zealots now. Whenever we
work with clients who insist on PR workflow eventually we just bury them in
PRs and it basically slows velocity to a crawl until we sort out the ownership
and start committing to master and get proper piplined ci in place. Or we work
with folks who have historically done PRs and think they are helping code
quality but everything is a mess because there’s so much resistance to
refactoring as part of normal work. No one wants to accept massive changes in
PRs.

