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

141–150 of 167 posts

Re: A better pull request

#141

Post by robotfelix »

sytse wrote: post #135

GitLab B.V. CEO here. The proposed diff is interesting and I love the way they explained the idea. During the development of GitLab (over 10.000 commits) we never experienced the problem that was mentioned. Maybe it is because our codebase contains many tests. One related idea that a GitLab user proposed is not testing the feature branch but testing the merge of the feature branch into master. This would require rete…

The idea of testing the merge commit is a great one. Even if the test suite is able to catch the 'logical conflict' mentioned in the article, current solutions will only catch the conflict after it's merged into master.

Of course, it's not too hard for the person responsible for merging the pull request to replicate this themselves (branch off master, merge in feature branch, run tests).

Re: A better pull request

#142

Post by porker »

oneeyedpigeon wrote: post #46
stephenr wrote: post #43

Earlier quoted context omitted.

I'm pretty sure it's wanting to be one of the 'cool kids' that pulls people to GitHub. If your decision about where to host your code repos is decided by how pretty the webpage to create pull-requests is, the wrong people are making decisions such in your business.

The differences aren't as superficial as you suggest, in my experience. GitHub is just far more polished. Take commit history as an example: bitbucket makes it uncommonly awkward to step through a series of commit diffs for a given file, whilst github makes it a bit easier (although still not perfect - am I the only one who wants this feature?). Nicer looking doesn't always mean more usable, but there's often a corre…

> bitbucket makes it uncommonly awkward to step through a series of commit diffs for a given file, whilst github makes it a bit easier (although still not perfect - am I the only one who wants this feature?)

I want it too :) A time-machine style forward & back, showing changes to a file.

Re: A better pull request

#143

Post by nfm »

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

The merge example that introduced a logical bug in the article was only visible in the diff because the bug occurred within 3 lines of the added line in the feature branch.

The best way to avoid this issue is automated testing. The second best way is to crack open the file itself, and review the entirety of any functions that changed. Even that approach assumes your encapsulation is nice and you're not introducing issues based on global state though.

While the BitBucket diff is better than nothing (and better than GitHub/GitLab), it's not sufficient to avoid these kinds of issues entirely.

Re: A better pull request

#144

Post by orbitur »

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.

Wow, the first thing I do when creating a new repo is disable fast-forwarding, especially for large teams.

The merge commit is the quickest way to see all the changes that came in from a branch, and if you do branching right, all the changes related to a particular feature by one developer.

Also, merge commits are much easier to roll back, no matter how rarely you need it.

Re: A better pull request

#145

Post by azakai »

ploxiln wrote: post #136
azakai wrote: post #117

Something doesn't add up for me. I constantly push the github "merge" button, and I've never, ever, seen an error AFTER I push it. It it wasn't checking for mergeability on actually current master, that seems surprising. Is the issue that the button knows if a pull will merge ok, because it does test on current master, but the diff shown in the UI is different from that, and not on current master, and just on the old…

Github does check if it's able to merge to the head of the intended branch automatically, and enable or disable the merge button. But the point of the article is that the context shown in the files (diff) view on github is that of the common ancestor, and doesn't include later changes to the branch being merged into. In addition to not showing merge conflicts if they exist (even though it disables the merge button),…

I think I get it now, thanks. My confusion was I didn't realize that the github merge button is "smarter" and more recent than the diff being show. I always assumed they were in sync, which is definitely surprising.

Re: A better pull request

#146

Post by cpeterso »

orbitur wrote: post #144
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.

Wow, the first thing I do when creating a new repo is disable fast-forwarding, especially for large teams. The merge commit is the quickest way to see all the changes that came in from a branch, and if you do branching right, all the changes related to a particular feature by one developer. Also, merge commits are much easier to roll back, no matter how rarely you need it.

Another option, used by large teams at Facebook, is to only use fast-forwarding but squashes all branch commits. You can still roll-back a merge, but your master's revision history is still linear (for what that's worth).

Re: A better pull request

#147

Post by cpeterso »

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.

That's a good point. The diffs would be more useful both the feature branch's changes (Bob's changes in the article's example) and master's diverged changes were highlighted (in different colors):

https://bitbucket.org/tpettersen/abpr/pull-request/2/fixed-a...

Re: A better pull request

#148

Post by evmar »

azakai wrote: post #133
kannonboy wrote: post #128

Earlier quoted context omitted.

From my experience, Github does attempt a merge commit behind the scenes and indicates that a PR can't be merged via the UI if there are conflicts. However, it still just shows the diff from the merge-base to the tip of your feature branch, which won't include any changes that have been added to master since the branch point.

I see. So the button status is more up to date than the diff, and I suppose it is rare enough that the two are out of sync, that people just don't see it much? Makes sense I guess.

The merge button status is always in sync with the diff. It is disabled if the diff won't merge correctly.

To answer your earlier question more directly: the diff that is shown is a diff against an earlier master. (It's in fact a diff against the point where the branch forked off of master.)

Re: A better pull request

#149

Post by sytse »

robotfelix wrote: post #141
sytse wrote: post #135

GitLab B.V. CEO here. The proposed diff is interesting and I love the way they explained the idea. During the development of GitLab (over 10.000 commits) we never experienced the problem that was mentioned. Maybe it is because our codebase contains many tests. One related idea that a GitLab user proposed is not testing the feature branch but testing the merge of the feature branch into master. This would require rete…

The idea of testing the merge commit is a great one. Even if the test suite is able to catch the 'logical conflict' mentioned in the article, current solutions will only catch the conflict after it's merged into master. Of course, it's not too hard for the person responsible for merging the pull request to replicate this themselves (branch off master, merge in feature branch, run tests).

Thanks! Indeed you can replicate it yourself but we like to automate as much as possible.

Re: A better pull request

#150

Post by sytse »

z-e-r-o wrote: post #82
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?

In my case, OS X has the latest git, while Windows is stuck on 1.9, and my git folders get corrupted if I do a push on OS X and simply do a status on Windows. I guess something has changed between 1.x and 2.x but this corruption is really not something which should ever happen in a VCS. My solution is to use Gitlab instead of Dropbox only local repos.

GitLab B.V. CEO here, thanks for using us!
Post reply on HN