9 ms·

Are pull requests bad because they originate from open-source development?

48 points by bigblind 3y ago

coldtea 3y ago
>"Pull requests were invented to gatekeep access to open-source projects. In open source, it's very common that not everyone is given free access to changing the code, so contributors will issue a pull request so that a trusted person can then approve the change. "I think this is really bad way to organise a development team. "If you can't trust your team mates to make changes carefully, then your version control system is not going to fix that for you." This guy (not the post author, but Dave Farley, the guy who wrote the above excerpt) has never heard of "trust but verify". PR is about verifying, having another set of eyes for anythine the original dev might have missed, giving an opinion before something on a branch goes into permanent git history, and so on. This is completely orthogonal with whether you trust your team members or not. Besides "blind trust" is not the only form of trust.
- jiggawatts 3y ago
  As far back as the year 2001, way before Git existed, a small startup I worked for used mandatory code review for all commits. You couldn't just upload whatever garbage and call it a day, it had to successfully compile and be reviewed by a randomly selected peer. These days Pull Requests are used to enable the same workflow.
  - winrid 3y ago
    Was the startup successful? :)
    - jiggawatts 3y ago
      Actually no, and IMHO a significant contribution to that failure was that they had bred a culture of cheating where developers would bypass these controls. For example, one of the managers would get his brother to approve his changes, and vice-versa, even if the code was total gibberish. Generally, the lesson I took away from that place is that you can blow $10 million in seed money and end up with worthless code if you don't pay attention to the basics. They were in the right market, at the right time, with the right product, and 3-5 years ahead of the competition, but you can't sell non-functioning software. My favourite example (of many) was that they used a source control system that could cache Java ".class" files to enable incremental compilation at all times, even without IDE support. This was literally a check-box that someone un-ticked years before I got there. I re-enabled it for myself as a local setting override and compile times went from 30 minutes to 5 seconds. That inner-loop speed-up was just insane, I could fix bugs in minutes that would take the rest of the team days. Speaking of code review, the code that I saw getting "rubber-stamped" was so bad it was unbelievable. One guy spent months working on multi-threaded animation code that I replaced with a single line of code. Which actually worked, didn't stutter, and didn't leak threads, or use threading at all for that matter. The essentials matter! Proper code review, well set up tooling, hygienic build pipelines, etc...
      - brazzy 3y ago
        Sounds to me that the real takeaway is that the essentials you list don't, in fact, matter if you have incompetent developers.
        RugnirViking 3y ago
        well certainly if you have incompetent developers in senior management, but is that a surprise to anyone? I don't think a good git workflow or whatever could ever fix something like that
        gorjusborg 3y ago
        To me, code review is a tool that can make incompetence a temporary condition. People have to be open to constructive feedback and want to grow, however.
        jiggawatts 3y ago
        Having a code review policy is like having speed limit signs on the motorway. Without enforcement, you can make the argument that speed limits don’t save lives because everyone drives as fast as they can anyway. Same thing for code review. If it’s not policed by management, then it does nothing. I saw one place that would send alert emails if a commit was approved too fast, assuming that someone must have had it insta-approved by a friend as a favour.
- arkaeologic 3y ago
  Just a small note: "trust but verify" is not a reasonable statement, and it was also unreasonable in its original context, which I encourage anyone to research. If you can verify, trust is not required.
  - funcDropShadow 3y ago
    How do you verify, that the requirements where correctly understood, if not by having another human look at it?
  - bluefirebrand 3y ago
    The idea is "trust early, verify eventually" As opposed to "trust blindly" The opportunity to verify immediately is rarely available.
  - ilyt 3y ago
    I think it is usually used in context of "we don't verify because we don't trust you but because mistakes always happen".
- mpweiher 3y ago
  > This guy ... has never heard of "trust but verify" I think he has, and actually knows where it comes from and what it means. "Trust but verify" is a Russian proverb that was used to describe the hoped-for and treaty-mediated relationship between the Soviet Union and the United States during the Reagan-Gorbachev era. So arch-enemies of a Cold War that are pointing world-ending nuclear arsenals at each other and slowly coming to realise that maybe it would be better to limit those by negotiated treaty. If that's a good description of the relationships in your team, then I think David Farley wasn't just right, but actually being very diplomatic ;-)
  - rob74 3y ago
    You can hopefully trust your colleagues to make a best-effort attempt to fulfill the requirements they are given and e.g. not intentionally introduce bugs or security issues, but everyone makes mistakes, so your software development workflow should account for this - and code review catches at least some of these mistakes.
  - whatshisface 3y ago
    Here are some examples of trust-but-verify relationships at work: - You are allowed to expense things while traveling, but someone will look at what you put on the company card afterwards. - They don't wait on your background check before moving ahead with interviews. - Someone really should look at your code, at some point, before it is released.
    - Cthulhu_ 3y ago
      And to extend the last point: before it is merged, to keep the mainline in a releasable state: 1. Continuous deployment means your mainline is sacred and always in a releasable state. 2. Unit tests and automated verifications only tell part of the story 3. Churn should be avoided (e.g. fixing code style issues); if fixes are made before it enters the mainline, it is avoided. 4. Code reviews should be asynchronous, because pulling someone out of their flow to switch to doing a code review is rude and unproductive.
      - 3y ago
- guntherhermann 3y ago
  "Trust but verify" is a nonsense statement. If you trust, you do not need to verify, and vice versa.
  - regularjack 3y ago
    People make mistakes.
  - dyingkneepad 3y ago
    Everybody introduces bugs, and for a lot of classes of bugs the best way to prevent them is manual code reviews. Not everything can be easily caught by CI and tools.
- SideburnsOfDoom 3y ago
  Committing directly to main does not imply total "blind trust" - there is still a version history. Not to mention, a test suite.
- gonzo41 3y ago
  Also, when you're working solo, PR's are just a great way to look at a bunch of changes before you smoosh everything together because even with a full history it's sometimes tricky unpicking everything.
  - pyrale 3y ago
    I believe in this context, PRs refers to a process in a team about steps to check code that goes into trunk, not to the tools that were developed to do the task.
