12 ms·

A better pull request

479 points by kannonboy 12y ago

legomaster 12y ago
It seems like the example of double fixing the calculation is a great reminder of the value of unit tests. Even if both commits added a unit test in slightly different ways so they didn't conflict, you'd just end up with a failing build and know something got screwed up.
- Revell 12y ago
  But still the test would only fail after the merge, whereas you'd want to catch this before.
  - jbrooksuk 12y ago
    On GitHub you can have tests run before you've merged, so you know whether it's safe or not.
    - Revell 12y ago
      Fair point!
    - longlivegnu 12y ago
      Can't you run unit tests before you merge on an Atlassian service too?
      - jbrooksuk 12y ago
        I'm not sure about BitBucket as I don't use it, but I expect so.
      - legomaster 12y ago
        You can, though you doing get to do it by using the ref/pull/## trick. That's just a connivence, you can get CI to do about anything.
    - deleted 12y ago
      [deleted]
    - russell_h 12y ago
      Don't the tests just run on the branch before the merge? If so, you wouldn't actually see the failure until after you merge.
      - caust1c 12y ago
        You can do either. Or both if you really want. $ git ls-remote From https://github.com/cyaninc/git-fat.git 4c86c39bb5fca55a692d11680ed62fd5cb183921 refs/pull/14/head cb850f9d0bd09f8bb903e1c5959cef24ceb51695 refs/pull/14/merge The merge ref is what the code would be if you merged it.
      - masklinn 12y ago
        > Don't the tests just run on the branch before the merge? The merge button is only green if github could generate a merge commit, and that merge commit is made available. You can run your tests on either the branch before merge or the branch after merge, as you prefer. You may want to do both and have the former block the latter: the merged head is going to change (and require a re-test) any time the target branch gets a new commit, no point in wasting cycle if the branch's own tests don't pass in the first place.
    - GolfyMcG 12y ago
      Yeah - we run both the pull request and the master + branch merge every time.
    - GolfyMcG 12y ago
      Yeah - we run both the pull request and the master + branch merge every time.
    - alexsop2 12y ago
      I'm guessing this is because a merge commit is created before the merge is fully executed.
brodney 12y ago
I really like the idea of merge conflicts being resolved in the open in a pull request, instead of the pull requester working in seclusion. For those of us with projects on GitHub, is there any way to replicate this behavior there?
- hk__2 12y ago
  A common practice is for pull requesters to rebase their branch on the upstream branch to avoid these conflicts.
  - maaaats 12y ago
    But if the master/upstream is changed, the feature branch must be updated again (manually).
    - taeric 12y ago
      There is no magic to avoid this problem. If you are trying to get changes into a repository that is constantly changing with no good isolation of concerns, then expect pain. Otherwise, merges really are better from most perspectives. I can trust people doing pull requests to have tested their code. Looking at the log, I can get an idea of whether or not their code was tested with someone elses. Something I can not do with a pure "rebase and push" mentality.
fenomas 12y ago
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/ http://zbowling.github.io/blog/2011/11/25/github/
- vezzy-fnord 12y ago
  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 you focus on pointless minutiae, but worse - it makes other people apply unnecessary weight on the significance of how "active" and "competitive" your profile is. Look at all the people who legitimately believe GitHub should be the be-all-end-all spot for software development, replacing your resume. Look at how Git the DVCS is becoming synonymous with GitHub the (centralized) platform. With Bitbucket, I have none of that. I can focus on my project without any intervention from the peanut gallery, without all the pointless competition and addictive side jobs that GitHub conditions you into partaking in. Also, I always found GitHub's issue tracker to be rather underdeveloped. I think it speaks about the "quick hack" culture of software development in general. Oh, and I think Mercurial needs more attention. I use both hg and git.
  - ci5er 12y ago
    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.
  - 12y ago
