12 ms·

Stacked Diffs versus Pull Requests

104 points by cocoflunchy 8y ago

mic47 8y ago
This is best workflow I've used so far (but usually I had separate branch for really unrelated changes).
jepler 8y ago
I think I agree with what the author is saying, by and large, but it doesn't matter: if it's a mental model that works for you, you can live in the "stacked diffs" world right up until you need to "curate" all your commits into something that can be considered for incorporation into the parent project. Then you still need to make a branch, put all the related commits into it in order, and adequately test that branch before sumbitting it. At that point, if changes are requested, you can make a choice about whether to work by rebasing that branch, or work on your "master" branch and then re-curate those commits. Now, working in the stacked diffs world seems like more work, but you do you. A place I disagree with the author is about rebasing frequently onto master. Assume you are working in a healthy project with frequent and largely bug-free releases, but occasional trouble during development cycles where tests end up red for awhile despite everyone's best efforts. In this situation, you are better off starting your work from a quality release tag with no red; occasionally you'll end up dealing with more merge conflicts when you're ready to merge the branch, but IMO not so much that you don't derive benefit from avoiding all those rebases onto a new upstream revision that just might be totally bonkers. If you do this, you should still validate that your branch, merged with or rebased onto master, doesn't make any new test failures or interact destructively with other work; deferring this to the end DOES have a cost, but in my experience it is outweighed by the benefits. (And if you're in one of those now-rare projects where it's the norm to be making point releases of one or more old releases, PLUS having new development on a master branch, basing your branch at the right place means you can merge it into the old releases with ease...)
- slobotron 8y ago
  Hm, I thought the idea is that the only way something will land in master is if it reviews OK and CI passes all the tests on top of the current master?
  - Joky 8y ago
    "on top of the current master" is limiting: how do you handle concurrency? I.e. two developers kicking tests on their PR on top of current master, everything green, but they merge and the result of their merge is not building (or passing tests) anymore.
    - zbentley 8y ago
      Most CI systems serialize builds on master, at least at the very end of the build pipeline. That can slow things down, but, if combined with a decently fast/thought-through build process with tears, I think the benefits of not risking untested states outweigh the costs.
      - zbentley 8y ago
        *tests, not tears, but I guess it works either way...
      - Joky 8y ago
        This is not about serializing builds on master, but builds on the PR before they get merged on master. If you have a few hours of test plans, you can't serialize every PR one after each other.
        zbentley 8y ago
        If your testing is time consuming to the point that you can only test on your PRs and never on what master would be after each one merges, then you have to choose between somehow forcing PR authors to serialize their merges somehow (external convention or tooling or something) or not testing the code that will actually be released. The latter isn’t always the wrong choice, though.
- deleted 8y ago
  [deleted]
- lima 8y ago
  The Phabricator project itself uses two branches - a fast-moving master and a stable branch. Changes are commited to master and then promoted to stable. Cherry-picking patches is a lot easier when the development team submits to a "one commit per idea" and "review single commits" workflow.
- krupan 8y ago
  "I think I agree with what the author is saying, by and large, but it doesn't matter: if it's a mental model that works for you, you can live in the "stacked diffs" world right up until you need to "curate" all your commits into something that can be considered for incorporation into the parent project" That's funny because he spent the whole blog post pointing out why it does matter. The tools we use and their defaults structure the way we work. Having lived in both worlds I can add my testament to that.
jchw 8y ago
This is a great model that scales very well to the largest projects and most complicated refactors. In fact, it forces you to think about your refactors differently, and that is actually great. You learn to boil the ocean incrementally more. We got the decentralized workflows alongside concepts like pull requests... but there's no real reason why you can't use your decentralized workflows with trunk based development. The secret, is combining the two: decentralized workflow for local development, with modern CI and code review, that all lead into the trunk. I've personally been veering toward this model for a while, and it's also somewhat similar to what Google is doing[1], and it works great across a lot of different circumstances. [1]: https://cacm.acm.org/magazines/2016/7/204032-why-google-stores-billions-of-lines-of-code-in-a-single-repository/ https://cacm.acm.org/magazines/2016/7/204032-why-google-stor...
petters 8y ago
> How do you git bisect a codebase where every 6th commit doesn’t build because it was jammed into the middle of a Pull Request? This I think is the main problem with pull requests. A better default would be squashing the commits into a single one after review.
- pbh101 8y ago
  This is also suboptimal. I’d rather each commit be an atomic unit: no larger and no smaller than it needs to be. Edit: in other words, concurring with TFA.
  - ghayes 8y ago
    Sure, but the best way to get there in my mind is `rebase -i`, which means you've made a few and revised a few commits along the way.
    - deleted 8y ago
      [deleted]
  - TomK32 8y ago
    yes, but often on a feature branch you have WIP, change your code a few commits later and so on. I use rebase -i a lot and it make better code.
    - Sean1708 8y ago
      `rebase -i` is fine (`commit --fixup` and `rebase --autosquash -i` is better), squashing all commits down into one is not (at least doing it all the time is not).
