51 ms·

Stacked changes: how FB and Google engineers stay unblocked and ship faster

467 points by tomasreimers 5y ago

londons_explore 5y ago
Not quite sure how a blog post about a new workflow for code review failed to have any screenshots or infographics showing said workflow...
sond813 5y ago
Looks like a great improvement to my workflow, does it require write access to GitHub or can it be read only? Any plans to support GitLab?
- tomasreimers 5y ago
  The CLI does not require any access to Github, and if you want to start doing code review on the platform, we ask for write access just so we can post comments and reviews :) Gitlab is actively on our roadmap; we currently have a few CLI users that have made it work, but feel free to reach out if it's blocking you.
darepublic 5y ago
Not a fan of phabricator. To me the whole stacked changes thing is at best a necessary evil but not something I would recommend for normal size codebase
stephenturban17 5y ago
I think this is a notable try at a strangely underserved space. Really interesting to see how code review is done in-house at FAANG+ and yet GitHub doesn’t quite get at the level of the tools of these giants.
deleted 5y ago
[deleted]
grozensmith 5y ago
I haven't been on the eng side for a while (PMs just get to pretend like we are technical) but when I was an eng, I basically did stacked changes but with infinite amounts of unnecessary complications. This tool looks fuego. Maybe I'll start coding again.... Kids these days are getting luxuries we never knew back in the day (in 2015 lol).
ayushsood 5y ago
this is the thing I've missed the most about development at Facebook. It seems so small, but is a huge productivity boost. Especially if applied to an entire engineering team
epage 5y ago
For anyone interested, I've been collecting notes on various tools in this space: https://github.com/epage/git-stack/blob/main/docs/comparison.md https://github.com/epage/git-stack/blob/main/docs/comparison... (granted the page doesn't mention git-stack since that is assumed)
- shepherdjerred 5y ago
  Do you have a favorite? I've struggled with this problem for a long time, but never knew that there were tools to solve it.
  - epage 5y ago
    I'm biased as the author of git-stack but struggling with this problem is why I wrote it. When I started it, I didn't know there were so many tools in this space. However, if I knew then what I knew now, I would have still written it. Too many tools require adopting a specialized workflow which doesn't play well with everyone's situation or other git tools. Some do but they require manually defining each change which is too much boilerplate for me. My goal with git-stack has been to automate my PR workflow and sit alongside other git tools. I've even tried to keep it open so you can use parts of it with Phab or other tools. You should be able to start using it out of the box with a standard PR model. It'll have better performance and behavior if you configure which branches are shared branches and mark them as "protected" in git-stack. If you don't use a PR model, then that might just don't run `git stack --push` but the rest should still do a reasonable job. If I were to pick a tool besides git-stack, it would either be git-branchless or Graphite.
RHSeeger 5y ago
> The blocker here isn’t Git. In fact, the reason you can’t go with option #4 is that your code review tools probably don’t support it I do stacked commits (branches off branches) all the time. Github certainly supports PRs based on such.
- jas14 5y ago
  I still haven't found a reliable way to prevent merging branches that aren't the first in the stack :/
  - fosterfriends 5y ago
    (Graphite engineer) We had the same problem. Coded in a feature that prevents you from casually merging branches that are not bottom of the stack :)
  - _freds 5y ago
    What I usually do is if I need to stack more PRs I open them as a draft to avoid accidental merges. I then convert each PR to a non-draft as I “pop” the stack.
- tomasreimers 5y ago
  How do you deal with rebases when you have to?
  - sigil 5y ago
    Like so: git switch offshoot git rebase --onto feature branchpoint^ Will replay your offshoot branch onto its underlying feature branch after the feature branch has been rebased. The branchpoint is the first commit that diverges from the feature branch. Finding the branchpoint commit is the only real annoyance here -- if there's a ref or a better trick for finding it, I'm all ears.
    - connorbrinton 5y ago
      My usual workflow to rebase a stacked set of feature branches off of main is: git checkout main git pull git checkout feature-branch-1 git rebase -i - git checkout feature-branch-2 git rebase -i - ... There's most likely a more efficient way to do this, but I find that referring to the most recently checked-out branch with a single hyphen is a lesser-known feature, so perhaps it will be helpful :)
secondcoming 5y ago
> You're under a deadline and your next highest priority task is to add reactions to those comments. What's a 'reaction'? > Why can’t you write the reactions branch on top of the comments branch What? Why would I have a 'comments branch'? Having never worked at FAAANG, I've no idea what this marketing material is about, or why I'd need it.
- joshuamorton 5y ago
  Feature branching is a fairly common practice. "Reactions" are emoji that you can use to respond to a post or comment, like Facebook's like/love/laugh/cry/angry, or slack/discord/messenger/chat/whatever's various ways to emoji-reply to messages. You would have a comments branch if you were adding a feature called "comments", I guess.
  - secondcoming 5y ago
    I understand what a feature branch is. I don't understand why I'd need a branch to add an emoji. I assumed that a 'reactions' branch might be a branch that addresses issues in the original PR, but even if it was, why would I need a new branch for that?
    - tomasreimers 5y ago
      Probably because you're trying to break up your changes. Two good blog posts that talk about this workflow are: https://jg.gg/2018/09/29/stacked-diffs-versus-pull-requests/ https://jg.gg/2018/09/29/stacked-diffs-versus-pull-requests/ and https://kurtisnusbaum.medium.com/stacked-diffs-keeping-phabricator-diffs-small-d9964f4dcfa6 https://kurtisnusbaum.medium.com/stacked-diffs-keeping-phabr...
      - secondcoming 5y ago
        Those were useful, thanks!
    - joshuamorton 5y ago
davidw 5y ago
After seeing so much noise and struggle at the past couple places I've worked regarding people having to beg and make noise about getting reviews, I half jokingly think someone should make a "begging for PR reviews as a service" startup.
- Nextgrid 5y ago
  The problem is that in most teams code review is considered an auxiliary task that's off the books (from a ticket tracker perspective) where as in reality it should be a ticket in its own right that someone can assign themselves to and review the associated PR.
  - ericbarrett 5y ago
    This is a team culture thing. Prompt and professional reviews have to be valued. If you're a senior engineer (or at least senior on a code base), you can add at least as much value to the development process by giving others timely and fair feedback as you can writing your own code—it's a force multiplier. (Edit) For big changes I've seen people schedule 30-minute review meetings. We all hate meetings but I don't think it's a bad idea.
    - davidw 5y ago
      Everyone says they value good reviews, and I think that's actually true to some extent, but 'prompt' comes at a real cost as you have to stop what you're doing to take a look.
      - ericbarrett 5y ago
        The strategy I used for smaller changes was twice-a-day queue review; noonish and end of day. "Prompt" doesn't have to mean instant, it just means not sitting on a change for days.
      - toast0 5y ago
        Not prompt reviewing has a real cost too. If it takes a long time to get a review, the submitter loses context and has to regain it when the review comes in. The longer the cycle, the more contexts to be juggled and the more effort to juggle them.
jrochkind1 5y ago
I have definitely had this problem before and wanted this solution. Not sure if I want it enough to introduce a third-party tool to my relatively small team(s). I wish Github just had it built in.
- fragmede 5y ago
  it is, it's just not exposed well. in git create two branches stacked on top of each other, and then in the PR creation window, the first PR goes from master to branch-1 as normal. Then create a second PR for branch-1 to branch-2. it's a bit of futzing around with git and GitHub but it's there/works without another tool. Or you could just use the tool, up to you.
  - jrochkind1 5y ago
    I have had trouble with GH displaying the correct diff. I guess in most cases I don't really want to merge branch-2 into branch-1 either, I want to merge them both into master -- but first branch-1, and only then branch-2. I don't know if that's the use-case the third party tool is focused on? When I try that with github alone, which I have... it just gets really messy. Sometimes I need to switch the PR to a different random base branch and then back to the actually desired one to get github to update the diff when the base branch has changed since PR opened.
zurawiki 5y ago
> This idea isn't new, and if you've worked at a company like Facebook, Google, Uber, Dropbox, or Lyft (among others), you've either had first-class support for stacked changes built into your code review platform (i.e. Phabricator at Facebook... Phabricator got a lot of stuff about code review right (I may be biased from working at FAANG). I'm happy to see someone trying to improve the code review situation on GitHub. When I did use GitHub at the startups I worked at, we were never fully satisfied with the code review experience. I hope they surface "Phabricator versions" as first-class objects. I never liked it how GitHub just clobbers the history left behind from force-pushes from to branch.
- gibolt 5y ago
  The ability to quickly manage branches and reorder commits within them in Mercurial is super powerful, something that Git and `rebase -i` could only dream of. Doing the same in Phabricator is awesome. One blocking commit PR on the bottom of a stack can easily be moved to the top or a new stack, unblocking the rest of your changes. Re-writing and entire stack due to one small change is mostly eliminated. (Good) Tooling is a major gain in efficiency.
  - tadfisher 5y ago
    > The ability to quickly manage branches and reorder commits within them in Mercurial is super powerful, something that Git and 'rebase could only dream of. This is a git CLI wart more than anything. Magit (git interface in Emacs) makes juggling branches/worktrees and rewriting history intuitive to humans. I feel super-powerful in Magit and absolutely feeble using the CLI.
  - surajrmal 5y ago
    I completely agree. When Google decided to invest in a mercurial front end to their version control system a few years ago, I was confused, but after using it for a while, I can honestly say it has much better ergonomics for common workflows. Creating a tree of dependent commits is simple and trivial. I sometimes wish I could create a dag instead, but that's probably more trouble than it's worth.
- 5y ago
fosterfriends 5y ago
The part of Graphite that helps manage stacked changes locally is open source - you can see how it handles the recursive rebases here: https://github.com/screenplaydev/graphite-cli/blob/main/src/actions/fix.ts#L179-L229 https://github.com/screenplaydev/graphite-cli/blob/main/src/...
barbazoo 5y ago
Usually when feature-branch-1 is still under review and feature 2 relies on feature 1, I just open a PR from feature-branch-2 onto feature-branch-1 to get the review process started, knowing that feature-branch-1 might still change which I can deal with by rebasing.
- milkytron 5y ago
  Yeah this shouldn't be an issue when following standard git branching practices. It's kinda the point
- fosterfriends 5y ago
  I came from Airbnb (vanilla github) and some friends converted me to a stacked workflow. I think the challenge with vanilla git is that rebasing and re-pushing becomes a brutal process if you stack 3+ changes. (At facebook people regularly stack over 10). Beyond recursive rebases, Github offers no support for navigating your stack, merging in a stack of approved PRs together, adjusting merge bases, etc. Most of these practices are just inspired by what Phabricator and a few of the big companies are already doing well :)
  - treis 5y ago
    >(At facebook people regularly stack over 10). Meaning branch 1 depends on branch 2 which depends branch 3... all the way to 10? That just seems crazy to me. What happens if branch 4 changes their implementation and borks everything depending on it?
    - shepherdjerred 5y ago
      You have to fix a lot of merge conflicts. I used this workflow at AWS -- code reviews can take some time, you're writing a lot of code, and you want your CRs to be small. If someone leaves a comment on the CR for the first branch you fix the issue and rebase. It's not fun, but this is the only effective way to do it (that I know of).
      - 5y ago
jpochtar 5y ago
I've been waiting for this forever. Stacked diffs is how I've always worked even before working at FANG, and github just doesn't support them right out of the box. Kudos for getting this right!
- unemphysbro 5y ago
  how do you typically stack diffs?
gmaster1440 5y ago
Worth noting that this is coming to GitHub: https://github.blog/changelog/2021-10-27-pull-request-merge-queue-limited-beta/ https://github.blog/changelog/2021-10-27-pull-request-merge-...
- tomasreimers 5y ago
  So merge queue, while cool - is slightly different than Stacked changes. Stacked changes are a better client-side git workflow for dealing with branches on top of branches.
