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

51–60 of 77 posts

Re: Stacked Diffs versus Pull Requests

#51

Post by zbentley »

Joky wrote: post #12
slobotron wrote: post #4

Earlier quoted context omitted.

Hm, I thought the idea is that the only way something will land in master is if it reviews OK and CI passes all the tests on top of the current master?

"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.

Re: Stacked Diffs versus Pull Requests

#52

Post by krupan »

Ajedi32 wrote: post #41

So basically the author proposes replacing Pull Requests with "Cherry-Pick Requests", where each commit is a single reviewable unit (replacing separate branches with the PR model), the local master branch represents a list of unreviewed commits, and the remote master branch represents reviewed, "merged" commits. It's an interesting idea, but even after reading this article I'm still not convinced it offers any seriou…

" It also has the downside of requiring extensive use of more advanced features like interactive rebasing."

This is true. In the early days of mercurial (back before it was clear that git was winning the popularity contest) there was an extension called Mercurial Queues[1] that made it really easy to work this way. The downsides were: it didn't handle conflicts well, and it could totally destroy your work if you weren't careful. Since then someone has written the Evolve extension which is Mercurial Queues done right. That, sadly, came about after git had "won" the popularity contest.

1. Best explanation of Mercurial Queues: http://stevelosh.com/blog/2010/08/a-git-users-guide-to-mercu...

Re: Stacked Diffs versus Pull Requests

#53

Post by krupan »

jepler wrote: post #3

I think I agree with what the author is saying, by and large, but it doesn't matter: if it's a mental model that works for you, you can live in the "stacked diffs" world right up until you need to "curate" all your commits into something that can be considered for incorporation into the parent project. Then you still need to make a branch, put all the related commits into it in order, and adequately test that branch…

"I think I agree with what the author is saying, by and large, but it doesn't matter: if it's a mental model that works for you, you can live in the "stacked diffs" world right up until you need to "curate" all your commits into something that can be considered for incorporation into the parent project"

That's funny because he spent the whole blog post pointing out why it does matter. The tools we use and their defaults structure the way we work. Having lived in both worlds I can add my testament to that.

Re: Stacked Diffs versus Pull Requests

#55

Post by slobotron »

Joky wrote: post #12
slobotron wrote: post #4

Earlier quoted context omitted.

Hm, I thought the idea is that the only way something will land in master is if it reviews OK and CI passes all the tests on top of the current master?

"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 coordinates and serializes all merges into master making sure master is always green.

Re: Stacked Diffs versus Pull Requests

#56

Post by yxhuvud »

peterjmag wrote: post #19

I think GitHub's great, and I'm mostly okay with having to rely on it heavily at my day job, but I dislike how code reviews + PRs are necessarily tied to branches. What are the chances of GitHub introducing stacked diffs (or a similar abstraction) as an option? It'd be great to try out this workflow on a non-trivial project, but it seems like a hard sell organizationally when it involves migrating to and supporting a…

I'm not totally buying the Phabricator way, but yes, it would be so nice to be able to individually greenlight individual commits in a pull request, and when reviewing, be able to a: see all commits on top of each other on the same page without mixing their changes and b: be able to comment on commit messages.

Re: Stacked Diffs versus Pull Requests

#57

Post by yxhuvud »

lima wrote: post #28
peterjmag wrote: post #19

I think GitHub's great, and I'm mostly okay with having to rely on it heavily at my day job, but I dislike how code reviews + PRs are necessarily tied to branches. What are the chances of GitHub introducing stacked diffs (or a similar abstraction) as an option? It'd be great to try out this workflow on a non-trivial project, but it seems like a hard sell organizationally when it involves migrating to and supporting a…

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.

Re: Stacked Diffs versus Pull Requests

#58

Post by marcinkuzminski »

yxhuvud wrote: post #56
peterjmag wrote: post #19

I think GitHub's great, and I'm mostly okay with having to rely on it heavily at my day job, but I dislike how code reviews + PRs are necessarily tied to branches. What are the chances of GitHub introducing stacked diffs (or a similar abstraction) as an option? It'd be great to try out this workflow on a non-trivial project, but it seems like a hard sell organizationally when it involves migrating to and supporting a…

I'm not totally buying the Phabricator way, but yes, it would be so nice to be able to individually greenlight individual commits in a pull request, and when reviewing, be able to a: see all commits on top of each other on the same page without mixing their changes and b: be able to comment on commit messages.

In RhodeCode we have a similar concept that we call diff ranges. It's not available in pull-request view directly, however, users can see this on a source repo if they select a range from changelog view.

example: https://code.rhodecode.com/rhodecode-enterprise-ce/changeset...

I agree with the concept and it's sometimes very important to see how each commit produced the final combined diff.

Re: Stacked Diffs versus Pull Requests

#59

Post by geofft »

dom96 wrote: post #46
yumaikas wrote: post #39

How does this workflow work when there are 3 commits that need to be applied in order (A, B, and C), when C gets approved from a code-review standpoint before A and B?

You are prevented from merging commits at the top of the stack until the commits below are accepted. The advantage is that each commit can be reviewed separately. You cannot achieve this with pulled requests as far as I know. I often see people creating a pull request and writing "this depends on PR #1233", the second PR is unreviewable because it contains changes from. The first PR. So the first PR has to merged fir…

One way I've solved this is to open the second PR against the branch for the first PR, and explain what's going on in the PR message. Then, once the first PR is merged, update the second PR to be against the real target branch.

This works better with a system that requires the PR owner to actually hit merge (so it's not accidentally merged into the first PR's branch) and where approvals stick across PR changes (which is undesirable in general, though).

Re: Stacked Diffs versus Pull Requests

#60

Post by Ajedi32 »

dom96 wrote: post #47
Ajedi32 wrote: post #43

Earlier quoted context omitted.

But what commit do you run the CI against? If you run it against a commit that's stacked on top of 3-4 other unreviewed changes, then the CI will include errors from all those other commits, not just yours.

You don't stack against random commits. Only commits that have passed tests before and so are on master, or your own commits.

That's not what the article says:

> Yang sees that the Diff for the “fix” is out for review. Yang uses the Phabricator command line tool to patch that commit on top of master. This means that it’s not a branch. It’s just a throwaway local commit. Yang then starts working on the first change. Yang submits a Diff for review from the command line. Later, the “fix” has changed, so Yang drops the patch of the old version from the Git history and patches in the updated one via interactive rebase.

Post reply on HN