7 ms·

Branches and PRs: institutionalised distrust?

11 points by wheresvic1 8y ago

andymoe 8y ago
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 8y ago
  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 8y ago
    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 8y ago
      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.
- benji-york 8y ago
  You might like this talk about code reviews from an art critique perspective: https://www.youtube.com/watch?v=lpWvYxEW09k https://www.youtube.com/watch?v=lpWvYxEW09k
jedbrown 8y ago
Nonsense. Code review increases quality and test suites take time to run. Topic branches and PRs create parallelism and accommodate asynchronous review.
- mping 8y ago
  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).
ndiscussion 8y ago
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 8y ago
  "Pilots don't use a checklist because they are dumb, they use a checklist because they are smart".
Raphmedia 8y ago
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 8y ago
  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 8y ago
    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.
  - Raphmedia 8y ago
    Because that way people "stamp" their work. There's no excuse, you can't lie to yourself about half done tasks. You made a PR and even approved it.
Scaevolus 8y ago
> "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.
microtherion 8y ago
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 8y ago
  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.
al2o3cr 8y ago
> 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.
alexandercrohde 8y ago
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.
tylertreat 8y ago
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.
Yen 8y ago
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 8y ago
  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.
whorleater 8y ago
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.
Michielvv 8y ago
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.
acjohnson55 8y ago
> 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 8y ago
Normal brain: commit directly to master. Expanding brain: give up distributed version control entirely. Galaxy brain: write directly to the deployment server.
jack9 8y ago
> 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).
mdekkers 8y ago
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.

11 points by wheresvic1 8y ago

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 8y ago
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 8y ago
  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 8y ago
    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.
benji-york 8y ago
You might like this talk about code reviews from an art critique perspective: https://www.youtube.com/watch?v=lpWvYxEW09k https://www.youtube.com/watch?v=lpWvYxEW09k

jedbrown 8y ago

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

mping 8y ago
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).

ndiscussion 8y ago

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 8y ago
"Pilots don't use a checklist because they are dumb, they use a checklist because they are smart".

Raphmedia 8y ago

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 8y ago
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 8y ago
  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.
- Raphmedia 8y ago
  Because that way people "stamp" their work. There's no excuse, you can't lie to yourself about half done tasks. You made a PR and even approved it.

Scaevolus 8y ago

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

microtherion 8y ago

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 8y ago
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.

al2o3cr 8y ago

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

alexandercrohde 8y ago

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.

tylertreat 8y ago

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.

Yen 8y ago

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 8y ago
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.

whorleater 8y ago

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.

Michielvv 8y ago

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.

acjohnson55 8y ago

> 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 8y ago

Normal brain: commit directly to master. Expanding brain: give up distributed version control entirely. Galaxy brain: write directly to the deployment server.

jack9 8y ago

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

mdekkers 8y ago

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.