jeffbee 5y ago
Having worked with several large commercial monorepos including Google's I can recall using stacked changes a few times but I don't remember them being the secret sauce by any means. They were more of an edge case.
- fosterfriends 5y ago
  Even if you're not a fan of stacking your changes, I think the web dashboard has a lot of fan favorites from the now deprecated Phabricator, such as seeing the PR comment timeline side-by-side the code, macros, showing lines that were simply copied, etc. We've also tried to make it as snappy and modern as possible, resulting in something that feels a lot faster than github for reviewing PRs :) Would love to get your feedback if you try it out!
didibus 5y ago
Even when the tool support it, I'm not sure it's a good practice. Small code reviews often miss the forest for the tree. When you make the extent of the feature changes even harder to mentally trace back and put together by having a diff over another set of code that is itself an unapproved diff, the code review becomes pretty low quality. I've also done this where my change that depend on other changes get approved, but then the other set of code gets critiqued so I have to change it and that breaks the code that depends on it so I have to change that as well, and now I still need to get it all reviewed again, so really I just wasted someone's time the first round. What I do now is that I make commits over it in my branch, so I keep working while they review my first commit, and when that's approved I send the next commit to be reviewed, and so on. If there are comments I do an interactive rebase to fix them.
- polishdude20 5y ago
  That's what we do as well but we've got merge block labels. If a pr depends on another PR, we put a merge blocked label on it and reference the PR in the comments. We've got a GitHub action which doesn't let a merge happen unless zero labels are present.
- fosterfriends 5y ago
  Google had a good paper on the benefits of smaller changes: https://sback.it/publications/icse2018seip.pdf https://sback.it/publications/icse2018seip.pdf > A correlation between change size and review quality is acknowledged by Google and developers are strongly encouraged to make small, incremental changes Heard though about wanting to maintain context within a stack. I think the best balance is tooling that both lets you create a stack of small changes, while also seeing each change in the broader context (forest). The benefit of breaking out PRs instead of commits, is that each small change gets to pass review and CI atomically. I like your strategy of gating commit pushes until the first is reviewed, but I think the dream is to decouple those processes :)
  - jacobr1 5y ago
    Having worked in startups for most my career, and now recently joining a BigTechCo, I think some of the cross-talk comes from the commingling of concerns within a PR. PRs are used to review architecture, enforce security/code-conventions/style, check for logic bugs, catch potential conflicts with other efforts, and validate the logic of a feature. In BigTechCo you have more coordination problems, so you want to merge code to trunk as soon as possible to prevent integration risks. But that means you drive discussion of the architecture and features to other venues than than the code review. Code review becomes more of a "will this break things" check, and enabling a feature-flag is more the review stage for the feature itself.
KptMarchewa 5y ago
> How do you tell reviewers that your reactions PR depends on the comments PR? It's at the top of pr. "someone wants to merge 1 commit into random-branch from another-random-branch" > How will reviewers see the relevant diff for the reactions PR (excluding the comments PR changes)? It will show it by default? If they want to see comments PR changes, they will need to go to comments PR. > How can you propagate changes to the reactions PR if you ever need to update the comments PR before you land it (i.e. to address review comments)? Use git rebase --onto. It would be nice if the marketing page told prospective users what exact difference their tool provides - since I'm already using that workflow and never seen one of those internal tools.
- tomasreimers 5y ago
  Hi, sadly no marketing people at this company. Just engineers and product people. We were trying to get this to work on Github for almost a year before we decided to just build this ourselves https://twitter.com/TomasReimers/status/1325647290128850950 https://twitter.com/TomasReimers/status/1325647290128850950 Going through everything that's different is hard, but I'll try: When was the last time you made a stcak of changes that was over 30 branches tall? Why? Smaller PRs are generally agreed to be strictly better, and you've probably written a 2000-4000ln pr at least once (which could have been 30 100ln PRs). So why did you not break it out? What part of the tooling was broken?
  - gkop 5y ago
    Parent did not imply that you have or should have marketing people. Your first sentence comes across as defensive. You should express thanks for this feedback and avoid sounding defensive, as that is alienating to people, stifles dialog, and casts you in a weak light. This is your moment in the HN sun, soak it up!
    - fosterfriends 5y ago
      Thank you for the feedback :)
    - 5y ago
dahfizz 5y ago
I do this with Bitbucket all the time. I just create a pull request for the "stacked" change and target it to the branch containing the original work. Once the original work is reviewed and merged, I retarget the stacked pr into the release branch. If the stacked change gets reviewed first for whatever reason, the diff is displayed correctly.
tsvita 5y ago
Such an exciting launch!
stephenturban17 5y ago
I think there’s a real use case here, especially considering how so many of FAANG+ have decided to build their own code review platforms to improve code quality.
- errcorrectcode 5y ago
  Ugh. I'm annoyed when homegrown is the first reaction (something else to support, fewer people to fix it), rather than community &&/|| industry collab.
password4321 5y ago
What is the difference between stacked changes and branchless? https://github.com/arxanas/git-branchless https://github.com/arxanas/git-branchless based off of the branchless Mercurial workflows at large companies such as Google and Facebook
- tomasreimers 5y ago
  A bunch of small things, but the biggest change is we also have a website to help with the authoring/review of PRs experience :)
- arxanas 5y ago
  git-branchless doesn't (yet) aim to integrate with hosted Git providers in any particular way. You just use remotes and push as normal. I think the CLI is mostly at parity in terms of features. git-branchless has `git undo` while gt has `git repo sync`. (Tracking issue for git-branchless's sync: https://github.com/arxanas/git-branchless/issues/174 https://github.com/arxanas/git-branchless/issues/174) git-branchless integrates more directly with Git, so you don't need to learn any new commands if you don't want to. If you ruin your stack by amending an old commit, a warning will appear telling you to run `git restack`, which will fix it. Besides that, git-branchless does rebase operations in-memory, which makes it faster.
errcorrectcode 5y ago
Tiny commits seems normal. I'm wondering if this can squash regions of sequential commits by the same author where it makes sense. The naming clash might be problematic because when I think of graphite, it means: [0]. A unique name would really help avoid confusion. 0. https://graphiteapp.org https://graphiteapp.org
- fosterfriends 5y ago
  The website lets you merge in a stack of approved PRs together, (similar to Facebook's land all). Helps when merging in tons of small changes together. As far as the name, at least the CLI command is clean/similar to git - `gt` :)
willyxiao1 5y ago
Been playing around with this for the last couple of weeks and now I'm getting the whole team to use it at OfficeTogether.com. Really nice!
lingrace16 5y ago
Looks fantastic! Critique (at Google) makes my life so much easier, glad to know that if I ever move companies that there’s a reasonable alternative! Also finally something to recommend to friends that are jealous of Google’s dev tools
strogonoff 5y ago
The article manages to stay entirely free of useful information, despite such a captivating title. See previous HN discussion for more background and substance: https://news.ycombinator.com/item?id=26922633 https://news.ycombinator.com/item?id=26922633
- fosterfriends 5y ago
  Some of the other blog posts and docs have some good deeper info: - CLI docs: https://docs.graphite.dev/guides/graphite-cli https://docs.graphite.dev/guides/graphite-cli - Dashboard docs: https://docs.graphite.dev/guides/graphite-dashboard https://docs.graphite.dev/guides/graphite-dashboard - Storing stack metadata in refs: https://graphite.dev/blog/post/y6ysWaplagKc8YEFzYfr https://graphite.dev/blog/post/y6ysWaplagKc8YEFzYfr - Tracing stacks of branches: https://graphite.dev/blog/post/hGn1nt8nFr5cMhvFJs87 https://graphite.dev/blog/post/hGn1nt8nFr5cMhvFJs87 - Lengthy conversations in the community slack :) https://join.slack.com/t/graphite-community/shared_invite/zt-v828g9dz-TIRvlutxTCqgZmxnsO9Knw https://join.slack.com/t/graphite-community/shared_invite/zt...
havafitz 5y ago
This is awesome! When can those of us on the waitlist expect to get our hands on it? Absolutely cannot wait to get this integrated on some current projects!
- mlutsky1231 5y ago
  Graphite engineer here - we'll be letting folks off the waitlist in waves over the next couple months as we keep iterating on the product (we chat regularly with almost all of our early users), but we're letting anyone with a team of 3 or more on the waitlist get early access so they can try Graphite as a team!
yosito 5y ago
Does this do anything beyond helping you and your team keep track of which branches your feature branch is based off of, and which order they should be merged in? I think you can already do this in git / GitHub. I usually do it by starting a PR with "based on #n which should be merged first". You can stack any number of PRs that way. You can even set the PR target to the base branch to narrow down your diff for review, and change the target to your main branch any time you want. What does Graphite add to this workflow?
- fosterfriends 5y ago
  Great question! It does lots of things. The CLI (https://github.com/screenplaydev/graphite-cli https://github.com/screenplaydev/graphite-cli) lets you: - Recursively rebase changes to keep your branches correctly stacked - Allow you to shift half of a stack onto a different branch - Open up PRs/push changes for all the branches in a stack - Offer to delete merged branches from local, rebases the remaining branches, and adjust github pr merge bases. The web dashboard lets you: - See an inbox of PRs spanning any number of repos, based on which ones need action - Navigate between PRs in a stack - Modify Prs, review, all of which is synced to Github - Shortcuts, client side caching for fast loading, Phabricator style interface, macros, landing a stack of PRs together, and much more :) Give the tool a try, we'd love to hear your feedback in the community Slack! https://join.slack.com/t/graphite-community/shared_invite/zt-v828g9dz-TIRvlutxTCqgZmxnsO9Knw https://join.slack.com/t/graphite-community/shared_invite/zt...
zaven 5y ago
This is awesome, I can’t wait to try it out! I remember when stacked changes first become a thing at FB, and it noticeably improved engineering output and code review ease
naguas 5y ago
Currently waiting on 3 PRs to get reviewed just so I can start developing on top them. This is a major pain point in my current development. My workaround is to have two separate copies of our monorepo locally and develop on both of them since our internal git wrapper doesn't allow two PRs from the same file. It probably wouldn't be that hard to extend our git wrapper to call graphite. Just signed up for the waitlist!
dkhenry 5y ago
I really didn't understand what the big deal was with Stacked Commits when I was just working in vanilla github, and it used to cause friction with members of my team who knew the stacked workflow, but didn't know the Github workflow. After moving to a company that uses stacked commits I will never go back to the pull request workflow. I think there are some famous people saying that Pull Requests aren't Code Reviews, and that becomes obvious once you start using stacked commits for code reviews. This is essentially how cli git is set up to function, and mailing list driven workflows use this concept to have small incremental reviews. I am excited to see this tool, but honestly I have soured so much on github because of the lack of code review, that I would just as soon it didn't try to shoehorn into pull requests, and just did what people do with Phabricator, optionally use GH too host the git repo if you need to, but do everything else in a different tool
- notreallyserio 5y ago
  I still don't understand the big deal so maybe you could help: what is the difference between stacked commits and pull requests targeting branches in other pull requests? I've used GH for code review so I'm not sure what's missing there, either. Reading this whole comment section leaves me a bit confused, like I'm the only one using a hammer to pound in screws when everyone else has moved on to a screwdriver.
  - imiric 5y ago
    GH will also automatically retarget the dependent PRs to the `main` branch after the dependency PR is merged. I usually just make a note in each stacked PR, "Depends on #N", mostly so that the review and merge sequence is followed. But even this would be clear on its own by the PR number and target branch. I agree that the PR code review process leaves a lot to be desired, but this particular workflow doesn't seem like a big problem. Though I've never used Phabricator and similar tooling, so there must be something we're missing.
    - howinteresting 5y ago
      Only for branches in the main repo, not those in forks.
corndoge 5y ago
Can't believe I read this entire article and it just ended with "check out our product" without going into any detail on what it does or how stacked changes work
- tomasreimers 5y ago
  Ah, I'm sorry you didn't find it a good explanation! I Can try and update that; in the meanwhile, maybe you would find the explanation of what the workflow is and how to do it in our docs helpful? https://docs.graphite.dev/getting-started/the-graphite-workflow https://docs.graphite.dev/getting-started/the-graphite-workf...
amedway 5y ago
This has been the biggest pain point for me since leaving big tech! Really excited that there is a solution out there. Well done team!
- fosterfriends 5y ago
  That pain was what inspired us to create Graphite as well :) Thank you
michlim 5y ago
congrats on the launch! love the ui on the dashboard!
- gregmfoster 5y ago
  Thank you! All thanks to Xiulung Choy's hard work
mlovett 5y ago
Love to see tech devoted to improving the lives of developers. This has been a pain point for me for so long.
dlvhdr 5y ago
This is exactly what’s missing in Github. Amazing job!
chillee 5y ago
How is this different from ghstack? https://github.com/ezyang/ghstack https://github.com/ezyang/ghstack (which is what Edward Yang for PyTorch developers to mimic the stacked workflow, although it works with other repos).
- tomasreimers 5y ago
  Hi! We love ghstack here and used it for a while before building this. There are a bunch of small differences between this and the various open source projects that support this workflow (ghstack, git town, spr, etc.), but the biggest reason we chose to diverge was we felt that our code review tooling also had to support stacks in order to have a first class experience. That's why we built out the web dashboard to work in concert with our CLI.
  - chillee 5y ago
    Cool, thanks! Yeah, stack-based PRs are awesome - excited to perhaps try it out.
alexkern 5y ago
Stacked changes are an essential QOL feature when working with large monorepos. Kudos to the Graphite team for expanding the visibility of this development pattern and making it more accessible to teams of all size.
tydalwave 5y ago
So dope! Been watching Tomas and Merrill work on this for the past few months, and now watching it spread through my dev team like wildfire. Congrats Graphite team!!
- fosterfriends 5y ago
  Thank you for the kind words!
luke_heine 5y ago
Super cool product - defo know this problem
brundolf 5y ago
What I sometimes do in this situation is just branch the second feature off of the first one's branch while it's in review, and then when the first one gets merged and/or updated, I rebase the second one. This is a bit of a pain because you have to keep the moving pieces in your head, but it usually works out cleanly enough. Of course you can't really put the second one up for review until the first one has been merged, but you can at least finish the implementation.
- fosterfriends 5y ago
  We tried manually rebasing too, before building Graphite. The challenge you face with manual rebasing is two parts: 1) If you update the bottom branch, you need to manually rebase each branch above it. That becomes brutal if your stack is >3 branches. 2) You cant perform a simple rebase-onto, because you'll copy all commits between the higher branch and trunk. You'd have to perform a three-way rebase, specifying the range of commits you'd like to copy onto the destination. This becomes infeasible by hand. Graphite-cli gets around this by tracking branch metadata and storing it in native git refs (https://graphite.dev/blog/post/y6ysWaplagKc8YEFzYfr https://graphite.dev/blog/post/y6ysWaplagKc8YEFzYfr). When you rebase a stack, it recursively performs the three-way merge to fix things up smoothly. On top of this, git provides no good mechanisms for submitting the stack. Graphite cli can submit/sync your whole stack as individual PRs, and can prune merged branches from the bottom of local stacks. Ends up coming together as a really powerful workflow :) The cli is open source here: https://github.com/screenplaydev/graphite-cli https://github.com/screenplaydev/graphite-cli, with docs here https://docs.graphite.dev/guides/graphite-cli https://docs.graphite.dev/guides/graphite-cli. There's also an active Slack community which helps provide input on new features and adjustments. Please let me know if you have any other questions!
  - onekorg 5y ago
    > 1) If you update the bottom branch, you need to manually rebase each branch above it. That becomes brutal if your stack is >3 branches. I do this relatively easily with `--rebase-merges` flag.
