Hacker News Board Hacker News, browsed like a classic message board
Live data from Hacker News

In Praise of Stacked PRs

benjamincongdon.me

Post reply on HN
Print view View original

131–140 of 230 posts

Re: In Praise of Stacked PRs

#131

Post by wizofaus »

nightpool wrote: post #121
wizofaus wrote: post #102

Earlier quoted context omitted.

In one of my first jobs, code reviews were done exactly like this, with the the reviewer (my boss) and I sitting together at one computer going through the changes. It definitely has benefits but it's still important to ensure recommendations/concerns etc. get written down and doesn't necessarily help if there's a need to go away and make significant changes based on the outcomes of discussions. But while the back &…

> and doesn't necessarily help if there's a need to go away and make significant changes based on the outcomes of discussions. See, I disagree, because this is absolutely the place where it helps the most—you can now go away and keep working on the same task without having to context switch to anything else or remember where you were or what you were working on. So you're never in a state where you're blocked and can…

But if you're expecting another dev to always be available to help you with a review, then they're the ones having to "context-switch", interrupting their own work. The reality is context-switching is something that comes with the territory if you're working as part of a team developing software. Which isn't to say there aren't opportunities to minimise the disruption it causes, but the idea that you can more or less eliminate it seems like wishful thinking. Perhaps there's a case for minimising it for more junior devs that aren't as capable of dealing with it, though it's equally possible younger brains are better at it!

Re: In Praise of Stacked PRs

#132

Post by arxanas »

Sol- wrote: post #128

We have some people in our company that use tools like Graphite to implement this pattern on top of Github and I don't know if I'm a big fan. Maybe just a matter of developer discipline, but in my experience people tend to create large stacks of 3+ PRs that then take a while to resolve. Yeah sure, without these tools the code would also exist somewhere, but at least you don't have your pull request list full with PRs…

> Probably some arcane git magic to (interactively) rebase branch

There is not really a command for that yet, short of adding a bunch of `exec` steps to your interactive rebase manually. See https://news.ycombinator.com/item?id=32217204 for an upcoming command.

You might enjoy using https://github.com/gitext-rs/git-stack, which specifically tries to let you manage stacked branches locally while not exposing tons of PRs to your coworkers.

git-branchless itself also lets you manage stacked branches in various ways. For example, you can do `git checkout `, `git commit --amend`, and then `git restack` to rebase all the descendant branches sensibly. You can use it on the local side of things only and then use Github PRs as normal.

Re: In Praise of Stacked PRs

#133

Post by morelisp »

arxanas wrote: post #130
morelisp wrote: post #127

Earlier quoted context omitted.

> There is such a thing in that there is no branch involved in the detached HEAD state. But at the very least there's still an existing remote branch, which is the ultimate merge target, for example - perhaps even multiple. Since we're talking about coordination, there's also the actual state at the remote vs. my view of the remote vs. my teammates' views of the remote. But we don't think about this too much, because…

Which Git client do you use, and also, how do you remember all the remote branch names? > And even the "long version" is pretty negligible (like, maybe 10 seconds per branch?) I notice that mainly the differences in opinion with regards to workflow is disagreement about "how long is too long" for various operations :)

Magit, and the remote branch names are visible in the log view and easy to enter via tab completion. (We're still talking about stacked PRs right? Median stacked at any given time is probably 2 and p90 is maybe 4? Max I've ever seen is ~10 and that was quite a fucked up situation for plenty of other reasons.)

10 seconds would certainly be too long for me. It is obviously not too long for them, or they'd learn some keyboard shortcuts to get the easy 5 out of the way to begin with. But regardless, it's all pretty much dwarfed by things that need actual brainpower like re-reading my commit messages for grammatical errors / broken links / etc.

Re: In Praise of Stacked PRs

#134

Post by arxanas »

morelisp wrote: post #133
arxanas wrote: post #130

Earlier quoted context omitted.

Which Git client do you use, and also, how do you remember all the remote branch names? > And even the "long version" is pretty negligible (like, maybe 10 seconds per branch?) I notice that mainly the differences in opinion with regards to workflow is disagreement about "how long is too long" for various operations :)

Magit, and the remote branch names are visible in the log view and easy to enter via tab completion. (We're still talking about stacked PRs right? Median stacked at any given time is probably 2 and p90 is maybe 4? Max I've ever seen is ~10 and that was quite a fucked up situation for plenty of other reasons.) 10 seconds would certainly be too long for me. It is obviously not too long for them, or they'd learn some ke…

I see, thanks for the information. I often stack 10+ commits, so I don't think your workflow would be reasonable for me. (For example, here, I have 11 finished commits and 3 in progress: https://github.com/arxanas/git-branchless/pull/451 — but I don't actually use stacked PRs on Github, particularly if no one is reviewing my code. Most of my stacking is at work with Phabricator anyways, which handles this better.)

Re: In Praise of Stacked PRs

#135

Post by dcow »

johnfn wrote: post #35
dcow wrote: post #24

Earlier quoted context omitted.

I’m not talking about the author, specifically or criticizing them directly. I’m lamenting how often I encounter people putting intense amounts of effort into arguing about how git should be used. My hypothesis is that if people understood their tools better before getting all evangelical about forcing their entire eng team to work in their style, others wouldn't have to spend so much time trying to convince them tha…

I'm not understanding how stacking PRs is inconsistent with knowing git well.

That’s not what I’m saying at all. My comment is a meta comment about why we even need to write grand defenses of these practices in the first place. I’ve been “stacking PRs” forever… when it makes sense to do so. There’s no golden git usage pattern. I’ve seen the industry all the rave about gitflow. And then it wasn’t cool anymore with tags being the preference. Now it’s cool to just forgo branch names entirely? I’ve watched teams fight over whether every PR should be exactly one commit because they don’t realize that you don't always have to squash merge if you want two unrelated changes pulled in at the same time—just keep the commits clean and revert doesn't care. My point is that if you know how to use the tool none of this really matters and all seems quite distracting. I no problem with people sharing what works for them, that’s great (as is this article, abstractly).

