git fetch https://github.com/gitgitgadget/git pr-git-1032/michaelcompressconsult/mergetoollib_is_available-v1

git fetch --no-tags https://github.com/gitgitgadget/git tag pr-git-1032/michaelcompressconsult/mergetoollib_is_available-v1

Adding David Aguilar as an area expert for help reviewing.

Thanks.

"Michael Schindler via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> From: Michael Schindler <michael@compressconsult.com>
>
> 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()
>
> Signed-off-by: Michael Schindler <michael@compressconsult.com>
> ---
>     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()
>     
>     Signed-off-by: Michael Schindler michael@compressconsult.com
>
> 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(-)
>
> diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
> index 542a6a75eb3c..8b946e585d7f 100644
> --- a/git-mergetool--lib.sh
> +++ b/git-mergetool--lib.sh
> @@ -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.

On Mon, Jun 7, 2021 at 1:52 PM Michael Schindler via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> From: Michael Schindler <michael@compressconsult.com>
>
> 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

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/.

>
> 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".

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".
"""

>
> 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()

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().

"""

>
> Signed-off-by: Michael Schindler <michael@compressconsult.com>
> 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(-)
>
> diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
> index 542a6a75eb3c..8b946e585d7f 100644
> --- a/git-mergetool--lib.sh
> +++ b/git-mergetool--lib.sh
> @@ -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 to
git-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,
-- 
David

On Mon, 30 Aug 2021 at 01:09, David Aguilar <davvid@gmail.com> wrote:
> 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?

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.