bhuga 12y ago
Wouldn't this result in an ever-growing diff? You'd see everything that's gone into master since the branch last rebased/merged master as "changed". When I think about a branch, I want to see changes unique to it, not what's going on in the rest of the codebase. Showing merge conflicts inline like that is pretty cool, though.
- kannonboy 12y ago
  In Bitbucket's implementation you still only see the changes that are unique to your branch, but they're diff'd against the current tip of master rather than the merge base.
  - bhuga 12y ago
    That makes sense. Thanks for clarifying.
  - taeric 12y ago
    I've said this in another thread, but I feel it really needs repeating. The diff that bitbucket is showing you is one that has not been tested. That is, the diff against the merge base shows what the requester did and tested. The diff against the current tip, shows what will be the result. I fully agree that it is important to bear that in mind. But, this is all the more reason for the merge to be done by another party before committing/pushing. Consider, if you send a pull request for the kernel, Linus will do a local merge, then test, then push. In all of these gui apps, the final merged code is just there. No last second "fails sanity test, so not really going to merge."
    - Jacqued 12y ago
      However, if per convention you state that a branch going under review must always be rebased to the latest commit of [your main branch], you will be reviewing all at once the code that will effectively be merged (by a simple fast-forward), and the code that the requester tested. It seems to me that this is the safest method as, when you add an automated testing tool to the mix, it's pretty much guaranteed that you cannot break the main branch when merging a PR. I think rebase is not advertised enough, but it's the _de facto_ solution to most of these kinds of problems.
b6fan 12y ago
I think both the old and new diff worth a look. Maybe showing them using different colors is a good idea. Phabricator uses light colors to show changes not introduced in this patch but a rebase against master [1]. The experience is pretty good if the workflow is to always fast-forward and the patch author does rebase. Changes introduced by other people are in light green and red. Duplicated-line issue can be obviously seen. [1]: https://secure.phabricator.com/book/phabricator/article/differential_faq/#what-do-the-very-light-g https://secure.phabricator.com/book/phabricator/article/diff...
bsimpson 12y ago
It would make me happy if SublimeGit was better at showing merge conflicts. I feel like I only know they exist when I'm surprised that my build is now broken, and my only resolution is to Find in Project for `>>>>>` and hope I catch all of them.
- pc86 12y ago
  I've never used SublimeGit (or heard of it before today, to be honest), but if that's how you fix merge conflicts it sounds like a broken product.
- jbrooksuk 12y ago
  It'd make me happy if SublimeGit fixed the pull issues on OSX, let alone any update :(
- mynameisvlad 12y ago
  git status will tell you which files are conflicted.
