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

191–200 of 230 posts

Re: In Praise of Stacked PRs

#191

Post by morelisp »

zackmorris wrote: post #61
compsciphd wrote: post #9

One thing I wish git did (maybe it does and I don't know how?) is to be able to say that a new branch is based off an old branch (not a commit that used to be that branches head). so I can branch a single pr in progress to start the next. Then if I change the base pr in progress (say via rebase or via squashing or the like), I can easily rebase my new commits in the new pr on top of the current state of the branch. C…

Came here to say the same thing. I always rebase my own branch onto the trunk and force-push before merging. But that's hard to explain to other developers, and there's no way to enforce it, so the repo inevitably becomes filled with overlapping merges, which is an unforced error IMHO. Does anyone know if there's a way to only allow rebase-and-merge on GitHub and GitLab? https://docs.github.com/en/pull-requests/colla…

In GitLab this is referred to as "Merge commit with semi-linear history" and it's a checkbox in the general MR settings.

Re: In Praise of Stacked PRs

#192

Post by endorphine »

dboreham wrote: post #158

Feel the need to observe that we don't need to use PRs at all. Furthermore I see the PR workflow is mostly a cult (which by the way I don't remember anyone asking for -- it seemed to come out of the woodwork either from gitHUB, or from Google). I think it makes some sense in the context of maintenance fixes to a mature release. Perhaps also makes sense in the context of "over the transom" contributions to an OSS proj…

What's an alternative, considering that you still want co-workers to review your code before you ship it?

Re: In Praise of Stacked PRs

#193

Post by WookieRushing »

avl999 wrote: post #46

I do stacked PRs at work, they work great until someone suggests an invasive change in a PR lower down in the stack. Does anyone have ideas on how to deal with merge conflicts in this type of flow? For example let's say I have the following stack of PRs PR1 -> PR2 -> PR3 -> PR4 The reviewer reviews and suggests a change in PR1, this change causes a merge conflict in PR2 and therefore in PR3 and PR4 as well. And then…

Mercurial solves this 90% of the way with hg absorb: https://gregoryszorc.com/blog/2018/11/05/absorbing-commit-ch...

Its extremely close to magic. You make all the requested changes on the PR4 and type hg absorb and it'll figure out which commit in your stack each change belongs to.

I've used this to work on lots of stacks 20 commits deep in mercurial.

Re: In Praise of Stacked PRs

#194

Post by WookieRushing »

hamandcheese wrote: post #170
sidlls wrote: post #167

Stacked PRs are an indicator of an incompletely thought out approach to the implementation. Prefer sequential PRs that layer in the implementation: test code, interfaces/API stubs, and then concrete implementations of the stubs (for example). This tends to make PRs less complex and more inviting for colleagues to review, which in my experience tends to make them go much faster. It also leads to generally better code…

But how can you write the concrete implementation without stacking? The only thing stacked PRs indicate, IMO, is that your coworkers are slow to review your code. And in my experience, when folks send out stacked PRs, it’s because they put much _more_ thought and effort into identifying the right boundaries, not less.

Yup, stacks are wonderful for new features. Typically I run a stack of 5 for new features.

First one is the API/interface with null implementations. Then subsequent ones each implement a method with associated tests

Re: In Praise of Stacked PRs

#195

Post by compsciphd »

msbarnett wrote: post #34
compsciphd wrote: post #9

One thing I wish git did (maybe it does and I don't know how?) is to be able to say that a new branch is based off an old branch (not a commit that used to be that branches head). so I can branch a single pr in progress to start the next. Then if I change the base pr in progress (say via rebase or via squashing or the like), I can easily rebase my new commits in the new pr on top of the current state of the branch. C…

> TLDR: Basically, want to be able to state that branch depends on branch (which is currently commit id x) but if I rebase, use whatever the current commit id is for that branch, not whatever it was when I first made the branch. As far as I can tell (the “problem” you’re describing is a bit vague) this is already how git works with the sole exception being that git doesn’t default to a particular branch for a rebase?…

think of it this way. When I say

git branch ..., I'm creating a new branch pointer that points to a commit and any future commit will update that branch pointer (not the original).

if I then add a new commit to the original branch, I'll update its branch pointer.

if I do then do a git rebase -i original_branch (on new branch), (I believe) git will look for the common ancestor commit, update the head to original_branch's current id, and then apply individual every commit id from that common ancestor to the head of my what was my new branch.

if I just add a commit to original branch, this is "clean" (i.e. i might have lots of conflicts, but it makes sense, its just effectively inserting a new commit into the middle of the commit stream). However, if I have rebased the original branch, such that common ancestor isn't where it used to be, but much further back in time), it no longer makes sense.

What I was proposing (and what I could do manually), is when i rebase against the proposed type of branch, then we essentially, move the head to the current head of the updated "old branch" and then cherry-pick one by one each commit i made against the new branch (fixing merge errors along the way, just like with rebase).

is this a bit clearer?

Re: In Praise of Stacked PRs

#196

Post by hans_castorp »

sebastianz wrote: post #186
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".

https://git-scm.com/docs/git-request-pull

Thanks, learned something new today

Re: In Praise of Stacked PRs

#197

Post by dcow »

djur wrote: post #181
dcow wrote: post #179

Earlier quoted context omitted.

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…

> There's no amount of git competency that changes the fact that GitHub PRs operate at the branch level, not the commit level.

Most all CI tools support per-commit checks with simple configuration/scripting of their execution DSL/API. In GitHub, push actions already works this way. You can do the same thing on any branch after a PR is opened: on synchronize you can iterate over all the updated refs, run checks, and publish results to the commit status API https://docs.github.com/en/rest/commits/statuses, and https://docs.github.com/en/pull-requests/collaborating-with-....

Re: In Praise of Stacked PRs

#198

Post by gocartStatue »

macintux wrote: post #84
fallingknife wrote: post #25

This just shows that people will go to any length to procrastinate PR reviews. The truth is that to be an effective team, reviews need to be highest priority because they are blockers. There is just no way around it.

Any categorical assertion like that feels wrong. Not all projects, all teams, all problems are the same.

Lower work in progress generally results in faster & more predictable delivery. PR's are an inventory of "almost finished goods". "Stacking" PR's essentially means delaying integration and increasing WIP.

Re: In Praise of Stacked PRs

#199

Post by thurn »

rsstack wrote: post #3

We switched to using https://graphite.dev/ after yet-another huge epic that caused a mess of PRs and one PR that touched 100+ files and included dozens of semi-related changed. It's been a blessing so far.

Little suspicious of a tool with no pricing information from a company that just raised $20 million. I assume it will not remain free.

Re: In Praise of Stacked PRs

#200

Post by wizofaus »

nightpool wrote: post #141
wizofaus wrote: post #131

Earlier quoted context omitted.

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…

I think maybe we're using different definitions of the term "context-switch". Certainly it's an interruption , but I don't really think that sitting down with another engineer to do a focused code review where they've already written out a PR description and thought hard about the problem is comparable to starting a brand new branch or picking it up after a while away and trying to juggle 2, 3, or 4 in-progress ticke…

It requires a mental switch in focus. For me anyway, it's not a big difference if it's between working on implementing feature A then switching to working on implementing feature B, and working on feature A then switching to doing a code review for feature B, but I probably take my code reviewing responsibilities more seriously than most (e.g. I always checkout the branch locally, ensure it builds and runs etc.).
Post reply on HN