Hacker News Reader

Comment by rubiquity | Hacker News Reader

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.

View on HN

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.

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.

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

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.

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.

> the clean merge that actually results in a logical bug

Theoretically possible, Probability < 3%. Besides, if master has tests and your branch has a test for the branch's feature, and you still have this problem, then maybe the 2 developers are overlapping so much that they should pair.

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

This is how I prefer to handle PRs, too. `git merge --squash pr-branch`. Prevents the history from turning into an indecipherable tangle of branches, and prevents tons of "fixing typo," "code review feedback," "more code review feedback," "adding back file" kinds of commits from taking over the history.

We are on GH, it's quite annoying that they provide 0 support for rebase/squash style PR application. http://stackoverflow.com/q/27974175/1366219

You can rebase commits in your feature branch as well and clean up the history. Having separate and small commits also helps in blaming and looking at the history do figure out why a certain change is in there.

If you squash all the commits you probably should summarize all the commits into that single commit message which is also work if done properly.

In the end it's preference and how your developers create commits and documment them.

Yes this is what I advocate doing as well in my post above.

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

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.

That's how we build using Jenkins - it's a nice feature, but it doesn't retrigger when the target branch changes. So, while it's useful so far as that it's better than simply building the branch, it doesn't ensure that "what you tested is what you get."

In the Rust project, the merging itself is automated and the merge bot (bors) runs the test after having effected the merge. The merging is attempted after the branch has passed review.

That doesn't preclude running e.g. travis to get the branch's own status before the review happens.

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

If you use the Travis CI integration, it actually tests the result of merging a PR, rather than the branch being PRed. This seems to solve a lot of problems on projects with thorough testing.

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

I disagree still. Fundamentally, I agree that it is important. I'd go further and say that these two diffs should be similar. If they are dissimilar, then there should be a red flag.

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

Yea same here. I stopped using github's PR for similar reasons since I always rebase onto master locally and compare that diff. Seems like the bitbucket feature is emulating this which sounds like a great improvement to me.

Reply on news.ycombinator.com