mborch 12y ago
Interestingly, only Github manages to syntax highlight the diff.
city41 12y ago
I agree this is a better way to look at pull requests, but I'm surprised it's as much of a problem as the post implies. For us, whenever we do a pull request, we always rebase against the latest master first. That can be a cat and mouse game, but it usually isn't. Our repos don't change that rapidly.
- jMyles 12y ago
  But again, the age-old response to this flow is that it creates an untrue version history. This is great if cleanliness is your exclusive priority, but if you find that you need to actually try to jump into the mind of the person who made the commit (especially if it's someone you've never met and with whose habits you aren't familiar), it's impossible to actually follow along and recreate what really happened. The rebase strategy and merge strategy both have their places - neither is fit for "whenever we do a pull request" for all people everywhere.
  - city41 12y ago
    I don't follow because the rebase is done by the person who wrote the code and is proposing the pull request. They rewrite their local history, but that doesn't seem to relevant. They still have to get their code to work with the latest master and remain in control of it. When the PR goes through, it can be a standard merge to maintain history.
    - comex 12y ago
      True, but while that person probably tested each original commit as they wrote it (at least to the extent the application compiled and didn't completely blow up), they probably didn't do the same for each rebased commit every time they updated to master. If the resulting history turns out to be broken, it isn't the end of the world, but makes bisecting harder. I think rebasing is a good idea anyway, but it's a tradeoff.
      - city41 12y ago
        There's no perfect solution and all of this still requires a fair amount of vigilance to get it right. But I would still argue that if you are proposing a PR, you've done the work to verify your changes work, tests pass, etc. If you later need to rebase your PR, you still should run tests, smoke test, etc. Small issues can slip through, but on teams I've worked on, submitting a PR that is truly broken is really bad form. Sometimes that does mean submitting PRs is tedious, but that's just how it goes sometimes. Thankfully, it's not that often in my experience.
puls 12y ago
This is so telling: GitHub had a blog post yesterday[1] about pull requests and now Atlassian does today with a very similar title. The one from GitHub was about social dynamics and how to work together better, while the one from Atlassian is about technical minutiae. Having used both GitHub and Stash, the difference in focus between the two companies comes across plainly, and these two blog posts only back it up. [1]: https://github.com/blog/1943-how-to-write-the-perfect-pull-request https://github.com/blog/1943-how-to-write-the-perfect-pull-r...
- jessaustin 12y ago
  Atlassian are talking about a real problem here, which problem they appear to have solved. Working code is usually superior to social convention (rebase, etc.); that's why we're using git (or possibly other DVCSs) instead of RCS, etc.
  - jMyles 12y ago
    Not sure I agree. In either case, both a social convention and an expected response from the executed command are necessary because both the "double dot" and "triple dot" logic are necessary to truly understand the implications of a complex pull request. In the case of Atlassian's solution, the "merged" social convention will have to be use to first use and personally verify the "triple dot" findings in order to review that the change to the branch is, in isolation, as expected - before proceeding through the "double dot" flow that now characterizes the pull request feature. For Github, it's simply the inverse. Either way, both are relatively easy to achieve with the command line or a diff tool like Meld or LiClipse.
- tootie 12y ago
  It speaks to github's prominence in the open source community and Atlassians's prominence in professional/enterprise environments.
  - mkinsella 12y ago
    Are there that many non-enterprise companies using Bitbucket over Github? Most startups and mid-sized companies I know use Github for private repo hosting.
davvid 12y ago
Very interesting, I never considered looking at the conflicts that way. It might be helpful to allow looking at the conflicts in the diff3 conflict marker style. The advantage is that you get to see the common ancestor, which can be crucial to figuring out a tricky merge. git config --global merge.conflictstyle diff3 http://gitster.livejournal.com/25801.html http://gitster.livejournal.com/25801.html
rubiquity 12y ago
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.
Cub3 12y ago
Forgive my ignorance but wouldn't rebasing your current branch from master before opening a pull request solve this issue?
- kannonboy 12y ago
  That would solve the issue until master progresses again. The problem is that on many projects the master branch will progress during the lifetime of a pull request.
sytse 12y ago
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.
- robotfelix 12y ago
  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).
  - sytse 12y ago
    Thanks! Indeed you can replicate it yourself but we like to automate as much as possible.
- juriansluiman 12y ago
  It is exactly what Travis CI does for auto-PR testing on Github: they pull in master (or whatever base) and fetch the feature branch. They create the commit themselves and test the result. That way, you always get the tests based on the merged result which gives far more insights than tests on your (local) feature branch.
- lloeki 12y ago
  I hear GitLab EE has a 'rebase before merge' checkbox next to the 'merge' button. What I'd like to see is have it enforced, that is, the UI won't allow a merge if the MR has not been rebased onto master's tip. Enforcing this policy has saved us a few headaches, including the article's case which happened tree times on a project counting ~2000 commits. (BTW, we love GitLab, congrats on the new UI layout, which has rave reviews coming back to me mere hours after the upgrade)
stevebot 12y ago
Applying the God principle to this, there should never be merge conflicts. I should get notified that the file has changed before I even change it. That is how I imagine it would be if a God made it.
- SoftwareMaven 12y ago
  But at the time you modify it, the other file may not be modified. And, when your merge partner modifies it, she may be on an 18 flight to the South Pole with no internet access. Now, sure, you can say "well, God can get around that", but, unfortunately, there are real constraints that cannot be gotten around. Good design is how you deal with those constraints.
  - saraid216 12y ago
    Complete tangent: I've never heard the term "an 18 flight". What is that, and what is it from?
    - sah2ed 12y ago
      Perhaps adding "hour" fixes the sentence. " ... she may be on an 18 hour flight to the South Pole with no internet access."
- deleted 12y ago
  [deleted]
dominotw 12y ago
We run unit tests on jenkins on the merged branch( which github creates automatically) and set the commit status on PR. Shouldn't that take care of logic bugs?
- biot 12y ago
  If you have 100% code coverage and have solved the halting problem, yes.
erikb 12y ago
I can't really say why, but many things I've read from Atlassian/Bitbucket in recent months always had the feeling of B class (in contrast to A class). Even at the end of the article I still don't get the real problem that is solved. Commiting Mergeconflicts is not an advantage. And that logical error thingy I have never heard or seen before. Maybe because the people (who's opinion I care about) review commit diffs in detail and are able to rebase -i anything that looks bad?
- jjwiseman 12y ago
  Atlassian's bug tracker (Jira), wiki (Confluence) and CI system (Bamboo) all feel that way to me; B team, or even C. "Atlassian--we make software that you can eventually get to work in some fashion but will never please you in any way." Jira is almost OK, but still has that enterprise stank of trying to do too much and inserting itself too much in your workflow, instead of helping you and getting out of your way. Some people are less generous toward Jira than I am: "Next time I'm looking for a job I think use of @jira might be a deal breaker."--Peter Seibel[1]. I have no generosity toward Confluence, their wiki. Bad UI choices, a weird & inconsistent markup language that again becomes another thing to wrestle with indefinitely instead of unobtrusively helping you. I've had to use it at two jobs, and there won't be a third. Bamboo, their CI system, seems OK, but for some reason I end up debugging a lot of build failures having to do with leftover or broken state on the build machine, which seems like one of the first things a CI system should help you avoid. [1]https://twitter.com/peterseibel/status/451758184470835200 https://twitter.com/peterseibel/status/451758184470835200
  - erikb 12y ago
    There are so many very small things. Like Bitbucket adds a web-editor like Github, but the editor automatically replaces all new lines with \r\n and has no field for a commit message (corresponding bug not treated until today and only considered minor priority, maybe because most people use Windows anyway or what). Or the markup language that has the ability to highlight code but only in 3 or so languages. You don't even have the choice to switch to others. The wiki I have only used two or three times. Can't get warm with it either.
