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

131–140 of 167 posts

Re: A better pull request

#131

Post by kannonboy »

mindprince wrote: post #112

When I first encountered this behavior on BitBucket, I spent a lot of time reading the man pages and searching the internet trying to find out which option were they providing to `git diff` to get this diff output but couldn't find anything. Looks like there is indeed no such option. It would be awesome if git had something like `git diff branch-name --merge-commit-diff`.

One of the Bitbucket engineers put a one-liner in the comments on the post that will generate diff-like output similar to what you're describing: https://developer.atlassian.com/blog/2015/01/a-better-pull-r...

Re: A better pull request

#132

Post by rubiquity »

natrius wrote: post #85
rubiquity wrote: post #76

Earlier quoted context omitted.

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

You can rebase without squashing to get the benefits of both approaches.

Hah! Great point. I always forget that because I do so very rarely.

Re: A better pull request

#133

Post by azakai »

kannonboy wrote: post #128
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…

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.

Re: A better pull request

#134

Post by cataphract »

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

That is not necessarily true. The test results (e.g. travis, but you can use for instance Jenkins for your Github repository) shown next to the merge button are the result of a merge between the pull request branch and the branch you want to merge into. That's why when you do an ls-remote, you will see something like:

c3ca093c99495d0ddbb3197c14e0cdf514266392 refs/pull/2/head 0f5993828108bd2960713dc34a9ac7bef6dbc653 refs/pull/2/merge

one is the head of the pull request, the other is the result of the merge that you will end up with. Though I'm not sure Github will trigger another build when the target branch moves.

Re: A better pull request

#135

Post by sytse »

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 retesting of all feature branches on every commit to master. We're open to proposals to add this to GitLab CI. But so far we didn't experience this problem frequently.

Re: A better pull request

#136

Post by ploxiln »

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), it also hides logic conflicts. These are situations where a clean automatic merge is possible, but the result will be incorrect logic due to the slightly distant interaction of those not-shown later changes. The simplest example of this was in the blog post.

All that said, even the bitbucket diff view doesn't show you logic conflicts caused by changes in completely different files, or otherwise more-distant. Nothing really does, it would be too much.

So, in an ideal world, you thoroughly test the merged result (in addition to the feature branch itself) before actually merging into the upstream branch.

Re: A better pull request

#137

Post by mindprince »

kannonboy wrote: post #131
mindprince wrote: post #112

When I first encountered this behavior on BitBucket, I spent a lot of time reading the man pages and searching the internet trying to find out which option were they providing to `git diff` to get this diff output but couldn't find anything. Looks like there is indeed no such option. It would be awesome if git had something like `git diff branch-name --merge-commit-diff`.

One of the Bitbucket engineers put a one-liner in the comments on the post that will generate diff-like output similar to what you're describing: https://developer.atlassian.com/blog/2015/01/a-better-pull-r...

Yup, that was me who asked the question there. :-)

Re: A better pull request

#138

Post by nnutter »

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…

As others have said, GitHub does produce a merge commit for the PR. You can checkout that commit like so,

git fetch origin +refs/pull/$N/merge && git checkout FETCH_HEAD

However, in my experience I have seen both cases where GitHub makes new merge commits when the receiving branch changes (current master) and where it does not. Due to externalities in our test suite we sometimes have "current master" break and when we push a fix we sometimes see PRs go green and sometimes they do not. I haven't figured out why we see the two different behaviors.

Re: A better pull request

#139

Post by ci5er »

vezzy-fnord wrote: post #47
fenomas wrote: post #7

Seems quite sensible. This, some commentary on forking[1] I ran across the other day, and the way SourceTree is miles beyond github's client, all make me suspect I should be at least trying out bitbucket. But since literally everything I collaborate with is on github, I've not gotten around to it. Am I missing out, or does bitbucket have its own weak spots? [1] http://zbowling.github.io/blog/2011/11/25/github/

Personally I enjoy Bitbucket for two reasons: a) free private repos mean I can use it as a remote backup for my projects, since I'm of the philosophy that you shouldn't be talking too much until you have at least a 0.1 version to show, b) no gamification and distractions. The gamification and awkward "social coding" environment that GitHub provides just feels forced to me. It's distracting from your work and it makes…

I sometimes do quick patches of things that get in my way. Sometimes I do pull requests on github. Sometimes (usually?) I don't. And I use AWS, Google, Drop Box and Github to hold things for me in the face of (yet another) hard drive crash.

I like git well enough. Github seems nice. What is this gamification of which you speak? I'm not saying that it isn't staring me in the face, but I'm probably looking right past it.

Re: A better pull request

#140

Post by nirvdrum »

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…

Unsurprisingly, this is very much a personal choice. I vastly prefer BitBucket's issue tracker. I love their side-by-side diff (somewhat recently added to GitHub). I love that they don't try to force this 72 char commit message on me and seem to handle it rather nicely via hover text. And I very much liked that they didn't support emoji and inline GIFs (since implemented, lamentably).

The more GitHub seems to adjust itself, the less I enjoy it. The more BitBucket seems to try to copy GitHub, the less I enjoy it. At the end of the day, I probably want GitHub circa 2008. I find when they really became opinionated about things to be the inflection point about whether they pushed things that were truly more usable.

That's not to say your view of things is wrong for you. But I don't think it's clear that "in this case, it definitely bears out".

Post reply on HN