Hacker News new | past | comments | ask | show | jobs | submit login
Resolve simple merge conflicts on GitHub (github.com/blog)
228 points by moby on Dec 12, 2016 | hide | past | favorite | 67 comments



On my team I've found that it's incredibly useful to commit the merge conflicts and conflict markers, then immediately resolve the conflicts in the next commit. This gives you one commit that shows exactly how the two branches merged together, followed by a commit that shows exactly how the conflicts were resolved. The resolution commit can then be code reviewed independently for a nice clean view of the conflicts introduced in the merge. It also allows you to easily reset to the merge commit and resolve the conflicts differently.

The standard git workflow (and this github feature) seems to promote resolving the conflicts alongside all of the other changes in the merge working copy. This make me nervous, as there's no way to differentiate the new lines that were introduced to resolve merge conflicts from the thousands of lines of (previously reviewed) code from the feature branch.

If you're not careful, completely unrelated working-copy code and behavior can be introduced in a "merge commit" and neither you or any of your reviewers will notice. "Looks good to me."


Yes, that is a problem.

On the other hand, with your approach you are going to have revisions that won't even build/compile.

If you have automatic builds or/and unit/integration tests, then you'll have failed builds every time you have a merge conflict.

Also, you are kind of 'polluting a well': what if meanwhile someone merges that revision into his/her branch? Or what if you have automatic merges configured?


Presumably in this model the dev wouldn't push their branch until the merge is actually complete, and there's presumably a convention like prepending `[CONFLICT]` to those commits to discourage people from checking them out directly.


And you just lost git bisect


Not really; git bisect skip still works.

I agree I don't like it either.


`git bisect skip`


The more you know; I'll have to look into that!


You can get it back by making your bisect test function return "good" whenever it sees a commit with merge conflicts.


I don't think that would work, but correct me if I'm wrong.

As far as I know, git bisect does a binary search along the commits; `good` tells it to look at the latter half, `bad` to look at the former.

So suppose you have five commits (1,2,3,4,5), where 1 is the working state, and 3 is a conflict commit. It will start by asking about the middle commit (3), automatically choose `good`, and determine that 3 was the latest working commit (after checking 4, which says `bad`).

----

EDIT: Obviously this is simplified to explain the issue with marking "good" those commits.


This is resolved if you skip the commits with a conflict instead of marking as good.


I think you'd want to skip, not mark as good?


> test function return "good" whenever it sees a commit with merge conflicts

That's a terrible idea and would completely break bisect. You should signal "skip" (return code 125) if the commit can not be tested at all.


You actually can distinguish the new lines. For any non-trivial merge conflict resolution committed as part of the merge, `git show $SHA` will actually show you the conflict resolution. More specifically, if the diff contains anything that's not just a line taken from either of the parents, then that thing is shown.


Yeah, I have no doubt that you can somehow show this information via the command line. The problem is that it's hidden in GitHub's Pull Request web UI, where all of our code review happens. Committing the conflicts and then resolving in the next commit surfaces the conflict resolutions to the PR where it can be reviewed like all of the other code we write.


If you don't rewrite commit history (it sounds like you don't) you can see it by looking at the diff of the latest commit on the PR.

Also, I haven't used it extensively since its release, but doesn't the Reviews feature resolve this now?


The PR UI does this. After resolving conflicts, the resulting merge commit will show just the resolution.


Sounds like this would be a nightmare to rebase onto.


Not to mention breaking git bisect horribly.


You could change your bisect script to pass on broken builds.


s/pass/skip


IMO, it is unnecessary to commit the conflicts. Instead, you can see how merges were resolved by diffing the commits.


"Diffing the commits" isn't really available in in a GitHub-style Pull Request web UI, which is where 99% of our code review is happening. I'm definitely optimizing for that view of the merge over everything else.


I love how github fosters discovery and remote collaboration, though one of its liabilities is when great git command-line features are effectively lost unless github re-implements or exposes them, because some conventions incentivize only doing what github itself can do.


Or much simpler: do not allow merge conflicts to happen in the first place! Any Pull Request that shows with merge conflicts must be rebased on top of the target branch. Problem solved.


Great idea! Although this does break cherry-pick, doesn't it?


I'd imagine this also breaks bisect (and might make your CI system very confused), since you have a non-good commit.


Bisect knows how to deal with this; you can tell it to ignore a commit that you know is broken for reasons unrelated to the issue upper investigating (and try adjacent ones instead).


Why would it? You can get a conflict on a cherry-pick as well.


I meant to write bisect---I was in the middle of an actual git workflow and accidentally wrote this instead. :)


That's a really good idea! I'm going to start doing that.


So simple, so useful, I wonder if this feature wasn't already in Gitlab since it seems to be more full featured



That's flipping awesome. Does it already have code review too?


Yes, with versions.


haha I was just guessing and there you have it ^^ I'm slowly switching to Gitlab, only issue is its slighly slower than Github


diff3 conflict style display would be considerably more useful.


Agreed.

It can be a bit noisier at first but once you learn to read it, I find it makes resolving conflicts so much easier.

For those of you who haven't used it, try switching it on and/or read https://psung.blogspot.com.au/2011/02/reducing-merge-headach... for more details.

tl;dr it shows


That's great, thanks for sharing.

FYI: If anyone is interested in the section about git rerere (reuse recorded resolution), the link at the bottom of that article leads to a 503; a repost can be found here: https://git-scm.com/2010/03/08/rerere.html


Finally! This is excellent news.


Yeah this is quite a great update.