- ritzaco 3y ago
  I think these days the emphasis has changed a bit from "required reviews" (more senior dev or peer signs off on code) to "request review" (developer __wants__ the review, because it's a good learning experience and a way to share responsibility for the code (and any problems it might cause down the line). Anecdotal, but I feel like I see more teams where that "Merge" button is available to most people, but in general doesn't get pressed until the code has at least one review, whereas before it was often only accessible to a select few, or a review was required before it activated.
  - WorldMaker 3y ago
    I got into a bit of a fight over whether or not "Merge" should be an option on one's own PRs. "Trust" suggests that a developer should be able to review their own code changes, especially for small/trivial things or "expert" level things that no one else feels qualified to review and just leave open for weeks/months instead of even trying to review it (in turn often making it harder to merge without conflicts). There's a great value in "every change was peer reviewed by someone", of course. There's also a value in "not every change is significant and sometimes you can trust developers to know that". You can possibly install the fear of accountability if a bug happens in code that the reviewer was the developer until they want that learning opportunity of a code review. In my experience, most of the "expert-level" developers almost always would love any feedback anyone wanted to offer because their imposter syndrome tells them it is awful and not to sign off on it, but are also more likely to get blocked if their PRs don't move fast enough.
- dahart 3y ago
  > PR is about verifying This is interesting as a counterpoint to trusting, but it’s funny, I’ve never thought about PRs in either of those terms, not within a team or company anyway. The Pull Request can also be about the code writer asking for review, asking the question ‘is this generally the right way to go?’ The funny thing about projecting ‘trust’ and ‘verification’ onto the process is that PRs simply do not prevent all or even most accidents, automated builds and tests are better at that. PRs can help steer things in the right direction, and they’re good as one of many tools for mentoring people about code they’re unfamiliar with. But PRs can also be used as the opposite of verification: if the reviewer is responsible for verification then PRs can be a CYA where the reviewer is implicated in a mistake along with the writer.
  - lowercased 3y ago
    I've had groups where I eventually 'approved' a PR, because... deadlines... even when it wasn't meeting defined standards (primarily around tests/docs). Comments like "please add a test" or "please document X" were just ignored (both inside the version control system and in slack). Eventually, I would merge, and occasionally, we'd hit some bug, trace it back to that, and I would catch blame for "passing" it. Not the person who wrote it, not the same person who didn't test it or add any automated tests. I did, even though I was doing so under protest. The couple times I added to their PR - added a test to verify behaviour, for example - I got called out for "stepping on others' code". Apparently if you went out on smoke breaks with 'the guys', they were 'cool' wit h you and that was the political way of getting people 'on your side' to go along with stuff. Very ... odd dynamics. Apparently, if you're dating the dept head's niece, writing tests was optional, but I never got that memo.
    - recursive 3y ago
      Simple solution to that is don't pass it. When the deadline comes, you'll have the reason documented.
    - dkersten 3y ago
      That sounds like a team problem. In my team, contents get addressed and team members don’t approve PR’s they don’t feel meet standards even if there are deadlines.
- SkyBelow 3y ago
  >trust but verify If you have to verify, is there really trust? We need to get to the point that a lack of trust isn't some inherently bad thing. I don't trust myself to never make mistakes. A PR is a way to get others to add a layer of verification to help catch bugs. I don't even trust the PR to catch all bugs, but it is a cost effective for the amount of bugs it does catch. Why is this lack of trust a bad thing? Humans are imperfect, we should build systems that require verification because we cannot trust ourselves to never make a mistake.
  - burnished 3y ago
    In this context if you arent trusted then your PR isnt even looked at.
- ilyt 3y ago
  > This guy (not the post author, but Dave Farley, the guy who wrote the above excerpt) has never heard of "trust but verify". "This guy" sounds like someone that never worked on project size bigger than one. It is utterly idiotic take, on every level.
- nanidin 3y ago
  A PR is just a properly scoped nugget of review-ready changes. A formalized review process helps way more than it hurts in the long run.
- dkersten 3y ago
  Yeah. I trust my team mates, but we use PR’s and approvals as a means of doing code review to make sure things don’t slip through and that everything has had multiple eyes. It’s not about trust, but improving quality, because we’re humans and humans miss things or make mistakes. It also provides a nice record of why changes were added, a place to link tickets/documentation, automaton hooks etc. I love PR’s as a concept and tool. I also often use PR’s to merge changes on my personal projects even when I’m the only developer.
eikenberry 3y ago
Pull requests enforce coding conventions. The simple fact that someone is going to look at the code you're contributing is enough of an impetus to write better code. Prevents us from self rationalizing lazy decisions and the like. IMO that is really the idea, to mimic that extra level of care that is taken like when submitting to an open source project.
v3ss0n 3y ago
[flagged]
- flohofwoe 3y ago
  Up to about 10 years ago I used to work on projects where up to 100 people (of those about 20 coders, the rest artists and designers) committed directly into the main branch without a formal review process, and it was ok (this was with SVN though). Code review happened informally "after the fact", usually as the first thing in the morning when updating (each person simply looked through the changes of other people in the part of the project they felt responsible for - usually those changes were no surprises because people talked to each other before doing this). "Post-commit" breakage was surprisingly rare, and when it happened it was quickly fixed. One just was a bit more careful checking for errors before committing changes. I cannot imagine how this scenario could work with git though.
  - lukeramsden 3y ago
    Works absolutely fine with git, you just have to take the time to keep the pre-commit checks reasonably quick
    - flohofwoe 3y ago
      IMHO what makes it harder in git (for many people committing into the same branch) is that each team member has its own local history which needs to be synchronised via push and fetch (which quickly gets out of control for 'non-technical' team members). This separate step doesn't exist in centralized version control systems like SVN where everybody is always on the same timeline. Working exclusively on branches and then merging into a common main branch pretty much fixes this problem though (that's why the PR workflow make a lot more sense in git than svn).
      - ilyt 3y ago
        You can make it do automatic rebase on pull to make it look more like SVN single history. We do that on our CI repo because of many tiny changes of unrelated parts that otherwise just made almost every second commit into a merge
