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

181–190 of 230 posts

Re: In Praise of Stacked PRs

#181

Post by djur »

dcow wrote: post #179
djur wrote: post #165

Earlier quoted context omitted.

> This will fundamentally be up to the author's git skill whether or not they are presenting the PRs to reviewers / mergers as stacked. If they're skilled git users there's little to no cost presenting them one at a time and keeping the not-yet-PRd branches fresh. On multiple teams I've worked on, this has been explicitly discouraged because reviewers often want to see the changeset in as much context as is available…

What's better context than presenting all the related atomic commits at one time in a single pull request so you know exactly which related set of commits are being added and for what reason? Slap a merge commit on and you can revert the whole thing! But your commits are a mess, and I want to see clean ones. That is what interactive rebase is for and what attentive project contributors would do in order to clean up b…

Then we get back to the problem of not having independent review, CI status, and deployability for each individual changeset. There's no amount of git competency that changes the fact that GitHub PRs operate at the branch level, not the commit level. I've actually done exactly what you say many times -- interactive rebase to clean up the commit history -- but the end result of that process is the creation of multiple "stacked" branches, solely so I can submit multiple PRs and conform with the team's expectations.

Re: In Praise of Stacked PRs

#183

Post by arxanas »

dcow wrote: post #177
arxanas wrote: post #137

Earlier quoted context omitted.

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.

You can click on individual commits in a pull request on GH (and in every single git tool I know) to review each commit individually. A pull request is an abstraction over a set of commits. Sometimes it's 1:1, many times it's not. Some large companies force the issue because even though it is more rigamarole for everyone involved, they can afford it. So then people invent silly scripts and coin entire workflows just…

I (and many developers I know) would use the GitHub individual commit review system, but it has a lot of missing features which make it unreasonable to update commits in the PR. For example, if you rewrite one of the commits in a multi-commit PR, then GitHub loses the association and old comments are lost. Similarly, you can't view the diff between an old and new version of a commit.

As far as I know, Phabricator does not let you view or comment on individual commits in a code review (which I use at work). Let me know if you know differently and I might switch to doing that.

Can you advise me on running CI on each commit in the PR in GitHub? As far as I can see, it's technically possible in that you can run arbitrary code as part of your CI, but there's no convenient way to do it. (In particular, I would want to be able to view the CI runs for each individual commit, i.e. render the little checkmark/x next to each commit in GitHub, which seems like it would require a lot of integration via the API.)

One thing that often happens in a stack is the earlier commits are regularly merged into the main branch while the later commits await review. This helps to keep things in sync in a trunk-based development workflow. Is there a way to split out and merge only the earliest (reviewed and accepted) commits, and leave the later commits pending review, in a tool like GitHub?

Overall, I would like to be able to use GitHub PRs for big stacks, but there seems to be a lot of friction in doing so, to the point where it's more convenient to adopt an alternate code review tool (like Graphite) or PR management tool.

Re: In Praise of Stacked PRs

#184

Post by morelisp »

Aeolun wrote: post #161
morelisp wrote: post #43

Earlier quoted context omitted.

If this was just about team review workflow I wouldn't expect to find "easier to rollback" and "don’t need to wait on a particular change to be merged" to be on the list of upsides, as these are properties of the repository structure and not the review workflow.

> "easier to rollback" and "don’t need to wait on a particular change to be merged" to be on the list of upsides Can you explain how these are not just facts? We’re not using stacked branches because it has enough downsides, but these are indubitably upsides of stacked branches.

Stacked branches tend to produce repository structures that make this easy, but so do lots of other development practices. It is not related to "how to evaluate the completion of work", it's entirely about how you use the hammer.

Re: In Praise of Stacked PRs

#185

Post by rendaw »

jacobegold wrote: post #147
isoos wrote: post #115

Earlier quoted context omitted.

> what else are you going to do? Depends on the team and the product. My personal approach is to have 2-3 larger things to work on, so while I wait for reviews on one, I can switch and work on the other. This usually means minimum 1-2 weeks of planned work, sometimes even more, without being blocked on reviews. If everything is blocked, then it is time for some code health cleanup, refactoring and fixing those TODOs…

