Re: [PATCH v2 2/2] git-merge-file: do not add LF at EOF while applying unrelated change
Hi Max, On Wed, 2 Jul 2014, Max Kirillov wrote: On Mon, Jun 30, 2014 at 04:55:10PM +0200, Johannes Schindelin wrote: I just wish the tests were a little easier to understand... What could be improved with them? Oh, I would name the files more appropriately, for example. That is, instead of test1.txt I would call it mixed-endings.txt or lf-only.txt or some such. And instead of the Latin version of Psalm 23, I would put lines into the files that describe their own role in the test, i.e. unchanged ends with a carriage return ends with a line feed unchanged or similar. Please keep in mind that this critique is most likely on my *own* work, for all I know *I* introduced those files. By the way, for \r\n eol it did even worse, adding just \n. And I guess it still adds just \n for union merge. Should file merge consider the core.eol? I think it should, and for the conflict markers also, it looks ugly when whole file has \r\n but the conflict markers have \n. But then git-merge-file could not be used outside of repository, I guess. Oh, why not? It could read the configuration if it's inside a working directory, and just read /etc/gitconfig and $HOME/.gitconfig when outside... In general, I wish file merging (and diffing) were more tolerant of the line endings in input. Because in windows environment, when people have different core.autocrlf, it becomes quite frustrating to always get conflicts and changes. Amen! Ciao, Dscho -- 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 v2 2/2] git-merge-file: do not add LF at EOF while applying unrelated change
On Wed, Jul 02, 2014 at 04:08:28PM +0200, Johannes Schindelin wrote: What could be improved with them? Oh, I would name the files more appropriately, for example. That is, instead of test1.txt I would call it mixed-endings.txt or lf-only.txt or some such. And instead of the Latin version of Psalm 23, I would put lines into the files that describe their own role in the test, i.e. unchanged ends with a carriage return ends with a line feed unchanged or similar. Please keep in mind that this critique is most likely on my *own* work, for all I know *I* introduced those files. I asked to have something in mind if I return to this. Thanks for the notes. -- Max -- 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 v2 2/2] git-merge-file: do not add LF at EOF while applying unrelated change
Johannes Schindelin johannes.schinde...@gmx.de writes: Hi Max, On Sun, 29 Jun 2014, Max Kirillov wrote: diff --git a/xdiff/xmerge.c b/xdiff/xmerge.c index 9e13b25..625198e 100644 --- a/xdiff/xmerge.c +++ b/xdiff/xmerge.c @@ -245,11 +245,11 @@ static int xdl_fill_merge_buffer(xdfenv_t *xe1, const char *name1, dest ? dest + size : NULL); /* Postimage from side #1 */ if (m-mode 1) -size += xdl_recs_copy(xe1, m-i1, m-chg1, 1, +size += xdl_recs_copy(xe1, m-i1, m-chg1, (m-mode 2), dest ? dest + size : NULL); /* Postimage from side #2 */ if (m-mode 2) -size += xdl_recs_copy(xe2, m-i2, m-chg2, 1, +size += xdl_recs_copy(xe2, m-i2, m-chg2, 0, dest ? dest + size : NULL); } else continue; Makes sense to me, especially with the nice explanation in the commit message. Having said that, here is my ACK for the current revision of the patch series ... Thanks, both. Queued. -- 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 v2 2/2] git-merge-file: do not add LF at EOF while applying unrelated change
On Mon, Jun 30, 2014 at 04:55:10PM +0200, Johannes Schindelin wrote: I just wish the tests were a little easier to understand... What could be improved with them? Having said that, here is my ACK for the current revision of the patch series Thanks. By the way, for \r\n eol it did even worse, adding just \n. And I guess it still adds just \n for union merge. Should file merge consider the core.eol? I think it should, and for the conflict markers also, it looks ugly when whole file has \r\n but the conflict markers have \n. But then git-merge-file could not be used outside of repository, I guess. In general, I wish file merging (and diffing) were more tolerant of the line endings in input. Because in windows environment, when people have different core.autocrlf, it becomes quite frustrating to always get conflicts and changes. -- Max -- 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 v2 2/2] git-merge-file: do not add LF at EOF while applying unrelated change
Hi Max, On Sun, 29 Jun 2014, Max Kirillov wrote: diff --git a/xdiff/xmerge.c b/xdiff/xmerge.c index 9e13b25..625198e 100644 --- a/xdiff/xmerge.c +++ b/xdiff/xmerge.c @@ -245,11 +245,11 @@ static int xdl_fill_merge_buffer(xdfenv_t *xe1, const char *name1, dest ? dest + size : NULL); /* Postimage from side #1 */ if (m-mode 1) - size += xdl_recs_copy(xe1, m-i1, m-chg1, 1, + size += xdl_recs_copy(xe1, m-i1, m-chg1, (m-mode 2), dest ? dest + size : NULL); /* Postimage from side #2 */ if (m-mode 2) - size += xdl_recs_copy(xe2, m-i2, m-chg2, 1, + size += xdl_recs_copy(xe2, m-i2, m-chg2, 0, dest ? dest + size : NULL); } else continue; Makes sense to me, especially with the nice explanation in the commit message. I just wish the tests were a little easier to understand... It is probably my fault because I insisted on using a text that has *nothing* to do with Git. These days, I would probably have used better file names and would have used file contents that reflect the purpose in the tests (i.e. a line saying This line ends with a carriage return or some such). Having said that, here is my ACK for the current revision of the patch series (because I know how much work it would be to fix the issue I described above, and it is an *entirely different* issue from the one you fixed with this series, too). Ciao, Dscho -- 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 v2 2/2] git-merge-file: do not add LF at EOF while applying unrelated change
If 'current-file' does not contain LF at EOF, and change between 'base-file' and 'other-file' does not change any line close to EOF, the 3-way merge should not add LF to EOF. This is what 'diff3 -m' does, and seems to be a reasonable expectation. The change which introduced the behavior is cd1d61c44f. It always calls function xdl_recs_copy() for sides with add_nl == 1. In fact, it looks like the only case when this is needed is when 2 files are being union-merged, and they do not have LF at EOF (strictly speaking, the first of them). Add tests: * merge without conflict (missing LF at EOF, away from change in the other file) and merge does not add LF away of change, to demonstrate the changed behavior. * conflict at EOF without LF resolved by --union, to verify that the union-merge at the end inerts newline between versions. * some more tests which I felt like not covering the functionality well Signed-off-by: Max Kirillov m...@max630.net --- t/t6023-merge-file.sh | 85 +++ xdiff/xmerge.c| 4 +-- 2 files changed, 87 insertions(+), 2 deletions(-) diff --git a/t/t6023-merge-file.sh b/t/t6023-merge-file.sh index 6da921c..59ae712 100755 --- a/t/t6023-merge-file.sh +++ b/t/t6023-merge-file.sh @@ -83,6 +83,23 @@ test_expect_failure merge without conflict (missing LF at EOF) \ test_expect_failure merge result added missing LF \ test_cmp test.txt test2.txt +cp new4.txt test3.txt +test_expect_success merge without conflict (missing LF at EOF, away from change in the other file) \ + git merge-file --quiet test3.txt new2.txt new3.txt + +cat expect.txt EOF +DOMINUS regit me, +et nihil mihi deerit. +In loco pascuae ibi me collocavit, +super aquam refectionis educavit me; +animam meam convertit, +deduxit me super semitas jusitiae, +EOF +printf propter nomen suum. expect.txt + +test_expect_success merge does not add LF away of change \ + test_cmp test3.txt expect.txt + cp test.txt backup.txt test_expect_success merge with conflicts \ test_must_fail git merge-file test.txt orig.txt new3.txt @@ -107,6 +124,55 @@ EOF test_expect_success expected conflict markers test_cmp test.txt expect.txt cp backup.txt test.txt + +cat expect.txt EOF +Dominus regit me, et nihil mihi deerit. +In loco pascuae ibi me collocavit, +super aquam refectionis educavit me; +animam meam convertit, +deduxit me super semitas jusitiae, +propter nomen suum. +Nam et si ambulavero in medio umbrae mortis, +non timebo mala, quoniam tu mecum es: +virga tua et baculus tuus ipsa me consolata sunt. +EOF +test_expect_success merge conflicting with --ours \ + git merge-file --ours test.txt orig.txt new3.txt test_cmp test.txt expect.txt +cp backup.txt test.txt + +cat expect.txt EOF +DOMINUS regit me, +et nihil mihi deerit. +In loco pascuae ibi me collocavit, +super aquam refectionis educavit me; +animam meam convertit, +deduxit me super semitas jusitiae, +propter nomen suum. +Nam et si ambulavero in medio umbrae mortis, +non timebo mala, quoniam tu mecum es: +virga tua et baculus tuus ipsa me consolata sunt. +EOF +test_expect_success merge conflicting with --theirs \ + git merge-file --theirs test.txt orig.txt new3.txt test_cmp test.txt expect.txt +cp backup.txt test.txt + +cat expect.txt EOF +Dominus regit me, et nihil mihi deerit. +DOMINUS regit me, +et nihil mihi deerit. +In loco pascuae ibi me collocavit, +super aquam refectionis educavit me; +animam meam convertit, +deduxit me super semitas jusitiae, +propter nomen suum. +Nam et si ambulavero in medio umbrae mortis, +non timebo mala, quoniam tu mecum es: +virga tua et baculus tuus ipsa me consolata sunt. +EOF +test_expect_success merge conflicting with --union \ + git merge-file --union test.txt orig.txt new3.txt test_cmp test.txt expect.txt +cp backup.txt test.txt + test_expect_success merge with conflicts, using -L \ test_must_fail git merge-file -L 1 -L 2 test.txt orig.txt new3.txt @@ -260,4 +326,23 @@ test_expect_success 'marker size' ' test_cmp expect actual ' +printf line1\nline2\nline3 nolf-orig.txt +printf line1\nline2\nline3x nolf-diff1.txt +printf line1\nline2\nline3y nolf-diff2.txt + +test_expect_success 'conflict at EOF without LF resolved by --ours' \ + 'git merge-file -p --ours nolf-diff1.txt nolf-orig.txt nolf-diff2.txt output.txt +printf line1\nline2\nline3x expect.txt +test_cmp expect.txt output.txt' + +test_expect_success 'conflict at EOF without LF resolved by --theirs' \ + 'git merge-file -p --theirs nolf-diff1.txt nolf-orig.txt nolf-diff2.txt output.txt +printf line1\nline2\nline3y expect.txt +test_cmp expect.txt output.txt' + +test_expect_success 'conflict at EOF without LF resolved by --union' \ + 'git merge-file -p --union nolf-diff1.txt nolf-orig.txt nolf-diff2.txt output.txt +printf line1\nline2\nline3x\nline3y expect.txt +test_cmp expect.txt output.txt' +