gorgoiler 3y ago
I’ve yet to try reviewing code locally, instead of in a browser. Does anyone here had a good experience of how to review code outside of the traditional browser based tools? This topic reminds of something Joel Spolsky told me about Stack Exchange, in the context of social networks. Giving users the ability quote text when replying is something they really wanted to avoid because it causes arguments to devolve into semantics — far more so than if people had to write out responses in full. A tool like quote-replying shapes (and deforms) our ability to have productive debate. The unit of change in software is a patch and yet most of us use completely different tools for writing patches, reading our own patches, and reading other peoples (IDE/editor, git diff/show, and GitHub/Lab respectively.) I like the change of context that web based patch review gives me. I often find bugs or untidiness when looking at my own changes in a browser. When working with others, I’ve found the best code review to be about collaboration and sharing of ideas and responsibility, picking out unseen errors, adding overlooked refinements, giving advice, and improving the team’s bus factor. If we’re supposed to be playing co-op instead of death-match, shouldn’t we all be in the same level?
the_gipsy 3y ago
In OSS, after merging a PR, generally you can't get back at the author and make them do a follow-up change, because they are volunteers. A certain degree of gatekeeping is necessary. In a company however, you absolutely can just assign such a task to the author or someone else. That means it's possible to integrate changes into the main branch faster, which is very beneficial. I've seen teams that try to mimic OSS development and only merge absolutely perfect PRs, dragging out long-living branches and integration. At some point the perfectness can't keep up with the plan, huge PRs are suddenly reviewed hastily, and the end result is worse and slower. Bottomline: PRs can be used slightly differently in companies to improve speed and reduce overhead.
- dherikb 3y ago
  I have a similar opinion. Also if the team is mature enough, the PR can be reviewed after the merge, so the necessary changes can be made with the current code already integrated and already running in production. The downside for this is that the team needs to have some discipline to review the already merged PRs and the developers needs to be disciplined to organize your time to make the fixes after the PR merged.
- schwartzworld 3y ago
  Big PRs with lots of changes are a smell of their own. If you're seeing this cycle, then PRs are just too big and should be broken into smaller tasks. Of course you're going to miss stuff and have a long review cycle if you have 20+ files with meaningful changes in a PR.
  - the_gipsy 3y ago
    I'm specifically talking about small (or reasonable) tasks that get blown up in the PR phase because of perfectionist reviewers. Requesting for example a big architectural change in a pull request is not a good idea IMO. Ideally it should have happened in the design phase, but overspecifying everything is not the solution. Now, in a company, a request for a bigger change can simply be mentioned in the PR, and the tasks can then be created and assigned as follow ups. This is usually not possible in OSS; the author of the PR might agree to the necessary changes but later not do them for whatever reason, and there might be no other volunteers to "clean up". Therefore it does makes sense to gatekeep and drag out some PRs in OSS.
flohofwoe 3y ago
IMHO PR/MRs mainly make sense specifically because git sucks for multiple people working on the same branch, so it makes more sense to split even small work items off into a separate branch. And as an UI workflows which 'formalises' the discussion that needs to happen around a merge anyway they are quite nice. (e.g. without PRs you still need to talk about "hey, I'm going to do this thing" => "hey this thing is ready" => "ok, let's merge this thing")
lone-commenter 3y ago
Relevant paragraph IMO: > First, it includes an appeal to trust, which is a line of reasoning with which I don't agree. You can't trust your colleagues, just like you can't trust yourself. A code review serves more purposes than keeping malicious actors out of the code base. It also helps catch mistakes, security issues, or misunderstandings. It can also improve shared understanding of common goals and standards. Yes, this is also possible with other means, such as pair or ensemble programming, but from that, it doesn't follow that code reviews can't do that. They can. I've lived that dream. I think that the other counter-argument discussed in the article can't confute Farley because, contrary to the author's stated intentions, it is in fact a straw man. The author focuses on the benefits of "asynchronous" workflows over "synchronous" ones: i.e., in his words, reviewing pull requests is easier (for an introvert like himself, at least) than dealing with the social stress involved in pair/ensemble programming.
Lapsa 3y ago
no
someweirdperson 3y ago
Calling pull requests "pull request" when it is a job to create a change is what's wrong. The author isn't actually requesting anything. But what else could be a nice short term for "collection of changes with technically enforced reviews and tests"?
- HPsquared 3y ago
  Change proposal
- alpaca128 3y ago
  Review submission?
- giantrobot 3y ago
  It's a "pull request" because you're asking someone to pull a commit from your branch/repo into theirs. You're quite literally making a request. This actual change could be anything. You could just as well provide them as a patch they could apply by hand. The only formal aspects of the process, e.g. reviews and tests, are organizational. As an organization the acceptance of a pull request doesn't need a bunch of reviews and tests associated.
- ilyt 3y ago
  The problem is again, Github Originally, "pull request" was exactly what name implied, someone asked you to pull from their repository. The original Git collaboration model was that, you pulling changes from other repos and integrating them in your own, vs handing merge request. Github co-opted the term to mean exact opposite, accepting essentially push from different repo.
- javier_e06 3y ago
  I couldn't agree more. When the PR is approved then the button to complete the PR in Github is (drum-roll please) MERGE. And who is doing the merge? Me. Gitlab calls it merge request.
alex-moon 3y ago
"If you can't trust your team mates to make changes carefully..." Buddy I don't trust myself to make changes carefully The way I'd put this is: writing code and reviewing code are - not two different skillsets maybe, but - two different headspaces. Code review is a kind of "technical QA", and indeed I am pretty sure that you see the same basic practice in factory work (where we get the concept of "QA" from).
Cthulhu_ 3y ago
> If you can't trust your team mates to make changes carefully, then your version control system is not going to fix that for you. I don't trust myself either; trusting your own or others code is hubris. The two-person rule - or more - is applied EVERYWHERE.
zabil 3y ago
We use pull requests to spin out ephemeral environments and test the changes before merging to trunk. All merges to main is deployed across environments so pull requests help us get faster feedback if something fails. I think pull requests get a bad rep because it is thought as a review process. We see them as short lived branches. They actually improve code quality and unwanted commits even without manual review/approval.
kerkeslager 3y ago
> "Pull requests were invented to gatekeep access to open-source projects. In open source, it's very common that not everyone is given free access to changing the code, so contributors will issue a pull request so that a trusted person can then approve the change. > "I think this is really bad way to organise a development team. > "If you can't trust your team mates to make changes carefully, then your version control system is not going to fix that for you." Man, I don't trust myself to be perfect. I trust myself to be careful but no amount of care makes me infallible. I make mistakes. And if we can catch my mistakes early by getting a second set of eyes on them, they're less painful for everyone to fix. When someone spends their valuable time catching my mistakes, they're doing me a favor. Distrust might be part of the origin of pull requests, but that doesn't have to be the reason why your team uses pull requests. It can be about catching each others mistakes to learn, create a better product, and get better rewards. The tool is yours to use how you see fit--don't let ego prevent you from making it useful.
quadhome 3y ago
A pull request is when one maintainer asks another to merge branches. If you’re interpreting this through the lens of a single main/master/trunk and multiple branches, then like both Farley and the author, you’re trying to interpret a foreign culture (distributed version control) through a parochial lens (single trunk-multiple branch). The DVCS world is one of many sources of truth. For example, in the kernel community, Red Hat, Google, Debian, Android, etc. all have their own branches. Linus has a particularly popular branch. None of them are the main branch.
teekert 3y ago
I think we have GH set up so we can make direct pushes to even main, we just agreed to do PRs because frankly I don’t even trust myself to do everything right and optimal. PRs are nice for reviews, but also for testing and discussion, sometimes just to share something you are not sure if it is the best. It’s just a way to present changes to people just trust (or don’t) and to have a place for discussion. That’s how we treat PRs anyway. Occasionally I or a colleague indeed accepts their own PR (or use direct pushing) if it’s really minor (think a doc string) and need to get something out. So PRs are not just for envs without trust.
jabradoodle 3y ago
It frustrates me the one size fits all approach often taken to software development best practices. If no one is to review the code, then all code is either written by pairing/mobbing, or else it is the product of one persons thinking, blindspots, biases and all. It's great if you work in a team that pairs in everything, does trunk based development etc; that doesn't make it suitable for every team.
madsbuch 3y ago
It seems like we are reaching the same conclusions as Dave Farley on our development team. We have pivoted to just let everyone merge in their stuff and do "reviews" retroactively when we stumble upon code not up to the standard we set. The reasoning behind this is exactly that we trust individuals on the team to merge. and because reviewing in the traditional sense seems to be a formalisation of distrust: Something you do when you don't trust your peers to implement and put in production properly tested code.
- PretzelPirate 3y ago
  I would certainly fail my SOC2 audits if I told the auditor that anyone on the team can merge in any unreviewed change and that gets pushed to production, but not to worry be cause we'll catch any issues eventually. I trust my peers, but we all make mistakes and we do code reviews because we understand that working together can catch issues sooner. We aren't overly picky or mean during code reviews, but we make sure there aren't any glaring issues, and if there are, we talk about them and work together to fix them.
  - madsbuch 3y ago
    Seems like you are at a place where you need to be SOC2 compliant. Good for you that you are able to satisfy those requirements.
