{"thread":{"id":"54972","subject":"[PATCH] fixup! mergetool: add automerge configuration","startedAt":"2021-01-09T21:50:12Z","lastAt":"2021-01-29T00:42:40Z","messageCount":24,"participants":["David Aguilar","brian m. carlson","Seth House","Junio C Hamano","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"413928","messageId":"20210109214922.33972-1-davvid@gmail.com","threadId":"54972","inReplyTo":null,"subject":"[PATCH] fixup! mergetool: add automerge configuration","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2021-01-09T21:49:22Z","receivedAt":"2021-01-09T21:50:12Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"The use of \"\\r\\?\" in sed is not portable to macOS and possibly\nother unix flavors.\n\nReplace \"\\r\" with a substituted variable that contains \"\\r\".\nReplace \"\\?\" with \"\\{0,1\\}\".\n\nSigned-off-by: David Aguilar \n---\nThis is based on top of fc/mergetool-automerge and can be squashed in\n(with the addition of my sign-off) if desired.\n\nLet me know if you'd prefer a separate patch. I figured we'd want\na squash to preserve bisectability.\n\n git-mergetool.sh | 13 ++++++++++---\n 1 file changed, 10 insertions(+), 3 deletions(-)\n\ndiff --git a/git-mergetool.sh b/git-mergetool.sh\nindex a44afd3822..12c3e83aa7 100755\n--- a/git-mergetool.sh\n+++ b/git-mergetool.sh\n@@ -243,9 +243,16 @@ auto_merge () {\n \tgit merge-file --diff3 --marker-size=7 -q -p \"$LOCAL\" \"$BASE\" \"$REMOTE\" >\"$DIFF3\"\n \tif test -s \"$DIFF3\"\n \tthen\n-\t\tsed -e '/^<<<<<<< /,/^||||||| /d' -e '/^=======\\r\\?$/,/^>>>>>>> /d' \"$DIFF3\" >\"$BASE\"\n-\t\tsed -e '/^||||||| /,/^>>>>>>> /d' -e '/^<<<<<<< /d' \"$DIFF3\" >\"$LOCAL\"\n-\t\tsed -e '/^<<<<<<< /,/^=======\\r\\?$/d' -e '/^>>>>>>> /d' \"$DIFF3\" >\"$REMOTE\"\n+\t\tcr=$(printf '\\x0d')\n+\t\tsed -e '/^<<<<<<< /,/^||||||| /d' \\\n+\t\t\t-e \"/^=======$cr\\{0,1\\}$/,/^>>>>>>> /d\" \\\n+\t\t\t\"$DIFF3\" >\"$BASE\"\n+\t\tsed -e '/^||||||| /,/^>>>>>>> /d' \\\n+\t\t\t-e '/^<<<<<<< /d' \\\n+\t\t\t\"$DIFF3\" >\"$LOCAL\"\n+\t\tsed -e \"/^<<<<<<< /,/^=======$cr\\{0,1\\}$/d\" \\\n+\t\t\t-e '/^>>>>>>> /d' \\\n+\t\t\t\"$DIFF3\" >\"$REMOTE\"\n \tfi\n \trm -- \"$DIFF3\"\n }\n-- \n2.30.0.rc1.6.g2bc636d1d6\n\n"},{"id":"413929","messageId":"X/onP6vFAHH8SUBo@camp.crustytoothpaste.net","threadId":"54972","inReplyTo":"20210109214922.33972-1-davvid@gmail.com","subject":"Re: [PATCH] fixup! mergetool: add automerge configuration","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2021-01-09T21:59:27Z","receivedAt":"2021-01-09T22:00:29Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2021-01-09 at 21:49:22, David Aguilar wrote:\n> The use of \"\\r\\?\" in sed is not portable to macOS and possibly\n> other unix flavors.\n> \n> Replace \"\\r\" with a substituted variable that contains \"\\r\".\n> Replace \"\\?\" with \"\\{0,1\\}\".\n\nAh, yes, this is true. The statement about \"\\r\" is also true for Linux\nwith POSIXLY_CORRECT, IIRC.\n\n> Signed-off-by: David Aguilar \n> ---\n> This is based on top of fc/mergetool-automerge and can be squashed in\n> (with the addition of my sign-off) if desired.\n> \n> Let me know if you'd prefer a separate patch. I figured we'd want\n> a squash to preserve bisectability.\n> \n> git-mergetool.sh | 13 ++++++++++---\n> 1 file changed, 10 insertions(+), 3 deletions(-)\n> \n> diff --git a/git-mergetool.sh b/git-mergetool.sh\n> index a44afd3822..12c3e83aa7 100755\n> --- a/git-mergetool.sh\n> +++ b/git-mergetool.sh\n> @@ -243,9 +243,16 @@ auto_merge () {\n> \tgit merge-file --diff3 --marker-size=7 -q -p \"$LOCAL\" \"$BASE\" \"$REMOTE\" >\"$DIFF3\"\n> \tif test -s \"$DIFF3\"\n> \tthen\n> -\t\tsed -e '/^<<<<<<< /,/^||||||| /d' -e '/^=======\\r\\?$/,/^>>>>>>> /d' \"$DIFF3\" >\"$BASE\"\n> -\t\tsed -e '/^||||||| /,/^>>>>>>> /d' -e '/^<<<<<<< /d' \"$DIFF3\" >\"$LOCAL\"\n> -\t\tsed -e '/^<<<<<<< /,/^=======\\r\\?$/d' -e '/^>>>>>>> /d' \"$DIFF3\" >\"$REMOTE\"\n> +\t\tcr=$(printf '\\x0d')\n\nUnfortunately, printf is not specified by POSIX to take hex escapes, so\nthis, too is nonportable. We are unfortunately allowed to use only\noctal escapes (yuck). However, we can write this:\n\n cr=$(printf '\\r')\n\nor\n\n cr=$(printf '\\015')\n\nI think the former is clearer, since that's what we were writing before.\n-- \nbrian m. carlson (he/him or they/them)\nHouston, Texas, US\n"},{"id":"413938","messageId":"20210109224236.50363-1-davvid@gmail.com","threadId":"54972","inReplyTo":"X/onP6vFAHH8SUBo@camp.crustytoothpaste.net","subject":"[PATCH v2] fixup! mergetool: add automerge configuration","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2021-01-09T22:42:36Z","receivedAt":"2021-01-09T22:43:41Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"\"\\r\\?\" in sed is not portable to macOS and possibly others.\n\"\\r\" is not valid on Linux with POSIXLY_CORRECT.\n\nReplace \"\\r\" with a substituted variable that contains \"\\r\".\nReplace \"\\?\" with \"\\{0,1\\}\".\n\nHelped-by: brian m. carlson \nSigned-off-by: David Aguilar \n---\nThis is based on top of fc/mergetool-automerge and can be squashed in\n(with the addition of my sign-off) if desired.\n\nChanges since last time:\n- printf '\\r' instead of printf '\\x0d'\n- The commit message now mentions POSIXLY_CORRECT.\n\n git-mergetool.sh | 13 ++++++++++---\n 1 file changed, 10 insertions(+), 3 deletions(-)\n\ndiff --git a/git-mergetool.sh b/git-mergetool.sh\nindex a44afd3822..9029a79431 100755\n--- a/git-mergetool.sh\n+++ b/git-mergetool.sh\n@@ -243,9 +243,16 @@ auto_merge () {\n \tgit merge-file --diff3 --marker-size=7 -q -p \"$LOCAL\" \"$BASE\" \"$REMOTE\" >\"$DIFF3\"\n \tif test -s \"$DIFF3\"\n \tthen\n-\t\tsed -e '/^<<<<<<< /,/^||||||| /d' -e '/^=======\\r\\?$/,/^>>>>>>> /d' \"$DIFF3\" >\"$BASE\"\n-\t\tsed -e '/^||||||| /,/^>>>>>>> /d' -e '/^<<<<<<< /d' \"$DIFF3\" >\"$LOCAL\"\n-\t\tsed -e '/^<<<<<<< /,/^=======\\r\\?$/d' -e '/^>>>>>>> /d' \"$DIFF3\" >\"$REMOTE\"\n+\t\tcr=$(printf '\\r')\n+\t\tsed -e '/^<<<<<<< /,/^||||||| /d' \\\n+\t\t\t-e \"/^=======$cr\\{0,1\\}$/,/^>>>>>>> /d\" \\\n+\t\t\t\"$DIFF3\" >\"$BASE\"\n+\t\tsed -e '/^||||||| /,/^>>>>>>> /d' \\\n+\t\t\t-e '/^<<<<<<< /d' \\\n+\t\t\t\"$DIFF3\" >\"$LOCAL\"\n+\t\tsed -e \"/^<<<<<<< /,/^=======$cr\\{0,1\\}$/d\" \\\n+\t\t\t-e '/^>>>>>>> /d' \\\n+\t\t\t\"$DIFF3\" >\"$REMOTE\"\n \tfi\n \trm -- \"$DIFF3\"\n }\n-- \n2.30.0.rc1.6.g3f22546b2b.dirty\n\n"},{"id":"413941","messageId":"20210109225400.GA156779@ellen","threadId":"54972","inReplyTo":"20210109224236.50363-1-davvid@gmail.com","subject":"Re: [PATCH v2] fixup! mergetool: add automerge configuration","fromName":"Seth House","fromEmail":"seth@eseth.com","sentAt":"2021-01-09T22:54:00Z","receivedAt":"2021-01-09T22:58:08Z","isPatch":true,"sender":{"key":"seth@eseth.com","avatar":"https://avatars.githubusercontent.com/u/91293?v=4"},"body":"On Sat, Jan 09, 2021 at 02:42:36PM -0800, David Aguilar wrote:\n> Replace \"\\r\" with a substituted variable that contains \"\\r\".\n> Replace \"\\?\" with \"\\{0,1\\}\".\n\nNice. I was (very slowly) converging on that as the culprit. Thanks for\nthe elegant fix! I'm running the test suite on Windows and OSX (now that\nthey're set up locally) with this included and I'll send a v10 once\ncomplete.\n\n"},{"id":"413943","messageId":"xmqqbldxem24.fsf@gitster.c.googlers.com","threadId":"54972","inReplyTo":"20210109224236.50363-1-davvid@gmail.com","subject":"Re: [PATCH v2] fixup! mergetool: add automerge configuration","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-01-09T23:18:43Z","receivedAt":"2021-01-09T23:19:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"David Aguilar writes:\n\n> \"\\r\\?\" in sed is not portable to macOS and possibly others.\n> \"\\r\" is not valid on Linux with POSIXLY_CORRECT.\n>\n> Replace \"\\r\" with a substituted variable that contains \"\\r\".\n> Replace \"\\?\" with \"\\{0,1\\}\".\n>\n> Helped-by: brian m. carlson \n> Signed-off-by: David Aguilar \n> ---\n> This is based on top of fc/mergetool-automerge and can be squashed in\n> (with the addition of my sign-off) if desired.\n>\n> Changes since last time:\n> - printf '\\r' instead of printf '\\x0d'\n> - The commit message now mentions POSIXLY_CORRECT.\n>\n> git-mergetool.sh | 13 ++++++++++---\n> 1 file changed, 10 insertions(+), 3 deletions(-)\n>\n> diff --git a/git-mergetool.sh b/git-mergetool.sh\n> index a44afd3822..9029a79431 100755\n> --- a/git-mergetool.sh\n> +++ b/git-mergetool.sh\n> @@ -243,9 +243,16 @@ auto_merge () {\n> \tgit merge-file --diff3 --marker-size=7 -q -p \"$LOCAL\" \"$BASE\" \"$REMOTE\" >\"$DIFF3\"\n> \tif test -s \"$DIFF3\"\n> \tthen\n> -\t\tsed -e '/^<<<<<<< /,/^||||||| /d' -e '/^=======\\r\\?$/,/^>>>>>>> /d' \"$DIFF3\" >\"$BASE\"\n> -\t\tsed -e '/^||||||| /,/^>>>>>>> /d' -e '/^<<<<<<< /d' \"$DIFF3\" >\"$LOCAL\"\n> -\t\tsed -e '/^<<<<<<< /,/^=======\\r\\?$/d' -e '/^>>>>>>> /d' \"$DIFF3\" >\"$REMOTE\"\n> +\t\tcr=$(printf '\\r')\n> +\t\tsed -e '/^<<<<<<< /,/^||||||| /d' \\\n> +\t\t\t-e \"/^=======$cr\\{0,1\\}$/,/^>>>>>>> /d\" \\\n> +\t\t\t\"$DIFF3\" >\"$BASE\"\n> +\t\tsed -e '/^||||||| /,/^>>>>>>> /d' \\\n> +\t\t\t-e '/^<<<<<<< /d' \\\n> +\t\t\t\"$DIFF3\" >\"$LOCAL\"\n> +\t\tsed -e \"/^<<<<<<< /,/^=======$cr\\{0,1\\}$/d\" \\\n> +\t\t\t-e '/^>>>>>>> /d' \\\n> +\t\t\t\"$DIFF3\" >\"$REMOTE\"\n> \tfi\n> \trm -- \"$DIFF3\"\n> }\n\nI was hoping that we can avoid repetition that can cause bugs with\nsomething like\n\ndiff --git i/git-mergetool.sh w/git-mergetool.sh\nindex a44afd3822..e07dabbf72 100755\n--- i/git-mergetool.sh\n+++ w/git-mergetool.sh\n@@ -243,9 +243,13 @@ auto_merge () {\n \tgit merge-file --diff3 --marker-size=7 -q -p \"$LOCAL\" \"$BASE\" \"$REMOTE\" >\"$DIFF3\"\n \tif test -s \"$DIFF3\"\n \tthen\n-\t\tsed -e '/^<<<<<<< /,/^||||||| /d' -e '/^=======\\r\\?$/,/^>>>>>>> /d' \"$DIFF3\" >\"$BASE\"\n-\t\tsed -e '/^||||||| /,/^>>>>>>> /d' -e '/^<<<<<<< /d' \"$DIFF3\" >\"$LOCAL\"\n-\t\tsed -e '/^<<<<<<< /,/^=======\\r\\?$/d' -e '/^>>>>>>> /d' \"$DIFF3\" >\"$REMOTE\"\n+\t\tC0=\"^<<<<<<< \"\n+\t\tC1=\"^||||||| \"\n+\t\tC2=\"^=======$(printf '\\015')*\\$\"\n+\t\tC3=\"^>>>>>>> \"\n+\t\tsed -e \"/$C0/,/$C1/d\" -e \"/$C2/,/$C3/d\" \"$DIFF3\" >\"$BASE\"\n+\t\tsed -e \"/$C1/,/$C3/d\" -e \"/$C0/d\" \"$DIFF3\" >\"$LOCAL\"\n+\t\tsed -e \"/$C0/,/$C2/d\" -e \"/$C3 /d\" \"$DIFF3\" >\"$REMOTE\"\n \tfi\n \trm -- \"$DIFF3\"\n }\n"},{"id":"413945","messageId":"xmqqzh1hd7ci.fsf@gitster.c.googlers.com","threadId":"54972","inReplyTo":"X/onP6vFAHH8SUBo@camp.crustytoothpaste.net","subject":"Re: [PATCH] fixup! mergetool: add automerge configuration","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-01-09T23:21:49Z","receivedAt":"2021-01-09T23:22:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"brian m. carlson\" writes:\n\n> Ah, yes, this is true. The statement about \"\\r\" is also true for Linux\n> with POSIXLY_CORRECT, IIRC.\n\nIt's nice to have a way to reproduce without having a separate\ntoolchain. Thanks for the suggestion.\n\n> Unfortunately, printf is not specified by POSIX to take hex escapes, so\n> this, too is nonportable. We are unfortunately allowed to use only\n> octal escapes (yuck). However, we can write this:\n>\n> cr=$(printf '\\r')\n>\n> or\n>\n> cr=$(printf '\\015')\n>\n> I think the former is clearer, since that's what we were writing before.\n\nThe latter however appears more portable at least to traditionalists\n;-)\n"},{"id":"413947","messageId":"xmqqmtxhd1zx.fsf@gitster.c.googlers.com","threadId":"54972","inReplyTo":"20210109225400.GA156779@ellen","subject":"Re: [PATCH v2] fixup! mergetool: add automerge configuration","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-01-10T01:17:22Z","receivedAt":"2021-01-10T01:18:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Seth House writes:\n\n> On Sat, Jan 09, 2021 at 02:42:36PM -0800, David Aguilar wrote:\n>> Replace \"\\r\" with a substituted variable that contains \"\\r\".\n>> Replace \"\\?\" with \"\\{0,1\\}\".\n>\n> Nice. I was (very slowly) converging on that as the culprit. Thanks for\n> the elegant fix! I'm running the test suite on Windows and OSX (now that\n> they're set up locally) with this included and I'll send a v10 once\n> complete.\n\nNote that the topic fails t7800.5 even with the fix-up (and without\nthese fix-up on a platform with sed that do not need the portability\nfix-up).\n"},{"id":"413949","messageId":"xmqqeeitd0dh.fsf@gitster.c.googlers.com","threadId":"54972","inReplyTo":"xmqqbldxem24.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2] fixup! mergetool: add automerge configuration","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-01-10T01:52:26Z","receivedAt":"2021-01-10T01:53:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> I was hoping that we can avoid repetition that can cause bugs with\n> something like\n> ...\n\nHere is a cleaned-up version that would apply cleanly on top of\nyours.\n\nI suspect that it would make it even easier to follow the logic if\nthe two sed \"-e\" expressions are swapped for the $LOCAL one. \n\nIt would clarify that we remove $C0 (separator before ours) and\n$C1..$C3 (ancestor's and theirs, including the separators) to get\nthe local version.\n\nThe other two are already described in the correct order.\n\nThanks.\n\n git-mergetool.sh | 18 ++++++++----------\n 1 file changed, 8 insertions(+), 10 deletions(-)\n\ndiff --git a/git-mergetool.sh b/git-mergetool.sh\nindex 9029a79431..ed152a4187 100755\n--- a/git-mergetool.sh\n+++ b/git-mergetool.sh\n@@ -243,16 +243,14 @@ auto_merge () {\n \tgit merge-file --diff3 --marker-size=7 -q -p \"$LOCAL\" \"$BASE\" \"$REMOTE\" >\"$DIFF3\"\n \tif test -s \"$DIFF3\"\n \tthen\n-\t\tcr=$(printf '\\r')\n-\t\tsed -e '/^<<<<<<< /,/^||||||| /d' \\\n-\t\t\t-e \"/^=======$cr\\{0,1\\}$/,/^>>>>>>> /d\" \\\n-\t\t\t\"$DIFF3\" >\"$BASE\"\n-\t\tsed -e '/^||||||| /,/^>>>>>>> /d' \\\n-\t\t\t-e '/^<<<<<<< /d' \\\n-\t\t\t\"$DIFF3\" >\"$LOCAL\"\n-\t\tsed -e \"/^<<<<<<< /,/^=======$cr\\{0,1\\}$/d\" \\\n-\t\t\t-e '/^>>>>>>> /d' \\\n-\t\t\t\"$DIFF3\" >\"$REMOTE\"\n+\t\tC0=\"^<<<<<<< \"\n+\t\tC1=\"^||||||| \"\n+\t\tC2=\"^=======$(printf '\\015')\\{0,1\\}$\"\n+\t\tC3=\"^>>>>>>> \"\n+\n+\t\tsed -e \"/$C0/,/$C1/d\" -e \"/$C2/,/$C3/d\" \"$DIFF3\" >\"$BASE\"\n+\t\tsed -e \"/$C1/,/$C3/d\" -e \"/$C0/d\" \"$DIFF3\" >\"$LOCAL\"\n+\t\tsed -e \"/$C0/,/$C2/d\" -e \"/$C3/d\" \"$DIFF3\" >\"$REMOTE\"\n \tfi\n \trm -- \"$DIFF3\"\n }\n-- \n2.30.0-307-g37795e20d9\n\n"},{"id":"413953","messageId":"xmqqa6thcn1n.fsf_-_@gitster.c.googlers.com","threadId":"54972","inReplyTo":"xmqqmtxhd1zx.fsf@gitster.c.googlers.com","subject":"Re* [PATCH v2] fixup! mergetool: add automerge configuration","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-01-10T06:40:20Z","receivedAt":"2021-01-10T06:41:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Seth House writes:\n>\n>> On Sat, Jan 09, 2021 at 02:42:36PM -0800, David Aguilar wrote:\n>>> Replace \"\\r\" with a substituted variable that contains \"\\r\".\n>>> Replace \"\\?\" with \"\\{0,1\\}\".\n>>\n>> Nice. I was (very slowly) converging on that as the culprit. Thanks for\n>> the elegant fix! I'm running the test suite on Windows and OSX (now that\n>> they're set up locally) with this included and I'll send a v10 once\n>> complete.\n>\n> Note that the topic fails t7800.5 even with the fix-up (and without\n> these fix-up on a platform with sed that do not need the portability\n> fix-up).\n\nIt seems that 51b27370 (mergetool: break setup_tool out into\nseparate initialization function, 2020-12-28) is the culprit.\n\ngit-difftool--helper is taught to do this:\n\n diff --git a/git-difftool--helper.sh b/git-difftool--helper.sh\n index 46af3e60b7..234dd6944e 100755\n --- a/git-difftool--helper.sh\n +++ b/git-difftool--helper.sh\n ...\n @@ -79,6 +80,7 @@ if test -n \"$GIT_DIFFTOOL_DIRDIFF\"\n then\n LOCAL=\"$1\"\n REMOTE=\"$2\"\n +\tinitialize_merge_tool \"$merge_tool\" &&\n run_merge_tool \"$merge_tool\" false\n else\n # Launch the merge tool on each path provided by 'git diff'\n\nBut the thing is, t7800.5 gives a name of merge-tool that does not\nexist, and expects difftool to notice it and error out. The callchain\nstarting from the above \"run_merge_tool\" should look like\n\n run_merge_tool () {\n merge_tool_path=$(get_merge_tool_path ...) || exit\n\t...\n\nwhich in turn calls\n\n get_merge_tool_path () {\n # A merge tool has been set, so verify that it's valid.\n merge_tool=\"$1\"\n if ! valid_tool \"$merge_tool\"\n then\n echo >&2 \"Unknown merge tool $merge_tool\"\n exit 1\n fi\n\nand \"valid_tool bad-tool\" would return false, which lets us safely\nexit with the error message.\n\nBut the call to initialize_merge_tool inserted above, that makes us\nto SKIP the call to run_merge_tool, would defeat the error detection.\n\nAn ugly workaround patch that caters only to difftool breakage is\nattached at the end; I did not look if a similar treatment is\nnecessary for the mergetool side.\n\nBy the way, debugging this was somewhat painful as difftool is partly\nrewritten in C. If it were still pure script, it would have been a\nlot easier to diagnose with a single \"set -x\" somewhere X-<.\n\n----- >8 ---------- >8 ---------- >8 ---------- >8 -----\nFrom: Junio C Hamano \nDate: Sat, 9 Jan 2021 22:35:18 -0800\nSubject: [PATCH] fixup! mergetool: break setup_tool out into\n separate initialization function\n\nAt least difftool wants to see even a broken tool name in its call\nto run_merge_tool and have it diagnose an error. &&-cascading the\ncall to initialize_merge_tool and run_merge_tool means that a bogus\ntool name that does not please initialize_merge_tool is not even seen\nby run_merge_tool and the error goes unnoticed.\n\nI didn't audit the original commit for its use of initialize_merge_tool\non the merge-tool side. It may share the same issue, or it may not.\n\nThis should fix the errors we've been seeing in t7800.5 (and\npossibly others) with this topic.\n---\n git-difftool--helper.sh | 8 ++++++--\n 1 file changed, 6 insertions(+), 2 deletions(-)\n\ndiff --git a/git-difftool--helper.sh b/git-difftool--helper.sh\nindex 234dd6944e..992124cc67 100755\n--- a/git-difftool--helper.sh\n+++ b/git-difftool--helper.sh\n@@ -61,7 +61,9 @@ launch_merge_tool () {\n \t\texport BASE\n \t\teval $GIT_DIFFTOOL_EXTCMD '\"$LOCAL\"' '\"$REMOTE\"'\n \telse\n-\t\tinitialize_merge_tool \"$merge_tool\" &&\n+\t\tinitialize_merge_tool \"$merge_tool\"\n+\t\t# ignore the error from the above --- run_merge_tool\n+\t\t# will diagnose unusable tool by itself\n \t\trun_merge_tool \"$merge_tool\"\n \tfi\n }\n@@ -80,7 +82,9 @@ if test -n \"$GIT_DIFFTOOL_DIRDIFF\"\n then\n \tLOCAL=\"$1\"\n \tREMOTE=\"$2\"\n-\tinitialize_merge_tool \"$merge_tool\" &&\n+\tinitialize_merge_tool \"$merge_tool\"\n+\t# ignore the error from the above --- run_merge_tool\n+\t# will diagnose unusable tool by itself\n \trun_merge_tool \"$merge_tool\" false\n else\n \t# Launch the merge tool on each path provided by 'git diff'\n-- \n2.30.0-311-g8a3956654a\n\n"},{"id":"413957","messageId":"20210110072902.GA247325@ellen","threadId":"54972","inReplyTo":"xmqqa6thcn1n.fsf_-_@gitster.c.googlers.com","subject":"Re: Re* [PATCH v2] fixup! mergetool: add automerge configuration","fromName":"Seth House","fromEmail":"seth@eseth.com","sentAt":"2021-01-10T07:29:02Z","receivedAt":"2021-01-10T07:32:18Z","isPatch":true,"sender":{"key":"seth@eseth.com","avatar":"https://avatars.githubusercontent.com/u/91293?v=4"},"body":"On Sat, Jan 09, 2021 at 10:40:20PM -0800, Junio C Hamano wrote:\n> An ugly workaround patch that caters only to difftool breakage is\n> attached at the end; I did not look if a similar treatment is\n> necessary for the mergetool side.\n\nThat fixup does the trick on my machine too. Thank you.\n\nHow wary are you of continuing with `initialize_merge_tool`? Do you see\na better approach to get `automerge_enabled `into scope? While it is\na nice feature to have, is it worth the risk vs. reward?\n\n"},{"id":"413959","messageId":"xmqqh7np9gqn.fsf@gitster.c.googlers.com","threadId":"54972","inReplyTo":"20210110072902.GA247325@ellen","subject":"Re: Re* [PATCH v2] fixup! mergetool: add automerge configuration","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-01-10T11:24:48Z","receivedAt":"2021-01-10T11:25:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Seth House writes:\n\n> On Sat, Jan 09, 2021 at 10:40:20PM -0800, Junio C Hamano wrote:\n>> An ugly workaround patch that caters only to difftool breakage is\n>> attached at the end; I did not look if a similar treatment is\n>> necessary for the mergetool side.\n>\n> That fixup does the trick on my machine too. Thank you.\n\nNote that with t7800 fixed with the patch, non Windows jobs all seem\nto pass, but t7610 seems to have problem(s) on Windows.\n\nhttps://github.com/git/git/runs/1675932107?check_suite_focus=true#step:7:10373\n\n> How wary are you of continuing with `initialize_merge_tool`? Do you see\n> a better approach to get `automerge_enabled `into scope? While it is\n> a nice feature to have, is it worth the risk vs. reward?\n\nHmph. We need to have a way to define helper routines customized\nfor the tool, and in the codebase without this series, it is the job\nfor setup_tool. It defines fallback implementations, allows tool\nspecific customizations. \n\nYour initialize_merge_tool is just a thin wrapper around setup_tool.\nIt calls setup_tool, and if the function exits with non-zero status,\nreturns with status==1 (and otherwise returns with status==0). As I\nexpect all the callers of setup_tool or initialize_merge_tool would\neither ignore the status or check if it succeeded (i.e. compare $?\nagainst 0 and any non-zero values are treated equally), it does not\nseem to do anything useful.\n\nI think we may be able to get rid of initialize_merge_tool, but you\nwould need to call setup_tool in places initialize_merge_tool was\ncalled in your patch, as you must have needed to make sure that the\ntool specific customizations have been carried out before going\nforward in these places.\n\nSo, no, I do not see a reason to be wary of initialize/setup. \n\nWith or without the seemingly needless initialize wrapper, I think\ncalling setup before starting to do certain operations that need\ntool specific customization is just necessary. The same machanism\nhas been in use to give can_merge/can_diff to each tool and the way\nit works ought to be fairly well understood.\n\nIt is a different story if it makes sense not to exit when you see\nfailure from initialize/setup, and instead _skip_ running helpers\nlike run_merge_tool. It was just a mistake we all make every once\nin a while (i.e. a bug), and I am reasonably sure that we will\nintroduce more of them but we will be capable of fixing all.\n\nThanks.\n"},{"id":"414486","messageId":"20210116042454.GA4913@ellen","threadId":"54972","inReplyTo":"xmqqh7np9gqn.fsf@gitster.c.googlers.com","subject":"Re: Re* [PATCH v2] fixup! mergetool: add automerge configuration","fromName":"Seth House","fromEmail":"seth@eseth.com","sentAt":"2021-01-16T04:24:54Z","receivedAt":"2021-01-16T04:26:04Z","isPatch":true,"sender":{"key":"seth@eseth.com","avatar":"https://avatars.githubusercontent.com/u/91293?v=4"},"body":"On Sun, Jan 10, 2021 at 03:24:48AM -0800, Junio C Hamano wrote:\n> Note that with t7800 fixed with the patch, non Windows jobs all seem\n> to pass, but t7610 seems to have problem(s) on Windows.\n\nThe autocrlf test is breaking because the sed that ships with some mingw\nversions (and also some minsys and cygwin versions) will *automatically*\nremove carriage returns:\n\n$ printf 'foo\\r\\nbar\\r\\n' | sed -e '/bar/d' | cat -A\nfoo$\n\n$ printf 'foo\\r\\nbar\\r\\n' | sed -b -e '/bar/d' | cat -A\nfoo^M$\n\n(Note: the -b flag above is just for comparison. We can't use it here.\nIt's not in POSIX and is not present in sed for busybox or OSX.)\n\nI haven't found official docs that describe this behavior yet but\nI found a few discussions about it across a few lists. E.g.:\nhttps://cygwin.com/pipermail/cygwin/2017-June/233121.html\n\nSuggestions welcome.\n\nObviously we could try to detect crlf's before the sed and then try to\nre-add them back in after sed but that strikes me as error-prone.\n\nI recall someone mentioning calling merge-file twice, once with --ours\nand once with --theirs, to independently generate each \"side\" of the\nconflict instead of using sed to split MERGED. Is that a viable\napproach?\n\n"},{"id":"414872","messageId":"20210120232447.GA35105@ellen","threadId":"54972","inReplyTo":"20210116042454.GA4913@ellen","subject":"automerge implementation ideas for Windows","fromName":"Seth House","fromEmail":"seth@eseth.com","sentAt":"2021-01-20T23:24:47Z","receivedAt":"2021-01-21T00:49:46Z","isPatch":false,"sender":{"key":"seth@eseth.com","avatar":"https://avatars.githubusercontent.com/u/91293?v=4"},"body":"On Fri, Jan 15, 2021 at 09:24:59PM -0700, Seth House wrote:\n> The autocrlf test is breaking because the sed that ships with some mingw\n> versions (and also some minsys and cygwin versions) will *automatically*\n> remove carriage returns:\n\nSo the mingw builds of both sed and awk change carriage returns. So far\nI haven't found documentation on it so I'm not aware of a portable way\nto disable the behavior. Instead I've been playing with alternate\napproaches. The two patches below work and pass the autocrlf test on\nWindows, however they are first-draft implementations. Feedback welcome.\n\nOne other point of discussion: I would like to change the name of this\nfeature. \"Automerge\" is a bit of an overloaded term and, IMO, doesn't\ndescribe this feature very well. Several of the GUI diff programs have\na feature that they call \"automerge\" or \"auto merge\", and there's a flag\nfor Meld already in Git called \"mergetool.meld.useAutoMerge\" which could\ncause confusion.\n\nInstead, I'd like to propose \"mergetool.hideResolved\" or the more\nverbose \"mergetool.hideResolvedConflicts\" as the name. We're not really\nmerging anything (Git aleady did that before the mergetool is invoked),\nbut rather we're just not showing any conflicts that Git was already\nable to resolve.\n\n---------->8--------->8--------->8--------->8-------\n\n#1: Use POSIX read and a while loop to emulate an awk-like approach:\n\ndiff --git a/git-mergetool.sh b/git-mergetool.sh\nindex 246d6b76fc..94728dd518 100755\n--- a/git-mergetool.sh\n+++ b/git-mergetool.sh\n@@ -240,19 +240,46 @@ checkout_staged_file () {\n }\n \n auto_merge () {\n+\tC0=\"<<<<<<< \"\n+\tC1=\"||||||| \"\n+\tC2=\"=======\"\n+\tC3=\">>>>>>> \"\n+\tinl=0\n+\tinb=0\n+\tinr=0\n+\n \tgit merge-file --diff3 --marker-size=7 -q -p \"$LOCAL\" \"$BASE\" \"$REMOTE\" >\"$DIFF3\"\n+\n \tif test -s \"$DIFF3\"\n \tthen\n-\t\tC0=\"^<<<<<<< \"\n-\t\tC1=\"^||||||| \"\n-\t\tC2=\"^=======$(printf '\\015')\\{0,1\\}$\"\n-\t\tC3=\"^>>>>>>> \"\n-\n-\t\tsed -e \"/$C0/,/$C1/d\" -e \"/$C2/,/$C3/d\" \"$DIFF3\" >\"$BASE\"\n-\t\tsed -e \"/$C1/,/$C3/d\" -e \"/$C0/d\" \"$DIFF3\" >\"$LOCAL\"\n-\t\tsed -e \"/$C0/,/$C2/d\" -e \"/$C3/d\" \"$DIFF3\" >\"$REMOTE\"\n+\t\ttouch \"$LCONFL\" \"$BCONFL\" \"$RCONFL\"\n+\n+\t\twhile read -r line\n+\t\tdo\n+\t\t\tcase $line in\n+\t\t\t\t$C0*) inl=1; continue ;;\n+\t\t\t\t$C1*) inl=0; inb=1; continue ;;\n+\t\t\t\t$C2*) inb=0; inr=1; continue ;;\n+\t\t\t\t$C3*) inr=0; continue ;;\n+\t\t\tesac\n+\n+\t\t\tcase 1 in\n+\t\t\t\t$inl) printf '%s\\n' \"$line\" >>\"$LCONFL\" ;;\n+\t\t\t\t$inb) printf '%s\\n' \"$line\" >>\"$BCONFL\" ;;\n+\t\t\t\t$inr) printf '%s\\n' \"$line\" >>\"$RCONFL\" ;;\n+\t\t\t\t*)\n+\t\t\t\t\tprintf '%s\\n' \"$line\" >>\"$LCONFL\"\n+\t\t\t\t\tprintf '%s\\n' \"$line\" >>\"$BCONFL\"\n+\t\t\t\t\tprintf '%s\\n' \"$line\" >>\"$RCONFL\"\n+\t\t\t\t;;\n+\t\t\tesac\n+\t\tdone < \"$DIFF3\"\n+\n+\t\tmv -- \"$LCONFL\" \"$LOCAL\"\n+\t\tmv -- \"$BCONFL\" \"$BASE\"\n+\t\tmv -- \"$RCONFL\" \"$REMOTE\"\n+\t\trm -- \"$DIFF3\"\n \tfi\n-\trm -- \"$DIFF3\"\n }\n \n merge_file () {\n@@ -295,8 +322,11 @@ merge_file () {\n \tDIFF3=\"$MERGETOOL_TMPDIR/${BASE}_DIFF3_$$$ext\"\n \tBACKUP=\"$MERGETOOL_TMPDIR/${BASE}_BACKUP_$$$ext\"\n \tLOCAL=\"$MERGETOOL_TMPDIR/${BASE}_LOCAL_$$$ext\"\n+\tLCONFL=\"$MERGETOOL_TMPDIR/${BASE}_LOCAL_LCONFL_$$$ext\"\n \tREMOTE=\"$MERGETOOL_TMPDIR/${BASE}_REMOTE_$$$ext\"\n+\tRCONFL=\"$MERGETOOL_TMPDIR/${BASE}_REMOTE_RCONFL_$$$ext\"\n \tBASE=\"$MERGETOOL_TMPDIR/${BASE}_BASE_$$$ext\"\n+\tBCONFL=\"$MERGETOOL_TMPDIR/${BASE}_BASE_BCONFL_$$$ext\"\n \n \tbase_mode= local_mode= remote_mode=\n\n---------->8--------->8--------->8--------->8-------\n\n#2: Call merge-file twice:\n\nA much simpler implementation but probably less performant. Is it enough\nto matter?\n\ndiff --git a/git-mergetool.sh b/git-mergetool.sh\nindex 246d6b76fc..1cb45a7437 100755\n--- a/git-mergetool.sh\n+++ b/git-mergetool.sh\n@@ -240,19 +240,10 @@ checkout_staged_file () {\n }\n \n auto_merge () {\n-\tgit merge-file --diff3 --marker-size=7 -q -p \"$LOCAL\" \"$BASE\" \"$REMOTE\" >\"$DIFF3\"\n-\tif test -s \"$DIFF3\"\n-\tthen\n-\t\tC0=\"^<<<<<<< \"\n-\t\tC1=\"^||||||| \"\n-\t\tC2=\"^=======$(printf '\\015')\\{0,1\\}$\"\n-\t\tC3=\"^>>>>>>> \"\n-\n-\t\tsed -e \"/$C0/,/$C1/d\" -e \"/$C2/,/$C3/d\" \"$DIFF3\" >\"$BASE\"\n-\t\tsed -e \"/$C1/,/$C3/d\" -e \"/$C0/d\" \"$DIFF3\" >\"$LOCAL\"\n-\t\tsed -e \"/$C0/,/$C2/d\" -e \"/$C3/d\" \"$DIFF3\" >\"$REMOTE\"\n-\tfi\n-\trm -- \"$DIFF3\"\n+\tgit merge-file --ours -q -p \"$LOCAL\" \"$BASE\" \"$REMOTE\" >\"$LCONFL\"\n+\tgit merge-file --theirs -q -p \"$LOCAL\" \"$BASE\" \"$REMOTE\" >\"$RCONFL\"\n+\tmv -- \"$LCONFL\" \"$LOCAL\"\n+\tmv -- \"$RCONFL\" \"$REMOTE\"\n }\n \n merge_file () {\n@@ -295,7 +286,9 @@ merge_file () {\n \tDIFF3=\"$MERGETOOL_TMPDIR/${BASE}_DIFF3_$$$ext\"\n \tBACKUP=\"$MERGETOOL_TMPDIR/${BASE}_BACKUP_$$$ext\"\n \tLOCAL=\"$MERGETOOL_TMPDIR/${BASE}_LOCAL_$$$ext\"\n+\tLCONFL=\"$MERGETOOL_TMPDIR/${BASE}_LOCAL_LCONFL_$$$ext\"\n \tREMOTE=\"$MERGETOOL_TMPDIR/${BASE}_REMOTE_$$$ext\"\n+\tRCONFL=\"$MERGETOOL_TMPDIR/${BASE}_REMOTE_RCONFL_$$$ext\"\n \tBASE=\"$MERGETOOL_TMPDIR/${BASE}_BASE_$$$ext\"\n \n \tbase_mode= local_mode= remote_mode=\n\n"},{"id":"414934","messageId":"xmqqk0s5c3bv.fsf@gitster.c.googlers.com","threadId":"54972","inReplyTo":"20210120232447.GA35105@ellen","subject":"Re: automerge implementation ideas for Windows","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-01-21T22:50:12Z","receivedAt":"2021-01-21T22:50:59Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Seth House writes:\n\n> One other point of discussion: I would like to change the name of this\n> feature. \"Automerge\" is a bit of an overloaded term and, IMO, doesn't\n> describe this feature very well. Several of the GUI diff programs have\n> a feature that they call \"automerge\" or \"auto merge\", and there's a flag\n> for Meld already in Git called \"mergetool.meld.useAutoMerge\" which could\n> cause confusion.\n>\n> Instead, I'd like to propose \"mergetool.hideResolved\" or the more\n> verbose \"mergetool.hideResolvedConflicts\" as the name. We're not really\n> merging anything (Git aleady did that before the mergetool is invoked),\n> but rather we're just not showing any conflicts that Git was already\n> able to resolve.\n\nI have no objetion. I didn't think 'automerge' was bad, but it\nprobably is too broad a word as you discuss in the above.\n\n\"hide resolved\" sounds like the name that describes what it does\nquite well.\n\n> #1: Use POSIX read and a while loop to emulate an awk-like approach:\n\nI'd rather not to see us do \"text processing\" in shell, especially\nwith \"read -r\". I just do not trust it (even with the \"-r\" option).\n\nHaving said that, I am not familiar enough to the Windows\nenvironment to know what is trustworthy and what is not (apparently,\nthings like \"sed\" that I would intuitively place as much trust as\nanything else is giving us so much trouble out of box), so I'll\nshut up and listen to others.\n"},{"id":"414946","messageId":"20210122010902.GA48178@ellen","threadId":"54972","inReplyTo":"xmqqk0s5c3bv.fsf@gitster.c.googlers.com","subject":"Re: automerge implementation ideas for Windows","fromName":"Seth House","fromEmail":"seth@eseth.com","sentAt":"2021-01-22T01:09:02Z","receivedAt":"2021-01-22T01:09:54Z","isPatch":false,"sender":{"key":"seth@eseth.com","avatar":"https://avatars.githubusercontent.com/u/91293?v=4"},"body":"On Thu, Jan 21, 2021 at 02:50:12PM -0800, Junio C Hamano wrote:\n> I'd rather not to see us do \"text processing\" in shell\n\nAgreed. What are your thoughts on the #2 approach?\n\nI noticed the comment in `git/xdiff-interface.h` about xdiff's gigabyte\nlimit so I created a 973 MB text file with a conflict and ran #2 through\na few mergetools to see how it went. I put /usr/bin/time in front of the\ntwo `git merge-file` invocations. I know one person's machine is not\na benchmark but perhaps it's a discussion point?\n\nEach `git merge-file` call took ~11 seconds on my middle-tier laptop and\ndid not use enough RAM to hit swap.\n\nWriting the near-gigabyte LOCAL, BASE, REMOTE, & BACKUP files went\npretty quick. The mergetools themselves had mixed results:\n\n- vimdiff took several minutes (and a lot of swap) to open all four\n files but did eventually work.\n- tkdiff crashed.\n- Meld spun for ~10 minutes and never opened.\n\nMy takeaway: when trying to use a mergetool on a very large file, the\ntwo `git merge-file` invocations are not likely to be where the\nperformance concern is. #2 is my preferred approach so far.\n\n"},{"id":"414949","messageId":"xmqqh7n9aer5.fsf@gitster.c.googlers.com","threadId":"54972","inReplyTo":"20210122010902.GA48178@ellen","subject":"Re: automerge implementation ideas for Windows","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-01-22T02:26:22Z","receivedAt":"2021-01-22T02:27:30Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Seth House writes:\n\n> On Thu, Jan 21, 2021 at 02:50:12PM -0800, Junio C Hamano wrote:\n>> I'd rather not to see us do \"text processing\" in shell\n>\n> Agreed. What are your thoughts on the #2 approach?\n>\n> I noticed the comment in `git/xdiff-interface.h` about xdiff's gigabyte\n> limit so I created a 973 MB text file with a conflict and ran #2 through\n> a few mergetools to see how it went. I put /usr/bin/time in front of the\n> two `git merge-file` invocations. I know one person's machine is not\n> a benchmark but perhaps it's a discussion point?\n>\n> Each `git merge-file` call took ~11 seconds on my middle-tier laptop and\n> did not use enough RAM to hit swap.\n>\n> Writing the near-gigabyte LOCAL, BASE, REMOTE, & BACKUP files went\n> pretty quick. The mergetools themselves had mixed results:\n>\n> - vimdiff took several minutes (and a lot of swap) to open all four\n> files but did eventually work.\n> - tkdiff crashed.\n> - Meld spun for ~10 minutes and never opened.\n>\n> My takeaway: when trying to use a mergetool on a very large file, the\n> two `git merge-file` invocations are not likely to be where the\n> performance concern is. #2 is my preferred approach so far.\n\nYeah, I am no expert about Windows, but at least I know how well\n\"git merge-file\" should work _anywhere_ (as opposed to \"read -r\"\nplus shell loop that I would not trust on a platform where even\nbasic things like \"sed\" behaves differently from what we expect\nX-<), so from that point of view, it is vastly more preferrable,\nif the choices were only between #1 and #2.\n"},{"id":"414951","messageId":"YAo9aTkZBCSGLYTT@camp.crustytoothpaste.net","threadId":"54972","inReplyTo":"20210116042454.GA4913@ellen","subject":"Re: Re* [PATCH v2] fixup! mergetool: add automerge configuration","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2021-01-22T02:50:17Z","receivedAt":"2021-01-22T02:51:57Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2021-01-16 at 04:24:54, Seth House wrote:\n> On Sun, Jan 10, 2021 at 03:24:48AM -0800, Junio C Hamano wrote:\n> > Note that with t7800 fixed with the patch, non Windows jobs all seem\n> > to pass, but t7610 seems to have problem(s) on Windows.\n> \n> The autocrlf test is breaking because the sed that ships with some mingw\n> versions (and also some minsys and cygwin versions) will *automatically*\n> remove carriage returns:\n> \n> $ printf 'foo\\r\\nbar\\r\\n' | sed -e '/bar/d' | cat -A\n> foo$\n> \n> $ printf 'foo\\r\\nbar\\r\\n' | sed -b -e '/bar/d' | cat -A\n> foo^M$\n> \n> (Note: the -b flag above is just for comparison. We can't use it here.\n> It's not in POSIX and is not present in sed for busybox or OSX.)\n\nCan you report this as a bug? This behavior isn't compliant with POSIX\nand it makes it really hard for folks to write portable code if these\nversions implement POSIX utilities in a nonstandard way. As a\nnon-Windows user, I have no hope of writing code that works on Windows\nif we can't rely on our standard utilities working properly.\n-- \nbrian m. carlson (he/him or they/them)\nHouston, Texas, US\n"},{"id":"414975","messageId":"nycvar.QRO.7.76.6.2101221728410.52@tvgsbejvaqbjf.bet","threadId":"54972","inReplyTo":"YAo9aTkZBCSGLYTT@camp.crustytoothpaste.net","subject":"Re: Re* [PATCH v2] fixup! mergetool: add automerge configuration","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2021-01-22T16:29:46Z","receivedAt":"2021-01-22T16:56:37Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi brian,\n\nOn Fri, 22 Jan 2021, brian m. carlson wrote:\n\n> On 2021-01-16 at 04:24:54, Seth House wrote:\n> > On Sun, Jan 10, 2021 at 03:24:48AM -0800, Junio C Hamano wrote:\n> > > Note that with t7800 fixed with the patch, non Windows jobs all seem\n> > > to pass, but t7610 seems to have problem(s) on Windows.\n> >\n> > The autocrlf test is breaking because the sed that ships with some mingw\n> > versions (and also some minsys and cygwin versions) will *automatically*\n> > remove carriage returns:\n> >\n> > $ printf 'foo\\r\\nbar\\r\\n' | sed -e '/bar/d' | cat -A\n> > foo$\n> >\n> > $ printf 'foo\\r\\nbar\\r\\n' | sed -b -e '/bar/d' | cat -A\n> > foo^M$\n> >\n> > (Note: the -b flag above is just for comparison. We can't use it here.\n> > It's not in POSIX and is not present in sed for busybox or OSX.)\n>\n> Can you report this as a bug? This behavior isn't compliant with POSIX\n> and it makes it really hard for folks to write portable code if these\n> versions implement POSIX utilities in a nonstandard way. As a\n> non-Windows user, I have no hope of writing code that works on Windows\n> if we can't rely on our standard utilities working properly.\n\nI fear that the Windows-based tools do the correct thing, though: they are\nmeant to process _text_, and newlines are supposed to be\nplatform-dependent in text.\n\nFrom that perspective, it sounds to me as if we're trying to ask `sed` to\ndo something it was not designed to do: binary editing.\n\nCiao,\nDscho\n"},{"id":"415023","messageId":"YAte7ixZYdz1AOMX@camp.crustytoothpaste.net","threadId":"54972","inReplyTo":"nycvar.QRO.7.76.6.2101221728410.52@tvgsbejvaqbjf.bet","subject":"Re: Re* [PATCH v2] fixup! mergetool: add automerge configuration","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2021-01-22T23:25:34Z","receivedAt":"2021-01-22T23:26:59Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2021-01-22 at 16:29:46, Johannes Schindelin wrote:\n> Hi brian,\n> \n> On Fri, 22 Jan 2021, brian m. carlson wrote:\n> \n> > On 2021-01-16 at 04:24:54, Seth House wrote:\n> > > The autocrlf test is breaking because the sed that ships with some mingw\n> > > versions (and also some minsys and cygwin versions) will *automatically*\n> > > remove carriage returns:\n> > >\n> > > $ printf 'foo\\r\\nbar\\r\\n' | sed -e '/bar/d' | cat -A\n> > > foo$\n> > >\n> > > $ printf 'foo\\r\\nbar\\r\\n' | sed -b -e '/bar/d' | cat -A\n> > > foo^M$\n> > >\n> > > (Note: the -b flag above is just for comparison. We can't use it here.\n> > > It's not in POSIX and is not present in sed for busybox or OSX.)\n> >\n> > Can you report this as a bug? This behavior isn't compliant with POSIX\n> > and it makes it really hard for folks to write portable code if these\n> > versions implement POSIX utilities in a nonstandard way. As a\n> > non-Windows user, I have no hope of writing code that works on Windows\n> > if we can't rely on our standard utilities working properly.\n> \n> I fear that the Windows-based tools do the correct thing, though: they are\n> meant to process _text_, and newlines are supposed to be\n> platform-dependent in text.\n\nAh, but POSIX gives a very specific meaning to \"newline\", and it refers\nto a single byte. If you want tools that process CRLF line endings like\nthat, then that should be opt-in as either different tools or additional\noptions, not the default behavior of a POSIX tool. This behavior is not\nconforming to POSIX and it is therefore a defect.\n\n> From that perspective, it sounds to me as if we're trying to ask `sed` to\n> do something it was not designed to do: binary editing.\n\nMost Windows tools are perfectly capable of handling LF line endings.\nEven the famously incapable Notepad can now handle LF without CR. With\nthe advent of WSL, handling LF line endings is now pretty much required.\n-- \nbrian m. carlson (he/him or they/them)\nHouston, Texas, US\n"},{"id":"415284","messageId":"nycvar.QRO.7.76.6.2101261522200.57@tvgsbejvaqbjf.bet","threadId":"54972","inReplyTo":"YAte7ixZYdz1AOMX@camp.crustytoothpaste.net","subject":"Re: Re* [PATCH v2] fixup! mergetool: add automerge configuration","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2021-01-26T14:32:13Z","receivedAt":"2021-01-26T14:35:11Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi brian,\n\nOn Fri, 22 Jan 2021, brian m. carlson wrote:\n\n> On 2021-01-22 at 16:29:46, Johannes Schindelin wrote:\n> >\n> > On Fri, 22 Jan 2021, brian m. carlson wrote:\n> >\n> > > On 2021-01-16 at 04:24:54, Seth House wrote:\n> > > > The autocrlf test is breaking because the sed that ships with some mingw\n> > > > versions (and also some minsys and cygwin versions) will *automatically*\n> > > > remove carriage returns:\n> > > >\n> > > > $ printf 'foo\\r\\nbar\\r\\n' | sed -e '/bar/d' | cat -A\n> > > > foo$\n> > > >\n> > > > $ printf 'foo\\r\\nbar\\r\\n' | sed -b -e '/bar/d' | cat -A\n> > > > foo^M$\n> > > >\n> > > > (Note: the -b flag above is just for comparison. We can't use it here.\n> > > > It's not in POSIX and is not present in sed for busybox or OSX.)\n> > >\n> > > Can you report this as a bug? This behavior isn't compliant with POSIX\n> > > and it makes it really hard for folks to write portable code if these\n> > > versions implement POSIX utilities in a nonstandard way. As a\n> > > non-Windows user, I have no hope of writing code that works on Windows\n> > > if we can't rely on our standard utilities working properly.\n> >\n> > I fear that the Windows-based tools do the correct thing, though: they are\n> > meant to process _text_, and newlines are supposed to be\n> > platform-dependent in text.\n>\n> Ah, but POSIX gives a very specific meaning to \"newline\", and it refers\n> to a single byte. If you want tools that process CRLF line endings like\n> that, then that should be opt-in as either different tools or additional\n> options, not the default behavior of a POSIX tool. This behavior is not\n> conforming to POSIX and it is therefore a defect.\n\nIf we needed another reminder that\n\n\twe never say \"It's in POSIX; we'll happily ignore your needs\n\tshould your system not conform to it.\"\n\n(https://github.com/git/git/blob/v2.30.0/Documentation/CodingGuidelines),\nthen we have it right here ;-)\n\n> > From that perspective, it sounds to me as if we're trying to ask `sed` to\n> > do something it was not designed to do: binary editing.\n>\n> Most Windows tools are perfectly capable of handling LF line endings.\n> Even the famously incapable Notepad can now handle LF without CR. With\n> the advent of WSL, handling LF line endings is now pretty much required.\n\nI have two comments on that:\n\n1. We could spend a splendid time questioning MSYS2's (and before that,\n MSys') choices regarding newlines, but I think we can spend that time a\n lot better.\n\n2. Newer software _seems_ to handle LF line endings just fine. And the\n `sed` invocation you mentioned above does so: it groks input delimited\n by Line Feed as newline characters. That does not mean that its output\n is LF-only by default. I seem to remember that recent Visual Studio\n versions did something similar: happily read an LF-only `.xml` file,\n but then write out a modified version using CR/LF.\n\nWould I wish that Windows used LF-only instead of CR/LF, just like macOS\nswitched away from CR-only to LF-only after MacOS 9? Sure I do. It would\nremove quite a few obstacles in my daily work. Can I do anything about it?\nNo.\n\nSo I'd rather see `git mergetool` be turned into a portable C program, or\nalternatively using a built-in helper that _is_ written in C, to perform\nthat desired text munging instead of losing the battle to the challenge of\ncross-platform, advanced text processing in pure shell script.\n\nCiao,\nDscho\n"},{"id":"415334","messageId":"20210126180635.GA28241@ellen","threadId":"54972","inReplyTo":"nycvar.QRO.7.76.6.2101261522200.57@tvgsbejvaqbjf.bet","subject":"Re: Re* [PATCH v2] fixup! mergetool: add automerge configuration","fromName":"Seth House","fromEmail":"seth@eseth.com","sentAt":"2021-01-26T18:06:35Z","receivedAt":"2021-01-26T22:08:57Z","isPatch":true,"sender":{"key":"seth@eseth.com","avatar":"https://avatars.githubusercontent.com/u/91293?v=4"},"body":"On Tue, Jan 26, 2021 at 03:32:13PM +0100, Johannes Schindelin wrote:\n> So I'd rather see `git mergetool` be turned into a portable C program, or\n> alternatively using a built-in helper that _is_ written in C, to perform\n> that desired text munging\n\nI tend to agree. Though my personal preference is Cygwin's (eventual)\napproach, I can appreciate the arguments made by the MSYS2 folk. But\nsetting that aside, IMO, the ideal place to handle this would be the\nsame place where the conflict markers are written in the first place,\nxmerge.c if my limited C literacy is correct.\n\nI don't see a big distinction between writing a single file with\nconflict markers and writing two, diff-able files with each \"side\" of\nthe conflict -- they're ultimately two different formats for expressing\nthe same information. That would give us the portability you described\nand the (pretty amazing) performance that merge-file already enjoys. :)\n\nI'm more than happy with calling merge-file twice for now. A future\nC optimisation, perhaps exposed via merge-file as a new (e.g.)\n--write-conflict-files flag, would be even more awesome.\n\n"},{"id":"415358","messageId":"xmqqr1m71mty.fsf@gitster.c.googlers.com","threadId":"54972","inReplyTo":"20210126180635.GA28241@ellen","subject":"Re: Re* [PATCH v2] fixup! mergetool: add automerge configuration","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-01-26T20:10:17Z","receivedAt":"2021-01-26T22:19:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Seth House writes:\n\n> On Tue, Jan 26, 2021 at 03:32:13PM +0100, Johannes Schindelin wrote:\n>> So I'd rather see `git mergetool` be turned into a portable C program, or\n>> alternatively using a built-in helper that _is_ written in C, to perform\n>> that desired text munging\n>\n> I tend to agree. Though my personal preference is Cygwin's (eventual)\n> approach, I can appreciate the arguments made by the MSYS2 folk. But\n> setting that aside, IMO, the ideal place to handle this would be the\n> same place where the conflict markers are written in the first place,\n> xmerge.c if my limited C literacy is correct.\n>\n> I don't see a big distinction between writing a single file with\n> conflict markers and writing two, diff-able files with each \"side\" of\n> the conflict -- they're ultimately two different formats for expressing\n> the same information. That would give us the portability you described\n> and the (pretty amazing) performance that merge-file already enjoys. :)\n>\n> I'm more than happy with calling merge-file twice for now. A future\n> C optimisation, perhaps exposed via merge-file as a new (e.g.)\n> --write-conflict-files flag, would be even more awesome.\n\nI am OK with that \"two merge-file invocations, one with --ours and\nthen another with --theirs\" approach, as I already said in\nhttps://lore.kernel.org/git/xmqqh7n9aer5.fsf@gitster.c.googlers.com/\n\n\n"},{"id":"415373","messageId":"20210127033714.GA16914@ellen","threadId":"54972","inReplyTo":"xmqqr1m71mty.fsf@gitster.c.googlers.com","subject":"Re: Re* [PATCH v2] fixup! mergetool: add automerge configuration","fromName":"Seth House","fromEmail":"seth@eseth.com","sentAt":"2021-01-27T03:37:14Z","receivedAt":"2021-01-27T03:59:47Z","isPatch":true,"sender":{"key":"seth@eseth.com","avatar":"https://avatars.githubusercontent.com/u/91293?v=4"},"body":"On Tue, Jan 26, 2021 at 12:10:17PM -0800, Junio C Hamano wrote:\n> I am OK with that \"two merge-file invocations, one with --ours and\n> then another with --theirs\" approach, as I already said in\n> https://lore.kernel.org/git/xmqqh7n9aer5.fsf@gitster.c.googlers.com/\n\nSorry if it seemed like I left that thread hanging (busy week). Thanks\nfor your reply(s). I'm finishing a v10 patchset with that change which\nI'll submit to the list for review this week.\n\n"},{"id":"415538","messageId":"xmqqh7n0vakk.fsf@gitster.c.googlers.com","threadId":"54972","inReplyTo":"20210127033714.GA16914@ellen","subject":"Re: Re* [PATCH v2] fixup! mergetool: add automerge configuration","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-01-29T00:41:31Z","receivedAt":"2021-01-29T00:42:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Seth House writes:\n\n> Sorry if it seemed like I left that thread hanging (busy week). Thanks\n> for your reply(s). I'm finishing a v10 patchset with that change which\n> I'll submit to the list for review this week.\n\nThanks.\n"}]}