Re: [PATCH] xmerge.c: fix xdl_merge to conform with the manual

From: Anton Trunov <anton.a.trunov@gmail.com>
Date: Mar 9, 2015, 10:07 UTC
Message-ID: <54FD70FA.1080606@gmail.com>
In-Reply-To: <xmqqtwxvsyjq.fsf@gitster.dls.corp.google.com>

On 08/03/15 11:06, Junio C Hamano wrote:

Show 15 quoted lines

> Anton Trunov <anton.a.trunov@gmail.com> writes:
> 
>> On 04/03/15 23:01, Junio C Hamano wrote:
>>
>> My apologies for pushing this topic, but what would you recommend?
>> Should we treat both sides line-wise or should we correct the documentation?
> 
> My gut feeling is that the change to swap which side is examined
> first would end up to be a patch to rob Peter to pay Paul, and a
> line-by-line approach might end up paying too expensive a runtime
> cost in practice (and it should not really matter which side's
> whitespace the end result matches, because the user says "I do not
> care about whitespace changes", so paying that cost is not something
> we would want to do).  So it may be that the best course of action
> may be documentation updates.

Thanks for your reply. I agree with your point on performance penalty. But I don't want to commit a robbery, just to commit a nice expected merge result (pun intended).

I believe merge is inherently asymmetric operation: as git help merge says it "incorporates changes from the named commits ... into the current branch." We have a notion of direction here, right? So my approach would be "if their changes are insignificant don't take them at all, just keep my version".

I guess, the case "but if our version contains only trivial changes, and
theirs has some real work in it" can be solved in different ways:
  1) take their complete version, discarding our whitespace changes;
  2) or merge their and our versions line-wise, keeping our lines with
trivial changes when their corresponding lines also have only trivial
changes.

First solution may be a bit surprising at first glance (as you commented earlier), but if we don't want to pay performance price then (with proper documentation correction) it could be the solution.

In addition, if they changed something and polished indentation alongside then we should accept their work as a whole, and not mix our (indented) lines with theirs, shouldn't we?

And one more. Let's consider merging master with two branches (one-by-one, as octopus doesn't work with ignore-options). Here I assume all changes in all branches to be whitespace-only.

git merge -Xignore-all-space -m'merge2' test-branch-2 git merge -Xignore-all-space -m'merge1' test-branch-1

git merge -Xignore-all-space -m'merge1' test-branch-1 git merge -Xignore-all-space -m'merge2' test-branch-2

Those merges should produce the same results, right? But with current code version that is not true. And the patch resolves this issue too, because it discards all insignificant changes in _other_ branches.

So let me sum this up.
I think we should keep the patch, but
 - change the commit message adding some of the explanations from
current email-thread;
 - add more tests to cover the situation when we don't have any
significant changes and they do (resolve it in favor of their *file*);
 - correct the documentation to clarify those corner cases.

I'm sorry for my long reply (maybe too long for this topic). Thank you.