The advantage of using stacks, though, is that you can keep focus on what you're working on, or even still work on 2-3 things simultaneously but work on multiple dependent pieces of each at once. Maybe I'm biased because I've used this workflow my whole career, but I think everyone can benefit from trying out a stacked PR workflow. Not to mentioned getting reviews for smaller, atomic changes is just SO much easier. E…

I don't think anyone wouldn't agree that being able to work deeply on a single async task would be a great benefit, but your reply completely ignores the issue OP brought up:

> Stacking in my personal experience usually leads to merge conflict hell as changes and PR suggestions get merged underneath you.

That's been my experience too.

And beyond the technical issues, the deeper you work on a single issue the more at risk you are of the simple issue of finding design or requirements issues in the base MR that require going a different direction and discarding the whole stack. So even if you can somehow avoid conflict issues, stacks are still dangerous.

Re: In Praise of Stacked PRs

#187

Post by djur »

dcow wrote: post #177
arxanas wrote: post #137

Earlier quoted context omitted.

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.

You can click on individual commits in a pull request on GH (and in every single git tool I know) to review each commit individually. A pull request is an abstraction over a set of commits. Sometimes it's 1:1, many times it's not. Some large companies force the issue because even though it is more rigamarole for everyone involved, they can afford it. So then people invent silly scripts and coin entire workflows just…

Please take a moment to remind yourself that different people are in different situations and have different social and technical constraints influencing their decisionmaking, and that people can come to different conclusions from your own for reasons other than incompetence and ignorance. I am sure you are a very smart and effective person, but that doesn't mean that you know what is best for every person in every situation.

Re: In Praise of Stacked PRs

#188

Post by morelisp »

hans_castorp wrote: post #182

> “Pull Request” has a specific meaning in Git Aren't "pull requests" a GitHub (and similar systems) thing? As far as I know, Git itself does not have a concept of "pull requests".

My memory is fuzzy at this point but I'm pretty sure the command has been named `git-request-pull` as long as it's existed. It actually makes less sense on most forges since there's never an explicit pull step to apply them, assuming you use the forge to do approval.

Re: In Praise of Stacked PRs

#189

Post by smaudet »

arxanas wrote: post #103
is0tope wrote: post #8

I've usually kept a rule that you should avoid stacking, and if you must only one level deep. The fact that you have to stack in the first place typically suggests that PRs aren't being merged fast enough. Stacking in my personal experience usually leads to merge conflict hell as changes and PR suggestions get merged underneath you.

If you work at asynchronous/remote work company, i.e. your coworkers are in different timezones and can't review immediately, what else are you going to do? Put out exactly one code review per day until your feature is fully merged? Some things like refactoring changes can be reviewed and committed individually, but lots of feature work is fundamentally dependent on the previous work. Stacking PRs is like pipelining…

> what else are you going to do

You can also change the definition of done - something I try to do when a task gets too large, spin off new sub-tasks, etc. The first feature doesn't quite work right, but the merge differential is usually smaller for tweaks or bug-fixes than it is for major initial features.

You can spend a lot time planning how best to break up your work, and sometimes you may be able to do that successfully, but often times it you are constrained not by git but by your tracking tool (jira, trac), which impose somewhat artificial sizes on code tasks....

And really that's the whole thing stacked prs or commits are trying to solve, minimize the merge differential.

Re: In Praise of Stacked PRs

#190

Post by smaudet »

majormajor wrote: post #92
kqr wrote: post #49

Earlier quoted context omitted.

But is it really effective use of your time to make changes to the code that you know won't be relevant for weeks or months? You could spend the same time on other changes that would start earning you money tomorrow, instead.

Sometimes? Consider also the stakeholder who gets annoyed whenever the dev team wants to work on something that would take longer than a week to turn around, and limits the things they'll ask for to those estimated at a week. So bigger things can never get done at all, and you'll just be looking for a local maxima instead of having the chance to make more significant changes. Sometimes it's worth it to prep and clean…

Agreed, sizing is one of the things that can set apart devs, either due to experience, knowledge, or some combination of both.
Post reply on HN