5 ms·

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

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.

efuquen 12y ago
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.
- nfm 12y ago
  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.
- ajross 12y ago
  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.
- rubiquity 12y ago
  > 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.
  - natrius 12y ago
    You can rebase without squashing to get the benefits of both approaches.
    - rubiquity 12y ago
      Hah! Great point. I always forget that because I do so very rarely.
- aragot 12y ago
  > 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.
serve_yay 12y ago
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.
- taeric 12y ago
  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.
  - mkozlows 12y ago
    Yes, but this is the diff that is going to happen if it's merged, so it's what reviewers should be looking at.
    - taeric 12y ago
      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.
  - cpeterso 12y ago
    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-bug-that-would-charge-2x/diff https://bitbucket.org/tpettersen/abpr/pull-request/2/fixed-a...
  - bri3d 12y ago
    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).
sbilstein 12y ago
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.
orbitur 12y ago
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.
- cpeterso 12y ago
  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).
  - rubiquity 12y ago
    Yes this is what I advocate doing as well in my post above.
  - latkin 12y ago
    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 http://stackoverflow.com/q/27974175/1366219
    - rndstr 12y ago
      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.

efuquen 12y ago

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.

nfm 12y ago
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.
ajross 12y ago
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.
rubiquity 12y ago
> 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.
- natrius 12y ago
  You can rebase without squashing to get the benefits of both approaches.
  - rubiquity 12y ago
    Hah! Great point. I always forget that because I do so very rarely.
aragot 12y ago
> 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.

serve_yay 12y ago

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.

taeric 12y ago
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.
- mkozlows 12y ago
  Yes, but this is the diff that is going to happen if it's merged, so it's what reviewers should be looking at.
  - taeric 12y ago
    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.
- cpeterso 12y ago
  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-bug-that-would-charge-2x/diff https://bitbucket.org/tpettersen/abpr/pull-request/2/fixed-a...
- bri3d 12y ago
  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).

sbilstein 12y ago

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.

orbitur 12y ago

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.

cpeterso 12y ago
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).
- rubiquity 12y ago
  Yes this is what I advocate doing as well in my post above.
- latkin 12y ago
  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 http://stackoverflow.com/q/27974175/1366219
  - rndstr 12y ago
    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.