- falsedan 8y ago
  You can start bisecting and then skip every commit on a feature branch that was merged in. Painful to script and I wish bisect had a --first-parent flag to do it for me
- 8y ago
stevebmark 8y ago
This is very long and i'm tired, is there a more condensed resource?
- falsedan 8y ago
  I think the tl;dr is “stacked diffs require running different commands than git pull --rebase” The author complains about wasting time branching and merging, but other examples at the end still have the same number of operations and still hit the same merge conflicts. They make the point of how dependant work requires multiple branches but I feel it’s a workflow constraint, or a technical one. It can be easily avoided by creating branches as late as possible! Usually, when I’m working on a problem that requires multiple merges, I 1. Create a branch to fix the problem 2. Work on it 3. Rebase it so I can merge the first few commits 4. Push just those commits with a different branch name 5. Get it reviewed 6. a. Merge the reviewed branch into master and rebase the problem branch on top of master 6. b. Update the branch under review with feedback fixes, push it & update the review & rebase the problem branch onto the updated branch under review & go back to 5 7. Go back to 2 Just don’t do what I end up doing on personal projects and let 6 months go past between 2 and 3
  - rfoo 8y ago
    As the article pointed out, the problem here is you can't easily deal with multiple branches stacking on each other. You may argue that it is possible to put all these changes into one branch, but "asking 6 people to review a 500+ lines change, in which only ~100 lines is meaningful for each reviewer" is not a nice thing to do and it obviously delays the code review process.
    - falsedan 8y ago
      I feel like my workflow address that exactly: by making a separate branch per reviewable change from the base of the problem branch.
ajuc 8y ago
Isn't this basically how gerrit is supposed to be used? At least that's how we use it (but it's a very small team so anything would work I guess). You work on master, you rebase -i your commits to make them nice to review, you push them to gerrit, someone reviews them and pushes them to master or returns with stuff to fix. You usually have a few commits on top of remote master in your local master, and when any of them is pushed to remote master from gerrit you just git fetch && git rebase.
- deleted 8y ago
  [deleted]
- lima 8y ago
  Yes, the Phabricator and Gerrit workflows are very similar. It's basically the same workflow the Linux kernel uses, with Phabricator or Gerrit taking the place of git format-patch and git apply-patch.
jmiserez 8y ago
So basically what Gerrit offers as the "cherry-pick" submit type? You work on the master branch, then push commits as changesets to Gerrit. After review they are cherry-picked by the system onto the target (master) branch, in whatever order they happened to be reviewed. Or am I missing something?
fredmorcos 8y ago
Can someone explain why this is better than branch, commit and rebase from master if needed, then submit the pull request?
- mullsork 8y ago
  The case studies articulate the difference pretty well.
peterjmag 8y ago
I think GitHub's great, and I'm mostly okay with having to rely on it heavily at my day job, but I dislike how code reviews + PRs are necessarily tied to branches. What are the chances of GitHub introducing stacked diffs (or a similar abstraction) as an option? It'd be great to try out this workflow on a non-trivial project, but it seems like a hard sell organizationally when it involves migrating to and supporting a completely different tool like Phabricator or Gerrit.
- lima 8y ago
  You can do stacked diffs on GitHub by always using squash merges and rebasing on top of master, and it's a great way to get started. It's just a lot nicer with the extra tooling on top that Phabricator and Gerrit provide (dependencies, command line tools, and so on).
  - mullsork 8y ago
    It's true that you can do that indeed. Annoyingly, you have to manually rebase if you want to use the "merge & squash" feature. I don't understand why there's no "merge into one commit, rebase with master, merge onto master" feature. It's tedious to manually keep rebasing branches that were approved for review but have gone behind.
  - yxhuvud 8y ago
    If you squash merge you just lost all the small commits that do one thing well that you spent time creating, unless you review them one by one. Bleh.
    - UK-Al05 8y ago
      My pull request branches are of the size that it only makes sense together.
    - lima 8y ago
      That's the entire point of a stacked diff workflow - you would submit smaller PRs which are the equivalent of one commit.
saketmehta 8y ago
We recently shifted from Gerrit to Bitbucket. I don't miss Gerrit. Perhaps I am missing something.
IshKebab 8y ago
This is silly. I use Phabricator and I don't commit my changes directly to `master`. I always make a branch, exactly the same as you would for pull requests. And I have multiple commits on that branch. Why? Firstly it means I can still pull master and rebase my changes to update them fairly easily. Secondly it means I can work on more than one thing at a time (who only ever has one task in progress?), thirdly it means I can reorganise my commits easier (`git rebase -i master`), and finally having a change split into several commits can make code review easier. As far as I can tell the only significant difference between these two approaches is that with `arc` the history in the repository ends up nice and linear, without loads of branches.
- lima 8y ago
  I also use Phabricator like you do, but the author makes a compelling argument to use a stack of master commits rather than individual branches. > Firstly it means I can still pull master and rebase my changes to update them fairly easily. Like the author says, you can do this, but it's easier when your commits are stacked on master (you only do one rebase vs. rebasing each of your branches). > Secondly it means I can work on more than one thing at a time (who only ever has one task in progress?) Read the article, the author uses interactive rebases to edit commits. > finally having a change split into several commits can make code review easier How? The reviewer doesn't see those commits, unless you submit them as separate revisions, in which case they wouldn't be on the same branch. > As far as I can tell the only significant difference between these two approaches is that with `arc` the history in the repository ends up nice and linear, without loads of branches. Either approach would have the same result, it's all about how things are organized in your local repository. I'm not fully convinced (the tooling isn't exactly built for this workflow and you lose "arc feature"), but I like the idea.
