[HN Gopher] Stacked changes: how FB and Google engineers stay un...
___________________________________________________________________
Stacked changes: how FB and Google engineers stay unblocked and
ship faster
Author : tomasreimers
Score : 232 points
Date : 2021-11-17 16:31 UTC (6 hours ago)
(HTM) web link (graphite.dev)
(TXT) w3m dump (graphite.dev)
| alexkern wrote:
| 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.
| epage wrote:
| For anyone interested, I've been collecting notes on various
| tools in this space:
|
| https://github.com/epage/git-stack/blob/main/docs/comparison...
|
| (granted the page doesn't mention git-stack since that is
| assumed)
| vander_elst wrote:
| Doesn't Gerrit already allow this?
| RHSeeger wrote:
| > 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 wrote:
| I still haven't found a reliable way to prevent merging
| branches that aren't the first in the stack :/
| gregmfoster wrote:
| (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 :)
| tomasreimers wrote:
| How do you deal with rebases when you have to?
| sigil wrote:
| 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 wrote:
| 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 :)
| sigil wrote:
| 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.
| didibus wrote:
| 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.
| gregmfoster wrote:
| Google had a good paper on the benefits of smaller changes:
| 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 wrote:
| 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 wrote:
| > 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.
| gregmfoster wrote:
| The docs for the CLI and dashboard go into some more depth -
| might be what you're looking for?
|
| - 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.
| KptMarchewa wrote:
| >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.
| dv_dt wrote:
| As far as I've used it, git rebases ignore duplicate commits.
| KptMarchewa wrote:
| They are not duplicate if you change something (like, using
| amend) in one of the preceding branches.
| sp332 wrote:
| 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.
| tsss wrote:
| As long as you review the 'comments' branch first, merge it,
| then move on to reviewing the 'reactions' branch it should be
| fine.
| tomasreimers wrote:
| 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
|
| 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?
| skipants wrote:
| > 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?
| SOLAR_FIELDS wrote:
| 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.
| skrtskrt wrote:
| 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
| notpachet wrote:
| 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.
| skrtskrt wrote:
| 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?
| joshuamorton wrote:
| You revert C, because that's what caused the issue. Then
| you investigate.
| KptMarchewa wrote:
| 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.
| gkop wrote:
| 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!
| tomasreimers wrote:
| Oh hey, I'm sorry! :(
|
| 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.
| jacobr1 wrote:
| Ironically, this reply is also an apology!
|
| No judgment, I just found it funny.
| gkop wrote:
| No worries! My intention was just helping you all look
| your best -- which you are doing with this friendly
| reply.
| kayson wrote:
| 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
| gkop wrote:
| Thanks. I can see that I could have worded my feedback
| more tactfully, and will do so in the future.
| gregmfoster wrote:
| Thank you for the feedback :)
| dahfizz wrote:
| 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.
| secondcoming wrote:
| > 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 wrote:
| 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 wrote:
| 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?
| Jensson wrote:
| There are two features you want to add, called "comments"
| and "reactions". Those aren't PR lingo, they could be named
| "Feature X" and "Feature Y".
| tomasreimers wrote:
| 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/
|
| and
|
| https://kurtisnusbaum.medium.com/stacked-diffs-keeping-
| phabr...
| secondcoming wrote:
| Those were useful, thanks!
| joshuamorton wrote:
| 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.
| taylorqcarol wrote:
| Really exciting - looking forward to implementing this with my
| team!
| taylorqcarol wrote:
| When can we expect to get off the waitlist??
| amedway wrote:
| 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!
| gregmfoster wrote:
| That pain was what inspired us to create Graphite as well :)
| Thank you
| davidw wrote:
| 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.
| atom_arranger wrote:
| This exists: https://pullpanda.com/
| svachalek wrote:
| 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..."
| Nextgrid wrote:
| 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.
| davidw wrote:
| 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.
| the_duke wrote:
| In theory time should be split into 30% research and
| planning, 30% code reviews and 30% writing code.
|
| In practice most devs spend 90% coding and 10% hastily
| rushing through reviews.
| prerok wrote:
| 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.
| ericbarrett wrote:
| 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 wrote:
| 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.
| toast0 wrote:
| 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.
| davidw wrote:
| 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.
| jacobr1 wrote:
| 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.
| ericbarrett wrote:
| 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.
| alexjplant wrote:
| 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.
| prerok wrote:
| 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 :)
| dlvhdr wrote:
| This is exactly what's missing in Github. Amazing job!
| slaymaker1907 wrote:
| 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.
| jpochtar wrote:
| 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 wrote:
| how do you typically stack diffs?
| hardwaregeek wrote:
| 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.
| KptMarchewa wrote:
| I amend my commits. My PRs are always exactly one commit, so
| this problem does not exist.
| gregmfoster wrote:
| This is exactly the problem that graphite-cli solves
| (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/...
|
| 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
| hardwaregeek wrote:
| Sounds great! I signed up for the waitlist. I'm always for
| ways to make version control more ergonomic and feature full
| tex0 wrote:
| Sooo, like Gerrit?
| tazjin wrote:
| 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 wrote:
| 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.
| dathinab wrote:
| 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.
| errcorrectcode wrote:
| 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
| gregmfoster wrote:
| 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` :)
| michlim wrote:
| congrats on the launch! love the ui on the dashboard!
| gregmfoster wrote:
| Thank you! All thanks to Xiulung Choy's hard work
| [deleted]
| epage wrote:
| 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.
| tomasreimers wrote:
| Totally totally totally agree. I think a lot of the slow down
| comes from not understanding when the ball is in your court.
| And Github doesn't prioritize this b/c I think this problem is
| less prevalent in open source, where there is usually no
| expectation of a reasonable time until review.
|
| A lot of what you described is exactly what we've built / we're
| planning for the code review portion of the site - you should
| sign up and check it out :)
| zdw wrote:
| Gerrit does many of the "ideal solution" items, as it
| replicates the internal Google workflow.
|
| It's a code review system with browsing as an afterthought, as
| opposed to GitHub which is a code browser with review as an
| afterthought.
| bialpio wrote:
| Gerrit is nice, but to me feels a bit dated, UX-wise. Before
| joining Google, I used Azure DevOps and I feel that it was
| easier to review code over there (less jumping around the
| page). At first I thought it's a matter of (in)familiarity
| with the tool, but I still feel like I'm fighting with it
| after 3 years of use...
| londons_explore wrote:
| 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.
| willyxiao1 wrote:
| 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!
| trhway wrote:
| 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.
| zurawiki wrote:
| > 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.
| xapata wrote:
| 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.
| baby wrote:
| 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).
| Denvercoder9 wrote:
| Gitlab got this right, Merge Requests are versioned and you can
| look at the differences between versions of a MR.
|
| It's somewhat amazing and shocking how little the review
| experience on GitHub has improved over the last 10 years.
| [deleted]
| gibolt wrote:
| 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.
| gregmfoster wrote:
| 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/...
| zaven wrote:
| 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 wrote:
| 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!
| londons_explore wrote:
| Not quite sure how a blog post about a new workflow for code
| review failed to have any screenshots or infographics showing
| said workflow...
| jrochkind1 wrote:
| 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 wrote:
| 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 wrote:
| 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.
| mlovett wrote:
| Love to see tech devoted to improving the lives of developers.
| This has been a pain point for me for so long.
| grozensmith wrote:
| 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).
| tydalwave wrote:
| 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!!
| gregmfoster wrote:
| Thank you for the kind words!
| lingrace16 wrote:
| 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
| vp8989 wrote:
| 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/
| exitheone wrote:
| 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.
| bagels wrote:
| Stacked diffs are great. I can now simultaneously wait for many
| code reviews instead of just one.
| yasyfm wrote:
| This is super cool! Any plans to build out more PR features? I'd
| love to see a Linear-for-pull-requests product.
| dkhenry wrote:
| 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
| nawgz wrote:
| "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...
| ayushsood wrote:
| 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
| kvm wrote:
| 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!
| zchauvin wrote:
| 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!
| tsvita wrote:
| Such an exciting launch!
| stephenturban17 wrote:
| 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 wrote:
| Ugh. I'm annoyed when homegrown is the first reaction
| (something else to support, fewer people to fix it), rather
| than community &&/|| industry collab.
| luke_heine wrote:
| Super cool product - defo know this problem
| gmaster1440 wrote:
| Worth noting that this is coming to GitHub:
| https://github.blog/changelog/2021-10-27-pull-request-merge-...
| tomasreimers wrote:
| 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 wrote:
| 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.
| gregmfoster wrote:
| 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!
| strogonoff wrote:
| 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
| gregmfoster wrote:
| Some of the other blog posts and docs have some good deeper
| info:
|
| - 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...
| sond813 wrote:
| 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 wrote:
| 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.
| corndoge wrote:
| 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
| stephenturban17 wrote:
| 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.
| password4321 wrote:
| What is the difference between stacked changes and branchless?
|
| https://github.com/arxanas/git-branchless
|
| _based off of the branchless Mercurial workflows at large
| companies such as Google and Facebook_
| tomasreimers wrote:
| A bunch of small things, but the biggest change is we also have
| a website to help with the authoring/review of PRs experience
| :)
| brundolf wrote:
| 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.
| gregmfoster wrote:
| 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). 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, with docs here
| 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 wrote:
| > 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.
| barbazoo wrote:
| 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.
| Denvercoder9 wrote:
| One problem with GitHub is that you can't do this with branches
| from a fork.
| hobofan wrote:
| Can you be more specific what you think can't be done?
|
| 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).
| Denvercoder9 wrote:
| 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.
| gregmfoster wrote:
| 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 wrote:
| >(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 wrote:
| 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).
| treis wrote:
| So are the 10 branches mostly yours then? That seems
| slightly less crazy.
| milkytron wrote:
| Yeah this shouldn't be an issue when following standard git
| branching practices. It's kinda the point
| havafitz wrote:
| 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 wrote:
| 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!
| chillee wrote:
| How is this different from 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 wrote:
| 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 wrote:
| Cool, thanks!
|
| Yeah, stack-based PRs are awesome - excited to perhaps try it
| out.
| yosito wrote:
| 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?
| gregmfoster wrote:
| Great question! It does lots of things. The 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...
___________________________________________________________________
(page generated 2021-11-17 23:00 UTC)