Comment by necovek | Hacker News Reader

Comment by necovek | Hacker News Reader

I dislike them reinforcing the component approach to delivering work through their examples, like the top screenshot showing "database schema changes", "api changes" and "frontend implementation" as separate branches in a stack.

So really, one does consider full stack a single feature, but unless they are reviewed in one go — which defeats the purpose of stacked branches and pull requests — you can end up landing one and a later review in branches higher in the stack needing changes in the lower branches even if they were already reviewed.

When you instead focus on full use-case per branch, but scope them down, it is much less likely you will need to change branches lower in the stack after they are reviewed.

Another obvious use-case is to do a pre-emptive refactor, though I actually prefer doing a post-refactor after the new use-case has been merged in — it's much easier to know the target best approach when you've got your use-cases right in front of you (or you may hit a similar problem as above).

FWIW, I remember fondly using bzr-pipeline plugin to bzr VCS ~15 years ago to do exactly this.

Thanks for mentioning this. It seemed odd to me, too, so I spent some time trying to work it out. As a reviewer, I'm not sure how I'm supposed to assess database or API changes without knowing how they're intended to be used. And deploying them independently seems odd, too, especially if you need to roll it all back.

I think in my ideal world there would be a clean history and I could review a PR commit-by-commit. But, you can't just approve a single commit, so there's a tooling problem there. And most CI runs on an entire push rather than individual commits. And increasingly I see devs using git as an offsite backup for whatever change they just made, rather than breaking commits up into logical chunks. In that workflow, squashed merges make the most sense.

It's probably flawed, but the mental model I came up with is each stacked PR collapses into what would have been an individual commit in a clean PR, with the advantage of being able to be reviewed separately from the other changes and forced to clear CI. And then the whole stack becomes what would have been a clean PR in the old model. That, I can kinda see the benefit of. But, merging only part of the stack into trunk is the mental hurdle I can't clear; it'd be like merging only some commits from a PR. It kinda reminds me of when projects used CVS.

I've really only seen stacked PRs used on projects where history is little more than an audit log. I'm keen to see how this gets employed by open source projects. I think there's a disconnect and it's likely I'm not going to really get it until I see it.

This is my confusion, we already have commits to bundle changes, why not simply allow commits to be reviewed independently within a pr?

Doesn't this have the same issue? If you need database changes that also need query changes or api changes, then you need to modify multiple commits.

This is similar to how things like Gerrit do code review, and it's pretty nice

Ok, then how do you approve/take the first 2 commits and not the last one? Or how do you insert a commit in between 2 commits or address feedback on a given commit?

The person making the change is now going to have to run multiple confusing interactive rebases and git shenanigans, the rewrite the history of the PR branch on every feedback, then you have to re-review all the commits again because they are all different.

It’s possible of course to push all that complexity on the tooling. Have GitHub and git provide tooling for doing all that within the context of a single branch/PR. But why is that better? Multiple branches are easier to manage in git, and as long as they don’t conflict on the merge. Obviously if a feedback on PR#1 causes a conflict in PR#2 which causes a conflict in PR#3 it’s still tedious, but it’s a lot more doable than managing interactive rebases on every feedback comment.

Jujutsu makes all this easy

Jujutsu makes the author-side of it all fairly nice, indeed. But the reviewer-side has been so horribly abandoned by github that it has been frustrating me for a long while now.

A JujutsuHub.com business opportunity presents itself. How many tokens do you have?

Already working on that!

Yeah, clean, neatly seperated and logically independent PRs are very nice for reviewers, but usually it requires one to complete the whole feature and then go back and think about the best way to seperate it into a series of smaller changes again. It works for projects like linux where there is tons of motivated manpower such that requiring authors put in a day of additional effort to make a change as presentable as possible is acceptable.

I usually just amend commits as I go and don’t find it all that onerous. Sometimes it gets tricky, particularly if a rebase effectively changes what the code would have looked like. But, with LLMs even that’s gotten much easier.

Regarding reviewing commit-by-commit, I actually do believe a branch is a unit of work to be reviewed, as long as it is fully standalone and implements a usecase end-to-end (no matter how small, and small it should be).

Commits on a branch are a tool for a developer, and they will go back and forth a bit as they learn more about it, perhaps explore a path, and then go back on it.

With bzr (Bazaar, since abandoned by Canonical, but maintainers forked it as Breezy), you had a nested history: top-line merges look like squashed merges in git, but you simply do a bzr log -n1 and get to see the next level of commits in each merged branch and you can understand the build process and explore what other things original author tried out which did not work. It is simply a way to get the best of both worlds IMO (it was noticeably slower than git, though).

I did find it hard to get the product and design to adopt a similar mindset of developing a feature iteratively, so it was usually the developer who'd come up with in-between designs and UX flows while they converge to the final design over multiple small branches.

At Facebook (i left >4 years ago) we used Phabricator where shipping stacked changes like this was table stakes. I still miss it. Some few random points, not really organized, i'm sure i'm forgetting bunch more benefits:

* I often used it as a way to make reviews significantly easier for the reviewer. If I split something more complex into smaller units where the small change seems obviously correct, it's much more likely i'll get a faster accept than with a very large change.

* Critically, once a unit of the stack has been approved, the approval will still stay with rebases of the whole stack.

* You can keep building up a stacs and once you think you have enough for whatever you are doing, ship the whole stack.

* Suppose you have a 'hole' in the reviews and you're missing a review for a middle part of the stack. You can still merge the bottom part up to where all are reviewed.

* Let's assume you're the reviewer: smaller changes is much easier to review. Also, each part of the stack builds incrementally on the previous one. With your example, suppose you're reviewing 'api changes' part of the stack, but it has failing tests because something is broken with the lower 'db schema changes' change - but otherwise, the 'api changes' diff looks fine - you can then accept it and this part of the stack is already reviewed. The author can now whenever fix their bottom-most change (db schema change) to have tests passing, and get someone to review that (perhaps someone completely different more familiar with db), and then rebase the 'api changes' diff, and _it will not_ require review because you haven't changed anything, just updated the diff underneath it. In this way you parallelized your approvals. Right now at work, using github, I have to serialize my PRs and can't effectively get things reviewed until the previous PR is approved.

As I mentioned, this is similar to a workflow we used at Canonical ~15 years ago with bzr-pipeline plugin for bzr.

Except that you tried not to divide it by technical aspect (DB changes vs API vs frontend) bit instead into smaller, semantic full features (we also used a 800/400 line diff limit which helped enforce it).

> it's much more likely i'll get a faster accept than with a very large change.

I do agree the above is true. And maybe it's just me, but it feels like I'm kind of cheating if I stack PRs to get a faster approval. Because when one reviews some dangling code without the context of how it's going to be wired in, they can't really tell what the blast radius will be and consequently what bar they should hold the code against (e.g. performance, security, etc). In these circumstances, usually the most one can do is raise some stylistic nits (which to be fair PR process often degrades into anyway) and move on.

Reply on news.ycombinator.com