hardwaregeek 5y ago
Yeah one pain point is if you squash your commits on PR merges, you'll have a situation where the branches look like: master: 1, 2, 3 feature-1: 1, 2, 3, 4, 5, 6, feature-2: 1, 2, 3, 4, 5, 6, 7, 8 Then we merge feature-1 into master (numbers in parentheses indicate squashing): master: 1, 2, 3, (4, 5, 6) feature-2: 1, 2, 3, 4, 5, 6, 7, 8 Then say we have another commit to master: master: 1, 2, 3, (4, 5, 6), 9 feature-2: 1, 2, 3, 4, 5, 6, 7, 8 And now we need to rebase feature-2 onto master. We can't do a normal rebase because git will try to do: feature-2: 1, 2, 3, (4, 5, 6), 9, 4, 5, 6, 7, 8 Instead we have to do a slightly tricky maneuver where we rebase just 7, 8 onto master, getting: feature-2: 1, 2, 3, (4, 5, 6), 9, 7, 8 It's not impossible and you can memorize the process pretty easily, but I'd love a natural solution to this problem.
- fosterfriends 5y ago
  This is exactly the problem that graphite-cli solves (https://github.com/screenplaydev/graphite-cli https://github.com/screenplaydev/graphite-cli) It keeps track of branchs and their parents by storing a tiny bit of metadata in the native git refs. It uses that information to perform recursive rebases: https://github.com/screenplaydev/graphite-cli/blob/main/src/actions/fix.ts#L179-L229 https://github.com/screenplaydev/graphite-cli/blob/main/src/... It ends up working seamlessly - you just modify some branch, and then run `gt stack fix` to recursively rebase everything. (and then `gt stack submit` to sync everything to github :) docs here: https://docs.graphite.dev/guides/graphite-cli https://docs.graphite.dev/guides/graphite-cli
  - hardwaregeek 5y ago
    Sounds great! I signed up for the waitlist. I’m always for ways to make version control more ergonomic and feature full
- KptMarchewa 5y ago
  I amend my commits. My PRs are always exactly one commit, so this problem does not exist.
tex0 5y ago
Sooo, like Gerrit?
- tazjin 5y ago
  Gerrit is probably high up on the "Most underrated dev tools" list. People don't like it because it really uses git (which people don't really like) and doesn't have a flashy UI, but it is so good.
karmakaze 5y ago
If you have a stack of 5+ branches, the easiest way I've found to deal with this is to take the top branch with all the commits and rebase it. Then list all the commits in one window and list all the origin commits. You can then do a git branch -f branch-name commit for each branch-name without having to switch branches or do any more rebases. It helps if you have short, easily distinguishable commit messages. Then `git push --force-with-lease` each branch.
yasyfm 5y ago
This is super cool! Any plans to build out more PR features? I'd love to see a Linear-for-pull-requests product.
vander_elst 5y ago
Doesn't Gerrit already allow this?
slaymaker1907 5y ago
You really don't need a bunch of complicated infrastructure for this. I did this in the past by just creating my own high level feature branch and having PRs into said branch. This was all my own branch, but the purpose of the PRs was to get feedback before the whole thing was ready to be merged back to main. While the final PR will be large, everything has already been reviewed so you aren't losing anything in terms of review quality.
vp8989 5y ago
The actual observed problem in the article is that the team's SDLC pipeline can't handle the current flow it's being subjected to. Work is getting "blocked" in code review. Devising a coping mechanism for developers so that they can increase the inbound flow to an already overloaded system is not actually a solution. The real solution to this rather common problem is to introduce a culture where "done" means "shipped" and not "I created a bunch of PRs for my team mates to read and while they all do that Im gonna create some more" and to use tech where you can to increase the flow the SDLC pipeline can handle. This involves things like moving as much "code review" into the compiler/linter as you can, do a little more design/consensus building upfront, streamline all acceptance testing and deployment steps to their bare essentials etc... https://trunkbaseddevelopment.com/ https://trunkbaseddevelopment.com/
- exitheone 5y ago
  None of which works well if your reviewer sits in another timezone and your next business day is their public holiday. Waiting 3 days or more is not an option when I could just comfortably stack my changes and have the whole stack reviewed whenever my reviewer is available.
  - indymike 5y ago
    > Waiting 3 days or more is not an option Waiting for more than 20-30 minutes for a review is ridiculous and means some combination of three problems are present: * commits are too big to be reviewed quickly * Tooling/testing is not catching a lot of blatant problems before the pull request happens. * Review process has turned into some bizarre management ritual
    - richwater 5y ago
      > Waiting for more than 20-30 minutes for a review is ridiculous Does your entire team just sit around doing nothing waiting to review your code? That timeline is asinine.
      - 5y ago