Re: In Praise of Stacked PRs

#136

Post by morelisp »

arxanas wrote: post #134
morelisp wrote: post #133

Earlier quoted context omitted.

Magit, and the remote branch names are visible in the log view and easy to enter via tab completion. (We're still talking about stacked PRs right? Median stacked at any given time is probably 2 and p90 is maybe 4? Max I've ever seen is ~10 and that was quite a fucked up situation for plenty of other reasons.) 10 seconds would certainly be too long for me. It is obviously not too long for them, or they'd learn some ke…

I see, thanks for the information. I often stack 10+ commits, so I don't think your workflow would be reasonable for me. (For example, here, I have 11 finished commits and 3 in progress: https://github.com/arxanas/git-branchless/pull/451 — but I don't actually use stacked PRs on Github, particularly if no one is reviewing my code. Most of my stacking is at work with Phabricator anyways, which handles this better.)

It’s only necessary to push ten branches, one per commit, if you intend ten separate, let’s say, “review events” though. For that example we would have fewer than that nor would we want that many. I do stack 10+ commits fairly regularly but that may be only 1-2 reviews.

(I’m not familiar with Phabricator, only Gerrit and GH/GL, as well as some more manual Workflows.)

Re: In Praise of Stacked PRs

#137

Post by arxanas »

morelisp wrote: post #136
arxanas wrote: post #134

Earlier quoted context omitted.

I see, thanks for the information. I often stack 10+ commits, so I don't think your workflow would be reasonable for me. (For example, here, I have 11 finished commits and 3 in progress: https://github.com/arxanas/git-branchless/pull/451 — but I don't actually use stacked PRs on Github, particularly if no one is reviewing my code. Most of my stacking is at work with Phabricator anyways, which handles this better.)

It’s only necessary to push ten branches, one per commit, if you intend ten separate, let’s say, “review events” though. For that example we would have fewer than that nor would we want that many. I do stack 10+ commits fairly regularly but that may be only 1-2 reviews. (I’m not familiar with Phabricator, only Gerrit and GH/GL, as well as some more manual Workflows.)

Right, in this case, I would prefer 10 separate review events. Other than for code review purposes themselves, I would want CI to run on each of these commits individually, rather than only on the result at the end.

Re: In Praise of Stacked PRs

#138

Post by dcow »

kqr wrote: post #51
dcow wrote: post #15

Earlier quoted context omitted.

I feel similar about languages and style-guides, but I didn't want to conflate the two in my previous comment. Unless you are a royal newb and just learning to write code (which is fine, everybody has to learn), you’re probably writing code in a way that best expresses your intent or goal given your understanding of the language. Let people do that! If you know the language you can handle encountering different style…

This is something I've come to realise as I've matured as a developer. I just don't care what style you write in, if you don't use all the syntactic sugar, etc. I care where you draw your module boundaries, which units depend on which, and if you've handled edge cases, and so on. That's what's going to matter in the end. That's where I'll have to spend a lot of time figuring stuff out. Not syntax.

Exactly. I’ve seen style-perfect lint-free “write once” spaghetti code that drops edge cases and error conditions left and right, and I’ve seen “ugly” but clean code that needs no introduction and works perfectly to boot. I worry that auto-formatters (not to discredit their upsides) do sometimes give people a false sense that they’re writing good code.

Re: In Praise of Stacked PRs

#139

Post by travisb »

jasonhansel wrote: post #63

Why not just break up your change into small components, then merge each one into master separately, hiding the change behind a feature flag until the whole thing is done?

Your suggestion comes after the changes are merged. That is, you wouldn't normally merge anything to master without having it reviewed.

Stacked PRs is _how_ you can break up your changes into small components to merge more quickly and block development less.

With stacked PRs the changes are broken into smaller pieces because a later PR can depend on an earlier PR. This means it can be developed while earlier PRs are still under review. It also means each review is smaller and therefore likely to complete more quickly.

Stacked PRs can also work like patch series in the Linux Kernel development where a large change is broken into smaller independent changes done in the logical order, even if their order of discovery was probably reverse. For example, if you discover you need a refactor or bug fix while halfway through developing a feature. In that case you simply insert a PR earlier in the stack/graph which contains just that isolated fix.

In your feature flag example, a common patten would be to have a dozen or so stacked PRs. The early ones do necessary refactors and bug fixes. The middle few add the feature flag and code. The last make the feature flag the default and delete the now obsolete !feature path. Then the prereqequites (which you didn't discover until half-way through) can be reviewed and merged quickly. The meat of the feature is broken up into easily digestible pieces for quick review, but reviewers can get a sense of the entire feature by considering the PR series as a whole; this is especially useful where one PR adds some infrastructure that a later PR uses but both PRs together would be too large to effectively review. As PRs are ready to merge, they can. Often the last couple of PRs which make the feature the default and delete the obsolete code do not merge for quite some time, but they were written when everything was still fresh in the developers head.

Re: In Praise of Stacked PRs

#140

Post by agentultra »

It depends on the project being worked on. If the main branch changes often enough that you need to merge master before you merge the branch you’ll spend a lot of time propagating changes.

And then there’s the problem of large codebases. Without a really good cache strategy for builds all of the branch switching and merging will cause a lot of rebuilds. For some languages and large projects that can be 20+ minutes for a fresh build on each switch. That’s enough time to get distracted by emails or hacker news.

I’m a big fan of small commits, often and breaking down changes but it does require a large amount of automation and tooling to support well.

Post reply on HN