Re: [PATCH v2 0/7] var(win32): do report the GIT_SHELL_PATH that is actually used
- From
- Phillip Wood <phillip.wood123@gmail.com>
- Date
- Jul 12, 2024, 13:51 UTC
- Message-ID
- <3b687707-7aa7-4f4f-8043-0e31fb0c09ce@gmail.com>
- In-Reply-To
- <pull.1760.v2.git.1720739496.gitgitgadget@gmail.com>
Hi Johannes
On 12/07/2024 00:11, Johannes Schindelin via GitGitGadget wrote:
Show 5 quoted lines
> 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
Show 96 quoted lines
> 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)
>