Re: [PATCH v2 0/7] var(win32): do report the GIT_SHELL_PATH that is actually used

[Phillip Wood <phillip.wood123@gmail.com>]

Hi Johannes

On 12/07/2024 00:11, Johannes Schindelin via GitGitGadget wrote:
> Changes since v1:
> 
>   * This patch series now shares the logic that determines the path of the
>     Unix shell that Git uses between prepare_shell_cmd() and git var
>     GIT_SHELL_PATH.

This was a pleasant read, each step was well described and easy to 
follow. I've left a couple of comments but I think this is good as it is.

Thanks

Phillip

> Johannes Schindelin (7):
>    run-command: refactor getting the Unix shell path into its own
>      function
>    strvec: declare the `strvec_push_nodup()` function globally
>    win32: override `fspathcmp()` with a directory separator-aware version
>    mingw(is_msys2_sh): handle forward slashes in the `sh.exe` path, too
>    run-command(win32): resolve the path to the Unix shell early
>    run-command: declare the `git_shell_path()` function globally
>    var(win32): do report the GIT_SHELL_PATH that is actually used
> 
>   builtin/var.c             |  3 ++-
>   compat/mingw.c            |  2 +-
>   compat/win32/path-utils.c | 25 +++++++++++++++++++++++++
>   compat/win32/path-utils.h |  2 ++
>   dir.c                     |  2 +-
>   dir.h                     |  2 +-
>   git-compat-util.h         |  5 +++++
>   run-command.c             | 17 ++++++++++++-----
>   run-command.h             |  5 +++++
>   strvec.c                  |  2 +-
>   strvec.h                  |  3 +++
>   t/t0007-git-var.sh        |  2 +-
>   12 files changed, 59 insertions(+), 11 deletions(-)
> 
> 
> base-commit: 06e570c0dfb2a2deb64d217db78e2ec21672f558
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1760%2Fdscho%2Fgit-var-on-windows-v2
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1760/dscho/git-var-on-windows-v2
> Pull-Request: https://github.com/gitgitgadget/git/pull/1760
> 
> Range-diff vs v1:
> 
>   -:  ----------- > 1:  e0970381042 run-command: refactor getting the Unix shell path into its own function
>   -:  ----------- > 2:  91ebccbc39f strvec: declare the `strvec_push_nodup()` function globally
>   -:  ----------- > 3:  a718183bb3b win32: override `fspathcmp()` with a directory separator-aware version
>   -:  ----------- > 4:  f04cfd91bd9 mingw(is_msys2_sh): handle forward slashes in the `sh.exe` path, too
>   -:  ----------- > 5:  707daf246bd run-command(win32): resolve the path to the Unix shell early
>   -:  ----------- > 6:  a74a7b4bb11 run-command: declare the `git_shell_path()` function globally
>   1:  ef62c3fc122 ! 7:  8bfd23cfa00 var(win32): do report the GIT_SHELL_PATH that is actually used
>       @@ Commit message
>            associated with the current directory.
>        
>            To that end, Git does not actually use the path `/bin/sh` that is
>       -    recorded e.g. in Unix shell scripts' hash-bang lines. Instead, as of
>       -    776297548e (Do not use SHELL_PATH from build system in prepare_shell_cmd
>       -    on Windows, 2012-04-17), it re-interprets `/bin/sh` as "look up `sh` on
>       -    the `PATH` and use the result instead".
>       +    recorded e.g. when `run_command()` is called with a Unix shell
>       +    command-line. Instead, as of 776297548e (Do not use SHELL_PATH from
>       +    build system in prepare_shell_cmd on Windows, 2012-04-17), it
>       +    re-interprets `/bin/sh` as "look up `sh` on the `PATH` and use the
>       +    result instead".
>       +
>       +    This is the logic users expect to be followed when running `git var
>       +    GIT_SHELL_PATH`.
>        
>            However, when 1e65721227 (var: add support for listing the shell,
>            2023-06-27) introduced support for `git var GIT_SHELL_PATH`, Windows was
>            not special-cased as above, which is why it outputs `/bin/sh` even
>            though that disagrees with what Git actually uses.
>        
>       -    Let's fix this, and also adjust the corresponding test case to verify
>       -    that it actually finds a working executable.
>       +    Let's fix this by using the exact same logic as `prepare_shell_cmd()`,
>       +    adjusting the Windows-specific `git var GIT_SHELL_PATH` test case to
>       +    verify that it actually finds a working executable.
>        
>            Reported-by: Phillip Wood <phillip.wood123@gmail.com>
>            Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
>        
>         ## builtin/var.c ##
>       +@@
>       + #include "refs.h"
>       + #include "path.h"
>       + #include "strbuf.h"
>       ++#include "run-command.h"
>       +
>       + static const char var_usage[] = "git var (-l | <variable>)";
>       +
>        @@ builtin/var.c: static char *default_branch(int ident_flag UNUSED)
>         
>         static char *shell_path(int ident_flag UNUSED)
>         {
>       -+#ifdef WIN32
>       -+	char *p = locate_in_PATH("sh");
>       -+	convert_slashes(p);
>       -+	return p;
>       -+#else
>       - 	return xstrdup(SHELL_PATH);
>       -+#endif
>       +-	return xstrdup(SHELL_PATH);
>       ++	return git_shell_path();
>         }
>         
>         static char *git_attr_val_system(int ident_flag UNUSED)
>