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.
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.
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 :)
Nice, didn't know about `-`. What's your process once you're in the interactive rebase? The `git rebase --onto` I use is noninteractive and could be an alias, were it not for the business of finding the `branchpoint` commit.
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.
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?
Since I don't use git much these days, I'm going to speak in generalities.
I would guess the assumption is that you can't have reactions without comments. So comments v1 is "comments" and v2 is "reactions".
They need to be different PRs, so I guess that's what leads to different branches. But they can be reviewed independently, you just have to merge one before the other.
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.
Hmm it could work like those LinkedIn emails. Automatically send after two days, "Hey I know you're busy, aren't we all? And the last thing I want to do is give you more code to review. But you really should check out..."
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.
There's just a lot going on with them. The way they pop up and you can choose to either wait or interrupt your own work to process one. If everyone waits, then the person needing the PR gets hassled by their manager that they need to "go out and get it", which means more aggressively hassling other people, which is stressful and unpleasant for everyone.
There's some kind of game theory dynamic going on where I don't think the outcome is really ideal, even if most people involved are trying to make it work.
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.
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.
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.
Exactly - and waiting for that review leaves you in this holding pattern where you'd really like to be moving the ticket along, but you kinda sorta have time to do something else.
Also, if you are building upon the code from the prior review, and you need to shuffle things around based on feedback from the review, then your more recent work also must be redone, resulting in waste.
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.
Right, each one of us needs to finish the work before the deadline. I really don't like how we perpetuate assignments to individuals. I suppose it stems from our education system and ticket assignments but I would really like to see more teamwork built into our daily workflow.
Some people assign timeslots for reviews, but when deadline is pressing, all the timeslots go away.
The only solution that I have seen, and am happy to be a part of currently, is to have a close coworking team that's responsive. In short, it's not the tooling, it's the people.
We've actually started doing this, though it's working around the fact that I couldn't figure out how an author can send a PR back to a reviewer for another round of review so that it shows up in a queue nicely.
It's really nice. We have a kanban column 'Needs Review' and I've dumped tasks there for PRs, documents, and all kinds of other stuff.
A few jobs ago I put a PR in a few days into our two-week sprint. Our team of purportedly full-stack developers was actually a team of two front-end and seven back-end developers which meant that the Jira tasks were written such that tasks in the same sprint were all interdependent on one another. This always resulted in long cycle times and a mad rush to get everything merged the day before sprint review. This was a bit painful as our team lead was obsessed with Git commit history and required branches to be rebased onto the main branch, so every PR that got merged ahead of yours meant that you had to immediately jump in and rebase yours to make it eligible for review again.
The PR that I put in before everybody else's was summarily ignored because everybody was busy getting their own stuff in. I spent cram day rebasing my PR at least a half-dozen times as commits flew into the main branch. A few minutes before cut-off I asked in a team call if I could get a review; my team lead told me that there was no time and that it would have to wait.
Needless to say I've since found a team that does the opposite (reviews each others' code in a timely and constructive manner). I really enjoy working with people that care about delivering value and helping others... it's vastly improved my job satisfaction.
Agreed. In my opinion, just goes for show that you need a good team organization/culture more than another tool.
While I'm sure a lot of people will find this tool useful, I believe it will also help disfunctional teams last longer, which just means that the real problems (like, why people don't review code in timely fashion) will take longer to resolve.
As always, just my two cents based on my experience :)
> 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.
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.
> 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.
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.
One thing Phabricator did/does worse than GitHub is the interface for browsing the individual commits that make up a PR. I like being able to tell a story with the sequence of commits and their messages.
I honestly don't feel like Github did a good job there either, I mean at least on the web UI because the VSCode Github extension helps a lot (when it's not buggy).
Gerrit is the only thing I've seen do change exploration well.
Github is annoying as crap. Can't even comment on a line in the file that needs changing but was missed in the PR, can only comment on a tual changes.
Phab is god awful, it's like someone built that whole system with a purist mindset, everything is perfectly designed with perfect referential IDs. But every screen is a view into the db table, no screens join information and make it usable. This is worse when you have n:m relationships because they expose those tables via screens too, it's just plain awful.
> Phab is god awful, it's like someone built that whole system with a purist mindset, everything is perfectly designed with perfect referential IDs. But every screen is a view into the db table, no screens join information and make it usable. This is worse when you have n:m relationships because they expose those tables via screens too, it's just plain awful.
I actually like Phabricator, but the DB schema is insane. Like, literally insane. I spent hours trying to figure it out once before giving up in disgust.
Honestly what i want is for github to purchase https://reviewable.io/ and make it native.
Maybe gate sone of the more advanced features behind an opt-in so beginners are not too overwhelmed (and because you probably don’t need them on smaller projects), but the current state of things is just appalling, and more importantly has not at all kept up with workflows on large projects or long / complicated PRs.
The complete lack of support for reviewing over force pushes is insane.
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.
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.
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.
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.
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 :)
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).
Glue code is pretty easily split up into a stack. 90% of what I’ve worked on has been glue code.
To ship your change you need to update what’s there already. Tool A needs an integration point. Tool B needs a hook added. Tool C finally joins A to B.
Implementing the bits for A and B found two useful refactorings in each codebase. At the end of it all there was also some dead code.
That’s 8 commits. The downside is you end up yak shaving a lot.
You can't open a PR in the parent repository against a branch in the fork. You can create that PR inside the forked repository, but that doesn't allow the maintainers of the parent repository to do anything with it. So if you've got two stacked branch in a fork, you can't open two PRs for them in the manner GP described.
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.
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.
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.
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!
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.
> 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 :)
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.
> Previous studies have found that the number of
useful comments decreases [11, 14] and the review latency
increases [8, 24] as the size of the change increases. Size also
influences developers’ perception of the code review process; a
survey of Mozilla contributors found that developers feel that
size-related factors have the greatest effect on review latency
[26]
I'll have to dig into those. It makes sense that it decreases latency, but I'm curious how they assessed useful comments.
That said, I'm not saying to make massive code reviews, because reviewers are lazy, they won't want to spend more than an hour to review no matter the size of it. But when the reviewed code is made on top of code that itself has not been reviewed, the quality of the review suffers in my opinion.
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.
> 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.
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?
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!
That sincerely was not my intention, and a lot of context and playfulness can get lost in comments. For what it's worth, I was simply trying to make a joke about why our marketing materials may not be as good as they should be and continue the conversation. I apologize if that came off as defensive and will make sure I double check my future comments so that they don't come off in the same light.
That did not come across as defensive at all to me. It was pretty clearly tongue in cheek. Your comment, in fact, comes across as judgmental and condescending
> When was the last time you made a stcak of changes that was over 30 branches tall?
Is this a thing? I think the highest stack I've ever had was 2. I suppose it could depend on company culture/tech stack/code organization (monorepo vs. services).
> 2000-4000ln pr at least once (which could have been 30 100ln PRs).
I can't say I have. Maybe deleting a lot of code where it's very obvious what is being deleted and why. What sort of work takes 2000 lines that can't be broken down into smaller, incremental pieces?
I’m working on having a large scale open source project support a new SQL database (Postgres) and the migration work is about 2500 lines currently since I have to touch basically every part of the project. I don’t want to do the migration piecemeal since turning on the Postgres unit tests for only sections of the code base and keeping track of that and worrying about all the dependencies of code changing as other people touch the project is a lot more overhead than doing it all waterfall style.
Now, I will say that these type of things are not everyday kind of PR’s, but they have certainly happened several times over the course of my career. Probably once or twice a year if I had to take a rough guess.
You can break down a big feature into smaller bits of code, but what's the point of releasing a some new code that's not being called by anything yet, when you might change it as you converge on the final solution?
What's the point of a PR that's just an HTTP handler + adapter that's not exposed via an endpoint yet, and might change once we implement related the domain layer methods? What kind of context will a reviewer have on whether this is a decent handler + adapter when there is no other context about which methods the handler will call and what data will be returned?
If we release the domain layer first, we may want to adjust the domain data model in order to better serve the application layer, or work better with the infrastructure/datastore layer.
And so on.
IMO the PRs need to be big enough to encompass an atomic, complete unit of functionality or else it's basically impossible to tell if the approach and code is any good.
"It doesn't break anything" is not good enough, you can still create tech debt by releasing code that may need to be changed to complete the feature. Or someone will just implement the rest of the feature in a worse way in order to integrate with the non-ideal code that you already released when you didn't have a good enough view of the final solution
It doesn't have to be an either/or. The method that I recommend to my reports is to develop the feature in a large WIP PR branch, get some rough reviews on the macro architectural choices in that PR, and then once you have a greenlight on the larger approach, break it out into smaller PR's that each represent a smaller change necessary to effect the feature.
There are definite benefits to shipping less code to production in one atomic deploy. When something breaks (and it will), you want to wade through as little code as possible while things are burning in order to identify what went wrong. Sure, you could just revert everything that went out with the last deploy, but sometimes things aren't that straightforward. Say when there are other sibling changes that are not easily roll-backable that you'd have to clobber to do a pure rollback. In these scenarios, it may be safer (albeit slower) to revert your individual change and deploy the revert by itself.
I get what you mean, but rolling out in chunks can make it even harder to pinpoint and revert issues.
If you roll out chunks A, B, then C, and things broke after C, the issue can still be in A, it just doesn't get exposed until it is accessed in a certain way by C.
How quickly can you determine which one(s) to revert?
The point is that others see that it's there, they can start to take it into account when they develop their solutions.
Of course if there are problems with people depending on not yet final code that seems more of a higher level communication issue than git workflow issue.
Again, if atomic unit of functionality needs huge towers of stacked PRs that indicates that they are not really atomic and the design phase (communication) seems to be missing.
After all the reviewer should know about the design, the HTTP handler + adapter should not be a mystery to the reviewer. And so on...
...
Now, that said, sometimes looking at a big PR as whole takes up less review time than in daily/weekly increments. But if the reviewer only sees the thing late in the dev process plus the design is not agreed upon then that means the developer writes a lot of code that might go straight to /dev/null when the reviewer disagrees with the fundamental design of it.
The assumptions needed for these huge dangling WIP PRs to make sense seem very strange (risky and suboptimal) :o
I think you have an useful feature, but needing to get someone else answer me what it is in comment (https://news.ycombinator.com/item?id=29256481) isn't a good way to explain the product.
Regarding your questions, the most I've done was 4, usually at most 2. My PRs are single commits that I amend and force-with-lease after getting useful code review feedback. I don't think rebasing one after another is particularly annoying, but I think not having to do it is a nice feature. I wonder whether it's a product tho. I wish you success anyway.
One of the reasons you cant use a simple rebase --onto is that you dont want to accidentally copy all downstack commits between the merge base and what you're restacking onto. The CLI tracks branch bases commits through git refs to ensure that restacking never duplicates commits.
yes that’s a possibility too, but then the typical merge resolution rules should apply. If there are common areas modified that is going to incur a manual review. There’s no magic automation I’d really entrust to fixing that.
>Re `git rebase --onto`, the open source CLI offers a recursive implementation to prevent you from having to carefully rebase each branch in your stack
Thanks - you've answered what I've been looking for and I'd prefer that this information was in the marketing copy.
I agree that this would be an useful feature. I don't think this is a product though - I don't see myself doing more than 3 or 4 stacked diffs when this functionality would really shine.
Merging a branch into another branch is fine, but then you have to merge that other branch up into main. And that shows effectively two branches' worth of diffs, which really muddles things if some of the changes have already been reviewed.
I second that. It's a bit surprising to me that teams could have such big PRs and branch stacks. I can only say from own perspective but I imagine this kind of nightmare would be our reality now if we didn't pay attention to improve the we work and use PRs along the way.
PRs work very well for us and if we find ourselves stacking them, then we keep an eye on getting them into master (yes, we don't use the 'main' bs) in reasonable order. Then Github is actually doing job for us changing the target branch once the underlaying changes are merged into it.
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.
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.
Ugh. I'm annoyed when homegrown is the first reaction (something else to support, fewer people to fix it), rather than community &&/|| industry collab.
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.
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)
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.
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` :)
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).
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
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!
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!
284 comments
[ 3.9 ms ] story [ 264 ms ] threadI do stacked commits (branches off branches) all the time. Github certainly supports PRs based on such.
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.
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.
You would have a comments branch if you were adding a feature called "comments", I guess.
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?
https://jg.gg/2018/09/29/stacked-diffs-versus-pull-requests/
and
https://kurtisnusbaum.medium.com/stacked-diffs-keeping-phabr...
I would guess the assumption is that you can't have reactions without comments. So comments v1 is "comments" and v2 is "reactions".
They need to be different PRs, so I guess that's what leads to different branches. But they can be reviewed independently, you just have to merge one before the other.
There's some kind of game theory dynamic going on where I don't think the outcome is really ideal, even if most people involved are trying to make it work.
(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.
The longer the cycle, the more contexts to be juggled and the more effort to juggle them.
In practice most devs spend 90% coding and 10% hastily rushing through reviews.
Some people assign timeslots for reviews, but when deadline is pressing, all the timeslots go away.
The only solution that I have seen, and am happy to be a part of currently, is to have a close coworking team that's responsive. In short, it's not the tooling, it's the people.
It's really nice. We have a kanban column 'Needs Review' and I've dumped tasks there for PRs, documents, and all kinds of other stuff.
The PR that I put in before everybody else's was summarily ignored because everybody was busy getting their own stuff in. I spent cram day rebasing my PR at least a half-dozen times as commits flew into the main branch. A few minutes before cut-off I asked in a team call if I could get a review; my team lead told me that there was no time and that it would have to wait.
Needless to say I've since found a team that does the opposite (reviews each others' code in a timely and constructive manner). I really enjoy working with people that care about delivering value and helping others... it's vastly improved my job satisfaction.
While I'm sure a lot of people will find this tool useful, I believe it will also help disfunctional teams last longer, which just means that the real problems (like, why people don't review code in timely fashion) will take longer to resolve.
As always, just my two cents based on my experience :)
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.
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.
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.
Github is annoying as crap. Can't even comment on a line in the file that needs changing but was missed in the PR, can only comment on a tual changes.
Phab is god awful, it's like someone built that whole system with a purist mindset, everything is perfectly designed with perfect referential IDs. But every screen is a view into the db table, no screens join information and make it usable. This is worse when you have n:m relationships because they expose those tables via screens too, it's just plain awful.
I actually like Phabricator, but the DB schema is insane. Like, literally insane. I spent hours trying to figure it out once before giving up in disgust.
It's somewhat amazing and shocking how little the review experience on GitHub has improved over the last 10 years.
Maybe gate sone of the more advanced features behind an opt-in so beginners are not too overwhelmed (and because you probably don’t need them on smaller projects), but the current state of things is just appalling, and more importantly has not at all kept up with workflows on large projects or long / complicated PRs.
The complete lack of support for reviewing over force pushes is insane.
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.
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.
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.
Most of these practices are just inspired by what Phabricator and a few of the big companies are already doing well :)
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?
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).
To ship your change you need to update what’s there already. Tool A needs an integration point. Tool B needs a hook added. Tool C finally joins A to B.
Implementing the bits for A and B found two useful refactorings in each codebase. At the end of it all there was also some dead code.
That’s 8 commits. The downside is you end up yak shaving a lot.
What you're describing is sometimes a problem but often not really one.
I just tried it, and I can without problem open a PR from a fork against an arbitrary branch of an arbitrary fork (or main repository).
Kudos for getting this right!
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.
> 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 :)
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.
If reviews are only "will this break things" I would gadly review their big PRs :D
> Previous studies have found that the number of useful comments decreases [11, 14] and the review latency increases [8, 24] as the size of the change increases. Size also influences developers’ perception of the code review process; a survey of Mozilla contributors found that developers feel that size-related factors have the greatest effect on review latency [26]
I'll have to dig into those. It makes sense that it decreases latency, but I'm curious how they assessed useful comments.
That said, I'm not saying to make massive code reviews, because reviewers are lazy, they won't want to spend more than an hour to review no matter the size of it. But when the reviewed code is made on top of code that itself has not been reviewed, the quality of the review suffers in my opinion.
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.
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
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?
That sincerely was not my intention, and a lot of context and playfulness can get lost in comments. For what it's worth, I was simply trying to make a joke about why our marketing materials may not be as good as they should be and continue the conversation. I apologize if that came off as defensive and will make sure I double check my future comments so that they don't come off in the same light.
No judgment, I just found it funny.
Is this a thing? I think the highest stack I've ever had was 2. I suppose it could depend on company culture/tech stack/code organization (monorepo vs. services).
> 2000-4000ln pr at least once (which could have been 30 100ln PRs).
I can't say I have. Maybe deleting a lot of code where it's very obvious what is being deleted and why. What sort of work takes 2000 lines that can't be broken down into smaller, incremental pieces?
Now, I will say that these type of things are not everyday kind of PR’s, but they have certainly happened several times over the course of my career. Probably once or twice a year if I had to take a rough guess.
What's the point of a PR that's just an HTTP handler + adapter that's not exposed via an endpoint yet, and might change once we implement related the domain layer methods? What kind of context will a reviewer have on whether this is a decent handler + adapter when there is no other context about which methods the handler will call and what data will be returned?
If we release the domain layer first, we may want to adjust the domain data model in order to better serve the application layer, or work better with the infrastructure/datastore layer.
And so on.
IMO the PRs need to be big enough to encompass an atomic, complete unit of functionality or else it's basically impossible to tell if the approach and code is any good.
"It doesn't break anything" is not good enough, you can still create tech debt by releasing code that may need to be changed to complete the feature. Or someone will just implement the rest of the feature in a worse way in order to integrate with the non-ideal code that you already released when you didn't have a good enough view of the final solution
There are definite benefits to shipping less code to production in one atomic deploy. When something breaks (and it will), you want to wade through as little code as possible while things are burning in order to identify what went wrong. Sure, you could just revert everything that went out with the last deploy, but sometimes things aren't that straightforward. Say when there are other sibling changes that are not easily roll-backable that you'd have to clobber to do a pure rollback. In these scenarios, it may be safer (albeit slower) to revert your individual change and deploy the revert by itself.
If you roll out chunks A, B, then C, and things broke after C, the issue can still be in A, it just doesn't get exposed until it is accessed in a certain way by C.
How quickly can you determine which one(s) to revert?
Of course if there are problems with people depending on not yet final code that seems more of a higher level communication issue than git workflow issue.
Again, if atomic unit of functionality needs huge towers of stacked PRs that indicates that they are not really atomic and the design phase (communication) seems to be missing.
After all the reviewer should know about the design, the HTTP handler + adapter should not be a mystery to the reviewer. And so on...
...
Now, that said, sometimes looking at a big PR as whole takes up less review time than in daily/weekly increments. But if the reviewer only sees the thing late in the dev process plus the design is not agreed upon then that means the developer writes a lot of code that might go straight to /dev/null when the reviewer disagrees with the fundamental design of it.
The assumptions needed for these huge dangling WIP PRs to make sense seem very strange (risky and suboptimal) :o
Regarding your questions, the most I've done was 4, usually at most 2. My PRs are single commits that I amend and force-with-lease after getting useful code review feedback. I don't think rebasing one after another is particularly annoying, but I think not having to do it is a nice feature. I wonder whether it's a product tho. I wish you success anyway.
- https://docs.graphite.dev/guides/graphite-cli
- https://docs.graphite.dev/guides/graphite-dashboard
Re `git rebase --onto`, the open source CLI offers a recursive implementation to prevent you from having to carefully rebase each branch in your stack (https://github.com/screenplaydev/graphite-cli/blob/main/src/...)
One of the reasons you cant use a simple rebase --onto is that you dont want to accidentally copy all downstack commits between the merge base and what you're restacking onto. The CLI tracks branch bases commits through git refs to ensure that restacking never duplicates commits.
Thanks - you've answered what I've been looking for and I'd prefer that this information was in the marketing copy.
I agree that this would be an useful feature. I don't think this is a product though - I don't see myself doing more than 3 or 4 stacked diffs when this functionality would really shine.
PRs work very well for us and if we find ourselves stacking them, then we keep an eye on getting them into master (yes, we don't use the 'main' bs) in reasonable order. Then Github is actually doing job for us changing the target branch once the underlaying changes are merged into it.
If the stacked change gets reviewed first for whatever reason, the diff is displayed correctly.
https://github.com/arxanas/git-branchless
based off of the branchless Mercurial workflows at large companies such as Google and Facebook
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)
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.
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
As far as the name, at least the CLI command is clean/similar to git - `gt` :)
Really nice!
Kids these days are getting luxuries we never knew back in the day (in 2015 lol).
See previous HN discussion for more background and substance: https://news.ycombinator.com/item?id=26922633
- CLI docs: https://docs.graphite.dev/guides/graphite-cli
- Dashboard docs: https://docs.graphite.dev/guides/graphite-dashboard
- Storing stack metadata in refs: https://graphite.dev/blog/post/y6ysWaplagKc8YEFzYfr
- Tracing stacks of branches: https://graphite.dev/blog/post/hGn1nt8nFr5cMhvFJs87
- Lengthy conversations in the community slack :) https://join.slack.com/t/graphite-community/shared_invite/zt...