| Message ID | pull.1760.v2.git.1720739496.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | var(win32): do report the GIT_SHELL_PATH that is actually used | expand |
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) >
Phillip Wood <phillip.wood123@gmail.com> writes: > 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. Ditto. > I've left a couple of comments but I think this is good as it > is. > > Thanks Thanks, both.
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. 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)