pyrale 3y ago
With all the precautions the author takes, he ends up with a weird take: > If you take away the appeal to trust, though, there isn't much left of the argument. What remains is essentially: Pull requests were invented to solve a particular problem in open-source development. Internal software development is not open source. Pull requests are bad for internal software development. The specific point he tries to reword exists to explain that not everyone needs as much trust than open-source, and, therefore, that not having PR isn't a fatal flaw outside of open-source. The people who use it do so within a discourse along with other points explaining what they gain from forgoing PRs (usually, better continuous integration). I don't buy the whole discourse about not using PRs, but I'm also not sold on the idea of dismantling a coherent discourse into separate talking points and demanding that they should stand separately. I'm also not a fan of rewording someone else's point and slipping a big non-sequitur in that rewording.
onion2k 3y ago
I don't really have any opinion about PRs versus trunk-based development, but I do have a small insight that I think is useful when you're thinking about which one to choose. The main advantage of trunk-based dev is speed. You don't have to wait for a review of work. You're trusting the team to merge things that won't break stuff. That's awesome, but if that's the case then you should also be able to trust them to review PRs quickly enough that they don't block work. Equally the converse is true - if you can trust your team is motivated enough to review PRs quickly they you can probably trust them to do work together well enough not to really need PRs. If you can trust the team with one and not the other then something is wrong. There shouldn't be any "this way is better than this other way" argument. Your team should be capable of doing both. If it isn't then neither will work very well. You're going to have problems, conflicts, bugs, etc no matter what you do. Believing that the team should stop PRs because they're too slow, or they should stop trunk-based dev because pairing takes too much effort, are both red flags. They're both saying that the team has a problem to address.
fmajid 3y ago
It's fine to allow some senior engineers to merge a pull request without going through CI and/or code review, but the discipline the PR process introduces is highly desirable, whether working on open-source or not. It's not a question of trust, it's a question of avoiding unchallenged assumptions and blind spots.
usrusr 3y ago
Isn't there a bit of an elephant at the other end of the room? PR are not only about placing a bouncer at the club door to, figuratively speaking, check entering members for excessive drunkenness, they are about placing a bouncer at the door to give access to a wider group. This is quite obvious in open source, where the PR model has been a revolution in terms of lowering barriers of entry. You can join the party without asking for the keys. But similar barriers exist in companies. When they don't exits on the technical level (they usually do, sometimes to the point where collaborating teams wouldn't even know what version control the other side is using), they exist on the social level and a model like PR/MR can help nudge both towards the "internal open source" mindset.
papito 3y ago
"then your version control system is not going to fix that for you." Uh... Pull requests are not a version control workflow - it's a human workflow. It's also a way for the team to actually LOOK at the code that is going in and to be aware of what changes are incoming. I mean, that's just one point, there are so many ways in which this view is fatally flawed, I am not even going to... ugh. I know this is not trolling, but this is so scandalous, it comes across that way. This is beyond a "hot take".
dwheeler 3y ago
I disagree: it's about humility. We all make mistakes. Having a review process means we are humble enough to admit it.
Illniyar 3y ago
PRs might have been an open source innovation, but not "trusting" your developers has a very long history that predates OSS and PRs. The way it was done was to have QA check your code, usually as part of a release. I think PRs became more popular in non-OSS because of CI/CD and agile methodologies.
muskmusk 3y ago
Pr's are not bad. But using them to solve things that should be solved other things like catching bugs and enforcing style guides is bad. If doing a pr takes longer than 5 minutes in general then consider if you are relying too much on pr's.
manuelabeledo 3y ago
It seems that the quote comes from a big proponent of pair programming. Am I missing something, or is the irony lost on this one.
javier_e06 3y ago
Torvalds in one famous Google presentation he talked about the "circle of trust" in relation of how we all should handle sharing code as in: "Is my repo and I accept code from a selective group of trusted individuals" In the corporate world is more like Seinfield's Soup Nazi skit. We are all working towards AI accepting or rejecting code changes. Humans have egos and their idiosyncratic bias/baggage get in the way of adopting or rejecting code changes in an objective manner. Githook 50/72 rule anyone?
zajio1am 3y ago
The original purpose of PR was to accept patches from outsiders (people without write access to the project repository), as an alternative to sending patches to the project mailing list. In open-source development if there is a core team with write access to the project repository, i would be surprised if all patches from members of the core team were merged through PR process.
BerislavLopac 3y ago
This article recommends using merge requests (I'm not a big fan of the term pull request) only in specific cases: https://martinfowler.com/articles/ship-show-ask.html https://martinfowler.com/articles/ship-show-ask.html

48 points by bigblind 3y ago

>"Pull requests were invented to gatekeep access to open-source projects. In open source, it's very common that not everyone is given free access to changing the code, so contributors will issue a pull request so that a trusted person can then approve the change. "I think this is really bad way to organise a development team. "If you can't trust your team mates to make changes carefully, then your version control system is not going to fix that for you." This guy (not the post author, but Dave Farley, the guy who wrote the above excerpt) has never heard of "trust but verify". PR is about verifying, having another set of eyes for anythine the original dev might have missed, giving an opinion before something on a branch goes into permanent git history, and so on. This is completely orthogonal with whether you trust your team members or not. Besides "blind trust" is not the only form of trust.

jiggawatts 3y ago
As far back as the year 2001, way before Git existed, a small startup I worked for used mandatory code review for all commits. You couldn't just upload whatever garbage and call it a day, it had to successfully compile and be reviewed by a randomly selected peer. These days Pull Requests are used to enable the same workflow.
- winrid 3y ago
  Was the startup successful? :)
  - jiggawatts 3y ago
    Actually no, and IMHO a significant contribution to that failure was that they had bred a culture of cheating where developers would bypass these controls. For example, one of the managers would get his brother to approve his changes, and vice-versa, even if the code was total gibberish. Generally, the lesson I took away from that place is that you can blow $10 million in seed money and end up with worthless code if you don't pay attention to the basics. They were in the right market, at the right time, with the right product, and 3-5 years ahead of the competition, but you can't sell non-functioning software. My favourite example (of many) was that they used a source control system that could cache Java ".class" files to enable incremental compilation at all times, even without IDE support. This was literally a check-box that someone un-ticked years before I got there. I re-enabled it for myself as a local setting override and compile times went from 30 minutes to 5 seconds. That inner-loop speed-up was just insane, I could fix bugs in minutes that would take the rest of the team days. Speaking of code review, the code that I saw getting "rubber-stamped" was so bad it was unbelievable. One guy spent months working on multi-threaded animation code that I replaced with a single line of code. Which actually worked, didn't stutter, and didn't leak threads, or use threading at all for that matter. The essentials matter! Proper code review, well set up tooling, hygienic build pipelines, etc...
    - brazzy 3y ago
      Sounds to me that the real takeaway is that the essentials you list don't, in fact, matter if you have incompetent developers.
      - RugnirViking 3y ago
        well certainly if you have incompetent developers in senior management, but is that a surprise to anyone? I don't think a good git workflow or whatever could ever fix something like that
      - gorjusborg 3y ago
        To me, code review is a tool that can make incompetence a temporary condition. People have to be open to constructive feedback and want to grow, however.
      - jiggawatts 3y ago
        Having a code review policy is like having speed limit signs on the motorway. Without enforcement, you can make the argument that speed limits don’t save lives because everyone drives as fast as they can anyway. Same thing for code review. If it’s not policed by management, then it does nothing. I saw one place that would send alert emails if a commit was approved too fast, assuming that someone must have had it insta-approved by a friend as a favour.
arkaeologic 3y ago
Just a small note: "trust but verify" is not a reasonable statement, and it was also unreasonable in its original context, which I encourage anyone to research. If you can verify, trust is not required.
- funcDropShadow 3y ago
  How do you verify, that the requirements where correctly understood, if not by having another human look at it?
- bluefirebrand 3y ago
  The idea is "trust early, verify eventually" As opposed to "trust blindly" The opportunity to verify immediately is rarely available.
- ilyt 3y ago
  I think it is usually used in context of "we don't verify because we don't trust you but because mistakes always happen".
mpweiher 3y ago
> This guy ... has never heard of "trust but verify" I think he has, and actually knows where it comes from and what it means. "Trust but verify" is a Russian proverb that was used to describe the hoped-for and treaty-mediated relationship between the Soviet Union and the United States during the Reagan-Gorbachev era. So arch-enemies of a Cold War that are pointing world-ending nuclear arsenals at each other and slowly coming to realise that maybe it would be better to limit those by negotiated treaty. If that's a good description of the relationships in your team, then I think David Farley wasn't just right, but actually being very diplomatic ;-)
- rob74 3y ago
  You can hopefully trust your colleagues to make a best-effort attempt to fulfill the requirements they are given and e.g. not intentionally introduce bugs or security issues, but everyone makes mistakes, so your software development workflow should account for this - and code review catches at least some of these mistakes.