Mikhail_Edoshin 8y ago
Is this similar to Mercurial Queues?
- krupan 8y ago
  Very much so. The difference is that it uses regular git commits, and you push and pop by doing interactive rebase. Sometimes I really miss mq
dooglius 8y ago
This seems like a good approach. However, it's unrealistic that my team is going to move off GitHub. Is there any way I can do something like this locally, but then have it fit into a GitHub PR flow?
jorams 8y ago
The idea of "stacked diffs" looks like a good one, but this article doesn't do a great job of presenting them. It's way too long for the actual claims it makes. It combines two things: Working on a single branch and reviewing commits instead of branches. Reviewing commits instead of branches makes a lot of sense, and would definitely result in a cleaner history than pull requests seem to do in practice. The article also makes the claim "They’ll get reviewed in some order and committed in some order". If the changes in the second commit depend on the first commit, that's just as false as with pull requests that depend on each other. The article names one concrete downside of the use of many branches: If one branch depends on the changes in another branch that's not in master yet, you end up with an annoying bunch of rebases or ugly merges. Committing everything to a single branch turns that into a single rebase. --- So that's the content. Then there are the case studies, which are odd. Charlie the free spirited hacker magically turns from someone who... > starts the day by branching master and spamming the branch with five commits that are only vaguely related. into someone who > blasts out five commits and five Diffs back to back. Each one addresses something specific. All thanks to the magic of Stacked Diffs.
- krupan 8y ago
  You are missing the subtle and hard to explain point that he tried to illustrate with the Tale of the Tree Icon. It's that our tools and their defaults shape the way we work. I watched a team switch from branches to stacked diffs (mercurial queues, way back in the day) and it was almost exactly how this article describes.
  - WorldMaker 8y ago
    It's the tools and their defaults that leads me to my biggest criticism with the article. On darcs something like "stacked diffs" makes a ton of sense because the UI was built for it from top down, and cherry-picking is _magic_ (when it works), including that it tracks your dependencies between commits/patches for you. In git, we have a dependency tracker in the DAG, but it requires the work of branching to make use of its power. In git, cherry-picking is a footgun with all sorts of potential problems. I've seen far too much work lost to bad rebases to trust junior developers with cherry-picking and rebase-heavy workflows in git. (Yes, I realize that a lot of people disagree and a lot of people use very heavy rebase workflows, this is just my opinion that in general rebase workflows are more danger than I want in my source control, as the resident source control guru on many of the teams I've worked on.) Pull Requests actually work really well with this dependency tracking. If I create a PR for the database changes, a PR for the REST API changes, and a PR for the UI work that depends on both of the previous two, and all of those PRs are well merged branches (no rebases) using the DAG to encode dependencies, my reviewers have the option to review those in any order. If they review the two smaller subset PRs first, as that content merges to master most good PR tools simplify the remaining UI PR to just the UI stuff not in master. If they review and merge everything all at once in the giant UI change PR, then most good PR tools will automatically mark the other two PRs as merged and complete, no work needed for individual review of the two subset PRs. The choice there is left to the reviewers, and the tools just do the right thing, whatever the preference the reviewers have.
saketmehta 8y ago
In the PR model, we can easily hook up a build system to automatically trigger builds on remote branches. How would that fit into this?
- woadwarrior01 8y ago
  And that can be (and is) done with the stacked diffs model as well. CI is orthogonal to whether you use the PR model of the stacked diffs model.
  - Ajedi32 8y ago
    But what commit do you run the CI against? If you run it against a commit that's stacked on top of 3-4 other unreviewed changes, then the CI will include errors from all those other commits, not just yours.
    - dom96 8y ago
      You don't stack against random commits. Only commits that have passed tests before and so are on master, or your own commits.
      - Ajedi32 8y ago
        That's not what the article says: > Yang sees that the Diff for the “fix” is out for review. Yang uses the Phabricator command line tool to patch that commit on top of master. This means that it’s not a branch. It’s just a throwaway local commit. Yang then starts working on the first change. Yang submits a Diff for review from the command line. Later, the “fix” has changed, so Yang drops the patch of the old version from the Git history and patches in the updated one via interactive rebase.
- Ajedi32 8y ago
  I guess the CI would have to cherry-pick the PR onto master before running the tests?
yumaikas 8y ago
How does this workflow work when there are 3 commits that need to be applied in order (A, B, and C), when C gets approved from a code-review standpoint before A and B?
- dom96 8y ago
  You are prevented from merging commits at the top of the stack until the commits below are accepted. The advantage is that each commit can be reviewed separately. You cannot achieve this with pulled requests as far as I know. I often see people creating a pull request and writing "this depends on PR #1233", the second PR is unreviewable because it contains changes from. The first PR. So the first PR has to merged first... It's a mess. But maybe I am just missing a big part of the PR workflow that solves this problem.
  - geofft 8y ago
    One way I've solved this is to open the second PR against the branch for the first PR, and explain what's going on in the PR message. Then, once the first PR is merged, update the second PR to be against the real target branch. This works better with a system that requires the PR owner to actually hit merge (so it's not accidentally merged into the first PR's branch) and where approvals stick across PR changes (which is undesirable in general, though).
  - WorldMaker 8y ago
    If the branches are properly merged so that the dependency is represented in the git DAG, most good PR tools just handle this correctly: if you merge the dependent PR first the dependee updates to show the commits as already merged and "simplify" their diff views appropriately; if you merge the dependee PR first, the dependent PR simply gets marked as completed/merged for you. I find it is still useful to include helpful hints like "this PR includes all of PR #1234, if you prefer to review a piece at a time", but that's for the reviewers' sake, not the tools'.
    - dom96 8y ago
      Yes, that's nice. But the problem is that these are still shown as separate PRs, GitHub for example doesn't tie these together unlike Phabricator.
Ajedi32 8y ago
So basically the author proposes replacing Pull Requests with "Cherry-Pick Requests", where each commit is a single reviewable unit (replacing separate branches with the PR model), the local master branch represents a list of unreviewed commits, and the remote master branch represents reviewed, "merged" commits. It's an interesting idea, but even after reading this article I'm still not convinced it offers any serious advantages over a more traditional workflow. It also has the downside of requiring extensive use of more advanced features like interactive rebasing. (Which I personally have no issue with, but seems like a potential nightmare for developers who aren't as experienced with git.)
- krupan 8y ago
  " It also has the downside of requiring extensive use of more advanced features like interactive rebasing." This is true. In the early days of mercurial (back before it was clear that git was winning the popularity contest) there was an extension called Mercurial Queues[1] that made it really easy to work this way. The downsides were: it didn't handle conflicts well, and it could totally destroy your work if you weren't careful. Since then someone has written the Evolve extension which is Mercurial Queues done right. That, sadly, came about after git had "won" the popularity contest. 1. Best explanation of Mercurial Queues: http://stevelosh.com/blog/2010/08/a-git-users-guide-to-mercurial-queues/ http://stevelosh.com/blog/2010/08/a-git-users-guide-to-mercu...
agapon 8y ago
It seems that topgit can be used to achieve this workflow. The beauty of it is that it is built on top of git, so it has the familiar mechanics internally while providing a new way of doing patch series / stacked diffs.
pimlottc 8y ago
Interesting, does anyone know of any scripts to allow you to use stacked diffs locally when your team is using tools based around pull requests (i.e. GitHub)?
krupan 8y ago
Another code review tool that lets you work this way is Review Board: https://www.reviewboard.org/ https://www.reviewboard.org/

104 points by cocoflunchy 8y ago

This is best workflow I've used so far (but usually I had separate branch for really unrelated changes).

I think I agree with what the author is saying, by and large, but it doesn't matter: if it's a mental model that works for you, you can live in the "stacked diffs" world right up until you need to "curate" all your commits into something that can be considered for incorporation into the parent project. Then you still need to make a branch, put all the related commits into it in order, and adequately test that branch before sumbitting it. At that point, if changes are requested, you can make a choice about whether to work by rebasing that branch, or work on your "master" branch and then re-curate those commits. Now, working in the stacked diffs world seems like more work, but you do you. A place I disagree with the author is about rebasing frequently onto master. Assume you are working in a healthy project with frequent and largely bug-free releases, but occasional trouble during development cycles where tests end up red for awhile despite everyone's best efforts. In this situation, you are better off starting your work from a quality release tag with no red; occasionally you'll end up dealing with more merge conflicts when you're ready to merge the branch, but IMO not so much that you don't derive benefit from avoiding all those rebases onto a new upstream revision that just might be totally bonkers. If you do this, you should still validate that your branch, merged with or rebased onto master, doesn't make any new test failures or interact destructively with other work; deferring this to the end DOES have a cost, but in my experience it is outweighed by the benefits. (And if you're in one of those now-rare projects where it's the norm to be making point releases of one or more old releases, PLUS having new development on a master branch, basing your branch at the right place means you can merge it into the old releases with ease...)

slobotron 8y ago
Hm, I thought the idea is that the only way something will land in master is if it reviews OK and CI passes all the tests on top of the current master?
- Joky 8y ago
  "on top of the current master" is limiting: how do you handle concurrency? I.e. two developers kicking tests on their PR on top of current master, everything green, but they merge and the result of their merge is not building (or passing tests) anymore.
  - zbentley 8y ago
    Most CI systems serialize builds on master, at least at the very end of the build pipeline. That can slow things down, but, if combined with a decently fast/thought-through build process with tears, I think the benefits of not risking untested states outweigh the costs.
    - zbentley 8y ago
      *tests, not tears, but I guess it works either way...
    - Joky 8y ago
      This is not about serializing builds on master, but builds on the PR before they get merged on master. If you have a few hours of test plans, you can't serialize every PR one after each other.
      - zbentley 8y ago
        If your testing is time consuming to the point that you can only test on your PRs and never on what master would be after each one merges, then you have to choose between somehow forcing PR authors to serialize their merges somehow (external convention or tooling or something) or not testing the code that will actually be released. The latter isn’t always the wrong choice, though.
deleted 8y ago
[deleted]
lima 8y ago
The Phabricator project itself uses two branches - a fast-moving master and a stable branch. Changes are commited to master and then promoted to stable. Cherry-picking patches is a lot easier when the development team submits to a "one commit per idea" and "review single commits" workflow.
krupan 8y ago
"I think I agree with what the author is saying, by and large, but it doesn't matter: if it's a mental model that works for you, you can live in the "stacked diffs" world right up until you need to "curate" all your commits into something that can be considered for incorporation into the parent project" That's funny because he spent the whole blog post pointing out why it does matter. The tools we use and their defaults structure the way we work. Having lived in both worlds I can add my testament to that.

jchw 8y ago

This is a great model that scales very well to the largest projects and most complicated refactors. In fact, it forces you to think about your refactors differently, and that is actually great. You learn to boil the ocean incrementally more. We got the decentralized workflows alongside concepts like pull requests... but there's no real reason why you can't use your decentralized workflows with trunk based development. The secret, is combining the two: decentralized workflow for local development, with modern CI and code review, that all lead into the trunk. I've personally been veering toward this model for a while, and it's also somewhat similar to what Google is doing[1], and it works great across a lot of different circumstances. [1]: https://cacm.acm.org/magazines/2016/7/204032-why-google-stores-billions-of-lines-of-code-in-a-single-repository/ https://cacm.acm.org/magazines/2016/7/204032-why-google-stor...

petters 8y ago

> How do you git bisect a codebase where every 6th commit doesn’t build because it was jammed into the middle of a Pull Request? This I think is the main problem with pull requests. A better default would be squashing the commits into a single one after review.

pbh101 8y ago
This is also suboptimal. I’d rather each commit be an atomic unit: no larger and no smaller than it needs to be. Edit: in other words, concurring with TFA.
- ghayes 8y ago
  Sure, but the best way to get there in my mind is `rebase -i`, which means you've made a few and revised a few commits along the way.
  - deleted 8y ago
    [deleted]
- TomK32 8y ago
  yes, but often on a feature branch you have WIP, change your code a few commits later and so on. I use rebase -i a lot and it make better code.
  - Sean1708 8y ago
    `rebase -i` is fine (`commit --fixup` and `rebase --autosquash -i` is better), squashing all commits down into one is not (at least doing it all the time is not).
falsedan 8y ago
You can start bisecting and then skip every commit on a feature branch that was merged in. Painful to script and I wish bisect had a --first-parent flag to do it for me
8y ago

stevebmark 8y ago

This is very long and i'm tired, is there a more condensed resource?

falsedan 8y ago
I think the tl;dr is “stacked diffs require running different commands than git pull --rebase” The author complains about wasting time branching and merging, but other examples at the end still have the same number of operations and still hit the same merge conflicts. They make the point of how dependant work requires multiple branches but I feel it’s a workflow constraint, or a technical one. It can be easily avoided by creating branches as late as possible! Usually, when I’m working on a problem that requires multiple merges, I 1. Create a branch to fix the problem 2. Work on it 3. Rebase it so I can merge the first few commits 4. Push just those commits with a different branch name 5. Get it reviewed 6. a. Merge the reviewed branch into master and rebase the problem branch on top of master 6. b. Update the branch under review with feedback fixes, push it & update the review & rebase the problem branch onto the updated branch under review & go back to 5 7. Go back to 2 Just don’t do what I end up doing on personal projects and let 6 months go past between 2 and 3
- rfoo 8y ago
  As the article pointed out, the problem here is you can't easily deal with multiple branches stacking on each other. You may argue that it is possible to put all these changes into one branch, but "asking 6 people to review a 500+ lines change, in which only ~100 lines is meaningful for each reviewer" is not a nice thing to do and it obviously delays the code review process.
  - falsedan 8y ago
    I feel like my workflow address that exactly: by making a separate branch per reviewable change from the base of the problem branch.

ajuc 8y ago

Isn't this basically how gerrit is supposed to be used? At least that's how we use it (but it's a very small team so anything would work I guess). You work on master, you rebase -i your commits to make them nice to review, you push them to gerrit, someone reviews them and pushes them to master or returns with stuff to fix. You usually have a few commits on top of remote master in your local master, and when any of them is pushed to remote master from gerrit you just git fetch && git rebase.

deleted 8y ago
[deleted]
lima 8y ago
Yes, the Phabricator and Gerrit workflows are very similar. It's basically the same workflow the Linux kernel uses, with Phabricator or Gerrit taking the place of git format-patch and git apply-patch.

jmiserez 8y ago

So basically what Gerrit offers as the "cherry-pick" submit type? You work on the master branch, then push commits as changesets to Gerrit. After review they are cherry-picked by the system onto the target (master) branch, in whatever order they happened to be reviewed. Or am I missing something?

fredmorcos 8y ago

Can someone explain why this is better than branch, commit and rebase from master if needed, then submit the pull request?

mullsork 8y ago
The case studies articulate the difference pretty well.

peterjmag 8y ago

I think GitHub's great, and I'm mostly okay with having to rely on it heavily at my day job, but I dislike how code reviews + PRs are necessarily tied to branches. What are the chances of GitHub introducing stacked diffs (or a similar abstraction) as an option? It'd be great to try out this workflow on a non-trivial project, but it seems like a hard sell organizationally when it involves migrating to and supporting a completely different tool like Phabricator or Gerrit.

lima 8y ago
You can do stacked diffs on GitHub by always using squash merges and rebasing on top of master, and it's a great way to get started. It's just a lot nicer with the extra tooling on top that Phabricator and Gerrit provide (dependencies, command line tools, and so on).
- mullsork 8y ago
  It's true that you can do that indeed. Annoyingly, you have to manually rebase if you want to use the "merge & squash" feature. I don't understand why there's no "merge into one commit, rebase with master, merge onto master" feature. It's tedious to manually keep rebasing branches that were approved for review but have gone behind.
- yxhuvud 8y ago
  If you squash merge you just lost all the small commits that do one thing well that you spent time creating, unless you review them one by one. Bleh.
  - UK-Al05 8y ago
    My pull request branches are of the size that it only makes sense together.
  - lima 8y ago
    That's the entire point of a stacked diff workflow - you would submit smaller PRs which are the equivalent of one commit.

saketmehta 8y ago

We recently shifted from Gerrit to Bitbucket. I don't miss Gerrit. Perhaps I am missing something.

IshKebab 8y ago

This is silly. I use Phabricator and I don't commit my changes directly to `master`. I always make a branch, exactly the same as you would for pull requests. And I have multiple commits on that branch. Why? Firstly it means I can still pull master and rebase my changes to update them fairly easily. Secondly it means I can work on more than one thing at a time (who only ever has one task in progress?), thirdly it means I can reorganise my commits easier (`git rebase -i master`), and finally having a change split into several commits can make code review easier. As far as I can tell the only significant difference between these two approaches is that with `arc` the history in the repository ends up nice and linear, without loads of branches.

lima 8y ago
I also use Phabricator like you do, but the author makes a compelling argument to use a stack of master commits rather than individual branches. > Firstly it means I can still pull master and rebase my changes to update them fairly easily. Like the author says, you can do this, but it's easier when your commits are stacked on master (you only do one rebase vs. rebasing each of your branches). > Secondly it means I can work on more than one thing at a time (who only ever has one task in progress?) Read the article, the author uses interactive rebases to edit commits. > finally having a change split into several commits can make code review easier How? The reviewer doesn't see those commits, unless you submit them as separate revisions, in which case they wouldn't be on the same branch. > As far as I can tell the only significant difference between these two approaches is that with `arc` the history in the repository ends up nice and linear, without loads of branches. Either approach would have the same result, it's all about how things are organized in your local repository. I'm not fully convinced (the tooling isn't exactly built for this workflow and you lose "arc feature"), but I like the idea.

Mikhail_Edoshin 8y ago

Is this similar to Mercurial Queues?

krupan 8y ago
Very much so. The difference is that it uses regular git commits, and you push and pop by doing interactive rebase. Sometimes I really miss mq

dooglius 8y ago

This seems like a good approach. However, it's unrealistic that my team is going to move off GitHub. Is there any way I can do something like this locally, but then have it fit into a GitHub PR flow?

jorams 8y ago

The idea of "stacked diffs" looks like a good one, but this article doesn't do a great job of presenting them. It's way too long for the actual claims it makes. It combines two things: Working on a single branch and reviewing commits instead of branches. Reviewing commits instead of branches makes a lot of sense, and would definitely result in a cleaner history than pull requests seem to do in practice. The article also makes the claim "They’ll get reviewed in some order and committed in some order". If the changes in the second commit depend on the first commit, that's just as false as with pull requests that depend on each other. The article names one concrete downside of the use of many branches: If one branch depends on the changes in another branch that's not in master yet, you end up with an annoying bunch of rebases or ugly merges. Committing everything to a single branch turns that into a single rebase. --- So that's the content. Then there are the case studies, which are odd. Charlie the free spirited hacker magically turns from someone who... > starts the day by branching master and spamming the branch with five commits that are only vaguely related. into someone who > blasts out five commits and five Diffs back to back. Each one addresses something specific. All thanks to the magic of Stacked Diffs.

krupan 8y ago
You are missing the subtle and hard to explain point that he tried to illustrate with the Tale of the Tree Icon. It's that our tools and their defaults shape the way we work. I watched a team switch from branches to stacked diffs (mercurial queues, way back in the day) and it was almost exactly how this article describes.
- WorldMaker 8y ago
  It's the tools and their defaults that leads me to my biggest criticism with the article. On darcs something like "stacked diffs" makes a ton of sense because the UI was built for it from top down, and cherry-picking is _magic_ (when it works), including that it tracks your dependencies between commits/patches for you. In git, we have a dependency tracker in the DAG, but it requires the work of branching to make use of its power. In git, cherry-picking is a footgun with all sorts of potential problems. I've seen far too much work lost to bad rebases to trust junior developers with cherry-picking and rebase-heavy workflows in git. (Yes, I realize that a lot of people disagree and a lot of people use very heavy rebase workflows, this is just my opinion that in general rebase workflows are more danger than I want in my source control, as the resident source control guru on many of the teams I've worked on.) Pull Requests actually work really well with this dependency tracking. If I create a PR for the database changes, a PR for the REST API changes, and a PR for the UI work that depends on both of the previous two, and all of those PRs are well merged branches (no rebases) using the DAG to encode dependencies, my reviewers have the option to review those in any order. If they review the two smaller subset PRs first, as that content merges to master most good PR tools simplify the remaining UI PR to just the UI stuff not in master. If they review and merge everything all at once in the giant UI change PR, then most good PR tools will automatically mark the other two PRs as merged and complete, no work needed for individual review of the two subset PRs. The choice there is left to the reviewers, and the tools just do the right thing, whatever the preference the reviewers have.

saketmehta 8y ago

In the PR model, we can easily hook up a build system to automatically trigger builds on remote branches. How would that fit into this?

woadwarrior01 8y ago
And that can be (and is) done with the stacked diffs model as well. CI is orthogonal to whether you use the PR model of the stacked diffs model.
- Ajedi32 8y ago
  But what commit do you run the CI against? If you run it against a commit that's stacked on top of 3-4 other unreviewed changes, then the CI will include errors from all those other commits, not just yours.
  - dom96 8y ago
    You don't stack against random commits. Only commits that have passed tests before and so are on master, or your own commits.
    - Ajedi32 8y ago
      That's not what the article says: > Yang sees that the Diff for the “fix” is out for review. Yang uses the Phabricator command line tool to patch that commit on top of master. This means that it’s not a branch. It’s just a throwaway local commit. Yang then starts working on the first change. Yang submits a Diff for review from the command line. Later, the “fix” has changed, so Yang drops the patch of the old version from the Git history and patches in the updated one via interactive rebase.
Ajedi32 8y ago
I guess the CI would have to cherry-pick the PR onto master before running the tests?

yumaikas 8y ago

How does this workflow work when there are 3 commits that need to be applied in order (A, B, and C), when C gets approved from a code-review standpoint before A and B?

dom96 8y ago
You are prevented from merging commits at the top of the stack until the commits below are accepted. The advantage is that each commit can be reviewed separately. You cannot achieve this with pulled requests as far as I know. I often see people creating a pull request and writing "this depends on PR #1233", the second PR is unreviewable because it contains changes from. The first PR. So the first PR has to merged first... It's a mess. But maybe I am just missing a big part of the PR workflow that solves this problem.
- geofft 8y ago
  One way I've solved this is to open the second PR against the branch for the first PR, and explain what's going on in the PR message. Then, once the first PR is merged, update the second PR to be against the real target branch. This works better with a system that requires the PR owner to actually hit merge (so it's not accidentally merged into the first PR's branch) and where approvals stick across PR changes (which is undesirable in general, though).
- WorldMaker 8y ago
  If the branches are properly merged so that the dependency is represented in the git DAG, most good PR tools just handle this correctly: if you merge the dependent PR first the dependee updates to show the commits as already merged and "simplify" their diff views appropriately; if you merge the dependee PR first, the dependent PR simply gets marked as completed/merged for you. I find it is still useful to include helpful hints like "this PR includes all of PR #1234, if you prefer to review a piece at a time", but that's for the reviewers' sake, not the tools'.
  - dom96 8y ago
    Yes, that's nice. But the problem is that these are still shown as separate PRs, GitHub for example doesn't tie these together unlike Phabricator.

Ajedi32 8y ago

So basically the author proposes replacing Pull Requests with "Cherry-Pick Requests", where each commit is a single reviewable unit (replacing separate branches with the PR model), the local master branch represents a list of unreviewed commits, and the remote master branch represents reviewed, "merged" commits. It's an interesting idea, but even after reading this article I'm still not convinced it offers any serious advantages over a more traditional workflow. It also has the downside of requiring extensive use of more advanced features like interactive rebasing. (Which I personally have no issue with, but seems like a potential nightmare for developers who aren't as experienced with git.)

krupan 8y ago
" It also has the downside of requiring extensive use of more advanced features like interactive rebasing." This is true. In the early days of mercurial (back before it was clear that git was winning the popularity contest) there was an extension called Mercurial Queues[1] that made it really easy to work this way. The downsides were: it didn't handle conflicts well, and it could totally destroy your work if you weren't careful. Since then someone has written the Evolve extension which is Mercurial Queues done right. That, sadly, came about after git had "won" the popularity contest. 1. Best explanation of Mercurial Queues: http://stevelosh.com/blog/2010/08/a-git-users-guide-to-mercurial-queues/ http://stevelosh.com/blog/2010/08/a-git-users-guide-to-mercu...

agapon 8y ago

It seems that topgit can be used to achieve this workflow. The beauty of it is that it is built on top of git, so it has the familiar mechanics internally while providing a new way of doing patch series / stacked diffs.

pimlottc 8y ago

Interesting, does anyone know of any scripts to allow you to use stacked diffs locally when your team is using tools based around pull requests (i.e. GitHub)?

krupan 8y ago

Another code review tool that lets you work this way is Review Board: https://www.reviewboard.org/ https://www.reviewboard.org/