use get_merge_tool_path() also in is_available() to honor settings
PATCH 1/1 use get_merge_tool_path() also in is_available() to honor settings
From: Michael Schindler <[email protected]>
fix the is_available test used in git mergetool --tool-help or git difftool --tool-help or to check the list of tools available when no tool is configured/given with --tool
symtoms: the actual tool running run_merge_tool () considers the difftool. "$merge_tool".path and mergetool."$merge_tool".path and if configured honors these. See get_merge_tool_path () in git-mergetool--lib.sh If not set use fallback: translate_merge_tool_path "$merge_tool".
The is_available () just uses translate_merge_tool_path "$merge_tool".
repo 1: Configure an invalid path in mergetool."$merge_tool".path for a tool of your choice.
You will be informed that the tool is available, but when trying to use it will not be found because the invalid configured path is used.
repo2: Install a tool of your choice on a nonstandard place (e.g. rename the program) and configure mergetool."$merge_tool".path for this tool. You will be informed that the tool is not available (because not found on standard place), but when trying to run it will work.
This fix will make the information consistent by using get_merge_tool_path() also in is_available()
---
use get_merge_tool_path() also in is_available() to honor settings
fix the is_available() used in git mergetool --tool-help or git difftool --tool-help or used to check the list of tools available when no tool is configured/given with --tool
symtoms: the actual tool running run_merge_tool () considers the difftool."$merge_tool".path and mergetool."$merge_tool".path and if configured honors these. See get_merge_tool_path () in git-mergetool--lib.sh If not defined use fallback:
translate_merge_tool_path "$merge_tool".
The is_available () just uses translate_merge_tool_path "$merge_tool".
repo 1: Configure an invalid path in mergetool."$merge_tool".path for a tool of your choice. You will be informed that the tool is available, but when trying to use it it will not be found because the invalid configured path is used.
repo2: Install a tool of your choice on a nonstandard place (e.g. rename the program) and configure mergetool."$merge_tool".path for this tool. You will be informed that the tool is not available (because not found on standard place), but when trying to run it will work.
This fix will make the information consistent by using get_merge_tool_path() also in is_available()
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1032%2Fmichaelcompressconsult%2Fmergetoollib_is_available-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1032/michaelcompressconsult/mergetoollib_is_available-v1
Pull-Request: https://github.com/git/git/pull/1032
git-mergetool--lib.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
git-mergetool--lib.sh +1 -1
@@ -18,7 +18,7 @@ mode_ok () { } is_available () { - merge_tool_path=$(translate_merge_tool_path "$1") && + merge_tool_path=$(get_merge_tool_path "$1") && type "$merge_tool_path" >/dev/null 2>&1 }
base-commit: c09b6306c6ca275ed9d0348a8c8014b2ff723cfb
Signature · 1 line
gitgitgadget
Adding David Aguilar as an area expert for help reviewing.
Thanks.
"Michael Schindler via GitGitGadget" <[email protected]>
Quoted 79 lines · writes:
From: Michael Schindler <[email protected]>
fix the
is_availabletest used in git mergetool --tool-help or git difftool --tool-help or to check the list of tools available when no tool is configured/given with --toolsymtoms: the actual tool running
run_merge_tool() considers the difftool. "$merge_tool".path and mergetool."$merge_tool".path and if configured honors these. Seeget_merge_tool_path() ingit-mergetool--lib.shIf not set use fallback:translate_merge_tool_path"$merge_tool".The
is_available() just usestranslate_merge_tool_path"$merge_tool".repo 1: Configure an invalid path in mergetool."$merge_tool".path for a tool of your choice.
You will be informed that the tool is available, but when trying to use it will not be found because the invalid configured path is used.repo2: Install a tool of your choice on a nonstandard place (e.g. rename the program) and configure mergetool."$merge_tool".path for this tool. You will be informed that the tool is not available (because not found on standard place), but when trying to run it will work.
This fix will make the information consistent by using
get_merge_tool_path()also inis_available()Signed-off-by Michael Schindler <[email protected]>---
useget_merge_tool_path()also inis_available()to honor settingsfix the
is_available()used in git mergetool --tool-help or git difftool --tool-help or used to check the list of tools available when no tool is configured/given with --toolsymtoms: the actual tool running
run_merge_tool() considers the difftool."$merge_tool".path and mergetool."$merge_tool".path and if configured honors these. Seeget_merge_tool_path() ingit-mergetool--lib.shIf not defined use fallback:
translate_merge_tool_path"$merge_tool".The
is_available() just usestranslate_merge_tool_path"$merge_tool".repo 1: Configure an invalid path in mergetool."$merge_tool".path for a tool of your choice. You will be informed that the tool is available, but when trying to use it it will not be found because the invalid configured path is used.
repo2: Install a tool of your choice on a nonstandard place (e.g. rename the program) and configure mergetool."$merge_tool".path for this tool. You will be informed that the tool is not available (because not found on standard place), but when trying to run it will work.
This fix will make the information consistent by using
get_merge_tool_path()also inis_available()Signed-off-by Michael Schindler [email protected]Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1032%2Fmichaelcompressconsult%2Fmergetoollib_is_available-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1032/michaelcompressconsult/mergetoollib_is_available-v1
Pull-Request: https://github.com/git/git/pull/1032git-mergetool--lib.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)git-mergetool--lib.sh +1
-1@@ -18,7 +18,7 @@ mode_ok () { } is_available () { - merge_tool_path=$(translate_merge_tool_path "$1") && + merge_tool_path=$(get_merge_tool_path "$1") && type "$merge_tool_path" >/dev/null 2>&1 }base-commit:
c09b6306c6ca275ed9d0348a8c8014b2ff723cfb
Sorry for taking so long to review this change and thanks for taking a stab at this. Comments inline below.
Quoted 6 lines · Michael Schindler via GitGitGadget
From: Michael Schindler <[email protected]>
fix the
is_availabletest used in git mergetool --tool-help or git difftool --tool-help or to check the list of tools available when no tool is configured/given with --tool
Nit: the commit message can probably be fixed up a bit. Capitalize the sentence and add "." full stops at the end.
There are some minor typos as well ~ s/symtoms/symptoms/.
Quoted 7 lines
symtoms: the actual tool running
run_merge_tool() considers the difftool. "$merge_tool".path and mergetool."$merge_tool".path and if configured honors these. Seeget_merge_tool_path() ingit-mergetool--lib.shIf not set use fallback:translate_merge_tool_path"$merge_tool".The
is_available() just usestranslate_merge_tool_path"$merge_tool".
I think we can focus on explaining the intent of the patch. Maybe something like..
"""
Teach the is_available() function to translate mergetool paths to handle the case where the user has configured mergetool.<tool>.path. This fixes the reported tools as displayed by "git difftool --tool-help".
"""
Quoted 6 lines
repo 1: Configure an invalid path in mergetool."$merge_tool".path for a tool of your choice.
You will be informed that the tool is available, but when trying to use it will not be found because the invalid configured path is used.
Quoted 7 lines
repo2: Install a tool of your choice on a nonstandard place (e.g. rename the program) and configure mergetool."$merge_tool".path for this tool. You will be informed that the tool is not available (because not found on standard place), but when trying to run it will work.
This fix will make the information consistent by using
get_merge_tool_path()also inis_available()
There's not really a necessity to have two repos here. We could trim down these notes to only mention the reproduction steps.
Maybe something along the lines of,
"""
A discrepancy occurs when configuring invalid paths in mergetool.<tool>.path or when tools are not available in the default $PATH and mergetool.<tool>.path contains the correct location.
When mergetool.<tool>.path contains invalid paths then you will be informed that the tool is available. It will not be found when you try to use it, though, because
the invalid configured path is used.
When mergetool.<tool>.path is configured to a custom path, and the tool does not exist in the default $PATH, you will be informed that the tool is not available. Yet it will actually work when trying to run it.
Fix the discrepancy by using the configured paths in is_available().
"""
Quoted 14 lines
Signed-off-by Michael Schindler <[email protected]>Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1032%2Fmichaelcompressconsult%2Fmergetoollib_is_available-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1032/michaelcompressconsult/mergetoollib_is_available-v1
Pull-Request: https://github.com/git/git/pull/1032git-mergetool--lib.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)git-mergetool--lib.sh +0
-0
git-mergetool--lib.sh mode_ok () { · line 23
Quoted 7 lines
git-mergetool--lib.sh +1
-1@@ -18,7 +18,7 @@ mode_ok () { } is_available () { - merge_tool_path=$(translate_merge_tool_path "$1") && + merge_tool_path=$(get_merge_tool_path "$1") && type "$merge_tool_path" >/dev/null 2>&1 }
Thanks, you're on the right track. There's only a few minor adjustments that're needed to help move this forward.
Running "git difftool --tool-help" with this patch applied prints a bunch of new error messages:
"""
The diff tool ecmerge is not available as 'ecmerge' The diff tool ... is not available as ... (dozens of times)
...
"""
The root cause is that get_merge_tool_path contains an "exit 1" and error reporting when given an invalid tool.
Thus, it's not really suitable for this use case where we want to just do the translation.
I think the path forward would be one of the following two options.
Option 1) split get_merge_tool_path into two functions. The wrapper can stay as get_merge_tool_path() and a new helper function can handle the lookup (the remainder of the function after the error checking).
Maybe the helper (without the "exit 1" and error print) can be called lookup_merge_tool_path()
or maybe get_configured_merge_tool_path() to make it clear its purpose is to get the configured paths.
This helper can then be used in is_available() since it will not contain the error printing and exit call.
The error checking will remain in get_merge_tool_path() only.
Option 2) I kinda prefer this one because the code is simpler. Technically we're changing the
behavior of get_merge_tool_path(), though.
Move the error reporting and exit 1 into run_merge_tool() since it's the only caller that
cares about that stuff, and then remove it from get_merge_tool_path() so that it can be
used inside is_available() like you've done here.
We have get_merge_tool_path documented in Documentation/git-mergetool--lib.txt so it's a "semi-public" function. In practice it's really only for mergetool/difftool's internal use.
The docs say, "This is not a command the end user would want to run. Ever. This documentation is meant for people who are studying the Porcelain-ish scripts and/or are writing new ones."
With that in mind, it sounds like we don't really have to consider external users because
it's only for people writing (in-tree) porcelain-ish scripts.
We never documented that it prints errors and exits on error, but the fear is that
Hyrum's Law will bite because someone out there relies on the current behavior.
Despite what I interpret as mergetool--lib having "internal" status, I did a search on github
to see if there are other users and all I got was hundreds of pages of results pointing togit-mergetool--lib.sh. I think we're mostly okay but I'll defer to Junio and the list
as to whether we're comfortable with entertaining Option 2.
Option 2) seems okay to me if we agree that mergetool--lib is considered "internal".
Oh, and sorry for inserting this random question (not directed at you, but rather at the whole list) --
Is there a reliable shell idiom for detecting Windows / Git for Windows?
I noticed that our error messages are now printing "kdiff3.exe" on Linux/unix/macOS.
The root cause is we do some win32 kdiff3 -> kdiff3.exe translation when "kdiff3" is not found,
but we do it unconditionally. This is a cosmetic issue that only shows up when the tool is missing.
It still works correctly when kdiff3 is installed.
I'd like to extend 47eb4c6890 (mergetools/kdiff3: make kdiff3 work on Windows too, 2021-06-07)
but in order to do so I need a reliable way to special-case Windows so that we only do the x -> x.exe translation there.
What's the best way to detect Windows? t/test-lib.sh seems to detect Windows using "uname -s" and
checking for matches against *MINGW* or *CYGWIN*. Is that reliable / recommended?
cheers,
Signature · 1 line
David
Quoted 9 lines · David Aguilar
I'd like to extend
47eb4c6890(mergetools/kdiff3: make kdiff3 work on Windows too, 2021-06-07)
but in order to do so I need a reliable way to special-case Windows so that we only do the x -> x.exe translation there.What's the best way to detect Windows?
t/test-lib.shseems to detect Windows using "uname -s" and
checking for matches against *MINGW* or *CYGWIN*. Is that reliable / recommended?
I don't think there's a single canonical way to do this, but that's certainly an effective way.
However I'd suggest not adding special-case code for Cygwin here, only for MinGW: the general design principles for Cygwin are such that *nix-native code should require minimal changes to work on Cygwin, and in particular the Cygwin compatibility layer should look after adding that extension, rather than requiring the application code to handle the special case. Both approaches would work, but I think it makes sense to follow the general Cygwin design principle here of not adding unnecessary special casing to handle it, and to leave it to the Cygwin compatibility layer instead.