[pkamb]

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

[kovrik]

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?

[phpnode]

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.

[sethammons]

And you just lost git bisect

[paulddraper]

Not really; git bisect skip still works.

I agree I don't like it either.

[pkamb]

`git bisect skip`

[sethammons]

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

[jzwinck]

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

[robinson7d]

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.

[michaelmior]

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

[dantiberian]

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

[masklinn]

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

[eridius]

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.

[pkamb]

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.

[joatmon-snoo]

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?

[deeplyoptional]

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

[acchow]

Sounds like this would be a nightmare to rebase onto.

[StefanKarpinski]

Not to mention breaking git bisect horribly.

[acchow]

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

[BuuQu9hu]

s/pass/skip

[chrisamanse]

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

[pkamb]

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

[squidbidness]

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.

[sjrd]

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.

[nothrabannosir]

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

[btown]

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

[azernik]

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

[aargh_aargh]

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

[nothrabannosir]

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

[fogleman]

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