snowwrestler 12y ago
I'm surprised anyone would be willing to consider merging a branch into master that has not itself pulled the latest head from master. It puts the the person running the merge into the position of having to test someone else's code. It would seem to me that if a branch diffs against an older point in master, then the PR should be rejected as not properly tested. Merging into master should never create a merge conflict; the merge conflict should happen and be resolved on the feature branch first. Or what am I missing?
- forgottenpass 12y ago
  Or what am I missing? The linux kernel averages >5 patches per hour. Even with multi-teired maintanership, good luck getting everyone to base their patch sets on the latest upstream head. I know nobody other than the kernel actually uses git as a distributed system, but there are use cases where your ideal workflow can't actually exist.
- kannonboy 12y ago
  This is tricky to pull off on busy projects, as master is continually progressing and all open pull request branches would need to be constantly rebased. With the Bitbucket approach, you get to see the conflict in the pull request UI, but you should still resolve it locally on your branch before merging to master.
arpitverma007 12y ago
that was simple and awesome
jfaucett 12y ago
Ok this is nice. Of course, if you're just rebasing before reviewing and merging the PR into master it makes this pointless. Odd that they never mention this in the post, I was under the assumption that rebasing before creating the PR is standard practice, at least that's how we do it at work and on all the projects I've contributed to. EDIT: I've never used bitbucket, so maybe this is just a github community thing with the rebasing?
- firethief 12y ago
  That's a good (and standard) practice that solves the problem if your project never has more than one PR open at a time.
- Nemo157 12y ago
  For smaller projects that's fine, but if you have a lot of outstanding pull requests under discussion that's a lot of continual rebasing as they each get merged. (What if the two fee fixes in the article were done at the same time and reviewed by different project maintainers?)
  - jfaucett 12y ago
    That sounds really unorganized to me at least, I mean the linux kernel is large and there's only one guy at the end of the day merging all that stuff. I would think that in larger projects a better practice would be a distributed hierarchy of merges until a PR finally got submitted to master, not a massive blob of just send everything to master as a PR and let various maintainers merge whatever at will.
mindprince 12y ago
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`.
- kannonboy 12y ago
  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-request/#comment-1811819137 https://developer.atlassian.com/blog/2015/01/a-better-pull-r...
  - mindprince 12y ago
    Yup, that was me who asked the question there. :-)
azakai 12y ago
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.
- kannonboy 12y ago
  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.
  - azakai 12y ago
    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.
    - evmar 12y ago
      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.)
- ploxiln 12y ago
  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.
falsedan 12y ago
Surely the bug in logic would be caught by tests?
Too 12y ago
Something that bugs me with almost every source control GUI is that there is usually no way to spawn a three way diff after a merge has been done. They might support three way merge on conflict but bang, once you save you can not get that view back. They also often don't understand that no branch was the master branch so if you diff the merged revision (diffing a single revision doesn't make sense yes but guis usually present it this way and diff it with the last revision) then you get all changes that has happened on master since the branch was created, wtf?? To see what only the branch contributed you have to manually select two revisions, diff them and then select the other revision and diff.
ska 12y ago
Why hasn't the alice/master branch brought over all of the changes in master since branching, and tested them, before review and (eventual) merge?

479 points by kannonboy 12y ago

It seems like the example of double fixing the calculation is a great reminder of the value of unit tests. Even if both commits added a unit test in slightly different ways so they didn't conflict, you'd just end up with a failing build and know something got screwed up.

Revell 12y ago
But still the test would only fail after the merge, whereas you'd want to catch this before.
- jbrooksuk 12y ago
  On GitHub you can have tests run before you've merged, so you know whether it's safe or not.
  - Revell 12y ago
    Fair point!
  - longlivegnu 12y ago
    Can't you run unit tests before you merge on an Atlassian service too?
    - jbrooksuk 12y ago
      I'm not sure about BitBucket as I don't use it, but I expect so.
    - legomaster 12y ago
      You can, though you doing get to do it by using the ref/pull/## trick. That's just a connivence, you can get CI to do about anything.
  - deleted 12y ago
    [deleted]
  - russell_h 12y ago
    Don't the tests just run on the branch before the merge? If so, you wouldn't actually see the failure until after you merge.
    - caust1c 12y ago
      You can do either. Or both if you really want. $ git ls-remote From https://github.com/cyaninc/git-fat.git 4c86c39bb5fca55a692d11680ed62fd5cb183921 refs/pull/14/head cb850f9d0bd09f8bb903e1c5959cef24ceb51695 refs/pull/14/merge The merge ref is what the code would be if you merged it.
    - masklinn 12y ago
      > Don't the tests just run on the branch before the merge? The merge button is only green if github could generate a merge commit, and that merge commit is made available. You can run your tests on either the branch before merge or the branch after merge, as you prefer. You may want to do both and have the former block the latter: the merged head is going to change (and require a re-test) any time the target branch gets a new commit, no point in wasting cycle if the branch's own tests don't pass in the first place.
  - GolfyMcG 12y ago
    Yeah - we run both the pull request and the master + branch merge every time.
  - GolfyMcG 12y ago
    Yeah - we run both the pull request and the master + branch merge every time.
  - alexsop2 12y ago
    I'm guessing this is because a merge commit is created before the merge is fully executed.

brodney 12y ago

I really like the idea of merge conflicts being resolved in the open in a pull request, instead of the pull requester working in seclusion. For those of us with projects on GitHub, is there any way to replicate this behavior there?

hk__2 12y ago
A common practice is for pull requesters to rebase their branch on the upstream branch to avoid these conflicts.
- maaaats 12y ago
  But if the master/upstream is changed, the feature branch must be updated again (manually).
  - taeric 12y ago
    There is no magic to avoid this problem. If you are trying to get changes into a repository that is constantly changing with no good isolation of concerns, then expect pain. Otherwise, merges really are better from most perspectives. I can trust people doing pull requests to have tested their code. Looking at the log, I can get an idea of whether or not their code was tested with someone elses. Something I can not do with a pure "rebase and push" mentality.

fenomas 12y ago

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/ http://zbowling.github.io/blog/2011/11/25/github/

vezzy-fnord 12y ago
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 you focus on pointless minutiae, but worse - it makes other people apply unnecessary weight on the significance of how "active" and "competitive" your profile is. Look at all the people who legitimately believe GitHub should be the be-all-end-all spot for software development, replacing your resume. Look at how Git the DVCS is becoming synonymous with GitHub the (centralized) platform. With Bitbucket, I have none of that. I can focus on my project without any intervention from the peanut gallery, without all the pointless competition and addictive side jobs that GitHub conditions you into partaking in. Also, I always found GitHub's issue tracker to be rather underdeveloped. I think it speaks about the "quick hack" culture of software development in general. Oh, and I think Mercurial needs more attention. I use both hg and git.
- ci5er 12y ago
  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.
- 12y ago

bhuga 12y ago

Wouldn't this result in an ever-growing diff? You'd see everything that's gone into master since the branch last rebased/merged master as "changed". When I think about a branch, I want to see changes unique to it, not what's going on in the rest of the codebase. Showing merge conflicts inline like that is pretty cool, though.

kannonboy 12y ago
In Bitbucket's implementation you still only see the changes that are unique to your branch, but they're diff'd against the current tip of master rather than the merge base.
- bhuga 12y ago
  That makes sense. Thanks for clarifying.
- taeric 12y ago
  I've said this in another thread, but I feel it really needs repeating. The diff that bitbucket is showing you is one that has not been tested. That is, the diff against the merge base shows what the requester did and tested. The diff against the current tip, shows what will be the result. I fully agree that it is important to bear that in mind. But, this is all the more reason for the merge to be done by another party before committing/pushing. Consider, if you send a pull request for the kernel, Linus will do a local merge, then test, then push. In all of these gui apps, the final merged code is just there. No last second "fails sanity test, so not really going to merge."
  - Jacqued 12y ago
    However, if per convention you state that a branch going under review must always be rebased to the latest commit of [your main branch], you will be reviewing all at once the code that will effectively be merged (by a simple fast-forward), and the code that the requester tested. It seems to me that this is the safest method as, when you add an automated testing tool to the mix, it's pretty much guaranteed that you cannot break the main branch when merging a PR. I think rebase is not advertised enough, but it's the _de facto_ solution to most of these kinds of problems.

b6fan 12y ago

I think both the old and new diff worth a look. Maybe showing them using different colors is a good idea. Phabricator uses light colors to show changes not introduced in this patch but a rebase against master [1]. The experience is pretty good if the workflow is to always fast-forward and the patch author does rebase. Changes introduced by other people are in light green and red. Duplicated-line issue can be obviously seen. [1]: https://secure.phabricator.com/book/phabricator/article/differential_faq/#what-do-the-very-light-g https://secure.phabricator.com/book/phabricator/article/diff...

bsimpson 12y ago

It would make me happy if SublimeGit was better at showing merge conflicts. I feel like I only know they exist when I'm surprised that my build is now broken, and my only resolution is to Find in Project for `>>>>>` and hope I catch all of them.

pc86 12y ago
I've never used SublimeGit (or heard of it before today, to be honest), but if that's how you fix merge conflicts it sounds like a broken product.
jbrooksuk 12y ago
It'd make me happy if SublimeGit fixed the pull issues on OSX, let alone any update :(
mynameisvlad 12y ago
git status will tell you which files are conflicted.

mborch 12y ago

Interestingly, only Github manages to syntax highlight the diff.

city41 12y ago

I agree this is a better way to look at pull requests, but I'm surprised it's as much of a problem as the post implies. For us, whenever we do a pull request, we always rebase against the latest master first. That can be a cat and mouse game, but it usually isn't. Our repos don't change that rapidly.

jMyles 12y ago
But again, the age-old response to this flow is that it creates an untrue version history. This is great if cleanliness is your exclusive priority, but if you find that you need to actually try to jump into the mind of the person who made the commit (especially if it's someone you've never met and with whose habits you aren't familiar), it's impossible to actually follow along and recreate what really happened. The rebase strategy and merge strategy both have their places - neither is fit for "whenever we do a pull request" for all people everywhere.
- city41 12y ago
  I don't follow because the rebase is done by the person who wrote the code and is proposing the pull request. They rewrite their local history, but that doesn't seem to relevant. They still have to get their code to work with the latest master and remain in control of it. When the PR goes through, it can be a standard merge to maintain history.
  - comex 12y ago
    True, but while that person probably tested each original commit as they wrote it (at least to the extent the application compiled and didn't completely blow up), they probably didn't do the same for each rebased commit every time they updated to master. If the resulting history turns out to be broken, it isn't the end of the world, but makes bisecting harder. I think rebasing is a good idea anyway, but it's a tradeoff.
    - city41 12y ago
      There's no perfect solution and all of this still requires a fair amount of vigilance to get it right. But I would still argue that if you are proposing a PR, you've done the work to verify your changes work, tests pass, etc. If you later need to rebase your PR, you still should run tests, smoke test, etc. Small issues can slip through, but on teams I've worked on, submitting a PR that is truly broken is really bad form. Sometimes that does mean submitting PRs is tedious, but that's just how it goes sometimes. Thankfully, it's not that often in my experience.

puls 12y ago

This is so telling: GitHub had a blog post yesterday[1] about pull requests and now Atlassian does today with a very similar title. The one from GitHub was about social dynamics and how to work together better, while the one from Atlassian is about technical minutiae. Having used both GitHub and Stash, the difference in focus between the two companies comes across plainly, and these two blog posts only back it up. [1]: https://github.com/blog/1943-how-to-write-the-perfect-pull-request https://github.com/blog/1943-how-to-write-the-perfect-pull-r...

jessaustin 12y ago
Atlassian are talking about a real problem here, which problem they appear to have solved. Working code is usually superior to social convention (rebase, etc.); that's why we're using git (or possibly other DVCSs) instead of RCS, etc.
- jMyles 12y ago
  Not sure I agree. In either case, both a social convention and an expected response from the executed command are necessary because both the "double dot" and "triple dot" logic are necessary to truly understand the implications of a complex pull request. In the case of Atlassian's solution, the "merged" social convention will have to be use to first use and personally verify the "triple dot" findings in order to review that the change to the branch is, in isolation, as expected - before proceeding through the "double dot" flow that now characterizes the pull request feature. For Github, it's simply the inverse. Either way, both are relatively easy to achieve with the command line or a diff tool like Meld or LiClipse.
tootie 12y ago
It speaks to github's prominence in the open source community and Atlassians's prominence in professional/enterprise environments.
- mkinsella 12y ago
  Are there that many non-enterprise companies using Bitbucket over Github? Most startups and mid-sized companies I know use Github for private repo hosting.

davvid 12y ago

Very interesting, I never considered looking at the conflicts that way. It might be helpful to allow looking at the conflicts in the diff3 conflict marker style. The advantage is that you get to see the common ancestor, which can be crucial to figuring out a tricky merge. git config --global merge.conflictstyle diff3 http://gitster.livejournal.com/25801.html http://gitster.livejournal.com/25801.html

rubiquity 12y ago

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.

Cub3 12y ago

Forgive my ignorance but wouldn't rebasing your current branch from master before opening a pull request solve this issue?

kannonboy 12y ago
That would solve the issue until master progresses again. The problem is that on many projects the master branch will progress during the lifetime of a pull request.

sytse 12y ago

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.

robotfelix 12y ago
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).
- sytse 12y ago
  Thanks! Indeed you can replicate it yourself but we like to automate as much as possible.
juriansluiman 12y ago
It is exactly what Travis CI does for auto-PR testing on Github: they pull in master (or whatever base) and fetch the feature branch. They create the commit themselves and test the result. That way, you always get the tests based on the merged result which gives far more insights than tests on your (local) feature branch.
lloeki 12y ago
I hear GitLab EE has a 'rebase before merge' checkbox next to the 'merge' button. What I'd like to see is have it enforced, that is, the UI won't allow a merge if the MR has not been rebased onto master's tip. Enforcing this policy has saved us a few headaches, including the article's case which happened tree times on a project counting ~2000 commits. (BTW, we love GitLab, congrats on the new UI layout, which has rave reviews coming back to me mere hours after the upgrade)

stevebot 12y ago

Applying the God principle to this, there should never be merge conflicts. I should get notified that the file has changed before I even change it. That is how I imagine it would be if a God made it.

SoftwareMaven 12y ago
But at the time you modify it, the other file may not be modified. And, when your merge partner modifies it, she may be on an 18 flight to the South Pole with no internet access. Now, sure, you can say "well, God can get around that", but, unfortunately, there are real constraints that cannot be gotten around. Good design is how you deal with those constraints.
- saraid216 12y ago
  Complete tangent: I've never heard the term "an 18 flight". What is that, and what is it from?
  - sah2ed 12y ago
    Perhaps adding "hour" fixes the sentence. " ... she may be on an 18 hour flight to the South Pole with no internet access."
deleted 12y ago
[deleted]

dominotw 12y ago

We run unit tests on jenkins on the merged branch( which github creates automatically) and set the commit status on PR. Shouldn't that take care of logic bugs?

biot 12y ago
If you have 100% code coverage and have solved the halting problem, yes.

erikb 12y ago

I can't really say why, but many things I've read from Atlassian/Bitbucket in recent months always had the feeling of B class (in contrast to A class). Even at the end of the article I still don't get the real problem that is solved. Commiting Mergeconflicts is not an advantage. And that logical error thingy I have never heard or seen before. Maybe because the people (who's opinion I care about) review commit diffs in detail and are able to rebase -i anything that looks bad?

jjwiseman 12y ago
Atlassian's bug tracker (Jira), wiki (Confluence) and CI system (Bamboo) all feel that way to me; B team, or even C. "Atlassian--we make software that you can eventually get to work in some fashion but will never please you in any way." Jira is almost OK, but still has that enterprise stank of trying to do too much and inserting itself too much in your workflow, instead of helping you and getting out of your way. Some people are less generous toward Jira than I am: "Next time I'm looking for a job I think use of @jira might be a deal breaker."--Peter Seibel[1]. I have no generosity toward Confluence, their wiki. Bad UI choices, a weird & inconsistent markup language that again becomes another thing to wrestle with indefinitely instead of unobtrusively helping you. I've had to use it at two jobs, and there won't be a third. Bamboo, their CI system, seems OK, but for some reason I end up debugging a lot of build failures having to do with leftover or broken state on the build machine, which seems like one of the first things a CI system should help you avoid. [1]https://twitter.com/peterseibel/status/451758184470835200 https://twitter.com/peterseibel/status/451758184470835200
- erikb 12y ago
  There are so many very small things. Like Bitbucket adds a web-editor like Github, but the editor automatically replaces all new lines with \r\n and has no field for a commit message (corresponding bug not treated until today and only considered minor priority, maybe because most people use Windows anyway or what). Or the markup language that has the ability to highlight code but only in 3 or so languages. You don't even have the choice to switch to others. The wiki I have only used two or three times. Can't get warm with it either.

snowwrestler 12y ago

I'm surprised anyone would be willing to consider merging a branch into master that has not itself pulled the latest head from master. It puts the the person running the merge into the position of having to test someone else's code. It would seem to me that if a branch diffs against an older point in master, then the PR should be rejected as not properly tested. Merging into master should never create a merge conflict; the merge conflict should happen and be resolved on the feature branch first. Or what am I missing?

forgottenpass 12y ago
Or what am I missing? The linux kernel averages >5 patches per hour. Even with multi-teired maintanership, good luck getting everyone to base their patch sets on the latest upstream head. I know nobody other than the kernel actually uses git as a distributed system, but there are use cases where your ideal workflow can't actually exist.
kannonboy 12y ago
This is tricky to pull off on busy projects, as master is continually progressing and all open pull request branches would need to be constantly rebased. With the Bitbucket approach, you get to see the conflict in the pull request UI, but you should still resolve it locally on your branch before merging to master.

arpitverma007 12y ago

that was simple and awesome

jfaucett 12y ago

Ok this is nice. Of course, if you're just rebasing before reviewing and merging the PR into master it makes this pointless. Odd that they never mention this in the post, I was under the assumption that rebasing before creating the PR is standard practice, at least that's how we do it at work and on all the projects I've contributed to. EDIT: I've never used bitbucket, so maybe this is just a github community thing with the rebasing?

firethief 12y ago
That's a good (and standard) practice that solves the problem if your project never has more than one PR open at a time.
Nemo157 12y ago
For smaller projects that's fine, but if you have a lot of outstanding pull requests under discussion that's a lot of continual rebasing as they each get merged. (What if the two fee fixes in the article were done at the same time and reviewed by different project maintainers?)
- jfaucett 12y ago
  That sounds really unorganized to me at least, I mean the linux kernel is large and there's only one guy at the end of the day merging all that stuff. I would think that in larger projects a better practice would be a distributed hierarchy of merges until a PR finally got submitted to master, not a massive blob of just send everything to master as a PR and let various maintainers merge whatever at will.

mindprince 12y ago

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

kannonboy 12y ago
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-request/#comment-1811819137 https://developer.atlassian.com/blog/2015/01/a-better-pull-r...
- mindprince 12y ago
  Yup, that was me who asked the question there. :-)

azakai 12y ago

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.

kannonboy 12y ago
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.
- azakai 12y ago
  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.
  - evmar 12y ago
    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.)
ploxiln 12y ago
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.

falsedan 12y ago

Surely the bug in logic would be caught by tests?

Too 12y ago

Something that bugs me with almost every source control GUI is that there is usually no way to spawn a three way diff after a merge has been done. They might support three way merge on conflict but bang, once you save you can not get that view back. They also often don't understand that no branch was the master branch so if you diff the merged revision (diffing a single revision doesn't make sense yes but guis usually present it this way and diff it with the last revision) then you get all changes that has happened on master since the branch was created, wtf?? To see what only the branch contributed you have to manually select two revisions, diff them and then select the other revision and diff.

ska 12y ago

Why hasn't the alice/master branch brought over all of the changes in master since branching, and tested them, before review and (eventual) merge?