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

A better pull request

developer.atlassian.com

Post reply on HN
Print view View original

71–80 of 167 posts

Re: A better pull request

#71

Post by Jacqued »

taeric wrote: post #67
kannonboy wrote: post #59

Earlier quoted context omitted.

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 par…

However, if per convention you state that a branch going under review must always be rebased to the latest commit of [your main branch], you will be reviewing all at once the code that will effectively be merged (by a simple fast-forward), and the code that the requester tested.

It seems to me that this is the safest method as, when you add an automated testing tool to the mix, it's pretty much guaranteed that you cannot break the main branch when merging a PR.

I think rebase is not advertised enough, but it's the _de facto_ solution to most of these kinds of problems.

Re: A better pull request

#72

Post by tracker1 »

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! :)

FYI, you really don't need a git server, just an SSH host you have filesystem access to.

Re: A better pull request

#73

Post by ajross »

efuquen wrote: post #68
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'…

Why would you not be able to review the diff with a local tree? If anything that's much easier in a local environment.

The GUIs by nature hide detail: the pull request becomes a thing to be "displayed" instead of "explored". It's a problem that requires careful attention to detail. I don't know that I actually disagree with Atlassian's decision, but the fact that it had to be made isn't evidence that it's the "right" solution either.

Re: A better pull request

#74

Post by arnarbi »

jackweirdy wrote: post #48
baldfat wrote: post #44

Earlier quoted context omitted.

Personal use = BitBucket WHY? Free private repos unlimited in number of repos only limited on how many can access them. So all my .config files and anything private goes to BitBucket. Great product for my own use.

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?

Because even though it's personal stuff, it's shared between many computers. For people that live in VCSs all day, it's just more convenient (history, branches, etc.) than e.g. storing it on Dropbox or similar.

Re: A better pull request

#75

Post by tnorthcutt »

Alupis wrote: post #30
mkinsella wrote: post #27

Earlier quoted context omitted.

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.

Bitbucket provides free private repos, and for smaller development teams and/or companies not wanting to deal with the cost of Github (which does admittedly grow exponentially the more repos you require), Bitbucket is a fine choice. When we were making the decision at my company, we went with Github because the dev team cared about having the little green squares show up on the "activity" chart for their account's...…

which does admittedly grow exponentially the more repos you require

Just to clarify, GH's pricing doesn't actually grow exponentially. The per repo price gets lower the more you pay for:

5 private repos: $7

10 private repos: $12

20 private repos: $22

50 private repos: $50

Re: A better pull request

#76

Post by rubiquity »

efuquen wrote: post #68
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'…

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

I rebase after the PR has been discussed and "approved" for merging. I feel having the individual commits during discussion time are useful for context, so long as team members are earnest enough to use them. Usually good commit messages can answer every "Why did you do it this way?" question before it even gets asked.

Re: A better pull request

#77

Post by bri3d »

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.

But once you click the "Merge" button in GitHub, nobody has ever seen or tested the new state of master.

I'd rather see the diff that's going to go into master (tested or not) than see a diff that effectively means nothing (as it will never be applied to anything).

Re: A better pull request

#78

Post by city41 »

jMyles wrote: post #64
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 r…

I don't follow because the rebase is done by the person who wrote the code and is proposing the pull request. They rewrite their local history, but that doesn't seem to relevant. They still have to get their code to work with the latest master and remain in control of it. When the PR goes through, it can be a standard merge to maintain history.

Re: A better pull request

#79

Post by taeric »

Jacqued wrote: post #71
taeric wrote: post #67

Earlier quoted context omitted.

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 par…

However, if per convention you state that a branch going under review must always be rebased to the latest commit of [your main branch], you will be reviewing all at once the code that will effectively be merged (by a simple fast-forward), and the code that the requester tested. It seems to me that this is the safest method as, when you add an automated testing tool to the mix, it's pretty much guaranteed that you ca…

Close. Under this convention, somebody has to do a final test before pushing the merge commit. It can be a simple sanity test, not a full blown integration test. But somewhere somebody at least did a compile check.

If it fails that test, then you push it back to the person doing the work saying so and they need to fix it.

Re: A better pull request

#80

Post by taeric »

bri3d wrote: post #77
taeric wrote: post #66

Earlier quoted context omitted.

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.

But once you click the "Merge" button in GitHub, nobody has ever seen or tested the new state of master. I'd rather see the diff that's going to go into master (tested or not) than see a diff that effectively means nothing (as it will never be applied to anything).

This is an argument against github and bitbucket style pull requests, though. And is basically my point.
Post reply on HN