{"thread":{"id":"40862","subject":"[PATCH] merge-file: consider core.crlf when writing merge markers","startedAt":"2015-11-23T21:32:12Z","lastAt":"2015-11-25T11:09:17Z","messageCount":5,"participants":["Beat Bolli","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"273623","messageId":"1448314332-15581-1-git-send-email-dev+git@drbeat.li","threadId":"40862","inReplyTo":null,"subject":"[PATCH] merge-file: consider core.crlf when writing merge markers","fromName":"Beat Bolli","fromEmail":"dev+git@drbeat.li","sentAt":"2015-11-23T21:32:12Z","receivedAt":"2015-11-23T21:32:12Z","isPatch":true,"sender":{"key":"dev+git@drbeat.li","avatar":"https://avatars.githubusercontent.com/u/21444?v=4"},"body":"When 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 Git's behavior under Windows.\n\nSigned-off-by: Beat Bolli <dev+git@drbeat.li>\nCc: Johannes Schindelin <johannes.schindelin@gmx.de>\n---\n builtin/merge-file.c  |  1 +\n ll-merge.c            |  1 +\n t/t6023-merge-file.sh |  7 +++++++\n xdiff/xdiff.h         |  1 +\n xdiff/xmerge.c        | 29 +++++++++++++++++++----------\n 5 files changed, 29 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/merge-file.c b/builtin/merge-file.c\nindex 5544705..c534c91 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 = (core_eol == EOL_CRLF);\n \txmp.ancestor = names[1];\n \txmp.file1 = names[0];\n \txmp.file2 = names[2];\ndiff --git a/ll-merge.c b/ll-merge.c\nindex 0338630..a548a29 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 *drv_unused,\n \t\txmp.style = git_xmerge_style;\n \tif (marker_size > 0)\n \t\txmp.marker_size = marker_size;\n+\txmp.crlf = (core_eol == EOL_CRLF);\n \txmp.ancestor = orig_name;\n \txmp.file1 = name1;\n \txmp.file2 = name2;\ndiff --git a/t/t6023-merge-file.sh b/t/t6023-merge-file.sh\nindex 190ee90..245359a 100755\n--- a/t/t6023-merge-file.sh\n+++ b/t/t6023-merge-file.sh\n@@ -346,4 +346,11 @@ 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+\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_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.6.2.440.g063f640\n"},{"id":"273635","messageId":"alpine.DEB.1.00.1511240844380.1686@s15462909.onlinehome-server.info","threadId":"40862","inReplyTo":"1448314332-15581-1-git-send-email-dev+git@drbeat.li","subject":"Re: [PATCH] merge-file: consider core.crlf when writing merge markers","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2015-11-24T08:21:56Z","receivedAt":"2015-11-24T08:21:56Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Beat,\n\nOn Mon, 23 Nov 2015, Beat Bolli wrote:\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\nWow, what a beautiful contribution!\n\nI wonder how difficult it would be to make this work with gitattributes,\ni.e. when .gitattributes' `eol` setting disagrees with core.eol.\n\nI imagine that we could use convert.c to do all the hard work, e.g. by\nadding a function\n\n\tconst char *eol_for_path(const char *path, const char *contents)\n\t{\n\t\tenum eol eol;\n\t\tstruct conv_attrs ca;\n\t\tstruct text_stat stats;\n\n\t\tconvert_attrs(&ca, path);\n\t\teol = output_eol(ca.crlf_action);\n\t\tif (eol != EOL_CRLF)\n\t\t\teol = EOL_LF;\n\t\telse if (!*contents || (crlf_action != CRLF_AUTO &&\n\t\t\t\tcrlf_action != CRLF_GUESS)\n\t\t\teol = EOL_CRLF;\n\t\telse {\n\t\t\tca.crlf_action = input_crlf_action(ca.crlf_action,\n\t\t\t\t\tca.eol_attr);\n\t\t\tif (crlf_action == CRLF_GUESS && stats.cr > stats.crlf)\n\t\t\t\teol = core_eol;\n\t\t\telse if (stats.crlf)\n\t\t\t\teol = EOL_CRLF;\n\t\t\telse\n\t\t\t\teol = EOL_LF;\n\t\t}\n\n\t\treturn eol == EOL_CRLF ? \"\\r\\n\" : \"\\n\";\n\t}\n\nOf course you could also make that function return `enum eol` instead of\n`const char *`; that would probably make the changes to xdiff/xmerge.c\nless invasive (and closer to what you have already).\n\nCiao,\nDscho\n"},{"id":"273667","messageId":"5654E7FD.2060000@drbeat.li","threadId":"40862","inReplyTo":"alpine.DEB.1.00.1511240844380.1686@s15462909.onlinehome-server.info","subject":"Re: [PATCH] merge-file: consider core.crlf when writing merge markers","fromName":"Beat Bolli","fromEmail":"dev+git@drbeat.li","sentAt":"2015-11-24T22:43:09Z","receivedAt":"2015-11-24T22:43:09Z","isPatch":true,"sender":{"key":"dev+git@drbeat.li","avatar":"https://avatars.githubusercontent.com/u/21444?v=4"},"body":"On 24.11.15 09:21, Johannes Schindelin wrote:\n> Hi Beat,\n> \n> On Mon, 23 Nov 2015, Beat Bolli wrote:\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> Wow, what a beautiful contribution!\n> \n> I wonder how difficult it would be to make this work with gitattributes,\n> i.e. when .gitattributes' `eol` setting disagrees with core.eol.\n> \n> I imagine that we could use convert.c to do all the hard work, e.g. by\n> adding a function\n> \n> \tconst char *eol_for_path(const char *path, const char *contents)\n> \t{\n> \t\tenum eol eol;\n> \t\tstruct conv_attrs ca;\n> \t\tstruct text_stat stats;\n> \n> \t\tconvert_attrs(&ca, path);\n> \t\teol = output_eol(ca.crlf_action);\n> \t\tif (eol != EOL_CRLF)\n> \t\t\teol = EOL_LF;\n> \t\telse if (!*contents || (crlf_action != CRLF_AUTO &&\n> \t\t\t\tcrlf_action != CRLF_GUESS)\n> \t\t\teol = EOL_CRLF;\n> \t\telse {\n> \t\t\tca.crlf_action = input_crlf_action(ca.crlf_action,\n> \t\t\t\t\tca.eol_attr);\n> \t\t\tif (crlf_action == CRLF_GUESS && stats.cr > stats.crlf)\n> \t\t\t\teol = core_eol;\n> \t\t\telse if (stats.crlf)\n> \t\t\t\teol = EOL_CRLF;\n> \t\t\telse\n> \t\t\t\teol = EOL_LF;\n> \t\t}\n> \n> \t\treturn eol == EOL_CRLF ? \"\\r\\n\" : \"\\n\";\n> \t}\n\nHi Johannes,\n\nI have implemented this according to your algorithm. Now, I have to set\ncore.autocrlf to true for making the new test pass. Setting core.eol no\nlonger has an effect on the merge markers. Is this expected? (I haven't\nset any attributes)\n\nClearly I don't have much experience with gitattributes and how they\ninteract with the core config settings :-\\\n\nRegards,\nBeat\n"},{"id":"273668","messageId":"5654E9B6.3090801@drbeat.li","threadId":"40862","inReplyTo":"5654E7FD.2060000@drbeat.li","subject":"Re: [PATCH] merge-file: consider core.crlf when writing merge markers","fromName":"Beat Bolli","fromEmail":"dev+git@drbeat.li","sentAt":"2015-11-24T22:50:30Z","receivedAt":"2015-11-24T22:50:30Z","isPatch":true,"sender":{"key":"dev+git@drbeat.li","avatar":"https://avatars.githubusercontent.com/u/21444?v=4"},"body":"On 24.11.15 23:43, Beat Bolli wrote:\n> On 24.11.15 09:21, Johannes Schindelin wrote:\n>> Hi Beat,\n>>\n>> On Mon, 23 Nov 2015, Beat Bolli wrote:\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>> Wow, what a beautiful contribution!\n>>\n>> I wonder how difficult it would be to make this work with gitattributes,\n>> i.e. when .gitattributes' `eol` setting disagrees with core.eol.\n>>\n>> I imagine that we could use convert.c to do all the hard work, e.g. by\n>> adding a function\n>>\n>> \tconst char *eol_for_path(const char *path, const char *contents)\n>> \t{\n>> \t\tenum eol eol;\n>> \t\tstruct conv_attrs ca;\n>> \t\tstruct text_stat stats;\n>>\n>> \t\tconvert_attrs(&ca, path);\n>> \t\teol = output_eol(ca.crlf_action);\n>> \t\tif (eol != EOL_CRLF)\n>> \t\t\teol = EOL_LF;\n>> \t\telse if (!*contents || (crlf_action != CRLF_AUTO &&\n>> \t\t\t\tcrlf_action != CRLF_GUESS)\n>> \t\t\teol = EOL_CRLF;\n>> \t\telse {\n>> \t\t\tca.crlf_action = input_crlf_action(ca.crlf_action,\n>> \t\t\t\t\tca.eol_attr);\n>> \t\t\tif (crlf_action == CRLF_GUESS && stats.cr > stats.crlf)\n>> \t\t\t\teol = core_eol;\n>> \t\t\telse if (stats.crlf)\n>> \t\t\t\teol = EOL_CRLF;\n>> \t\t\telse\n>> \t\t\t\teol = EOL_LF;\n>> \t\t}\n>>\n>> \t\treturn eol == EOL_CRLF ? \"\\r\\n\" : \"\\n\";\n>> \t}\n> \n> Hi Johannes,\n> \n> I have implemented this according to your algorithm. Now, I have to set\n> core.autocrlf to true for making the new test pass. Setting core.eol no\n> longer has an effect on the merge markers. Is this expected? (I haven't\n> set any attributes)\n\nPS: the function looks like this now:\n\nenum eol eol_for_path(const char *path, const char *src, size_t len)\n{\n        struct conv_attrs ca;\n        struct text_stat stats;\n\n        convert_attrs(&ca, path);\n        if (output_eol(ca.crlf_action) != EOL_CRLF)\n                return EOL_LF;\n        if (!len || ca.crlf_action != CRLF_AUTO &&\n\t            ca.crlf_action != CRLF_GUESS)\n                return EOL_CRLF;\n        ca.crlf_action = input_crlf_action(ca.crlf_action, ca.eol_attr);\n        gather_stats(src, len, &stats);\n        if (ca.crlf_action == CRLF_GUESS && stats.cr > stats.crlf)\n                return core_eol;\n        else if (stats.crlf)\n                return EOL_CRLF;\n        else\n                return EOL_LF;\n}\n"},{"id":"273724","messageId":"alpine.DEB.1.00.1511251205430.1686@s15462909.onlinehome-server.info","threadId":"40862","inReplyTo":"5654E9B6.3090801@drbeat.li","subject":"Re: [PATCH] merge-file: consider core.crlf when writing merge markers","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2015-11-25T11:09:17Z","receivedAt":"2015-11-25T11:09:17Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Beat,\n\nOn Tue, 24 Nov 2015, Beat Bolli wrote:\n\n> On 24.11.15 23:43, Beat Bolli wrote:\n> > On 24.11.15 09:21, Johannes Schindelin wrote:\n> >>\n> >> On Mon, 23 Nov 2015, Beat Bolli wrote:\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\n> >>> merge\" is reported as simply \"binary file matches\".\n> >>\n> >> Wow, what a beautiful contribution!\n> >>\n> >> I wonder how difficult it would be to make this work with\n> >> gitattributes, i.e. when .gitattributes' `eol` setting disagrees with\n> >> core.eol.\n> > \n> > I have implemented this according to your algorithm. Now, I have to\n> > set core.autocrlf to true for making the new test pass. Setting\n> > core.eol no longer has an effect on the merge markers. Is this\n> > expected? (I haven't set any attributes)\n> \n\nNo, this is not expected. I guess that I was a bit careless in my\nsuggestion (it was just a sketch, after all, not a full implementation):\n\n> enum eol eol_for_path(const char *path, const char *src, size_t len)\n> {\n>         struct conv_attrs ca;\n>         struct text_stat stats;\n> \n>         convert_attrs(&ca, path);\n>         if (output_eol(ca.crlf_action) != EOL_CRLF)\n>                 return EOL_LF;\n\nAt this point, output_eol(ca.crlf_action) would be EOL_UNSET, I guess, in\nwhich case we would have to consult core_eol, of course. So it is not\nenough to test for EOL_CRLF but we really need to test for EOL_CRLF,\nEOL_LF and EOL_UNSET explicitly (I would use a switch() statement and use\na default: rule instead of handling EOL_UNSET specifically).\n\nCiao,\nDscho\n"}]}