- whatshisface 3y ago
  Here are some examples of trust-but-verify relationships at work: - You are allowed to expense things while traveling, but someone will look at what you put on the company card afterwards. - They don't wait on your background check before moving ahead with interviews. - Someone really should look at your code, at some point, before it is released.
  - Cthulhu_ 3y ago
    And to extend the last point: before it is merged, to keep the mainline in a releasable state: 1. Continuous deployment means your mainline is sacred and always in a releasable state. 2. Unit tests and automated verifications only tell part of the story 3. Churn should be avoided (e.g. fixing code style issues); if fixes are made before it enters the mainline, it is avoided. 4. Code reviews should be asynchronous, because pulling someone out of their flow to switch to doing a code review is rude and unproductive.
    - 3y ago
guntherhermann 3y ago
"Trust but verify" is a nonsense statement. If you trust, you do not need to verify, and vice versa.
- regularjack 3y ago
  People make mistakes.
- dyingkneepad 3y ago
  Everybody introduces bugs, and for a lot of classes of bugs the best way to prevent them is manual code reviews. Not everything can be easily caught by CI and tools.
SideburnsOfDoom 3y ago
Committing directly to main does not imply total "blind trust" - there is still a version history. Not to mention, a test suite.
gonzo41 3y ago
Also, when you're working solo, PR's are just a great way to look at a bunch of changes before you smoosh everything together because even with a full history it's sometimes tricky unpicking everything.
- pyrale 3y ago
  I believe in this context, PRs refers to a process in a team about steps to check code that goes into trunk, not to the tools that were developed to do the task.
