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

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

[Anton Trunov]

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.

This commit fixes merge behavior in this case by checking
change for their version first.

Signed-off-by: Anton Trunov <anton.a.trunov <at> gmail.com>
---
 t/t3032-merge-recursive-options.sh | 43 ++++++++++++++++++++++++++++++++++++++
 xdiff/xmerge.c                     | 10 ++++-----
 2 files changed, 48 insertions(+), 5 deletions(-)

diff --git a/t/t3032-merge-recursive-options.sh b/t/t3032-merge-recursive-options.sh
index 4029c9c..4cbedb4 100755
--- a/t/t3032-merge-recursive-options.sh
+++ b/t/t3032-merge-recursive-options.sh
@@ -204,4 +204,47 @@ test_expect_success '--ignore-space-at-eol' '
 	test_cmp expected actual
 '
 
+# Setup for automerging with whitespace-only changes
+# on both sides and in *all* lines
+
+test_expect_success 'setup: w/s only changes in all lines on both sides' '
+	git rm -rf . &&
+	git clean -fdqx &&
+	rm -rf .git &&
+	git init
+
+	echo " two words" >text.txt &&
+	git add text.txt &&
+	test_tick &&
+	git commit -m "Initial revision" &&
+
+	git checkout -b remote &&
+	echo " \t\ttwo     words  " >text.txt &&
+	git commit -a -m "remote: insert whitespace only" &&
+
+	git checkout master &&
+	echo "    two words" >text.txt &&
+	git commit -a -m "master: insert whitespace only"
+'
+
+test_expect_success 'w/s only in all lines: --ignore-space-change preserves ours' '
+	echo "    two words" >expected &&
+	git read-tree --reset -u HEAD &&
+	git merge-recursive --ignore-space-change HEAD^ -- HEAD remote &&
+	test_cmp expected text.txt
+'
+
+test_expect_success 'w/s only in all lines: --ignore-all-space preserves ours' '
+	echo "    two words" >expected &&
+	git read-tree --reset -u HEAD &&
+	git merge-recursive --ignore-all-space HEAD^ -- HEAD remote &&
+	test_cmp expected text.txt
+'
+
+test_expect_success 'w/s only in all lines: --ignore-space-at-eol fails' '
+	git read-tree --reset -u HEAD &&
+	test_must_fail git merge-recursive --ignore-space-at-eol HEAD^ -- HEAD remote &&
+	grep "<<<<<<" text.txt
+'
+
 test_done
diff --git a/xdiff/xmerge.c b/xdiff/xmerge.c
index 625198e..2c7db26 100644
--- a/xdiff/xmerge.c
+++ b/xdiff/xmerge.c
@@ -596,14 +596,14 @@ int xdl_merge(mmfile_t *orig, mmfile_t *mf1, mmfile_t *mf2,
 		return -1;
 	}
 	status = 0;