zchauvin 5y ago
When I moved from Facebook to a startup, I remember being surprised by lots about our GitHub-based review process: the prevalence of large PRs, the ambiguity in how PRs related to each other, and the frequent pings you'd get to review other's recently pushed work. I initially encouraged others to use stacked changes to help with all this, but after realizing how clunky it is in GitHub, I stopped. Based on what I've seen so far of Graphite, I think it'll go a long way to making stacked changes simple such that they should be the norm on engineering teams, excited to use it more!
kvm 5y ago
Folks commenting that "I can do stacked diffs by branching off a branch off a branch" are missing the core point that code reviews suck right now. The PR authors hate them, so do the reviewers, and so do folks who will look at them in the future to try and debug code. There's a reason big tech uses stacking: once you stack you can't go back!
nawgz 5y ago
"Stacked changes" is just branching from your PR branch and rebasing when the PR branch changes, no? I am not so sure. I see what makes this idea so revolutionary. Seems pretty obvious to me that after your first PR is ready, if the reviewer isn't, you start working on the downstream topic anyways...
dathinab 5y ago
While I know to well that a lot of tools don't support it well aren't stacked changes the standard??? (Which now that I think about it makes it even more strange that github doesn't support them.) I mean yes, e.g. on github it's somehow pain-full as you have to manually select only the last "n" commits (excluding commits from the previous stack) and then the "file viewed" function might not work correctly anymore :(, oh and the "changes since last review" function also like won't work well anymore (not that it does work well normally). Still even with the drawbacks stacked reviews tend to be faster in small teams (e.g. startups) with a lot to do. Through often you only start reviewing thinks "later in the stack" after a initial review of the first PR and any "major change requests" are done. Similar you have often a less strict security model/more trust into your co-worker to not maliciously abuse it to sneak something in.
epage 5y ago
At my last job, I found our code review process was terribly slow and set about analyzing it and coming up with solutions. I found a lot of developers were blocked on reviews. They either task switched (expensive) or babysat their review (expensive). Even though we were using Phab, few did stacked commits because of how unfamiliar the tools were which made them feel brittle. Improving the stacked workflow is one part of the equation but I'd recommend being careful of letting that mask other problems I identified a series of busy-waits including: - Post, wait, CI failure, fix, post - Post: wait for reviewer, ping reviewer, wait, find feedback was posted, fix, post Some of the root causes included: - Developers weren't being pinged when the ball was in their court (as author or reviewer) - Well, some notifications happened, but review groups were so noisy no one looked - The shared responsibility of review groups is everyone assumed someone else would look. - The tools we were using made it hard to identify active conversations My ideal solution: - Auto-assign reviewers from a review group, allowing shared responsibility of the code base while keeping individual accountability of reviews - Review tools that ping the person the review is blocked on when they are ready to act - Review tools that give a reminder after a set period of time - Azure DevOps' tracking of conversation state with filtering (and toggling between original code and the "fixed" version) - Auto-merge-on-green-checks (including reviewer sign off) With this, responses are timely without people polling the service, letting machines micro-manage the process rather than humans.
- londons_explore 5y ago
  To add to your list: > Reviewer able to make tiny changes to the code without a further round trip to the developer. Things like "missing full stop here" or "Please put a comment warning about X here". Developer should get a heads up that their code has been merged with changes, just so the tiny changes don't go unreviewed.
  - chii 5y ago
    i'm a bit ambivalent about this sort of idea - reviewers adding "tiny" changes without extra reviews could be a place where bugs leak in. what's tiny or insignificant might turn out to be big - that's why code is reviewed by multiple people! I think for style formatting, making it an auto-format step during commit/push is the "better" way. Codified formatting means you argue and hash it all out once and be done with it forever.
bagels 5y ago
Stacked diffs are great. I can now simultaneously wait for many code reviews instead of just one.
taylorqcarol 5y ago
Really exciting - looking forward to implementing this with my team!
- taylorqcarol 5y ago
  When can we expect to get off the waitlist??
trhway 5y ago
it sounds like self-inflicted GitHub specific issue. I was very surprised that people still do it that way when we had to host a project on GitHub instead of our tools. And even on GitHub one can just fork new branch for another change and keep stacking that way.
tdrd 5y ago
I'm surprised Reviewable[0] hasn't come up in this discussion. It does a great job of allowing stacked code reviews and even handles rebases nicely; the reviewer sees the diff between commit #1 and commit #1' (prime = after rebase). CockroachDB[1] has been using it since very early in the project. [0] https://reviewable.io/ https://reviewable.io/ [1] https://github.com/cockroachdb/cockroach https://github.com/cockroachdb/cockroach
- m0zg 5y ago
  I also switched my dev team to it once it grew sufficiently large (at a now acquired startup). The primary thing it enables is actually having a structured conversation about your changes, and having several _revisions_ of your changes through which the reviewer can navigate, much like Gerrit or its progenitor - Critique. Once you experience that, there's simply no going back to anything else. The UX makes me want to gouge my eyes out with a dirty spoon, but it's still worth it, in spite of the UX. I'm baffled why MS/GitHub can't just go ahead and rip it off at this point, to replace its own utterly atrocious PR review workflow.
baybal2 5y ago
Versioned merge requests anyone? This is a solution looking for a (solved) problem.
carlgao 5y ago
This is an absolute game-changer for both writing and reviewing code. Kudos to the Graphite team for bringing this workflow to the world.
reissbaker 5y ago
I've worked at both FB and at smaller companies. The "stacked diff" workflow is quite good, although it can be replicated via Github's PRs via "logical commits." The basic workflow is: 1. Each commit is a Phabricator-style diff. Put the whole idea into a single commit via `git add -p` and rebasing. 2. Multiple ideas/diffs for the same feature? Split them into multiple commits on the same PR. 3. Review the PR commit-by-commit: you can click on the links in Github to the individual commits and see+comment on the changes there. Comments on commits automatically propagate back to the PR FWIW, so this works pretty well. The mental model becomes: the PR is the overview of the full feature, but not how reviewers are expected to review. Reviewers review commits. It works pretty well! Assuming you have buy-in from your team to operate this way. In fact, I actually somewhat prefer it: with "stacked diffs," it can be harder to get the birds-eye view of the entire feature's set of changes. Git and Github are very flexible, so in practice I find you do need some established workflows that everyone agrees to follow, or else it's chaos.
- s4i 5y ago
  But, when using GitHub, isn't the missing puzzle piece here (which Graphite and other "stacked review tools" mentioned here attempt to solve) that you then cannot just merge those individual PRs one by one – you either have to merge the whole PR or nothing? Which leads us to the second option: to use multiple dependent PRs, which causes its own issues explained in other comments.
  - tumetab1 5y ago
    I think his/her perspective, and mine, is that we don't want to merge individual commits. My personal feeling is that I need to review each commit and then I need to review the whole PR (list of commits). If both are good then the PR can be merged but each individual commit is not OK to merge unless it's part of a PR.
  - Liskni_si 5y ago
    > you either have to merge the whole PR or nothing That's not true, though. You absolutely can merge a single commit, via git push, there's just no button in the GitHub UI for it.
mkasberg 5y ago
I proposed a feature along these lines as GitHub feedback. Would love to see more upvotes there if you support the idea! https://github.com/github/feedback/discussions/6125 https://github.com/github/feedback/discussions/6125
taigereye 5y ago
Leveling the playing field a bit more between Google/FB and other companies one diff at a time (:
taigereye 5y ago
Leveling the playing field one diff at a time (:
emodendroket 5y ago
> The blocker here isn’t Git. In fact, the reason you can’t go with option #4 is that your code review tools probably don’t support it: No, git is not the blocker... but neither are the tools. The big blocker is a lack of confidence that someone won't ask me to drastically rework my PR.
- anon9001 5y ago
  > The big blocker is a lack of confidence that someone won't ask me to drastically rework my PR. Another big blocker is the confidence to delicately review a PR and demand it to be drastically reworked. Actually, I think they're the same problem. When your livelihood is tied to other humans, you generally want to minimize conflict.
  - dboreham 5y ago
    There are also control freak psychopaths in our midst who just love telling people what to do and exercising control. They don't really know anything about producing quality software, but they sure know how to tell people they're wrong.
  - emodendroket 5y ago
    Not really the same thing. The point is that if someone wants you to totally rework the base PR then the follow-on work will need to be redone.
jeblair 5y ago
Zuul is an open-source CI/CD/project-gating-system that implements this for Github, GitLab, Gerrit, and pagure. It actually lets you stack dependencies between any of those systems. Just say "Depends-On: <url>" in the PR. See https://zuul-ci.org/ https://zuul-ci.org/ and https://zuul-ci.org/docs/zuul/discussion/gating.html#cross-project-dependencies https://zuul-ci.org/docs/zuul/discussion/gating.html#cross-p...
- tomasreimers 5y ago
  This is awesome! We should add support for this to the CLI, I took a note
- delsarto 5y ago
  As the parent poster well knows (as they invented it) the trick here is that you write your jobs to install from a checkout the CI system does for you. https://opendev.org/opendev/system-config/src/commit/0a279742fc1bbf1a5ceed6afbbe5ab9647bcbc3c/zuul.d/system-config-run.yaml#L73 https://opendev.org/opendev/system-config/src/commit/0a27974... is really nice practical example. What it does isn't even really that important; but it runs a simulation of deploying production code in OpenDev. This is a "devel" job, we deliberately test any changes against all the latest HEADs of the master/main/development branches of projects we use. This is a non-voting job -- a notice that what you're introducing might be fine now, but there is trouble brewing when our dependencies release their next version. Sometimes that's fine and a known issue, or upstream is broken, and other times it's something totally unique and needs to be fixed (this is why, despite AI being able to write code for you, so far the implications of using that code still need a human in the loop :) The "required-projects" in the job definition tells Zuul what repositories this test needs. You can clearly see how it handles various projects having "devel", "main" or "master" branches to pull from to get their latest versions. In the "vars" section you can see we're setting variables that get passed to the job roles flagging "this is the devel job, don't install the latest release but use the source Zuul will checkout for you from here". The amazing thing? If I make a change and this job fails, I may debug it and find that it wasn't actually my fault, but something in upstream Ansible committed recently. I can propose the fix upstream. They do all their CI, and that's fine. But I now put in my change comment Depends-On: https://github.com/ansible/ansible/pull/1234 https://github.com/ansible/ansible/pull/1234 and magically Zuul will recognise that I want to apply that pull request to the Ansible tree in testing and set it up for me (as noted, Zuul can do this for all sorts of systems, not just github). Additionally, Zuul will not merge the change until the dependency is satisfied -- I can NOT commit broken code!
dvirsky 5y ago
While the stacked code review process at Google is pretty smooth - if you're using the default version control system, keeping your chain in sync is a nightmare. If C depends on B depends on A and you made a modification in A, there's no automatic way to rebase anything and you have to start patching B then C manually. It's much much easier with the internal version of Mercurial, but the default VCS doesn't support this method.
omegalulw 5y ago
I don't agree with the premise. There's option 4, start off a new branch for the new feature, make that a new PR. That solves most problems, you can push that new PR for review as soon as your earlier PR is merged in which will allow you to rebase on master.
- ak217 5y ago
  That does not work if your second PR builds upon the first.
ickyforce 5y ago
I don't like the fact that I need to grant read/write access to all public and private repositories to use the dashboard or create PRs. I know that github has no decent solution to limit access scope to 1 repository but this is the reason why I haven't tested graphite on any real work. I guess that the only sane way would be to create a separate github user which only has access to a single repo where I'd like to use/test graphite. At the same time I'd probably be ok with the CLI having wider access, as long as the website does not have it. I can control/monitor CLI to some degree but not the web part.
systematical 5y ago
I'm confused. The solution seems easy. I have PR #80 in review and my next task depends on it. So I branch off of #80 and just starting working on #94. If there is a holdup on #80 and I want feedback on #94 I just create a draft PR with a note about #80. When #80 gets approved then I'll merge it and merge those changes into PR #94. If it gets messy, I'll just torch #94 and use my IDEs local history and do it manually. What exactly do stacked changes do?
- stevewodil 5y ago
  Right but your diff for #94 will include all of the changes in #80 as well, because it's being compared to the main branch. When you do this a few layers up your diff is suddenly massive and hard to navigate for review. Also if you need to change anything about #80 you have to rebase #94. Which also means the same for any other branches that you or someone else created on top of #94 that depended on #80 landing
  - jimmaswell 5y ago
    Make feature branch off main, push, make a PR, checkout main again, make a different feature branch with a different PR, repeat. Why would I continue using the other feature branch for the 2nd?
    - stevewodil 5y ago
      Because if the second feature requires the code changes from the first PR you can't do that... If your team doesn't have a use for stacked diffs that's fine. Teams of one also don't use pull requests and might not understand why we need those.
      - jimmaswell 5y ago
        It's very rarely come up for me that multiple open issues depend on each other. Maybe it happens more in some codebases than others.
algorist 5y ago
this is a game-changer. such a PITA to manage cascading and compounding feature branches. can't wait to give this a try!
amir-h 5y ago
GitHub allows stacking PRs you send PR1 to master and PR2 is a pull request to PR1's branch, the diffs look good, and when PR1 is merged GitHub automatically changes its target branch to master.
- wutbrodo 5y ago
  Yea, I don't quite understand how the article's product is different from this. I also don't understand how anyone uses Github in a professional context without using the workflow you describe... EDIT: Ah, from comments further downthread, it seems that the level of stacking enabled is much deeper, as a lot of the manual process is automated. I stopped using Critique over half a decade ago so I no longer remember the details of how it handled this.
coderdd 5y ago
I worked at Google and have no idea what they mean by stacked change. You could push multiple changes to a single CL, but those were rarely reviewed independently. Maybe they mean keeping a stack of CLs ready, while waiting for a review? I often had 1-2 CLs based on.. rebasing was fun sometimes. Or they mean diffbasing these subsequent CLs, for an early review? That was useful, but due to volatility dependent on the first review, a bit risky. Plug for a code review project: https://reviewpad.com/ https://reviewpad.com/
- exclipy 5y ago
  Stacked diffs is Facebook terminology for diffbased CLs (Google). Except Facebook has invested a lot of tooling into it to make it possible without many of the downsides (see the comments here about 10+ diffs stacked on top of each other).
xunn0026 5y ago
This is just an ad for the startup. It literally doesn't tell you want the thing does. But FB / Google uses it so it must be perfect.
jackhkmatthews 5y ago
Excited for this. I've used Gerrit in the past and really miss it.
penjelly 5y ago
im an idiot but can someone help me understand the following questions? - Doesn’t this mean graphite can access non-public codebases of large organizations that authorize it? - What prevents graphite or its employees from stealing intellectual property or maliciously viewing code for their own purposes? - are there risks in allowing graphite access to codebases in any way? - any and all information regarding privacy with regards to code are welcome, i simply cannot give access to certain codebases to third party organizations without some proof that the data is not going to be maliciously used or accessed. all this comes into question when simply trying to login to graphite website.
throwawaymanbot 5y ago
Unblocked?? Is this a colon-ease advert?

467 points by tomasreimers 5y ago

Not quite sure how a blog post about a new workflow for code review failed to have any screenshots or infographics showing said workflow...

sond813 5y ago

Looks like a great improvement to my workflow, does it require write access to GitHub or can it be read only? Any plans to support GitLab?

tomasreimers 5y ago
The CLI does not require any access to Github, and if you want to start doing code review on the platform, we ask for write access just so we can post comments and reviews :) Gitlab is actively on our roadmap; we currently have a few CLI users that have made it work, but feel free to reach out if it's blocking you.

darepublic 5y ago

Not a fan of phabricator. To me the whole stacked changes thing is at best a necessary evil but not something I would recommend for normal size codebase

stephenturban17 5y ago

I think this is a notable try at a strangely underserved space. Really interesting to see how code review is done in-house at FAANG+ and yet GitHub doesn’t quite get at the level of the tools of these giants.

deleted 5y ago

[deleted]

grozensmith 5y ago

I haven't been on the eng side for a while (PMs just get to pretend like we are technical) but when I was an eng, I basically did stacked changes but with infinite amounts of unnecessary complications. This tool looks fuego. Maybe I'll start coding again.... Kids these days are getting luxuries we never knew back in the day (in 2015 lol).

ayushsood 5y ago

this is the thing I've missed the most about development at Facebook. It seems so small, but is a huge productivity boost. Especially if applied to an entire engineering team

epage 5y ago

For anyone interested, I've been collecting notes on various tools in this space: https://github.com/epage/git-stack/blob/main/docs/comparison.md https://github.com/epage/git-stack/blob/main/docs/comparison... (granted the page doesn't mention git-stack since that is assumed)

shepherdjerred 5y ago
Do you have a favorite? I've struggled with this problem for a long time, but never knew that there were tools to solve it.
- epage 5y ago
  I'm biased as the author of git-stack but struggling with this problem is why I wrote it. When I started it, I didn't know there were so many tools in this space. However, if I knew then what I knew now, I would have still written it. Too many tools require adopting a specialized workflow which doesn't play well with everyone's situation or other git tools. Some do but they require manually defining each change which is too much boilerplate for me. My goal with git-stack has been to automate my PR workflow and sit alongside other git tools. I've even tried to keep it open so you can use parts of it with Phab or other tools. You should be able to start using it out of the box with a standard PR model. It'll have better performance and behavior if you configure which branches are shared branches and mark them as "protected" in git-stack. If you don't use a PR model, then that might just don't run `git stack --push` but the rest should still do a reasonable job. If I were to pick a tool besides git-stack, it would either be git-branchless or Graphite.

RHSeeger 5y ago

> The blocker here isn’t Git. In fact, the reason you can’t go with option #4 is that your code review tools probably don’t support it I do stacked commits (branches off branches) all the time. Github certainly supports PRs based on such.

jas14 5y ago
I still haven't found a reliable way to prevent merging branches that aren't the first in the stack :/
- fosterfriends 5y ago
  (Graphite engineer) We had the same problem. Coded in a feature that prevents you from casually merging branches that are not bottom of the stack :)
- _freds 5y ago
  What I usually do is if I need to stack more PRs I open them as a draft to avoid accidental merges. I then convert each PR to a non-draft as I “pop” the stack.
tomasreimers 5y ago
How do you deal with rebases when you have to?
- sigil 5y ago
  Like so: git switch offshoot git rebase --onto feature branchpoint^ Will replay your offshoot branch onto its underlying feature branch after the feature branch has been rebased. The branchpoint is the first commit that diverges from the feature branch. Finding the branchpoint commit is the only real annoyance here -- if there's a ref or a better trick for finding it, I'm all ears.
  - connorbrinton 5y ago
    My usual workflow to rebase a stacked set of feature branches off of main is: git checkout main git pull git checkout feature-branch-1 git rebase -i - git checkout feature-branch-2 git rebase -i - ... There's most likely a more efficient way to do this, but I find that referring to the most recently checked-out branch with a single hyphen is a lesser-known feature, so perhaps it will be helpful :)

secondcoming 5y ago

> You're under a deadline and your next highest priority task is to add reactions to those comments. What's a 'reaction'? > Why can’t you write the reactions branch on top of the comments branch What? Why would I have a 'comments branch'? Having never worked at FAAANG, I've no idea what this marketing material is about, or why I'd need it.

joshuamorton 5y ago
Feature branching is a fairly common practice. "Reactions" are emoji that you can use to respond to a post or comment, like Facebook's like/love/laugh/cry/angry, or slack/discord/messenger/chat/whatever's various ways to emoji-reply to messages. You would have a comments branch if you were adding a feature called "comments", I guess.
- secondcoming 5y ago
  I understand what a feature branch is. I don't understand why I'd need a branch to add an emoji. I assumed that a 'reactions' branch might be a branch that addresses issues in the original PR, but even if it was, why would I need a new branch for that?
  - tomasreimers 5y ago
    Probably because you're trying to break up your changes. Two good blog posts that talk about this workflow are: https://jg.gg/2018/09/29/stacked-diffs-versus-pull-requests/ https://jg.gg/2018/09/29/stacked-diffs-versus-pull-requests/ and https://kurtisnusbaum.medium.com/stacked-diffs-keeping-phabricator-diffs-small-d9964f4dcfa6 https://kurtisnusbaum.medium.com/stacked-diffs-keeping-phabr...
    - secondcoming 5y ago
      Those were useful, thanks!
  - joshuamorton 5y ago

davidw 5y ago

After seeing so much noise and struggle at the past couple places I've worked regarding people having to beg and make noise about getting reviews, I half jokingly think someone should make a "begging for PR reviews as a service" startup.

Nextgrid 5y ago
The problem is that in most teams code review is considered an auxiliary task that's off the books (from a ticket tracker perspective) where as in reality it should be a ticket in its own right that someone can assign themselves to and review the associated PR.
- ericbarrett 5y ago
  This is a team culture thing. Prompt and professional reviews have to be valued. If you're a senior engineer (or at least senior on a code base), you can add at least as much value to the development process by giving others timely and fair feedback as you can writing your own code—it's a force multiplier. (Edit) For big changes I've seen people schedule 30-minute review meetings. We all hate meetings but I don't think it's a bad idea.
  - davidw 5y ago
    Everyone says they value good reviews, and I think that's actually true to some extent, but 'prompt' comes at a real cost as you have to stop what you're doing to take a look.
    - ericbarrett 5y ago
      The strategy I used for smaller changes was twice-a-day queue review; noonish and end of day. "Prompt" doesn't have to mean instant, it just means not sitting on a change for days.
    - toast0 5y ago
      Not prompt reviewing has a real cost too. If it takes a long time to get a review, the submitter loses context and has to regain it when the review comes in. The longer the cycle, the more contexts to be juggled and the more effort to juggle them.

jrochkind1 5y ago

I have definitely had this problem before and wanted this solution. Not sure if I want it enough to introduce a third-party tool to my relatively small team(s). I wish Github just had it built in.

fragmede 5y ago
it is, it's just not exposed well. in git create two branches stacked on top of each other, and then in the PR creation window, the first PR goes from master to branch-1 as normal. Then create a second PR for branch-1 to branch-2. it's a bit of futzing around with git and GitHub but it's there/works without another tool. Or you could just use the tool, up to you.
- jrochkind1 5y ago
  I have had trouble with GH displaying the correct diff. I guess in most cases I don't really want to merge branch-2 into branch-1 either, I want to merge them both into master -- but first branch-1, and only then branch-2. I don't know if that's the use-case the third party tool is focused on? When I try that with github alone, which I have... it just gets really messy. Sometimes I need to switch the PR to a different random base branch and then back to the actually desired one to get github to update the diff when the base branch has changed since PR opened.

zurawiki 5y ago

> This idea isn't new, and if you've worked at a company like Facebook, Google, Uber, Dropbox, or Lyft (among others), you've either had first-class support for stacked changes built into your code review platform (i.e. Phabricator at Facebook... Phabricator got a lot of stuff about code review right (I may be biased from working at FAANG). I'm happy to see someone trying to improve the code review situation on GitHub. When I did use GitHub at the startups I worked at, we were never fully satisfied with the code review experience. I hope they surface "Phabricator versions" as first-class objects. I never liked it how GitHub just clobbers the history left behind from force-pushes from to branch.

gibolt 5y ago
The ability to quickly manage branches and reorder commits within them in Mercurial is super powerful, something that Git and `rebase -i` could only dream of. Doing the same in Phabricator is awesome. One blocking commit PR on the bottom of a stack can easily be moved to the top or a new stack, unblocking the rest of your changes. Re-writing and entire stack due to one small change is mostly eliminated. (Good) Tooling is a major gain in efficiency.
- tadfisher 5y ago
  > The ability to quickly manage branches and reorder commits within them in Mercurial is super powerful, something that Git and 'rebase could only dream of. This is a git CLI wart more than anything. Magit (git interface in Emacs) makes juggling branches/worktrees and rewriting history intuitive to humans. I feel super-powerful in Magit and absolutely feeble using the CLI.
- surajrmal 5y ago
  I completely agree. When Google decided to invest in a mercurial front end to their version control system a few years ago, I was confused, but after using it for a while, I can honestly say it has much better ergonomics for common workflows. Creating a tree of dependent commits is simple and trivial. I sometimes wish I could create a dag instead, but that's probably more trouble than it's worth.
5y ago

fosterfriends 5y ago

The part of Graphite that helps manage stacked changes locally is open source - you can see how it handles the recursive rebases here: https://github.com/screenplaydev/graphite-cli/blob/main/src/actions/fix.ts#L179-L229 https://github.com/screenplaydev/graphite-cli/blob/main/src/...

barbazoo 5y ago

Usually when feature-branch-1 is still under review and feature 2 relies on feature 1, I just open a PR from feature-branch-2 onto feature-branch-1 to get the review process started, knowing that feature-branch-1 might still change which I can deal with by rebasing.

milkytron 5y ago
Yeah this shouldn't be an issue when following standard git branching practices. It's kinda the point
fosterfriends 5y ago
I came from Airbnb (vanilla github) and some friends converted me to a stacked workflow. I think the challenge with vanilla git is that rebasing and re-pushing becomes a brutal process if you stack 3+ changes. (At facebook people regularly stack over 10). Beyond recursive rebases, Github offers no support for navigating your stack, merging in a stack of approved PRs together, adjusting merge bases, etc. Most of these practices are just inspired by what Phabricator and a few of the big companies are already doing well :)
- treis 5y ago
  >(At facebook people regularly stack over 10). Meaning branch 1 depends on branch 2 which depends branch 3... all the way to 10? That just seems crazy to me. What happens if branch 4 changes their implementation and borks everything depending on it?
  - shepherdjerred 5y ago
    You have to fix a lot of merge conflicts. I used this workflow at AWS -- code reviews can take some time, you're writing a lot of code, and you want your CRs to be small. If someone leaves a comment on the CR for the first branch you fix the issue and rebase. It's not fun, but this is the only effective way to do it (that I know of).
    - 5y ago