ritzaco 3y ago
I think these days the emphasis has changed a bit from "required reviews" (more senior dev or peer signs off on code) to "request review" (developer __wants__ the review, because it's a good learning experience and a way to share responsibility for the code (and any problems it might cause down the line). Anecdotal, but I feel like I see more teams where that "Merge" button is available to most people, but in general doesn't get pressed until the code has at least one review, whereas before it was often only accessible to a select few, or a review was required before it activated.
- WorldMaker 3y ago
  I got into a bit of a fight over whether or not "Merge" should be an option on one's own PRs. "Trust" suggests that a developer should be able to review their own code changes, especially for small/trivial things or "expert" level things that no one else feels qualified to review and just leave open for weeks/months instead of even trying to review it (in turn often making it harder to merge without conflicts). There's a great value in "every change was peer reviewed by someone", of course. There's also a value in "not every change is significant and sometimes you can trust developers to know that". You can possibly install the fear of accountability if a bug happens in code that the reviewer was the developer until they want that learning opportunity of a code review. In my experience, most of the "expert-level" developers almost always would love any feedback anyone wanted to offer because their imposter syndrome tells them it is awful and not to sign off on it, but are also more likely to get blocked if their PRs don't move fast enough.
dahart 3y ago
> PR is about verifying This is interesting as a counterpoint to trusting, but it’s funny, I’ve never thought about PRs in either of those terms, not within a team or company anyway. The Pull Request can also be about the code writer asking for review, asking the question ‘is this generally the right way to go?’ The funny thing about projecting ‘trust’ and ‘verification’ onto the process is that PRs simply do not prevent all or even most accidents, automated builds and tests are better at that. PRs can help steer things in the right direction, and they’re good as one of many tools for mentoring people about code they’re unfamiliar with. But PRs can also be used as the opposite of verification: if the reviewer is responsible for verification then PRs can be a CYA where the reviewer is implicated in a mistake along with the writer.
- lowercased 3y ago
  I've had groups where I eventually 'approved' a PR, because... deadlines... even when it wasn't meeting defined standards (primarily around tests/docs). Comments like "please add a test" or "please document X" were just ignored (both inside the version control system and in slack). Eventually, I would merge, and occasionally, we'd hit some bug, trace it back to that, and I would catch blame for "passing" it. Not the person who wrote it, not the same person who didn't test it or add any automated tests. I did, even though I was doing so under protest. The couple times I added to their PR - added a test to verify behaviour, for example - I got called out for "stepping on others' code". Apparently if you went out on smoke breaks with 'the guys', they were 'cool' wit h you and that was the political way of getting people 'on your side' to go along with stuff. Very ... odd dynamics. Apparently, if you're dating the dept head's niece, writing tests was optional, but I never got that memo.
  - recursive 3y ago
    Simple solution to that is don't pass it. When the deadline comes, you'll have the reason documented.
  - dkersten 3y ago
    That sounds like a team problem. In my team, contents get addressed and team members don’t approve PR’s they don’t feel meet standards even if there are deadlines.
SkyBelow 3y ago
>trust but verify If you have to verify, is there really trust? We need to get to the point that a lack of trust isn't some inherently bad thing. I don't trust myself to never make mistakes. A PR is a way to get others to add a layer of verification to help catch bugs. I don't even trust the PR to catch all bugs, but it is a cost effective for the amount of bugs it does catch. Why is this lack of trust a bad thing? Humans are imperfect, we should build systems that require verification because we cannot trust ourselves to never make a mistake.
- burnished 3y ago
  In this context if you arent trusted then your PR isnt even looked at.
ilyt 3y ago
> This guy (not the post author, but Dave Farley, the guy who wrote the above excerpt) has never heard of "trust but verify". "This guy" sounds like someone that never worked on project size bigger than one. It is utterly idiotic take, on every level.
nanidin 3y ago
A PR is just a properly scoped nugget of review-ready changes. A formalized review process helps way more than it hurts in the long run.
dkersten 3y ago
Yeah. I trust my team mates, but we use PR’s and approvals as a means of doing code review to make sure things don’t slip through and that everything has had multiple eyes. It’s not about trust, but improving quality, because we’re humans and humans miss things or make mistakes. It also provides a nice record of why changes were added, a place to link tickets/documentation, automaton hooks etc. I love PR’s as a concept and tool. I also often use PR’s to merge changes on my personal projects even when I’m the only developer.

eikenberry 3y ago

Pull requests enforce coding conventions. The simple fact that someone is going to look at the code you're contributing is enough of an impetus to write better code. Prevents us from self rationalizing lazy decisions and the like. IMO that is really the idea, to mimic that extra level of care that is taken like when submitting to an open source project.

v3ss0n 3y ago

[flagged]

flohofwoe 3y ago
Up to about 10 years ago I used to work on projects where up to 100 people (of those about 20 coders, the rest artists and designers) committed directly into the main branch without a formal review process, and it was ok (this was with SVN though). Code review happened informally "after the fact", usually as the first thing in the morning when updating (each person simply looked through the changes of other people in the part of the project they felt responsible for - usually those changes were no surprises because people talked to each other before doing this). "Post-commit" breakage was surprisingly rare, and when it happened it was quickly fixed. One just was a bit more careful checking for errors before committing changes. I cannot imagine how this scenario could work with git though.
- lukeramsden 3y ago
  Works absolutely fine with git, you just have to take the time to keep the pre-commit checks reasonably quick
  - flohofwoe 3y ago
    IMHO what makes it harder in git (for many people committing into the same branch) is that each team member has its own local history which needs to be synchronised via push and fetch (which quickly gets out of control for 'non-technical' team members). This separate step doesn't exist in centralized version control systems like SVN where everybody is always on the same timeline. Working exclusively on branches and then merging into a common main branch pretty much fixes this problem though (that's why the PR workflow make a lot more sense in git than svn).
    - ilyt 3y ago
      You can make it do automatic rebase on pull to make it look more like SVN single history. We do that on our CI repo because of many tiny changes of unrelated parts that otherwise just made almost every second commit into a merge

gorgoiler 3y ago

I’ve yet to try reviewing code locally, instead of in a browser. Does anyone here had a good experience of how to review code outside of the traditional browser based tools? This topic reminds of something Joel Spolsky told me about Stack Exchange, in the context of social networks. Giving users the ability quote text when replying is something they really wanted to avoid because it causes arguments to devolve into semantics — far more so than if people had to write out responses in full. A tool like quote-replying shapes (and deforms) our ability to have productive debate. The unit of change in software is a patch and yet most of us use completely different tools for writing patches, reading our own patches, and reading other peoples (IDE/editor, git diff/show, and GitHub/Lab respectively.) I like the change of context that web based patch review gives me. I often find bugs or untidiness when looking at my own changes in a browser. When working with others, I’ve found the best code review to be about collaboration and sharing of ideas and responsibility, picking out unseen errors, adding overlooked refinements, giving advice, and improving the team’s bus factor. If we’re supposed to be playing co-op instead of death-match, shouldn’t we all be in the same level?

