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

Stacked Diffs versus Pull Requests

jg.gg

Post reply on HN
Print view View original

71–77 of 77 posts

Re: Stacked Diffs versus Pull Requests

#71

Post by WorldMaker »

charrondev wrote: post #70
WorldMaker wrote: post #61

Earlier quoted context omitted.

It's the tools and their defaults that leads me to my biggest criticism with the article. On darcs something like "stacked diffs" makes a ton of sense because the UI was built for it from top down, and cherry-picking is _magic_ (when it works), including that it tracks your dependencies between commits/patches for you. In git, we have a dependency tracker in the DAG, but it requires the work of branching to make use…

What PR tools are you referring to? We just use github pull requests, but stacking PRs seems impossible because merging the parent PR doesn’t seem to update the children PRs automatically.

I've used GitHub, Azure Repos (nee VSTS nee VSOnline), and Bitbucket (Cloud and Server), and I've seen some version of support for it in all of them.

Re: Stacked Diffs versus Pull Requests

#72

Post by WorldMaker »

dom96 wrote: post #69
WorldMaker wrote: post #62

Earlier quoted context omitted.

If the branches are properly merged so that the dependency is represented in the git DAG, most good PR tools just handle this correctly: if you merge the dependent PR first the dependee updates to show the commits as already merged and "simplify" their diff views appropriately; if you merge the dependee PR first, the dependent PR simply gets marked as completed/merged for you. I find it is still useful to include hel…

Yes, that's nice. But the problem is that these are still shown as separate PRs, GitHub for example doesn't tie these together unlike Phabricator.

GitHub has enough information that it could surface that better. It's a useful feature request.

Re: Stacked Diffs versus Pull Requests

#73

Post by Joky »

slobotron wrote: post #55
Joky wrote: post #12

Earlier quoted context omitted.

"on top of the current master" is limiting: how do you handle concurrency? I.e. two developers kicking tests on their PR on top of current master, everything green, but they merge and the result of their merge is not building (or passing tests) anymore.

Say two PRs get sent in by two different developers. CI builds both, passes them. Reviewer comes and approves PR#1, it is merged into master. Depending how much CI power you have, PR#2 (and any other PR in queue) can be automatically built again on top of new master to verify it wont break anything. You can do it lazily too and just re-check PR#2 once it gets approved by human - like other comment mentioned, CI coord…

That's not very scalable: with a team of 100 developers merging on average 100 PRs a day, you have lot of concurrency. Imagine everyone want to merge at the same time, only one can, the 99 others have to re-run the build/tests. Then only one merges and 98 others have to re-run the build tests, etc. On a project where the build/tests can take hours, this becomes difficult to justify...

And then, there is the "I merged a fix into the release branch, which auto-merge into master, the fix built and tested OK in the release branch but not when propagated to master".

Re: Stacked Diffs versus Pull Requests

#74

Post by Joky »

zbentley wrote: post #51
Joky wrote: post #12

Earlier quoted context omitted.

"on top of the current master" is limiting: how do you handle concurrency? I.e. two developers kicking tests on their PR on top of current master, everything green, but they merge and the result of their merge is not building (or passing tests) anymore.

Most CI systems serialize builds on master, at least at the very end of the build pipeline. That can slow things down, but, if combined with a decently fast/thought-through build process with tears, I think the benefits of not risking untested states outweigh the costs.

This is not about serializing builds on master, but builds on the PR before they get merged on master. If you have a few hours of test plans, you can't serialize every PR one after each other.

Re: Stacked Diffs versus Pull Requests

#75

Post by Joky »

eequah9L wrote: post #38
Joky wrote: post #14

Earlier quoted context omitted.

Bisect on first-parent? Seems like a script exists (haven't tried): https://gist.github.com/ayust/2040290

Wouldn't this mean you just find the merge that caused the regression? The merge itself might bring in an awful lot of changes (of a single topic).

The merge won't bring more changes than if the PR was squashed in a single commit though. Hopefully if you have a test broken and the PR merged two commits: one that refactored the code and the second one that leveraged the refector to change the behavior, you can easily try to revert the latter to check if it fixes the problem. The commit is smaller, easier to re-review, and the bug likely easier to spot.

Re: Stacked Diffs versus Pull Requests

#76

Post by lima »

yxhuvud wrote: post #57
lima wrote: post #28

Earlier quoted context omitted.

You can do stacked diffs on GitHub by always using squash merges and rebasing on top of master, and it's a great way to get started. It's just a lot nicer with the extra tooling on top that Phabricator and Gerrit provide (dependencies, command line tools, and so on).

If you squash merge you just lost all the small commits that do one thing well that you spent time creating, unless you review them one by one. Bleh.

That's the entire point of a stacked diff workflow - you would submit smaller PRs which are the equivalent of one commit.

Re: Stacked Diffs versus Pull Requests

#77

Post by zbentley »

Joky wrote: post #74
zbentley wrote: post #51

Earlier quoted context omitted.

Most CI systems serialize builds on master, at least at the very end of the build pipeline. That can slow things down, but, if combined with a decently fast/thought-through build process with tears, I think the benefits of not risking untested states outweigh the costs.

This is not about serializing builds on master, but builds on the PR before they get merged on master. If you have a few hours of test plans, you can't serialize every PR one after each other.

If your testing is time consuming to the point that you can only test on your PRs and never on what master would be after each one merges, then you have to choose between somehow forcing PR authors to serialize their merges somehow (external convention or tooling or something) or not testing the code that will actually be released. The latter isn’t always the wrong choice, though.
Post reply on HN