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

From: Anton Trunov <anton.a.trunov@gmail.com>
Date: Mar 4, 2015, 09:43 UTC
Message-ID: <54F6D3B0.60600@gmail.com>
In-Reply-To: <xmqqzj7takks.fsf@gitster.dls.corp.google.com>

On 03/03/15 23:32, Junio C Hamano wrote:

Show 12 quoted lines

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

Show 15 quoted lines

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

Show 5 quoted lines

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