{"thread":{"id":"37020","subject":"[PATCH v2 0/2] git-merge-file: do not add LF at EOF while applying unrelated change","startedAt":"2014-06-28T22:04:44Z","lastAt":"2014-07-03T04:31:41Z","messageCount":8,"participants":["Max Kirillov","Johannes Schindelin","Junio C Hamano"],"isPatch":true,"patchVersion":2,"patchTotal":2},"messages":[{"id":"245138","messageId":"1403993086-15625-1-git-send-email-max@max630.net","threadId":"37020","inReplyTo":null,"subject":"[PATCH v2 0/2] git-merge-file: do not add LF at EOF while applying unrelated change","fromName":"Max Kirillov","fromEmail":"max@max630.net","sentAt":"2014-06-28T22:04:44Z","receivedAt":"2014-06-28T22:04:44Z","isPatch":true,"sender":{"key":"max@max630.net","avatar":"https://avatars.githubusercontent.com/u/381560?v=4"},"body":"I realized the case when the newline adding can be needed.\n\nThe version 2 have this case (union-merge of changes at EOF without LF)\nfixed, with adding corresponding tests.\n\nMax Kirillov (2):\n  t6023-merge-file.sh: fix and mark as broken invalid tests\n  git-merge-file: do not add LF at EOF while applying unrelated change\n\n t/t6023-merge-file.sh | 91 +++++++++++++++++++++++++++++++++++++++++++++++++--\n xdiff/xmerge.c        |  4 +--\n 2 files changed, 90 insertions(+), 5 deletions(-)\n\n-- \n2.0.0.526.g5318336\n"},{"id":"245139","messageId":"1403993086-15625-2-git-send-email-max@max630.net","threadId":"37020","inReplyTo":"1403993086-15625-1-git-send-email-max@max630.net","subject":"[PATCH v2 1/2] t6023-merge-file.sh: fix and mark as broken invalid tests","fromName":"Max Kirillov","fromEmail":"max@max630.net","sentAt":"2014-06-28T22:04:45Z","receivedAt":"2014-06-28T22:04:45Z","isPatch":true,"sender":{"key":"max@max630.net","avatar":"https://avatars.githubusercontent.com/u/381560?v=4"},"body":"Tests \"merge without conflict (missing LF at EOF\" and \"merge result\nadded missing LF\" are meaningless - the first one is identical to\n\"merge without conflict\" and the second compares results of those\nidentical tests, which are always same.\n\nThis has been so since their addition in ba1f5f3537. Probably \"new4.txt\"\nwas meant to be used instead of \"new2.txt\". Unfortunately, the current\nmerge-file breaks with new4 - conflict is reported. They also fail at\nthat revision if fixed.\n\nFix the file reference to \"new4.txt\" and mark the tests as failing -\nthey look like legitimate expectations, just not satisfied at time\nbeing.\n\nSigned-off-by: Max Kirillov <max@max630.net>\n---\n t/t6023-merge-file.sh | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/t/t6023-merge-file.sh b/t/t6023-merge-file.sh\nindex d9f3439..6da921c 100755\n--- a/t/t6023-merge-file.sh\n+++ b/t/t6023-merge-file.sh\n@@ -77,10 +77,10 @@ test_expect_success \"merge without conflict (--quiet)\" \\\n \t\"git merge-file --quiet test.txt orig.txt new2.txt\"\n \n cp new1.txt test2.txt\n-test_expect_success \"merge without conflict (missing LF at EOF)\" \\\n-\t\"git merge-file test2.txt orig.txt new2.txt\"\n+test_expect_failure \"merge without conflict (missing LF at EOF)\" \\\n+\t\"git merge-file test2.txt orig.txt new4.txt\"\n \n-test_expect_success \"merge result added missing LF\" \\\n+test_expect_failure \"merge result added missing LF\" \\\n \t\"test_cmp test.txt test2.txt\"\n \n cp test.txt backup.txt\n-- \n2.0.0.526.g5318336\n"},{"id":"245140","messageId":"1403993086-15625-3-git-send-email-max@max630.net","threadId":"37020","inReplyTo":"1403993086-15625-1-git-send-email-max@max630.net","subject":"[PATCH v2 2/2] git-merge-file: do not add LF at EOF while applying unrelated change","fromName":"Max Kirillov","fromEmail":"max@max630.net","sentAt":"2014-06-28T22:04:46Z","receivedAt":"2014-06-28T22:04:46Z","isPatch":true,"sender":{"key":"max@max630.net","avatar":"https://avatars.githubusercontent.com/u/381560?v=4"},"body":"If 'current-file' does not contain LF at EOF, and change between\n'base-file' and 'other-file' does not change any line close to EOF, the\n3-way merge should not add LF to EOF.  This is what 'diff3 -m' does, and\nseems to be a reasonable expectation.\n\nThe change which introduced the behavior is cd1d61c44f. It always calls\nfunction xdl_recs_copy() for sides with add_nl == 1. In fact, it looks\nlike the only case when this is needed is when 2 files are being\nunion-merged, and they do not have LF at EOF (strictly speaking, the\nfirst of them).\n\nAdd tests:\n* \"merge without conflict (missing LF at EOF, away from change in the\nother file)\" and \"merge does not add LF away of change\", to demonstrate\nthe changed behavior.\n* \"conflict at EOF without LF resolved by --union\", to verify that the\nunion-merge at the end inerts newline between versions.\n* some more tests which I felt like not covering the functionality well\n\nSigned-off-by: Max Kirillov <max@max630.net>\n---\n t/t6023-merge-file.sh | 85 +++++++++++++++++++++++++++++++++++++++++++++++++++\n xdiff/xmerge.c        |  4 +--\n 2 files changed, 87 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t6023-merge-file.sh b/t/t6023-merge-file.sh\nindex 6da921c..59ae712 100755\n--- a/t/t6023-merge-file.sh\n+++ b/t/t6023-merge-file.sh\n@@ -83,6 +83,23 @@ test_expect_failure \"merge without conflict (missing LF at EOF)\" \\\n test_expect_failure \"merge result added missing LF\" \\\n \t\"test_cmp test.txt test2.txt\"\n \n+cp new4.txt test3.txt\n+test_expect_success \"merge without conflict (missing LF at EOF, away from change in the other file)\" \\\n+\t\"git merge-file --quiet test3.txt new2.txt new3.txt\"\n+\n+cat > expect.txt << EOF\n+DOMINUS regit me,\n+et nihil mihi deerit.\n+In loco pascuae ibi me collocavit,\n+super aquam refectionis educavit me;\n+animam meam convertit,\n+deduxit me super semitas jusitiae,\n+EOF\n+printf \"propter nomen suum.\" >> expect.txt\n+\n+test_expect_success \"merge does not add LF away of change\" \\\n+\t\"test_cmp test3.txt expect.txt\"\n+\n cp test.txt backup.txt\n test_expect_success \"merge with conflicts\" \\\n \t\"test_must_fail git merge-file test.txt orig.txt new3.txt\"\n@@ -107,6 +124,55 @@ EOF\n test_expect_success \"expected conflict markers\" \"test_cmp test.txt expect.txt\"\n \n cp backup.txt test.txt\n+\n+cat > expect.txt << EOF\n+Dominus regit me, et nihil mihi deerit.\n+In loco pascuae ibi me collocavit,\n+super aquam refectionis educavit me;\n+animam meam convertit,\n+deduxit me super semitas jusitiae,\n+propter nomen suum.\n+Nam et si ambulavero in medio umbrae mortis,\n+non timebo mala, quoniam tu mecum es:\n+virga tua et baculus tuus ipsa me consolata sunt.\n+EOF\n+test_expect_success \"merge conflicting with --ours\" \\\n+\t\"git merge-file --ours test.txt orig.txt new3.txt && test_cmp test.txt expect.txt\"\n+cp backup.txt test.txt\n+\n+cat > expect.txt << EOF\n+DOMINUS regit me,\n+et nihil mihi deerit.\n+In loco pascuae ibi me collocavit,\n+super aquam refectionis educavit me;\n+animam meam convertit,\n+deduxit me super semitas jusitiae,\n+propter nomen suum.\n+Nam et si ambulavero in medio umbrae mortis,\n+non timebo mala, quoniam tu mecum es:\n+virga tua et baculus tuus ipsa me consolata sunt.\n+EOF\n+test_expect_success \"merge conflicting with --theirs\" \\\n+\t\"git merge-file --theirs test.txt orig.txt new3.txt && test_cmp test.txt expect.txt\"\n+cp backup.txt test.txt\n+\n+cat > expect.txt << EOF\n+Dominus regit me, et nihil mihi deerit.\n+DOMINUS regit me,\n+et nihil mihi deerit.\n+In loco pascuae ibi me collocavit,\n+super aquam refectionis educavit me;\n+animam meam convertit,\n+deduxit me super semitas jusitiae,\n+propter nomen suum.\n+Nam et si ambulavero in medio umbrae mortis,\n+non timebo mala, quoniam tu mecum es:\n+virga tua et baculus tuus ipsa me consolata sunt.\n+EOF\n+test_expect_success \"merge conflicting with --union\" \\\n+\t\"git merge-file --union test.txt orig.txt new3.txt && test_cmp test.txt expect.txt\"\n+cp backup.txt test.txt\n+\n test_expect_success \"merge with conflicts, using -L\" \\\n \t\"test_must_fail git merge-file -L 1 -L 2 test.txt orig.txt new3.txt\"\n \n@@ -260,4 +326,23 @@ test_expect_success 'marker size' '\n \ttest_cmp expect actual\n '\n \n+printf \"line1\\nline2\\nline3\" >nolf-orig.txt\n+printf \"line1\\nline2\\nline3x\" >nolf-diff1.txt\n+printf \"line1\\nline2\\nline3y\" >nolf-diff2.txt\n+\n+test_expect_success 'conflict at EOF without LF resolved by --ours' \\\n+\t'git merge-file -p --ours nolf-diff1.txt nolf-orig.txt nolf-diff2.txt >output.txt &&\n+\t printf \"line1\\nline2\\nline3x\" >expect.txt &&\n+\t test_cmp expect.txt output.txt'\n+\n+test_expect_success 'conflict at EOF without LF resolved by --theirs' \\\n+\t'git merge-file -p --theirs nolf-diff1.txt nolf-orig.txt nolf-diff2.txt >output.txt &&\n+\t printf \"line1\\nline2\\nline3y\" >expect.txt &&\n+\t test_cmp expect.txt output.txt'\n+\n+test_expect_success 'conflict at EOF without LF resolved by --union' \\\n+\t'git merge-file -p --union nolf-diff1.txt nolf-orig.txt nolf-diff2.txt >output.txt &&\n+\t printf \"line1\\nline2\\nline3x\\nline3y\" >expect.txt &&\n+\t test_cmp expect.txt output.txt'\n+\n test_done\ndiff --git a/xdiff/xmerge.c b/xdiff/xmerge.c\nindex 9e13b25..625198e 100644\n--- a/xdiff/xmerge.c\n+++ b/xdiff/xmerge.c\n@@ -245,11 +245,11 @@ static int xdl_fill_merge_buffer(xdfenv_t *xe1, const char *name1,\n \t\t\t\t\t      dest ? dest + size : NULL);\n \t\t\t/* Postimage from side #1 */\n \t\t\tif (m->mode & 1)\n-\t\t\t\tsize += xdl_recs_copy(xe1, m->i1, m->chg1, 1,\n+\t\t\t\tsize += xdl_recs_copy(xe1, m->i1, m->chg1, (m->mode & 2),\n \t\t\t\t\t\t      dest ? dest + size : NULL);\n \t\t\t/* Postimage from side #2 */\n \t\t\tif (m->mode & 2)\n-\t\t\t\tsize += xdl_recs_copy(xe2, m->i2, m->chg2, 1,\n+\t\t\t\tsize += xdl_recs_copy(xe2, m->i2, m->chg2, 0,\n \t\t\t\t\t\t      dest ? dest + size : NULL);\n \t\t} else\n \t\t\tcontinue;\n-- \n2.0.0.526.g5318336\n"},{"id":"245190","messageId":"alpine.DEB.1.00.1406301650430.14982@s15462909.onlinehome-server.info","threadId":"37020","inReplyTo":"1403993086-15625-3-git-send-email-max@max630.net","subject":"Re: [PATCH v2 2/2] git-merge-file: do not add LF at EOF while applying unrelated change","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2014-06-30T14:55:10Z","receivedAt":"2014-06-30T14:55:10Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Max,\n\nOn Sun, 29 Jun 2014, Max Kirillov wrote:\n\n> diff --git a/xdiff/xmerge.c b/xdiff/xmerge.c\n> index 9e13b25..625198e 100644\n> --- a/xdiff/xmerge.c\n> +++ b/xdiff/xmerge.c\n> @@ -245,11 +245,11 @@ static int xdl_fill_merge_buffer(xdfenv_t *xe1, const char *name1,\n>  \t\t\t\t\t      dest ? dest + size : NULL);\n>  \t\t\t/* Postimage from side #1 */\n>  \t\t\tif (m->mode & 1)\n> -\t\t\t\tsize += xdl_recs_copy(xe1, m->i1, m->chg1, 1,\n> +\t\t\t\tsize += xdl_recs_copy(xe1, m->i1, m->chg1, (m->mode & 2),\n>  \t\t\t\t\t\t      dest ? dest + size : NULL);\n>  \t\t\t/* Postimage from side #2 */\n>  \t\t\tif (m->mode & 2)\n> -\t\t\t\tsize += xdl_recs_copy(xe2, m->i2, m->chg2, 1,\n> +\t\t\t\tsize += xdl_recs_copy(xe2, m->i2, m->chg2, 0,\n>  \t\t\t\t\t\t      dest ? dest + size : NULL);\n>  \t\t} else\n>  \t\t\tcontinue;\n\nMakes sense to me, especially with the nice explanation in the commit\nmessage. I just wish the tests were a little easier to understand... It is\nprobably my fault because I insisted on using a text that has *nothing* to\ndo with Git. These days, I would probably have used better file names and\nwould have used file contents that reflect the purpose in the tests (i.e.\na line saying \"This line ends with a carriage return\" or some such).\n\nHaving said that, here is my ACK for the current revision of the patch\nseries (because I know how much work it would be to fix the issue I\ndescribed above, and it is an *entirely different* issue from the one you\nfixed with this series, too).\n\nCiao,\nDscho\n"},{"id":"245239","messageId":"xmqqa98txbba.fsf@gitster.dls.corp.google.com","threadId":"37020","inReplyTo":"alpine.DEB.1.00.1406301650430.14982@s15462909.onlinehome-server.info","subject":"Re: [PATCH v2 2/2] git-merge-file: do not add LF at EOF while applying unrelated change","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-07-01T17:01:45Z","receivedAt":"2014-07-01T17:01:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n\n> Hi Max,\n>\n> On Sun, 29 Jun 2014, Max Kirillov wrote:\n>\n>> diff --git a/xdiff/xmerge.c b/xdiff/xmerge.c\n>> index 9e13b25..625198e 100644\n>> --- a/xdiff/xmerge.c\n>> +++ b/xdiff/xmerge.c\n>> @@ -245,11 +245,11 @@ static int xdl_fill_merge_buffer(xdfenv_t *xe1, const char *name1,\n>>  \t\t\t\t\t      dest ? dest + size : NULL);\n>>  \t\t\t/* Postimage from side #1 */\n>>  \t\t\tif (m->mode & 1)\n>> -\t\t\t\tsize += xdl_recs_copy(xe1, m->i1, m->chg1, 1,\n>> +\t\t\t\tsize += xdl_recs_copy(xe1, m->i1, m->chg1, (m->mode & 2),\n>>  \t\t\t\t\t\t      dest ? dest + size : NULL);\n>>  \t\t\t/* Postimage from side #2 */\n>>  \t\t\tif (m->mode & 2)\n>> -\t\t\t\tsize += xdl_recs_copy(xe2, m->i2, m->chg2, 1,\n>> +\t\t\t\tsize += xdl_recs_copy(xe2, m->i2, m->chg2, 0,\n>>  \t\t\t\t\t\t      dest ? dest + size : NULL);\n>>  \t\t} else\n>>  \t\t\tcontinue;\n>\n> Makes sense to me, especially with the nice explanation in the commit\n> message.\n\n> Having said that, here is my ACK for the current revision of the patch\n> series ...\n\nThanks, both.  Queued.\n"},{"id":"245280","messageId":"20140702044427.GA7325@wheezy.local","threadId":"37020","inReplyTo":"alpine.DEB.1.00.1406301650430.14982@s15462909.onlinehome-server.info","subject":"Re: [PATCH v2 2/2] git-merge-file: do not add LF at EOF while applying unrelated change","fromName":"Max Kirillov","fromEmail":"max@max630.net","sentAt":"2014-07-02T04:44:28Z","receivedAt":"2014-07-02T04:44:28Z","isPatch":true,"sender":{"key":"max@max630.net","avatar":"https://avatars.githubusercontent.com/u/381560?v=4"},"body":"On Mon, Jun 30, 2014 at 04:55:10PM +0200, Johannes Schindelin wrote:\n> I just wish the tests were a little easier to understand...\n\nWhat could be improved with them?\n\n> Having said that, here is my ACK for the current revision\n> of the patch series\n\nThanks.\n\nBy the way, for \"\\r\\n\" eol it did even worse, adding just\n\"\\n\". And I guess it still adds just \"\\n\" for union merge.\nShould file merge consider the core.eol? I think it should,\nand for the conflict markers also, it looks ugly when whole\nfile has \"\\r\\n\" but the conflict markers have \"\\n\". But then\ngit-merge-file could not be used outside of repository, I\nguess.\n\nIn general, I wish file merging (and diffing) were more\ntolerant of the line endings in input. Because in windows\nenvironment, when people have different core.autocrlf, it\nbecomes quite frustrating to always get conflicts and\nchanges.\n\n-- \nMax\n"},{"id":"245293","messageId":"alpine.DEB.1.00.1407021604510.14982@s15462909.onlinehome-server.info","threadId":"37020","inReplyTo":"20140702044427.GA7325@wheezy.local","subject":"Re: [PATCH v2 2/2] git-merge-file: do not add LF at EOF while applying unrelated change","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2014-07-02T14:08:28Z","receivedAt":"2014-07-02T14:08:28Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Max,\n\nOn Wed, 2 Jul 2014, Max Kirillov wrote:\n\n> On Mon, Jun 30, 2014 at 04:55:10PM +0200, Johannes Schindelin wrote:\n> > I just wish the tests were a little easier to understand...\n> \n> What could be improved with them?\n\nOh, I would name the files more appropriately, for example. That is,\ninstead of test1.txt I would call it mixed-endings.txt or lf-only.txt or\nsome such.\n\nAnd instead of the Latin version of Psalm 23, I would put lines into the\nfiles that describe their own role in the test, i.e.\n\n\tunchanged\n\tends with a carriage return\n\tends with a line feed\n\tunchanged\n\nor similar.\n\nPlease keep in mind that this critique is most likely on my *own* work,\nfor all I know *I* introduced those files.\n\n> By the way, for \"\\r\\n\" eol it did even worse, adding just \"\\n\". And I\n> guess it still adds just \"\\n\" for union merge.  Should file merge\n> consider the core.eol? I think it should, and for the conflict markers\n> also, it looks ugly when whole file has \"\\r\\n\" but the conflict markers\n> have \"\\n\". But then git-merge-file could not be used outside of\n> repository, I guess.\n\nOh, why not? It could read the configuration if it's inside a working\ndirectory, and just read /etc/gitconfig and $HOME/.gitconfig when\noutside...\n\n> In general, I wish file merging (and diffing) were more tolerant of the\n> line endings in input. Because in windows environment, when people have\n> different core.autocrlf, it becomes quite frustrating to always get\n> conflicts and changes.\n\nAmen!\n\nCiao,\nDscho\n"},{"id":"245366","messageId":"20140703043141.GA21237@wheezy.local","threadId":"37020","inReplyTo":"alpine.DEB.1.00.1407021604510.14982@s15462909.onlinehome-server.info","subject":"Re: [PATCH v2 2/2] git-merge-file: do not add LF at EOF while applying unrelated change","fromName":"Max Kirillov","fromEmail":"max@max630.net","sentAt":"2014-07-03T04:31:41Z","receivedAt":"2014-07-03T04:31:41Z","isPatch":true,"sender":{"key":"max@max630.net","avatar":"https://avatars.githubusercontent.com/u/381560?v=4"},"body":"On Wed, Jul 02, 2014 at 04:08:28PM +0200, Johannes Schindelin wrote:\n>> What could be improved with them?\n> \n> Oh, I would name the files more appropriately, for example. That is,\n> instead of test1.txt I would call it mixed-endings.txt or lf-only.txt or\n> some such.\n> \n> And instead of the Latin version of Psalm 23, I would put lines into the\n> files that describe their own role in the test, i.e.\n> \n> \tunchanged\n> \tends with a carriage return\n> \tends with a line feed\n> \tunchanged\n> \n> or similar.\n> \n> Please keep in mind that this critique is most likely on my *own* work,\n> for all I know *I* introduced those files.\n\nI asked to have something in mind if I return to this.\n \nThanks for the notes.\n\n-- \nMax\n"}]}