-	if (!xscr1) {
-		result->ptr = xdl_malloc(mf2->size);
-		memcpy(result->ptr, mf2->ptr, mf2->size);
-		result->size = mf2->size;
-	} else if (!xscr2) {
+	if (!xscr2) {
 		result->ptr = xdl_malloc(mf1->size);
 		memcpy(result->ptr, mf1->ptr, mf1->size);
 		result->size = mf1->size;
+	} else if (!xscr1) {
+		result->ptr = xdl_malloc(mf2->size);
+		memcpy(result->ptr, mf2->ptr, mf2->size);
+		result->size = mf2->size;
 	} else {
 		status = xdl_do_merge(&xe1, xscr1,
 				      &xe2, xscr2,
-- 
2.3.1.167.g7f4ba4b

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

[Torsten Bögershausen]

On 2015-03-03 18.37, Anton Trunov wrote:
[]
> Signed-off-by: Anton Trunov <anton.a.trunov <at> gmail.com>
Should we use the "real email" here (with the '@') ?
> ---
>  t/t3032-merge-recursive-options.sh | 43 ++++++++++++++++++++++++++++++++++++++
>  xdiff/xmerge.c                     | 10 ++++-----
>  2 files changed, 48 insertions(+), 5 deletions(-)
> 
> diff --git a/t/t3032-merge-recursive-options.sh b/t/t3032-merge-recursive-options.sh
> index 4029c9c..4cbedb4 100755
> --- a/t/t3032-merge-recursive-options.sh
> +++ b/t/t3032-merge-recursive-options.sh
> @@ -204,4 +204,47 @@ test_expect_success '--ignore-space-at-eol' '
>  	test_cmp expected actual
>  '
>  
> +# Setup for automerging with whitespace-only changes
> +# on both sides and in *all* lines
> +
> +test_expect_success 'setup: w/s only changes in all lines on both sides' '
> +	git rm -rf . &&
> +	git clean -fdqx &&
> +	rm -rf .git &&
> +	git init
missing &&
> +
> +	echo " two words" >text.txt &&
> +	git add text.txt &&
> +	test_tick &&
> +	git commit -m "Initial revision" &&
> +
> +	git checkout -b remote &&
> +	echo " \t\ttwo     words  " >text.txt &&
> +	git commit -a -m "remote: insert whitespace only" &&
> +
> +	git checkout master &&
> +	echo "    two words" >text.txt &&
> +	git commit -a -m "master: insert whitespace only"
> +'
> +
> +test_expect_success 'w/s only in all lines: --ignore-space-change preserves ours' '
[]

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

[Junio C Hamano]

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

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.

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?

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

[Eric Sunshine]

On Tue, Mar 3, 2015 at 3:17 PM, Torsten Bögershausen <tboegi@web.de> wrote:
> On 2015-03-03 18.37, Anton Trunov wrote:
> []
>> Signed-off-by: Anton Trunov <anton.a.trunov <at> gmail.com>
> Should we use the "real email" here (with the '@') ?
>> ---
>> diff --git a/t/t3032-merge-recursive-options.sh b/t/t3032-merge-recursive-options.sh
>> index 4029c9c..4cbedb4 100755
>> --- a/t/t3032-merge-recursive-options.sh
>> +++ b/t/t3032-merge-recursive-options.sh
>> @@ -204,4 +204,47 @@ test_expect_success '--ignore-space-at-eol' '
>>       test_cmp expected actual
>>  '
>>
>> +# Setup for automerging with whitespace-only changes
>> +# on both sides and in *all* lines
>> +
>> +test_expect_success 'setup: w/s only changes in all lines on both sides' '
>> +     git rm -rf . &&
>> +     git clean -fdqx &&
>> +     rm -rf .git &&
>> +     git init
> missing &&
>> +
>> +     echo " two words" >text.txt &&
>> +     git add text.txt &&
>> +     test_tick &&
>> +     git commit -m "Initial revision" &&
>> +
>> +     git checkout -b remote &&
>> +     echo " \t\ttwo     words  " >text.txt &&

Use of echo "\t" is not portable. Either embed literal tab characters
or use printf "\t".

>> +     git commit -a -m "remote: insert whitespace only" &&
>> +
>> +     git checkout master &&
>> +     echo "    two words" >text.txt &&
>> +     git commit -a -m "master: insert whitespace only"
>> +'
>> +
>> +test_expect_success 'w/s only in all lines: --ignore-space-change preserves ours' '

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

[Anton Trunov]

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.

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

[Anton Trunov]

On 04/03/15 10:07, Eric Sunshine wrote:
> On Tue, Mar 3, 2015 at 3:17 PM, Torsten Bögershausen <tboegi@web.de> wrote:
>> On 2015-03-03 18.37, Anton Trunov wrote:
>> []
>>> Signed-off-by: Anton Trunov <anton.a.trunov <at> gmail.com>
>> Should we use the "real email" here (with the '@') ?
>>> ---
>>> diff --git a/t/t3032-merge-recursive-options.sh b/t/t3032-merge-recursive-options.sh
>>> index 4029c9c..4cbedb4 100755
>>> --- a/t/t3032-merge-recursive-options.sh
>>> +++ b/t/t3032-merge-recursive-options.sh
>>> @@ -204,4 +204,47 @@ test_expect_success '--ignore-space-at-eol' '
>>>       test_cmp expected actual
>>>  '
>>>
>>> +# Setup for automerging with whitespace-only changes
>>> +# on both sides and in *all* lines
>>> +
>>> +test_expect_success 'setup: w/s only changes in all lines on both sides' '
>>> +     git rm -rf . &&
>>> +     git clean -fdqx &&
>>> +     rm -rf .git &&
>>> +     git init
>> missing &&
>>> +
>>> +     echo " two words" >text.txt &&
>>> +     git add text.txt &&
>>> +     test_tick &&
>>> +     git commit -m "Initial revision" &&
>>> +
>>> +     git checkout -b remote &&
>>> +     echo " \t\ttwo     words  " >text.txt &&
> 
> Use of echo "\t" is not portable. Either embed literal tab characters
> or use printf "\t".
OK.
Shouldn't it be printf "\t\n" for exact substitute for echo "\t"?

>>> +     git commit -a -m "remote: insert whitespace only" &&
>>> +
>>> +     git checkout master &&
>>> +     echo "    two words" >text.txt &&
>>> +     git commit -a -m "master: insert whitespace only"
>>> +'
>>> +
>>> +test_expect_success 'w/s only in all lines: --ignore-space-change preserves ours' '

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

[Anton Trunov]

On 03/03/15 23:17, Torsten Bögershausen wrote:
> On 2015-03-03 18.37, Anton Trunov wrote:
> []
>> Signed-off-by: Anton Trunov <anton.a.trunov <at> gmail.com>
> Should we use the "real email" here (with the '@') ?

Didn't realize the parser for the web version mangles emails.
Will use the "real email".

>> ---
>>  t/t3032-merge-recursive-options.sh | 43 ++++++++++++++++++++++++++++++++++++++
>>  xdiff/xmerge.c                     | 10 ++++-----
>>  2 files changed, 48 insertions(+), 5 deletions(-)
>>
>> diff --git a/t/t3032-merge-recursive-options.sh b/t/t3032-merge-recursive-options.sh
>> index 4029c9c..4cbedb4 100755
>> --- a/t/t3032-merge-recursive-options.sh
>> +++ b/t/t3032-merge-recursive-options.sh
>> @@ -204,4 +204,47 @@ test_expect_success '--ignore-space-at-eol' '
>>  	test_cmp expected actual
>>  '
>>  
>> +# Setup for automerging with whitespace-only changes
>> +# on both sides and in *all* lines
>> +
>> +test_expect_success 'setup: w/s only changes in all lines on both sides' '
>> +	git rm -rf . &&
>> +	git clean -fdqx &&
>> +	rm -rf .git &&
>> +	git init
> missing &&

Nice catch! Thank you.

>> +
>> +	echo " two words" >text.txt &&
>> +	git add text.txt &&
>> +	test_tick &&
>> +	git commit -m "Initial revision" &&
>> +
>> +	git checkout -b remote &&
>> +	echo " \t\ttwo     words  " >text.txt &&
>> +	git commit -a -m "remote: insert whitespace only" &&
>> +
>> +	git checkout master &&
>> +	echo "    two words" >text.txt &&
>> +	git commit -a -m "master: insert whitespace only"
>> +'
>> +
>> +test_expect_success 'w/s only in all lines: --ignore-space-change preserves ours' '
> []
> 

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

[Eric Sunshine]

On Wed, Mar 4, 2015 at 4:55 AM, Anton Trunov <anton.a.trunov@gmail.com> wrote:
> On 04/03/15 10:07, Eric Sunshine wrote:
>>>> +     echo " \t\ttwo     words  " >text.txt &&
>>
>> Use of echo "\t" is not portable. Either embed literal tab characters
>> or use printf "\t".
> OK.
> Shouldn't it be printf "\t\n" for exact substitute for echo "\t"?

Yes, that was implied; it didn't seem necessary to describe the
conversion to printf in full detail. Sorry if there was any confusion.

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

[Junio C Hamano]

Anton Trunov <anton.a.trunov@gmail.com> writes:

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

Now, the above makes readers wonder what happens when you merged
master into the remote.  I.e. in the opposite direction.

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

[Anton Trunov]

On 04/03/15 23:01, Junio C Hamano wrote:
> Anton Trunov <anton.a.trunov@gmail.com> writes:
> 
>> 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.
> 
> Now, the above makes readers wonder what happens when you merged
> master into the remote.  I.e. in the opposite direction.
> 
Interesting observation.
I'd like to point out that the patch doesn't change the way this merge
works.

But let us discuss it with the help of git log.

The function xdl_merge() was introduced to the code base in commit
857b933e04bc21ce02043c3107c148f8dcbb4a01.

As it was pointed out in 1cd12926cedb340d176db607e087495381032ce2
(t6023: merge-file fails to output anything for a degenerate merge) the
merge-file command would just refuse to merge identical changes and
produce output.

Then in 5719db91ce5915ee07c50f1afdc94fe34e91529f (Change xdl_merge to
generate output even for null merges) this was fixed.

Let's return to the issue you've mentioned.
According to the manual the merge result file should be:
               1st line                     <- our line
   2nd line with substantial change         <- their line

The problem with the current code that it doesn't work line-wise when at
least one side has only whitespace changes. It just completely throws
away a side without any changes and takes the other side.

If we wanted to fix it then I'd suggest to completely remove the checks
for null changes in xdl_merge() because xdl_do_merge() handles those.

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

[Anton Trunov]

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?

Current version for git help merge:

...
ignore-space-change, ignore-all-space, ignore-space-at-eol
    Treats lines with the indicated type of whitespace change as
unchanged for the sake of a three-way merge. Whitespace
    changes mixed with other changes to a line are not ignored. See also
git-diff(1)-b, -w, and --ignore-space-at-eol.

    o   If their version only introduces whitespace changes to a line,
our version is used;

    o   If our version introduces whitespace changes but their version
includes a substantial change, their version is used;

    o   Otherwise, the merge proceeds in the usual way.
...


The 1st bullet point could be changed into
    o   If their version only introduces whitespace changes to *all
changed lines*, our version is used;

And the 2nd one into:
    o   If our version only introduces whitespace changes to all changed
lines, but their version includes at least one substantially changed
line, all lines from their version are used;

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

[Junio C Hamano]

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.

But I haven't had a chance to think about it through yet to form a
definite opinion.

Thanks.

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

[Anton Trunov]

On 08/03/15 11:06, Junio C Hamano wrote:
> 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.