the_gipsy 3y ago

In OSS, after merging a PR, generally you can't get back at the author and make them do a follow-up change, because they are volunteers. A certain degree of gatekeeping is necessary. In a company however, you absolutely can just assign such a task to the author or someone else. That means it's possible to integrate changes into the main branch faster, which is very beneficial. I've seen teams that try to mimic OSS development and only merge absolutely perfect PRs, dragging out long-living branches and integration. At some point the perfectness can't keep up with the plan, huge PRs are suddenly reviewed hastily, and the end result is worse and slower. Bottomline: PRs can be used slightly differently in companies to improve speed and reduce overhead.

dherikb 3y ago
I have a similar opinion. Also if the team is mature enough, the PR can be reviewed after the merge, so the necessary changes can be made with the current code already integrated and already running in production. The downside for this is that the team needs to have some discipline to review the already merged PRs and the developers needs to be disciplined to organize your time to make the fixes after the PR merged.
schwartzworld 3y ago
Big PRs with lots of changes are a smell of their own. If you're seeing this cycle, then PRs are just too big and should be broken into smaller tasks. Of course you're going to miss stuff and have a long review cycle if you have 20+ files with meaningful changes in a PR.
- the_gipsy 3y ago
  I'm specifically talking about small (or reasonable) tasks that get blown up in the PR phase because of perfectionist reviewers. Requesting for example a big architectural change in a pull request is not a good idea IMO. Ideally it should have happened in the design phase, but overspecifying everything is not the solution. Now, in a company, a request for a bigger change can simply be mentioned in the PR, and the tasks can then be created and assigned as follow ups. This is usually not possible in OSS; the author of the PR might agree to the necessary changes but later not do them for whatever reason, and there might be no other volunteers to "clean up". Therefore it does makes sense to gatekeep and drag out some PRs in OSS.

flohofwoe 3y ago

IMHO PR/MRs mainly make sense specifically because git sucks for multiple people working on the same branch, so it makes more sense to split even small work items off into a separate branch. And as an UI workflows which 'formalises' the discussion that needs to happen around a merge anyway they are quite nice. (e.g. without PRs you still need to talk about "hey, I'm going to do this thing" => "hey this thing is ready" => "ok, let's merge this thing")

lone-commenter 3y ago

Relevant paragraph IMO: > First, it includes an appeal to trust, which is a line of reasoning with which I don't agree. You can't trust your colleagues, just like you can't trust yourself. A code review serves more purposes than keeping malicious actors out of the code base. It also helps catch mistakes, security issues, or misunderstandings. It can also improve shared understanding of common goals and standards. Yes, this is also possible with other means, such as pair or ensemble programming, but from that, it doesn't follow that code reviews can't do that. They can. I've lived that dream. I think that the other counter-argument discussed in the article can't confute Farley because, contrary to the author's stated intentions, it is in fact a straw man. The author focuses on the benefits of "asynchronous" workflows over "synchronous" ones: i.e., in his words, reviewing pull requests is easier (for an introvert like himself, at least) than dealing with the social stress involved in pair/ensemble programming.

Lapsa 3y ago

someweirdperson 3y ago

Calling pull requests "pull request" when it is a job to create a change is what's wrong. The author isn't actually requesting anything. But what else could be a nice short term for "collection of changes with technically enforced reviews and tests"?

HPsquared 3y ago
Change proposal
alpaca128 3y ago
Review submission?
giantrobot 3y ago
It's a "pull request" because you're asking someone to pull a commit from your branch/repo into theirs. You're quite literally making a request. This actual change could be anything. You could just as well provide them as a patch they could apply by hand. The only formal aspects of the process, e.g. reviews and tests, are organizational. As an organization the acceptance of a pull request doesn't need a bunch of reviews and tests associated.
ilyt 3y ago
The problem is again, Github Originally, "pull request" was exactly what name implied, someone asked you to pull from their repository. The original Git collaboration model was that, you pulling changes from other repos and integrating them in your own, vs handing merge request. Github co-opted the term to mean exact opposite, accepting essentially push from different repo.
javier_e06 3y ago
I couldn't agree more. When the PR is approved then the button to complete the PR in Github is (drum-roll please) MERGE. And who is doing the merge? Me. Gitlab calls it merge request.

alex-moon 3y ago

"If you can't trust your team mates to make changes carefully..." Buddy I don't trust myself to make changes carefully The way I'd put this is: writing code and reviewing code are - not two different skillsets maybe, but - two different headspaces. Code review is a kind of "technical QA", and indeed I am pretty sure that you see the same basic practice in factory work (where we get the concept of "QA" from).

Cthulhu_ 3y ago

> If you can't trust your team mates to make changes carefully, then your version control system is not going to fix that for you. I don't trust myself either; trusting your own or others code is hubris. The two-person rule - or more - is applied EVERYWHERE.

zabil 3y ago

We use pull requests to spin out ephemeral environments and test the changes before merging to trunk. All merges to main is deployed across environments so pull requests help us get faster feedback if something fails. I think pull requests get a bad rep because it is thought as a review process. We see them as short lived branches. They actually improve code quality and unwanted commits even without manual review/approval.

kerkeslager 3y ago

> "Pull requests were invented to gatekeep access to open-source projects. In open source, it's very common that not everyone is given free access to changing the code, so contributors will issue a pull request so that a trusted person can then approve the change. > "I think this is really bad way to organise a development team. > "If you can't trust your team mates to make changes carefully, then your version control system is not going to fix that for you." Man, I don't trust myself to be perfect. I trust myself to be careful but no amount of care makes me infallible. I make mistakes. And if we can catch my mistakes early by getting a second set of eyes on them, they're less painful for everyone to fix. When someone spends their valuable time catching my mistakes, they're doing me a favor. Distrust might be part of the origin of pull requests, but that doesn't have to be the reason why your team uses pull requests. It can be about catching each others mistakes to learn, create a better product, and get better rewards. The tool is yours to use how you see fit--don't let ego prevent you from making it useful.

quadhome 3y ago

A pull request is when one maintainer asks another to merge branches. If you’re interpreting this through the lens of a single main/master/trunk and multiple branches, then like both Farley and the author, you’re trying to interpret a foreign culture (distributed version control) through a parochial lens (single trunk-multiple branch). The DVCS world is one of many sources of truth. For example, in the kernel community, Red Hat, Google, Debian, Android, etc. all have their own branches. Linus has a particularly popular branch. None of them are the main branch.