jpochtar 5y ago

I've been waiting for this forever. Stacked diffs is how I've always worked even before working at FANG, and github just doesn't support them right out of the box. Kudos for getting this right!

unemphysbro 5y ago
how do you typically stack diffs?

gmaster1440 5y ago

Worth noting that this is coming to GitHub: https://github.blog/changelog/2021-10-27-pull-request-merge-queue-limited-beta/ https://github.blog/changelog/2021-10-27-pull-request-merge-...

tomasreimers 5y ago
So merge queue, while cool - is slightly different than Stacked changes. Stacked changes are a better client-side git workflow for dealing with branches on top of branches.

jeffbee 5y ago

Having worked with several large commercial monorepos including Google's I can recall using stacked changes a few times but I don't remember them being the secret sauce by any means. They were more of an edge case.

fosterfriends 5y ago
Even if you're not a fan of stacking your changes, I think the web dashboard has a lot of fan favorites from the now deprecated Phabricator, such as seeing the PR comment timeline side-by-side the code, macros, showing lines that were simply copied, etc. We've also tried to make it as snappy and modern as possible, resulting in something that feels a lot faster than github for reviewing PRs :) Would love to get your feedback if you try it out!

didibus 5y ago

Even when the tool support it, I'm not sure it's a good practice. Small code reviews often miss the forest for the tree. When you make the extent of the feature changes even harder to mentally trace back and put together by having a diff over another set of code that is itself an unapproved diff, the code review becomes pretty low quality. I've also done this where my change that depend on other changes get approved, but then the other set of code gets critiqued so I have to change it and that breaks the code that depends on it so I have to change that as well, and now I still need to get it all reviewed again, so really I just wasted someone's time the first round. What I do now is that I make commits over it in my branch, so I keep working while they review my first commit, and when that's approved I send the next commit to be reviewed, and so on. If there are comments I do an interactive rebase to fix them.

