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

21–30 of 230 posts

Re: In Praise of Stacked PRs

#21

Post by dan-robertson »

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.

One advantage of ‘stacking’ is breaking up review into more logical units, eg

1. Introduce new test exhibiting bug

2. Introduce bug fix and update the test

If 1 and 2 are reviewed together you have less evidence that the test actually shows the bug being fixed.

Re: In Praise of Stacked PRs

#23

Post by regularjack »

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…

Isn't that how rebase already works? It will "apply" commits of the current branch onto commits of the base branch, including new commits that were created in the meantime.

Re: In Praise of Stacked PRs

#24

Post by dcow »

seoaeu wrote: post #14
dcow wrote: post #7

I’m going to be a little crass for a moment: people don’t know how to use their tools! Why are we even talking about “stacked PRs” and “branchless” workflows?! It’s clear the author is documenting their git learning process and that’s great, really. But I’m just surprised how pedantic people get about forcing others to use a tool in a way that caters to their own limited understanding of it. Or, even worse, caters to…

This comment would be more compelling if it gave any hint about what aspect of the tooling the author didn’t understand or what they should be doing differently.

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 that other styles are exist and are valid.

Compare with a construction site: there are obvious uses for tools and there are less obvious uses and I haven't been on a single one where tools are only used specifically in one way for the entirety of the project. They’re used dynamically by different people with different experiences in order to complete the project. Nobody forces other workers to use a specific grip when holding their hammer…

Re: In Praise of Stacked PRs

#25

Post by fallingknife »

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.

Re: In Praise of Stacked PRs

#26

Post by dtech »

dan-robertson wrote: post #21
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.

One advantage of ‘stacking’ is breaking up review into more logical units, eg 1. Introduce new test exhibiting bug 2. Introduce bug fix and update the test If 1 and 2 are reviewed together you have less evidence that the test actually shows the bug being fixed.

You can still have them in 2 commits, and configure your CI to build both of them, and the first 1 should fail.

We actually have a rule that there must be 1 commit just introducing a test that fails on CI for bugfix PRs.

Re: In Praise of Stacked PRs

#27

Post by YZF »

dan-robertson wrote: post #21
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.

One advantage of ‘stacking’ is breaking up review into more logical units, eg 1. Introduce new test exhibiting bug 2. Introduce bug fix and update the test If 1 and 2 are reviewed together you have less evidence that the test actually shows the bug being fixed.

Ideally your CI workflow prevents merging something that breaks the tests. Also now if the first review merges but the other doesn't for some reason you've broken everyone else which is bad.

EDIT: at least in rebase workflows which is what I'm used to. I guess in merge workflows this works.

Re: In Praise of Stacked PRs

#28

Post by theptip »

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…

This is one of the big features from git-branchless (https://github.com/arxanas/git-branchless/wiki/Command:-git-...) which is recommended in the article; moving a stack (sub-tree) of commits instead of just one branch.

Might be useful for you if you find the rebase flow to be painful. In particular this can be good for local prototyping where you have a bunch of functional candidates on top of some foundational refactors, and you may be rebasing frequently to refine those foundational commits.

Re: In Praise of Stacked PRs

#29

Post by dan-robertson »

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…

It feels like roughly the issue is that git thinks in terms of snapshots whereas people often think in terms of patches. Darcs and pijul are version control systems that try to have patches as the fundamental object you operate on instead of snapshots. In particular, there isn’t a rerere operation to change the base of a commit because patches don’t have bases in the same way. However if you’re reviewing code you probably do care about the base because changing the base may change the meaning of the branch and if eg some library function is renamed between when you write/approve a patch and when you merge it, the patch may no-longer be valid.

Re: In Praise of Stacked PRs

#30

Post by computronus »

ramraj07 wrote: post #12
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.

It’s unavoidable sometimes. I get inspiration and time together rarely, I can’t wait for small chunks of code to be merged before I continue. A lot of times it’s an extremely Productive Sunday afternoon and I have 2500 new lines of code that builds a full new prototype. What am I to do?

First, think about how difficult, and time-consuming, it will be for others to digest and review 2500 new lines of code that sprung from someone else's mind. So you will end up waiting anyway, for even a small part of your work to be merged.

The work of breaking up a big, inspired chunk of work into small pieces helps you learn more about it, and the perspective can reveal improvements that weren't obvious in the initial effort. You might notice those yourself, or reviewers may. The final outcome will end up overall better for it, so spending that time is worthwhile.

Post reply on HN