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

GitHub Stacked PRs

github.github.com

Post reply on HN
Print view View original

291–300 of 548 posts

Re: GitHub Stacked PRs

#291

Post by ninkendo »

nerdypepper wrote: post #280
ninkendo wrote: post #107

> a chain of small, focused pull requests that build on each other — each one independently reviewable. I have never understood what this even means. Either changes are orthogonal (and can be merged independently), or they’re not. If they are, they can each be their own PR. If they’re not, why do you want to review them independently? If you reject change A and approve change B, nothing can merge, because B needs A t…

we have been stacking on tangled.org for a while now, you can see a few examples of stacks we have made here: https://tangled.org/tangled.org/core/pulls?state=merged&q=st... for example, this stack adds a search bar: https://tangled.org/tangled.org/core/pulls/1287 - the first PR in the stack creates a search index. - the second one adds a search API handler. - the last few do the UI. these are all related. you are ri…

> - the first PR in the stack creates a search index.

> - the second one adds a search API handler.

> - the last few do the UI.

So you're saying you're going to merge (and continuously integrate, perhaps to production) a dangling, unused search index, consuming resources with no code using it, just to make your review process easier?

It's very depressing that review UX is so abysmal that you have to merge features before they're done just to un-fuck it.

Why can't the change still be a big branch that is either all merged or not... and people can review it in chunks? Why do we require that the unit of integration equals the unit of review?

The perverse logic always goes something like this:

"This PR is too big, break it up into several"

Why?

"It's easier to review small, focused changes"

Why can't we do that in one PR?

"Because... well, you see GitHub's UI makes it really hard to ..."

And that ends up being the root-cause answer. I should be able to make a 10,000 line change in a single commit if I want, and reviewers should be able to view subsets of it however they want: A thread of discussion for the diffs within the `backend` folder. A thread of discussion for the diffs within the `frontend` folder, etc etc. Or at the very least I should be able to make a single branch with multiple commits based on topic (and under no obligation for any of them to even compile, let alone be merge-able) and it should feel natural to review each commit independently. None of this should require me to contort the change into allowing integration partially-completed work, just to allow the review UX to be manageable.

Re: GitHub Stacked PRs

#293

Post by dtech »

jmalicki wrote: post #289
eru wrote: post #283

Earlier quoted context omitted.

> [...], it is a uniquely horrible slow language. To be fair, Python as implement today is horribly slow. You could leave the language the same but apply all the tricks and heroic efforts they used to make JavaScript fast. The language would be the same, but the implementations would be faster. Of course, in practice the available implementations are very much part of the language and its ecosystems; especially for a…

Fair! I guess I didn't mean language as such, but as used. But a lot of the monkey-patching kind of things and dynamism of python also means a lot of those sorts of things have to be re-checked often for correctness, so it does take a ton of optimizations off the table. (Of course, those are rare corner cases, so compilers like pypy have been able to optimize for the "happy case" and have a slow fall-back path - but…

Javascript has a lot of the same theoretical dynamism, yet V8 and WebkitCore were able to make it fast

Re: GitHub Stacked PRs

#294

Post by ninkendo »

mh2266 wrote: post #243
ninkendo wrote: post #107

> a chain of small, focused pull requests that build on each other — each one independently reviewable. I have never understood what this even means. Either changes are orthogonal (and can be merged independently), or they’re not. If they are, they can each be their own PR. If they’re not, why do you want to review them independently? If you reject change A and approve change B, nothing can merge, because B needs A t…

you're upgrading the repository from language version 1 to 2, version 2 adds new compiler errors that rejects some old code, or the library has removed some old deprecated API the repository was still using in some places—the key here being that it can't be something that needs to be completely atomic. you have hundreds or thousands of files to fix. that is unreviewable as a single commit, but as a per-file, per-libr…

> you have hundreds or thousands of files to fix. that is unreviewable as a single commit, but as a per-file, per-library, per-oncall, etc. commit it is not that bad

Why is it intrinsically unreviewable as a single commit? Why can't the discussion/review system allow scoping discussions to a single folder of the change, or a single library, or a particular code-owner's "slice" of the repo, etc? The answer to this question is always unsatisfactory to me. It always ends up being "because GitHub's UI makes it hard to " and it's just taken as an immutable law of the universe that we're stuck with that UI's limitations.

If a change is huge, find some basis by which to discuss it in smaller chunks. That basis doesn't have to be the PR itself (such that you have to make smaller PR's to make discussion manageable.) It can be a subdirectory of the diff. A wildcard-match over the source files. Whatever the case needs to be, the idea is still that the discussion UX shouldn't make reviewing large changes painful.

Why do we tolerate the fact that GitHub doesn't let you say "approved for changes in `frontend/*`" or "approved for the changes I'm a code-owner of", and have the PR check system mark the PR as approved once all slices have been approved? Why do we tolerate that a thousand-file change is "unreviewable"? Instead we have to change our unit of integration, allowing partially-complete work to be merged, just because the review UX sucks.

Re: GitHub Stacked PRs

#295