polishdude20 5y ago
That's what we do as well but we've got merge block labels. If a pr depends on another PR, we put a merge blocked label on it and reference the PR in the comments. We've got a GitHub action which doesn't let a merge happen unless zero labels are present.
fosterfriends 5y ago
Google had a good paper on the benefits of smaller changes: https://sback.it/publications/icse2018seip.pdf https://sback.it/publications/icse2018seip.pdf > A correlation between change size and review quality is acknowledged by Google and developers are strongly encouraged to make small, incremental changes Heard though about wanting to maintain context within a stack. I think the best balance is tooling that both lets you create a stack of small changes, while also seeing each change in the broader context (forest). The benefit of breaking out PRs instead of commits, is that each small change gets to pass review and CI atomically. I like your strategy of gating commit pushes until the first is reviewed, but I think the dream is to decouple those processes :)
- jacobr1 5y ago
  Having worked in startups for most my career, and now recently joining a BigTechCo, I think some of the cross-talk comes from the commingling of concerns within a PR. PRs are used to review architecture, enforce security/code-conventions/style, check for logic bugs, catch potential conflicts with other efforts, and validate the logic of a feature. In BigTechCo you have more coordination problems, so you want to merge code to trunk as soon as possible to prevent integration risks. But that means you drive discussion of the architecture and features to other venues than than the code review. Code review becomes more of a "will this break things" check, and enabling a feature-flag is more the review stage for the feature itself.

KptMarchewa 5y ago

> How do you tell reviewers that your reactions PR depends on the comments PR? It's at the top of pr. "someone wants to merge 1 commit into random-branch from another-random-branch" > How will reviewers see the relevant diff for the reactions PR (excluding the comments PR changes)? It will show it by default? If they want to see comments PR changes, they will need to go to comments PR. > How can you propagate changes to the reactions PR if you ever need to update the comments PR before you land it (i.e. to address review comments)? Use git rebase --onto. It would be nice if the marketing page told prospective users what exact difference their tool provides - since I'm already using that workflow and never seen one of those internal tools.

tomasreimers 5y ago
Hi, sadly no marketing people at this company. Just engineers and product people. We were trying to get this to work on Github for almost a year before we decided to just build this ourselves https://twitter.com/TomasReimers/status/1325647290128850950 https://twitter.com/TomasReimers/status/1325647290128850950 Going through everything that's different is hard, but I'll try: When was the last time you made a stcak of changes that was over 30 branches tall? Why? Smaller PRs are generally agreed to be strictly better, and you've probably written a 2000-4000ln pr at least once (which could have been 30 100ln PRs). So why did you not break it out? What part of the tooling was broken?
- gkop 5y ago
  Parent did not imply that you have or should have marketing people. Your first sentence comes across as defensive. You should express thanks for this feedback and avoid sounding defensive, as that is alienating to people, stifles dialog, and casts you in a weak light. This is your moment in the HN sun, soak it up!
  - fosterfriends 5y ago
    Thank you for the feedback :)
  - 5y ago

dahfizz 5y ago

I do this with Bitbucket all the time. I just create a pull request for the "stacked" change and target it to the branch containing the original work. Once the original work is reviewed and merged, I retarget the stacked pr into the release branch. If the stacked change gets reviewed first for whatever reason, the diff is displayed correctly.

tsvita 5y ago

Such an exciting launch!

stephenturban17 5y ago

I think there’s a real use case here, especially considering how so many of FAANG+ have decided to build their own code review platforms to improve code quality.

errcorrectcode 5y ago
Ugh. I'm annoyed when homegrown is the first reaction (something else to support, fewer people to fix it), rather than community &&/|| industry collab.

password4321 5y ago

What is the difference between stacked changes and branchless? https://github.com/arxanas/git-branchless https://github.com/arxanas/git-branchless based off of the branchless Mercurial workflows at large companies such as Google and Facebook

tomasreimers 5y ago
A bunch of small things, but the biggest change is we also have a website to help with the authoring/review of PRs experience :)
arxanas 5y ago
git-branchless doesn't (yet) aim to integrate with hosted Git providers in any particular way. You just use remotes and push as normal. I think the CLI is mostly at parity in terms of features. git-branchless has `git undo` while gt has `git repo sync`. (Tracking issue for git-branchless's sync: https://github.com/arxanas/git-branchless/issues/174 https://github.com/arxanas/git-branchless/issues/174) git-branchless integrates more directly with Git, so you don't need to learn any new commands if you don't want to. If you ruin your stack by amending an old commit, a warning will appear telling you to run `git restack`, which will fix it. Besides that, git-branchless does rebase operations in-memory, which makes it faster.

errcorrectcode 5y ago

Tiny commits seems normal. I'm wondering if this can squash regions of sequential commits by the same author where it makes sense. The naming clash might be problematic because when I think of graphite, it means: [0]. A unique name would really help avoid confusion. 0. https://graphiteapp.org https://graphiteapp.org

fosterfriends 5y ago
The website lets you merge in a stack of approved PRs together, (similar to Facebook's land all). Helps when merging in tons of small changes together. As far as the name, at least the CLI command is clean/similar to git - `gt` :)

willyxiao1 5y ago

Been playing around with this for the last couple of weeks and now I'm getting the whole team to use it at OfficeTogether.com. Really nice!

lingrace16 5y ago

Looks fantastic! Critique (at Google) makes my life so much easier, glad to know that if I ever move companies that there’s a reasonable alternative! Also finally something to recommend to friends that are jealous of Google’s dev tools

strogonoff 5y ago

The article manages to stay entirely free of useful information, despite such a captivating title. See previous HN discussion for more background and substance: https://news.ycombinator.com/item?id=26922633 https://news.ycombinator.com/item?id=26922633

fosterfriends 5y ago
Some of the other blog posts and docs have some good deeper info: - CLI docs: https://docs.graphite.dev/guides/graphite-cli https://docs.graphite.dev/guides/graphite-cli - Dashboard docs: https://docs.graphite.dev/guides/graphite-dashboard https://docs.graphite.dev/guides/graphite-dashboard - Storing stack metadata in refs: https://graphite.dev/blog/post/y6ysWaplagKc8YEFzYfr https://graphite.dev/blog/post/y6ysWaplagKc8YEFzYfr - Tracing stacks of branches: https://graphite.dev/blog/post/hGn1nt8nFr5cMhvFJs87 https://graphite.dev/blog/post/hGn1nt8nFr5cMhvFJs87 - Lengthy conversations in the community slack :) https://join.slack.com/t/graphite-community/shared_invite/zt-v828g9dz-TIRvlutxTCqgZmxnsO9Knw https://join.slack.com/t/graphite-community/shared_invite/zt...

havafitz 5y ago

This is awesome! When can those of us on the waitlist expect to get our hands on it? Absolutely cannot wait to get this integrated on some current projects!

mlutsky1231 5y ago
Graphite engineer here - we'll be letting folks off the waitlist in waves over the next couple months as we keep iterating on the product (we chat regularly with almost all of our early users), but we're letting anyone with a team of 3 or more on the waitlist get early access so they can try Graphite as a team!

yosito 5y ago

Does this do anything beyond helping you and your team keep track of which branches your feature branch is based off of, and which order they should be merged in? I think you can already do this in git / GitHub. I usually do it by starting a PR with "based on #n which should be merged first". You can stack any number of PRs that way. You can even set the PR target to the base branch to narrow down your diff for review, and change the target to your main branch any time you want. What does Graphite add to this workflow?

fosterfriends 5y ago
Great question! It does lots of things. The CLI (https://github.com/screenplaydev/graphite-cli https://github.com/screenplaydev/graphite-cli) lets you: - Recursively rebase changes to keep your branches correctly stacked - Allow you to shift half of a stack onto a different branch - Open up PRs/push changes for all the branches in a stack - Offer to delete merged branches from local, rebases the remaining branches, and adjust github pr merge bases. The web dashboard lets you: - See an inbox of PRs spanning any number of repos, based on which ones need action - Navigate between PRs in a stack - Modify Prs, review, all of which is synced to Github - Shortcuts, client side caching for fast loading, Phabricator style interface, macros, landing a stack of PRs together, and much more :) Give the tool a try, we'd love to hear your feedback in the community Slack! https://join.slack.com/t/graphite-community/shared_invite/zt-v828g9dz-TIRvlutxTCqgZmxnsO9Knw https://join.slack.com/t/graphite-community/shared_invite/zt...

zaven 5y ago

This is awesome, I can’t wait to try it out! I remember when stacked changes first become a thing at FB, and it noticeably improved engineering output and code review ease

naguas 5y ago

Currently waiting on 3 PRs to get reviewed just so I can start developing on top them. This is a major pain point in my current development. My workaround is to have two separate copies of our monorepo locally and develop on both of them since our internal git wrapper doesn't allow two PRs from the same file. It probably wouldn't be that hard to extend our git wrapper to call graphite. Just signed up for the waitlist!

dkhenry 5y ago

I really didn't understand what the big deal was with Stacked Commits when I was just working in vanilla github, and it used to cause friction with members of my team who knew the stacked workflow, but didn't know the Github workflow. After moving to a company that uses stacked commits I will never go back to the pull request workflow. I think there are some famous people saying that Pull Requests aren't Code Reviews, and that becomes obvious once you start using stacked commits for code reviews. This is essentially how cli git is set up to function, and mailing list driven workflows use this concept to have small incremental reviews. I am excited to see this tool, but honestly I have soured so much on github because of the lack of code review, that I would just as soon it didn't try to shoehorn into pull requests, and just did what people do with Phabricator, optionally use GH too host the git repo if you need to, but do everything else in a different tool

notreallyserio 5y ago
I still don't understand the big deal so maybe you could help: what is the difference between stacked commits and pull requests targeting branches in other pull requests? I've used GH for code review so I'm not sure what's missing there, either. Reading this whole comment section leaves me a bit confused, like I'm the only one using a hammer to pound in screws when everyone else has moved on to a screwdriver.
- imiric 5y ago
  GH will also automatically retarget the dependent PRs to the `main` branch after the dependency PR is merged. I usually just make a note in each stacked PR, "Depends on #N", mostly so that the review and merge sequence is followed. But even this would be clear on its own by the PR number and target branch. I agree that the PR code review process leaves a lot to be desired, but this particular workflow doesn't seem like a big problem. Though I've never used Phabricator and similar tooling, so there must be something we're missing.
  - howinteresting 5y ago
    Only for branches in the main repo, not those in forks.

corndoge 5y ago

Can't believe I read this entire article and it just ended with "check out our product" without going into any detail on what it does or how stacked changes work

tomasreimers 5y ago
Ah, I'm sorry you didn't find it a good explanation! I Can try and update that; in the meanwhile, maybe you would find the explanation of what the workflow is and how to do it in our docs helpful? https://docs.graphite.dev/getting-started/the-graphite-workflow https://docs.graphite.dev/getting-started/the-graphite-workf...

amedway 5y ago

This has been the biggest pain point for me since leaving big tech! Really excited that there is a solution out there. Well done team!

fosterfriends 5y ago
That pain was what inspired us to create Graphite as well :) Thank you

michlim 5y ago

congrats on the launch! love the ui on the dashboard!

gregmfoster 5y ago
Thank you! All thanks to Xiulung Choy's hard work

mlovett 5y ago

Love to see tech devoted to improving the lives of developers. This has been a pain point for me for so long.

dlvhdr 5y ago

This is exactly what’s missing in Github. Amazing job!

chillee 5y ago

How is this different from ghstack? https://github.com/ezyang/ghstack https://github.com/ezyang/ghstack (which is what Edward Yang for PyTorch developers to mimic the stacked workflow, although it works with other repos).

tomasreimers 5y ago
Hi! We love ghstack here and used it for a while before building this. There are a bunch of small differences between this and the various open source projects that support this workflow (ghstack, git town, spr, etc.), but the biggest reason we chose to diverge was we felt that our code review tooling also had to support stacks in order to have a first class experience. That's why we built out the web dashboard to work in concert with our CLI.
- chillee 5y ago
  Cool, thanks! Yeah, stack-based PRs are awesome - excited to perhaps try it out.

alexkern 5y ago

Stacked changes are an essential QOL feature when working with large monorepos. Kudos to the Graphite team for expanding the visibility of this development pattern and making it more accessible to teams of all size.

tydalwave 5y ago

So dope! Been watching Tomas and Merrill work on this for the past few months, and now watching it spread through my dev team like wildfire. Congrats Graphite team!!

fosterfriends 5y ago
Thank you for the kind words!

luke_heine 5y ago

Super cool product - defo know this problem

brundolf 5y ago

What I sometimes do in this situation is just branch the second feature off of the first one's branch while it's in review, and then when the first one gets merged and/or updated, I rebase the second one. This is a bit of a pain because you have to keep the moving pieces in your head, but it usually works out cleanly enough. Of course you can't really put the second one up for review until the first one has been merged, but you can at least finish the implementation.

fosterfriends 5y ago
We tried manually rebasing too, before building Graphite. The challenge you face with manual rebasing is two parts: 1) If you update the bottom branch, you need to manually rebase each branch above it. That becomes brutal if your stack is >3 branches. 2) You cant perform a simple rebase-onto, because you'll copy all commits between the higher branch and trunk. You'd have to perform a three-way rebase, specifying the range of commits you'd like to copy onto the destination. This becomes infeasible by hand. Graphite-cli gets around this by tracking branch metadata and storing it in native git refs (https://graphite.dev/blog/post/y6ysWaplagKc8YEFzYfr https://graphite.dev/blog/post/y6ysWaplagKc8YEFzYfr). When you rebase a stack, it recursively performs the three-way merge to fix things up smoothly. On top of this, git provides no good mechanisms for submitting the stack. Graphite cli can submit/sync your whole stack as individual PRs, and can prune merged branches from the bottom of local stacks. Ends up coming together as a really powerful workflow :) The cli is open source here: https://github.com/screenplaydev/graphite-cli https://github.com/screenplaydev/graphite-cli, with docs here https://docs.graphite.dev/guides/graphite-cli https://docs.graphite.dev/guides/graphite-cli. There's also an active Slack community which helps provide input on new features and adjustments. Please let me know if you have any other questions!
- onekorg 5y ago
  > 1) If you update the bottom branch, you need to manually rebase each branch above it. That becomes brutal if your stack is >3 branches. I do this relatively easily with `--rebase-merges` flag.

