A better pull request • Hacker News Board

Re: A better pull request

#61

Post by jMyles » Thu, Jan 22, 2015, 5:51 PM UTC

jessaustin wrote: post #24

puls wrote: post #18

This is so telling: GitHub had a blog post yesterday[1] about pull requests and now Atlassian does today with a very similar title. The one from GitHub was about social dynamics and how to work together better, while the one from Atlassian is about technical minutiae. Having used both GitHub and Stash, the difference in focus between the two companies comes across plainly, and these two blog posts only back it up. [1…

Atlassian are talking about a real problem here, which problem they appear to have solved. Working code is usually superior to social convention (rebase, etc.); that's why we're using git (or possibly other DVCSs) instead of RCS, etc.

Not sure I agree. In either case, both a social convention and an expected response from the executed command are necessary because both the "double dot" and "triple dot" logic are necessary to truly understand the implications of a complex pull request.

In the case of Atlassian's solution, the "merged" social convention will have to be use to first use and personally verify the "triple dot" findings in order to review that the change to the branch is, in isolation, as expected - before proceeding through the "double dot" flow that now characterizes the pull request feature.

For Github, it's simply the inverse.

Either way, both are relatively easy to achieve with the command line or a diff tool like Meld or LiClipse.

Re: A better pull request

#62

Post by saraid216 » Thu, Jan 22, 2015, 5:52 PM UTC

wzsddtc wrote: post #54

jackweirdy wrote: post #48

Earlier quoted context omitted.

If it’s personal stuff, why do you need an upstream? Assuming it’s for redundancy, why not just copy the folders as part of a normal backup?

The benefit that you don't need a git server and can have the code running on different machines! :)

Wait, wait, wait.

You mean the version control system would be... distributed!?

Re: A better pull request

#63

Post by taeric » Thu, Jan 22, 2015, 5:53 PM UTC

maaaats wrote: post #23

hk__2 wrote: post #12

Earlier quoted context omitted.

A common practice is for pull requesters to rebase their branch on the upstream branch to avoid these conflicts.

But if the master/upstream is changed, the feature branch must be updated again (manually).

There is no magic to avoid this problem. If you are trying to get changes into a repository that is constantly changing with no good isolation of concerns, then expect pain.

Otherwise, merges really are better from most perspectives. I can trust people doing pull requests to have tested their code. Looking at the log, I can get an idea of whether or not their code was tested with someone elses. Something I can not do with a pure "rebase and push" mentality.

Re: A better pull request

#64

Post by jMyles » Thu, Jan 22, 2015, 5:54 PM UTC

city41 wrote: post #14

I agree this is a better way to look at pull requests, but I'm surprised it's as much of a problem as the post implies. For us, whenever we do a pull request, we always rebase against the latest master first. That can be a cat and mouse game, but it usually isn't. Our repos don't change that rapidly.

But again, the age-old response to this flow is that it creates an untrue version history. This is great if cleanliness is your exclusive priority, but if you find that you need to actually try to jump into the mind of the person who made the commit (especially if it's someone you've never met and with whose habits you aren't familiar), it's impossible to actually follow along and recreate what really happened.

The rebase strategy and merge strategy both have their places - neither is fit for "whenever we do a pull request" for all people everywhere.

Re: A better pull request

#65

Post by bhuga » Thu, Jan 22, 2015, 5:57 PM UTC

kannonboy wrote: post #59

bhuga wrote: post #50

Wouldn't this result in an ever-growing diff? You'd see everything that's gone into master since the branch last rebased/merged master as "changed". When I think about a branch, I want to see changes unique to it, not what's going on in the rest of the codebase. Showing merge conflicts inline like that is pretty cool, though.

In Bitbucket's implementation you still only see the changes that are unique to your branch, but they're diff'd against the current tip of master rather than the merge base.

That makes sense. Thanks for clarifying.

Re: A better pull request

#66

Post by taeric » Thu, Jan 22, 2015, 5:57 PM UTC

serve_yay wrote: post #35

rubiquity wrote: post #33

Solution: Don't ever merge branches using a web GUI. Do the merge locally and then push to master. For the past couple of years I've only used GitHub's web-based PR tool for code discussion/peer review. Don't ever click that "merge" button. Another reason I hate the merge button: It creates an extra commit solely for the merge.

Still, TFA raises a good point, which is that the diff should be between the feature branch and current master, not master at the time the branch was created.

I disagree. It should be both. Specifically because what was tested and run by the person sending in the request was the diff to the master at the time the branch was created.

Please let that sink in. Nobody ever tested the diff that this is going to be showing to the user.

Re: A better pull request

#67

Post by taeric » Thu, Jan 22, 2015, 6:01 PM UTC

kannonboy wrote: post #59

bhuga wrote: post #50

Wouldn't this result in an ever-growing diff? You'd see everything that's gone into master since the branch last rebased/merged master as "changed". When I think about a branch, I want to see changes unique to it, not what's going on in the rest of the codebase. Showing merge conflicts inline like that is pretty cool, though.

In Bitbucket's implementation you still only see the changes that are unique to your branch, but they're diff'd against the current tip of master rather than the merge base.

I've said this in another thread, but I feel it really needs repeating.

The diff that bitbucket is showing you is one that has not been tested. That is, the diff against the merge base shows what the requester did and tested. The diff against the current tip, shows what will be the result.

I fully agree that it is important to bear that in mind. But, this is all the more reason for the merge to be done by another party before committing/pushing.

Consider, if you send a pull request for the kernel, Linus will do a local merge, then test, then push. In all of these gui apps, the final merged code is just there. No last second "fails sanity test, so not really going to merge."

Re: A better pull request

#68

Post by efuquen » Thu, Jan 22, 2015, 6:09 PM UTC

rubiquity wrote: post #33

Solution: Don't ever merge branches using a web GUI. Do the merge locally and then push to master. For the past couple of years I've only used GitHub's web-based PR tool for code discussion/peer review. Don't ever click that "merge" button. Another reason I hate the merge button: It creates an extra commit solely for the merge.

That does not solve what I consider the biggest problem, the clean merge that actually results in a logical bug. Without being able to review the diff between the tip of master that will happen with your solution regardless.

EDIT: currently I always rebase our feature branches before submitting the PR to try and mitigate this and make sure review happens quickly after that. it doesn't fully solve the problem but it's the best flow I've found.

Re: A better pull request

#69

Post by mkozlows » Thu, Jan 22, 2015, 6:11 PM UTC

taeric wrote: post #66

serve_yay wrote: post #35

Earlier quoted context omitted.

Still, TFA raises a good point, which is that the diff should be between the feature branch and current master, not master at the time the branch was created.

I disagree. It should be both. Specifically because what was tested and run by the person sending in the request was the diff to the master at the time the branch was created. Please let that sink in. Nobody ever tested the diff that this is going to be showing to the user.

Yes, but this is the diff that is going to happen if it's merged, so it's what reviewers should be looking at.

Re: A better pull request

#70

Post by iopq » Thu, Jan 22, 2015, 6:13 PM UTC

mkinsella wrote: post #27

tootie wrote: post #25

Earlier quoted context omitted.

It speaks to github's prominence in the open source community and Atlassians's prominence in professional/enterprise environments.

Are there that many non-enterprise companies using Bitbucket over Github? Most startups and mid-sized companies I know use Github for private repo hosting.

Tons of startups that you don't even know about use Bitbucket for their free private repos. When you have a team of 1-5 people there is no reason to pay for Github.