Post by verst »

steveklabnik wrote: post #233
dathanb82 wrote: post #226

Earlier quoted context omitted.

Unless you have a “every commit must build” rule, why would you review commits independently? The entire PR is the change set - what’s problematic about reviewing it as such?

In stacked diffs system, each commit is expected to land cleanly, yes.

But isn't that why you would squash before merging your PR? If you define a rule that PRs must be squashed you would still have the per commit build.

Re: GitHub Stacked PRs

#296

Post by jillesvangurp »

cleverdash wrote: post #30

As a solo dev I rarely need stacked PRs, but the underlying problem, keeping PRs small and reviewable, is real even when you're your own reviewer. I've found that forcing myself to break work into small branches before I start (rather than retroactively splitting a giant branch) is the actual discipline. The tooling just makes it less painful when you don't. Curious whether this changes anything for the AI-assisted w…

It's easier to pile on a lot of changes with AI assisted workflows. And reviewing all that is definitely a challenge just because of the volume of changes. I've actually stopped pretending I can review everything in detail because it makes me a bottleneck in the process. Anything that makes reviewing easier is welcome.

To me, stacked PRs seems overly complicated. It seems to boil down to propagating git rebases through stacks of interdependent branches.

I'm fine with that as long as I don't have to deal with people force pushing changes and routinely rewriting upstream history. It's something you probably should do in your own private fork of a repository that you aren't sharing with anyone. Or if you are, you need to communicate clearly. But if the goal is to produce a stack of PRs that in the end merge cleanly, stacked PRs might be a good thing.

As soon as you have multiple collaborators working on a feature branch force pushing can become a problem and you need to impose some rules. Because otherwise you might end up breaking people's local branches and create work for them. The core issue here is that in many teams, people don't actually fork the main repository and have push access to the main repository. Which emulates the central repository model that people were used to twenty years ago. Having push access is not normal in most OSS projects. I've actually gotten the request from some rookie developers that apparently don't get forking to "please give me access to your repository" on some of my OSS projects.

A proper pull request (whether stacked or not) to an OSS project needs to be clean. If you want to work on some feature for weeks you of course need mechanisms to stay on top of up stream changes. OSS maintainers will probably reject anything that looks overly messy to merge. That's their job.

Re: GitHub Stacked PRs

#297

Post by pabs3 »

forrestthewoods wrote: post #212
jstimpfle wrote: post #203

Earlier quoted context omitted.

Now that is interesting too, because git is very fast for all I have ever done. It may not scale to Google monorepo size, it would ve the wrong tool for that. But if you are talking Linux kernel source scale, it asolutely, is fast enough even for that. For everything I've ever done, git was practically instant (except network IO of course). It's one of the fastest and most reliable tools I know. If it isn't fast for…

The fact that Git has an extremely strong preference for storing full and complete history on every machine is a major annoyance! “Except for network IO” is not a valid excuse imho. Cloning the Linux kernel should take only a few seconds. It does not. This is slow and bad. The mere fact that Git is unable to handle large binary files makes it an unusable tool for literally every project I have ever worked on in my en…

Git handles large text files and large directories fairly poorly too.

Re: GitHub Stacked PRs

#298

Post by Rodeoclash »

montag wrote: post #137
calebio wrote: post #106

Earlier quoted context omitted.

I miss the Phabricator review UI so much.

Me too. And I'm speaking from using it at Rdio 15 years ago. Nothing since (Gerrit, Reviewboard, Github, Critique) has measured up...

Thanks for your work on Rdio. I miss it. Were you around when that guy managed to spam plays to get fake albums to the top of the charts?

Re: GitHub Stacked PRs

#299

Post by dbetteridge »

eru wrote: post #288
illamint wrote: post #286

Earlier quoted context omitted.

Good. That's the point.

The point of what? I hope they fixed phabricator in the meantime.

The point is the main branch reflects the "units" of change, not the individual commits to get there.

One merged pr is a unit of change, at the end of the day the steps you took to produce it aren't relevant to others.

My opinion of course, I'm open to understanding why preserving individual commits is beneficial

Re: GitHub Stacked PRs

#300

Post by pabs3 »

js2 wrote: post #86
tcoff91 wrote: post #75

Earlier quoted context omitted.

It's basically trying to bring the stacked diff workflow pioneered by Phabricator to GitHub. The idea is that it allows you to better handle working on top of stuff that's not merged yet, and makes it easier for reviewers to review pieces of a larger stack of work independently. It's really useful in larger corporate environments. I've used stacked PRs when doing things like upgrading react-native in a monorepo. It r…

> stacked diff workflow pioneered by Phabricator Ahem, pioneered by gerrit. But actually, I'm almost certain even that wasn't original art. I think gerrit just brought it to git. https://www.gerritcodereview.com/about.html

Gerrit was forked from Rietveld. Not sure if Rietveld or Gerrit are better though.

https://github.com/rietveld-codereview/rietveld https://en.wikipedia.org/wiki/Rietveld_(software) https://codereview.appspot.com/

Post reply on HN