{"thread":{"id":"41241","subject":"[PATCH 0/2] Let merge-file write out conflict markers with correct EOLs","startedAt":"2016-01-22T17:01:09Z","lastAt":"2016-01-28T07:55:21Z","messageCount":55,"participants":["Johannes Schindelin","Beat Bolli","Junio C Hamano","Eric Sunshine","Torsten Bögershausen","Ramsay Jones"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"276546","messageId":"cover.1453482052.git.johannes.schindelin@gmx.de","threadId":"41241","inReplyTo":null,"subject":"[PATCH 0/2] Let merge-file write out conflict markers with correct EOLs","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-01-22T17:01:09Z","receivedAt":"2016-01-22T17:01:09Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"The original patch was sent by Beat Bolli in\nhttp://thread.gmane.org/gmane.comp.version-control.git/281600\n\nMy suggestion to extend it to respect gitattributes led to\nchanges that broke the original patch.\n\nSince there have been a couple of \"What's cooking\" mails\ncontaining unheard calls for updates on this patch, I took it\non myself to fix things.\n\nThe most crucial fix is that ll-merge.c is now *untouched*,\nas it works on blob contents (i.e. LF-only line endings),\n*not* on file contents from the working directory.\n\n\nBeat Bolli (1):\n merge-file: consider core.crlf when writing merge markers\n\nJohannes Schindelin (1):\n convert: add a helper to determine the correct EOL for a given path\n\n builtin/merge-file.c | 1 +\n convert.c | 29 +++++++++++++++++++++++++++++\n convert.h | 2 ++\n t/t6023-merge-file.sh | 14 ++++++++++++++\n xdiff/xdiff.h | 1 +\n xdiff/xmerge.c | 29 +++++++++++++++++++----------\n 6 files changed, 66 insertions(+), 10 deletions(-)\n\n-- \n2.7.0.windows.1.7.g55a05c8\n"},{"id":"276547","messageId":"c4e6707b53d82cdeeb16c83bddf9c4870a53d9df.1453482052.git.johannes.schindelin@gmx.de","threadId":"41241","inReplyTo":"cover.1453482052.git.johannes.schindelin@gmx.de","subject":"[PATCH 1/2] convert: add a helper to determine the correct EOL for a given path","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-01-22T17:01:16Z","receivedAt":"2016-01-22T17:01:16Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Signed-off-by: Johannes Schindelin \n---\n convert.c | 29 +++++++++++++++++++++++++++++\n convert.h | 2 ++\n 2 files changed, 31 insertions(+)\n\ndiff --git a/convert.c b/convert.c\nindex 814e814..b458734 100644\n--- a/convert.c\n+++ b/convert.c\n@@ -758,6 +758,35 @@ static void convert_attrs(struct conv_attrs *ca, const char *path)\n \t}\n }\n \n+enum eol eol_for_path(const char *path, const char *src, size_t len)\n+{\n+\tstruct conv_attrs ca;\n+\tstruct text_stat stats;\n+\n+\tif (!path) {\n+\t\tmemset(&ca, 0, sizeof(ca));\n+\t\tca.crlf_action = CRLF_AUTO;\n+\t\tca.eol_attr = EOL_UNSET;\n+\t} else {\n+\t\tconvert_attrs(&ca, path);\n+\t\tif (ca.eol_attr == EOL_UNSET)\n+\t\t\tca.eol_attr = output_eol(ca.crlf_action);\n+\t\tif (ca.eol_attr != EOL_UNSET)\n+\t\t\treturn ca.eol_attr;\n+\t}\n+\tif (!len || (ca.crlf_action != CRLF_AUTO &&\n+\t\t\t\tca.crlf_action != CRLF_GUESS))\n+\t\treturn core_eol;\n+\tca.crlf_action = input_crlf_action(ca.crlf_action, ca.eol_attr);\n+\tgather_stats(src, len, &stats);\n+\tif (ca.crlf_action == CRLF_GUESS && stats.cr > stats.crlf)\n+\t\treturn core_eol;\n+\telse if (stats.crlf)\n+\t\treturn EOL_CRLF;\n+\telse\n+\t\treturn EOL_LF;\n+}\n+\n int would_convert_to_git_filter_fd(const char *path)\n {\n \tstruct conv_attrs ca;\ndiff --git a/convert.h b/convert.h\nindex d9d853c..1892867 100644\n--- a/convert.h\n+++ b/convert.h\n@@ -33,6 +33,8 @@ enum eol {\n \n extern enum eol core_eol;\n \n+extern enum eol eol_for_path(const char *path, const char *src, size_t len);\n+\n /* returns 1 if *dst was used */\n extern int convert_to_git(const char *path, const char *src, size_t len,\n \t\t\t struct strbuf *dst, enum safe_crlf checksafe);\n-- \n2.7.0.windows.1.7.g55a05c8\n"},{"id":"276548","messageId":"c0c775ea7a9ba3244748b784241de685cefc73b1.1453482052.git.johannes.schindelin@gmx.de","threadId":"41241","inReplyTo":"cover.1453482052.git.johannes.schindelin@gmx.de","subject":"[PATCH 2/2] merge-file: consider core.crlf when writing merge markers","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-01-22T17:01:25Z","receivedAt":"2016-01-22T17:01:25Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Beat Bolli \n\nWhen merging files in repos with core.eol = crlf, git merge-file inserts\njust a LF at the end of the merge markers. Files with mixed line endings\ncause trouble in Windows editors and e.g. contrib/git-jump, where an\nunmerged file in a run of \"git jump merge\" is reported as simply \"binary\nfile matches\".\n\nFixing this improves merge-file's behavior under Windows.\n\nThe original version of this patch also modified ll_merge(), but that\nwas incorrect: low-level merge operates on blobs, not on working files.\nTherefore, the data passed to the low-level merge, as well as its\nresult, is expected to have LF-only line endings.\n\nIt is the duty of ll_merge()'s *caller* (in case of Git's `merge`\ncommand, the merge_content() function) to convert the merge result into\nthe correct working file contents, and ll_merge() should not muck with\nline endings at all.\n\nSigned-off-by: Beat Bolli \nSigned-off-by: Jeff King \nSigned-off-by: Johannes Schindelin \n---\n builtin/merge-file.c | 1 +\n t/t6023-merge-file.sh | 14 ++++++++++++++\n xdiff/xdiff.h | 1 +\n xdiff/xmerge.c | 29 +++++++++++++++++++----------\n 4 files changed, 35 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/merge-file.c b/builtin/merge-file.c\nindex 5544705..9ce830a 100644\n--- a/builtin/merge-file.c\n+++ b/builtin/merge-file.c\n@@ -81,6 +81,7 @@ int cmd_merge_file(int argc, const char **argv, const char *prefix)\n \t\t\t\t\targv[i]);\n \t}\n \n+\txmp.crlf = eol_for_path(names[0], NULL, 0) == EOL_CRLF;\n \txmp.ancestor = names[1];\n \txmp.file1 = names[0];\n \txmp.file2 = names[2];\ndiff --git a/t/t6023-merge-file.sh b/t/t6023-merge-file.sh\nindex 190ee90..a131749 100755\n--- a/t/t6023-merge-file.sh\n+++ b/t/t6023-merge-file.sh\n@@ -346,4 +346,18 @@ test_expect_success 'conflict at EOF without LF resolved by --union' \\\n \t printf \"line1\\nline2\\nline3x\\nline3y\" >expect.txt &&\n \t test_cmp expect.txt output.txt'\n \n+test_expect_success 'conflict markers contain CRLF when core.eol=crlf' '\n+\ttest_must_fail git -c core.eol=crlf merge-file -p \\\n+\t\tnolf-diff1.txt nolf-orig.txt nolf-diff2.txt >output.txt &&\n+\ttest $(sed -n \"/\\.txt\\r$/p\" output.txt | wc -l) = 3\n+'\n+\n+test_expect_success 'conflict markers heed gitattributes over core.eol=crlf' '\n+\tgit config core.eol crlf &&\n+\techo \"*.txt eol=lf\" >>.gitattributes &&\n+\ttest_must_fail git -c core.eol=crlf merge-file -p \\\n+\t\tnolf-diff1.txt nolf-orig.txt nolf-diff2.txt >output.txt &&\n+\ttest $(sed -n \"/\\.txt\\r$/p\" output.txt | wc -l) = 0\n+'\n+\n test_done\ndiff --git a/xdiff/xdiff.h b/xdiff/xdiff.h\nindex c033991..a5bea4a 100644\n--- a/xdiff/xdiff.h\n+++ b/xdiff/xdiff.h\n@@ -122,6 +122,7 @@ typedef struct s_xmparam {\n \tint level;\n \tint favor;\n \tint style;\n+\tint crlf;\n \tconst char *ancestor;\t/* label for orig */\n \tconst char *file1;\t/* label for mf1 */\n \tconst char *file2;\t/* label for mf2 */\ndiff --git a/xdiff/xmerge.c b/xdiff/xmerge.c\nindex 625198e..4ff0db4 100644\n--- a/xdiff/xmerge.c\n+++ b/xdiff/xmerge.c\n@@ -146,7 +146,7 @@ static int xdl_orig_copy(xdfenv_t *xe, int i, int count, int add_nl, char *dest)\n static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,\n \t\t\t xdfenv_t *xe2, const char *name2,\n \t\t\t const char *name3,\n-\t\t\t int size, int i, int style,\n+\t\t\t int size, int i, int style, int crlf,\n \t\t\t xdmerge_t *m, char *dest, int marker_size)\n {\n \tint marker1_size = (name1 ? strlen(name1) + 1 : 0);\n@@ -161,7 +161,7 @@ static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,\n \t\t\t dest ? dest + size : NULL);\n \n \tif (!dest) {\n-\t\tsize += marker_size + 1 + marker1_size;\n+\t\tsize += marker_size + 1 + crlf + marker1_size;\n \t} else {\n \t\tmemset(dest + size, '<', marker_size);\n \t\tsize += marker_size;\n@@ -170,6 +170,8 @@ static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,\n \t\t\tmemcpy(dest + size + 1, name1, marker1_size - 1);\n \t\t\tsize += marker1_size;\n \t\t}\n+\t\tif (crlf)\n+\t\t\tdest[size++] = '\\r';\n \t\tdest[size++] = '\\n';\n \t}\n \n@@ -180,7 +182,7 @@ static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,\n \tif (style == XDL_MERGE_DIFF3) {\n \t\t/* Shared preimage */\n \t\tif (!dest) {\n-\t\t\tsize += marker_size + 1 + marker3_size;\n+\t\t\tsize += marker_size + 1 + crlf + marker3_size;\n \t\t} else {\n \t\t\tmemset(dest + size, '|', marker_size);\n \t\t\tsize += marker_size;\n@@ -189,6 +191,8 @@ static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,\n \t\t\t\tmemcpy(dest + size + 1, name3, marker3_size - 1);\n \t\t\t\tsize += marker3_size;\n \t\t\t}\n+\t\t\tif (crlf)\n+\t\t\t\tdest[size++] = '\\r';\n \t\t\tdest[size++] = '\\n';\n \t\t}\n \t\tsize += xdl_orig_copy(xe1, m->i0, m->chg0, 1,\n@@ -196,10 +200,12 @@ static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,\n \t}\n \n \tif (!dest) {\n-\t\tsize += marker_size + 1;\n+\t\tsize += marker_size + 1 + crlf;\n \t} else {\n \t\tmemset(dest + size, '=', marker_size);\n \t\tsize += marker_size;\n+\t\tif (crlf)\n+\t\t\tdest[size++] = '\\r';\n \t\tdest[size++] = '\\n';\n \t}\n \n@@ -207,7 +213,7 @@ static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,\n \tsize += xdl_recs_copy(xe2, m->i2, m->chg2, 1,\n \t\t\t dest ? dest + size : NULL);\n \tif (!dest) {\n-\t\tsize += marker_size + 1 + marker2_size;\n+\t\tsize += marker_size + 1 + crlf + marker2_size;\n \t} else {\n \t\tmemset(dest + size, '>', marker_size);\n \t\tsize += marker_size;\n@@ -216,6 +222,8 @@ static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,\n \t\t\tmemcpy(dest + size + 1, name2, marker2_size - 1);\n \t\t\tsize += marker2_size;\n \t\t}\n+\t\tif (crlf)\n+\t\t\tdest[size++] = '\\r';\n \t\tdest[size++] = '\\n';\n \t}\n \treturn size;\n@@ -226,7 +234,7 @@ static int xdl_fill_merge_buffer(xdfenv_t *xe1, const char *name1,\n \t\t\t\t const char *ancestor_name,\n \t\t\t\t int favor,\n \t\t\t\t xdmerge_t *m, char *dest, int style,\n-\t\t\t\t int marker_size)\n+\t\t\t\t int crlf, int marker_size)\n {\n \tint size, i;\n \n@@ -237,8 +245,8 @@ static int xdl_fill_merge_buffer(xdfenv_t *xe1, const char *name1,\n \t\tif (m->mode == 0)\n \t\t\tsize = fill_conflict_hunk(xe1, name1, xe2, name2,\n \t\t\t\t\t\t ancestor_name,\n-\t\t\t\t\t\t size, i, style, m, dest,\n-\t\t\t\t\t\t marker_size);\n+\t\t\t\t\t\t size, i, style, crlf, m,\n+\t\t\t\t\t\t dest, marker_size);\n \t\telse if (m->mode & 3) {\n \t\t\t/* Before conflicting part */\n \t\t\tsize += xdl_recs_copy(xe1, i, m->i1 - i, 0,\n@@ -419,6 +427,7 @@ static int xdl_do_merge(xdfenv_t *xe1, xdchange_t *xscr1,\n \tint level = xmp->level;\n \tint style = xmp->style;\n \tint favor = xmp->favor;\n+\tint crlf = xmp->crlf;\n \n \tif (style == XDL_MERGE_DIFF3) {\n \t\t/*\n@@ -554,7 +563,7 @@ static int xdl_do_merge(xdfenv_t *xe1, xdchange_t *xscr1,\n \t\tint size = xdl_fill_merge_buffer(xe1, name1, xe2, name2,\n \t\t\t\t\t\t ancestor_name,\n \t\t\t\t\t\t favor, changes, NULL, style,\n-\t\t\t\t\t\t marker_size);\n+\t\t\t\t\t\t crlf, marker_size);\n \t\tresult->ptr = xdl_malloc(size);\n \t\tif (!result->ptr) {\n \t\t\txdl_cleanup_merge(changes);\n@@ -563,7 +572,7 @@ static int xdl_do_merge(xdfenv_t *xe1, xdchange_t *xscr1,\n \t\tresult->size = size;\n \t\txdl_fill_merge_buffer(xe1, name1, xe2, name2,\n \t\t\t\t ancestor_name, favor, changes,\n-\t\t\t\t result->ptr, style, marker_size);\n+\t\t\t\t result->ptr, style, crlf, marker_size);\n \t}\n \treturn xdl_cleanup_merge(changes);\n }\n-- \n2.7.0.windows.1.7.g55a05c8\n"},{"id":"276555","messageId":"56A26C79.1000506@drbeat.li","threadId":"41241","inReplyTo":"cover.1453482052.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH 0/2] Let merge-file write out conflict markers with correct EOLs","fromName":"Beat Bolli","fromEmail":"dev+git@drbeat.li","sentAt":"2016-01-22T17:52:57Z","receivedAt":"2016-01-22T17:52:57Z","isPatch":true,"sender":{"key":"dev+git@drbeat.li","avatar":"https://avatars.githubusercontent.com/u/21444?v=4"},"body":"On 22.01.16 18:01, Johannes Schindelin wrote:\n> The original patch was sent by Beat Bolli in\n> http://thread.gmane.org/gmane.comp.version-control.git/281600\n> \n> My suggestion to extend it to respect gitattributes led to\n> changes that broke the original patch.\n> \n> Since there have been a couple of \"What's cooking\" mails\n> containing unheard calls for updates on this patch, I took it\n> on myself to fix things.\n\nThanks a lot! I was really stuck on the heuristics of eol_for_path(), so\nI'm very glad you took over.\n\nRegards,\nBeat\n"},{"id":"276559","messageId":"xmqqmvrxv5tn.fsf@gitster.mtv.corp.google.com","threadId":"41241","inReplyTo":"c0c775ea7a9ba3244748b784241de685cefc73b1.1453482052.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH 2/2] merge-file: consider core.crlf when writing merge markers","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-01-22T18:15:32Z","receivedAt":"2016-01-22T18:15:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> The original version of this patch also modified ll_merge(), but that\n> was incorrect: low-level merge operates on blobs, not on working files.\n> Therefore, the data passed to the low-level merge, as well as its\n> result, is expected to have LF-only line endings.\n>\n> It is the duty of ll_merge()'s *caller* (in case of Git's `merge`\n> command, the merge_content() function) to convert the merge result into\n> the correct working file contents, and ll_merge() should not muck with\n> line endings at all.\n\nHmph, aren't there people who use CRLF throughout their set-up (that\nis, it is normal for both of their blobs and working tree files to\nuse CRLF line endings)? Or is such a setting illegal and unsupported?\n\nIf such a set-up is supported, using \"<<<\\r\\n\" etc. instead of the\nusual \"<<<\\n\" would be the correct thing to do.\n\nIn a setting where people use whatever they like on the filesystem\nbut their blobs are standardized to use LF, you are right. Contents\nthat come from the filesystem and from the object store should be\nmade to the internal format, by using convert_to_git() as necessary,\nand adding the markers with \"<<<\\n\" without CR, and writing out the\nresult as CRLF (or whatever their local convention is) is absolutely\nthe right thing to do.\n\nIt's just I am not sure what our stance is about storing CRLF blobs.\nIIRC, in the meeting we met in person the last time I thought I\nheard some from the Windows camp talk against use of any of the\ncore.crlf and other settings, saying that it is much better using\nthe platform native convention throughout, and I got an impression\nthat it is a recommended practice at least in some circles.\n\nWould the approach taken by this patch still work for them, too?\n\n> Signed-off-by: Beat Bolli \n> Signed-off-by: Jeff King \n> Signed-off-by: Johannes Schindelin \n> ---\n> builtin/merge-file.c | 1 +\n> t/t6023-merge-file.sh | 14 ++++++++++++++\n> xdiff/xdiff.h | 1 +\n> xdiff/xmerge.c | 29 +++++++++++++++++++----------\n> 4 files changed, 35 insertions(+), 10 deletions(-)\n>\n> diff --git a/builtin/merge-file.c b/builtin/merge-file.c\n> index 5544705..9ce830a 100644\n> --- a/builtin/merge-file.c\n> +++ b/builtin/merge-file.c\n> @@ -81,6 +81,7 @@ int cmd_merge_file(int argc, const char **argv, const char *prefix)\n> \t\t\t\t\targv[i]);\n> \t}\n> \n> +\txmp.crlf = eol_for_path(names[0], NULL, 0) == EOL_CRLF;\n> \txmp.ancestor = names[1];\n> \txmp.file1 = names[0];\n> \txmp.file2 = names[2];\n> diff --git a/t/t6023-merge-file.sh b/t/t6023-merge-file.sh\n> index 190ee90..a131749 100755\n> --- a/t/t6023-merge-file.sh\n> +++ b/t/t6023-merge-file.sh\n> @@ -346,4 +346,18 @@ test_expect_success 'conflict at EOF without LF resolved by --union' \\\n> \t printf \"line1\\nline2\\nline3x\\nline3y\" >expect.txt &&\n> \t test_cmp expect.txt output.txt'\n> \n> +test_expect_success 'conflict markers contain CRLF when core.eol=crlf' '\n> +\ttest_must_fail git -c core.eol=crlf merge-file -p \\\n> +\t\tnolf-diff1.txt nolf-orig.txt nolf-diff2.txt >output.txt &&\n> +\ttest $(sed -n \"/\\.txt\\r$/p\" output.txt | wc -l) = 3\n> +'\n> +\n> +test_expect_success 'conflict markers heed gitattributes over core.eol=crlf' '\n> +\tgit config core.eol crlf &&\n> +\techo \"*.txt eol=lf\" >>.gitattributes &&\n> +\ttest_must_fail git -c core.eol=crlf merge-file -p \\\n> +\t\tnolf-diff1.txt nolf-orig.txt nolf-diff2.txt >output.txt &&\n> +\ttest $(sed -n \"/\\.txt\\r$/p\" output.txt | wc -l) = 0\n> +'\n> +\n> test_done\n> diff --git a/xdiff/xdiff.h b/xdiff/xdiff.h\n> index c033991..a5bea4a 100644\n> --- a/xdiff/xdiff.h\n> +++ b/xdiff/xdiff.h\n> @@ -122,6 +122,7 @@ typedef struct s_xmparam {\n> \tint level;\n> \tint favor;\n> \tint style;\n> +\tint crlf;\n> \tconst char *ancestor;\t/* label for orig */\n> \tconst char *file1;\t/* label for mf1 */\n> \tconst char *file2;\t/* label for mf2 */\n> diff --git a/xdiff/xmerge.c b/xdiff/xmerge.c\n> index 625198e..4ff0db4 100644\n> --- a/xdiff/xmerge.c\n> +++ b/xdiff/xmerge.c\n> @@ -146,7 +146,7 @@ static int xdl_orig_copy(xdfenv_t *xe, int i, int count, int add_nl, char *dest)\n> static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,\n> \t\t\t xdfenv_t *xe2, const char *name2,\n> \t\t\t const char *name3,\n> -\t\t\t int size, int i, int style,\n> +\t\t\t int size, int i, int style, int crlf,\n> \t\t\t xdmerge_t *m, char *dest, int marker_size)\n> {\n> \tint marker1_size = (name1 ? strlen(name1) + 1 : 0);\n> @@ -161,7 +161,7 @@ static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,\n> \t\t\t dest ? dest + size : NULL);\n> \n> \tif (!dest) {\n> -\t\tsize += marker_size + 1 + marker1_size;\n> +\t\tsize += marker_size + 1 + crlf + marker1_size;\n> \t} else {\n> \t\tmemset(dest + size, '<', marker_size);\n> \t\tsize += marker_size;\n> @@ -170,6 +170,8 @@ static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,\n> \t\t\tmemcpy(dest + size + 1, name1, marker1_size - 1);\n> \t\t\tsize += marker1_size;\n> \t\t}\n> +\t\tif (crlf)\n> +\t\t\tdest[size++] = '\\r';\n> \t\tdest[size++] = '\\n';\n> \t}\n> \n> @@ -180,7 +182,7 @@ static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,\n> \tif (style == XDL_MERGE_DIFF3) {\n> \t\t/* Shared preimage */\n> \t\tif (!dest) {\n> -\t\t\tsize += marker_size + 1 + marker3_size;\n> +\t\t\tsize += marker_size + 1 + crlf + marker3_size;\n> \t\t} else {\n> \t\t\tmemset(dest + size, '|', marker_size);\n> \t\t\tsize += marker_size;\n> @@ -189,6 +191,8 @@ static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,\n> \t\t\t\tmemcpy(dest + size + 1, name3, marker3_size - 1);\n> \t\t\t\tsize += marker3_size;\n> \t\t\t}\n> +\t\t\tif (crlf)\n> +\t\t\t\tdest[size++] = '\\r';\n> \t\t\tdest[size++] = '\\n';\n> \t\t}\n> \t\tsize += xdl_orig_copy(xe1, m->i0, m->chg0, 1,\n> @@ -196,10 +200,12 @@ static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,\n> \t}\n> \n> \tif (!dest) {\n> -\t\tsize += marker_size + 1;\n> +\t\tsize += marker_size + 1 + crlf;\n> \t} else {\n> \t\tmemset(dest + size, '=', marker_size);\n> \t\tsize += marker_size;\n> +\t\tif (crlf)\n> +\t\t\tdest[size++] = '\\r';\n> \t\tdest[size++] = '\\n';\n> \t}\n> \n> @@ -207,7 +213,7 @@ static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,\n> \tsize += xdl_recs_copy(xe2, m->i2, m->chg2, 1,\n> \t\t\t dest ? dest + size : NULL);\n> \tif (!dest) {\n> -\t\tsize += marker_size + 1 + marker2_size;\n> +\t\tsize += marker_size + 1 + crlf + marker2_size;\n> \t} else {\n> \t\tmemset(dest + size, '>', marker_size);\n> \t\tsize += marker_size;\n> @@ -216,6 +222,8 @@ static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,\n> \t\t\tmemcpy(dest + size + 1, name2, marker2_size - 1);\n> \t\t\tsize += marker2_size;\n> \t\t}\n> +\t\tif (crlf)\n> +\t\t\tdest[size++] = '\\r';\n> \t\tdest[size++] = '\\n';\n> \t}\n> \treturn size;\n> @@ -226,7 +234,7 @@ static int xdl_fill_merge_buffer(xdfenv_t *xe1, const char *name1,\n> \t\t\t\t const char *ancestor_name,\n> \t\t\t\t int favor,\n> \t\t\t\t xdmerge_t *m, char *dest, int style,\n> -\t\t\t\t int marker_size)\n> +\t\t\t\t int crlf, int marker_size)\n> {\n> \tint size, i;\n> \n> @@ -237,8 +245,8 @@ static int xdl_fill_merge_buffer(xdfenv_t *xe1, const char *name1,\n> \t\tif (m->mode == 0)\n> \t\t\tsize = fill_conflict_hunk(xe1, name1, xe2, name2,\n> \t\t\t\t\t\t ancestor_name,\n> -\t\t\t\t\t\t size, i, style, m, dest,\n> -\t\t\t\t\t\t marker_size);\n> +\t\t\t\t\t\t size, i, style, crlf, m,\n> +\t\t\t\t\t\t dest, marker_size);\n> \t\telse if (m->mode & 3) {\n> \t\t\t/* Before conflicting part */\n> \t\t\tsize += xdl_recs_copy(xe1, i, m->i1 - i, 0,\n> @@ -419,6 +427,7 @@ static int xdl_do_merge(xdfenv_t *xe1, xdchange_t *xscr1,\n> \tint level = xmp->level;\n> \tint style = xmp->style;\n> \tint favor = xmp->favor;\n> +\tint crlf = xmp->crlf;\n> \n> \tif (style == XDL_MERGE_DIFF3) {\n> \t\t/*\n> @@ -554,7 +563,7 @@ static int xdl_do_merge(xdfenv_t *xe1, xdchange_t *xscr1,\n> \t\tint size = xdl_fill_merge_buffer(xe1, name1, xe2, name2,\n> \t\t\t\t\t\t ancestor_name,\n> \t\t\t\t\t\t favor, changes, NULL, style,\n> -\t\t\t\t\t\t marker_size);\n> +\t\t\t\t\t\t crlf, marker_size);\n> \t\tresult->ptr = xdl_malloc(size);\n> \t\tif (!result->ptr) {\n> \t\t\txdl_cleanup_merge(changes);\n> @@ -563,7 +572,7 @@ static int xdl_do_merge(xdfenv_t *xe1, xdchange_t *xscr1,\n> \t\tresult->size = size;\n> \t\txdl_fill_merge_buffer(xe1, name1, xe2, name2,\n> \t\t\t\t ancestor_name, favor, changes,\n> -\t\t\t\t result->ptr, style, marker_size);\n> +\t\t\t\t result->ptr, style, crlf, marker_size);\n> \t}\n> \treturn xdl_cleanup_merge(changes);\n> }\n"},{"id":"276561","messageId":"alpine.DEB.2.20.1601221937440.2964@virtualbox","threadId":"41241","inReplyTo":"xmqqmvrxv5tn.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 2/2] merge-file: consider core.crlf when writing merge markers","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-01-22T18:47:38Z","receivedAt":"2016-01-22T18:47:38Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Fri, 22 Jan 2016, Junio C Hamano wrote:\n\n> Johannes Schindelin writes:\n> \n> > The original version of this patch also modified ll_merge(), but that\n> > was incorrect: low-level merge operates on blobs, not on working\n> > files. Therefore, the data passed to the low-level merge, as well as\n> > its result, is expected to have LF-only line endings.\n> >\n> > It is the duty of ll_merge()'s *caller* (in case of Git's `merge`\n> > command, the merge_content() function) to convert the merge result\n> > into the correct working file contents, and ll_merge() should not muck\n> > with line endings at all.\n> \n> Hmph, aren't there people who use CRLF throughout their set-up (that is,\n> it is normal for both of their blobs and working tree files to use CRLF\n> line endings)? Or is such a setting illegal and unsupported?\n\nGood point.\n\nWhile I would love to simply not support such a case, it would be turning\na blind eye to reality.\n\nSo I guess I need another patch like this (plus a test case):\n\n-- snipsnap --\nt a/ll-merge.c b/ll-merge.c\nindex 0338630..4a565c8 100644\n--- a/ll-merge.c\n+++ b/ll-merge.c\n@@ -111,6 +111,7 @@ static int ll_xdl_merge(const struct ll_merge_driver\n*drv_unused,\n \t\txmp.style = git_xmerge_style;\n \tif (marker_size > 0)\n \t\txmp.marker_size = marker_size;\n+\txmp.crlf = eol_for_path(NULL, src1->ptr, src1->size) == EOL_CRLF;\n \txmp.ancestor = orig_name;\n \txmp.file1 = name1;\n \txmp.file2 = name2;\n"},{"id":"276562","messageId":"xmqqegd9v4bl.fsf@gitster.mtv.corp.google.com","threadId":"41241","inReplyTo":"c4e6707b53d82cdeeb16c83bddf9c4870a53d9df.1453482052.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH 1/2] convert: add a helper to determine the correct EOL for a given path","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-01-22T18:47:58Z","receivedAt":"2016-01-22T18:47:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> Signed-off-by: Johannes Schindelin \n> ---\n\nThis change somehow ringed a bell and reminded me of your recent\nls-files stuff. Are there things that these topics can use from\neach other?\n\nThese topics are similar in that they add many lines of code as\n\"helpers\" that inspect data and guess what the existing code would\ndo to the data, without refactoring much existing code whose\nbehaviour these helpers are guessing/defining to use them, which is\nsomewhat disturbing, as the behaviour the actual code exhibits and\nthe guesses the helpers make can easily drift apart.\n\n\n\n> convert.c | 29 +++++++++++++++++++++++++++++\n> convert.h | 2 ++\n> 2 files changed, 31 insertions(+)\n>\n> diff --git a/convert.c b/convert.c\n> index 814e814..b458734 100644\n> --- a/convert.c\n> +++ b/convert.c\n> @@ -758,6 +758,35 @@ static void convert_attrs(struct conv_attrs *ca, const char *path)\n> \t}\n> }\n> \n> +enum eol eol_for_path(const char *path, const char *src, size_t len)\n> +{\n> +\tstruct conv_attrs ca;\n> +\tstruct text_stat stats;\n> +\n> +\tif (!path) {\n> +\t\tmemset(&ca, 0, sizeof(ca));\n> +\t\tca.crlf_action = CRLF_AUTO;\n> +\t\tca.eol_attr = EOL_UNSET;\n> +\t} else {\n> +\t\tconvert_attrs(&ca, path);\n> +\t\tif (ca.eol_attr == EOL_UNSET)\n> +\t\t\tca.eol_attr = output_eol(ca.crlf_action);\n> +\t\tif (ca.eol_attr != EOL_UNSET)\n> +\t\t\treturn ca.eol_attr;\n> +\t}\n> +\tif (!len || (ca.crlf_action != CRLF_AUTO &&\n> +\t\t\t\tca.crlf_action != CRLF_GUESS))\n> +\t\treturn core_eol;\n> +\tca.crlf_action = input_crlf_action(ca.crlf_action, ca.eol_attr);\n> +\tgather_stats(src, len, &stats);\n> +\tif (ca.crlf_action == CRLF_GUESS && stats.cr > stats.crlf)\n> +\t\treturn core_eol;\n> +\telse if (stats.crlf)\n> +\t\treturn EOL_CRLF;\n> +\telse\n> +\t\treturn EOL_LF;\n> +}\n> +\n> int would_convert_to_git_filter_fd(const char *path)\n> {\n> \tstruct conv_attrs ca;\n> diff --git a/convert.h b/convert.h\n> index d9d853c..1892867 100644\n> --- a/convert.h\n> +++ b/convert.h\n> @@ -33,6 +33,8 @@ enum eol {\n> \n> extern enum eol core_eol;\n> \n> +extern enum eol eol_for_path(const char *path, const char *src, size_t len);\n> +\n> /* returns 1 if *dst was used */\n> extern int convert_to_git(const char *path, const char *src, size_t len,\n> \t\t\t struct strbuf *dst, enum safe_crlf checksafe);\n"},{"id":"276567","messageId":"alpine.DEB.2.20.1601222003360.2964@virtualbox","threadId":"41241","inReplyTo":"xmqqegd9v4bl.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 1/2] convert: add a helper to determine the correct EOL for a given path","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-01-22T19:04:41Z","receivedAt":"2016-01-22T19:04:41Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Fri, 22 Jan 2016, Junio C Hamano wrote:\n\n> Johannes Schindelin writes:\n> \n> > Signed-off-by: Johannes Schindelin \n> > ---\n> \n> This change somehow ringed a bell and reminded me of your recent\n> ls-files stuff. Are there things that these topics can use from\n> each other?\n\nQuite possibly. I'll have a look tomorrow.\n\nCiao,\nDscho\n"},{"id":"276568","messageId":"xmqqa8nxv3do.fsf@gitster.mtv.corp.google.com","threadId":"41241","inReplyTo":"alpine.DEB.2.20.1601221937440.2964@virtualbox","subject":"Re: [PATCH 2/2] merge-file: consider core.crlf when writing merge markers","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-01-22T19:08:19Z","receivedAt":"2016-01-22T19:08:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n>> > It is the duty of ll_merge()'s *caller* (in case of Git's `merge`\n>> > command, the merge_content() function) to convert the merge result\n>> > into the correct working file contents, and ll_merge() should not muck\n>> > with line endings at all.\n>> \n>> Hmph, aren't there people who use CRLF throughout their set-up (that is,\n>> it is normal for both of their blobs and working tree files to use CRLF\n>> line endings)? Or is such a setting illegal and unsupported?\n>\n> Good point.\n>\n> While I would love to simply not support such a case, it would be turning\n> a blind eye to reality.\n>\n> So I guess I need another patch like this (plus a test case):\n>\n> -- snipsnap --\n> t a/ll-merge.c b/ll-merge.c\n> index 0338630..4a565c8 100644\n> --- a/ll-merge.c\n> +++ b/ll-merge.c\n> @@ -111,6 +111,7 @@ static int ll_xdl_merge(const struct ll_merge_driver\n> *drv_unused,\n> \t\txmp.style = git_xmerge_style;\n> \tif (marker_size > 0)\n> \t\txmp.marker_size = marker_size;\n> +\txmp.crlf = eol_for_path(NULL, src1->ptr, src1->size) == EOL_CRLF;\n> \txmp.ancestor = orig_name;\n> \txmp.file1 = name1;\n> \txmp.file2 = name2;\n\nWhat I do not understand is that you already had xmp.crlf even the\nlog message claimed that the caller is supposed to feed LF blobs and\nthe ll_merge() shouldn't have to worry about this.\n\nIf the user sets the repository up to use CRLF in working tree and\nLF in blob (e.g. crlf = input), shouldn't cmd_merge_file() be doing\nthe convert_to_git() to the buffer stored in mmfs[] before calling\ndown to xdl_merge() so that ll_merge() layer will see LF not CRLF?\n\nAnd if the interface is truly done in such a way that was outlined\nin the proposed log message, I do not think xmp.crlf is a good name\nfor the new field. What the updated code needs is a boolean option,\nxmp.end_marker_with_crlf, that is set only when the internal blob\nencoding is CRLF and affects only the ending of the marker strings.\n\nIt looks to me that the code is doing something different from what\nthe proposed log message claimed, which is puzzling.\n"},{"id":"276570","messageId":"20160122195015.GA5897@flurp.local","threadId":"41241","inReplyTo":"c0c775ea7a9ba3244748b784241de685cefc73b1.1453482052.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH 2/2] merge-file: consider core.crlf when writing merge markers","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-01-22T19:50:15Z","receivedAt":"2016-01-22T19:50:15Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Jan 22, 2016 at 06:01:25PM +0100, Johannes Schindelin wrote:\n> When merging files in repos with core.eol = crlf, git merge-file inserts\n> just a LF at the end of the merge markers. Files with mixed line endings\n> cause trouble in Windows editors and e.g. contrib/git-jump, where an\n> unmerged file in a run of \"git jump merge\" is reported as simply \"binary\n> file matches\".\n> [...]\n> Signed-off-by: Beat Bolli \n> Signed-off-by: Jeff King \n> Signed-off-by: Johannes Schindelin \n> ---\n> diff --git a/t/t6023-merge-file.sh b/t/t6023-merge-file.sh\n> @@ -346,4 +346,18 @@ test_expect_success 'conflict at EOF without LF resolved by --union' \\\n> +test_expect_success 'conflict markers contain CRLF when core.eol=crlf' '\n> +\ttest_must_fail git -c core.eol=crlf merge-file -p \\\n> +\t\tnolf-diff1.txt nolf-orig.txt nolf-diff2.txt >output.txt &&\n> +\ttest $(sed -n \"/\\.txt\\r$/p\" output.txt | wc -l) = 3\n\nThe \"\\r\" isn't recognized by 'sed' on Mac OS X or BSD. Perhaps use\ninstead:\n\n test $(cat output.txt | tr \"\\015\" Q | sed -n \"/\\.txtQ$/p\" | wc -l) = 3\n\nwhich works on Linux, Mac OS X, and BSD (in my tests).\n\nOr, implement a cr_to_q() to complement the existing q_to_cr() in\nt/test-lib-functions.sh.\n\n> +'\n> +\n> +test_expect_success 'conflict markers heed gitattributes over core.eol=crlf' '\n> +\tgit config core.eol crlf &&\n> +\techo \"*.txt eol=lf\" >>.gitattributes &&\n> +\ttest_must_fail git -c core.eol=crlf merge-file -p \\\n> +\t\tnolf-diff1.txt nolf-orig.txt nolf-diff2.txt >output.txt &&\n> +\ttest $(sed -n \"/\\.txt\\r$/p\" output.txt | wc -l) = 0\n\nDitto.\n\n> +'\n"},{"id":"276571","messageId":"CAPig+cSf_Hac=4+F1bRaFeYdFrjo6SH=vApdUDkhep5BUve+Kw@mail.gmail.com","threadId":"41241","inReplyTo":"20160122195015.GA5897@flurp.local","subject":"Re: [PATCH 2/2] merge-file: consider core.crlf when writing merge markers","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-01-22T19:52:41Z","receivedAt":"2016-01-22T19:52:41Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Jan 22, 2016 at 2:50 PM, Eric Sunshine wrote:\n> On Fri, Jan 22, 2016 at 06:01:25PM +0100, Johannes Schindelin wrote:\n>> +test_expect_success 'conflict markers contain CRLF when core.eol=crlf' '\n>> + test_must_fail git -c core.eol=crlf merge-file -p \\\n>> + nolf-diff1.txt nolf-orig.txt nolf-diff2.txt >output.txt &&\n>> + test $(sed -n \"/\\.txt\\r$/p\" output.txt | wc -l) = 3\n>\n> The \"\\r\" isn't recognized by 'sed' on Mac OS X or BSD. Perhaps use\n> instead:\n>\n> test $(cat output.txt | tr \"\\015\" Q | sed -n \"/\\.txtQ$/p\" | wc -l) = 3\n\nOr the 'sed' could even become at 'grep' at this point.\n"},{"id":"276606","messageId":"56A319B9.50900@web.de","threadId":"41241","inReplyTo":"c4e6707b53d82cdeeb16c83bddf9c4870a53d9df.1453482052.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH 1/2] convert: add a helper to determine the correct EOL for a given path","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2016-01-23T06:12:09Z","receivedAt":"2016-01-23T06:12:09Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 2016-01-22 18.01, Johannes Schindelin wrote:\n> Signed-off-by: Johannes Schindelin \n> ---\n> convert.c | 29 +++++++++++++++++++++++++++++\n> convert.h | 2 ++\n> 2 files changed, 31 insertions(+)\n> \n> diff --git a/convert.c b/convert.c\n> index 814e814..b458734 100644\n> --- a/convert.c\n> +++ b/convert.c\n> @@ -758,6 +758,35 @@ static void convert_attrs(struct conv_attrs *ca, const char *path)\n> \t}\n> }\n> \n> +enum eol eol_for_path(const char *path, const char *src, size_t len)\n> +{\n> +\tstruct conv_attrs ca;\n> +\tstruct text_stat stats;\n> +\n> +\tif (!path) {\nI don't think this is ideal.\nWhen path is NULL, can we set it to \"*\" to catch .gitattributes like\n\"* text=auto\"\n?\n> +\t\tmemset(&ca, 0, sizeof(ca));\n> +\t\tca.crlf_action = CRLF_AUTO;\n> +\t\tca.eol_attr = EOL_UNSET;\n> +\t} else {\n> +\t\tconvert_attrs(&ca, path);\n> +\t\tif (ca.eol_attr == EOL_UNSET)\n> +\t\t\tca.eol_attr = output_eol(ca.crlf_action);\n> +\t\tif (ca.eol_attr != EOL_UNSET)\n> +\t\t\treturn ca.eol_attr;\nThis doesn't seem to be correct.\nWhen \"File -text\" is set, no CRLF needs to be added.\n> +\t}\n> +\tif (!len || (ca.crlf_action != CRLF_AUTO &&\n> +\t\t\t\tca.crlf_action != CRLF_GUESS))\nCurrently core.autocrlf = true overrules \"core.eol\".\n\nSee the line in t0027:\n# core.eol core.autocrlf\ncheckout_files lf true \"\" CRLF CRLF CRLF_mix_LF LF_mix_CR\nLF_nul\n\nLetting core.autocrlf overrule core.eol is not documented very well,\nI have it on my todo-list. But I think it makes sense to keep the behavour.\n\nI'm currently working on simplification of convert.c, so a funtion like this\nis really a step forward.\nDoes it makes sense to add a comment, that some corner cases may\nneed additional tweaking ?\n"},{"id":"276607","messageId":"56A31E56.9040700@web.de","threadId":"41241","inReplyTo":"c0c775ea7a9ba3244748b784241de685cefc73b1.1453482052.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH 2/2] merge-file: consider core.crlf when writing merge markers","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2016-01-23T06:31:50Z","receivedAt":"2016-01-23T06:31:50Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 2016-01-22 18.01, Johannes Schindelin wrote:\n> From: Beat Bolli \n> \n> When merging files in repos with core.eol = crlf, git merge-file inserts\n> just a LF at the end of the merge markers. Files with mixed line endings\n> cause trouble in Windows editors and e.g. contrib/git-jump, where an\n> unmerged file in a run of \"git jump merge\" is reported as simply \"binary\n> file matches\".\n> \n> Fixing this improves merge-file's behavior under Windows.\n> \n> The original version of this patch also modified ll_merge(), but that\n> was incorrect: low-level merge operates on blobs, not on working files.\n> Therefore, the data passed to the low-level merge, as well as its\n> result, is expected to have LF-only line endings.\n> \n> It is the duty of ll_merge()'s *caller* (in case of Git's `merge`\n> command, the merge_content() function) to convert the merge result into\n> the correct working file contents, and ll_merge() should not muck with\n> line endings at all.\n> \n> Signed-off-by: Beat Bolli \n> Signed-off-by: Jeff King \n> Signed-off-by: Johannes Schindelin \n> ---\n> builtin/merge-file.c | 1 +\n> t/t6023-merge-file.sh | 14 ++++++++++++++\n> xdiff/xdiff.h | 1 +\n> xdiff/xmerge.c | 29 +++++++++++++++++++----------\n> 4 files changed, 35 insertions(+), 10 deletions(-)\n> \n> diff --git a/builtin/merge-file.c b/builtin/merge-file.c\n> index 5544705..9ce830a 100644\n> --- a/builtin/merge-file.c\n> +++ b/builtin/merge-file.c\n> @@ -81,6 +81,7 @@ int cmd_merge_file(int argc, const char **argv, const char *prefix)\n> \t\t\t\t\targv[i]);\n> \t}\n> \n> +\txmp.crlf = eol_for_path(names[0], NULL, 0) == EOL_CRLF;\n2 comments:\nWhat happens when the original file already has CRLF ?\nAnd why don't we feed any content into the function ?\nWhen the source is already in CRLF, do we need to add any CR ?\n\nOr is the content feed into the merge machine always normalized ?\nAnd in this case you can skip this comment completely.\n> \txmp.ancestor = names[1];\n> \txmp.file1 = names[0];\n> \txmp.file2 = names[2];\n> diff --git a/t/t6023-merge-file.sh b/t/t6023-merge-file.sh\n> index 190ee90..a131749 100755\n> --- a/t/t6023-merge-file.sh\n> +++ b/t/t6023-merge-file.sh\n> @@ -346,4 +346,18 @@ test_expect_success 'conflict at EOF without LF resolved by --union' \\\n> \t printf \"line1\\nline2\\nline3x\\nline3y\" >expect.txt &&\n> \t test_cmp expect.txt output.txt'\n> \n> +test_expect_success 'conflict markers contain CRLF when core.eol=crlf' '\n> +\ttest_must_fail git -c core.eol=crlf merge-file -p \\\n> +\t\tnolf-diff1.txt nolf-orig.txt nolf-diff2.txt >output.txt &&\n> +\ttest $(sed -n \"/\\.txt\\r$/p\" output.txt | wc -l) = 3\n$(wc -l) == 3 tends to be non-portable - either use \"test .. -eq\" or\nuse test_line_count()\n> +'\n\n> +\n> +test_expect_success 'conflict markers heed gitattributes over core.eol=crlf' '\n> +\tgit config core.eol crlf &&\n> +\techo \"*.txt eol=lf\" >>.gitattributes &&\n> +\ttest_must_fail git -c core.eol=crlf merge-file -p \\\n> +\t\tnolf-diff1.txt nolf-orig.txt nolf-diff2.txt >output.txt &&\n> +\ttest $(sed -n \"/\\.txt\\r$/p\" output.txt | wc -l) = 0\nSame here\nAnd If I remember that \"\\r\" in sed is non-portable, Ramsay suggested a nice fix:\ngmane/283262\n\"t6023 broken under Mac OS\"\n"},{"id":"276608","messageId":"56A32638.9060802@web.de","threadId":"41241","inReplyTo":"alpine.DEB.2.20.1601222003360.2964@virtualbox","subject":"Re: [PATCH 1/2] convert: add a helper to determine the correct EOL for a given path","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2016-01-23T07:05:28Z","receivedAt":"2016-01-23T07:05:28Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 2016-01-22 20.04, Johannes Schindelin wrote:\n> Hi Junio,\n> \n> On Fri, 22 Jan 2016, Junio C Hamano wrote:\n> \n>> Johannes Schindelin writes:\n>>\n>>> Signed-off-by: Johannes Schindelin \n>>> ---\n>>\n>> This change somehow ringed a bell and reminded me of your recent\n>> ls-files stuff. Are there things that these topics can use from\n>> each other?\n> \n> Quite possibly. I'll have a look tomorrow.\n\nSomething like this should do:\n(Fully untested)\n\nenum eol eol_for_path(const char *path, const char *src, size_t len)\n{\n struct conv_attrs ca;\n if (!path)\n path = \"*\"; /* catch * text=xxx */\n convert_attrs(&ca, path);\n ca.crlf_action = input_crlf_action(ca.crlf_action, ca.eol_attr);\n\n return output_eol(crlf_action);\n}\n"},{"id":"276618","messageId":"alpine.DEB.2.20.1601241137260.2964@virtualbox","threadId":"41241","inReplyTo":"CAPig+cSf_Hac=4+F1bRaFeYdFrjo6SH=vApdUDkhep5BUve+Kw@mail.gmail.com","subject":"Re: [PATCH 2/2] merge-file: consider core.crlf when writing merge markers","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-01-24T10:37:51Z","receivedAt":"2016-01-24T10:37:51Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Eric,\n\nOn Fri, 22 Jan 2016, Eric Sunshine wrote:\n\n> On Fri, Jan 22, 2016 at 2:50 PM, Eric Sunshine wrote:\n> > On Fri, Jan 22, 2016 at 06:01:25PM +0100, Johannes Schindelin wrote:\n> >> +test_expect_success 'conflict markers contain CRLF when core.eol=crlf' '\n> >> + test_must_fail git -c core.eol=crlf merge-file -p \\\n> >> + nolf-diff1.txt nolf-orig.txt nolf-diff2.txt >output.txt &&\n> >> + test $(sed -n \"/\\.txt\\r$/p\" output.txt | wc -l) = 3\n> >\n> > The \"\\r\" isn't recognized by 'sed' on Mac OS X or BSD. Perhaps use\n> > instead:\n> >\n> > test $(cat output.txt | tr \"\\015\" Q | sed -n \"/\\.txtQ$/p\" | wc -l) = 3\n> \n> Or the 'sed' could even become at 'grep' at this point.\n\nGood point, thanks!\n\nCiao,\nDscho\n"},{"id":"276619","messageId":"alpine.DEB.2.20.1601241138460.2964@virtualbox","threadId":"41241","inReplyTo":"56A31E56.9040700@web.de","subject":"Re: [PATCH 2/2] merge-file: consider core.crlf when writing merge markers","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-01-24T10:39:58Z","receivedAt":"2016-01-24T10:39:58Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Torsten,\n\nOn Sat, 23 Jan 2016, Torsten Bögershausen wrote:\n\n> On 2016-01-22 18.01, Johannes Schindelin wrote:\n> > From: Beat Bolli \n> > \n> > When merging files in repos with core.eol = crlf, git merge-file\n> > inserts just a LF at the end of the merge markers. Files with mixed\n> > line endings cause trouble in Windows editors and e.g.\n> > contrib/git-jump, where an unmerged file in a run of \"git jump merge\"\n> > is reported as simply \"binary file matches\".\n> > \n> > [...]\n>\n> What happens when the original file already has CRLF ?\n> And why don't we feed any content into the function ?\n\nWhy, indeed. I completely reworked the entire approach and will send out\nthe result in a moment.\n\nCiao,\nDscho"},{"id":"276620","messageId":"alpine.DEB.2.20.1601241140190.2964@virtualbox","threadId":"41241","inReplyTo":"56A319B9.50900@web.de","subject":"Re: [PATCH 1/2] convert: add a helper to determine the correct EOL for a given path","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-01-24T10:41:19Z","receivedAt":"2016-01-24T10:41:19Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Torsten,\n\nOn Sat, 23 Jan 2016, Torsten Bögershausen wrote:\n\n> When path is NULL, can we set it to \"*\" to catch .gitattributes like\n> \"* text=auto\"\n> ?\n\nThe point of that part of the patch was to make things *independent* of\nthe gitattributes because we operate not on working files, but on blob\ncontents.\n\nIn any case, this point will be moot with the upcoming iteration of the\npatch.\n\nCiao,\nDscho"},{"id":"276621","messageId":"alpine.DEB.2.20.1601241141360.2964@virtualbox","threadId":"41241","inReplyTo":"alpine.DEB.2.20.1601222003360.2964@virtualbox","subject":"Re: [PATCH 1/2] convert: add a helper to determine the correct EOL for a given path","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-01-24T10:42:10Z","receivedAt":"2016-01-24T10:42:10Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Fri, 22 Jan 2016, Johannes Schindelin wrote:\n\n> On Fri, 22 Jan 2016, Junio C Hamano wrote:\n> \n> > Johannes Schindelin writes:\n> > \n> > > Signed-off-by: Johannes Schindelin \n> > > ---\n> > \n> > This change somehow ringed a bell and reminded me of your recent\n> > ls-files stuff. Are there things that these topics can use from\n> > each other?\n> \n> Quite possibly. I'll have a look tomorrow.\n\nI did, but could not quite finish reworking the patch until today. Will\nsend out the result in a moment.\n\nCiao,\nDscho\n"},{"id":"276622","messageId":"alpine.DEB.2.20.1601241143090.2964@virtualbox","threadId":"41241","inReplyTo":"xmqqa8nxv3do.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 2/2] merge-file: consider core.crlf when writing merge markers","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-01-24T10:44:34Z","receivedAt":"2016-01-24T10:44:34Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Fri, 22 Jan 2016, Junio C Hamano wrote:\n\n> Johannes Schindelin writes:\n> \n> >> > It is the duty of ll_merge()'s *caller* (in case of Git's `merge`\n> >> > command, the merge_content() function) to convert the merge result\n> >> > into the correct working file contents, and ll_merge() should not muck\n> >> > with line endings at all.\n> >> \n> >> Hmph, aren't there people who use CRLF throughout their set-up (that is,\n> >> it is normal for both of their blobs and working tree files to use CRLF\n> >> line endings)? Or is such a setting illegal and unsupported?\n> >\n> > Good point.\n> >\n> > While I would love to simply not support such a case, it would be turning\n> > a blind eye to reality.\n> >\n> > So I guess I need another patch like this (plus a test case):\n> >\n> > -- snipsnap --\n> > t a/ll-merge.c b/ll-merge.c\n> > index 0338630..4a565c8 100644\n> > --- a/ll-merge.c\n> > +++ b/ll-merge.c\n> > @@ -111,6 +111,7 @@ static int ll_xdl_merge(const struct ll_merge_driver\n> > *drv_unused,\n> > \t\txmp.style = git_xmerge_style;\n> > \tif (marker_size > 0)\n> > \t\txmp.marker_size = marker_size;\n> > +\txmp.crlf = eol_for_path(NULL, src1->ptr, src1->size) == EOL_CRLF;\n> > \txmp.ancestor = orig_name;\n> > \txmp.file1 = name1;\n> > \txmp.file2 = name2;\n> \n> What I do not understand is that you already had xmp.crlf even the\n> log message claimed that the caller is supposed to feed LF blobs and\n> the ll_merge() shouldn't have to worry about this.\n\nWell, I just heeded your point about blobs not necessarily having LF-only\nend-of-lines ;-)\n\nIn any case, the upcoming iteration works quite a bit differently, and I\nwant to add, robustly.\n\nCiao,\nDscho\n"},{"id":"276623","messageId":"cover.1453632296.git.johannes.schindelin@gmx.de","threadId":"41241","inReplyTo":"cover.1453482052.git.johannes.schindelin@gmx.de","subject":"[PATCH v2 0/1] Let merge-file write out conflict markers with correct EOLs","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-01-24T10:48:27Z","receivedAt":"2016-01-24T10:48:27Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"The original patch was sent by Beat Bolli in\nhttp://thread.gmane.org/gmane.comp.version-control.git/281600\n\nMy suggestion to extend it to respect gitattributes led to\nchanges that broke the original patch. And they were misguided\nto begin with (see below).\n\nSince there have been a couple of \"What's cooking\" mails\ncontaining unheard calls for updates on this patch, I took it\non myself to fix things.\n\nJunio's comment that we might look at blobs containing CR/LF\nline endings made me rethink the entire approach, and I am now\nconvinced that we need to abandon the entire idea to make the\nconflict markers depend on config settings or attributes:\nafter all, I introduced `git merge-file` as a replacement for\nGNU merge that can be used *outside* of any repository, by design.\n\nThe crucial idea hit me yesterday when I took a step back: all\nwe need to do is to ensure that the end-of-line style is matched\nwhen *all* input files are LF-only, or when they are all CR/LF.\nIn all other cases, we have mixed line endings anyway.\n\nAnd to do that, it is sufficient to look at *one single line\nending* in the context. Technically, it does not even have to be\nthe context, but the first line endings of the first file would\nbe enough, however it is so much more pleasant if the conflict\nmarker's eol matches the one of the preceding line.\n\nTo prevent my future self from repeating mistakes, I added a\nlittle bit of background to the commit message.\n\nSince this is a complete rewrite, the interdiff is pretty useless\nand only included for the record.\n\n\nJohannes Schindelin (1):\n merge-file: let conflict markers match end-of-line style of the\n context\n\n t/t6023-merge-file.sh | 12 +++++++++++\n xdiff/xmerge.c | 57 +++++++++++++++++++++++++++++++++++++++++++++++----\n 2 files changed, 65 insertions(+), 4 deletions(-)\n\nInterdiff vs v1:\n\n diff --git a/builtin/merge-file.c b/builtin/merge-file.c\n index 9ce830a..5544705 100644\n --- a/builtin/merge-file.c\n +++ b/builtin/merge-file.c\n @@ -81,7 +81,6 @@ int cmd_merge_file(int argc, const char **argv, const char *prefix)\n \t\t\t\t\targv[i]);\n \t}\n \n -\txmp.crlf = eol_for_path(names[0], NULL, 0) == EOL_CRLF;\n \txmp.ancestor = names[1];\n \txmp.file1 = names[0];\n \txmp.file2 = names[2];\n diff --git a/convert.c b/convert.c\n index b458734..814e814 100644\n --- a/convert.c\n +++ b/convert.c\n @@ -758,35 +758,6 @@ static void convert_attrs(struct conv_attrs *ca, const char *path)\n \t}\n }\n \n -enum eol eol_for_path(const char *path, const char *src, size_t len)\n -{\n -\tstruct conv_attrs ca;\n -\tstruct text_stat stats;\n -\n -\tif (!path) {\n -\t\tmemset(&ca, 0, sizeof(ca));\n -\t\tca.crlf_action = CRLF_AUTO;\n -\t\tca.eol_attr = EOL_UNSET;\n -\t} else {\n -\t\tconvert_attrs(&ca, path);\n -\t\tif (ca.eol_attr == EOL_UNSET)\n -\t\t\tca.eol_attr = output_eol(ca.crlf_action);\n -\t\tif (ca.eol_attr != EOL_UNSET)\n -\t\t\treturn ca.eol_attr;\n -\t}\n -\tif (!len || (ca.crlf_action != CRLF_AUTO &&\n -\t\t\t\tca.crlf_action != CRLF_GUESS))\n -\t\treturn core_eol;\n -\tca.crlf_action = input_crlf_action(ca.crlf_action, ca.eol_attr);\n -\tgather_stats(src, len, &stats);\n -\tif (ca.crlf_action == CRLF_GUESS && stats.cr > stats.crlf)\n -\t\treturn core_eol;\n -\telse if (stats.crlf)\n -\t\treturn EOL_CRLF;\n -\telse\n -\t\treturn EOL_LF;\n -}\n -\n int would_convert_to_git_filter_fd(const char *path)\n {\n \tstruct conv_attrs ca;\n diff --git a/convert.h b/convert.h\n index 1892867..d9d853c 100644\n --- a/convert.h\n +++ b/convert.h\n @@ -33,8 +33,6 @@ enum eol {\n \n extern enum eol core_eol;\n \n -extern enum eol eol_for_path(const char *path, const char *src, size_t len);\n -\n /* returns 1 if *dst was used */\n extern int convert_to_git(const char *path, const char *src, size_t len,\n \t\t\t struct strbuf *dst, enum safe_crlf checksafe);\n diff --git a/t/t6023-merge-file.sh b/t/t6023-merge-file.sh\n index a131749..f0cb9ce 100755\n --- a/t/t6023-merge-file.sh\n +++ b/t/t6023-merge-file.sh\n @@ -346,18 +346,16 @@ test_expect_success 'conflict at EOF without LF resolved by --union' \\\n \t printf \"line1\\nline2\\nline3x\\nline3y\" >expect.txt &&\n \t test_cmp expect.txt output.txt'\n \n -test_expect_success 'conflict markers contain CRLF when core.eol=crlf' '\n +test_expect_success 'conflict markers match existing line endings' '\n +\tappend_cr crlf-orig.txt &&\n +\tappend_cr crlf-diff1.txt &&\n +\tappend_cr crlf-diff2.txt &&\n \ttest_must_fail git -c core.eol=crlf merge-file -p \\\n -\t\tnolf-diff1.txt nolf-orig.txt nolf-diff2.txt >output.txt &&\n -\ttest $(sed -n \"/\\.txt\\r$/p\" output.txt | wc -l) = 3\n -'\n -\n -test_expect_success 'conflict markers heed gitattributes over core.eol=crlf' '\n -\tgit config core.eol crlf &&\n -\techo \"*.txt eol=lf\" >>.gitattributes &&\n +\t\tcrlf-diff1.txt crlf-orig.txt crlf-diff2.txt >crlf.txt &&\n +\ttest $(tr \"\\015\" Q output.txt &&\n -\ttest $(sed -n \"/\\.txt\\r$/p\" output.txt | wc -l) = 0\n +\t\tnolf-diff1.txt nolf-orig.txt nolf-diff2.txt >nolf.txt &&\n +\ttest $(tr \"\\015\" Q nrec - 1)\n +\t\t/* All lines before the last *must* end in LF */\n +\t\treturn (size = file->recs[i]->size) > 1 &&\n +\t\t\tfile->recs[i]->ptr[size - 2] == '\\r';\n +\tif (!file->nrec)\n +\t\t/* Cannot determine eol style from empty file */\n +\t\treturn -1;\n +\tif ((size = file->recs[i]->size) &&\n +\t\t\tfile->recs[i]->ptr[size - 1] == '\\n')\n +\t\t/* Last line; ends in LF; Is it CR/LF? */\n +\t\treturn size > 1 &&\n +\t\t\tfile->recs[i]->ptr[size - 2] == '\\r';\n +\tif (!i)\n +\t\t/* The only line has no eol */\n +\t\treturn -1;\n +\t/* Determine eol from second-to-last line */\n +\treturn (size = file->recs[i - 1]->size) > 1 &&\n +\t\tfile->recs[i - 1]->ptr[size - 2] == '\\r';\n +}\n +\n static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,\n \t\t\t xdfenv_t *xe2, const char *name2,\n \t\t\t const char *name3,\n -\t\t\t int size, int i, int style, int crlf,\n +\t\t\t int size, int i, int style,\n \t\t\t xdmerge_t *m, char *dest, int marker_size)\n {\n \tint marker1_size = (name1 ? strlen(name1) + 1 : 0);\n \tint marker2_size = (name2 ? strlen(name2) + 1 : 0);\n \tint marker3_size = (name3 ? strlen(name3) + 1 : 0);\n +\tint needs_cr;\n +\n +\t/* Match post-images' preceding, or first, lines' end-of-line style */\n +\tneeds_cr = is_eol_crlf(&xe1->xdf2, m->i1 ? m->i1 - 1 : 0);\n +\tif (needs_cr)\n +\t\tneeds_cr = is_eol_crlf(&xe2->xdf2, m->i2 ? m->i2 - 1 : 0);\n +\t/* Look at pre-image's first line, unless we already settled on LF */\n +\tif (needs_cr)\n +\t\tneeds_cr = is_eol_crlf(&xe1->xdf1, 0);\n +\t/* If still undecided, use LF-only */\n +\tif (needs_cr < 0)\n +\t\tneeds_cr = 0;\n \n \tif (marker_size <= 0)\n \t\tmarker_size = DEFAULT_CONFLICT_MARKER_SIZE;\n @@ -161,7 +202,7 @@ static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,\n \t\t\t dest ? dest + size : NULL);\n \n \tif (!dest) {\n -\t\tsize += marker_size + 1 + crlf + marker1_size;\n +\t\tsize += marker_size + 1 + needs_cr + marker1_size;\n \t} else {\n \t\tmemset(dest + size, '<', marker_size);\n \t\tsize += marker_size;\n @@ -170,7 +211,7 @@ static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,\n \t\t\tmemcpy(dest + size + 1, name1, marker1_size - 1);\n \t\t\tsize += marker1_size;\n \t\t}\n -\t\tif (crlf)\n +\t\tif (needs_cr)\n \t\t\tdest[size++] = '\\r';\n \t\tdest[size++] = '\\n';\n \t}\n @@ -182,7 +223,7 @@ static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,\n \tif (style == XDL_MERGE_DIFF3) {\n \t\t/* Shared preimage */\n \t\tif (!dest) {\n -\t\t\tsize += marker_size + 1 + crlf + marker3_size;\n +\t\t\tsize += marker_size + 1 + needs_cr + marker3_size;\n \t\t} else {\n \t\t\tmemset(dest + size, '|', marker_size);\n \t\t\tsize += marker_size;\n @@ -191,7 +232,7 @@ static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,\n \t\t\t\tmemcpy(dest + size + 1, name3, marker3_size - 1);\n \t\t\t\tsize += marker3_size;\n \t\t\t}\n -\t\t\tif (crlf)\n +\t\t\tif (needs_cr)\n \t\t\t\tdest[size++] = '\\r';\n \t\t\tdest[size++] = '\\n';\n \t\t}\n @@ -200,11 +241,11 @@ static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,\n \t}\n \n \tif (!dest) {\n -\t\tsize += marker_size + 1 + crlf;\n +\t\tsize += marker_size + 1 + needs_cr;\n \t} else {\n \t\tmemset(dest + size, '=', marker_size);\n \t\tsize += marker_size;\n -\t\tif (crlf)\n +\t\tif (needs_cr)\n \t\t\tdest[size++] = '\\r';\n \t\tdest[size++] = '\\n';\n \t}\n @@ -213,7 +254,7 @@ static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,\n \tsize += xdl_recs_copy(xe2, m->i2, m->chg2, 1,\n \t\t\t dest ? dest + size : NULL);\n \tif (!dest) {\n -\t\tsize += marker_size + 1 + crlf + marker2_size;\n +\t\tsize += marker_size + 1 + needs_cr + marker2_size;\n \t} else {\n \t\tmemset(dest + size, '>', marker_size);\n \t\tsize += marker_size;\n @@ -222,7 +263,7 @@ static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,\n \t\t\tmemcpy(dest + size + 1, name2, marker2_size - 1);\n \t\t\tsize += marker2_size;\n \t\t}\n -\t\tif (crlf)\n +\t\tif (needs_cr)\n \t\t\tdest[size++] = '\\r';\n \t\tdest[size++] = '\\n';\n \t}\n @@ -234,7 +275,7 @@ static int xdl_fill_merge_buffer(xdfenv_t *xe1, const char *name1,\n \t\t\t\t const char *ancestor_name,\n \t\t\t\t int favor,\n \t\t\t\t xdmerge_t *m, char *dest, int style,\n -\t\t\t\t int crlf, int marker_size)\n +\t\t\t\t int marker_size)\n {\n \tint size, i;\n \n @@ -245,8 +286,8 @@ static int xdl_fill_merge_buffer(xdfenv_t *xe1, const char *name1,\n \t\tif (m->mode == 0)\n \t\t\tsize = fill_conflict_hunk(xe1, name1, xe2, name2,\n \t\t\t\t\t\t ancestor_name,\n -\t\t\t\t\t\t size, i, style, crlf, m,\n -\t\t\t\t\t\t dest, marker_size);\n +\t\t\t\t\t\t size, i, style, m, dest,\n +\t\t\t\t\t\t marker_size);\n \t\telse if (m->mode & 3) {\n \t\t\t/* Before conflicting part */\n \t\t\tsize += xdl_recs_copy(xe1, i, m->i1 - i, 0,\n @@ -427,7 +468,6 @@ static int xdl_do_merge(xdfenv_t *xe1, xdchange_t *xscr1,\n \tint level = xmp->level;\n \tint style = xmp->style;\n \tint favor = xmp->favor;\n -\tint crlf = xmp->crlf;\n \n \tif (style == XDL_MERGE_DIFF3) {\n \t\t/*\n @@ -563,7 +603,7 @@ static int xdl_do_merge(xdfenv_t *xe1, xdchange_t *xscr1,\n \t\tint size = xdl_fill_merge_buffer(xe1, name1, xe2, name2,\n \t\t\t\t\t\t ancestor_name,\n \t\t\t\t\t\t favor, changes, NULL, style,\n -\t\t\t\t\t\t crlf, marker_size);\n +\t\t\t\t\t\t marker_size);\n \t\tresult->ptr = xdl_malloc(size);\n \t\tif (!result->ptr) {\n \t\t\txdl_cleanup_merge(changes);\n @@ -572,7 +612,7 @@ static int xdl_do_merge(xdfenv_t *xe1, xdchange_t *xscr1,\n \t\tresult->size = size;\n \t\txdl_fill_merge_buffer(xe1, name1, xe2, name2,\n \t\t\t\t ancestor_name, favor, changes,\n -\t\t\t\t result->ptr, style, crlf, marker_size);\n +\t\t\t\t result->ptr, style, marker_size);\n \t}\n \treturn xdl_cleanup_merge(changes);\n }\n\n-- \n2.7.0.windows.1.7.g55a05c8\n"},{"id":"276624","messageId":"d71c7abddd4cba85b967f9fe1d33c7c843176ca2.1453632296.git.johannes.schindelin@gmx.de","threadId":"41241","inReplyTo":"cover.1453632296.git.johannes.schindelin@gmx.de","subject":"[PATCH v2 1/1] merge-file: let conflict markers match end-of-line style of the context","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-01-24T10:48:46Z","receivedAt":"2016-01-24T10:48:46Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"When merging files with CR/LF line endings, the conflict markers should\nmatch those, lest the output file has mixed line endings.\n\nThis is particularly of interest on Windows, where some editors get\n*really* confused by mixed line endings.\n\nThe original version of this patch by Beat Bolli respected core.eol, and\na subsequent improvement by this developer also respected gitattributes.\nThis approach was suboptimal, though: `git merge-file` was invented as a\ndrop-in replacement for GNU merge and as such has no problem operating\noutside of any repository at all!\n\nAnother problem with the original approach was pointed out by Junio\nHamano: legacy repositories might have their text files committed using\nCR/LF line endings (and core.eol and the gitattributes would give us a\nfalse impression there). Therefore, the much superior approach is to\nsimply match the context's line endings, if any.\n\nWe actually do not have to look at the *entire* context at all: if the\nfiles are all LF-only, or if they all have CR/LF line endings, it is\nsufficient to look at just a *single* line to match that style. And if\nthe line endings are mixed anyway, it is *still* okay to imitate just a\nsingle line's eol: we will just add to the pile of mixed line endings,\nand there is nothing we can do about that.\n\nSo what we do is: we look at the line preceding the conflict, falling\nback to the line preceding that in case it was the last line and had no\nline ending, falling back to the first line, first in the first\npost-image, then the second post-image, and finally the pre-image.\nIf we find consistent CR/LF (or undecided) end-of-line style, we match\nthat, otherwise we use LF-only line endings for the conflict markers.\n\nNote that while it is true that there have to be at least two lines we\ncan look at (otherwise there would be no conflict), the same is not true\nfor line *endings*: the three files in question could all consist of a\nsingle line without any line ending, each. In this case we fall back to\nusing LF-only.\n\nSigned-off-by: Johannes Schindelin \n---\n t/t6023-merge-file.sh | 12 +++++++++++\n xdiff/xmerge.c | 57 +++++++++++++++++++++++++++++++++++++++++++++++----\n 2 files changed, 65 insertions(+), 4 deletions(-)\n\ndiff --git a/t/t6023-merge-file.sh b/t/t6023-merge-file.sh\nindex 190ee90..f0cb9ce 100755\n--- a/t/t6023-merge-file.sh\n+++ b/t/t6023-merge-file.sh\n@@ -346,4 +346,16 @@ test_expect_success 'conflict at EOF without LF resolved by --union' \\\n \t printf \"line1\\nline2\\nline3x\\nline3y\" >expect.txt &&\n \t test_cmp expect.txt output.txt'\n \n+test_expect_success 'conflict markers match existing line endings' '\n+\tappend_cr crlf-orig.txt &&\n+\tappend_cr crlf-diff1.txt &&\n+\tappend_cr crlf-diff2.txt &&\n+\ttest_must_fail git -c core.eol=crlf merge-file -p \\\n+\t\tcrlf-diff1.txt crlf-orig.txt crlf-diff2.txt >crlf.txt &&\n+\ttest $(tr \"\\015\" Q nolf.txt &&\n+\ttest $(tr \"\\015\" Q nrec - 1)\n+\t\t/* All lines before the last *must* end in LF */\n+\t\treturn (size = file->recs[i]->size) > 1 &&\n+\t\t\tfile->recs[i]->ptr[size - 2] == '\\r';\n+\tif (!file->nrec)\n+\t\t/* Cannot determine eol style from empty file */\n+\t\treturn -1;\n+\tif ((size = file->recs[i]->size) &&\n+\t\t\tfile->recs[i]->ptr[size - 1] == '\\n')\n+\t\t/* Last line; ends in LF; Is it CR/LF? */\n+\t\treturn size > 1 &&\n+\t\t\tfile->recs[i]->ptr[size - 2] == '\\r';\n+\tif (!i)\n+\t\t/* The only line has no eol */\n+\t\treturn -1;\n+\t/* Determine eol from second-to-last line */\n+\treturn (size = file->recs[i - 1]->size) > 1 &&\n+\t\tfile->recs[i - 1]->ptr[size - 2] == '\\r';\n+}\n+\n static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,\n \t\t\t xdfenv_t *xe2, const char *name2,\n \t\t\t const char *name3,\n@@ -152,6 +181,18 @@ static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,\n \tint marker1_size = (name1 ? strlen(name1) + 1 : 0);\n \tint marker2_size = (name2 ? strlen(name2) + 1 : 0);\n \tint marker3_size = (name3 ? strlen(name3) + 1 : 0);\n+\tint needs_cr;\n+\n+\t/* Match post-images' preceding, or first, lines' end-of-line style */\n+\tneeds_cr = is_eol_crlf(&xe1->xdf2, m->i1 ? m->i1 - 1 : 0);\n+\tif (needs_cr)\n+\t\tneeds_cr = is_eol_crlf(&xe2->xdf2, m->i2 ? m->i2 - 1 : 0);\n+\t/* Look at pre-image's first line, unless we already settled on LF */\n+\tif (needs_cr)\n+\t\tneeds_cr = is_eol_crlf(&xe1->xdf1, 0);\n+\t/* If still undecided, use LF-only */\n+\tif (needs_cr < 0)\n+\t\tneeds_cr = 0;\n \n \tif (marker_size <= 0)\n \t\tmarker_size = DEFAULT_CONFLICT_MARKER_SIZE;\n@@ -161,7 +202,7 @@ static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,\n \t\t\t dest ? dest + size : NULL);\n \n \tif (!dest) {\n-\t\tsize += marker_size + 1 + marker1_size;\n+\t\tsize += marker_size + 1 + needs_cr + marker1_size;\n \t} else {\n \t\tmemset(dest + size, '<', marker_size);\n \t\tsize += marker_size;\n@@ -170,6 +211,8 @@ static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,\n \t\t\tmemcpy(dest + size + 1, name1, marker1_size - 1);\n \t\t\tsize += marker1_size;\n \t\t}\n+\t\tif (needs_cr)\n+\t\t\tdest[size++] = '\\r';\n \t\tdest[size++] = '\\n';\n \t}\n \n@@ -180,7 +223,7 @@ static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,\n \tif (style == XDL_MERGE_DIFF3) {\n \t\t/* Shared preimage */\n \t\tif (!dest) {\n-\t\t\tsize += marker_size + 1 + marker3_size;\n+\t\t\tsize += marker_size + 1 + needs_cr + marker3_size;\n \t\t} else {\n \t\t\tmemset(dest + size, '|', marker_size);\n \t\t\tsize += marker_size;\n@@ -189,6 +232,8 @@ static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,\n \t\t\t\tmemcpy(dest + size + 1, name3, marker3_size - 1);\n \t\t\t\tsize += marker3_size;\n \t\t\t}\n+\t\t\tif (needs_cr)\n+\t\t\t\tdest[size++] = '\\r';\n \t\t\tdest[size++] = '\\n';\n \t\t}\n \t\tsize += xdl_orig_copy(xe1, m->i0, m->chg0, 1,\n@@ -196,10 +241,12 @@ static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,\n \t}\n \n \tif (!dest) {\n-\t\tsize += marker_size + 1;\n+\t\tsize += marker_size + 1 + needs_cr;\n \t} else {\n \t\tmemset(dest + size, '=', marker_size);\n \t\tsize += marker_size;\n+\t\tif (needs_cr)\n+\t\t\tdest[size++] = '\\r';\n \t\tdest[size++] = '\\n';\n \t}\n \n@@ -207,7 +254,7 @@ static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,\n \tsize += xdl_recs_copy(xe2, m->i2, m->chg2, 1,\n \t\t\t dest ? dest + size : NULL);\n \tif (!dest) {\n-\t\tsize += marker_size + 1 + marker2_size;\n+\t\tsize += marker_size + 1 + needs_cr + marker2_size;\n \t} else {\n \t\tmemset(dest + size, '>', marker_size);\n \t\tsize += marker_size;\n@@ -216,6 +263,8 @@ static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,\n \t\t\tmemcpy(dest + size + 1, name2, marker2_size - 1);\n \t\t\tsize += marker2_size;\n \t\t}\n+\t\tif (needs_cr)\n+\t\t\tdest[size++] = '\\r';\n \t\tdest[size++] = '\\n';\n \t}\n \treturn size;\n-- \n2.7.0.windows.1.7.g55a05c8\n"},{"id":"276665","messageId":"56A4FB64.4010609@web.de","threadId":"41241","inReplyTo":"d71c7abddd4cba85b967f9fe1d33c7c843176ca2.1453632296.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH v2 1/1] merge-file: let conflict markers match end-of-line style of the context","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2016-01-24T16:27:16Z","receivedAt":"2016-01-24T16:27:16Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 24.01.16 11:48, Johannes Schindelin wrote:\n(I had the same reasoning about the CRLF in the working tree:\nWe don't need to look at core.autocrlf/attributes, so Ack from me)\n\n> +test_expect_success 'conflict markers match existing line endings' '\n> +\tappend_cr crlf-orig.txt &&\n> +\tappend_cr crlf-diff1.txt &&\n> +\tappend_cr crlf-diff2.txt &&\n> +\ttest_must_fail git -c core.eol=crlf merge-file -p \\\n> +\t\tcrlf-diff1.txt crlf-orig.txt crlf-diff2.txt >crlf.txt &&\n> +\ttest $(tr \"\\015\" Q +\ttest_must_fail git -c core.eol=crlf merge-file -p \\\n> +\t\tnolf-diff1.txt nolf-orig.txt nolf-diff2.txt >nolf.txt &&\n> +\ttest $(tr \"\\015\" Q +'\n> +\n\nMinor remark:\n\nRamsay suggested a test that doesn't use grep or wc and looks like this:\n\ntest_expect_success 'conflict markers contain CRLF when core.eol=crlf' ' \n test_must_fail git -c core.eol=crlf merge-file -p \\ \n nolf-diff1.txt nolf-orig.txt nolf-diff2.txt >output.txt && \n tr \"\\015\" Q |].*Q$/p\" >out.txt && \n cat >expect.txt <<-\\EOF && \n <<<<<<< nolf-diff1.txtQ \n ||||||| nolf-orig.txtQ \n =======Q \n >>>>>>> nolf-diff2.txtQ \n EOF \n test_cmp expect.txt out.txt \n'\n"},{"id":"276666","messageId":"56A4FDA0.50906@web.de","threadId":"41241","inReplyTo":"56A4FB64.4010609@web.de","subject":"Re: [PATCH v2 1/1] merge-file: let conflict markers match end-of-line style of the context","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2016-01-24T16:36:48Z","receivedAt":"2016-01-24T16:36:48Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 24.01.16 17:27, Torsten Bögershausen wrote:\n> On 24.01.16 11:48, Johannes Schindelin wrote:\n> (I had the same reasoning about the CRLF in the working tree:\n> We don't need to look at core.autocrlf/attributes, so Ack from me)\n> \n>> +test_expect_success 'conflict markers match existing line endings' '\n>> +\tappend_cr crlf-orig.txt &&\n>> +\tappend_cr crlf-diff1.txt &&\n>> +\tappend_cr crlf-diff2.txt &&\n>> +\ttest_must_fail git -c core.eol=crlf merge-file -p \\\n>> +\t\tcrlf-diff1.txt crlf-orig.txt crlf-diff2.txt >crlf.txt &&\n>> +\ttest $(tr \"\\015\" Q > +\ttest_must_fail git -c core.eol=crlf merge-file -p \\\n>> +\t\tnolf-diff1.txt nolf-orig.txt nolf-diff2.txt >nolf.txt &&\n>> +\ttest $(tr \"\\015\" Q > +'\n>> +\n> \n> Minor remark:\n> \n> Ramsay suggested a test that doesn't use grep or wc and looks like this:\n(That looked really garbled in my mail pgm. The suggestion is here:) \n\n"},{"id":"276669","messageId":"CAPig+cT4gQi=OhYT3AwKA=_UER4nC-wOu93U72QcSWtMoVxhMg@mail.gmail.com","threadId":"41241","inReplyTo":"alpine.DEB.2.20.1601241137260.2964@virtualbox","subject":"Re: [PATCH 2/2] merge-file: consider core.crlf when writing merge markers","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-01-24T18:26:13Z","receivedAt":"2016-01-24T18:26:13Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Jan 24, 2016 at 5:37 AM, Johannes Schindelin\n wrote:\n> On Fri, 22 Jan 2016, Eric Sunshine wrote:\n>> On Fri, Jan 22, 2016 at 2:50 PM, Eric Sunshine wrote:\n>> > On Fri, Jan 22, 2016 at 06:01:25PM +0100, Johannes Schindelin wrote:\n>> >> +test_expect_success 'conflict markers contain CRLF when core.eol=crlf' '\n>> >> + test_must_fail git -c core.eol=crlf merge-file -p \\\n>> >> + nolf-diff1.txt nolf-orig.txt nolf-diff2.txt >output.txt &&\n>> >> + test $(sed -n \"/\\.txt\\r$/p\" output.txt | wc -l) = 3\n>> >\n>> > The \"\\r\" isn't recognized by 'sed' on Mac OS X or BSD. Perhaps use\n>> > instead:\n>> >\n>> > test $(cat output.txt | tr \"\\015\" Q | sed -n \"/\\.txtQ$/p\" | wc -l) = 3\n>>\n>> Or the 'sed' could even become at 'grep' at this point.\n>\n> Good point, thanks!\n\nAnd, without the unnecessary 'cat', of course (don't know what I was thinking):\n\n test $(tr \"\\015\" Q writes:\n\n> The crucial idea hit me yesterday when I took a step back: all\n> we need to do is to ensure that the end-of-line style is matched\n> when *all* input files are LF-only, or when they are all CR/LF.\n> In all other cases, we have mixed line endings anyway.\n>\n> And to do that, it is sufficient to look at *one single line\n> ending* in the context. Technically, it does not even have to be\n> the context, but the first line endings of the first file would\n> be enough, however it is so much more pleasant if the conflict\n> marker's eol matches the one of the preceding line.\n\nI like that approach. My understanding of the xdiff/xmerge code is\nvery rusty and I have some time to re-learn it before I can judge\nthat the change implements that approach correctly, though X-<.\n\nThanks.\n"},{"id":"276699","messageId":"alpine.DEB.2.20.1601250749580.2964@virtualbox","threadId":"41241","inReplyTo":"56A4FB64.4010609@web.de","subject":"Re: [PATCH v2 1/1] merge-file: let conflict markers match end-of-line style of the context","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-01-25T06:53:32Z","receivedAt":"2016-01-25T06:53:32Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Torsten,\n\nOn Sun, 24 Jan 2016, Torsten Bögershausen wrote:\n\n> On 24.01.16 11:48, Johannes Schindelin wrote:\n> (I had the same reasoning about the CRLF in the working tree:\n> We don't need to look at core.autocrlf/attributes, so Ack from me)\n> \n> > +test_expect_success 'conflict markers match existing line endings' '\n> > +\tappend_cr crlf-orig.txt &&\n> > +\tappend_cr crlf-diff1.txt &&\n> > +\tappend_cr crlf-diff2.txt &&\n> > +\ttest_must_fail git -c core.eol=crlf merge-file -p \\\n> > +\t\tcrlf-diff1.txt crlf-orig.txt crlf-diff2.txt >crlf.txt &&\n> > +\ttest $(tr \"\\015\" Q > +\ttest_must_fail git -c core.eol=crlf merge-file -p \\\n> > +\t\tnolf-diff1.txt nolf-orig.txt nolf-diff2.txt >nolf.txt &&\n> > +\ttest $(tr \"\\015\" Q > +'\n> > +\n> \n> Minor remark:\n> \n> Ramsay suggested a test that doesn't use grep or wc and looks like this:\n> \n> test_expect_success 'conflict markers contain CRLF when core.eol=crlf' '\n> test_must_fail git -c core.eol=crlf merge-file -p \\\n> nolf-diff1.txt nolf-orig.txt nolf-diff2.txt >output.txt &&\n> tr \"\\015\" Q |].*Q$/p\" >out.txt &&\n> cat >expect.txt <<-\\EOF &&\n> <<<<<<< nolf-diff1.txtQ\n> ||||||| nolf-orig.txtQ\n> =======Q\n> >>>>>>> nolf-diff2.txtQ\n> EOF\n> test_cmp expect.txt out.txt\n> '\n\nProbably he wrapped it at less than 192 columns per row, though ;-)\n\nSeriously again, this longer version might test more, but it definitely\nalso tests more than what I actually want to test: I am simply interested\nto verify that the conflict markers end in CR/LF when appropriate. Read: I\nam uncertain that I want to spend the additional lines on testing more\nthan actually necessary.\n\nCiao,\nDscho"},{"id":"276701","messageId":"alpine.DEB.2.20.1601250801050.2964@virtualbox","threadId":"41241","inReplyTo":"CAPig+cT4gQi=OhYT3AwKA=_UER4nC-wOu93U72QcSWtMoVxhMg@mail.gmail.com","subject":"Re: [PATCH 2/2] merge-file: consider core.crlf when writing merge markers","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-01-25T07:02:02Z","receivedAt":"2016-01-25T07:02:02Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Eric,\n\nOn Sun, 24 Jan 2016, Eric Sunshine wrote:\n\n> On Sun, Jan 24, 2016 at 5:37 AM, Johannes Schindelin\n> wrote:\n> > On Fri, 22 Jan 2016, Eric Sunshine wrote:\n> >> On Fri, Jan 22, 2016 at 2:50 PM, Eric Sunshine wrote:\n> >> > On Fri, Jan 22, 2016 at 06:01:25PM +0100, Johannes Schindelin wrote:\n> >> >> +test_expect_success 'conflict markers contain CRLF when core.eol=crlf' '\n> >> >> + test_must_fail git -c core.eol=crlf merge-file -p \\\n> >> >> + nolf-diff1.txt nolf-orig.txt nolf-diff2.txt >output.txt &&\n> >> >> + test $(sed -n \"/\\.txt\\r$/p\" output.txt | wc -l) = 3\n> >> >\n> >> > The \"\\r\" isn't recognized by 'sed' on Mac OS X or BSD. Perhaps use\n> >> > instead:\n> >> >\n> >> > test $(cat output.txt | tr \"\\015\" Q | sed -n \"/\\.txtQ$/p\" | wc -l) = 3\n> >>\n> >> Or the 'sed' could even become at 'grep' at this point.\n> >\n> > Good point, thanks!\n> \n> And, without the unnecessary 'cat', of course (don't know what I was thinking):\n> \n> test $(tr \"\\015\" Q Johannes Schindelin writes:\n> \n> > The crucial idea hit me yesterday when I took a step back: all\n> > we need to do is to ensure that the end-of-line style is matched\n> > when *all* input files are LF-only, or when they are all CR/LF.\n> > In all other cases, we have mixed line endings anyway.\n> >\n> > And to do that, it is sufficient to look at *one single line\n> > ending* in the context. Technically, it does not even have to be\n> > the context, but the first line endings of the first file would\n> > be enough, however it is so much more pleasant if the conflict\n> > marker's eol matches the one of the preceding line.\n> \n> I like that approach. My understanding of the xdiff/xmerge code is\n> very rusty and I have some time to re-learn it before I can judge\n> that the change implements that approach correctly, though X-<.\n\nI hear ya. The xmerge code is my fault, too, and it did not help that I\nimitated the lack of documentation of the surrounding code.\n\nBut it all came back to me when I looked at the code. Hopefully these\nhints are helpful:\n\n- xe1 and xe2 refer to \"xdfenv_t\" types that essentially hold pointers to\n one pre-image and one post-image, each\n\n- The pre-images of xe1 and xe2 are identical, of course, because we are\n looking at a three-way merge\n\n- The xdfenv_t type refers to the pre-image as xdf1 and to the post-image\n as xdf2, both of the type \"xdfile_t\"\n\n- The xdfile_t structure contains the pointers to a parsed file. The lines\n are called \"recs\" (record) here\n\n- Each record has a pointed to the start of the line and a size that\n includes the newline byte, if any\n\n- The record parsing hard-codes 0x0a as newline (NEWLINEBYTES), which is\n used in the xdl_hash_record() function that is called from the\n xdl_prepare_ctx() function that parses each input file, and which in\n turn is called from the xdl_prepare_env() function that prepares a\n pair of files for diff'ing by the xdl_do_diff() function that in turn\n is called twice from xdl_merge() to diff orig->a and orig->b\n\n- Imitating libdiff's existing code, \"i\" is used to refer to a specific\n line; it is the line number decremented by one, i.e. 0 <= i < nrec\n (where \"nrec\" is the field of the xdfile_t structure indicating the\n line count)\n\nYou know, cobbling these notes together had an unexpected side effect: I\nrealized that another patch is needed to properly support CR/LF files:\nwhen the conflict hunk is at the end of one file and said file has no\ntrailing line ending, we add \\n always, but we should add \\r before that\nwhen appropriate.\n\nI'm on it.\n\nCiao,\nDscho\n"},{"id":"276704","messageId":"cover.1453709205.git.johannes.schindelin@gmx.de","threadId":"41241","inReplyTo":"cover.1453632296.git.johannes.schindelin@gmx.de","subject":"[PATCH v3 0/2] Let merge-file write out conflict markers with correct EOLs","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-01-25T08:06:59Z","receivedAt":"2016-01-25T08:06:59Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"The original patch was sent by Beat Bolli in\nhttp://thread.gmane.org/gmane.comp.version-control.git/281600\n\nMy suggestion to extend it to respect gitattributes led to\nchanges that broke the original patch. And they were misguided\nto begin with (see below).\n\nSince there have been a couple of \"What's cooking\" mails\ncontaining unheard calls for updates on this patch, I took it\non myself to fix things.\n\nJunio's comment that we might look at blobs containing CR/LF\nline endings made me rethink the entire approach, and I am now\nconvinced that we need to abandon the entire idea to make the\nconflict markers depend on config settings or attributes:\nafter all, I introduced `git merge-file` as a replacement for\nGNU merge that can be used *outside* of any repository, by design.\n\nThe crucial idea hit me yesterday when I took a step back: all\nwe need to do is to ensure that the end-of-line style is matched\nwhen *all* input files are LF-only, or when they are all CR/LF.\nIn all other cases, we have mixed line endings anyway.\n\nAnd to do that, it is sufficient to look at *one single line\nending* in the context. Technically, it does not even have to be\nthe context, but the first line endings of the first file would\nbe enough, however it is so much more pleasant if the conflict\nmarker's eol matches the one of the preceding line.\n\nTo prevent my future self from repeating mistakes, I added a\nlittle bit of background to the first commit message.\n\nTriggered by a comment by Junio, I realized that a second patch is\nneeded: we need to make sure that the conflicting sections are also\naugmented by the appropriate line endings if they lack any.\n\nRelative to v2, the first patch changed only in the test code, to\naccomodate for the newly-introduced second patch.\n\n\nJohannes Schindelin (2):\n merge-file: let conflict markers match end-of-line style of the\n context\n merge-file: ensure that conflict sections match eol style\n\n t/t6023-merge-file.sh | 13 +++++++\n xdiff/xmerge.c | 98 +++++++++++++++++++++++++++++++++++++++++----------\n 2 files changed, 93 insertions(+), 18 deletions(-)\n\nInterdiff vs v2:\n\n diff --git a/t/t6023-merge-file.sh b/t/t6023-merge-file.sh\n index f0cb9ce..1390548 100755\n --- a/t/t6023-merge-file.sh\n +++ b/t/t6023-merge-file.sh\n @@ -346,13 +346,14 @@ test_expect_success 'conflict at EOF without LF resolved by --union' \\\n \t printf \"line1\\nline2\\nline3x\\nline3y\" >expect.txt &&\n \t test_cmp expect.txt output.txt'\n \n -test_expect_success 'conflict markers match existing line endings' '\n -\tappend_cr crlf-orig.txt &&\n -\tappend_cr crlf-diff1.txt &&\n -\tappend_cr crlf-diff2.txt &&\n +test_expect_success 'conflict sections match existing line endings' '\n +\tprintf \"1\\\\r\\\\n2\\\\r\\\\n3\" >crlf-orig.txt &&\n +\tprintf \"1\\\\r\\\\n2\\\\r\\\\n4\" >crlf-diff1.txt &&\n +\tprintf \"1\\\\r\\\\n2\\\\r\\\\n5\" >crlf-diff2.txt &&\n \ttest_must_fail git -c core.eol=crlf merge-file -p \\\n \t\tcrlf-diff1.txt crlf-orig.txt crlf-diff2.txt >crlf.txt &&\n \ttest $(tr \"\\015\" Q nolf.txt &&\n \ttest $(tr \"\\015\" Q size;\n \t\tif (i == 0 || recs[count - 1]->ptr[i - 1] != '\\n') {\n +\t\t\tif (needs_cr) {\n +\t\t\t\tif (dest)\n +\t\t\t\t\tdest[size] = '\\r';\n +\t\t\t\tsize++;\n +\t\t\t}\n +\n \t\t\tif (dest)\n \t\t\t\tdest[size] = '\\n';\n \t\t\tsize++;\n @@ -133,14 +139,14 @@ static int xdl_recs_copy_0(int use_orig, xdfenv_t *xe, int i, int count, int add\n \treturn size;\n }\n \n -static int xdl_recs_copy(xdfenv_t *xe, int i, int count, int add_nl, char *dest)\n +static int xdl_recs_copy(xdfenv_t *xe, int i, int count, int needs_cr, int add_nl, char *dest)\n {\n -\treturn xdl_recs_copy_0(0, xe, i, count, add_nl, dest);\n +\treturn xdl_recs_copy_0(0, xe, i, count, needs_cr, add_nl, dest);\n }\n \n -static int xdl_orig_copy(xdfenv_t *xe, int i, int count, int add_nl, char *dest)\n +static int xdl_orig_copy(xdfenv_t *xe, int i, int count, int needs_cr, int add_nl, char *dest)\n {\n -\treturn xdl_recs_copy_0(1, xe, i, count, add_nl, dest);\n +\treturn xdl_recs_copy_0(1, xe, i, count, needs_cr, add_nl, dest);\n }\n \n /*\n @@ -172,15 +178,8 @@ static int is_eol_crlf(xdfile_t *file, int i)\n \t\tfile->recs[i - 1]->ptr[size - 2] == '\\r';\n }\n \n -static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,\n -\t\t\t xdfenv_t *xe2, const char *name2,\n -\t\t\t const char *name3,\n -\t\t\t int size, int i, int style,\n -\t\t\t xdmerge_t *m, char *dest, int marker_size)\n +static int is_cr_needed(xdfenv_t *xe1, xdfenv_t *xe2, xdmerge_t *m)\n {\n -\tint marker1_size = (name1 ? strlen(name1) + 1 : 0);\n -\tint marker2_size = (name2 ? strlen(name2) + 1 : 0);\n -\tint marker3_size = (name3 ? strlen(name3) + 1 : 0);\n \tint needs_cr;\n \n \t/* Match post-images' preceding, or first, lines' end-of-line style */\n @@ -191,14 +190,25 @@ static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,\n \tif (needs_cr)\n \t\tneeds_cr = is_eol_crlf(&xe1->xdf1, 0);\n \t/* If still undecided, use LF-only */\n -\tif (needs_cr < 0)\n -\t\tneeds_cr = 0;\n +\treturn needs_cr < 0 ? 0 : needs_cr;\n +}\n +\n +static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,\n +\t\t\t xdfenv_t *xe2, const char *name2,\n +\t\t\t const char *name3,\n +\t\t\t int size, int i, int style,\n +\t\t\t xdmerge_t *m, char *dest, int marker_size)\n +{\n +\tint marker1_size = (name1 ? strlen(name1) + 1 : 0);\n +\tint marker2_size = (name2 ? strlen(name2) + 1 : 0);\n +\tint marker3_size = (name3 ? strlen(name3) + 1 : 0);\n +\tint needs_cr = is_cr_needed(xe1, xe2, m);\n \n \tif (marker_size <= 0)\n \t\tmarker_size = DEFAULT_CONFLICT_MARKER_SIZE;\n \n \t/* Before conflicting part */\n -\tsize += xdl_recs_copy(xe1, i, m->i1 - i, 0,\n +\tsize += xdl_recs_copy(xe1, i, m->i1 - i, 0, 0,\n \t\t\t dest ? dest + size : NULL);\n \n \tif (!dest) {\n @@ -217,7 +227,7 @@ static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,\n \t}\n \n \t/* Postimage from side #1 */\n -\tsize += xdl_recs_copy(xe1, m->i1, m->chg1, 1,\n +\tsize += xdl_recs_copy(xe1, m->i1, m->chg1, needs_cr, 1,\n \t\t\t dest ? dest + size : NULL);\n \n \tif (style == XDL_MERGE_DIFF3) {\n @@ -236,7 +246,7 @@ static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,\n \t\t\t\tdest[size++] = '\\r';\n \t\t\tdest[size++] = '\\n';\n \t\t}\n -\t\tsize += xdl_orig_copy(xe1, m->i0, m->chg0, 1,\n +\t\tsize += xdl_orig_copy(xe1, m->i0, m->chg0, needs_cr, 1,\n \t\t\t\t dest ? dest + size : NULL);\n \t}\n \n @@ -251,7 +261,7 @@ static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,\n \t}\n \n \t/* Postimage from side #2 */\n -\tsize += xdl_recs_copy(xe2, m->i2, m->chg2, 1,\n +\tsize += xdl_recs_copy(xe2, m->i2, m->chg2, needs_cr, 1,\n \t\t\t dest ? dest + size : NULL);\n \tif (!dest) {\n \t\tsize += marker_size + 1 + needs_cr + marker2_size;\n @@ -290,21 +300,24 @@ static int xdl_fill_merge_buffer(xdfenv_t *xe1, const char *name1,\n \t\t\t\t\t\t marker_size);\n \t\telse if (m->mode & 3) {\n \t\t\t/* Before conflicting part */\n -\t\t\tsize += xdl_recs_copy(xe1, i, m->i1 - i, 0,\n +\t\t\tsize += xdl_recs_copy(xe1, i, m->i1 - i, 0, 0,\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, (m->mode & 2),\n +\t\t\tif (m->mode & 1) {\n +\t\t\t\tint needs_cr = is_cr_needed(xe1, xe2, m);\n +\n +\t\t\t\tsize += xdl_recs_copy(xe1, m->i1, m->chg1, needs_cr, (m->mode & 2),\n \t\t\t\t\t\t dest ? dest + size : NULL);\n +\t\t\t}\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, 0,\n +\t\t\t\tsize += xdl_recs_copy(xe2, m->i2, m->chg2, 0, 0,\n \t\t\t\t\t\t dest ? dest + size : NULL);\n \t\t} else\n \t\t\tcontinue;\n \t\ti = m->i1 + m->chg1;\n \t}\n -\tsize += xdl_recs_copy(xe1, i, xe1->xdf2.nrec - i, 0,\n +\tsize += xdl_recs_copy(xe1, i, xe1->xdf2.nrec - i, 0, 0,\n \t\t\t dest ? dest + size : NULL);\n \treturn size;\n }\n\n-- \n2.7.0.windows.1.7.g55a05c8\n"},{"id":"276703","messageId":"26c973eb3414a8634d515f3621c0ded77cf030ed.1453709205.git.johannes.schindelin@gmx.de","threadId":"41241","inReplyTo":"cover.1453709205.git.johannes.schindelin@gmx.de","subject":"[PATCH v3 1/2] merge-file: let conflict markers match end-of-line style of the context","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-01-25T08:07:04Z","receivedAt":"2016-01-25T08:07:04Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"When merging files with CR/LF line endings, the conflict markers should\nmatch those, lest the output file has mixed line endings.\n\nThis is particularly of interest on Windows, where some editors get\n*really* confused by mixed line endings.\n\nThe original version of this patch by Beat Bolli respected core.eol, and\na subsequent improvement by this developer also respected gitattributes.\nThis approach was suboptimal, though: `git merge-file` was invented as a\ndrop-in replacement for GNU merge and as such has no problem operating\noutside of any repository at all!\n\nAnother problem with the original approach was pointed out by Junio\nHamano: legacy repositories might have their text files committed using\nCR/LF line endings (and core.eol and the gitattributes would give us a\nfalse impression there). Therefore, the much superior approach is to\nsimply match the context's line endings, if any.\n\nWe actually do not have to look at the *entire* context at all: if the\nfiles are all LF-only, or if they all have CR/LF line endings, it is\nsufficient to look at just a *single* line to match that style. And if\nthe line endings are mixed anyway, it is *still* okay to imitate just a\nsingle line's eol: we will just add to the pile of mixed line endings,\nand there is nothing we can do about that.\n\nSo what we do is: we look at the line preceding the conflict, falling\nback to the line preceding that in case it was the last line and had no\nline ending, falling back to the first line, first in the first\npost-image, then the second post-image, and finally the pre-image.\nIf we find consistent CR/LF (or undecided) end-of-line style, we match\nthat, otherwise we use LF-only line endings for the conflict markers.\n\nNote that while it is true that there have to be at least two lines we\ncan look at (otherwise there would be no conflict), the same is not true\nfor line *endings*: the three files in question could all consist of a\nsingle line without any line ending, each. In this case we fall back to\nusing LF-only.\n\nSigned-off-by: Johannes Schindelin \n---\n t/t6023-merge-file.sh | 12 +++++++++++\n xdiff/xmerge.c | 57 +++++++++++++++++++++++++++++++++++++++++++++++----\n 2 files changed, 65 insertions(+), 4 deletions(-)\n\ndiff --git a/t/t6023-merge-file.sh b/t/t6023-merge-file.sh\nindex 190ee90..bb20cbc 100755\n--- a/t/t6023-merge-file.sh\n+++ b/t/t6023-merge-file.sh\n@@ -346,4 +346,16 @@ test_expect_success 'conflict at EOF without LF resolved by --union' \\\n \t printf \"line1\\nline2\\nline3x\\nline3y\" >expect.txt &&\n \t test_cmp expect.txt output.txt'\n \n+test_expect_success 'conflict markers match existing line endings' '\n+\tprintf \"1\\\\r\\\\n2\\\\r\\\\n3\" >crlf-orig.txt &&\n+\tprintf \"1\\\\r\\\\n2\\\\r\\\\n4\" >crlf-diff1.txt &&\n+\tprintf \"1\\\\r\\\\n2\\\\r\\\\n5\" >crlf-diff2.txt &&\n+\ttest_must_fail git -c core.eol=crlf merge-file -p \\\n+\t\tcrlf-diff1.txt crlf-orig.txt crlf-diff2.txt >crlf.txt &&\n+\ttest $(tr \"\\015\" Q nolf.txt &&\n+\ttest $(tr \"\\015\" Q nrec - 1)\n+\t\t/* All lines before the last *must* end in LF */\n+\t\treturn (size = file->recs[i]->size) > 1 &&\n+\t\t\tfile->recs[i]->ptr[size - 2] == '\\r';\n+\tif (!file->nrec)\n+\t\t/* Cannot determine eol style from empty file */\n+\t\treturn -1;\n+\tif ((size = file->recs[i]->size) &&\n+\t\t\tfile->recs[i]->ptr[size - 1] == '\\n')\n+\t\t/* Last line; ends in LF; Is it CR/LF? */\n+\t\treturn size > 1 &&\n+\t\t\tfile->recs[i]->ptr[size - 2] == '\\r';\n+\tif (!i)\n+\t\t/* The only line has no eol */\n+\t\treturn -1;\n+\t/* Determine eol from second-to-last line */\n+\treturn (size = file->recs[i - 1]->size) > 1 &&\n+\t\tfile->recs[i - 1]->ptr[size - 2] == '\\r';\n+}\n+\n static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,\n \t\t\t xdfenv_t *xe2, const char *name2,\n \t\t\t const char *name3,\n@@ -152,6 +181,18 @@ static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,\n \tint marker1_size = (name1 ? strlen(name1) + 1 : 0);\n \tint marker2_size = (name2 ? strlen(name2) + 1 : 0);\n \tint marker3_size = (name3 ? strlen(name3) + 1 : 0);\n+\tint needs_cr;\n+\n+\t/* Match post-images' preceding, or first, lines' end-of-line style */\n+\tneeds_cr = is_eol_crlf(&xe1->xdf2, m->i1 ? m->i1 - 1 : 0);\n+\tif (needs_cr)\n+\t\tneeds_cr = is_eol_crlf(&xe2->xdf2, m->i2 ? m->i2 - 1 : 0);\n+\t/* Look at pre-image's first line, unless we already settled on LF */\n+\tif (needs_cr)\n+\t\tneeds_cr = is_eol_crlf(&xe1->xdf1, 0);\n+\t/* If still undecided, use LF-only */\n+\tif (needs_cr < 0)\n+\t\tneeds_cr = 0;\n \n \tif (marker_size <= 0)\n \t\tmarker_size = DEFAULT_CONFLICT_MARKER_SIZE;\n@@ -161,7 +202,7 @@ static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,\n \t\t\t dest ? dest + size : NULL);\n \n \tif (!dest) {\n-\t\tsize += marker_size + 1 + marker1_size;\n+\t\tsize += marker_size + 1 + needs_cr + marker1_size;\n \t} else {\n \t\tmemset(dest + size, '<', marker_size);\n \t\tsize += marker_size;\n@@ -170,6 +211,8 @@ static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,\n \t\t\tmemcpy(dest + size + 1, name1, marker1_size - 1);\n \t\t\tsize += marker1_size;\n \t\t}\n+\t\tif (needs_cr)\n+\t\t\tdest[size++] = '\\r';\n \t\tdest[size++] = '\\n';\n \t}\n \n@@ -180,7 +223,7 @@ static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,\n \tif (style == XDL_MERGE_DIFF3) {\n \t\t/* Shared preimage */\n \t\tif (!dest) {\n-\t\t\tsize += marker_size + 1 + marker3_size;\n+\t\t\tsize += marker_size + 1 + needs_cr + marker3_size;\n \t\t} else {\n \t\t\tmemset(dest + size, '|', marker_size);\n \t\t\tsize += marker_size;\n@@ -189,6 +232,8 @@ static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,\n \t\t\t\tmemcpy(dest + size + 1, name3, marker3_size - 1);\n \t\t\t\tsize += marker3_size;\n \t\t\t}\n+\t\t\tif (needs_cr)\n+\t\t\t\tdest[size++] = '\\r';\n \t\t\tdest[size++] = '\\n';\n \t\t}\n \t\tsize += xdl_orig_copy(xe1, m->i0, m->chg0, 1,\n@@ -196,10 +241,12 @@ static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,\n \t}\n \n \tif (!dest) {\n-\t\tsize += marker_size + 1;\n+\t\tsize += marker_size + 1 + needs_cr;\n \t} else {\n \t\tmemset(dest + size, '=', marker_size);\n \t\tsize += marker_size;\n+\t\tif (needs_cr)\n+\t\t\tdest[size++] = '\\r';\n \t\tdest[size++] = '\\n';\n \t}\n \n@@ -207,7 +254,7 @@ static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,\n \tsize += xdl_recs_copy(xe2, m->i2, m->chg2, 1,\n \t\t\t dest ? dest + size : NULL);\n \tif (!dest) {\n-\t\tsize += marker_size + 1 + marker2_size;\n+\t\tsize += marker_size + 1 + needs_cr + marker2_size;\n \t} else {\n \t\tmemset(dest + size, '>', marker_size);\n \t\tsize += marker_size;\n@@ -216,6 +263,8 @@ static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,\n \t\t\tmemcpy(dest + size + 1, name2, marker2_size - 1);\n \t\t\tsize += marker2_size;\n \t\t}\n+\t\tif (needs_cr)\n+\t\t\tdest[size++] = '\\r';\n \t\tdest[size++] = '\\n';\n \t}\n \treturn size;\n-- \n2.7.0.windows.1.7.g55a05c8\n"},{"id":"276705","messageId":"4bae6450eb64461584a9acdefc9a3226c95ef357.1453709205.git.johannes.schindelin@gmx.de","threadId":"41241","inReplyTo":"cover.1453709205.git.johannes.schindelin@gmx.de","subject":"[PATCH v3 2/2] merge-file: ensure that conflict sections match eol style","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-01-25T08:07:11Z","receivedAt":"2016-01-25T08:07:11Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"In the previous patch, we made sure that the conflict markers themselves\nmatch the end-of-line style of the input files. However, this still left\nout the conflicting text itself: if it lacks a trailing newline, we\nadd one, and should add a carriage return when appropriate, too.\n\nSigned-off-by: Johannes Schindelin \n---\n t/t6023-merge-file.sh | 3 ++-\n xdiff/xmerge.c | 61 +++++++++++++++++++++++++++++++--------------------\n 2 files changed, 39 insertions(+), 25 deletions(-)\n\ndiff --git a/t/t6023-merge-file.sh b/t/t6023-merge-file.sh\nindex bb20cbc..1390548 100755\n--- a/t/t6023-merge-file.sh\n+++ b/t/t6023-merge-file.sh\n@@ -346,13 +346,14 @@ test_expect_success 'conflict at EOF without LF resolved by --union' \\\n \t printf \"line1\\nline2\\nline3x\\nline3y\" >expect.txt &&\n \t test_cmp expect.txt output.txt'\n \n-test_expect_success 'conflict markers match existing line endings' '\n+test_expect_success 'conflict sections match existing line endings' '\n \tprintf \"1\\\\r\\\\n2\\\\r\\\\n3\" >crlf-orig.txt &&\n \tprintf \"1\\\\r\\\\n2\\\\r\\\\n4\" >crlf-diff1.txt &&\n \tprintf \"1\\\\r\\\\n2\\\\r\\\\n5\" >crlf-diff2.txt &&\n \ttest_must_fail git -c core.eol=crlf merge-file -p \\\n \t\tcrlf-diff1.txt crlf-orig.txt crlf-diff2.txt >crlf.txt &&\n \ttest $(tr \"\\015\" Q nolf.txt &&\n \ttest $(tr \"\\015\" Q size;\n \t\tif (i == 0 || recs[count - 1]->ptr[i - 1] != '\\n') {\n+\t\t\tif (needs_cr) {\n+\t\t\t\tif (dest)\n+\t\t\t\t\tdest[size] = '\\r';\n+\t\t\t\tsize++;\n+\t\t\t}\n+\n \t\t\tif (dest)\n \t\t\t\tdest[size] = '\\n';\n \t\t\tsize++;\n@@ -133,14 +139,14 @@ static int xdl_recs_copy_0(int use_orig, xdfenv_t *xe, int i, int count, int add\n \treturn size;\n }\n \n-static int xdl_recs_copy(xdfenv_t *xe, int i, int count, int add_nl, char *dest)\n+static int xdl_recs_copy(xdfenv_t *xe, int i, int count, int needs_cr, int add_nl, char *dest)\n {\n-\treturn xdl_recs_copy_0(0, xe, i, count, add_nl, dest);\n+\treturn xdl_recs_copy_0(0, xe, i, count, needs_cr, add_nl, dest);\n }\n \n-static int xdl_orig_copy(xdfenv_t *xe, int i, int count, int add_nl, char *dest)\n+static int xdl_orig_copy(xdfenv_t *xe, int i, int count, int needs_cr, int add_nl, char *dest)\n {\n-\treturn xdl_recs_copy_0(1, xe, i, count, add_nl, dest);\n+\treturn xdl_recs_copy_0(1, xe, i, count, needs_cr, add_nl, dest);\n }\n \n /*\n@@ -172,15 +178,8 @@ static int is_eol_crlf(xdfile_t *file, int i)\n \t\tfile->recs[i - 1]->ptr[size - 2] == '\\r';\n }\n \n-static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,\n-\t\t\t xdfenv_t *xe2, const char *name2,\n-\t\t\t const char *name3,\n-\t\t\t int size, int i, int style,\n-\t\t\t xdmerge_t *m, char *dest, int marker_size)\n+static int is_cr_needed(xdfenv_t *xe1, xdfenv_t *xe2, xdmerge_t *m)\n {\n-\tint marker1_size = (name1 ? strlen(name1) + 1 : 0);\n-\tint marker2_size = (name2 ? strlen(name2) + 1 : 0);\n-\tint marker3_size = (name3 ? strlen(name3) + 1 : 0);\n \tint needs_cr;\n \n \t/* Match post-images' preceding, or first, lines' end-of-line style */\n@@ -191,14 +190,25 @@ static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,\n \tif (needs_cr)\n \t\tneeds_cr = is_eol_crlf(&xe1->xdf1, 0);\n \t/* If still undecided, use LF-only */\n-\tif (needs_cr < 0)\n-\t\tneeds_cr = 0;\n+\treturn needs_cr < 0 ? 0 : needs_cr;\n+}\n+\n+static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,\n+\t\t\t xdfenv_t *xe2, const char *name2,\n+\t\t\t const char *name3,\n+\t\t\t int size, int i, int style,\n+\t\t\t xdmerge_t *m, char *dest, int marker_size)\n+{\n+\tint marker1_size = (name1 ? strlen(name1) + 1 : 0);\n+\tint marker2_size = (name2 ? strlen(name2) + 1 : 0);\n+\tint marker3_size = (name3 ? strlen(name3) + 1 : 0);\n+\tint needs_cr = is_cr_needed(xe1, xe2, m);\n \n \tif (marker_size <= 0)\n \t\tmarker_size = DEFAULT_CONFLICT_MARKER_SIZE;\n \n \t/* Before conflicting part */\n-\tsize += xdl_recs_copy(xe1, i, m->i1 - i, 0,\n+\tsize += xdl_recs_copy(xe1, i, m->i1 - i, 0, 0,\n \t\t\t dest ? dest + size : NULL);\n \n \tif (!dest) {\n@@ -217,7 +227,7 @@ static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,\n \t}\n \n \t/* Postimage from side #1 */\n-\tsize += xdl_recs_copy(xe1, m->i1, m->chg1, 1,\n+\tsize += xdl_recs_copy(xe1, m->i1, m->chg1, needs_cr, 1,\n \t\t\t dest ? dest + size : NULL);\n \n \tif (style == XDL_MERGE_DIFF3) {\n@@ -236,7 +246,7 @@ static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,\n \t\t\t\tdest[size++] = '\\r';\n \t\t\tdest[size++] = '\\n';\n \t\t}\n-\t\tsize += xdl_orig_copy(xe1, m->i0, m->chg0, 1,\n+\t\tsize += xdl_orig_copy(xe1, m->i0, m->chg0, needs_cr, 1,\n \t\t\t\t dest ? dest + size : NULL);\n \t}\n \n@@ -251,7 +261,7 @@ static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,\n \t}\n \n \t/* Postimage from side #2 */\n-\tsize += xdl_recs_copy(xe2, m->i2, m->chg2, 1,\n+\tsize += xdl_recs_copy(xe2, m->i2, m->chg2, needs_cr, 1,\n \t\t\t dest ? dest + size : NULL);\n \tif (!dest) {\n \t\tsize += marker_size + 1 + needs_cr + marker2_size;\n@@ -290,21 +300,24 @@ static int xdl_fill_merge_buffer(xdfenv_t *xe1, const char *name1,\n \t\t\t\t\t\t marker_size);\n \t\telse if (m->mode & 3) {\n \t\t\t/* Before conflicting part */\n-\t\t\tsize += xdl_recs_copy(xe1, i, m->i1 - i, 0,\n+\t\t\tsize += xdl_recs_copy(xe1, i, m->i1 - i, 0, 0,\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, (m->mode & 2),\n+\t\t\tif (m->mode & 1) {\n+\t\t\t\tint needs_cr = is_cr_needed(xe1, xe2, m);\n+\n+\t\t\t\tsize += xdl_recs_copy(xe1, m->i1, m->chg1, needs_cr, (m->mode & 2),\n \t\t\t\t\t\t dest ? dest + size : NULL);\n+\t\t\t}\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, 0,\n+\t\t\t\tsize += xdl_recs_copy(xe2, m->i2, m->chg2, 0, 0,\n \t\t\t\t\t\t dest ? dest + size : NULL);\n \t\t} else\n \t\t\tcontinue;\n \t\ti = m->i1 + m->chg1;\n \t}\n-\tsize += xdl_recs_copy(xe1, i, xe1->xdf2.nrec - i, 0,\n+\tsize += xdl_recs_copy(xe1, i, xe1->xdf2.nrec - i, 0, 0,\n \t\t\t dest ? dest + size : NULL);\n \treturn size;\n }\n-- \n2.7.0.windows.1.7.g55a05c8\n"},{"id":"276706","messageId":"56A5DDA8.1030404@web.de","threadId":"41241","inReplyTo":"26c973eb3414a8634d515f3621c0ded77cf030ed.1453709205.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH v3 1/2] merge-file: let conflict markers match end-of-line style of the context","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2016-01-25T08:32:40Z","receivedAt":"2016-01-25T08:32:40Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 25.01.16 09:07, Johannes Schindelin wrote:\n[]\n> diff --git a/xdiff/xmerge.c b/xdiff/xmerge.c\n> index 625198e..c852acc 100644\n> --- a/xdiff/xmerge.c\n> +++ b/xdiff/xmerge.c\n> @@ -143,6 +143,35 @@ static int xdl_orig_copy(xdfenv_t *xe, int i, int count, int add_nl, char *dest)\n> \treturn xdl_recs_copy_0(1, xe, i, count, add_nl, dest);\n> }\n> \n> +/*\n> + * Returns 1 if the i'th line ends in CR/LF (if it is the last line and\n> + * has no eol, the preceding line, if any), 0 if it ends in LF-only, and\n> + * -1 if the line ending cannot be determined.\n> + */\n> +static int is_eol_crlf(xdfile_t *file, int i)\nMinot nit: Could that be \nis_eol_crlf(xdfile_t *file, int lineno)\n(or may be)\nis_eol_crlf(const xdfile_t *file, int lineno)\n(or may be)\nhas_eol_crlf(const xdfile_t *file, int lineno)\n\n> +{\n> +\tlong size;\n> +\n> +\tif (i < file->nrec - 1)\n> +\t\t/* All lines before the last *must* end in LF */\n> +\t\treturn (size = file->recs[i]->size) > 1 &&\n> +\t\t\tfile->recs[i]->ptr[size - 2] == '\\r';\n> +\tif (!file->nrec)\n> +\t\t/* Cannot determine eol style from empty file */\n> +\t\treturn -1;\n> +\tif ((size = file->recs[i]->size) &&\n> +\t\t\tfile->recs[i]->ptr[size - 1] == '\\n')\n> +\t\t/* Last line; ends in LF; Is it CR/LF? */\n> +\t\treturn size > 1 &&\n> +\t\t\tfile->recs[i]->ptr[size - 2] == '\\r';\n> +\tif (!i)\n> +\t\t/* The only line has no eol */\n> +\t\treturn -1;\n> +\t/* Determine eol from second-to-last line */\n> +\treturn (size = file->recs[i - 1]->size) > 1 &&\n> +\t\tfile->recs[i - 1]->ptr[size - 2] == '\\r';\n> +}\n> +\n"},{"id":"276707","messageId":"alpine.DEB.2.20.1601250958330.2964@virtualbox","threadId":"41241","inReplyTo":"56A5DDA8.1030404@web.de","subject":"Re: [PATCH v3 1/2] merge-file: let conflict markers match end-of-line style of the context","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-01-25T08:59:47Z","receivedAt":"2016-01-25T08:59:47Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Torsten,\n\nOn Mon, 25 Jan 2016, Torsten Bögershausen wrote:\n\n> On 25.01.16 09:07, Johannes Schindelin wrote:\n> []\n> > diff --git a/xdiff/xmerge.c b/xdiff/xmerge.c\n> > index 625198e..c852acc 100644\n> > --- a/xdiff/xmerge.c\n> > +++ b/xdiff/xmerge.c\n> > @@ -143,6 +143,35 @@ static int xdl_orig_copy(xdfenv_t *xe, int i, int count, int add_nl, char *dest)\n> > \treturn xdl_recs_copy_0(1, xe, i, count, add_nl, dest);\n> > }\n> > \n> > +/*\n> > + * Returns 1 if the i'th line ends in CR/LF (if it is the last line and\n> > + * has no eol, the preceding line, if any), 0 if it ends in LF-only, and\n> > + * -1 if the line ending cannot be determined.\n> > + */\n> > +static int is_eol_crlf(xdfile_t *file, int i)\n> Minot nit: Could that be \n> is_eol_crlf(xdfile_t *file, int lineno)\n> (or may be)\n> is_eol_crlf(const xdfile_t *file, int lineno)\n> (or may be)\n> has_eol_crlf(const xdfile_t *file, int lineno)\n\nThe surrounding code consistently uses i, and not lineno. The reason (I\nguess) is that i starts at 0 whereas lineno would have to start at 1, and\nwe can easily avoid adding and subtracting 1 all the time.\n\nCiao,\nDscho"},{"id":"276708","messageId":"alpine.DEB.2.20.1601251001360.2964@virtualbox","threadId":"41241","inReplyTo":"4bae6450eb64461584a9acdefc9a3226c95ef357.1453709205.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH v3 2/2] merge-file: ensure that conflict sections match eol style","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-01-25T09:20:47Z","receivedAt":"2016-01-25T09:20:47Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Mon, 25 Jan 2016, Johannes Schindelin wrote:\n\n> diff --git a/xdiff/xmerge.c b/xdiff/xmerge.c\n> index c852acc..d98f430 100644\n> --- a/xdiff/xmerge.c\n> +++ b/xdiff/xmerge.c\n> @@ -172,15 +178,8 @@ static int is_eol_crlf(xdfile_t *file, int i)\n> \t\tfile->recs[i - 1]->ptr[size - 2] == '\\r';\n> }\n> \n> -static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,\n> -\t\t\t xdfenv_t *xe2, const char *name2,\n> -\t\t\t const char *name3,\n> -\t\t\t int size, int i, int style,\n> -\t\t\t xdmerge_t *m, char *dest, int marker_size)\n> +static int is_cr_needed(xdfenv_t *xe1, xdfenv_t *xe2, xdmerge_t *m)\n> {\n> -\tint marker1_size = (name1 ? strlen(name1) + 1 : 0);\n> -\tint marker2_size = (name2 ? strlen(name2) + 1 : 0);\n> -\tint marker3_size = (name3 ? strlen(name3) + 1 : 0);\n> \tint needs_cr;\n> \n> \t/* Match post-images' preceding, or first, lines' end-of-line style */\n> @@ -191,14 +190,25 @@ static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,\n> \tif (needs_cr)\n> \t\tneeds_cr = is_eol_crlf(&xe1->xdf1, 0);\n> \t/* If still undecided, use LF-only */\n> -\tif (needs_cr < 0)\n> -\t\tneeds_cr = 0;\n> +\treturn needs_cr < 0 ? 0 : needs_cr;\n> +}\n> +\n> +static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,\n> +\t\t\t xdfenv_t *xe2, const char *name2,\n> +\t\t\t const char *name3,\n> +\t\t\t int size, int i, int style,\n> +\t\t\t xdmerge_t *m, char *dest, int marker_size)\n> +{\n> +\tint marker1_size = (name1 ? strlen(name1) + 1 : 0);\n> +\tint marker2_size = (name2 ? strlen(name2) + 1 : 0);\n> +\tint marker3_size = (name3 ? strlen(name3) + 1 : 0);\n> +\tint needs_cr = is_cr_needed(xe1, xe2, m);\n> \n> \tif (marker_size <= 0)\n> \t\tmarker_size = DEFAULT_CONFLICT_MARKER_SIZE;\n> \n\nOh bummer. I just realized that I should have refactored that already in\npatch 1/2 before sending out v3. Of course it would be true to history to\ndo the refactoring only as part of 2/2, but who cares about true history\nwhen one can rewrite it?\n\nWill send out v4 in a while (I want to wait for more feedback in case I\nneed to change more things.) In the meantime, you can always look at my\npatch series' current state here:\n\n\thttps://github.com/git/git/compare/next...dscho:merge-marker-crlf\n\nCiao,\nDscho\n"},{"id":"276732","messageId":"56A67B6F.60300@ramsayjones.plus.com","threadId":"41241","inReplyTo":"alpine.DEB.2.20.1601250749580.2964@virtualbox","subject":"Re: [PATCH v2 1/1] merge-file: let conflict markers match end-of-line style of the context","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2016-01-25T19:45:51Z","receivedAt":"2016-01-25T19:45:51Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 25/01/16 06:53, Johannes Schindelin wrote:\n> Hi Torsten,\n> \n> On Sun, 24 Jan 2016, Torsten Bögershausen wrote:\n> \n>> On 24.01.16 11:48, Johannes Schindelin wrote:\n>> (I had the same reasoning about the CRLF in the working tree:\n>> We don't need to look at core.autocrlf/attributes, so Ack from me)\n>>\n>>> +test_expect_success 'conflict markers match existing line endings' '\n>>> +\tappend_cr crlf-orig.txt &&\n>>> +\tappend_cr crlf-diff1.txt &&\n>>> +\tappend_cr crlf-diff2.txt &&\n>>> +\ttest_must_fail git -c core.eol=crlf merge-file -p \\\n>>> +\t\tcrlf-diff1.txt crlf-orig.txt crlf-diff2.txt >crlf.txt &&\n>>> +\ttest $(tr \"\\015\" Q >> +\ttest_must_fail git -c core.eol=crlf merge-file -p \\\n>>> +\t\tnolf-diff1.txt nolf-orig.txt nolf-diff2.txt >nolf.txt &&\n>>> +\ttest $(tr \"\\015\" Q >> +'\n>>> +\n>>\n>> Minor remark:\n>>\n>> Ramsay suggested a test that doesn't use grep or wc and looks like this:\n>>\n>> test_expect_success 'conflict markers contain CRLF when core.eol=crlf' '\n>> test_must_fail git -c core.eol=crlf merge-file -p \\\n>> nolf-diff1.txt nolf-orig.txt nolf-diff2.txt >output.txt &&\n>> tr \"\\015\" Q |].*Q$/p\" >out.txt &&\n>> cat >expect.txt <<-\\EOF &&\n>> <<<<<<< nolf-diff1.txtQ\n>> ||||||| nolf-orig.txtQ\n>> =======Q\n>> >>>>>>> nolf-diff2.txtQ\n>> EOF\n>> test_cmp expect.txt out.txt\n>> '\n> \n> Probably he wrapped it at less than 192 columns per row, though ;-)\n> \n;-)\n> Seriously again, this longer version might test more, but it definitely\n> also tests more than what I actually want to test: I am simply interested\n> to verify that the conflict markers end in CR/LF when appropriate.\n\nBut you are only testing 3/4 conflict markers end in CR/LF. :-D\n\n> Read: I\n> am uncertain that I want to spend the additional lines on testing more\n> than actually necessary.\n\nIf the here doc is too verbose for you, how about something like this\n(totally untested):\n\n test $(tr \"\\015\" Q |].*Q$\" | wc -l) -eq 4\n\ninstead?\n\nHTH\n\nATB,\nRamsay Jones\n"},{"id":"276733","messageId":"xmqqr3h5fmf4.fsf@gitster.mtv.corp.google.com","threadId":"41241","inReplyTo":"26c973eb3414a8634d515f3621c0ded77cf030ed.1453709205.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH v3 1/2] merge-file: let conflict markers match end-of-line style of the context","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-01-25T20:12:47Z","receivedAt":"2016-01-25T20:12:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> We actually do not have to look at the *entire* context at all: if the\n> files are all LF-only, or if they all have CR/LF line endings, it is\n> sufficient to look at just a *single* line to match that style. And if\n> the line endings are mixed anyway, it is *still* okay to imitate just a\n> single line's eol: we will just add to the pile of mixed line endings,\n> and there is nothing we can do about that.\n\nIsn't there one thing we can do still? If we use CRLF for the\nmarker lines when the content is already mixed, I'd think it would\nhelp Notepad (not necessary for Notepad2 or Wordpad IIUC) by making\nsure that they can see where the marker lines end correctly.\n\nI do not care too deeply about this; just throwing it out as a\npossibility to help Windowsy folks a bit more.\n\n> Note that while it is true that there have to be at least two lines we\n> can look at (otherwise there would be no conflict), the same is not true\n> for line *endings*: the three files in question could all consist of a\n> single line without any line ending, each. In this case we fall back to\n> using LF-only.\n\nYeah, this is tricky, and from the same \"helping Notepad that\nconcatenates lines with LF-only\" perspective I should perhaps be\nsuggesting to use CRLF in such a case, too, but I would say we\nshould not do so. Three variants of a LF-only file may have\nconflict at the incomplete last line, and if we only look at their\n\"no EOL\"-ness and decide to add CRLF to the result, that would be\nirritatingly wrong.\n"},{"id":"276767","messageId":"alpine.DEB.2.20.1601260951030.2964@virtualbox","threadId":"41241","inReplyTo":"56A67B6F.60300@ramsayjones.plus.com","subject":"Re: [PATCH v2 1/1] merge-file: let conflict markers match end-of-line style of the context","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-01-26T08:54:24Z","receivedAt":"2016-01-26T08:54:24Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Ramsay,\n\nOn Mon, 25 Jan 2016, Ramsay Jones wrote:\n\n> On 25/01/16 06:53, Johannes Schindelin wrote:\n> > \n> > On Sun, 24 Jan 2016, Torsten Bögershausen wrote:\n> > \n> >> On 24.01.16 11:48, Johannes Schindelin wrote:\n> >> (I had the same reasoning about the CRLF in the working tree:\n> >> We don't need to look at core.autocrlf/attributes, so Ack from me)\n> >>\n> >>> +test_expect_success 'conflict markers match existing line endings' '\n> >>> +\tappend_cr crlf-orig.txt &&\n> >>> +\tappend_cr crlf-diff1.txt &&\n> >>> +\tappend_cr crlf-diff2.txt &&\n> >>> +\ttest_must_fail git -c core.eol=crlf merge-file -p \\\n> >>> +\t\tcrlf-diff1.txt crlf-orig.txt crlf-diff2.txt >crlf.txt &&\n> >>> +\ttest $(tr \"\\015\" Q >>> +\ttest_must_fail git -c core.eol=crlf merge-file -p \\\n> >>> +\t\tnolf-diff1.txt nolf-orig.txt nolf-diff2.txt >nolf.txt &&\n> >>> +\ttest $(tr \"\\015\" Q >>> +'\n> >>> +\n> >>\n> >> Minor remark:\n> >>\n> >> Ramsay suggested a test that doesn't use grep or wc and looks like this:\n> >>\n> >> test_expect_success 'conflict markers contain CRLF when core.eol=crlf' '\n> >> test_must_fail git -c core.eol=crlf merge-file -p \\\n> >> nolf-diff1.txt nolf-orig.txt nolf-diff2.txt >output.txt &&\n> >> tr \"\\015\" Q |].*Q$/p\" >out.txt &&\n> >> cat >expect.txt <<-\\EOF &&\n> >> <<<<<<< nolf-diff1.txtQ\n> >> ||||||| nolf-orig.txtQ\n> >> =======Q\n> >> >>>>>>> nolf-diff2.txtQ\n> >> EOF\n> >> test_cmp expect.txt out.txt\n> >> '\n> > \n> > Probably he wrapped it at less than 192 columns per row, though ;-)\n> > \n> ;-)\n> > Seriously again, this longer version might test more, but it definitely\n> > also tests more than what I actually want to test: I am simply interested\n> > to verify that the conflict markers end in CR/LF when appropriate.\n> \n> But you are only testing 3/4 conflict markers end in CR/LF. :-D\n\nThe fact that ||| markers are present is the fault of previous test cases.\nI tried to make a point of *not* relying on such a side effect (so as to\ndebug failures quicker by commenting out all previous test cases).\n\nSo the fact that I am testing only 3 of the 4 conflict markers is very\nmuch by design.\n\n> > Read: I am uncertain that I want to spend the additional lines on\n> > testing more than actually necessary.\n> \n> If the here doc is too verbose for you, how about something like this\n> (totally untested):\n> \n> test $(tr \"\\015\" Q |].*Q$\" | wc -l) -eq 4\n> \n> instead?\n\nHmm. I do not see the benefit over grepping for `txtQ$` it's essentially\nthe same.\n\n> HTH\n\nHow the hell?\n\n> ATB,\n> Ramsay Jones\n\nAuthorization to Buy?\n\nCiao,\nDscho"},{"id":"276768","messageId":"alpine.DEB.2.20.1601260955320.2964@virtualbox","threadId":"41241","inReplyTo":"xmqqr3h5fmf4.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v3 1/2] merge-file: let conflict markers match end-of-line style of the context","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-01-26T09:04:53Z","receivedAt":"2016-01-26T09:04:53Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Mon, 25 Jan 2016, Junio C Hamano wrote:\n\n> Johannes Schindelin writes:\n> \n> > We actually do not have to look at the *entire* context at all: if the\n> > files are all LF-only, or if they all have CR/LF line endings, it is\n> > sufficient to look at just a *single* line to match that style. And if\n> > the line endings are mixed anyway, it is *still* okay to imitate just a\n> > single line's eol: we will just add to the pile of mixed line endings,\n> > and there is nothing we can do about that.\n> \n> Isn't there one thing we can do still? If we use CRLF for the\n> marker lines when the content is already mixed, I'd think it would\n> help Notepad (not necessary for Notepad2 or Wordpad IIUC) by making\n> sure that they can see where the marker lines end correctly.\n\nNot sure. You might end up with a very long line (containing plenty of LF\n\"characters\") and the conflict marker *at the end* of said line, with a\nCR/LF after it. I would not call that particularly helpful.\n\nSeeing as we really cannot do anything in this case, I thought it would be\na good idea to avoid trying (and failing) to be smart here.\n\n> > Note that while it is true that there have to be at least two lines we\n> > can look at (otherwise there would be no conflict), the same is not true\n> > for line *endings*: the three files in question could all consist of a\n> > single line without any line ending, each. In this case we fall back to\n> > using LF-only.\n> \n> Yeah, this is tricky, and from the same \"helping Notepad that\n> concatenates lines with LF-only\" perspective I should perhaps be\n> suggesting to use CRLF in such a case, too, but I would say we\n> should not do so. Three variants of a LF-only file may have\n> conflict at the incomplete last line, and if we only look at their\n> \"no EOL\"-ness and decide to add CRLF to the result, that would be\n> irritatingly wrong.\n\nOh, but there is the fall-back to the first line. So if we have three\nvariants of an LF-only file, the logic will figure out that LF-only\nend-of-lines are to be used.\n\nSo the *only* case where we really have to pick and choose is when all\nthree files contain only one (or no) line that is not terminated by a line\nfeed.\n\nI briefly considered to choose EOL_NATIVE in that case, but I really do\nnot like that unnecessary deviation from Linux Git.\n\nCiao,\nDscho\n"},{"id":"276815","messageId":"cover.1453819314.git.johannes.schindelin@gmx.de","threadId":"41241","inReplyTo":"cover.1453709205.git.johannes.schindelin@gmx.de","subject":"[PATCH v4 0/2] Let merge-file write out conflict markers with correct EOLs","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-01-26T14:42:06Z","receivedAt":"2016-01-26T14:42:06Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"The original patch was sent by Beat Bolli in\nhttp://thread.gmane.org/gmane.comp.version-control.git/281600\n\nMy suggestion to extend it to respect gitattributes led to\nchanges that broke the original patch. And they were misguided\nto begin with (see below).\n\nSince there have been a couple of \"What's cooking\" mails\ncontaining unheard calls for updates on this patch, I took it\non myself to fix things.\n\nJunio's comment that we might look at blobs containing CR/LF\nline endings made me rethink the entire approach, and I am now\nconvinced that we need to abandon the entire idea to make the\nconflict markers depend on config settings or attributes:\nafter all, I introduced `git merge-file` as a replacement for\nGNU merge that can be used *outside* of any repository, by design.\n\nThe crucial idea hit me yesterday when I took a step back: all\nwe need to do is to ensure that the end-of-line style is matched\nwhen *all* input files are LF-only, or when they are all CR/LF.\nIn all other cases, we have mixed line endings anyway.\n\nAnd to do that, it is sufficient to look at *one single line\nending* in the context. Technically, it does not even have to be\nthe context, but the first line endings of the first file would\nbe enough, however it is so much more pleasant if the conflict\nmarker's eol matches the one of the preceding line.\n\nTo prevent my future self from repeating mistakes, I added a\nlittle bit of background to the first commit message.\n\nTriggered by a comment by Junio, I realized that a second patch is\nneeded: we need to make sure that the conflicting sections are also\naugmented by the appropriate line endings if they lack any.\n\nThe interdiff vs v3 is empty because I only pulled a refactoring\nfrom 2/2 into 1/2 already, making the series easier to read.\n\n\nJohannes Schindelin (2):\n merge-file: let conflict markers match end-of-line style of the\n context\n merge-file: ensure that conflict sections match eol style\n\n t/t6023-merge-file.sh | 13 +++++++\n xdiff/xmerge.c | 98 +++++++++++++++++++++++++++++++++++++++++----------\n 2 files changed, 93 insertions(+), 18 deletions(-)\n\n-- \n2.7.0.windows.1.7.g55a05c8\n"},{"id":"276814","messageId":"13379804ad901b53d78d741156677664924fc50d.1453819314.git.johannes.schindelin@gmx.de","threadId":"41241","inReplyTo":"cover.1453819314.git.johannes.schindelin@gmx.de","subject":"[PATCH v4 1/2] merge-file: let conflict markers match end-of-line style of the context","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-01-26T14:42:12Z","receivedAt":"2016-01-26T14:42:12Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"When merging files with CR/LF line endings, the conflict markers should\nmatch those, lest the output file has mixed line endings.\n\nThis is particularly of interest on Windows, where some editors get\n*really* confused by mixed line endings.\n\nThe original version of this patch by Beat Bolli respected core.eol, and\na subsequent improvement by this developer also respected gitattributes.\nThis approach was suboptimal, though: `git merge-file` was invented as a\ndrop-in replacement for GNU merge and as such has no problem operating\noutside of any repository at all!\n\nAnother problem with the original approach was pointed out by Junio\nHamano: legacy repositories might have their text files committed using\nCR/LF line endings (and core.eol and the gitattributes would give us a\nfalse impression there). Therefore, the much superior approach is to\nsimply match the context's line endings, if any.\n\nWe actually do not have to look at the *entire* context at all: if the\nfiles are all LF-only, or if they all have CR/LF line endings, it is\nsufficient to look at just a *single* line to match that style. And if\nthe line endings are mixed anyway, it is *still* okay to imitate just a\nsingle line's eol: we will just add to the pile of mixed line endings,\nand there is nothing we can do about that.\n\nSo what we do is: we look at the line preceding the conflict, falling\nback to the line preceding that in case it was the last line and had no\nline ending, falling back to the first line, first in the first\npost-image, then the second post-image, and finally the pre-image.\nIf we find consistent CR/LF (or undecided) end-of-line style, we match\nthat, otherwise we use LF-only line endings for the conflict markers.\n\nNote that while it is true that there have to be at least two lines we\ncan look at (otherwise there would be no conflict), the same is not true\nfor line *endings*: the three files in question could all consist of a\nsingle line without any line ending, each. In this case we fall back to\nusing LF-only.\n\nSigned-off-by: Johannes Schindelin \n---\n t/t6023-merge-file.sh | 12 ++++++++++\n xdiff/xmerge.c | 61 +++++++++++++++++++++++++++++++++++++++++++++++----\n 2 files changed, 69 insertions(+), 4 deletions(-)\n\ndiff --git a/t/t6023-merge-file.sh b/t/t6023-merge-file.sh\nindex 190ee90..bb20cbc 100755\n--- a/t/t6023-merge-file.sh\n+++ b/t/t6023-merge-file.sh\n@@ -346,4 +346,16 @@ test_expect_success 'conflict at EOF without LF resolved by --union' \\\n \t printf \"line1\\nline2\\nline3x\\nline3y\" >expect.txt &&\n \t test_cmp expect.txt output.txt'\n \n+test_expect_success 'conflict markers match existing line endings' '\n+\tprintf \"1\\\\r\\\\n2\\\\r\\\\n3\" >crlf-orig.txt &&\n+\tprintf \"1\\\\r\\\\n2\\\\r\\\\n4\" >crlf-diff1.txt &&\n+\tprintf \"1\\\\r\\\\n2\\\\r\\\\n5\" >crlf-diff2.txt &&\n+\ttest_must_fail git -c core.eol=crlf merge-file -p \\\n+\t\tcrlf-diff1.txt crlf-orig.txt crlf-diff2.txt >crlf.txt &&\n+\ttest $(tr \"\\015\" Q nolf.txt &&\n+\ttest $(tr \"\\015\" Q nrec - 1)\n+\t\t/* All lines before the last *must* end in LF */\n+\t\treturn (size = file->recs[i]->size) > 1 &&\n+\t\t\tfile->recs[i]->ptr[size - 2] == '\\r';\n+\tif (!file->nrec)\n+\t\t/* Cannot determine eol style from empty file */\n+\t\treturn -1;\n+\tif ((size = file->recs[i]->size) &&\n+\t\t\tfile->recs[i]->ptr[size - 1] == '\\n')\n+\t\t/* Last line; ends in LF; Is it CR/LF? */\n+\t\treturn size > 1 &&\n+\t\t\tfile->recs[i]->ptr[size - 2] == '\\r';\n+\tif (!i)\n+\t\t/* The only line has no eol */\n+\t\treturn -1;\n+\t/* Determine eol from second-to-last line */\n+\treturn (size = file->recs[i - 1]->size) > 1 &&\n+\t\tfile->recs[i - 1]->ptr[size - 2] == '\\r';\n+}\n+\n+static int is_cr_needed(xdfenv_t *xe1, xdfenv_t *xe2, xdmerge_t *m)\n+{\n+\tint needs_cr;\n+\n+\t/* Match post-images' preceding, or first, lines' end-of-line style */\n+\tneeds_cr = is_eol_crlf(&xe1->xdf2, m->i1 ? m->i1 - 1 : 0);\n+\tif (needs_cr)\n+\t\tneeds_cr = is_eol_crlf(&xe2->xdf2, m->i2 ? m->i2 - 1 : 0);\n+\t/* Look at pre-image's first line, unless we already settled on LF */\n+\tif (needs_cr)\n+\t\tneeds_cr = is_eol_crlf(&xe1->xdf1, 0);\n+\t/* If still undecided, use LF-only */\n+\treturn needs_cr < 0 ? 0 : needs_cr;\n+}\n+\n static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,\n \t\t\t xdfenv_t *xe2, const char *name2,\n \t\t\t const char *name3,\n@@ -152,6 +196,7 @@ static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,\n \tint marker1_size = (name1 ? strlen(name1) + 1 : 0);\n \tint marker2_size = (name2 ? strlen(name2) + 1 : 0);\n \tint marker3_size = (name3 ? strlen(name3) + 1 : 0);\n+\tint needs_cr = is_cr_needed(xe1, xe2, m);\n \n \tif (marker_size <= 0)\n \t\tmarker_size = DEFAULT_CONFLICT_MARKER_SIZE;\n@@ -161,7 +206,7 @@ static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,\n \t\t\t dest ? dest + size : NULL);\n \n \tif (!dest) {\n-\t\tsize += marker_size + 1 + marker1_size;\n+\t\tsize += marker_size + 1 + needs_cr + marker1_size;\n \t} else {\n \t\tmemset(dest + size, '<', marker_size);\n \t\tsize += marker_size;\n@@ -170,6 +215,8 @@ static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,\n \t\t\tmemcpy(dest + size + 1, name1, marker1_size - 1);\n \t\t\tsize += marker1_size;\n \t\t}\n+\t\tif (needs_cr)\n+\t\t\tdest[size++] = '\\r';\n \t\tdest[size++] = '\\n';\n \t}\n \n@@ -180,7 +227,7 @@ static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,\n \tif (style == XDL_MERGE_DIFF3) {\n \t\t/* Shared preimage */\n \t\tif (!dest) {\n-\t\t\tsize += marker_size + 1 + marker3_size;\n+\t\t\tsize += marker_size + 1 + needs_cr + marker3_size;\n \t\t} else {\n \t\t\tmemset(dest + size, '|', marker_size);\n \t\t\tsize += marker_size;\n@@ -189,6 +236,8 @@ static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,\n \t\t\t\tmemcpy(dest + size + 1, name3, marker3_size - 1);\n \t\t\t\tsize += marker3_size;\n \t\t\t}\n+\t\t\tif (needs_cr)\n+\t\t\t\tdest[size++] = '\\r';\n \t\t\tdest[size++] = '\\n';\n \t\t}\n \t\tsize += xdl_orig_copy(xe1, m->i0, m->chg0, 1,\n@@ -196,10 +245,12 @@ static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,\n \t}\n \n \tif (!dest) {\n-\t\tsize += marker_size + 1;\n+\t\tsize += marker_size + 1 + needs_cr;\n \t} else {\n \t\tmemset(dest + size, '=', marker_size);\n \t\tsize += marker_size;\n+\t\tif (needs_cr)\n+\t\t\tdest[size++] = '\\r';\n \t\tdest[size++] = '\\n';\n \t}\n \n@@ -207,7 +258,7 @@ static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,\n \tsize += xdl_recs_copy(xe2, m->i2, m->chg2, 1,\n \t\t\t dest ? dest + size : NULL);\n \tif (!dest) {\n-\t\tsize += marker_size + 1 + marker2_size;\n+\t\tsize += marker_size + 1 + needs_cr + marker2_size;\n \t} else {\n \t\tmemset(dest + size, '>', marker_size);\n \t\tsize += marker_size;\n@@ -216,6 +267,8 @@ static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,\n \t\t\tmemcpy(dest + size + 1, name2, marker2_size - 1);\n \t\t\tsize += marker2_size;\n \t\t}\n+\t\tif (needs_cr)\n+\t\t\tdest[size++] = '\\r';\n \t\tdest[size++] = '\\n';\n \t}\n \treturn size;\n-- \n2.7.0.windows.1.7.g55a05c8\n"},{"id":"276816","messageId":"fc0bbc768ce5cd868b686fcc6c22519c04a189a8.1453819314.git.johannes.schindelin@gmx.de","threadId":"41241","inReplyTo":"cover.1453819314.git.johannes.schindelin@gmx.de","subject":"[PATCH v4 2/2] merge-file: ensure that conflict sections match eol style","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-01-26T14:42:18Z","receivedAt":"2016-01-26T14:42:18Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"In the previous patch, we made sure that the conflict markers themselves\nmatch the end-of-line style of the input files. However, this still left\nout the conflicting text itself: if it lacks a trailing newline, we\nadd one, and should add a carriage return when appropriate, too.\n\nSigned-off-by: Johannes Schindelin \n---\n t/t6023-merge-file.sh | 3 ++-\n xdiff/xmerge.c | 37 +++++++++++++++++++++++--------------\n 2 files changed, 25 insertions(+), 15 deletions(-)\n\ndiff --git a/t/t6023-merge-file.sh b/t/t6023-merge-file.sh\nindex bb20cbc..1390548 100755\n--- a/t/t6023-merge-file.sh\n+++ b/t/t6023-merge-file.sh\n@@ -346,13 +346,14 @@ test_expect_success 'conflict at EOF without LF resolved by --union' \\\n \t printf \"line1\\nline2\\nline3x\\nline3y\" >expect.txt &&\n \t test_cmp expect.txt output.txt'\n \n-test_expect_success 'conflict markers match existing line endings' '\n+test_expect_success 'conflict sections match existing line endings' '\n \tprintf \"1\\\\r\\\\n2\\\\r\\\\n3\" >crlf-orig.txt &&\n \tprintf \"1\\\\r\\\\n2\\\\r\\\\n4\" >crlf-diff1.txt &&\n \tprintf \"1\\\\r\\\\n2\\\\r\\\\n5\" >crlf-diff2.txt &&\n \ttest_must_fail git -c core.eol=crlf merge-file -p \\\n \t\tcrlf-diff1.txt crlf-orig.txt crlf-diff2.txt >crlf.txt &&\n \ttest $(tr \"\\015\" Q nolf.txt &&\n \ttest $(tr \"\\015\" Q size;\n \t\tif (i == 0 || recs[count - 1]->ptr[i - 1] != '\\n') {\n+\t\t\tif (needs_cr) {\n+\t\t\t\tif (dest)\n+\t\t\t\t\tdest[size] = '\\r';\n+\t\t\t\tsize++;\n+\t\t\t}\n+\n \t\t\tif (dest)\n \t\t\t\tdest[size] = '\\n';\n \t\t\tsize++;\n@@ -133,14 +139,14 @@ static int xdl_recs_copy_0(int use_orig, xdfenv_t *xe, int i, int count, int add\n \treturn size;\n }\n \n-static int xdl_recs_copy(xdfenv_t *xe, int i, int count, int add_nl, char *dest)\n+static int xdl_recs_copy(xdfenv_t *xe, int i, int count, int needs_cr, int add_nl, char *dest)\n {\n-\treturn xdl_recs_copy_0(0, xe, i, count, add_nl, dest);\n+\treturn xdl_recs_copy_0(0, xe, i, count, needs_cr, add_nl, dest);\n }\n \n-static int xdl_orig_copy(xdfenv_t *xe, int i, int count, int add_nl, char *dest)\n+static int xdl_orig_copy(xdfenv_t *xe, int i, int count, int needs_cr, int add_nl, char *dest)\n {\n-\treturn xdl_recs_copy_0(1, xe, i, count, add_nl, dest);\n+\treturn xdl_recs_copy_0(1, xe, i, count, needs_cr, add_nl, dest);\n }\n \n /*\n@@ -202,7 +208,7 @@ static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,\n \t\tmarker_size = DEFAULT_CONFLICT_MARKER_SIZE;\n \n \t/* Before conflicting part */\n-\tsize += xdl_recs_copy(xe1, i, m->i1 - i, 0,\n+\tsize += xdl_recs_copy(xe1, i, m->i1 - i, 0, 0,\n \t\t\t dest ? dest + size : NULL);\n \n \tif (!dest) {\n@@ -221,7 +227,7 @@ static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,\n \t}\n \n \t/* Postimage from side #1 */\n-\tsize += xdl_recs_copy(xe1, m->i1, m->chg1, 1,\n+\tsize += xdl_recs_copy(xe1, m->i1, m->chg1, needs_cr, 1,\n \t\t\t dest ? dest + size : NULL);\n \n \tif (style == XDL_MERGE_DIFF3) {\n@@ -240,7 +246,7 @@ static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,\n \t\t\t\tdest[size++] = '\\r';\n \t\t\tdest[size++] = '\\n';\n \t\t}\n-\t\tsize += xdl_orig_copy(xe1, m->i0, m->chg0, 1,\n+\t\tsize += xdl_orig_copy(xe1, m->i0, m->chg0, needs_cr, 1,\n \t\t\t\t dest ? dest + size : NULL);\n \t}\n \n@@ -255,7 +261,7 @@ static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,\n \t}\n \n \t/* Postimage from side #2 */\n-\tsize += xdl_recs_copy(xe2, m->i2, m->chg2, 1,\n+\tsize += xdl_recs_copy(xe2, m->i2, m->chg2, needs_cr, 1,\n \t\t\t dest ? dest + size : NULL);\n \tif (!dest) {\n \t\tsize += marker_size + 1 + needs_cr + marker2_size;\n@@ -294,21 +300,24 @@ static int xdl_fill_merge_buffer(xdfenv_t *xe1, const char *name1,\n \t\t\t\t\t\t marker_size);\n \t\telse if (m->mode & 3) {\n \t\t\t/* Before conflicting part */\n-\t\t\tsize += xdl_recs_copy(xe1, i, m->i1 - i, 0,\n+\t\t\tsize += xdl_recs_copy(xe1, i, m->i1 - i, 0, 0,\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, (m->mode & 2),\n+\t\t\tif (m->mode & 1) {\n+\t\t\t\tint needs_cr = is_cr_needed(xe1, xe2, m);\n+\n+\t\t\t\tsize += xdl_recs_copy(xe1, m->i1, m->chg1, needs_cr, (m->mode & 2),\n \t\t\t\t\t\t dest ? dest + size : NULL);\n+\t\t\t}\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, 0,\n+\t\t\t\tsize += xdl_recs_copy(xe2, m->i2, m->chg2, 0, 0,\n \t\t\t\t\t\t dest ? dest + size : NULL);\n \t\t} else\n \t\t\tcontinue;\n \t\ti = m->i1 + m->chg1;\n \t}\n-\tsize += xdl_recs_copy(xe1, i, xe1->xdf2.nrec - i, 0,\n+\tsize += xdl_recs_copy(xe1, i, xe1->xdf2.nrec - i, 0, 0,\n \t\t\t dest ? dest + size : NULL);\n \treturn size;\n }\n-- \n2.7.0.windows.1.7.g55a05c8\n"},{"id":"276819","messageId":"56A7A240.7000102@ramsayjones.plus.com","threadId":"41241","inReplyTo":"alpine.DEB.2.20.1601260951030.2964@virtualbox","subject":"Re: [PATCH v2 1/1] merge-file: let conflict markers match end-of-line style of the context","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2016-01-26T16:43:44Z","receivedAt":"2016-01-26T16:43:44Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 26/01/16 08:54, Johannes Schindelin wrote:\n> Hi Ramsay,\n[snip]\n\n>>\n>> But you are only testing 3/4 conflict markers end in CR/LF. :-D\n> \n> The fact that ||| markers are present is the fault of previous test cases.\n> I tried to make a point of *not* relying on such a side effect (so as to\n> debug failures quicker by commenting out all previous test cases).\n> \n> So the fact that I am testing only 3 of the 4 conflict markers is very\n> much by design.\n\nI'm sorry, but I don't understand what you are trying to say here. :(\n\n> \n>>> Read: I am uncertain that I want to spend the additional lines on\n>>> testing more than actually necessary.\n>>\n>> If the here doc is too verbose for you, how about something like this\n>> (totally untested):\n>>\n>> test $(tr \"\\015\" Q |].*Q$\" | wc -l) -eq 4\n>>\n>> instead?\n> \n> Hmm. I do not see the benefit over grepping for `txtQ$` it's essentially\n> the same.\n\nWell, as I said 'totally untested', but it _should_ be different. ;-)\n\nThe output from the 'tr \"\\015\" Q >>>>>> crlf-diff2.txtQ\n\nso that your 'grep \"\\\\.txtQ$\"' should select these lines:\n\n <<<<<<< crlf-diff1.txtQ\n ||||||| crlf-orig.txtQ\n >>>>>>> crlf-diff2.txtQ\n\nwhereas my 'grep \"^[<=>|].*Q$\"' should select these lines:\n\n <<<<<<< crlf-diff1.txtQ\n ||||||| crlf-orig.txtQ\n =======Q\n >>>>>>> crlf-diff2.txtQ\n\n(ie. select all 4 conflict markers).\n\n> \n>> HTH\n> \n> How the hell?\n\n:-D Hope That Helps.\n\n> \n>> ATB,\n>> Ramsay Jones\n> \n> Authorization to Buy?\n\nAll The Best.\n\nATB,\nRamsay Jones\n"},{"id":"276820","messageId":"alpine.DEB.2.20.1601261745340.2964@virtualbox","threadId":"41241","inReplyTo":"56A7A240.7000102@ramsayjones.plus.com","subject":"Re: [PATCH v2 1/1] merge-file: let conflict markers match end-of-line style of the context","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-01-26T16:49:26Z","receivedAt":"2016-01-26T16:49:26Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Ramsay,\n\nOn Tue, 26 Jan 2016, Ramsay Jones wrote:\n\n> On 26/01/16 08:54, Johannes Schindelin wrote:\n> > Hi Ramsay,\n> [snip]\n> \n> >>\n> >> But you are only testing 3/4 conflict markers end in CR/LF. :-D\n> > \n> > The fact that ||| markers are present is the fault of previous test cases.\n> > I tried to make a point of *not* relying on such a side effect (so as to\n> > debug failures quicker by commenting out all previous test cases).\n> > \n> > So the fact that I am testing only 3 of the 4 conflict markers is very\n> > much by design.\n> \n> I'm sorry, but I don't understand what you are trying to say here. :(\n\nTypically, there are only 3 conflict markers:\n\n<<<\ndiff 1\n===\ndiff 2\n>>>\n\nHowever, you can tell merge-file to show also the original hunk, in this\ncase it was done by calling `git config merge.conflictstyle diff3` in an\nearlier test, and this implementation detail seeps through to the current\ntest case.\n\nGuess what, if you really only quickly want to run this last test (and\ncomment-out the previous ones), the conflict style is *not* diff3, and you\nend up with the three conflict markers again.\n\n> >>> Read: I am uncertain that I want to spend the additional lines on\n> >>> testing more than actually necessary.\n> >>\n> >> If the here doc is too verbose for you, how about something like this\n> >> (totally untested):\n> >>\n> >> test $(tr \"\\015\" Q |].*Q$\" | wc -l) -eq 4\n> >>\n> >> instead?\n> > \n> > Hmm. I do not see the benefit over grepping for `txtQ$` it's essentially\n> > the same.\n> \n> Well, as I said 'totally untested', but it _should_ be different. ;-)\n> \n> The output from the 'tr \"\\015\" Q something like:\n> \n> 1Q\n> 2Q\n> <<<<<<< crlf-diff1.txtQ\n> 4Q\n> ||||||| crlf-orig.txtQ\n> 3Q\n> =======Q\n> 5Q\n> >>>>>>> crlf-diff2.txtQ\n> \n> so that your 'grep \"\\\\.txtQ$\"' should select these lines:\n> \n> <<<<<<< crlf-diff1.txtQ\n> ||||||| crlf-orig.txtQ\n> >>>>>>> crlf-diff2.txtQ\n> \n> whereas my 'grep \"^[<=>|].*Q$\"' should select these lines:\n> \n> <<<<<<< crlf-diff1.txtQ\n> ||||||| crlf-orig.txtQ\n> =======Q\n> >>>>>>> crlf-diff2.txtQ\n> \n> (ie. select all 4 conflict markers).\n\nTrue, I overlooked that the middle txtQ comes from the ||| line that I\nactually did *not* want to test for (see above).\n\nWill fix.\n\nCiao,\nDscho\n"},{"id":"276821","messageId":"CAPc5daVc9nQ+V5itEKMHOzBiHzEoTw1-aoA+rr2n+X1w-1yCnw@mail.gmail.com","threadId":"41241","inReplyTo":"alpine.DEB.2.20.1601260955320.2964@virtualbox","subject":"Re: [PATCH v3 1/2] merge-file: let conflict markers match end-of-line style of the context","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-01-26T17:22:38Z","receivedAt":"2016-01-26T17:22:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"On Tue, Jan 26, 2016 at 1:04 AM, Johannes Schindelin\n wrote:\n> Not sure. You might end up with a very long line (containing plenty of LF\n> \"characters\") and the conflict marker *at the end* of said line, with a\n> CR/LF after it. I would not call that particularly helpful.\n>\n> Seeing as we really cannot do anything in this case, I thought it would be\n> a good idea to avoid trying (and failing) to be smart here.\n\nNicely and clearly explained; thanks, and I agree with your reasoning.\n"},{"id":"276824","messageId":"xmqqoac8ci8o.fsf@gitster.mtv.corp.google.com","threadId":"41241","inReplyTo":"13379804ad901b53d78d741156677664924fc50d.1453819314.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH v4 1/2] merge-file: let conflict markers match end-of-line style of the context","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-01-26T18:23:35Z","receivedAt":"2016-01-26T18:23:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> +static int is_cr_needed(xdfenv_t *xe1, xdfenv_t *xe2, xdmerge_t *m)\n> +{\n> +\tint needs_cr;\n> +\n> +\t/* Match post-images' preceding, or first, lines' end-of-line style */\n> +\tneeds_cr = is_eol_crlf(&xe1->xdf2, m->i1 ? m->i1 - 1 : 0);\n> +\tif (needs_cr)\n> +\t\tneeds_cr = is_eol_crlf(&xe2->xdf2, m->i2 ? m->i2 - 1 : 0);\n> +\t/* Look at pre-image's first line, unless we already settled on LF */\n> +\tif (needs_cr)\n> +\t\tneeds_cr = is_eol_crlf(&xe1->xdf1, 0);\n> +\t/* If still undecided, use LF-only */\n> +\treturn needs_cr < 0 ? 0 : needs_cr;\n> +}\n\nRetrying with other images when needs_cr is either -1 (unknown) or 1\n(known to be true) was tricky; I had to read it twice and think\nabout it for 30 seconds before convincing myself that the code does\nwhat the log message specifies it should.\n\nThat is probably because I was thinking in terms of \"do we know that\nwe need to add a CR?\"; if I read \"needs_cr\" in my head as \"we might\nwant to add a CR\", everything becomes much more clear, but perhaps\nit is just me.\n\nThe return value of this function is definitely \"do we need and want\nto add a CR\", and it is appropriately named.\n\nThanks.\n"},{"id":"276834","messageId":"xmqqsi1kavr2.fsf@gitster.mtv.corp.google.com","threadId":"41241","inReplyTo":"xmqqoac8ci8o.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v4 1/2] merge-file: let conflict markers match end-of-line style of the context","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-01-26T21:14:41Z","receivedAt":"2016-01-26T21:14:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Johannes Schindelin writes:\n>\n>> +static int is_cr_needed(xdfenv_t *xe1, xdfenv_t *xe2, xdmerge_t *m)\n>> +{\n>> +\tint needs_cr;\n>> +\n>> +\t/* Match post-images' preceding, or first, lines' end-of-line style */\n>> +\tneeds_cr = is_eol_crlf(&xe1->xdf2, m->i1 ? m->i1 - 1 : 0);\n>> +\tif (needs_cr)\n>> +\t\tneeds_cr = is_eol_crlf(&xe2->xdf2, m->i2 ? m->i2 - 1 : 0);\n>> +\t/* Look at pre-image's first line, unless we already settled on LF */\n>> +\tif (needs_cr)\n>> +\t\tneeds_cr = is_eol_crlf(&xe1->xdf1, 0);\n>> +\t/* If still undecided, use LF-only */\n>> +\treturn needs_cr < 0 ? 0 : needs_cr;\n>> +}\n>\n> Retrying with other images when needs_cr is either -1 (unknown) or 1\n> (known to be true) was tricky; I had to read it twice and think\n> about it for 30 seconds before convincing myself that the code does\n> what the log message specifies it should.\n>\n> That is probably because I was thinking in terms of \"do we know that\n> we need to add a CR?\"; if I read \"needs_cr\" in my head as \"we might\n> want to add a CR\", everything becomes much more clear, but perhaps\n> it is just me.\n>\n> The return value of this function is definitely \"do we need and want\n> to add a CR\", and it is appropriately named.\n>\n> Thanks.\n\nJust in case it was unclear, none of the comment above means I want\nany part of the patch redone--I am happy with this patch as-is.\n"},{"id":"276870","messageId":"alpine.DEB.2.20.1601270856150.2964@virtualbox","threadId":"41241","inReplyTo":"xmqqsi1kavr2.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v4 1/2] merge-file: let conflict markers match end-of-line style of the context","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-01-27T07:58:59Z","receivedAt":"2016-01-27T07:58:59Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Tue, 26 Jan 2016, Junio C Hamano wrote:\n\n> Junio C Hamano writes:\n> \n> > Johannes Schindelin writes:\n> >\n> >> +static int is_cr_needed(xdfenv_t *xe1, xdfenv_t *xe2, xdmerge_t *m)\n> >> +{\n> >> +\tint needs_cr;\n> >> +\n> >> +\t/* Match post-images' preceding, or first, lines' end-of-line style */\n> >> +\tneeds_cr = is_eol_crlf(&xe1->xdf2, m->i1 ? m->i1 - 1 : 0);\n> >> +\tif (needs_cr)\n> >> +\t\tneeds_cr = is_eol_crlf(&xe2->xdf2, m->i2 ? m->i2 - 1 : 0);\n> >> +\t/* Look at pre-image's first line, unless we already settled on LF */\n> >> +\tif (needs_cr)\n> >> +\t\tneeds_cr = is_eol_crlf(&xe1->xdf1, 0);\n> >> +\t/* If still undecided, use LF-only */\n> >> +\treturn needs_cr < 0 ? 0 : needs_cr;\n> >> +}\n> >\n> > Retrying with other images when needs_cr is either -1 (unknown) or 1\n> > (known to be true) was tricky; I had to read it twice and think\n> > about it for 30 seconds before convincing myself that the code does\n> > what the log message specifies it should.\n> >\n> > That is probably because I was thinking in terms of \"do we know that\n> > we need to add a CR?\"; if I read \"needs_cr\" in my head as \"we might\n> > want to add a CR\", everything becomes much more clear, but perhaps\n> > it is just me.\n> >\n> > The return value of this function is definitely \"do we need and want\n> > to add a CR\", and it is appropriately named.\n> >\n> > Thanks.\n> \n> Just in case it was unclear, none of the comment above means I want\n> any part of the patch redone--I am happy with this patch as-is.\n\nThanks for saying that... I was about to try to make things clearer, but I\ncould not think of a better term than \"needs_cr\". The existing code\nalready has \"add_nl\" (which means: add a newline *iff* the block to be\ncopied does not yet have a newline), which I also found confusing.\n\nOh wait, maybe \"eol_is_crlf\" instead of \"needs_cr\"?\n\nCiao,\nDscho\n"},{"id":"276918","messageId":"cover.1453912583.git.johannes.schindelin@gmx.de","threadId":"41241","inReplyTo":"cover.1453819314.git.johannes.schindelin@gmx.de","subject":"[PATCH v5 0/2] Let merge-file write out conflict markers with correct EOLs","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-01-27T16:37:25Z","receivedAt":"2016-01-27T16:37:25Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"The original patch was sent by Beat Bolli in\nhttp://thread.gmane.org/gmane.comp.version-control.git/281600\n\nMy suggestion to extend it to respect gitattributes led to\nchanges that broke the original patch. And they were misguided\nto begin with (see below).\n\nSince there have been a couple of \"What's cooking\" mails\ncontaining unheard calls for updates on this patch, I took it\non myself to fix things.\n\nJunio's comment that we might look at blobs containing CR/LF\nline endings made me rethink the entire approach, and I am now\nconvinced that we need to abandon the entire idea to make the\nconflict markers depend on config settings or attributes:\nafter all, I introduced `git merge-file` as a replacement for\nGNU merge that can be used *outside* of any repository, by design.\n\nThe crucial idea hit me yesterday when I took a step back: all\nwe need to do is to ensure that the end-of-line style is matched\nwhen *all* input files are LF-only, or when they are all CR/LF.\nIn all other cases, we have mixed line endings anyway.\n\nAnd to do that, it is sufficient to look at *one single line\nending* in the context. Technically, it does not even have to be\nthe context, but the first line endings of the first file would\nbe enough, however it is so much more pleasant if the conflict\nmarker's eol matches the one of the preceding line.\n\nTo prevent my future self from repeating mistakes, I added a\nlittle bit of background to the first commit message.\n\nTriggered by a comment by Junio, I realized that a second patch is\nneeded: we need to make sure that the conflicting sections are also\naugmented by the appropriate line endings if they lack any.\n\nThe change relative to v4 is that I now test the correct conflict\nmarkers: the merge-file call we add to t6023 actually produces four\nconflict markers because a previous test switched the conflict style\nto `diff3` mode. This is only a side effect, though, therefore I\nreally wanted to avoid testing for it. However, I managed to test\nan *incorrect* subset of three, but this is now fixed. Thanks go\nto Ramsay Jones for poking my nose onto that issue.\n\n\nJohannes Schindelin (2):\n merge-file: let conflict markers match end-of-line style of the\n context\n merge-file: ensure that conflict sections match eol style\n\n t/t6023-merge-file.sh | 13 +++++++\n xdiff/xmerge.c | 98 +++++++++++++++++++++++++++++++++++++++++----------\n 2 files changed, 93 insertions(+), 18 deletions(-)\n\nInterdiff vs v4:\n\n diff --git a/t/t6023-merge-file.sh b/t/t6023-merge-file.sh\n index 1390548..20aee43 100755\n --- a/t/t6023-merge-file.sh\n +++ b/t/t6023-merge-file.sh\n @@ -352,11 +352,11 @@ test_expect_success 'conflict sections match existing line endings' '\n \tprintf \"1\\\\r\\\\n2\\\\r\\\\n5\" >crlf-diff2.txt &&\n \ttest_must_fail git -c core.eol=crlf merge-file -p \\\n \t\tcrlf-diff1.txt crlf-orig.txt crlf-diff2.txt >crlf.txt &&\n -\ttest $(tr \"\\015\" Q ].*Q$\" | wc -l) = 3 &&\n \ttest $(tr \"\\015\" Q nolf.txt &&\n -\ttest $(tr \"\\015\" Q ].*Q$\" | wc -l) = 0\n '\n \n test_done\n\n-- \n2.7.0.windows.1.7.g55a05c8\n"},{"id":"276920","messageId":"8bd9298f9a5b6a6b027625478c9dfcb9188dfb39.1453912583.git.johannes.schindelin@gmx.de","threadId":"41241","inReplyTo":"cover.1453912583.git.johannes.schindelin@gmx.de","subject":"[PATCH v5 1/2] merge-file: let conflict markers match end-of-line style of the context","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-01-27T16:37:36Z","receivedAt":"2016-01-27T16:37:36Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"When merging files with CR/LF line endings, the conflict markers should\nmatch those, lest the output file has mixed line endings.\n\nThis is particularly of interest on Windows, where some editors get\n*really* confused by mixed line endings.\n\nThe original version of this patch by Beat Bolli respected core.eol, and\na subsequent improvement by this developer also respected gitattributes.\nThis approach was suboptimal, though: `git merge-file` was invented as a\ndrop-in replacement for GNU merge and as such has no problem operating\noutside of any repository at all!\n\nAnother problem with the original approach was pointed out by Junio\nHamano: legacy repositories might have their text files committed using\nCR/LF line endings (and core.eol and the gitattributes would give us a\nfalse impression there). Therefore, the much superior approach is to\nsimply match the context's line endings, if any.\n\nWe actually do not have to look at the *entire* context at all: if the\nfiles are all LF-only, or if they all have CR/LF line endings, it is\nsufficient to look at just a *single* line to match that style. And if\nthe line endings are mixed anyway, it is *still* okay to imitate just a\nsingle line's eol: we will just add to the pile of mixed line endings,\nand there is nothing we can do about that.\n\nSo what we do is: we look at the line preceding the conflict, falling\nback to the line preceding that in case it was the last line and had no\nline ending, falling back to the first line, first in the first\npost-image, then the second post-image, and finally the pre-image.\nIf we find consistent CR/LF (or undecided) end-of-line style, we match\nthat, otherwise we use LF-only line endings for the conflict markers.\n\nNote that while it is true that there have to be at least two lines we\ncan look at (otherwise there would be no conflict), the same is not true\nfor line *endings*: the three files in question could all consist of a\nsingle line without any line ending, each. In this case we fall back to\nusing LF-only.\n\nSigned-off-by: Johannes Schindelin \n---\n t/t6023-merge-file.sh | 12 ++++++++++\n xdiff/xmerge.c | 61 +++++++++++++++++++++++++++++++++++++++++++++++----\n 2 files changed, 69 insertions(+), 4 deletions(-)\n\ndiff --git a/t/t6023-merge-file.sh b/t/t6023-merge-file.sh\nindex 190ee90..a082116 100755\n--- a/t/t6023-merge-file.sh\n+++ b/t/t6023-merge-file.sh\n@@ -346,4 +346,16 @@ test_expect_success 'conflict at EOF without LF resolved by --union' \\\n \t printf \"line1\\nline2\\nline3x\\nline3y\" >expect.txt &&\n \t test_cmp expect.txt output.txt'\n \n+test_expect_success 'conflict markers match existing line endings' '\n+\tprintf \"1\\\\r\\\\n2\\\\r\\\\n3\" >crlf-orig.txt &&\n+\tprintf \"1\\\\r\\\\n2\\\\r\\\\n4\" >crlf-diff1.txt &&\n+\tprintf \"1\\\\r\\\\n2\\\\r\\\\n5\" >crlf-diff2.txt &&\n+\ttest_must_fail git -c core.eol=crlf merge-file -p \\\n+\t\tcrlf-diff1.txt crlf-orig.txt crlf-diff2.txt >crlf.txt &&\n+\ttest $(tr \"\\015\" Q ].*Q$\" | wc -l) = 3 &&\n+\ttest_must_fail git -c core.eol=crlf merge-file -p \\\n+\t\tnolf-diff1.txt nolf-orig.txt nolf-diff2.txt >nolf.txt &&\n+\ttest $(tr \"\\015\" Q ].*Q$\" | wc -l) = 0\n+'\n+\n test_done\ndiff --git a/xdiff/xmerge.c b/xdiff/xmerge.c\nindex 625198e..7b21a6b 100644\n--- a/xdiff/xmerge.c\n+++ b/xdiff/xmerge.c\n@@ -143,6 +143,50 @@ static int xdl_orig_copy(xdfenv_t *xe, int i, int count, int add_nl, char *dest)\n \treturn xdl_recs_copy_0(1, xe, i, count, add_nl, dest);\n }\n \n+/*\n+ * Returns 1 if the i'th line ends in CR/LF (if it is the last line and\n+ * has no eol, the preceding line, if any), 0 if it ends in LF-only, and\n+ * -1 if the line ending cannot be determined.\n+ */\n+static int is_eol_crlf(xdfile_t *file, int i)\n+{\n+\tlong size;\n+\n+\tif (i < file->nrec - 1)\n+\t\t/* All lines before the last *must* end in LF */\n+\t\treturn (size = file->recs[i]->size) > 1 &&\n+\t\t\tfile->recs[i]->ptr[size - 2] == '\\r';\n+\tif (!file->nrec)\n+\t\t/* Cannot determine eol style from empty file */\n+\t\treturn -1;\n+\tif ((size = file->recs[i]->size) &&\n+\t\t\tfile->recs[i]->ptr[size - 1] == '\\n')\n+\t\t/* Last line; ends in LF; Is it CR/LF? */\n+\t\treturn size > 1 &&\n+\t\t\tfile->recs[i]->ptr[size - 2] == '\\r';\n+\tif (!i)\n+\t\t/* The only line has no eol */\n+\t\treturn -1;\n+\t/* Determine eol from second-to-last line */\n+\treturn (size = file->recs[i - 1]->size) > 1 &&\n+\t\tfile->recs[i - 1]->ptr[size - 2] == '\\r';\n+}\n+\n+static int is_cr_needed(xdfenv_t *xe1, xdfenv_t *xe2, xdmerge_t *m)\n+{\n+\tint needs_cr;\n+\n+\t/* Match post-images' preceding, or first, lines' end-of-line style */\n+\tneeds_cr = is_eol_crlf(&xe1->xdf2, m->i1 ? m->i1 - 1 : 0);\n+\tif (needs_cr)\n+\t\tneeds_cr = is_eol_crlf(&xe2->xdf2, m->i2 ? m->i2 - 1 : 0);\n+\t/* Look at pre-image's first line, unless we already settled on LF */\n+\tif (needs_cr)\n+\t\tneeds_cr = is_eol_crlf(&xe1->xdf1, 0);\n+\t/* If still undecided, use LF-only */\n+\treturn needs_cr < 0 ? 0 : needs_cr;\n+}\n+\n static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,\n \t\t\t xdfenv_t *xe2, const char *name2,\n \t\t\t const char *name3,\n@@ -152,6 +196,7 @@ static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,\n \tint marker1_size = (name1 ? strlen(name1) + 1 : 0);\n \tint marker2_size = (name2 ? strlen(name2) + 1 : 0);\n \tint marker3_size = (name3 ? strlen(name3) + 1 : 0);\n+\tint needs_cr = is_cr_needed(xe1, xe2, m);\n \n \tif (marker_size <= 0)\n \t\tmarker_size = DEFAULT_CONFLICT_MARKER_SIZE;\n@@ -161,7 +206,7 @@ static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,\n \t\t\t dest ? dest + size : NULL);\n \n \tif (!dest) {\n-\t\tsize += marker_size + 1 + marker1_size;\n+\t\tsize += marker_size + 1 + needs_cr + marker1_size;\n \t} else {\n \t\tmemset(dest + size, '<', marker_size);\n \t\tsize += marker_size;\n@@ -170,6 +215,8 @@ static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,\n \t\t\tmemcpy(dest + size + 1, name1, marker1_size - 1);\n \t\t\tsize += marker1_size;\n \t\t}\n+\t\tif (needs_cr)\n+\t\t\tdest[size++] = '\\r';\n \t\tdest[size++] = '\\n';\n \t}\n \n@@ -180,7 +227,7 @@ static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,\n \tif (style == XDL_MERGE_DIFF3) {\n \t\t/* Shared preimage */\n \t\tif (!dest) {\n-\t\t\tsize += marker_size + 1 + marker3_size;\n+\t\t\tsize += marker_size + 1 + needs_cr + marker3_size;\n \t\t} else {\n \t\t\tmemset(dest + size, '|', marker_size);\n \t\t\tsize += marker_size;\n@@ -189,6 +236,8 @@ static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,\n \t\t\t\tmemcpy(dest + size + 1, name3, marker3_size - 1);\n \t\t\t\tsize += marker3_size;\n \t\t\t}\n+\t\t\tif (needs_cr)\n+\t\t\t\tdest[size++] = '\\r';\n \t\t\tdest[size++] = '\\n';\n \t\t}\n \t\tsize += xdl_orig_copy(xe1, m->i0, m->chg0, 1,\n@@ -196,10 +245,12 @@ static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,\n \t}\n \n \tif (!dest) {\n-\t\tsize += marker_size + 1;\n+\t\tsize += marker_size + 1 + needs_cr;\n \t} else {\n \t\tmemset(dest + size, '=', marker_size);\n \t\tsize += marker_size;\n+\t\tif (needs_cr)\n+\t\t\tdest[size++] = '\\r';\n \t\tdest[size++] = '\\n';\n \t}\n \n@@ -207,7 +258,7 @@ static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,\n \tsize += xdl_recs_copy(xe2, m->i2, m->chg2, 1,\n \t\t\t dest ? dest + size : NULL);\n \tif (!dest) {\n-\t\tsize += marker_size + 1 + marker2_size;\n+\t\tsize += marker_size + 1 + needs_cr + marker2_size;\n \t} else {\n \t\tmemset(dest + size, '>', marker_size);\n \t\tsize += marker_size;\n@@ -216,6 +267,8 @@ static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,\n \t\t\tmemcpy(dest + size + 1, name2, marker2_size - 1);\n \t\t\tsize += marker2_size;\n \t\t}\n+\t\tif (needs_cr)\n+\t\t\tdest[size++] = '\\r';\n \t\tdest[size++] = '\\n';\n \t}\n \treturn size;\n-- \n2.7.0.windows.1.7.g55a05c8\n"},{"id":"276919","messageId":"758de0ea18bad0e830ca298440e67002171251d6.1453912583.git.johannes.schindelin@gmx.de","threadId":"41241","inReplyTo":"cover.1453912583.git.johannes.schindelin@gmx.de","subject":"[PATCH v5 2/2] merge-file: ensure that conflict sections match eol style","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-01-27T16:37:40Z","receivedAt":"2016-01-27T16:37:40Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"In the previous patch, we made sure that the conflict markers themselves\nmatch the end-of-line style of the input files. However, this still left\nout the conflicting text itself: if it lacks a trailing newline, we\nadd one, and should add a carriage return when appropriate, too.\n\nSigned-off-by: Johannes Schindelin \n---\n t/t6023-merge-file.sh | 3 ++-\n xdiff/xmerge.c | 37 +++++++++++++++++++++++--------------\n 2 files changed, 25 insertions(+), 15 deletions(-)\n\ndiff --git a/t/t6023-merge-file.sh b/t/t6023-merge-file.sh\nindex a082116..20aee43 100755\n--- a/t/t6023-merge-file.sh\n+++ b/t/t6023-merge-file.sh\n@@ -346,13 +346,14 @@ test_expect_success 'conflict at EOF without LF resolved by --union' \\\n \t printf \"line1\\nline2\\nline3x\\nline3y\" >expect.txt &&\n \t test_cmp expect.txt output.txt'\n \n-test_expect_success 'conflict markers match existing line endings' '\n+test_expect_success 'conflict sections match existing line endings' '\n \tprintf \"1\\\\r\\\\n2\\\\r\\\\n3\" >crlf-orig.txt &&\n \tprintf \"1\\\\r\\\\n2\\\\r\\\\n4\" >crlf-diff1.txt &&\n \tprintf \"1\\\\r\\\\n2\\\\r\\\\n5\" >crlf-diff2.txt &&\n \ttest_must_fail git -c core.eol=crlf merge-file -p \\\n \t\tcrlf-diff1.txt crlf-orig.txt crlf-diff2.txt >crlf.txt &&\n \ttest $(tr \"\\015\" Q ].*Q$\" | wc -l) = 3 &&\n+\ttest $(tr \"\\015\" Q nolf.txt &&\n \ttest $(tr \"\\015\" Q ].*Q$\" | wc -l) = 0\ndiff --git a/xdiff/xmerge.c b/xdiff/xmerge.c\nindex 7b21a6b..d98f430 100644\n--- a/xdiff/xmerge.c\n+++ b/xdiff/xmerge.c\n@@ -109,7 +109,7 @@ static int xdl_merge_cmp_lines(xdfenv_t *xe1, int i1, xdfenv_t *xe2, int i2,\n \treturn 0;\n }\n \n-static int xdl_recs_copy_0(int use_orig, xdfenv_t *xe, int i, int count, int add_nl, char *dest)\n+static int xdl_recs_copy_0(int use_orig, xdfenv_t *xe, int i, int count, int needs_cr, int add_nl, char *dest)\n {\n \txrecord_t **recs;\n \tint size = 0;\n@@ -125,6 +125,12 @@ static int xdl_recs_copy_0(int use_orig, xdfenv_t *xe, int i, int count, int add\n \tif (add_nl) {\n \t\ti = recs[count - 1]->size;\n \t\tif (i == 0 || recs[count - 1]->ptr[i - 1] != '\\n') {\n+\t\t\tif (needs_cr) {\n+\t\t\t\tif (dest)\n+\t\t\t\t\tdest[size] = '\\r';\n+\t\t\t\tsize++;\n+\t\t\t}\n+\n \t\t\tif (dest)\n \t\t\t\tdest[size] = '\\n';\n \t\t\tsize++;\n@@ -133,14 +139,14 @@ static int xdl_recs_copy_0(int use_orig, xdfenv_t *xe, int i, int count, int add\n \treturn size;\n }\n \n-static int xdl_recs_copy(xdfenv_t *xe, int i, int count, int add_nl, char *dest)\n+static int xdl_recs_copy(xdfenv_t *xe, int i, int count, int needs_cr, int add_nl, char *dest)\n {\n-\treturn xdl_recs_copy_0(0, xe, i, count, add_nl, dest);\n+\treturn xdl_recs_copy_0(0, xe, i, count, needs_cr, add_nl, dest);\n }\n \n-static int xdl_orig_copy(xdfenv_t *xe, int i, int count, int add_nl, char *dest)\n+static int xdl_orig_copy(xdfenv_t *xe, int i, int count, int needs_cr, int add_nl, char *dest)\n {\n-\treturn xdl_recs_copy_0(1, xe, i, count, add_nl, dest);\n+\treturn xdl_recs_copy_0(1, xe, i, count, needs_cr, add_nl, dest);\n }\n \n /*\n@@ -202,7 +208,7 @@ static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,\n \t\tmarker_size = DEFAULT_CONFLICT_MARKER_SIZE;\n \n \t/* Before conflicting part */\n-\tsize += xdl_recs_copy(xe1, i, m->i1 - i, 0,\n+\tsize += xdl_recs_copy(xe1, i, m->i1 - i, 0, 0,\n \t\t\t dest ? dest + size : NULL);\n \n \tif (!dest) {\n@@ -221,7 +227,7 @@ static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,\n \t}\n \n \t/* Postimage from side #1 */\n-\tsize += xdl_recs_copy(xe1, m->i1, m->chg1, 1,\n+\tsize += xdl_recs_copy(xe1, m->i1, m->chg1, needs_cr, 1,\n \t\t\t dest ? dest + size : NULL);\n \n \tif (style == XDL_MERGE_DIFF3) {\n@@ -240,7 +246,7 @@ static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,\n \t\t\t\tdest[size++] = '\\r';\n \t\t\tdest[size++] = '\\n';\n \t\t}\n-\t\tsize += xdl_orig_copy(xe1, m->i0, m->chg0, 1,\n+\t\tsize += xdl_orig_copy(xe1, m->i0, m->chg0, needs_cr, 1,\n \t\t\t\t dest ? dest + size : NULL);\n \t}\n \n@@ -255,7 +261,7 @@ static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,\n \t}\n \n \t/* Postimage from side #2 */\n-\tsize += xdl_recs_copy(xe2, m->i2, m->chg2, 1,\n+\tsize += xdl_recs_copy(xe2, m->i2, m->chg2, needs_cr, 1,\n \t\t\t dest ? dest + size : NULL);\n \tif (!dest) {\n \t\tsize += marker_size + 1 + needs_cr + marker2_size;\n@@ -294,21 +300,24 @@ static int xdl_fill_merge_buffer(xdfenv_t *xe1, const char *name1,\n \t\t\t\t\t\t marker_size);\n \t\telse if (m->mode & 3) {\n \t\t\t/* Before conflicting part */\n-\t\t\tsize += xdl_recs_copy(xe1, i, m->i1 - i, 0,\n+\t\t\tsize += xdl_recs_copy(xe1, i, m->i1 - i, 0, 0,\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, (m->mode & 2),\n+\t\t\tif (m->mode & 1) {\n+\t\t\t\tint needs_cr = is_cr_needed(xe1, xe2, m);\n+\n+\t\t\t\tsize += xdl_recs_copy(xe1, m->i1, m->chg1, needs_cr, (m->mode & 2),\n \t\t\t\t\t\t dest ? dest + size : NULL);\n+\t\t\t}\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, 0,\n+\t\t\t\tsize += xdl_recs_copy(xe2, m->i2, m->chg2, 0, 0,\n \t\t\t\t\t\t dest ? dest + size : NULL);\n \t\t} else\n \t\t\tcontinue;\n \t\ti = m->i1 + m->chg1;\n \t}\n-\tsize += xdl_recs_copy(xe1, i, xe1->xdf2.nrec - i, 0,\n+\tsize += xdl_recs_copy(xe1, i, xe1->xdf2.nrec - i, 0, 0,\n \t\t\t dest ? dest + size : NULL);\n \treturn size;\n }\n-- \n2.7.0.windows.1.7.g55a05c8\n"},{"id":"276923","messageId":"xmqqio2e996c.fsf@gitster.mtv.corp.google.com","threadId":"41241","inReplyTo":"alpine.DEB.2.20.1601270856150.2964@virtualbox","subject":"Re: [PATCH v4 1/2] merge-file: let conflict markers match end-of-line style of the context","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-01-27T18:19:55Z","receivedAt":"2016-01-27T18:19:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n>> Just in case it was unclear, none of the comment above means I want\n>> any part of the patch redone--I am happy with this patch as-is.\n>\n> Thanks for saying that... I was about to try to make things clearer, but I\n> could not think of a better term than \"needs_cr\".\n\nI don't, either ;-).\n\nThe primary reason I respond with the \"I find this a bit confusing\nbut it probably is just me\" (not just to this patch) is to give an\nexample of a review comment that demonstrates to the others that the\nreviewer understood what is in the patch and the issues around the\nchange better than a mere unsubstantiated \"These look OK to me.\",\nwhich does not tell us how carefully the proposed change was\nreviewed by the reviewer--such a review does not allow me to \"trust\nthe review that is already done by others\" and apply the patches\nwith minimum cursory scanning and I end up having to carefully read\nthem myself.\n"},{"id":"276925","messageId":"alpine.DEB.2.20.1601272011570.2964@virtualbox","threadId":"41241","inReplyTo":"xmqqio2e996c.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v4 1/2] merge-file: let conflict markers match end-of-line style of the context","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-01-27T19:12:38Z","receivedAt":"2016-01-27T19:12:38Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Wed, 27 Jan 2016, Junio C Hamano wrote:\n\n> Johannes Schindelin writes:\n> \n> >> Just in case it was unclear, none of the comment above means I want\n> >> any part of the patch redone--I am happy with this patch as-is.\n> >\n> > Thanks for saying that... I was about to try to make things clearer, but I\n> > could not think of a better term than \"needs_cr\".\n> \n> I don't, either ;-).\n> \n> The primary reason I respond with the \"I find this a bit confusing\n> but it probably is just me\" (not just to this patch) is to give an\n> example of a review comment that demonstrates to the others that the\n> reviewer understood what is in the patch and the issues around the\n> change better than a mere unsubstantiated \"These look OK to me.\",\n> which does not tell us how carefully the proposed change was\n> reviewed by the reviewer--such a review does not allow me to \"trust\n> the review that is already done by others\" and apply the patches\n> with minimum cursory scanning and I end up having to carefully read\n> them myself.\n\nYour response is also an indicator to me that future myself will find the\nsame code just as confusing as you did, though.\n\nMaybe need_cr -> eol_is_crlf?\n\nCiao,\nDscho\n"},{"id":"276927","messageId":"xmqq8u3a95st.fsf@gitster.mtv.corp.google.com","threadId":"41241","inReplyTo":"alpine.DEB.2.20.1601272011570.2964@virtualbox","subject":"Re: [PATCH v4 1/2] merge-file: let conflict markers match end-of-line style of the context","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-01-27T19:32:50Z","receivedAt":"2016-01-27T19:32:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n[administrivia: I finally managed to correct e-mail address of peff@,\nwhich has been giving me bounces for all the previous messages in\nthe thread]\n\n> Your response is also an indicator to me that future myself will find the\n> same code just as confusing as you did, though.\n>\n> Maybe need_cr -> eol_is_crlf?\n\nThe crucial question is how well the variable conveys its meaning\nwhen it has -1 as its value. \"need_cr? -- I don't know yet\" is as\nclear as \"eol_is_crlf? -- I don't know yet\", if not clearer. I\npersonally do not think \"eol_is_crlf\" is an improvement. It makes\nit unclear _whose_ eol the variable is talking about. It can be\nmisread as talking about one or more of the files that are being\nmerged and mistaken as a statement of a fact (i.e. \"we inspected\nand know the input file's eol is CRLF\").\n\nCompared to that, it is clear that \"need_cr\" is talking about what\nthe EOL convention the resulting file should be.\n\nI briefly wondered if the if/if (need_cr)/... cascade that inspects\n(conditionally up to) three variants might become cleaner if the\npolarity of that variable is flipped (we are allowed to use CRLF\nonly when we know that among all of the three variants that we can\ndetermine the line termination convention used, all of them use\nCRLF), but I didn't think it through seriously.\n"},{"id":"276930","messageId":"xmqq4mdy93io.fsf@gitster.mtv.corp.google.com","threadId":"41241","inReplyTo":"cover.1453912583.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH v5 0/2] Let merge-file write out conflict markers with correct EOLs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-01-27T20:22:07Z","receivedAt":"2016-01-27T20:22:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thanks, will queue after looking at the interdiff since v4.\n"},{"id":"276959","messageId":"alpine.DEB.2.20.1601280851470.2964@virtualbox","threadId":"41241","inReplyTo":"xmqq8u3a95st.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v4 1/2] merge-file: let conflict markers match end-of-line style of the context","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-01-28T07:55:21Z","receivedAt":"2016-01-28T07:55:21Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Wed, 27 Jan 2016, Junio C Hamano wrote:\n\n> Johannes Schindelin writes:\n> \n> [administrivia: I finally managed to correct e-mail address of peff@,\n> which has been giving me bounces for all the previous messages in\n> the thread]\n\nOops! I got those bounces myself, but wrote them off to Eric's account\nwithout checking (because I consistently had problems sending emails to\nhim in the past). Sorry about that.\n\n> > Your response is also an indicator to me that future myself will find the\n> > same code just as confusing as you did, though.\n> >\n> > Maybe need_cr -> eol_is_crlf?\n> \n> The crucial question is how well the variable conveys its meaning\n> when it has -1 as its value. \"need_cr? -- I don't know yet\" is as\n> clear as \"eol_is_crlf? -- I don't know yet\", if not clearer. I\n> personally do not think \"eol_is_crlf\" is an improvement.\n\nOkay, then. Let's keep the \"needs_cr\" name.\n\nCiao,\nDscho\n> it unclear _whose_ eol the variable is talking about. It can be\n> misread as talking about one or more of the files that are being\n> merged and mistaken as a statement of a fact (i.e. \"we inspected\n> and know the input file's eol is CRLF\").\n> \n> Compared to that, it is clear that \"need_cr\" is talking about what\n> the EOL convention the resulting file should be.\n> \n> I briefly wondered if the if/if (need_cr)/... cascade that inspects\n> (conditionally up to) three variants might become cleaner if the\n> polarity of that variable is flipped (we are allowed to use CRLF\n> only when we know that among all of the three variants that we can\n> determine the line termination convention used, all of them use\n> CRLF), but I didn't think it through seriously.\n> \n> \n> \n> \n> --\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at http://vger.kernel.org/majordomo-info.html\n> \n"}]}