To me this feels like making a commit without unit testing first. When I find a conflict I like to be able to resolve it and then do some unit testing to make sure that my revision didn't miss anything.


I think they explicitly say "simple merge conflicts" in the title. At the end of the day, you should use your own best judgement for when this is useful, and for when you need to go back to your workspace. It's most definitely not meant to be used for every merge conflict.

But not everyone is working on big projects with tests, and not every merge conflict is actually complex code modification.

Sometimes it's just two commits adding something at the end of the file and there's not real conflict, or maybe you modified the same line twice and forgot to pull before doing your 2nd edit.


I suspect the use-case they have in mind are folks who have CI hooked up to Github (so using this feature will automatically trigger tests).


Sure, but that's a much slower cycle than running your unit tests locally.


I didn't know I could merge on github.com in the first place... where is their merge function, by the way?


It's literally the hallmark of GitHub... How else would pull requests work? (well, there _is_ the rebase option now...)


Hmm. I usually do merges locally as serious stuff should be built and tested before pushing anyway, so probably why never used GitHub's hosted functions.


> serious stuff should be built and tested before pushing anyway

Yes and no. Build in your CI server that's set up to mirror your prod environment after pushing, but before merging. That's what the whole industry of CI providers and integrations built into and around GitHub and GitLab is for.


To be fair, one of the annoying things about how PRs work is that they don't test the merge, they test the commit relative to its original base. Your tests may pass in the PR, but fail once applied to later changes in the main line.


Most CI solutions will take care of that; they'll pull in the PR, merge it locally with master, and run tests on that.


I can't speak to the "merge-and-run" behavior of CI, but in my experience, most of these merge conflicts arise because of a time delay between making the PR (at which point CI is run) and merging the PR. It would be quite resource intensive to re-build every PR against every new branch in master.


> It would be quite resource intensive to re-build every PR against every new branch in master.

Depends on just how resource intensive a build is, and how often commits are made to master.

Here at work, we don't do that, but it wouldn't be a complete clusterfuck if we were to.


In my experience, Travis CI tests the merge at the time the pullreq is created. It also has a "re-run" button that lets you merge the latest master and run the tests on top of that.


And depending on setups, testing locally is testing far fewer environments than CI. I only have one laptop, but many of my projects need at least OS X, Linux, and Windows, and often multiple versions of those.


If you write tests diligently, you can integrate CI with GitHub. Travis CI might be the most popular option.


[flagged]


I'd prefer it not be magic. Just because git COULD merge two of the same line changes doesn't mean it SHOULD.

Maybe you have two vastly different methods to solving the same problem and now they are both in there and both not working instead of leaving it up to the merger to decide.


This seems like a clear case of unequal tradeoffs.

There are a couple of conflict patterns that are particularly easy to identify, which git could merge smoothly. For instance, two unrelated code blocks are appended to the bottom of the same file - whoever merged those two probably wants to keep both in any order.

But if you don't want that behavior, git is going to quietly auto-merge a bad change. That's easily 10x as bad as the time savings is good, maybe 100x. So I agree - this should not be magic, and I'm pretty sure the design for auto-merge hits practical limits long before technical ones.


How do you propose that be dealt with programmatically?


To be fair git could be given a tiny bit of "smarts" per language it's looking at. So say 2 different people add attributes to an HTML item it could use some sort of system that let's it run an HTML merge resolution routine that says "hey that's cool let me just combine those".

At the same time adding extra smarts like that, while providing a better UX when it works, the times where it doesn't work especially if you don't notice it stopped working in a specific way...that all scares me.

I'm not sure we're ready for smarts in our merging.


I posted this in a comment above but because it's relevant I feel it's worth putting here as well[1]:

git rerere (reuse recorded resolution): https://git-scm.com/2010/03/08/rerere.html

"it allows you to ask Git to remember how you've resolved a hunk conflict so that the next time it sees the same conflict, Git can automatically resolve it for you."

[1] This isn't explicitly against the rules but if it's in bad taste I'll happily remove it.


That's good if the exact same conflict reoccurs, but how often does that happen? What would be far better would be a syntax-aware version control system that knows about HTML attributes and understands how to combine two separate additions of HTML attributes.


That would be an improvement, although you’d still need human intervention sometimes. Even structurally sound merges can produce semantically wrong results, because diffs aren’t enough to reconstruct a correct history in all cases.


This is great, I appreciate you putting it here. I have a conflict pattern that comes up with enormous frequency at work, and for various reasons solving it somewhere other than the merge is not a great plan. It's always the same solution, though, and I could even predict it and flag for it in the commit message.

This seems like exactly the answer to that.


rerere is dangerous and not the solution. What I want is language-aware merge that can do things like "both these changes are just adding imports, adding in either order is fine" or "this change has added a parameter and this change has reformatted this line, I can add the parameter to the reformatted line".


ISTR darcs had something like this feature, but not as advanced as your idea. One of its types of patches was 'word substution', but you could imagine an 'add import' patch type.


Git supports external merge tools (per file type too). They don't need to be packaged with the base install IMO, and git won't support every language under the sun.

For example Unity3D are distributing one for their assets (which are not nice to merge as simple text)


For sure and I didn't mean they should necessarily be packaged with the base install either (though I'd argue as long as they're developed in a way that they can be updated you could provide a better UX bundling a few most common ones in). It's certainly doable though my only worry is them being almost smart enough and making a big mistake that isn't immediately noticeable.




Guidelines | FAQ | Lists | API | Security | Legal | Apply to YC | Contact

Search: