A better pull request

https://developer.atlassian.com/blog/2015/01/a-better-pull-request/

by kannonboy • 12 years ago

479 161 12 years ago

developer.atlassian.com

legomaster3• 12 years 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.

Revell2• 12 years ago

But still the test would only fail after the merge, whereas you'd want to catch this before.

jbrooksuk3• 12 years ago

On GitHub you can have tests run before you've merged, so you know whether it's safe or not.

brodney• 12 years 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• 12 years ago

A common practice is for pull requesters to rebase their branch on the upstream branch to avoid these conflicts.

maaaats• 12 years ago

But if the master/upstream is changed, the feature branch must be updated again (manually).

fenomas2• 12 years 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/

toong• 12 years ago

What about in-repo code search ?

https://bitbucket.org/site/master/issue/2874/ability-to-sear...

raziel2p• 12 years ago

I find that the search results from Github are often useless - sorting by relevance is hard in terms of code, I suppose.

melvinmt• 12 years ago

I use bitbucket mainly to host all my private repos (unlimited free private repos) and use github for my public ones. I'm not sure what the tradeoffs are regarding features (so far I'm not missing anything on bitbucket) but it's still git so the core things are still the same.

JamesSwift• 12 years ago

I'd argue bitbucket has more features. At least in terms of granular repo permissions, they are way ahead.

thejosh• 12 years ago

Same, we use Stash locally due to latency, ($10/yr for our tiny team), then mirror to Bitbucket each commit automatically.

vezzy-fnord• 12 years 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.

ngoldbaum• 12 years ago

Yes! Mercurial love on HN always makes me happy. Code hosting platforms should encourage choice.

ci5er• 12 years 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.

mikepurvis• 12 years ago

I use both. I find Bitbucket just lacks polish— it's hard to come up with specific examples, but various things give off the vibe of it being maintained by a small team within a larger organization for whom it's not really a priority.

Code search is one, but I also find it harder to get to the exact diff or pull request that I want. I don't get Github's nice rendering of filetypes like STLs, and there's a lot less of the context-sensitive popups when you're mentioning users, issues, commits, etc.

Bitbucket's issue tracker is kind of limited from ever getting too good, because it's really just there as a teaser for JIRA.

bsimpson• 12 years 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.

jbrooksuk3• 12 years ago

It'd make me happy if SublimeGit fixed the pull issues on OSX, let alone any update :(

mynameisvlad• 12 years ago

git status will tell you which files are conflicted.

pc86• 12 years 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.

mborch• 12 years ago

Interestingly, only Github manages to syntax highlight the diff.

city413• 12 years 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.

chinhodado2• 12 years ago

The problem is, many pull requests take very long to get merged (because of the size of the PR, the review process, etc.), and by the time that they are ready to be merged, the target branch could very well have changed.

GolfyMcG2• 12 years ago

This is something that our team has relegated to a product development problem not a software development problem. As a product manager/technical lead you have to realize it's your job to make it as easy as possible for developers to do their job. When you set them up for HUGE merge conflicts - that only makes life harder. If you're really good about constructing discrete, small stories, you can avoid SO many pull request headaches. If you don't allow master to get miles ahead by merging small changes frequently, this is a non-issue. It's hard to do though and requires changing the mindset of your team.

SEMW• 12 years ago

Our solution to that is to always rebase & push right before you run through ci & merge. As well as updating the PR diff to be against the master it was actually merged against, this also has the advantage of making your git history a lot more linear[0], which has various other upsides (e.g. makes it a lot more sane when using fugitive or something to navigate backwards & forwards through the history of a file).

[0] i.e. looks like http://blog.carbonfive.com/wp-content/uploads/2010/12/multip...

legomaster3• 12 years ago

Talking to a lot of people at conferences and similar events, rebasing against the target branch before merging is pretty uncommon. Few people think a clean history is that important.

Personally I think, rebase + auto-squash/auto-fixit makes the history a lot easier when it's time to look back. It just happens so rarely I wonder if it's really worth the effort I expend on it.

serve_yay5• 12 years ago

I don't know what it is, but when I try to rebase master into the feature branch, my PR diff ends up littered with commits that aren't part of the PR. Then my reviewers have to wade through a bunch of irrelevant crap to see my changes. I thought the whole point of rebasing was so that wouldn't happen.

Anyway, now I just don't do it anymore. Git is a pain in the ass.

btilly• 12 years ago

YMMV. I've seen people all over the map on this.

I have noticed that among people I respect, there is a strong correlation between using git-bisect and wanting a clean history.

jMyles2• 12 years 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.

city413• 12 years 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.

puls• 12 years 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-r...

jessaustin• 12 years 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.

jMyles2• 12 years 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• 12 years ago

It speaks to github's prominence in the open source community and Atlassians's prominence in professional/enterprise environments.

mkinsella• 12 years 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.

baldfat• 12 years ago

Personal use = BitBucket

WHY? Free private repos unlimited in number of repos only limited on how many can access them.

So all my .config files and anything private goes to BitBucket. Great product for my own use.

rdtsc• 12 years ago

The one from GitHub was about social dynamics and how to work together better, while the one from Atlassian is about technical minutiae.

What is it telling us of? Are you implying something to the effect of Atlassian doesn't care about people working together (heck, maybe they hate humans, who knows) and Github loves community...?

davvid• 12 years 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

rubiquity4• 12 years 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.

serve_yay5• 12 years 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.

taeric11• 12 years 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.

sbilstein• 12 years 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.

efuquen• 12 years 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.

ajross• 12 years 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.

rubiquity4• 12 years 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.

nfm• 12 years 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.

aragot• 12 years 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.

orbitur• 12 years 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.

cpeterso2• 12 years 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).

bhuga2• 12 years 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.

kannonboy6• 12 years 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.

bhuga2• 12 years ago

That makes sense. Thanks for clarifying.

taeric11• 12 years 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."

b6fan• 12 years 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/diff...

stevebot• 12 years 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• 12 years 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.

saraid2162• 12 years ago

Complete tangent: I've never heard the term "an 18 flight". What is that, and what is it from?

dominotw• 12 years 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• 12 years ago

If you have 100% code coverage and have solved the halting problem, yes.

erikb2• 12 years 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• 12 years 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

erikb2• 12 years 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• 12 years 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?

kannonboy6• 12 years 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.

forgottenpass• 12 years 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.

jfaucett2• 12 years 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?

Nemo157• 12 years 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?)

jfaucett2• 12 years 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.

firethief• 12 years 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.

mindprince2• 12 years 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`.

kannonboy6• 12 years 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-r...

mindprince2• 12 years ago

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

azakai3• 12 years 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.

kannonboy6• 12 years 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.

azakai3• 12 years 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.

ploxiln• 12 years 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.

azakai3• 12 years ago

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.

nnutter• 12 years ago

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.

Cub3• 12 years ago

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

kannonboy6• 12 years 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.

sytse3• 12 years 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• 12 years 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).

sytse3• 12 years ago

Thanks! Indeed you can replicate it yourself but we like to automate as much as possible.

juriansluiman• 12 years 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• 12 years 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)

falsedan• 12 years ago

Surely the bug in logic would be caught by tests?

Too• 12 years 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• 12 years 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?

A better pull request

Related Stories