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

[Anton Trunov <anton.a.trunov@gmail.com>]

On 03/03/15 23:32, Junio C Hamano wrote:
> Anton Trunov <anton.a.trunov@gmail.com> writes:
> 
>> The git-merge manual says that the ignore-space-change,
>> ignore-all-space, ignore-space-at-eol options preserve our version
>> if their version only introduces whitespace changes to a line.
>>
>> So far if there is whitespace-only changes to both sides
>> in *all* lines their version will be used.
> 
> I am having hard time understanding the last sentence, especially
> the "So far" part.  Do you mean "With the current code, if ours and
> theirs change whitespaces on all lines, we take theirs"?

By "so far" I mean "until now, but not including it", i.e. the code
before applying the patch.

> I find the description in the documentation is a bit hard to read.
> 
>   * If 'their' version only introduces whitespace changes to a line,
>     'our' version is used;
> 
>   * If 'our' version introduces whitespace changes but 'their'
>     version includes a substantial change, 'their' version is used;
> 
>   * Otherwise, the merge proceeds in the usual way.
> 
> And it is unclear if your reading is correct to me.  In your "So
> far" scenario, 'our' version does introduce whitespace changes and
> 'their' version does quite a bit of damage to the file (after all,
> they both change *all* lines, right?).  It does not seem too wrong
> to invoke the second clause above and take 'theirs', at least to me.

Let me elaborate on this a bit.
It doesn't matter if all lines are changed or not.
The point is if all the changes in all the *changed* lines are trivial
(non-whitespace), i.e. there is no one line with substantial change on
both sides, then we just through away their version and keep our
whitespace changes.
We are talking here about non-so-probable corner-case of trivial changes
in our and their versions, perhaps an uncoordinated tabs-vs-space clean-up.
So I think I should add "changed lines" to the commit message.

For the code version before applying this patch the following scenario
will take place if "git merge -Xignore-all-space remote" gets executed.

base file:
1st line
2nd line

master file:
  1st line
  2nd line with substantial change

remote file:
              1st line
              2nd line

merge result file:
  1st line
  2nd line with substantial change

So essentially it does what "git merge -s ours remote" does in case if
all their changes are trivial.
This seems like reasonable solution to me: we _are_ trying to ignore
whitespace changes and we are explicit about it.

But, in the scenario with trivial changes everywhere we get a completely
different result:

base file:
1st line
2nd line

master file:
  1st line
  2nd line

remote file:
              1st line
              2nd line

merge result file:
              1st line
              2nd line

In my opinion if we respect the principle of least astonishment this
behavior should be fixed to:

merge result file:
  1st line
  2nd line

Exactly so does this patch.

> It is an entirely different matter if the behaviour the document
> describes is sane, and I didn't ask "git log" what the reasoning
> behind that second point is, but my guess is that a change made by
> them being "substantial" is a sign that it is a whitespace cleanup
> change and we should take the cleanup in such a case, perhaps?

If we want to take in their clean-up why would we use the
-Xignore-space-change option in the first place?
It looks like we're explicitly saying "we don't want any changes that
are whitespace-only", right?
And if we introduced some cleanup too what should we do when the
cleanups conflict? (exactly our case)
As far as I am concerned one should either manually resolve that kind of
conflicts without using the -Xignore-... options or just
git merge -X theirs remote.