hardwaregeek 5y ago

Yeah one pain point is if you squash your commits on PR merges, you'll have a situation where the branches look like: master: 1, 2, 3 feature-1: 1, 2, 3, 4, 5, 6, feature-2: 1, 2, 3, 4, 5, 6, 7, 8 Then we merge feature-1 into master (numbers in parentheses indicate squashing): master: 1, 2, 3, (4, 5, 6) feature-2: 1, 2, 3, 4, 5, 6, 7, 8 Then say we have another commit to master: master: 1, 2, 3, (4, 5, 6), 9 feature-2: 1, 2, 3, 4, 5, 6, 7, 8 And now we need to rebase feature-2 onto master. We can't do a normal rebase because git will try to do: feature-2: 1, 2, 3, (4, 5, 6), 9, 4, 5, 6, 7, 8 Instead we have to do a slightly tricky maneuver where we rebase just 7, 8 onto master, getting: feature-2: 1, 2, 3, (4, 5, 6), 9, 7, 8 It's not impossible and you can memorize the process pretty easily, but I'd love a natural solution to this problem.

fosterfriends 5y ago
This is exactly the problem that graphite-cli solves (https://github.com/screenplaydev/graphite-cli https://github.com/screenplaydev/graphite-cli) It keeps track of branchs and their parents by storing a tiny bit of metadata in the native git refs. It uses that information to perform recursive rebases: https://github.com/screenplaydev/graphite-cli/blob/main/src/actions/fix.ts#L179-L229 https://github.com/screenplaydev/graphite-cli/blob/main/src/... It ends up working seamlessly - you just modify some branch, and then run `gt stack fix` to recursively rebase everything. (and then `gt stack submit` to sync everything to github :) docs here: https://docs.graphite.dev/guides/graphite-cli https://docs.graphite.dev/guides/graphite-cli
- hardwaregeek 5y ago
  Sounds great! I signed up for the waitlist. I’m always for ways to make version control more ergonomic and feature full
KptMarchewa 5y ago
I amend my commits. My PRs are always exactly one commit, so this problem does not exist.

tex0 5y ago

Sooo, like Gerrit?

tazjin 5y ago
Gerrit is probably high up on the "Most underrated dev tools" list. People don't like it because it really uses git (which people don't really like) and doesn't have a flashy UI, but it is so good.

karmakaze 5y ago

If you have a stack of 5+ branches, the easiest way I've found to deal with this is to take the top branch with all the commits and rebase it. Then list all the commits in one window and list all the origin commits. You can then do a git branch -f branch-name commit for each branch-name without having to switch branches or do any more rebases. It helps if you have short, easily distinguishable commit messages. Then `git push --force-with-lease` each branch.

yasyfm 5y ago

This is super cool! Any plans to build out more PR features? I'd love to see a Linear-for-pull-requests product.

vander_elst 5y ago

Doesn't Gerrit already allow this?

slaymaker1907 5y ago

You really don't need a bunch of complicated infrastructure for this. I did this in the past by just creating my own high level feature branch and having PRs into said branch. This was all my own branch, but the purpose of the PRs was to get feedback before the whole thing was ready to be merged back to main. While the final PR will be large, everything has already been reviewed so you aren't losing anything in terms of review quality.

vp8989 5y ago

The actual observed problem in the article is that the team's SDLC pipeline can't handle the current flow it's being subjected to. Work is getting "blocked" in code review. Devising a coping mechanism for developers so that they can increase the inbound flow to an already overloaded system is not actually a solution. The real solution to this rather common problem is to introduce a culture where "done" means "shipped" and not "I created a bunch of PRs for my team mates to read and while they all do that Im gonna create some more" and to use tech where you can to increase the flow the SDLC pipeline can handle. This involves things like moving as much "code review" into the compiler/linter as you can, do a little more design/consensus building upfront, streamline all acceptance testing and deployment steps to their bare essentials etc... https://trunkbaseddevelopment.com/ https://trunkbaseddevelopment.com/

exitheone 5y ago
None of which works well if your reviewer sits in another timezone and your next business day is their public holiday. Waiting 3 days or more is not an option when I could just comfortably stack my changes and have the whole stack reviewed whenever my reviewer is available.
- indymike 5y ago
  > Waiting 3 days or more is not an option Waiting for more than 20-30 minutes for a review is ridiculous and means some combination of three problems are present: * commits are too big to be reviewed quickly * Tooling/testing is not catching a lot of blatant problems before the pull request happens. * Review process has turned into some bizarre management ritual
  - richwater 5y ago
    > Waiting for more than 20-30 minutes for a review is ridiculous Does your entire team just sit around doing nothing waiting to review your code? That timeline is asinine.
    - 5y ago

zchauvin 5y ago

When I moved from Facebook to a startup, I remember being surprised by lots about our GitHub-based review process: the prevalence of large PRs, the ambiguity in how PRs related to each other, and the frequent pings you'd get to review other's recently pushed work. I initially encouraged others to use stacked changes to help with all this, but after realizing how clunky it is in GitHub, I stopped. Based on what I've seen so far of Graphite, I think it'll go a long way to making stacked changes simple such that they should be the norm on engineering teams, excited to use it more!

kvm 5y ago

Folks commenting that "I can do stacked diffs by branching off a branch off a branch" are missing the core point that code reviews suck right now. The PR authors hate them, so do the reviewers, and so do folks who will look at them in the future to try and debug code. There's a reason big tech uses stacking: once you stack you can't go back!

nawgz 5y ago

"Stacked changes" is just branching from your PR branch and rebasing when the PR branch changes, no? I am not so sure. I see what makes this idea so revolutionary. Seems pretty obvious to me that after your first PR is ready, if the reviewer isn't, you start working on the downstream topic anyways...

dathinab 5y ago

