Hacker News Reader

Comment by azakai | Hacker News Reader

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 older commit? If so, I think this could have been clarified better in the post.

Even so, this means that sometimes the button should be red - can't merge, has conflicts - while the diff as shown looks fine. I've never seen that happen. So something is puzzling here.

View on HN

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.

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.

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

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.

Reply on news.ycombinator.com