On 03/03/15 23:32, Junio C Hamano wrote:
> Anton Trunov <anton.a.trunov@xxxxxxxxx> 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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html