Re: [PATCH] xmerge.c: fix xdl_merge to conform with the manual
On 08/03/15 11:06, Junio C Hamano wrote: Anton Trunov anton.a.tru...@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. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] xmerge.c: fix xdl_merge to conform with the manual
Anton Trunov anton.a.tru...@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. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] xmerge.c: fix xdl_merge to conform with the manual
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; -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] xmerge.c: fix xdl_merge to conform with the manual
On 04/03/15 23:01, Junio C Hamano wrote: Anton Trunov anton.a.tru...@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. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] xmerge.c: fix xdl_merge to conform with the manual
Anton Trunov anton.a.tru...@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. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] xmerge.c: fix xdl_merge to conform with the manual
On Wed, Mar 4, 2015 at 4:55 AM, Anton Trunov anton.a.tru...@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. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] xmerge.c: fix xdl_merge to conform with the manual
On 03/03/15 23:32, Junio C Hamano wrote: Anton Trunov anton.a.tru...@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. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] xmerge.c: fix xdl_merge to conform with the manual
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' ' [] -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] xmerge.c: fix xdl_merge to conform with the manual
On 04/03/15 10:07, Eric Sunshine wrote: On Tue, Mar 3, 2015 at 3:17 PM, Torsten Bögershausen tbo...@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' ' -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] xmerge.c: fix xdl_merge to conform with the manual
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' ' [] -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] xmerge.c: fix xdl_merge to conform with the manual
Anton Trunov anton.a.tru...@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? -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] xmerge.c: fix xdl_merge to conform with the manual
On Tue, Mar 3, 2015 at 3:17 PM, Torsten Bögershausen tbo...@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' ' -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] xmerge.c: fix xdl_merge to conform with the manual
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
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
