{"thread":{"id":"55876","subject":"[PATCH] use get_merge_tool_path() also in is_available() to honor settings","startedAt":"2021-06-07T20:48:42Z","lastAt":"2021-08-31T11:01:56Z","messageCount":4,"participants":["Michael Schindler via GitGitGadget","Junio C Hamano","David Aguilar","Adam Dinwoodie"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"426662","messageId":"pull.1032.git.git.1623098845733.gitgitgadget@gmail.com","threadId":"55876","inReplyTo":null,"subject":"[PATCH] use get_merge_tool_path() also in is_available() to honor settings","fromName":"Michael Schindler via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-06-07T20:47:25Z","receivedAt":"2021-06-07T20:48:42Z","isPatch":true,"sender":{"key":"name:Michael Schindler","avatar":null},"body":"From: Michael Schindler <michael@compressconsult.com>\n\nfix the is_available test used in git mergetool --tool-help or\ngit difftool --tool-help or to check the list of tools available when no\ntool is configured/given with --tool\n\nsymtoms: the actual tool running run_merge_tool () considers the difftool.\n\"$merge_tool\".path and mergetool.\"$merge_tool\".path and if configured\nhonors these. See get_merge_tool_path () in git-mergetool--lib.sh\nIf not set use fallback: translate_merge_tool_path \"$merge_tool\".\n\nThe is_available () just uses translate_merge_tool_path \"$merge_tool\".\n\nrepo 1: Configure an invalid path in mergetool.\"$merge_tool\".path for a\ntool of your choice.\nYou will be informed that the tool is available, but when trying to use\nit will not be found because the invalid configured path is used.\n\nrepo2: Install a tool of your choice on a nonstandard place (e.g. rename\nthe program) and configure mergetool.\"$merge_tool\".path for this tool.\nYou will be informed that the tool is not available (because not found on\nstandard place), but when trying to run it will work.\n\nThis fix will make the information consistent by using get_merge_tool_path()\nalso in is_available()\n\nSigned-off-by: Michael Schindler <michael@compressconsult.com>\n---\n    use get_merge_tool_path() also in is_available() to honor settings\n    \n    fix the is_available() used in git mergetool --tool-help or git difftool\n    --tool-help or used to check the list of tools available when no tool is\n    configured/given with --tool\n    \n    symtoms: the actual tool running run_merge_tool () considers the\n    difftool.\"$merge_tool\".path and mergetool.\"$merge_tool\".path and if\n    configured honors these. See get_merge_tool_path () in\n    git-mergetool--lib.sh If not defined use fallback:\n    translate_merge_tool_path \"$merge_tool\".\n    \n    The is_available () just uses translate_merge_tool_path \"$merge_tool\".\n    \n    repo 1: Configure an invalid path in mergetool.\"$merge_tool\".path for a\n    tool of your choice. You will be informed that the tool is available,\n    but when trying to use it it will not be found because the invalid\n    configured path is used.\n    \n    repo2: Install a tool of your choice on a nonstandard place (e.g. rename\n    the program) and configure mergetool.\"$merge_tool\".path for this tool.\n    You will be informed that the tool is not available (because not found\n    on standard place), but when trying to run it will work.\n    \n    This fix will make the information consistent by using\n    get_merge_tool_path() also in is_available()\n    \n    Signed-off-by: Michael Schindler michael@compressconsult.com\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1032%2Fmichaelcompressconsult%2Fmergetoollib_is_available-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1032/michaelcompressconsult/mergetoollib_is_available-v1\nPull-Request: https://github.com/git/git/pull/1032\n\n git-mergetool--lib.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh\nindex 542a6a75eb3c..8b946e585d7f 100644\n--- a/git-mergetool--lib.sh\n+++ b/git-mergetool--lib.sh\n@@ -18,7 +18,7 @@ mode_ok () {\n }\n \n is_available () {\n-\tmerge_tool_path=$(translate_merge_tool_path \"$1\") &&\n+\tmerge_tool_path=$(get_merge_tool_path \"$1\") &&\n \ttype \"$merge_tool_path\" >/dev/null 2>&1\n }\n \n\nbase-commit: c09b6306c6ca275ed9d0348a8c8014b2ff723cfb\n-- \ngitgitgadget\n"},{"id":"426686","messageId":"xmqq7dj5rvme.fsf@gitster.g","threadId":"55876","inReplyTo":"pull.1032.git.git.1623098845733.gitgitgadget@gmail.com","subject":"Re: [PATCH] use get_merge_tool_path() also in is_available() to honor settings","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-06-08T01:30:17Z","receivedAt":"2021-06-08T01:30:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Adding David Aguilar as an area expert for help reviewing.\n\nThanks.\n\n\"Michael Schindler via GitGitGadget\" <gitgitgadget@gmail.com>\nwrites:\n\n> From: Michael Schindler <michael@compressconsult.com>\n>\n> fix the is_available test used in git mergetool --tool-help or\n> git difftool --tool-help or to check the list of tools available when no\n> tool is configured/given with --tool\n>\n> symtoms: the actual tool running run_merge_tool () considers the difftool.\n> \"$merge_tool\".path and mergetool.\"$merge_tool\".path and if configured\n> honors these. See get_merge_tool_path () in git-mergetool--lib.sh\n> If not set use fallback: translate_merge_tool_path \"$merge_tool\".\n>\n> The is_available () just uses translate_merge_tool_path \"$merge_tool\".\n>\n> repo 1: Configure an invalid path in mergetool.\"$merge_tool\".path for a\n> tool of your choice.\n> You will be informed that the tool is available, but when trying to use\n> it will not be found because the invalid configured path is used.\n>\n> repo2: Install a tool of your choice on a nonstandard place (e.g. rename\n> the program) and configure mergetool.\"$merge_tool\".path for this tool.\n> You will be informed that the tool is not available (because not found on\n> standard place), but when trying to run it will work.\n>\n> This fix will make the information consistent by using get_merge_tool_path()\n> also in is_available()\n>\n> Signed-off-by: Michael Schindler <michael@compressconsult.com>\n> ---\n>     use get_merge_tool_path() also in is_available() to honor settings\n>     \n>     fix the is_available() used in git mergetool --tool-help or git difftool\n>     --tool-help or used to check the list of tools available when no tool is\n>     configured/given with --tool\n>     \n>     symtoms: the actual tool running run_merge_tool () considers the\n>     difftool.\"$merge_tool\".path and mergetool.\"$merge_tool\".path and if\n>     configured honors these. See get_merge_tool_path () in\n>     git-mergetool--lib.sh If not defined use fallback:\n>     translate_merge_tool_path \"$merge_tool\".\n>     \n>     The is_available () just uses translate_merge_tool_path \"$merge_tool\".\n>     \n>     repo 1: Configure an invalid path in mergetool.\"$merge_tool\".path for a\n>     tool of your choice. You will be informed that the tool is available,\n>     but when trying to use it it will not be found because the invalid\n>     configured path is used.\n>     \n>     repo2: Install a tool of your choice on a nonstandard place (e.g. rename\n>     the program) and configure mergetool.\"$merge_tool\".path for this tool.\n>     You will be informed that the tool is not available (because not found\n>     on standard place), but when trying to run it will work.\n>     \n>     This fix will make the information consistent by using\n>     get_merge_tool_path() also in is_available()\n>     \n>     Signed-off-by: Michael Schindler michael@compressconsult.com\n>\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1032%2Fmichaelcompressconsult%2Fmergetoollib_is_available-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1032/michaelcompressconsult/mergetoollib_is_available-v1\n> Pull-Request: https://github.com/git/git/pull/1032\n>\n>  git-mergetool--lib.sh | 2 +-\n>  1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh\n> index 542a6a75eb3c..8b946e585d7f 100644\n> --- a/git-mergetool--lib.sh\n> +++ b/git-mergetool--lib.sh\n> @@ -18,7 +18,7 @@ mode_ok () {\n>  }\n>  \n>  is_available () {\n> -\tmerge_tool_path=$(translate_merge_tool_path \"$1\") &&\n> +\tmerge_tool_path=$(get_merge_tool_path \"$1\") &&\n>  \ttype \"$merge_tool_path\" >/dev/null 2>&1\n>  }\n>  \n>\n> base-commit: c09b6306c6ca275ed9d0348a8c8014b2ff723cfb\n"},{"id":"434037","messageId":"CAJDDKr4rXEZDH4ZNU1ieUcfpOseikOVVfFQ=1tG9YNr2nibLCQ@mail.gmail.com","threadId":"55876","inReplyTo":"pull.1032.git.git.1623098845733.gitgitgadget@gmail.com","subject":"Re: [PATCH] use get_merge_tool_path() also in is_available() to honor settings","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2021-08-30T00:08:42Z","receivedAt":"2021-08-30T00:09:22Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"Sorry for taking so long to review this change and thanks for taking a\nstab at this. Comments inline below.\n\nOn Mon, Jun 7, 2021 at 1:52 PM Michael Schindler via GitGitGadget\n<gitgitgadget@gmail.com> wrote:\n>\n> From: Michael Schindler <michael@compressconsult.com>\n>\n> fix the is_available test used in git mergetool --tool-help or\n> git difftool --tool-help or to check the list of tools available when no\n> tool is configured/given with --tool\n\nNit: the commit message can probably be fixed up a bit. Capitalize the\nsentence and add \".\" full stops at the end.\n\nThere are some minor typos as well ~ s/symtoms/symptoms/.\n\n>\n> symtoms: the actual tool running run_merge_tool () considers the difftool.\n> \"$merge_tool\".path and mergetool.\"$merge_tool\".path and if configured\n> honors these. See get_merge_tool_path () in git-mergetool--lib.sh\n> If not set use fallback: translate_merge_tool_path \"$merge_tool\".\n>\n> The is_available () just uses translate_merge_tool_path \"$merge_tool\".\n\nI think we can focus on explaining the intent of the patch. Maybe\nsomething like..\n\n\"\"\"\nTeach the is_available() function to translate mergetool paths to handle the\ncase where the user has configured mergetool.<tool>.path. This fixes the\nreported tools as displayed by \"git difftool --tool-help\".\n\"\"\"\n\n>\n> repo 1: Configure an invalid path in mergetool.\"$merge_tool\".path for a\n> tool of your choice.\n> You will be informed that the tool is available, but when trying to use\n> it will not be found because the invalid configured path is used.\n>\n> repo2: Install a tool of your choice on a nonstandard place (e.g. rename\n> the program) and configure mergetool.\"$merge_tool\".path for this tool.\n> You will be informed that the tool is not available (because not found on\n> standard place), but when trying to run it will work.\n>\n> This fix will make the information consistent by using get_merge_tool_path()\n> also in is_available()\n\nThere's not really a necessity to have two repos here. We could trim down these\nnotes to only mention the reproduction steps.\n\nMaybe something along the lines of,\n\n\"\"\"\nA discrepancy occurs when configuring invalid paths in mergetool.<tool>.path\nor when tools are not available in the default $PATH and mergetool.<tool>.path\ncontains the correct location.\n\nWhen mergetool.<tool>.path contains invalid paths then you will be informed that\nthe tool is available. It will not be found when you try to use it,\nthough, because\nthe invalid configured path is used.\n\nWhen mergetool.<tool>.path is configured to a custom path, and the tool does not\nexist in the default $PATH, you will be informed that the tool is not available.\nYet it will actually work when trying to run it.\n\nFix the discrepancy by using the configured paths in is_available().\n\n\"\"\"\n\n>\n> Signed-off-by: Michael Schindler <michael@compressconsult.com>\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1032%2Fmichaelcompressconsult%2Fmergetoollib_is_available-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1032/michaelcompressconsult/mergetoollib_is_available-v1\n> Pull-Request: https://github.com/git/git/pull/1032\n>\n>  git-mergetool--lib.sh | 2 +-\n>  1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh\n> index 542a6a75eb3c..8b946e585d7f 100644\n> --- a/git-mergetool--lib.sh\n> +++ b/git-mergetool--lib.sh\n> @@ -18,7 +18,7 @@ mode_ok () {\n>  }\n>\n>  is_available () {\n> -       merge_tool_path=$(translate_merge_tool_path \"$1\") &&\n> +       merge_tool_path=$(get_merge_tool_path \"$1\") &&\n>         type \"$merge_tool_path\" >/dev/null 2>&1\n>  }\n\nThanks, you're on the right track. There's only a few minor\nadjustments that're needed to help move this forward.\n\nRunning \"git difftool --tool-help\" with this patch applied prints a\nbunch of new error messages:\n\"\"\"\n   The diff tool ecmerge is not available as 'ecmerge'\n   The diff tool ... is not available as ... (dozens of times)\n   ...\n\"\"\"\n\nThe root cause is that get_merge_tool_path contains an \"exit 1\" and\nerror reporting when given an invalid tool.\nThus, it's not really suitable for this use case where we want to just\ndo the translation.\n\nI think the path forward would be one of the following two options.\n\nOption 1) split get_merge_tool_path into two functions. The wrapper can stay as\nget_merge_tool_path() and a new helper function can handle the lookup\n(the remainder of the function after the error checking).\n\nMaybe the helper (without the \"exit 1\" and error print) can be called\nlookup_merge_tool_path()\nor maybe get_configured_merge_tool_path() to make it clear its purpose\nis to get the configured paths.\n\nThis helper can then be used in is_available() since it will not\ncontain the error printing and exit call.\nThe error checking will remain in get_merge_tool_path() only.\n\n\nOption 2) I kinda prefer this one because the code is simpler.\nTechnically we're changing the\nbehavior of get_merge_tool_path(), though.\n\nMove the error reporting and exit 1 into run_merge_tool() since it's\nthe only caller that\ncares about that stuff, and then remove it from get_merge_tool_path()\nso that it can be\nused inside is_available() like you've done here.\n\nWe have get_merge_tool_path documented in Documentation/git-mergetool--lib.txt\nso it's a \"semi-public\" function. In practice it's really only for\nmergetool/difftool's internal use.\n\nThe docs say, \"This is not a command the end user would want to run.  Ever.\nThis documentation is meant for people who are studying the\nPorcelain-ish scripts and/or are writing new ones.\"\n\nWith that in mind, it sounds like we don't really have to consider\nexternal users because\nit's only for people writing (in-tree) porcelain-ish scripts.\n\nWe never documented that it prints errors and exits on error, but the\nfear is that\nHyrum's Law will bite because someone out there relies on the current behavior.\n\nDespite what I interpret as mergetool--lib having \"internal\" status, I\ndid a search on github\nto see if there are other users and all I got was hundreds of pages of\nresults pointing to\ngit-mergetool--lib.sh. I think we're mostly okay but I'll defer to\nJunio and the list\nas to whether we're comfortable with entertaining Option 2.\n\nOption 2) seems okay to me if we agree that mergetool--lib is\nconsidered \"internal\".\n\n\nOh, and sorry for inserting this random question (not directed at you,\nbut rather at the whole list) --\n\nIs there a reliable shell idiom for detecting Windows / Git for Windows?\n\nI noticed that our error messages are now printing \"kdiff3.exe\" on\nLinux/unix/macOS.\n\nThe root cause is we do some win32 kdiff3 -> kdiff3.exe translation\nwhen \"kdiff3\" is not found,\nbut we do it unconditionally. This is a cosmetic issue that only shows\nup when the tool is missing.\nIt still works correctly when kdiff3 is installed.\n\nI'd like to extend 47eb4c6890 (mergetools/kdiff3: make kdiff3 work on\nWindows too, 2021-06-07)\nbut in order to do so I need a reliable way to special-case Windows so\nthat we only do the x -> x.exe translation there.\n\nWhat's the best way to detect Windows? t/test-lib.sh seems to detect\nWindows using \"uname -s\" and\nchecking for matches against *MINGW* or *CYGWIN*. Is that reliable /\nrecommended?\n\ncheers,\n-- \nDavid\n"},{"id":"434259","messageId":"CA+kUOamRLtsArhjMpo7A28=6vPabry8KsjM022gZ4nxDj0wU-Q@mail.gmail.com","threadId":"55876","inReplyTo":"CAJDDKr4rXEZDH4ZNU1ieUcfpOseikOVVfFQ=1tG9YNr2nibLCQ@mail.gmail.com","subject":"Re: [PATCH] use get_merge_tool_path() also in is_available() to honor settings","fromName":"Adam Dinwoodie","fromEmail":"adam@dinwoodie.org","sentAt":"2021-08-31T11:01:23Z","receivedAt":"2021-08-31T11:01:56Z","isPatch":true,"sender":{"key":"adam@dinwoodie.org","avatar":"https://avatars.githubusercontent.com/u/1397507?v=4"},"body":"On Mon, 30 Aug 2021 at 01:09, David Aguilar <davvid@gmail.com> wrote:\n> I'd like to extend 47eb4c6890 (mergetools/kdiff3: make kdiff3 work on\n> Windows too, 2021-06-07)\n> but in order to do so I need a reliable way to special-case Windows so\n> that we only do the x -> x.exe translation there.\n>\n> What's the best way to detect Windows? t/test-lib.sh seems to detect\n> Windows using \"uname -s\" and\n> checking for matches against *MINGW* or *CYGWIN*. Is that reliable /\n> recommended?\n\nI don't think there's a single canonical way to do this, but that's\ncertainly an effective way.\n\nHowever I'd suggest not adding special-case code for Cygwin here, only\nfor MinGW: the general design principles for Cygwin are such that\n*nix-native code should require minimal changes to work on Cygwin, and\nin particular the Cygwin compatibility layer should look after adding\nthat extension, rather than requiring the application code to handle\nthe special case. Both approaches would work, but I think it makes\nsense to follow the general Cygwin design principle here of not adding\nunnecessary special casing to handle it, and to leave it to the Cygwin\ncompatibility layer instead.\n"}]}