While I know to well that a lot of tools don't support it well aren't stacked changes the standard??? (Which now that I think about it makes it even more strange that github doesn't support them.) I mean yes, e.g. on github it's somehow pain-full as you have to manually select only the last "n" commits (excluding commits from the previous stack) and then the "file viewed" function might not work correctly anymore :(, oh and the "changes since last review" function also like won't work well anymore (not that it does work well normally). Still even with the drawbacks stacked reviews tend to be faster in small teams (e.g. startups) with a lot to do. Through often you only start reviewing thinks "later in the stack" after a initial review of the first PR and any "major change requests" are done. Similar you have often a less strict security model/more trust into your co-worker to not maliciously abuse it to sneak something in.

epage 5y ago

At my last job, I found our code review process was terribly slow and set about analyzing it and coming up with solutions. I found a lot of developers were blocked on reviews. They either task switched (expensive) or babysat their review (expensive). Even though we were using Phab, few did stacked commits because of how unfamiliar the tools were which made them feel brittle. Improving the stacked workflow is one part of the equation but I'd recommend being careful of letting that mask other problems I identified a series of busy-waits including: - Post, wait, CI failure, fix, post - Post: wait for reviewer, ping reviewer, wait, find feedback was posted, fix, post Some of the root causes included: - Developers weren't being pinged when the ball was in their court (as author or reviewer) - Well, some notifications happened, but review groups were so noisy no one looked - The shared responsibility of review groups is everyone assumed someone else would look. - The tools we were using made it hard to identify active conversations My ideal solution: - Auto-assign reviewers from a review group, allowing shared responsibility of the code base while keeping individual accountability of reviews - Review tools that ping the person the review is blocked on when they are ready to act - Review tools that give a reminder after a set period of time - Azure DevOps' tracking of conversation state with filtering (and toggling between original code and the "fixed" version) - Auto-merge-on-green-checks (including reviewer sign off) With this, responses are timely without people polling the service, letting machines micro-manage the process rather than humans.

londons_explore 5y ago
To add to your list: > Reviewer able to make tiny changes to the code without a further round trip to the developer. Things like "missing full stop here" or "Please put a comment warning about X here". Developer should get a heads up that their code has been merged with changes, just so the tiny changes don't go unreviewed.
- chii 5y ago
  i'm a bit ambivalent about this sort of idea - reviewers adding "tiny" changes without extra reviews could be a place where bugs leak in. what's tiny or insignificant might turn out to be big - that's why code is reviewed by multiple people! I think for style formatting, making it an auto-format step during commit/push is the "better" way. Codified formatting means you argue and hash it all out once and be done with it forever.

bagels 5y ago

Stacked diffs are great. I can now simultaneously wait for many code reviews instead of just one.

taylorqcarol 5y ago

Really exciting - looking forward to implementing this with my team!

taylorqcarol 5y ago
When can we expect to get off the waitlist??

trhway 5y ago

it sounds like self-inflicted GitHub specific issue. I was very surprised that people still do it that way when we had to host a project on GitHub instead of our tools. And even on GitHub one can just fork new branch for another change and keep stacking that way.

tdrd 5y ago

I'm surprised Reviewable[0] hasn't come up in this discussion. It does a great job of allowing stacked code reviews and even handles rebases nicely; the reviewer sees the diff between commit #1 and commit #1' (prime = after rebase). CockroachDB[1] has been using it since very early in the project. [0] https://reviewable.io/ https://reviewable.io/ [1] https://github.com/cockroachdb/cockroach https://github.com/cockroachdb/cockroach

m0zg 5y ago
I also switched my dev team to it once it grew sufficiently large (at a now acquired startup). The primary thing it enables is actually having a structured conversation about your changes, and having several _revisions_ of your changes through which the reviewer can navigate, much like Gerrit or its progenitor - Critique. Once you experience that, there's simply no going back to anything else. The UX makes me want to gouge my eyes out with a dirty spoon, but it's still worth it, in spite of the UX. I'm baffled why MS/GitHub can't just go ahead and rip it off at this point, to replace its own utterly atrocious PR review workflow.

baybal2 5y ago

Versioned merge requests anyone? This is a solution looking for a (solved) problem.

carlgao 5y ago

This is an absolute game-changer for both writing and reviewing code. Kudos to the Graphite team for bringing this workflow to the world.

reissbaker 5y ago

I've worked at both FB and at smaller companies. The "stacked diff" workflow is quite good, although it can be replicated via Github's PRs via "logical commits." The basic workflow is: 1. Each commit is a Phabricator-style diff. Put the whole idea into a single commit via `git add -p` and rebasing. 2. Multiple ideas/diffs for the same feature? Split them into multiple commits on the same PR. 3. Review the PR commit-by-commit: you can click on the links in Github to the individual commits and see+comment on the changes there. Comments on commits automatically propagate back to the PR FWIW, so this works pretty well. The mental model becomes: the PR is the overview of the full feature, but not how reviewers are expected to review. Reviewers review commits. It works pretty well! Assuming you have buy-in from your team to operate this way. In fact, I actually somewhat prefer it: with "stacked diffs," it can be harder to get the birds-eye view of the entire feature's set of changes. Git and Github are very flexible, so in practice I find you do need some established workflows that everyone agrees to follow, or else it's chaos.

s4i 5y ago
But, when using GitHub, isn't the missing puzzle piece here (which Graphite and other "stacked review tools" mentioned here attempt to solve) that you then cannot just merge those individual PRs one by one – you either have to merge the whole PR or nothing? Which leads us to the second option: to use multiple dependent PRs, which causes its own issues explained in other comments.
- tumetab1 5y ago
  I think his/her perspective, and mine, is that we don't want to merge individual commits. My personal feeling is that I need to review each commit and then I need to review the whole PR (list of commits). If both are good then the PR can be merged but each individual commit is not OK to merge unless it's part of a PR.
- Liskni_si 5y ago
  > you either have to merge the whole PR or nothing That's not true, though. You absolutely can merge a single commit, via git push, there's just no button in the GitHub UI for it.

mkasberg 5y ago

I proposed a feature along these lines as GitHub feedback. Would love to see more upvotes there if you support the idea! https://github.com/github/feedback/discussions/6125 https://github.com/github/feedback/discussions/6125

taigereye 5y ago

Leveling the playing field a bit more between Google/FB and other companies one diff at a time (:

taigereye 5y ago

Leveling the playing field one diff at a time (:

emodendroket 5y ago

> The blocker here isn’t Git. In fact, the reason you can’t go with option #4 is that your code review tools probably don’t support it: No, git is not the blocker... but neither are the tools. The big blocker is a lack of confidence that someone won't ask me to drastically rework my PR.

anon9001 5y ago
> The big blocker is a lack of confidence that someone won't ask me to drastically rework my PR. Another big blocker is the confidence to delicately review a PR and demand it to be drastically reworked. Actually, I think they're the same problem. When your livelihood is tied to other humans, you generally want to minimize conflict.
- dboreham 5y ago
  There are also control freak psychopaths in our midst who just love telling people what to do and exercising control. They don't really know anything about producing quality software, but they sure know how to tell people they're wrong.
- emodendroket 5y ago
  Not really the same thing. The point is that if someone wants you to totally rework the base PR then the follow-on work will need to be redone.

jeblair 5y ago

Zuul is an open-source CI/CD/project-gating-system that implements this for Github, GitLab, Gerrit, and pagure. It actually lets you stack dependencies between any of those systems. Just say "Depends-On: <url>" in the PR. See https://zuul-ci.org/ https://zuul-ci.org/ and https://zuul-ci.org/docs/zuul/discussion/gating.html#cross-project-dependencies https://zuul-ci.org/docs/zuul/discussion/gating.html#cross-p...

tomasreimers 5y ago
This is awesome! We should add support for this to the CLI, I took a note
delsarto 5y ago
As the parent poster well knows (as they invented it) the trick here is that you write your jobs to install from a checkout the CI system does for you. https://opendev.org/opendev/system-config/src/commit/0a279742fc1bbf1a5ceed6afbbe5ab9647bcbc3c/zuul.d/system-config-run.yaml#L73 https://opendev.org/opendev/system-config/src/commit/0a27974... is really nice practical example. What it does isn't even really that important; but it runs a simulation of deploying production code in OpenDev. This is a "devel" job, we deliberately test any changes against all the latest HEADs of the master/main/development branches of projects we use. This is a non-voting job -- a notice that what you're introducing might be fine now, but there is trouble brewing when our dependencies release their next version. Sometimes that's fine and a known issue, or upstream is broken, and other times it's something totally unique and needs to be fixed (this is why, despite AI being able to write code for you, so far the implications of using that code still need a human in the loop :) The "required-projects" in the job definition tells Zuul what repositories this test needs. You can clearly see how it handles various projects having "devel", "main" or "master" branches to pull from to get their latest versions. In the "vars" section you can see we're setting variables that get passed to the job roles flagging "this is the devel job, don't install the latest release but use the source Zuul will checkout for you from here". The amazing thing? If I make a change and this job fails, I may debug it and find that it wasn't actually my fault, but something in upstream Ansible committed recently. I can propose the fix upstream. They do all their CI, and that's fine. But I now put in my change comment Depends-On: https://github.com/ansible/ansible/pull/1234 https://github.com/ansible/ansible/pull/1234 and magically Zuul will recognise that I want to apply that pull request to the Ansible tree in testing and set it up for me (as noted, Zuul can do this for all sorts of systems, not just github). Additionally, Zuul will not merge the change until the dependency is satisfied -- I can NOT commit broken code!

dvirsky 5y ago

While the stacked code review process at Google is pretty smooth - if you're using the default version control system, keeping your chain in sync is a nightmare. If C depends on B depends on A and you made a modification in A, there's no automatic way to rebase anything and you have to start patching B then C manually. It's much much easier with the internal version of Mercurial, but the default VCS doesn't support this method.

omegalulw 5y ago

I don't agree with the premise. There's option 4, start off a new branch for the new feature, make that a new PR. That solves most problems, you can push that new PR for review as soon as your earlier PR is merged in which will allow you to rebase on master.

ak217 5y ago
That does not work if your second PR builds upon the first.

ickyforce 5y ago

I don't like the fact that I need to grant read/write access to all public and private repositories to use the dashboard or create PRs. I know that github has no decent solution to limit access scope to 1 repository but this is the reason why I haven't tested graphite on any real work. I guess that the only sane way would be to create a separate github user which only has access to a single repo where I'd like to use/test graphite. At the same time I'd probably be ok with the CLI having wider access, as long as the website does not have it. I can control/monitor CLI to some degree but not the web part.

systematical 5y ago

I'm confused. The solution seems easy. I have PR #80 in review and my next task depends on it. So I branch off of #80 and just starting working on #94. If there is a holdup on #80 and I want feedback on #94 I just create a draft PR with a note about #80. When #80 gets approved then I'll merge it and merge those changes into PR #94. If it gets messy, I'll just torch #94 and use my IDEs local history and do it manually. What exactly do stacked changes do?

stevewodil 5y ago
Right but your diff for #94 will include all of the changes in #80 as well, because it's being compared to the main branch. When you do this a few layers up your diff is suddenly massive and hard to navigate for review. Also if you need to change anything about #80 you have to rebase #94. Which also means the same for any other branches that you or someone else created on top of #94 that depended on #80 landing
- jimmaswell 5y ago
  Make feature branch off main, push, make a PR, checkout main again, make a different feature branch with a different PR, repeat. Why would I continue using the other feature branch for the 2nd?
  - stevewodil 5y ago
    Because if the second feature requires the code changes from the first PR you can't do that... If your team doesn't have a use for stacked diffs that's fine. Teams of one also don't use pull requests and might not understand why we need those.
    - jimmaswell 5y ago
      It's very rarely come up for me that multiple open issues depend on each other. Maybe it happens more in some codebases than others.

algorist 5y ago

this is a game-changer. such a PITA to manage cascading and compounding feature branches. can't wait to give this a try!

amir-h 5y ago

GitHub allows stacking PRs you send PR1 to master and PR2 is a pull request to PR1's branch, the diffs look good, and when PR1 is merged GitHub automatically changes its target branch to master.

wutbrodo 5y ago
Yea, I don't quite understand how the article's product is different from this. I also don't understand how anyone uses Github in a professional context without using the workflow you describe... EDIT: Ah, from comments further downthread, it seems that the level of stacking enabled is much deeper, as a lot of the manual process is automated. I stopped using Critique over half a decade ago so I no longer remember the details of how it handled this.

coderdd 5y ago

I worked at Google and have no idea what they mean by stacked change. You could push multiple changes to a single CL, but those were rarely reviewed independently. Maybe they mean keeping a stack of CLs ready, while waiting for a review? I often had 1-2 CLs based on.. rebasing was fun sometimes. Or they mean diffbasing these subsequent CLs, for an early review? That was useful, but due to volatility dependent on the first review, a bit risky. Plug for a code review project: https://reviewpad.com/ https://reviewpad.com/

exclipy 5y ago
Stacked diffs is Facebook terminology for diffbased CLs (Google). Except Facebook has invested a lot of tooling into it to make it possible without many of the downsides (see the comments here about 10+ diffs stacked on top of each other).

xunn0026 5y ago

This is just an ad for the startup. It literally doesn't tell you want the thing does. But FB / Google uses it so it must be perfect.

jackhkmatthews 5y ago

Excited for this. I've used Gerrit in the past and really miss it.

penjelly 5y ago

im an idiot but can someone help me understand the following questions? - Doesn’t this mean graphite can access non-public codebases of large organizations that authorize it? - What prevents graphite or its employees from stealing intellectual property or maliciously viewing code for their own purposes? - are there risks in allowing graphite access to codebases in any way? - any and all information regarding privacy with regards to code are welcome, i simply cannot give access to certain codebases to third party organizations without some proof that the data is not going to be maliciously used or accessed. all this comes into question when simply trying to login to graphite website.

throwawaymanbot 5y ago

Unblocked?? Is this a colon-ease advert?