
A better pull request - kannonboy
https://developer.atlassian.com/blog/2015/01/a-better-pull-request/
======
rubiquity
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
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.

~~~
rubiquity
> _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
You can rebase without squashing to get the benefits of both approaches.

~~~
rubiquity
Hah! Great point. I always forget that because I do so very rarely.

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

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

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

------
sytse
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
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
Thanks! Indeed you can replicate it yourself but we like to automate as much
as possible.

------
fenomas
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/)

~~~
melvinmt
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
I'd argue bitbucket has more features. At least in terms of granular repo
permissions, they are way ahead.

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

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

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

~~~
kannonboy
Great point. Almost all of the problems I've seen with pull requests and
branching workflows stem from the size of the change. If you can avoid long-
lived branches / giant diffs, most integration problems just disappear. It's
not always possible to avoid them, but bearing it in mind when you're
constructing stories and planning your backlog helps a lot.

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

------
puls
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...](https://github.com/blog/1943-how-to-write-the-perfect-pull-request)

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

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

~~~
Alupis
Bitbucket provides free private repos, and for smaller development teams
and/or companies not wanting to deal with the cost of Github (which does
admittedly grow exponentially the more repos you require), Bitbucket is a fine
choice.

When we were making the decision at my company, we went with Github because
the dev team cared about having the little green squares show up on the
"activity" chart for their account's... I know, petty, but it's something, and
since most of us do FOSS projects, it's a status thing.

It used to be any commit that made it into a repo's master branch, the green
squares showed up, even if it was a private repo (it just didn't show details
of the repo to public users). But now, those don't show up to the public, only
the user themselves sees them while logged in... so if we were making the
decision today, I'd probably lean towards Bitbucket.

~~~
tnorthcutt
_which does admittedly grow exponentially the more repos you require_

Just to clarify, GH's pricing doesn't actually grow exponentially. The per
repo price gets lower the more you pay for:

5 private repos: $7

10 private repos: $12

20 private repos: $22

50 private repos: $50

~~~
Alupis
For a team, pricing is different. 50 repos is $100 a month, 125 repos is $200
a month ($2,400 a year). Granted, it's not "exponential", I was using a
figure-of-speech referring to the pricing becoming significant.

For an internal-dev team which generates a great deal of new repos throughout
the year (one-off scripts/programs for different departments, etc...), this
adds up very quickly.

If you reach that 126th repo, it jumps to $450 monthly or $5,400 a year. At
those prices you get questions from Accounting about why we aren't hosting
this internally...

~~~
sls
GitHub Enterprise is an appliance VM you run internally. It's charged per
seat. The last enterprise-y company I was at used it primarily because you
keep your code inside the firewall, but also because the pricing model is more
aligned with the usage in that environment, as you suggest.

~~~
Alupis
GitHub Enterprise is even more expensive than what I quoted above. The quotes
above are for private "team" repos under an "Organization" on Github (they
host it all).

------
legomaster
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
But still the test would only fail after the merge, whereas you'd want to
catch this before.

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

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

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

------
brodney
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
A common practice is for pull requesters to rebase their branch on the
upstream branch to avoid these conflicts.

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

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

------
mindprince
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
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...](https://developer.atlassian.com/blog/2015/01/a-better-pull-
request/#comment-1811819137)

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

------
b6fan
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...](https://secure.phabricator.com/book/phabricator/article/differential_faq/#what-
do-the-very-light-g)

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

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

~~~
taeric
Close. Under this convention, somebody has to do a final test before pushing
the merge commit. It can be a simple sanity test, not a full blown integration
test. But somewhere somebody at least did a compile check.

If it fails that test, then you push it back to the person doing the work
saying so and they need to fix it.

~~~
comex
In a GitHub project I've contributed to (dolphin-emu), a compile check for all
supported platforms, plus a few tests, shows up next to the big merge button.
So the "other party" doing the merge is a robot, and you can still click the
button.

I haven't been a committer on any other large GitHub projects, so I'm not sure
how common this is.

~~~
taeric
So, sadly, I don't contribute to really any github projects. This is a new
feature to me and does go a long way to addressing my concern. Honestly, it
may completely address it. Certainly sounds like it does.

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

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

------
dominotw
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
If you have 100% code coverage and have solved the halting problem, yes.

------
mborch
Interestingly, only Github manages to syntax highlight the diff.

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

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

------
stevebot
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
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
Complete tangent: I've never heard the term "an 18 flight". What is that, and
what is it from?

~~~
sah2ed
Perhaps adding "hour" fixes the sentence. " ... she may be on an 18 _hour_
flight to the South Pole with no internet access."

------
ska
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?

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

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

------
falsedan
Surely the bug in logic would be caught by tests?

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

