{"thread":{"id":"38692","subject":"[PATCH] xmerge.c: fix xdl_merge to conform with the manual","startedAt":"2015-03-03T17:37:00Z","lastAt":"2015-03-09T10:07:54Z","messageCount":13,"participants":["Anton Trunov","Torsten Bögershausen","Junio C Hamano","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"256914","messageId":"1425404233-89907-1-git-send-email-anton.a.trunov@gmail.com","threadId":"38692","inReplyTo":null,"subject":"[PATCH] xmerge.c: fix xdl_merge to conform with the manual","fromName":"Anton Trunov","fromEmail":"anton.a.trunov@gmail.com","sentAt":"2015-03-03T17:37:00Z","receivedAt":"2015-03-03T17:37:00Z","isPatch":true,"sender":{"key":"anton.a.trunov@gmail.com","avatar":"https://gravatar.com/avatar/0c198a8971bd6147c766c491b7661f0a2d1875a80c5562bd70d4f5076460eccb?d=mp&s=160"},"body":"The git-merge manual says that the ignore-space-change,\nignore-all-space, ignore-space-at-eol options preserve our version\nif their version only introduces whitespace changes to a line.\n\nSo far if there is whitespace-only changes to both sides\nin *all* lines their version will be used.\n\nThis commit fixes merge behavior in this case by checking\nchange for their version first.\n\nSigned-off-by: Anton Trunov gmail.com>\n---\n t/t3032-merge-recursive-options.sh | 43 ++++++++++++++++++++++++++++++++++++++\n xdiff/xmerge.c | 10 ++++-----\n 2 files changed, 48 insertions(+), 5 deletions(-)\n\ndiff --git a/t/t3032-merge-recursive-options.sh b/t/t3032-merge-recursive-options.sh\nindex 4029c9c..4cbedb4 100755\n--- a/t/t3032-merge-recursive-options.sh\n+++ b/t/t3032-merge-recursive-options.sh\n@@ -204,4 +204,47 @@ test_expect_success '--ignore-space-at-eol' '\n \ttest_cmp expected actual\n '\n \n+# Setup for automerging with whitespace-only changes\n+# on both sides and in *all* lines\n+\n+test_expect_success 'setup: w/s only changes in all lines on both sides' '\n+\tgit rm -rf . &&\n+\tgit clean -fdqx &&\n+\trm -rf .git &&\n+\tgit init\n+\n+\techo \" two words\" >text.txt &&\n+\tgit add text.txt &&\n+\ttest_tick &&\n+\tgit commit -m \"Initial revision\" &&\n+\n+\tgit checkout -b remote &&\n+\techo \" \\t\\ttwo words \" >text.txt &&\n+\tgit commit -a -m \"remote: insert whitespace only\" &&\n+\n+\tgit checkout master &&\n+\techo \" two words\" >text.txt &&\n+\tgit commit -a -m \"master: insert whitespace only\"\n+'\n+\n+test_expect_success 'w/s only in all lines: --ignore-space-change preserves ours' '\n+\techo \" two words\" >expected &&\n+\tgit read-tree --reset -u HEAD &&\n+\tgit merge-recursive --ignore-space-change HEAD^ -- HEAD remote &&\n+\ttest_cmp expected text.txt\n+'\n+\n+test_expect_success 'w/s only in all lines: --ignore-all-space preserves ours' '\n+\techo \" two words\" >expected &&\n+\tgit read-tree --reset -u HEAD &&\n+\tgit merge-recursive --ignore-all-space HEAD^ -- HEAD remote &&\n+\ttest_cmp expected text.txt\n+'\n+\n+test_expect_success 'w/s only in all lines: --ignore-space-at-eol fails' '\n+\tgit read-tree --reset -u HEAD &&\n+\ttest_must_fail git merge-recursive --ignore-space-at-eol HEAD^ -- HEAD remote &&\n+\tgrep \"<<<<<<\" text.txt\n+'\n+\n test_done\ndiff --git a/xdiff/xmerge.c b/xdiff/xmerge.c\nindex 625198e..2c7db26 100644\n--- a/xdiff/xmerge.c\n+++ b/xdiff/xmerge.c\n@@ -596,14 +596,14 @@ int xdl_merge(mmfile_t *orig, mmfile_t *mf1, mmfile_t *mf2,\n \t\treturn -1;\n \t}\n \tstatus = 0;\n-\tif (!xscr1) {\n-\t\tresult->ptr = xdl_malloc(mf2->size);\n-\t\tmemcpy(result->ptr, mf2->ptr, mf2->size);\n-\t\tresult->size = mf2->size;\n-\t} else if (!xscr2) {\n+\tif (!xscr2) {\n \t\tresult->ptr = xdl_malloc(mf1->size);\n \t\tmemcpy(result->ptr, mf1->ptr, mf1->size);\n \t\tresult->size = mf1->size;\n+\t} else if (!xscr1) {\n+\t\tresult->ptr = xdl_malloc(mf2->size);\n+\t\tmemcpy(result->ptr, mf2->ptr, mf2->size);\n+\t\tresult->size = mf2->size;\n \t} else {\n \t\tstatus = xdl_do_merge(&xe1, xscr1,\n \t\t\t\t &xe2, xscr2,\n-- \n2.3.1.167.g7f4ba4b\n"},{"id":"256921","messageId":"54F616DD.8040904@web.de","threadId":"38692","inReplyTo":"1425404233-89907-1-git-send-email-anton.a.trunov@gmail.com","subject":"Re: [PATCH] xmerge.c: fix xdl_merge to conform with the manual","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2015-03-03T20:17:33Z","receivedAt":"2015-03-03T20:17:33Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 2015-03-03 18.37, Anton Trunov wrote:\n[]\n> Signed-off-by: Anton Trunov gmail.com>\nShould we use the \"real email\" here (with the '@') ?\n> ---\n> t/t3032-merge-recursive-options.sh | 43 ++++++++++++++++++++++++++++++++++++++\n> xdiff/xmerge.c | 10 ++++-----\n> 2 files changed, 48 insertions(+), 5 deletions(-)\n> \n> diff --git a/t/t3032-merge-recursive-options.sh b/t/t3032-merge-recursive-options.sh\n> index 4029c9c..4cbedb4 100755\n> --- a/t/t3032-merge-recursive-options.sh\n> +++ b/t/t3032-merge-recursive-options.sh\n> @@ -204,4 +204,47 @@ test_expect_success '--ignore-space-at-eol' '\n> \ttest_cmp expected actual\n> '\n> \n> +# Setup for automerging with whitespace-only changes\n> +# on both sides and in *all* lines\n> +\n> +test_expect_success 'setup: w/s only changes in all lines on both sides' '\n> +\tgit rm -rf . &&\n> +\tgit clean -fdqx &&\n> +\trm -rf .git &&\n> +\tgit init\nmissing &&\n> +\n> +\techo \" two words\" >text.txt &&\n> +\tgit add text.txt &&\n> +\ttest_tick &&\n> +\tgit commit -m \"Initial revision\" &&\n> +\n> +\tgit checkout -b remote &&\n> +\techo \" \\t\\ttwo words \" >text.txt &&\n> +\tgit commit -a -m \"remote: insert whitespace only\" &&\n> +\n> +\tgit checkout master &&\n> +\techo \" two words\" >text.txt &&\n> +\tgit commit -a -m \"master: insert whitespace only\"\n> +'\n> +\n> +test_expect_success 'w/s only in all lines: --ignore-space-change preserves ours' '\n[]\n"},{"id":"256926","messageId":"xmqqzj7takks.fsf@gitster.dls.corp.google.com","threadId":"38692","inReplyTo":"1425404233-89907-1-git-send-email-anton.a.trunov@gmail.com","subject":"Re: [PATCH] xmerge.c: fix xdl_merge to conform with the manual","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-03T20:32:19Z","receivedAt":"2015-03-03T20:32:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Anton Trunov writes:\n\n> The git-merge manual says that the ignore-space-change,\n> ignore-all-space, ignore-space-at-eol options preserve our version\n> if their version only introduces whitespace changes to a line.\n>\n> So far if there is whitespace-only changes to both sides\n> in *all* lines their version will be used.\n\nI am having hard time understanding the last sentence, especially\nthe \"So far\" part. Do you mean \"With the current code, if ours and\ntheirs change whitespaces on all lines, we take theirs\"?\n\nI find the description in the documentation is a bit hard to read.\n\n * If 'their' version only introduces whitespace changes to a line,\n 'our' version is used;\n\n * If 'our' version introduces whitespace changes but 'their'\n version includes a substantial change, 'their' version is used;\n\n * Otherwise, the merge proceeds in the usual way.\n\nAnd it is unclear if your reading is correct to me. In your \"So\nfar\" scenario, 'our' version does introduce whitespace changes and\n'their' version does quite a bit of damage to the file (after all,\nthey both change *all* lines, right?). It does not seem too wrong\nto invoke the second clause above and take 'theirs', at least to me.\n\nIt is an entirely different matter if the behaviour the document\ndescribes is sane, and I didn't ask \"git log\" what the reasoning\nbehind that second point is, but my guess is that a change made by\nthem being \"substantial\" is a sign that it is a whitespace cleanup\nchange and we should take the cleanup in such a case, perhaps?\n"},{"id":"256963","messageId":"CAPig+cQSNLH2poz-gtrsTd5XWpUgDh6a9APfj3K7TC78trf6xA@mail.gmail.com","threadId":"38692","inReplyTo":"54F616DD.8040904@web.de","subject":"Re: [PATCH] xmerge.c: fix xdl_merge to conform with the manual","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-03-04T07:07:21Z","receivedAt":"2015-03-04T07:07:21Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Mar 3, 2015 at 3:17 PM, Torsten Bögershausen wrote:\n> On 2015-03-03 18.37, Anton Trunov wrote:\n> []\n>> Signed-off-by: Anton Trunov gmail.com>\n> Should we use the \"real email\" here (with the '@') ?\n>> ---\n>> diff --git a/t/t3032-merge-recursive-options.sh b/t/t3032-merge-recursive-options.sh\n>> index 4029c9c..4cbedb4 100755\n>> --- a/t/t3032-merge-recursive-options.sh\n>> +++ b/t/t3032-merge-recursive-options.sh\n>> @@ -204,4 +204,47 @@ test_expect_success '--ignore-space-at-eol' '\n>> test_cmp expected actual\n>> '\n>>\n>> +# Setup for automerging with whitespace-only changes\n>> +# on both sides and in *all* lines\n>> +\n>> +test_expect_success 'setup: w/s only changes in all lines on both sides' '\n>> + git rm -rf . &&\n>> + git clean -fdqx &&\n>> + rm -rf .git &&\n>> + git init\n> missing &&\n>> +\n>> + echo \" two words\" >text.txt &&\n>> + git add text.txt &&\n>> + test_tick &&\n>> + git commit -m \"Initial revision\" &&\n>> +\n>> + git checkout -b remote &&\n>> + echo \" \\t\\ttwo words \" >text.txt &&\n\nUse of echo \"\\t\" is not portable. Either embed literal tab characters\nor use printf \"\\t\".\n\n>> + git commit -a -m \"remote: insert whitespace only\" &&\n>> +\n>> + git checkout master &&\n>> + echo \" two words\" >text.txt &&\n>> + git commit -a -m \"master: insert whitespace only\"\n>> +'\n>> +\n>> +test_expect_success 'w/s only in all lines: --ignore-space-change preserves ours' '\n"},{"id":"256979","messageId":"54F6D3B0.60600@gmail.com","threadId":"38692","inReplyTo":"xmqqzj7takks.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] xmerge.c: fix xdl_merge to conform with the manual","fromName":"Anton Trunov","fromEmail":"anton.a.trunov@gmail.com","sentAt":"2015-03-04T09:43:12Z","receivedAt":"2015-03-04T09:43:12Z","isPatch":true,"sender":{"key":"anton.a.trunov@gmail.com","avatar":"https://gravatar.com/avatar/0c198a8971bd6147c766c491b7661f0a2d1875a80c5562bd70d4f5076460eccb?d=mp&s=160"},"body":"On 03/03/15 23:32, Junio C Hamano wrote:\n> Anton Trunov writes:\n> \n>> The git-merge manual says that the ignore-space-change,\n>> ignore-all-space, ignore-space-at-eol options preserve our version\n>> if their version only introduces whitespace changes to a line.\n>>\n>> So far if there is whitespace-only changes to both sides\n>> in *all* lines their version will be used.\n> \n> I am having hard time understanding the last sentence, especially\n> the \"So far\" part. Do you mean \"With the current code, if ours and\n> theirs change whitespaces on all lines, we take theirs\"?\n\nBy \"so far\" I mean \"until now, but not including it\", i.e. the code\nbefore applying the patch.\n\n> I find the description in the documentation is a bit hard to read.\n> \n> * If 'their' version only introduces whitespace changes to a line,\n> 'our' version is used;\n> \n> * If 'our' version introduces whitespace changes but 'their'\n> version includes a substantial change, 'their' version is used;\n> \n> * Otherwise, the merge proceeds in the usual way.\n> \n> And it is unclear if your reading is correct to me. In your \"So\n> far\" scenario, 'our' version does introduce whitespace changes and\n> 'their' version does quite a bit of damage to the file (after all,\n> they both change *all* lines, right?). It does not seem too wrong\n> to invoke the second clause above and take 'theirs', at least to me.\n\nLet me elaborate on this a bit.\nIt doesn't matter if all lines are changed or not.\nThe point is if all the changes in all the *changed* lines are trivial\n(non-whitespace), i.e. there is no one line with substantial change on\nboth sides, then we just through away their version and keep our\nwhitespace changes.\nWe are talking here about non-so-probable corner-case of trivial changes\nin our and their versions, perhaps an uncoordinated tabs-vs-space clean-up.\nSo I think I should add \"changed lines\" to the commit message.\n\nFor the code version before applying this patch the following scenario\nwill take place if \"git merge -Xignore-all-space remote\" gets executed.\n\nbase file:\n1st line\n2nd line\n\nmaster file:\n 1st line\n 2nd line with substantial change\n\nremote file:\n 1st line\n 2nd line\n\nmerge result file:\n 1st line\n 2nd line with substantial change\n\nSo essentially it does what \"git merge -s ours remote\" does in case if\nall their changes are trivial.\nThis seems like reasonable solution to me: we _are_ trying to ignore\nwhitespace changes and we are explicit about it.\n\nBut, in the scenario with trivial changes everywhere we get a completely\ndifferent result:\n\nbase file:\n1st line\n2nd line\n\nmaster file:\n 1st line\n 2nd line\n\nremote file:\n 1st line\n 2nd line\n\nmerge result file:\n 1st line\n 2nd line\n\nIn my opinion if we respect the principle of least astonishment this\nbehavior should be fixed to:\n\nmerge result file:\n 1st line\n 2nd line\n\nExactly so does this patch.\n\n> It is an entirely different matter if the behaviour the document\n> describes is sane, and I didn't ask \"git log\" what the reasoning\n> behind that second point is, but my guess is that a change made by\n> them being \"substantial\" is a sign that it is a whitespace cleanup\n> change and we should take the cleanup in such a case, perhaps?\n\nIf we want to take in their clean-up why would we use the\n-Xignore-space-change option in the first place?\nIt looks like we're explicitly saying \"we don't want any changes that\nare whitespace-only\", right?\nAnd if we introduced some cleanup too what should we do when the\ncleanups conflict? (exactly our case)\nAs far as I am concerned one should either manually resolve that kind of\nconflicts without using the -Xignore-... options or just\ngit merge -X theirs remote.\n"},{"id":"256983","messageId":"54F6D695.7030109@gmail.com","threadId":"38692","inReplyTo":"CAPig+cQSNLH2poz-gtrsTd5XWpUgDh6a9APfj3K7TC78trf6xA@mail.gmail.com","subject":"Re: [PATCH] xmerge.c: fix xdl_merge to conform with the manual","fromName":"Anton Trunov","fromEmail":"anton.a.trunov@gmail.com","sentAt":"2015-03-04T09:55:33Z","receivedAt":"2015-03-04T09:55:33Z","isPatch":true,"sender":{"key":"anton.a.trunov@gmail.com","avatar":"https://gravatar.com/avatar/0c198a8971bd6147c766c491b7661f0a2d1875a80c5562bd70d4f5076460eccb?d=mp&s=160"},"body":"On 04/03/15 10:07, Eric Sunshine wrote:\n> On Tue, Mar 3, 2015 at 3:17 PM, Torsten Bögershausen wrote:\n>> On 2015-03-03 18.37, Anton Trunov wrote:\n>> []\n>>> Signed-off-by: Anton Trunov gmail.com>\n>> Should we use the \"real email\" here (with the '@') ?\n>>> ---\n>>> diff --git a/t/t3032-merge-recursive-options.sh b/t/t3032-merge-recursive-options.sh\n>>> index 4029c9c..4cbedb4 100755\n>>> --- a/t/t3032-merge-recursive-options.sh\n>>> +++ b/t/t3032-merge-recursive-options.sh\n>>> @@ -204,4 +204,47 @@ test_expect_success '--ignore-space-at-eol' '\n>>> test_cmp expected actual\n>>> '\n>>>\n>>> +# Setup for automerging with whitespace-only changes\n>>> +# on both sides and in *all* lines\n>>> +\n>>> +test_expect_success 'setup: w/s only changes in all lines on both sides' '\n>>> + git rm -rf . &&\n>>> + git clean -fdqx &&\n>>> + rm -rf .git &&\n>>> + git init\n>> missing &&\n>>> +\n>>> + echo \" two words\" >text.txt &&\n>>> + git add text.txt &&\n>>> + test_tick &&\n>>> + git commit -m \"Initial revision\" &&\n>>> +\n>>> + git checkout -b remote &&\n>>> + echo \" \\t\\ttwo words \" >text.txt &&\n> \n> Use of echo \"\\t\" is not portable. Either embed literal tab characters\n> or use printf \"\\t\".\nOK.\nShouldn't it be printf \"\\t\\n\" for exact substitute for echo \"\\t\"?\n\n>>> + git commit -a -m \"remote: insert whitespace only\" &&\n>>> +\n>>> + git checkout master &&\n>>> + echo \" two words\" >text.txt &&\n>>> + git commit -a -m \"master: insert whitespace only\"\n>>> +'\n>>> +\n>>> +test_expect_success 'w/s only in all lines: --ignore-space-change preserves ours' '\n"},{"id":"256984","messageId":"54F6D766.3070607@gmail.com","threadId":"38692","inReplyTo":"54F616DD.8040904@web.de","subject":"Re: [PATCH] xmerge.c: fix xdl_merge to conform with the manual","fromName":"Anton Trunov","fromEmail":"anton.a.trunov@gmail.com","sentAt":"2015-03-04T09:59:02Z","receivedAt":"2015-03-04T09:59:02Z","isPatch":true,"sender":{"key":"anton.a.trunov@gmail.com","avatar":"https://gravatar.com/avatar/0c198a8971bd6147c766c491b7661f0a2d1875a80c5562bd70d4f5076460eccb?d=mp&s=160"},"body":"On 03/03/15 23:17, Torsten Bögershausen wrote:\n> On 2015-03-03 18.37, Anton Trunov wrote:\n> []\n>> Signed-off-by: Anton Trunov gmail.com>\n> Should we use the \"real email\" here (with the '@') ?\n\nDidn't realize the parser for the web version mangles emails.\nWill use the \"real email\".\n\n>> ---\n>> t/t3032-merge-recursive-options.sh | 43 ++++++++++++++++++++++++++++++++++++++\n>> xdiff/xmerge.c | 10 ++++-----\n>> 2 files changed, 48 insertions(+), 5 deletions(-)\n>>\n>> diff --git a/t/t3032-merge-recursive-options.sh b/t/t3032-merge-recursive-options.sh\n>> index 4029c9c..4cbedb4 100755\n>> --- a/t/t3032-merge-recursive-options.sh\n>> +++ b/t/t3032-merge-recursive-options.sh\n>> @@ -204,4 +204,47 @@ test_expect_success '--ignore-space-at-eol' '\n>> \ttest_cmp expected actual\n>> '\n>> \n>> +# Setup for automerging with whitespace-only changes\n>> +# on both sides and in *all* lines\n>> +\n>> +test_expect_success 'setup: w/s only changes in all lines on both sides' '\n>> +\tgit rm -rf . &&\n>> +\tgit clean -fdqx &&\n>> +\trm -rf .git &&\n>> +\tgit init\n> missing &&\n\nNice catch! Thank you.\n\n>> +\n>> +\techo \" two words\" >text.txt &&\n>> +\tgit add text.txt &&\n>> +\ttest_tick &&\n>> +\tgit commit -m \"Initial revision\" &&\n>> +\n>> +\tgit checkout -b remote &&\n>> +\techo \" \\t\\ttwo words \" >text.txt &&\n>> +\tgit commit -a -m \"remote: insert whitespace only\" &&\n>> +\n>> +\tgit checkout master &&\n>> +\techo \" two words\" >text.txt &&\n>> +\tgit commit -a -m \"master: insert whitespace only\"\n>> +'\n>> +\n>> +test_expect_success 'w/s only in all lines: --ignore-space-change preserves ours' '\n> []\n> \n"},{"id":"256985","messageId":"CAPig+cS2hPN_7HwmZL5xBu6XtnkyKosxCSsscU2TZcNfw+ZavA@mail.gmail.com","threadId":"38692","inReplyTo":"54F6D695.7030109@gmail.com","subject":"Re: [PATCH] xmerge.c: fix xdl_merge to conform with the manual","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-03-04T10:01:08Z","receivedAt":"2015-03-04T10:01:08Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Mar 4, 2015 at 4:55 AM, Anton Trunov wrote:\n> On 04/03/15 10:07, Eric Sunshine wrote:\n>>>> + echo \" \\t\\ttwo words \" >text.txt &&\n>>\n>> Use of echo \"\\t\" is not portable. Either embed literal tab characters\n>> or use printf \"\\t\".\n> OK.\n> Shouldn't it be printf \"\\t\\n\" for exact substitute for echo \"\\t\"?\n\nYes, that was implied; it didn't seem necessary to describe the\nconversion to printf in full detail. Sorry if there was any confusion.\n"},{"id":"257012","messageId":"xmqqfv9k8rcs.fsf@gitster.dls.corp.google.com","threadId":"38692","inReplyTo":"54F6D3B0.60600@gmail.com","subject":"Re: [PATCH] xmerge.c: fix xdl_merge to conform with the manual","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-04T20:01:07Z","receivedAt":"2015-03-04T20:01:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Anton Trunov writes:\n\n> For the code version before applying this patch the following scenario\n> will take place if \"git merge -Xignore-all-space remote\" gets executed.\n>\n> base file:\n> 1st line\n> 2nd line\n>\n> master file:\n> 1st line\n> 2nd line with substantial change\n>\n> remote file:\n> 1st line\n> 2nd line\n>\n> merge result file:\n> 1st line\n> 2nd line with substantial change\n>\n> So essentially it does what \"git merge -s ours remote\" does in case if\n> all their changes are trivial.\n> This seems like reasonable solution to me: we _are_ trying to ignore\n> whitespace changes and we are explicit about it.\n\nNow, the above makes readers wonder what happens when you merged\nmaster into the remote. I.e. in the opposite direction.\n"},{"id":"257059","messageId":"54F826DE.6010506@gmail.com","threadId":"38692","inReplyTo":"xmqqfv9k8rcs.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] xmerge.c: fix xdl_merge to conform with the manual","fromName":"Anton Trunov","fromEmail":"anton.a.trunov@gmail.com","sentAt":"2015-03-05T09:50:22Z","receivedAt":"2015-03-05T09:50:22Z","isPatch":true,"sender":{"key":"anton.a.trunov@gmail.com","avatar":"https://gravatar.com/avatar/0c198a8971bd6147c766c491b7661f0a2d1875a80c5562bd70d4f5076460eccb?d=mp&s=160"},"body":"On 04/03/15 23:01, Junio C Hamano wrote:\n> Anton Trunov writes:\n> \n>> For the code version before applying this patch the following scenario\n>> will take place if \"git merge -Xignore-all-space remote\" gets executed.\n>>\n>> base file:\n>> 1st line\n>> 2nd line\n>>\n>> master file:\n>> 1st line\n>> 2nd line with substantial change\n>>\n>> remote file:\n>> 1st line\n>> 2nd line\n>>\n>> merge result file:\n>> 1st line\n>> 2nd line with substantial change\n>>\n>> So essentially it does what \"git merge -s ours remote\" does in case if\n>> all their changes are trivial.\n>> This seems like reasonable solution to me: we _are_ trying to ignore\n>> whitespace changes and we are explicit about it.\n> \n> Now, the above makes readers wonder what happens when you merged\n> master into the remote. I.e. in the opposite direction.\n> \nInteresting observation.\nI'd like to point out that the patch doesn't change the way this merge\nworks.\n\nBut let us discuss it with the help of git log.\n\nThe function xdl_merge() was introduced to the code base in commit\n857b933e04bc21ce02043c3107c148f8dcbb4a01.\n\nAs it was pointed out in 1cd12926cedb340d176db607e087495381032ce2\n(t6023: merge-file fails to output anything for a degenerate merge) the\nmerge-file command would just refuse to merge identical changes and\nproduce output.\n\nThen in 5719db91ce5915ee07c50f1afdc94fe34e91529f (Change xdl_merge to\ngenerate output even for null merges) this was fixed.\n\nLet's return to the issue you've mentioned.\nAccording to the manual the merge result file should be:\n 1st line <- our line\n 2nd line with substantial change <- their line\n\nThe problem with the current code that it doesn't work line-wise when at\nleast one side has only whitespace changes. It just completely throws\naway a side without any changes and takes the other side.\n\nIf we wanted to fix it then I'd suggest to completely remove the checks\nfor null changes in xdl_merge() because xdl_do_merge() handles those.\n"},{"id":"257132","messageId":"54F95F25.9090300@gmail.com","threadId":"38692","inReplyTo":"xmqqfv9k8rcs.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] xmerge.c: fix xdl_merge to conform with the manual","fromName":"Anton Trunov","fromEmail":"anton.a.trunov@gmail.com","sentAt":"2015-03-06T08:02:45Z","receivedAt":"2015-03-06T08:02:45Z","isPatch":true,"sender":{"key":"anton.a.trunov@gmail.com","avatar":"https://gravatar.com/avatar/0c198a8971bd6147c766c491b7661f0a2d1875a80c5562bd70d4f5076460eccb?d=mp&s=160"},"body":"On 04/03/15 23:01, Junio C Hamano wrote:\n> [] \n\nMy apologies for pushing this topic, but what would you recommend?\nShould we treat both sides line-wise or should we correct the documentation?\n\nCurrent version for git help merge:\n\n...\nignore-space-change, ignore-all-space, ignore-space-at-eol\n Treats lines with the indicated type of whitespace change as\nunchanged for the sake of a three-way merge. Whitespace\n changes mixed with other changes to a line are not ignored. See also\ngit-diff(1)-b, -w, and --ignore-space-at-eol.\n\n o If their version only introduces whitespace changes to a line,\nour version is used;\n\n o If our version introduces whitespace changes but their version\nincludes a substantial change, their version is used;\n\n o Otherwise, the merge proceeds in the usual way.\n...\n\n\nThe 1st bullet point could be changed into\n o If their version only introduces whitespace changes to *all\nchanged lines*, our version is used;\n\nAnd the 2nd one into:\n o If our version only introduces whitespace changes to all changed\nlines, but their version includes at least one substantially changed\nline, all lines from their version are used;\n"},{"id":"257278","messageId":"xmqqtwxvsyjq.fsf@gitster.dls.corp.google.com","threadId":"38692","inReplyTo":"54F95F25.9090300@gmail.com","subject":"Re: [PATCH] xmerge.c: fix xdl_merge to conform with the manual","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-08T08:06:49Z","receivedAt":"2015-03-08T08:06:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Anton Trunov writes:\n\n> On 04/03/15 23:01, Junio C Hamano wrote:\n>\n> My apologies for pushing this topic, but what would you recommend?\n> Should we treat both sides line-wise or should we correct the documentation?\n\nMy gut feeling is that the change to swap which side is examined\nfirst would end up to be a patch to rob Peter to pay Paul, and a\nline-by-line approach might end up paying too expensive a runtime\ncost in practice (and it should not really matter which side's\nwhitespace the end result matches, because the user says \"I do not\ncare about whitespace changes\", so paying that cost is not something\nwe would want to do). So it may be that the best course of action\nmay be documentation updates.\n\nBut I haven't had a chance to think about it through yet to form a\ndefinite opinion.\n\nThanks.\n"},{"id":"257375","messageId":"54FD70FA.1080606@gmail.com","threadId":"38692","inReplyTo":"xmqqtwxvsyjq.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] xmerge.c: fix xdl_merge to conform with the manual","fromName":"Anton Trunov","fromEmail":"anton.a.trunov@gmail.com","sentAt":"2015-03-09T10:07:54Z","receivedAt":"2015-03-09T10:07:54Z","isPatch":true,"sender":{"key":"anton.a.trunov@gmail.com","avatar":"https://gravatar.com/avatar/0c198a8971bd6147c766c491b7661f0a2d1875a80c5562bd70d4f5076460eccb?d=mp&s=160"},"body":"On 08/03/15 11:06, Junio C Hamano wrote:\n> Anton Trunov writes:\n> \n>> On 04/03/15 23:01, Junio C Hamano wrote:\n>>\n>> My apologies for pushing this topic, but what would you recommend?\n>> Should we treat both sides line-wise or should we correct the documentation?\n> \n> My gut feeling is that the change to swap which side is examined\n> first would end up to be a patch to rob Peter to pay Paul, and a\n> line-by-line approach might end up paying too expensive a runtime\n> cost in practice (and it should not really matter which side's\n> whitespace the end result matches, because the user says \"I do not\n> care about whitespace changes\", so paying that cost is not something\n> we would want to do). So it may be that the best course of action\n> may be documentation updates.\n\nThanks for your reply.\nI agree with your point on performance penalty.\nBut I don't want to commit a robbery, just to commit a nice expected\nmerge result (pun intended).\n\nI believe merge is inherently asymmetric operation: as git help merge\nsays it \"incorporates changes from the named commits ... into the\ncurrent branch.\" We have a notion of direction here, right?\nSo my approach would be \"if their changes are insignificant don't take\nthem at all, just keep my version\".\n\nI guess, the case \"but if our version contains only trivial changes, and\ntheirs has some real work in it\" can be solved in different ways:\n 1) take their complete version, discarding our whitespace changes;\n 2) or merge their and our versions line-wise, keeping our lines with\ntrivial changes when their corresponding lines also have only trivial\nchanges.\n\nFirst solution may be a bit surprising at first glance (as you commented\nearlier), but if we don't want to pay performance price then (with\nproper documentation correction) it could be the solution.\n\nIn addition, if they changed something and polished indentation\nalongside then we should accept their work as a whole, and not mix our\n(indented) lines with theirs, shouldn't we?\n\nAnd one more. Let's consider merging master with two branches\n(one-by-one, as octopus doesn't work with ignore-options).\nHere I assume all changes in all branches to be whitespace-only.\n\ngit merge -Xignore-all-space -m'merge2' test-branch-2\ngit merge -Xignore-all-space -m'merge1' test-branch-1\n\ngit merge -Xignore-all-space -m'merge1' test-branch-1\ngit merge -Xignore-all-space -m'merge2' test-branch-2\n\nThose merges should produce the same results, right?\nBut with current code version that is not true.\nAnd the patch resolves this issue too, because it discards all\ninsignificant changes in _other_ branches.\n\nSo let me sum this up.\nI think we should keep the patch, but\n - change the commit message adding some of the explanations from\ncurrent email-thread;\n - add more tests to cover the situation when we don't have any\nsignificant changes and they do (resolve it in favor of their *file*);\n - correct the documentation to clarify those corner cases.\n\nI'm sorry for my long reply (maybe too long for this topic).\nThank you.\n"}]}