teekert 3y ago

I think we have GH set up so we can make direct pushes to even main, we just agreed to do PRs because frankly I don’t even trust myself to do everything right and optimal. PRs are nice for reviews, but also for testing and discussion, sometimes just to share something you are not sure if it is the best. It’s just a way to present changes to people just trust (or don’t) and to have a place for discussion. That’s how we treat PRs anyway. Occasionally I or a colleague indeed accepts their own PR (or use direct pushing) if it’s really minor (think a doc string) and need to get something out. So PRs are not just for envs without trust.

jabradoodle 3y ago

It frustrates me the one size fits all approach often taken to software development best practices. If no one is to review the code, then all code is either written by pairing/mobbing, or else it is the product of one persons thinking, blindspots, biases and all. It's great if you work in a team that pairs in everything, does trunk based development etc; that doesn't make it suitable for every team.

madsbuch 3y ago

It seems like we are reaching the same conclusions as Dave Farley on our development team. We have pivoted to just let everyone merge in their stuff and do "reviews" retroactively when we stumble upon code not up to the standard we set. The reasoning behind this is exactly that we trust individuals on the team to merge. and because reviewing in the traditional sense seems to be a formalisation of distrust: Something you do when you don't trust your peers to implement and put in production properly tested code.

PretzelPirate 3y ago
I would certainly fail my SOC2 audits if I told the auditor that anyone on the team can merge in any unreviewed change and that gets pushed to production, but not to worry be cause we'll catch any issues eventually. I trust my peers, but we all make mistakes and we do code reviews because we understand that working together can catch issues sooner. We aren't overly picky or mean during code reviews, but we make sure there aren't any glaring issues, and if there are, we talk about them and work together to fix them.
- madsbuch 3y ago
  Seems like you are at a place where you need to be SOC2 compliant. Good for you that you are able to satisfy those requirements.

pyrale 3y ago

With all the precautions the author takes, he ends up with a weird take: > If you take away the appeal to trust, though, there isn't much left of the argument. What remains is essentially: Pull requests were invented to solve a particular problem in open-source development. Internal software development is not open source. Pull requests are bad for internal software development. The specific point he tries to reword exists to explain that not everyone needs as much trust than open-source, and, therefore, that not having PR isn't a fatal flaw outside of open-source. The people who use it do so within a discourse along with other points explaining what they gain from forgoing PRs (usually, better continuous integration). I don't buy the whole discourse about not using PRs, but I'm also not sold on the idea of dismantling a coherent discourse into separate talking points and demanding that they should stand separately. I'm also not a fan of rewording someone else's point and slipping a big non-sequitur in that rewording.

onion2k 3y ago

I don't really have any opinion about PRs versus trunk-based development, but I do have a small insight that I think is useful when you're thinking about which one to choose. The main advantage of trunk-based dev is speed. You don't have to wait for a review of work. You're trusting the team to merge things that won't break stuff. That's awesome, but if that's the case then you should also be able to trust them to review PRs quickly enough that they don't block work. Equally the converse is true - if you can trust your team is motivated enough to review PRs quickly they you can probably trust them to do work together well enough not to really need PRs. If you can trust the team with one and not the other then something is wrong. There shouldn't be any "this way is better than this other way" argument. Your team should be capable of doing both. If it isn't then neither will work very well. You're going to have problems, conflicts, bugs, etc no matter what you do. Believing that the team should stop PRs because they're too slow, or they should stop trunk-based dev because pairing takes too much effort, are both red flags. They're both saying that the team has a problem to address.

fmajid 3y ago

It's fine to allow some senior engineers to merge a pull request without going through CI and/or code review, but the discipline the PR process introduces is highly desirable, whether working on open-source or not. It's not a question of trust, it's a question of avoiding unchallenged assumptions and blind spots.

usrusr 3y ago

Isn't there a bit of an elephant at the other end of the room? PR are not only about placing a bouncer at the club door to, figuratively speaking, check entering members for excessive drunkenness, they are about placing a bouncer at the door to give access to a wider group. This is quite obvious in open source, where the PR model has been a revolution in terms of lowering barriers of entry. You can join the party without asking for the keys. But similar barriers exist in companies. When they don't exits on the technical level (they usually do, sometimes to the point where collaborating teams wouldn't even know what version control the other side is using), they exist on the social level and a model like PR/MR can help nudge both towards the "internal open source" mindset.

papito 3y ago

"then your version control system is not going to fix that for you." Uh... Pull requests are not a version control workflow - it's a human workflow. It's also a way for the team to actually LOOK at the code that is going in and to be aware of what changes are incoming. I mean, that's just one point, there are so many ways in which this view is fatally flawed, I am not even going to... ugh. I know this is not trolling, but this is so scandalous, it comes across that way. This is beyond a "hot take".

dwheeler 3y ago

I disagree: it's about humility. We all make mistakes. Having a review process means we are humble enough to admit it.

Illniyar 3y ago

PRs might have been an open source innovation, but not "trusting" your developers has a very long history that predates OSS and PRs. The way it was done was to have QA check your code, usually as part of a release. I think PRs became more popular in non-OSS because of CI/CD and agile methodologies.

muskmusk 3y ago

Pr's are not bad. But using them to solve things that should be solved other things like catching bugs and enforcing style guides is bad. If doing a pr takes longer than 5 minutes in general then consider if you are relying too much on pr's.

manuelabeledo 3y ago

It seems that the quote comes from a big proponent of pair programming. Am I missing something, or is the irony lost on this one.

javier_e06 3y ago

Torvalds in one famous Google presentation he talked about the "circle of trust" in relation of how we all should handle sharing code as in: "Is my repo and I accept code from a selective group of trusted individuals" In the corporate world is more like Seinfield's Soup Nazi skit. We are all working towards AI accepting or rejecting code changes. Humans have egos and their idiosyncratic bias/baggage get in the way of adopting or rejecting code changes in an objective manner. Githook 50/72 rule anyone?

zajio1am 3y ago

The original purpose of PR was to accept patches from outsiders (people without write access to the project repository), as an alternative to sending patches to the project mailing list. In open-source development if there is a core team with write access to the project repository, i would be surprised if all patches from members of the core team were merged through PR process.

BerislavLopac 3y ago

This article recommends using merge requests (I'm not a big fan of the term pull request) only in specific cases: https://martinfowler.com/articles/ship-show-ask.html https://martinfowler.com/articles/ship-show-ask.html