[PATCH] var(win32): do report the GIT_SHELL_PATH that is actually used

[PATCH] var(win32): do report the GIT_SHELL_PATH that is actually used

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

On Windows, Unix-like paths like `/bin/sh` make very little sense. In
the best case, they simply don't work, in the worst case they are
misinterpreted as absolute paths that are relative to the drive
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".

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.

Reported-by: Phillip Wood <phillip.wood123@gmail.com>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
    var(win32): do report the GIT_SHELL_PATH that is actually used

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1760%2Fdscho%2Fgit-var-on-windows-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1760/dscho/git-var-on-windows-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1760

 builtin/var.c      | 6 ++++++
 t/t0007-git-var.sh | 2 +-
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/builtin/var.c b/builtin/var.c
index 5dc384810c0..f4b1f34e403 100644
--- a/builtin/var.c
+++ b/builtin/var.c
@@ -51,7 +51,13 @@ 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
 }
 
 static char *git_attr_val_system(int ident_flag UNUSED)
diff --git a/t/t0007-git-var.sh b/t/t0007-git-var.sh
index ff4fd9348cc..9fc58823873 100755
--- a/t/t0007-git-var.sh
+++ b/t/t0007-git-var.sh
@@ -157,7 +157,7 @@ test_expect_success POSIXPERM 'GIT_SHELL_PATH points to a valid executable' '
 test_expect_success MINGW 'GIT_SHELL_PATH points to a suitable shell' '
 	shellpath=$(git var GIT_SHELL_PATH) &&
 	case "$shellpath" in
-	*sh) ;;
+	[A-Z]:/*/sh.exe) test -f "$shellpath";;
 	*) return 1;;
 	esac
 '

base-commit: 06e570c0dfb2a2deb64d217db78e2ec21672f558
-- 
gitgitgadget

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

[Junio C Hamano]

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> On Windows, Unix-like paths like `/bin/sh` make very little sense. In
> the best case, they simply don't work, in the worst case they are
> misinterpreted as absolute paths that are relative to the drive
> 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".

This asks for a few naïve questions.

If there is a fixed path the "git" binary was compiled for, which
can be referenced with a single variable GIT_SHELL_PATH, even though
on non-POSIX systems it won't be like /bin/sh, wouldn't there be a
path like "C:\Program Files\Git for Windows\bin\sh" (I do not do
Windows, so you may be installing somewhere completely different)
and wouldn't such a path work regardless of which drive is
associated with the current directory?

I would actually understand that, with relocatable build where
everything is relative to the installed directory, a single
GIT_SHELL_PATH that is defined at the compile-time may not make much
sense, and when you need to interpret a shell script, you may need
to recompute the actual path, relative to the installation
directory.

But I wonder why the replacement shell that is spawned is searched
for in PATH, not where you installed it (which of course would be
different from /bin/sh).  In other words, when running script that
calls for "#!/bin/sh", looking for "sh" on PATH might be a workable
hack, and it might even yield the same result, especially if you
prepend the path you installed git and bash as part of your
installation package to the user's PATH, but wouldn't it make more
sense to compute it just like how "git --exec-path" is recomputed
with the relocatable build?

The "look on the %PATH%" strategy does not make any sense as an
implementation for getting GIT_SHELL_PATH, which answers "what is
the shell this instanciation of Git was built to work with?", at
least to me.  Maybe I am missing some knowledge on limitations on
Windows and Git for Windows why it is done that way.

Thanks.

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

[Junio C Hamano]

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> diff --git a/builtin/var.c b/builtin/var.c
> index 5dc384810c0..f4b1f34e403 100644
> --- a/builtin/var.c
> +++ b/builtin/var.c
> @@ -51,7 +51,13 @@ 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
>  }

We have done well without any ugly conditional compilation in this
file so far.  If we needed to report dynamic path on certain
platforms, we'd need something a bit cleaner, like

 * rename "shell_path()" to something that is unlikely to conflict
   with others (e.g., "git_shell_path()") and make it extern

 * allow it to be overridden from compat/

which is often how we hide such an implementation quirk from the
general code paths.

>  static char *git_attr_val_system(int ident_flag UNUSED)
> diff --git a/t/t0007-git-var.sh b/t/t0007-git-var.sh
> index ff4fd9348cc..9fc58823873 100755
> --- a/t/t0007-git-var.sh
> +++ b/t/t0007-git-var.sh
> @@ -157,7 +157,7 @@ test_expect_success POSIXPERM 'GIT_SHELL_PATH points to a valid executable' '
>  test_expect_success MINGW 'GIT_SHELL_PATH points to a suitable shell' '
>  	shellpath=$(git var GIT_SHELL_PATH) &&
>  	case "$shellpath" in
> -	*sh) ;;
> +	[A-Z]:/*/sh.exe) test -f "$shellpath";;
>  	*) return 1;;
>  	esac
>  '
>
> base-commit: 06e570c0dfb2a2deb64d217db78e2ec21672f558

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

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 2775 bytes --]

On 2024-07-08 at 18:54:41, Junio C Hamano wrote:
> This asks for a few naïve questions.
> 
> If there is a fixed path the "git" binary was compiled for, which
> can be referenced with a single variable GIT_SHELL_PATH, even though
> on non-POSIX systems it won't be like /bin/sh, wouldn't there be a
> path like "C:\Program Files\Git for Windows\bin\sh" (I do not do
> Windows, so you may be installing somewhere completely different)
> and wouldn't such a path work regardless of which drive is
> associated with the current directory?
> 
> I would actually understand that, with relocatable build where
> everything is relative to the installed directory, a single
> GIT_SHELL_PATH that is defined at the compile-time may not make much
> sense, and when you need to interpret a shell script, you may need
> to recompute the actual path, relative to the installation
> directory.

I'll jump in here, and Dscho can correct me if I'm wrong, but I believe
there's one build that's always relocatable (well, there's MinGit and
the regular, but ignoring that).  You can install to almost any drive
and location, so it's not known at compile time.

> But I wonder why the replacement shell that is spawned is searched
> for in PATH, not where you installed it (which of course would be
> different from /bin/sh).  In other words, when running script that
> calls for "#!/bin/sh", looking for "sh" on PATH might be a workable
> hack, and it might even yield the same result, especially if you
> prepend the path you installed git and bash as part of your
> installation package to the user's PATH, but wouldn't it make more
> sense to compute it just like how "git --exec-path" is recomputed
> with the relocatable build?
> 
> The "look on the %PATH%" strategy does not make any sense as an
> implementation for getting GIT_SHELL_PATH, which answers "what is
> the shell this instanciation of Git was built to work with?", at
> least to me.  Maybe I am missing some knowledge on limitations on
> Windows and Git for Windows why it is done that way.

Well, it may be that that's the approach that Git for Windows takes to
look up the shell.  (I don't know for certain.)  If that _is_ what it
does, then that's absolutely the value we want because we want to use
whatever shell Git for Windows uses.  I will say it's a risky approach
because it could well also find a Cygwin or MINGW shell (or, if it were
called bash, WSL), but we really want whatever Git for Windows does
here.

That's because external users who rely on Git for Windows to furnish a
POSIX shell will want to know the path, and this variable is the best
way to do that (and the reason I added it).
-- 
brian m. carlson (they/them or he/him)
Toronto, Ontario, CA

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

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

[Junio C Hamano]

"brian m. carlson" <sandals@crustytoothpaste.net> writes:

>> The "look on the %PATH%" strategy does not make any sense as an
>> implementation for getting GIT_SHELL_PATH, which answers "what is
>> the shell this instanciation of Git was built to work with?", at
>> least to me.  Maybe I am missing some knowledge on limitations on
>> Windows and Git for Windows why it is done that way.
>
> Well, it may be that that's the approach that Git for Windows takes to
> look up the shell.  (I don't know for certain.)

> If that _is_ what it does, then that's absolutely the value we
> want because we want to use whatever shell Git for Windows uses.
> I will say it's a risky approach because it could well also find a
> Cygwin or MINGW shell (or, if it were called bash, WSL), but we
> really want whatever Git for Windows does here.

Yeah, absolutely it is risky unless it is doing the "we are
relocatable, so where is the 'sh' _we_ installed?", which is what I
would expect GIT_SHELL_PATH to be.

> That's because external users who rely on Git for Windows to furnish a
> POSIX shell will want to know the path, and this variable is the best
> way to do that (and the reason I added it).

If Git for Windows furnishes programs other than POSIX shell, paths
to which external users would also want to learn, what happens?
GIT_PERL_PATH, GIT_WISH_PATH, etc.?  Locate them on PATH themselves
shouldn't be rocket science (and instead of locating, just spawning
them themselves would be even easier, I would presume).

Thanks.

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

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 2407 bytes --]

On 2024-07-08 at 23:55:25, Junio C Hamano wrote:
> "brian m. carlson" <sandals@crustytoothpaste.net> writes:
> 
> > That's because external users who rely on Git for Windows to furnish a
> > POSIX shell will want to know the path, and this variable is the best
> > way to do that (and the reason I added it).
> 
> If Git for Windows furnishes programs other than POSIX shell, paths
> to which external users would also want to learn, what happens?
> GIT_PERL_PATH, GIT_WISH_PATH, etc.?  Locate them on PATH themselves
> shouldn't be rocket science (and instead of locating, just spawning
> them themselves would be even easier, I would presume).

In my experience, they have not always been on PATH.  It may be that
they are now, which is fantastic, but I seem to remember that at some
point bash.exe and git.exe were on PATH, but sh.exe and other commands
were not.  (There's also a different path under Git Bash than the
regular Windows PATH.)  You might say, "Well, why not use bash.exe?" and
that works great until you realize that there's also a bash.exe that is
part of WSL and will attempt to spawn WSL for you.

However, that doesn't always work, because sometimes WSL isn't installed
or is disabled or broken, and so the operation fails with an error
message.  Also, users will typically have wanted Git for Windows's bash
and not a Linux environment's bash, since the two environments will
typically have different software available.

Why not add Perl or Wish or something else?  Because once you have the
Git for Windows sh, you can use a fixed Unix path to look them up and
get a canonical Windows path with cygpath -w.  Thus, this is just the
minimal bootstrapping functionality to get that information.

Of course, on Unix, there can still be multiple Perl implementations,
but you're typically either going to build against one in particular,
which may or may not be what Git was built against, or you're going to
use /usr/bin/env and choose whatever's first in PATH.  In that case,
it's very unlikely that anyone cares what version of Perl Git actually
uses.

The only major benefit of using Git's shell on Unix is that you know it
supports POSIX functionality (which /bin/sh does not on some proprietary
Unices) and it also supports local, which may be convenient for
scripting.
-- 
brian m. carlson (they/them or he/him)
Toronto, Ontario, CA

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

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

[Phillip Wood]

Hi Johannes

On 08/07/2024 14:02, Johannes Schindelin via GitGitGadget wrote:
> From: Johannes Schindelin <johannes.schindelin@gmx.de>

Thanks for putting a patch together so quickly

>   static char *shell_path(int ident_flag UNUSED)
>   {
> +#ifdef WIN32
> +	char *p = locate_in_PATH("sh");

If I'm reading is_busybox_applet() (which only exists in 
git-for-windows) correctly then this will return "busybox.exe" under 
mingit-busybox rather than ash.exe, so the calling program would have to 
know to set argv[0] (which is likely not possible unless the calling 
program is written in C) or pass "sh" as the first argument. As the code 
to support busybox does not exist upstream I guess that's best handled 
downstream.

Best Wishes

Phillip

> +	convert_slashes(p);
> +	return p;
> +#else
>   	return xstrdup(SHELL_PATH);
> +#endif
>   }
>   
>   static char *git_attr_val_system(int ident_flag UNUSED)
> diff --git a/t/t0007-git-var.sh b/t/t0007-git-var.sh
> index ff4fd9348cc..9fc58823873 100755
> --- a/t/t0007-git-var.sh
> +++ b/t/t0007-git-var.sh
> @@ -157,7 +157,7 @@ test_expect_success POSIXPERM 'GIT_SHELL_PATH points to a valid executable' '
>   test_expect_success MINGW 'GIT_SHELL_PATH points to a suitable shell' '
>   	shellpath=$(git var GIT_SHELL_PATH) &&
>   	case "$shellpath" in
> -	*sh) ;;
> +	[A-Z]:/*/sh.exe) test -f "$shellpath";;
>   	*) return 1;;
>   	esac
>   '
> 
> base-commit: 06e570c0dfb2a2deb64d217db78e2ec21672f558

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

[Phillip Wood]

On 09/07/2024 00:40, brian m. carlson wrote:
> On 2024-07-08 at 18:54:41, Junio C Hamano wrote:
>> The "look on the %PATH%" strategy does not make any sense as an
>> implementation for getting GIT_SHELL_PATH, which answers "what is
>> the shell this instanciation of Git was built to work with?", at
>> least to me.  Maybe I am missing some knowledge on limitations on
>> Windows and Git for Windows why it is done that way.
> 
> Well, it may be that that's the approach that Git for Windows takes to
> look up the shell.  (I don't know for certain.)  If that _is_ what it
> does, then that's absolutely the value we want because we want to use
> whatever shell Git for Windows uses.

As I understand it this is the approach Git for Windows takes

> I will say it's a risky approach
> because it could well also find a Cygwin or MINGW shell (or, if it were
> called bash, WSL), but we really want whatever Git for Windows does
> here.

Git for Windows prepends the MINGW system directories to $PATH via some 
extra downstream code in setup_windows_environment() so looking up the 
shell in $PATH will find the correct executable.

Best Wishes

Phillip

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

[Junio C Hamano]

"brian m. carlson" <sandals@crustytoothpaste.net> writes:

> Why not add Perl or Wish or something else?  Because once you have the
> Git for Windows sh, you can use a fixed Unix path to look them up and
> get a canonical Windows path with cygpath -w.  Thus, this is just the
> minimal bootstrapping functionality to get that information.

Besides, perl and wish are not part of Git.  The same thing can be
said that shell is not part of Git.  

So stepping back and thinking why we have "git var GIT_SHELL_PATH",
what should it mean to begin with?  It is obviously not to tell
where we installed the thing as a part of "Git" suite, even though
Git for Windows installer may let users install the shell and other
stuff (similar to how "apt" lets users install package on Debian
derived systems).

Hence, I can accept that "git var GIT_SHELL_PATH" is not (and does
not have to be) about where we installed what we shipped.  It is
where we use the shell from (i.e., when we need "sh", we use that
shell).

   The documentation for GIT_SHELL_PATH is already good.  Sorry for
   my earlier confusion---I should have looked at it first.  It says
   it is not about what we ship, but what we use when we need to
   shell out.

I am OK with GIT_SHELL_PATH computed differently depending on the
platform, as different platform apparently use different mechanism
to decide which shell to spawn.  On POSIX systems, it is the one we
compiled to use, while on Windows it is the one that happens to be
on the end-user's PATH.

But then the comment I made in my earlier review still stands that
such a platform dependent logic should be implemented a bit more
cleanly than the posted patch.

"Which shell do we use at runtime" should influence a lot of what
the things in run-command.c do, so perhaps

 - we remove builtin/var.c:shell_path()

 - We create run-command.c:git_shell_path() immediately above
   run-command.c:prepare_shell_cmd().  We will add conditional
   compilation there (i.e. #ifdef GIT_WINDOWS_NATIVE).  The default
   implementation is to return SHELL_PATH, and on Windows it looks
   for "sh" on %PATH%.

 - The entry for GIT_SHELL_PATH in builtin/var.c:git_vars[] should
   be updated to point at git_shell_path() in run-command.c

 - Near the beginning of run-command.c:prepare_shell_cmd(), there is
   a conditional compilation.  If we can remove it by using
   git_shell_path(), that would be a nice bonus.

would give us a good approach for implementation?

Thanks.

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

[Johannes Schindelin]

Hi Phillip,

On Tue, 9 Jul 2024, Phillip Wood wrote:

> On 08/07/2024 14:02, Johannes Schindelin via GitGitGadget wrote:
> > From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> Thanks for putting a patch together so quickly
>
> >   static char *shell_path(int ident_flag UNUSED)
> >   {
> > +#ifdef WIN32
> > +	char *p = locate_in_PATH("sh");
>
> If I'm reading is_busybox_applet() (which only exists in git-for-windows)
> correctly then this will return "busybox.exe" under mingit-busybox rather than
> ash.exe, so the calling program would have to know to set argv[0] (which is
> likely not possible unless the calling program is written in C) or pass "sh"
> as the first argument. As the code to support busybox does not exist upstream
> I guess that's best handled downstream.

BusyBox-w32 is unfortunately displaying strange performance patterns. It
is partially (and expectedly) faster than the MSYS2 Bash, but in other
scenarios it is substantially slower (which is totally unexpected).

Some time ago, I tried to make this all work and investigate the
unexpected performance issues (and hoped to fix them, too), but ran out of
time [*1*]. That was almost two years ago, and I am unsure whether I will
ever be able to elevate the BusyBox flavor of MinGit to a non-experimental
state.

My original plan was to eventually no longer include `busybox.exe` in
the mingit-busybox packages, but instead a copy of that executable with
the name `sh.exe` and thereby have it work without that hack in the Git
code to call the `busybox.exe` with the `sh` argument inserted before the
regular command-line arguments.

In the context of the patch (or now: patch series) at hand, I don't think
we need to let BusyBox play any role.

Ciao,
Johannes

Footnote *1*: Interested parties can find the latest state here:
https://github.com/git-for-windows/git/compare/main...dscho:git:busybox

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

[Johannes Schindelin via GitGitGadget]

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)

-- 
gitgitgadget

[PATCH v2 1/7] run-command: refactor getting the Unix shell path into its own function

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

This encapsulates the platform-specific logic better.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 run-command.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/run-command.c b/run-command.c
index d9f80fabe6e..59e433bf91c 100644
--- a/run-command.c
+++ b/run-command.c
@@ -274,17 +274,22 @@ int sane_execvp(const char *file, char * const argv[])
 	return -1;
 }
 
+static const char *git_shell_path(void)
+{
+#ifndef GIT_WINDOWS_NATIVE
+	return SHELL_PATH;
+#else
+	return "sh";
+#endif
+}
+
 static const char **prepare_shell_cmd(struct strvec *out, const char **argv)
 {
 	if (!argv[0])
 		BUG("shell command is empty");
 
 	if (strcspn(argv[0], "|&;<>()$`\\\"' \t\n*?[#~=%") != strlen(argv[0])) {
-#ifndef GIT_WINDOWS_NATIVE
-		strvec_push(out, SHELL_PATH);
-#else
-		strvec_push(out, "sh");
-#endif
+		strvec_push(out, git_shell_path());
 		strvec_push(out, "-c");
 
 		/*
-- 
gitgitgadget

[PATCH v2 2/7] strvec: declare the `strvec_push_nodup()` function globally

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

This function differs from `strvec_push()` in that it takes ownership of
the allocated string that is passed as second argument.

This is useful when appending elements to the string array that have
been freshly allocated and serve no further other purpose after that.

Without declaring this function globally, call sites would allocate the
memory, only to have `strvec_push()` duplicate the string, and then the
first copy would need to be released. Having this function globally
avoids that kind of unnecessary work.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 strvec.c | 2 +-
 strvec.h | 3 +++
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/strvec.c b/strvec.c
index d4073ec9fa2..f712070f574 100644
--- a/strvec.c
+++ b/strvec.c
@@ -10,7 +10,7 @@ void strvec_init(struct strvec *array)
 	memcpy(array, &blank, sizeof(*array));
 }
 
-static void strvec_push_nodup(struct strvec *array, const char *value)
+void strvec_push_nodup(struct strvec *array, char *value)
 {
 	if (array->v == empty_strvec)
 		array->v = NULL;
diff --git a/strvec.h b/strvec.h
index 6c7e8b7d503..4b73c1f092e 100644
--- a/strvec.h
+++ b/strvec.h
@@ -46,6 +46,9 @@ void strvec_init(struct strvec *);
 /* Push a copy of a string onto the end of the array. */
 const char *strvec_push(struct strvec *, const char *);
 
+/* Push an allocated string onto the end of the array, taking ownership. */
+void strvec_push_nodup(struct strvec *array, char *value);
+
 /**
  * Format a string and push it onto the end of the array. This is a
  * convenience wrapper combining `strbuf_addf` and `strvec_push`.
-- 
gitgitgadget

[PATCH v2 3/7] win32: override `fspathcmp()` with a directory separator-aware version

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

On Windows, the backslash is the directory separator, even if the
forward slash can be used, too, at least since Windows NT.

This means that the paths `a/b` and `a\b` are equivalent, and
`fspathcmp()` needs to be made aware of that fact.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 compat/win32/path-utils.c | 25 +++++++++++++++++++++++++
 compat/win32/path-utils.h |  2 ++
 dir.c                     |  2 +-
 dir.h                     |  2 +-
 git-compat-util.h         |  5 +++++
 5 files changed, 34 insertions(+), 2 deletions(-)

diff --git a/compat/win32/path-utils.c b/compat/win32/path-utils.c
index ebf2f12eb66..af7ef957bbf 100644
--- a/compat/win32/path-utils.c
+++ b/compat/win32/path-utils.c
@@ -50,3 +50,28 @@ int win32_offset_1st_component(const char *path)
 
 	return pos + is_dir_sep(*pos) - path;
 }
+
+int win32_fspathcmp(const char *a, const char *b)
+{
+	int diff;
+
+	for (;;) {
+		if (!*a)
+			return !*b ? 0 : -1;
+		if (!*b)
+			return +1;
+
+		if (is_dir_sep(*a)) {
+			if (!is_dir_sep(*b))
+				return -1;
+			a++;
+			b++;
+			continue;
+		} else if (is_dir_sep(*b))
+			return +1;
+
+		diff = (unsigned char)tolower(*(a++)) - (unsigned char)tolower(*(b++));
+		if (diff)
+			return diff;
+	}
+}
diff --git a/compat/win32/path-utils.h b/compat/win32/path-utils.h
index 65fa3b9263a..e3c2a79df74 100644
--- a/compat/win32/path-utils.h
+++ b/compat/win32/path-utils.h
@@ -29,5 +29,7 @@ static inline int win32_has_dir_sep(const char *path)
 #define has_dir_sep(path) win32_has_dir_sep(path)
 int win32_offset_1st_component(const char *path);
 #define offset_1st_component win32_offset_1st_component
+int win32_fspathcmp(const char *a, const char *b);
+#define fspathcmp win32_fspathcmp
 
 #endif
diff --git a/dir.c b/dir.c
index b7a6625ebda..37d8e266b2c 100644
--- a/dir.c
+++ b/dir.c
@@ -95,7 +95,7 @@ int count_slashes(const char *s)
 	return cnt;
 }
 
-int fspathcmp(const char *a, const char *b)
+int git_fspathcmp(const char *a, const char *b)
 {
 	return ignore_case ? strcasecmp(a, b) : strcmp(a, b);
 }
diff --git a/dir.h b/dir.h
index 69a76d8bdd3..947e3d77442 100644
--- a/dir.h
+++ b/dir.h
@@ -541,7 +541,7 @@ int remove_dir_recursively(struct strbuf *path, int flag);
  */
 int remove_path(const char *path);
 
-int fspathcmp(const char *a, const char *b);
+int git_fspathcmp(const char *a, const char *b);
 int fspatheq(const char *a, const char *b);
 int fspathncmp(const char *a, const char *b, size_t count);
 unsigned int fspathhash(const char *str);
diff --git a/git-compat-util.h b/git-compat-util.h
index ca7678a379d..ac564a68987 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -506,6 +506,11 @@ static inline int git_offset_1st_component(const char *path)
 #define offset_1st_component git_offset_1st_component
 #endif
 
+
+#ifndef fspathcmp
+#define fspathcmp git_fspathcmp
+#endif
+
 #ifndef is_valid_path
 #define is_valid_path(path) 1
 #endif
-- 
gitgitgadget

[PATCH v2 4/7] mingw(is_msys2_sh): handle forward slashes in the `sh.exe` path, too

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

Whether the full path to the MSYS2 Bash is specified using backslashes
or forward slashes, in either case the command-line arguments need to be
quoted in the MSYS2-specific manner instead of using regular Win32
command-line quoting rules.

In preparation for `prepare_shell_cmd()` to use the full path to
`sh.exe` (with forward slashes for consistency), let's teach the
`is_msys2_sh()` function about this; Otherwise 5580.4 'clone with
backslashed path' would fail once `prepare_shell_cmd()` uses the full
path instead of merely `sh`.

This patch relies on the just-introduced fix where `fspathcmp()` handles
backslashes and forward slashes as equivalent on Windows.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 compat/mingw.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/compat/mingw.c b/compat/mingw.c
index 6097b8f9e60..29d3f09768c 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -1546,7 +1546,7 @@ static int is_msys2_sh(const char *cmd)
 		return ret;
 	}
 
-	if (ends_with(cmd, "\\sh.exe")) {
+	if (ends_with(cmd, "\\sh.exe") || ends_with(cmd, "/sh.exe")) {
 		static char *sh;
 
 		if (!sh)
-- 
gitgitgadget

[PATCH v2 5/7] run-command(win32): resolve the path to the Unix shell early

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

In 776297548e (Do not use SHELL_PATH from build system in
prepare_shell_cmd on Windows, 2012-04-17), the hard-coded path to the
Unix shell was replaced by passing `sh` instead when executing Unix
shell scripts in Git.

This was done because the hard-coded path to the Unix shell is incorrect
on Windows because it not only is a Unix-style absolute path instead of
a Windows one, but Git uses the runtime prefix feature on Windows, i.e.
the correct path cannot be hard-coded.

Naturally, the `sh` argument will be resolved to the full path of said
executable eventually.

To help fixing the bug where `git var GIT_SHELL_PATH` currently does not
reflect that logic, but shows that incorrect hard-coded Unix-style
absolute path, let's resolve the full path to the `sh` executable early
in the `git_shell_path()` function so that we can use it in `git var`,
too, and be sure that the output is equivalent to what `run_command()`
does when it is asked to execute a command-line using a Unix shell.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 run-command.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/run-command.c b/run-command.c
index 59e433bf91c..60a79db8f0e 100644
--- a/run-command.c
+++ b/run-command.c
@@ -274,12 +274,14 @@ int sane_execvp(const char *file, char * const argv[])
 	return -1;
 }
 
-static const char *git_shell_path(void)
+static char *git_shell_path(void)
 {
 #ifndef GIT_WINDOWS_NATIVE
-	return SHELL_PATH;
+	return xstrdup(SHELL_PATH);
 #else
-	return "sh";
+	char *p = locate_in_PATH("sh");
+	convert_slashes(p);
+	return p;
 #endif
 }
 
@@ -289,7 +291,7 @@ static const char **prepare_shell_cmd(struct strvec *out, const char **argv)
 		BUG("shell command is empty");
 
 	if (strcspn(argv[0], "|&;<>()$`\\\"' \t\n*?[#~=%") != strlen(argv[0])) {
-		strvec_push(out, git_shell_path());
+		strvec_push_nodup(out, git_shell_path());
 		strvec_push(out, "-c");
 
 		/*
-- 
gitgitgadget

[PATCH v2 6/7] run-command: declare the `git_shell_path()` function globally

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

The intention is to use it in `git var GIT_SHELL_PATH`, therefore we
need this function to stop being file-local only.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 run-command.c | 2 +-
 run-command.h | 5 +++++
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/run-command.c b/run-command.c
index 60a79db8f0e..45ba5449323 100644
--- a/run-command.c
+++ b/run-command.c
@@ -274,7 +274,7 @@ int sane_execvp(const char *file, char * const argv[])
 	return -1;
 }
 
-static char *git_shell_path(void)
+char *git_shell_path(void)
 {
 #ifndef GIT_WINDOWS_NATIVE
 	return xstrdup(SHELL_PATH);
diff --git a/run-command.h b/run-command.h
index 55f6631a2aa..03e7222d8b5 100644
--- a/run-command.h
+++ b/run-command.h
@@ -195,6 +195,11 @@ int is_executable(const char *name);
  */
 int exists_in_PATH(const char *command);
 
+/**
+ * Return the path that is used to execute Unix shell command-lines.
+ */
+char *git_shell_path(void);
+
 /**
  * Start a sub-process. Takes a pointer to a `struct child_process`
  * that specifies the details and returns pipe FDs (if requested).
-- 
gitgitgadget

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

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

On Windows, Unix-like paths like `/bin/sh` make very little sense. In
the best case, they simply don't work, in the worst case they are
misinterpreted as absolute paths that are relative to the drive
associated with the current directory.

To that end, Git does not actually use the path `/bin/sh` that is
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 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      | 3 ++-
 t/t0007-git-var.sh | 2 +-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/builtin/var.c b/builtin/var.c
index 5dc384810c0..e30ff45be1c 100644
--- a/builtin/var.c
+++ b/builtin/var.c
@@ -12,6 +12,7 @@
 #include "refs.h"
 #include "path.h"
 #include "strbuf.h"
+#include "run-command.h"
 
 static const char var_usage[] = "git var (-l | <variable>)";
 
@@ -51,7 +52,7 @@ static char *default_branch(int ident_flag UNUSED)
 
 static char *shell_path(int ident_flag UNUSED)
 {
-	return xstrdup(SHELL_PATH);
+	return git_shell_path();
 }
 
 static char *git_attr_val_system(int ident_flag UNUSED)
diff --git a/t/t0007-git-var.sh b/t/t0007-git-var.sh
index ff4fd9348cc..9fc58823873 100755
--- a/t/t0007-git-var.sh
+++ b/t/t0007-git-var.sh
@@ -157,7 +157,7 @@ test_expect_success POSIXPERM 'GIT_SHELL_PATH points to a valid executable' '
 test_expect_success MINGW 'GIT_SHELL_PATH points to a suitable shell' '
 	shellpath=$(git var GIT_SHELL_PATH) &&
 	case "$shellpath" in
-	*sh) ;;
+	[A-Z]:/*/sh.exe) test -f "$shellpath";;
 	*) return 1;;
 	esac
 '
-- 
gitgitgadget

Re: [PATCH v2 3/7] win32: override `fspathcmp()` with a directory separator-aware version

[Phillip Wood]

Hi Johannes

On 12/07/2024 00:11, Johannes Schindelin via GitGitGadget wrote:
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
> 
> On Windows, the backslash is the directory separator, even if the
> forward slash can be used, too, at least since Windows NT.
> 
> This means that the paths `a/b` and `a\b` are equivalent, and
> `fspathcmp()` needs to be made aware of that fact.

How does fspathncmp() behave? It would be good for the two to match. 
I've left a couple of thoughts below but I'm not sure they're worth 
re-rolling for

> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>   compat/win32/path-utils.c | 25 +++++++++++++++++++++++++
>   compat/win32/path-utils.h |  2 ++
>   dir.c                     |  2 +-
>   dir.h                     |  2 +-
>   git-compat-util.h         |  5 +++++
>   5 files changed, 34 insertions(+), 2 deletions(-)
> 
> diff --git a/compat/win32/path-utils.c b/compat/win32/path-utils.c
> index ebf2f12eb66..af7ef957bbf 100644
> --- a/compat/win32/path-utils.c
> +++ b/compat/win32/path-utils.c
> @@ -50,3 +50,28 @@ int win32_offset_1st_component(const char *path)
>   
>   	return pos + is_dir_sep(*pos) - path;
>   }
> +
> +int win32_fspathcmp(const char *a, const char *b)
> +{
> +	int diff;
> +
> +	for (;;) {
> +		if (!*a)
> +			return !*b ? 0 : -1;

Personally I'd find this easier to read as

	return *b ? -1 : 0;

> +		if (!*b)
> +			return +1;
> +
> +		if (is_dir_sep(*a)) {
> +			if (!is_dir_sep(*b))
> +				return -1;
> +			a++;
> +			b++;
> +			continue;
> +		} else if (is_dir_sep(*b))
> +			return +1;
> +
> +		diff = (unsigned char)tolower(*(a++)) - (unsigned char)tolower(*(b++));

There is a subtle difference between this and strcasecmp() because the 
latter is locale dependent but our tolower() is not because we override 
the standard library's version. As they're both useless on multibyte 
locales it probably doesn't make much difference in practice.

Thanks

Phillip

> +		if (diff)
> +			return diff;
> +	}
> +}
> diff --git a/compat/win32/path-utils.h b/compat/win32/path-utils.h
> index 65fa3b9263a..e3c2a79df74 100644
> --- a/compat/win32/path-utils.h
> +++ b/compat/win32/path-utils.h
> @@ -29,5 +29,7 @@ static inline int win32_has_dir_sep(const char *path)
>   #define has_dir_sep(path) win32_has_dir_sep(path)
>   int win32_offset_1st_component(const char *path);
>   #define offset_1st_component win32_offset_1st_component
> +int win32_fspathcmp(const char *a, const char *b);
> +#define fspathcmp win32_fspathcmp
>   
>   #endif
> diff --git a/dir.c b/dir.c
> index b7a6625ebda..37d8e266b2c 100644
> --- a/dir.c
> +++ b/dir.c
> @@ -95,7 +95,7 @@ int count_slashes(const char *s)
>   	return cnt;
>   }
>   
> -int fspathcmp(const char *a, const char *b)
> +int git_fspathcmp(const char *a, const char *b)
>   {
>   	return ignore_case ? strcasecmp(a, b) : strcmp(a, b);
>   }
> diff --git a/dir.h b/dir.h
> index 69a76d8bdd3..947e3d77442 100644
> --- a/dir.h
> +++ b/dir.h
> @@ -541,7 +541,7 @@ int remove_dir_recursively(struct strbuf *path, int flag);
>    */
>   int remove_path(const char *path);
>   
> -int fspathcmp(const char *a, const char *b);
> +int git_fspathcmp(const char *a, const char *b);
>   int fspatheq(const char *a, const char *b);
>   int fspathncmp(const char *a, const char *b, size_t count);
>   unsigned int fspathhash(const char *str);
> diff --git a/git-compat-util.h b/git-compat-util.h
> index ca7678a379d..ac564a68987 100644
> --- a/git-compat-util.h
> +++ b/git-compat-util.h
> @@ -506,6 +506,11 @@ static inline int git_offset_1st_component(const char *path)
>   #define offset_1st_component git_offset_1st_component
>   #endif
>   
> +
> +#ifndef fspathcmp
> +#define fspathcmp git_fspathcmp
> +#endif
> +
>   #ifndef is_valid_path
>   #define is_valid_path(path) 1
>   #endif

Re: [PATCH v2 4/7] mingw(is_msys2_sh): handle forward slashes in the `sh.exe` path, too

[Phillip Wood]

On 12/07/2024 00:11, Johannes Schindelin via GitGitGadget wrote:
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
> 
> Whether the full path to the MSYS2 Bash is specified using backslashes
> or forward slashes, in either case the command-line arguments need to be
> quoted in the MSYS2-specific manner instead of using regular Win32
> command-line quoting rules.
> 
> In preparation for `prepare_shell_cmd()` to use the full path to
> `sh.exe` (with forward slashes for consistency), let's teach the
> `is_msys2_sh()` function about this; Otherwise 5580.4 'clone with
> backslashed path' would fail once `prepare_shell_cmd()` uses the full
> path instead of merely `sh`.

Makes sense

> This patch relies on the just-introduced fix where `fspathcmp()` handles
> backslashes and forward slashes as equivalent on Windows.

That dependency isn't obvious from the patch but there is a call to 
fspathcmp() a couple of lines below the last context line.

Thanks

Phillip

> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>   compat/mingw.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/compat/mingw.c b/compat/mingw.c
> index 6097b8f9e60..29d3f09768c 100644
> --- a/compat/mingw.c
> +++ b/compat/mingw.c
> @@ -1546,7 +1546,7 @@ static int is_msys2_sh(const char *cmd)
>   		return ret;
>   	}
>   
> -	if (ends_with(cmd, "\\sh.exe")) {
> +	if (ends_with(cmd, "\\sh.exe") || ends_with(cmd, "/sh.exe")) {
>   		static char *sh;
>   
>   		if (!sh)

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

[Phillip Wood]

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

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

[Junio C Hamano]

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> On Windows, Unix-like paths like `/bin/sh` make very little sense. In
> the best case, they simply don't work, in the worst case they are
> misinterpreted as absolute paths that are relative to the drive
> associated with the current directory.
> To that end, ...

This does not quite explain why a hardcoded full path does not make
sense, though.  If you do not want "relative to the current drive",
you can even hardcode the path including the drive letter and now it
no longer falls into that "worst case" because there is no way to
misinterpret it as such.  I think the real reason is that the port's
"relocatable" nature does not mix well with the "let's make sure we
know what we use exactly by deciding the paths to various tools at
compile time" mentality.  So if I were rerolling this I would say

	Git ported to Windows expect to be installed at user-chosen
	places, which means hardcoded paths to tools decided at
	compile time are bad fit.

or something.

But it does not matter if the stated reason is to use the "find 'sh'
that is on PATH" approach was taken tells the whole story or not.
The important thing is that we do ...

> Git does not actually use the path `/bin/sh` that is
> 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.  And with the design constraints that it has to be
installable anywhere and cannot rely on compile-time determined
hardcoded paths, the above design decision is a very valid one.

> This is the logic users expect to be followed when running `git var
> GIT_SHELL_PATH`.

And matching `git var GIT_SHELL_PATH` to what the code path does
make sense.

> diff --git a/builtin/var.c b/builtin/var.c
> index 5dc384810c0..e30ff45be1c 100644
> --- a/builtin/var.c
> +++ b/builtin/var.c
> @@ -12,6 +12,7 @@
>  #include "refs.h"
>  #include "path.h"
>  #include "strbuf.h"
> +#include "run-command.h"
>  
>  static const char var_usage[] = "git var (-l | <variable>)";
>  
> @@ -51,7 +52,7 @@ static char *default_branch(int ident_flag UNUSED)
>  
>  static char *shell_path(int ident_flag UNUSED)
>  {
> -	return xstrdup(SHELL_PATH);
> +	return git_shell_path();
>  }

Simple and clear.  Nicely concluded.

Thanks.  Will queue.

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

[Junio C Hamano]

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.

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

[Johannes Schindelin via GitGitGadget]

Changes since v2:

 * Now fspathncmp() is overridden on Windows just like fspathcmp().
 * The win32_fspath*cmp() functions now respect core.ignoreCase.

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 | 37 +++++++++++++++++++++++++++++++++++++
 compat/win32/path-utils.h |  4 ++++
 dir.c                     |  4 ++--
 dir.h                     |  4 ++--
 git-compat-util.h         |  8 ++++++++
 run-command.c             | 17 ++++++++++++-----
 run-command.h             |  5 +++++
 strvec.c                  |  2 +-
 strvec.h                  |  3 +++
 t/t0007-git-var.sh        |  2 +-
 12 files changed, 78 insertions(+), 13 deletions(-)


base-commit: 06e570c0dfb2a2deb64d217db78e2ec21672f558
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1760%2Fdscho%2Fgit-var-on-windows-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1760/dscho/git-var-on-windows-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/1760

Range-diff vs v2:

 1:  e0970381042 = 1:  e0970381042 run-command: refactor getting the Unix shell path into its own function
 2:  91ebccbc39f = 2:  91ebccbc39f strvec: declare the `strvec_push_nodup()` function globally
 3:  a718183bb3b ! 3:  f46315ac0b2 win32: override `fspathcmp()` with a directory separator-aware version
     @@ Commit message
          This means that the paths `a/b` and `a\b` are equivalent, and
          `fspathcmp()` needs to be made aware of that fact.
      
     +    Note that we have to override both `fspathcmp()` and `fspathncmp()`, and
     +    the former cannot be a mere pre-processor constant that transforms calls
     +    to `fspathcmp(a, b)` into `fspathncmp(a, b, (size_t)-1)` because the
     +    function `report_collided_checkout()` in `unpack-trees.c` wants to
     +    assign `list.cmp = fspathcmp`.
     +
     +    Also note that `fspatheq()` does _not_ need to be overridden because it
     +    calls `fspathcmp()` internally.
     +
          Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
      
       ## compat/win32/path-utils.c ##
     +@@
     + #include "../../git-compat-util.h"
     ++#include "../../environment.h"
     + 
     + int win32_has_dos_drive_prefix(const char *path)
     + {
      @@ compat/win32/path-utils.c: int win32_offset_1st_component(const char *path)
       
       	return pos + is_dir_sep(*pos) - path;
       }
      +
     -+int win32_fspathcmp(const char *a, const char *b)
     ++int win32_fspathncmp(const char *a, const char *b, size_t count)
      +{
      +	int diff;
      +
      +	for (;;) {
     ++		if (!count--)
     ++			return 0;
      +		if (!*a)
     -+			return !*b ? 0 : -1;
     ++			return *b ? -1 : 0;
      +		if (!*b)
      +			return +1;
      +
     @@ compat/win32/path-utils.c: int win32_offset_1st_component(const char *path)
      +		} else if (is_dir_sep(*b))
      +			return +1;
      +
     -+		diff = (unsigned char)tolower(*(a++)) - (unsigned char)tolower(*(b++));
     ++		diff = ignore_case ?
     ++			(unsigned char)tolower(*a) - (int)(unsigned char)tolower(*b) :
     ++			(unsigned char)*a - (int)(unsigned char)*b;
      +		if (diff)
      +			return diff;
     ++		a++;
     ++		b++;
      +	}
     ++}
     ++
     ++int win32_fspathcmp(const char *a, const char *b)
     ++{
     ++	return win32_fspathncmp(a, b, (size_t)-1);
      +}
      
       ## compat/win32/path-utils.h ##
     @@ compat/win32/path-utils.h: static inline int win32_has_dir_sep(const char *path)
       #define offset_1st_component win32_offset_1st_component
      +int win32_fspathcmp(const char *a, const char *b);
      +#define fspathcmp win32_fspathcmp
     ++int win32_fspathncmp(const char *a, const char *b, size_t count);
     ++#define fspathncmp win32_fspathncmp
       
       #endif
      
     @@ dir.c: int count_slashes(const char *s)
       {
       	return ignore_case ? strcasecmp(a, b) : strcmp(a, b);
       }
     +@@ dir.c: int fspatheq(const char *a, const char *b)
     + 	return !fspathcmp(a, b);
     + }
     + 
     +-int fspathncmp(const char *a, const char *b, size_t count)
     ++int git_fspathncmp(const char *a, const char *b, size_t count)
     + {
     + 	return ignore_case ? strncasecmp(a, b, count) : strncmp(a, b, count);
     + }
      
       ## dir.h ##
      @@ dir.h: int remove_dir_recursively(struct strbuf *path, int flag);
     @@ dir.h: int remove_dir_recursively(struct strbuf *path, int flag);
      -int fspathcmp(const char *a, const char *b);
      +int git_fspathcmp(const char *a, const char *b);
       int fspatheq(const char *a, const char *b);
     - int fspathncmp(const char *a, const char *b, size_t count);
     +-int fspathncmp(const char *a, const char *b, size_t count);
     ++int git_fspathncmp(const char *a, const char *b, size_t count);
       unsigned int fspathhash(const char *str);
     + 
     + /*
      
       ## git-compat-util.h ##
      @@ git-compat-util.h: static inline int git_offset_1st_component(const char *path)
       #define offset_1st_component git_offset_1st_component
       #endif
       
     -+
      +#ifndef fspathcmp
      +#define fspathcmp git_fspathcmp
      +#endif
     ++
     ++#ifndef fspathncmp
     ++#define fspathncmp git_fspathncmp
     ++#endif
      +
       #ifndef is_valid_path
       #define is_valid_path(path) 1
 4:  f04cfd91bd9 = 4:  60fde81d35c mingw(is_msys2_sh): handle forward slashes in the `sh.exe` path, too
 5:  707daf246bd = 5:  797cf9094ea run-command(win32): resolve the path to the Unix shell early
 6:  a74a7b4bb11 = 6:  7c53a4f4707 run-command: declare the `git_shell_path()` function globally
 7:  8bfd23cfa00 = 7:  7c8eba5bfd8 var(win32): do report the GIT_SHELL_PATH that is actually used

-- 
gitgitgadget

[PATCH v3 1/7] run-command: refactor getting the Unix shell path into its own function

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

This encapsulates the platform-specific logic better.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 run-command.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/run-command.c b/run-command.c
index d9f80fabe6e..59e433bf91c 100644
--- a/run-command.c
+++ b/run-command.c
@@ -274,17 +274,22 @@ int sane_execvp(const char *file, char * const argv[])
 	return -1;
 }
 
+static const char *git_shell_path(void)
+{
+#ifndef GIT_WINDOWS_NATIVE
+	return SHELL_PATH;
+#else
+	return "sh";
+#endif
+}
+
 static const char **prepare_shell_cmd(struct strvec *out, const char **argv)
 {
 	if (!argv[0])
 		BUG("shell command is empty");
 
 	if (strcspn(argv[0], "|&;<>()$`\\\"' \t\n*?[#~=%") != strlen(argv[0])) {
-#ifndef GIT_WINDOWS_NATIVE
-		strvec_push(out, SHELL_PATH);
-#else
-		strvec_push(out, "sh");
-#endif
+		strvec_push(out, git_shell_path());
 		strvec_push(out, "-c");
 
 		/*
-- 
gitgitgadget

[PATCH v3 2/7] strvec: declare the `strvec_push_nodup()` function globally

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

This function differs from `strvec_push()` in that it takes ownership of
the allocated string that is passed as second argument.

This is useful when appending elements to the string array that have
been freshly allocated and serve no further other purpose after that.

Without declaring this function globally, call sites would allocate the
memory, only to have `strvec_push()` duplicate the string, and then the
first copy would need to be released. Having this function globally
avoids that kind of unnecessary work.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 strvec.c | 2 +-
 strvec.h | 3 +++
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/strvec.c b/strvec.c
index d4073ec9fa2..f712070f574 100644
--- a/strvec.c
+++ b/strvec.c
@@ -10,7 +10,7 @@ void strvec_init(struct strvec *array)
 	memcpy(array, &blank, sizeof(*array));
 }
 
-static void strvec_push_nodup(struct strvec *array, const char *value)
+void strvec_push_nodup(struct strvec *array, char *value)
 {
 	if (array->v == empty_strvec)
 		array->v = NULL;
diff --git a/strvec.h b/strvec.h
index 6c7e8b7d503..4b73c1f092e 100644
--- a/strvec.h
+++ b/strvec.h
@@ -46,6 +46,9 @@ void strvec_init(struct strvec *);
 /* Push a copy of a string onto the end of the array. */
 const char *strvec_push(struct strvec *, const char *);
 
+/* Push an allocated string onto the end of the array, taking ownership. */
+void strvec_push_nodup(struct strvec *array, char *value);
+
 /**
  * Format a string and push it onto the end of the array. This is a
  * convenience wrapper combining `strbuf_addf` and `strvec_push`.
-- 
gitgitgadget

[PATCH v3 3/7] win32: override `fspathcmp()` with a directory separator-aware version

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

On Windows, the backslash is the directory separator, even if the
forward slash can be used, too, at least since Windows NT.

This means that the paths `a/b` and `a\b` are equivalent, and
`fspathcmp()` needs to be made aware of that fact.

Note that we have to override both `fspathcmp()` and `fspathncmp()`, and
the former cannot be a mere pre-processor constant that transforms calls
to `fspathcmp(a, b)` into `fspathncmp(a, b, (size_t)-1)` because the
function `report_collided_checkout()` in `unpack-trees.c` wants to
assign `list.cmp = fspathcmp`.

Also note that `fspatheq()` does _not_ need to be overridden because it
calls `fspathcmp()` internally.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 compat/win32/path-utils.c | 37 +++++++++++++++++++++++++++++++++++++
 compat/win32/path-utils.h |  4 ++++
 dir.c                     |  4 ++--
 dir.h                     |  4 ++--
 git-compat-util.h         |  8 ++++++++
 5 files changed, 53 insertions(+), 4 deletions(-)

diff --git a/compat/win32/path-utils.c b/compat/win32/path-utils.c
index ebf2f12eb66..b658ca3f811 100644
--- a/compat/win32/path-utils.c
+++ b/compat/win32/path-utils.c
@@ -1,4 +1,5 @@
 #include "../../git-compat-util.h"
+#include "../../environment.h"
 
 int win32_has_dos_drive_prefix(const char *path)
 {
@@ -50,3 +51,39 @@ int win32_offset_1st_component(const char *path)
 
 	return pos + is_dir_sep(*pos) - path;
 }
+
+int win32_fspathncmp(const char *a, const char *b, size_t count)
+{
+	int diff;
+
+	for (;;) {
+		if (!count--)
+			return 0;
+		if (!*a)
+			return *b ? -1 : 0;
+		if (!*b)
+			return +1;
+
+		if (is_dir_sep(*a)) {
+			if (!is_dir_sep(*b))
+				return -1;
+			a++;
+			b++;
+			continue;
+		} else if (is_dir_sep(*b))
+			return +1;
+
+		diff = ignore_case ?
+			(unsigned char)tolower(*a) - (int)(unsigned char)tolower(*b) :
+			(unsigned char)*a - (int)(unsigned char)*b;
+		if (diff)
+			return diff;
+		a++;
+		b++;
+	}
+}
+
+int win32_fspathcmp(const char *a, const char *b)
+{
+	return win32_fspathncmp(a, b, (size_t)-1);
+}
diff --git a/compat/win32/path-utils.h b/compat/win32/path-utils.h
index 65fa3b9263a..a561c700e75 100644
--- a/compat/win32/path-utils.h
+++ b/compat/win32/path-utils.h
@@ -29,5 +29,9 @@ static inline int win32_has_dir_sep(const char *path)
 #define has_dir_sep(path) win32_has_dir_sep(path)
 int win32_offset_1st_component(const char *path);
 #define offset_1st_component win32_offset_1st_component
+int win32_fspathcmp(const char *a, const char *b);
+#define fspathcmp win32_fspathcmp
+int win32_fspathncmp(const char *a, const char *b, size_t count);
+#define fspathncmp win32_fspathncmp
 
 #endif
diff --git a/dir.c b/dir.c
index b7a6625ebda..5a23376bdae 100644
--- a/dir.c
+++ b/dir.c
@@ -95,7 +95,7 @@ int count_slashes(const char *s)
 	return cnt;
 }
 
-int fspathcmp(const char *a, const char *b)
+int git_fspathcmp(const char *a, const char *b)
 {
 	return ignore_case ? strcasecmp(a, b) : strcmp(a, b);
 }
@@ -105,7 +105,7 @@ int fspatheq(const char *a, const char *b)
 	return !fspathcmp(a, b);
 }
 
-int fspathncmp(const char *a, const char *b, size_t count)
+int git_fspathncmp(const char *a, const char *b, size_t count)
 {
 	return ignore_case ? strncasecmp(a, b, count) : strncmp(a, b, count);
 }
diff --git a/dir.h b/dir.h
index 69a76d8bdd3..a3a2f00f5d9 100644
--- a/dir.h
+++ b/dir.h
@@ -541,9 +541,9 @@ int remove_dir_recursively(struct strbuf *path, int flag);
  */
 int remove_path(const char *path);
 
-int fspathcmp(const char *a, const char *b);
+int git_fspathcmp(const char *a, const char *b);
 int fspatheq(const char *a, const char *b);
-int fspathncmp(const char *a, const char *b, size_t count);
+int git_fspathncmp(const char *a, const char *b, size_t count);
 unsigned int fspathhash(const char *str);
 
 /*
diff --git a/git-compat-util.h b/git-compat-util.h
index ca7678a379d..71b4d23f038 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -506,6 +506,14 @@ static inline int git_offset_1st_component(const char *path)
 #define offset_1st_component git_offset_1st_component
 #endif
 
+#ifndef fspathcmp
+#define fspathcmp git_fspathcmp
+#endif
+
+#ifndef fspathncmp
+#define fspathncmp git_fspathncmp
+#endif
+
 #ifndef is_valid_path
 #define is_valid_path(path) 1
 #endif
-- 
gitgitgadget

[PATCH v3 4/7] mingw(is_msys2_sh): handle forward slashes in the `sh.exe` path, too

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

Whether the full path to the MSYS2 Bash is specified using backslashes
or forward slashes, in either case the command-line arguments need to be
quoted in the MSYS2-specific manner instead of using regular Win32
command-line quoting rules.

In preparation for `prepare_shell_cmd()` to use the full path to
`sh.exe` (with forward slashes for consistency), let's teach the
`is_msys2_sh()` function about this; Otherwise 5580.4 'clone with
backslashed path' would fail once `prepare_shell_cmd()` uses the full
path instead of merely `sh`.

This patch relies on the just-introduced fix where `fspathcmp()` handles
backslashes and forward slashes as equivalent on Windows.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 compat/mingw.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/compat/mingw.c b/compat/mingw.c
index 6097b8f9e60..29d3f09768c 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -1546,7 +1546,7 @@ static int is_msys2_sh(const char *cmd)
 		return ret;
 	}
 
-	if (ends_with(cmd, "\\sh.exe")) {
+	if (ends_with(cmd, "\\sh.exe") || ends_with(cmd, "/sh.exe")) {
 		static char *sh;
 
 		if (!sh)
-- 
gitgitgadget

[PATCH v3 5/7] run-command(win32): resolve the path to the Unix shell early

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

In 776297548e (Do not use SHELL_PATH from build system in
prepare_shell_cmd on Windows, 2012-04-17), the hard-coded path to the
Unix shell was replaced by passing `sh` instead when executing Unix
shell scripts in Git.

This was done because the hard-coded path to the Unix shell is incorrect
on Windows because it not only is a Unix-style absolute path instead of
a Windows one, but Git uses the runtime prefix feature on Windows, i.e.
the correct path cannot be hard-coded.

Naturally, the `sh` argument will be resolved to the full path of said
executable eventually.

To help fixing the bug where `git var GIT_SHELL_PATH` currently does not
reflect that logic, but shows that incorrect hard-coded Unix-style
absolute path, let's resolve the full path to the `sh` executable early
in the `git_shell_path()` function so that we can use it in `git var`,
too, and be sure that the output is equivalent to what `run_command()`
does when it is asked to execute a command-line using a Unix shell.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 run-command.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/run-command.c b/run-command.c
index 59e433bf91c..60a79db8f0e 100644
--- a/run-command.c
+++ b/run-command.c
@@ -274,12 +274,14 @@ int sane_execvp(const char *file, char * const argv[])
 	return -1;
 }
 
-static const char *git_shell_path(void)
+static char *git_shell_path(void)
 {
 #ifndef GIT_WINDOWS_NATIVE
-	return SHELL_PATH;
+	return xstrdup(SHELL_PATH);
 #else
-	return "sh";
+	char *p = locate_in_PATH("sh");
+	convert_slashes(p);
+	return p;
 #endif
 }
 
@@ -289,7 +291,7 @@ static const char **prepare_shell_cmd(struct strvec *out, const char **argv)
 		BUG("shell command is empty");
 
 	if (strcspn(argv[0], "|&;<>()$`\\\"' \t\n*?[#~=%") != strlen(argv[0])) {
-		strvec_push(out, git_shell_path());
+		strvec_push_nodup(out, git_shell_path());
 		strvec_push(out, "-c");
 
 		/*
-- 
gitgitgadget

[PATCH v3 6/7] run-command: declare the `git_shell_path()` function globally

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

The intention is to use it in `git var GIT_SHELL_PATH`, therefore we
need this function to stop being file-local only.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 run-command.c | 2 +-
 run-command.h | 5 +++++
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/run-command.c b/run-command.c
index 60a79db8f0e..45ba5449323 100644
--- a/run-command.c
+++ b/run-command.c
@@ -274,7 +274,7 @@ int sane_execvp(const char *file, char * const argv[])
 	return -1;
 }
 
-static char *git_shell_path(void)
+char *git_shell_path(void)
 {
 #ifndef GIT_WINDOWS_NATIVE
 	return xstrdup(SHELL_PATH);
diff --git a/run-command.h b/run-command.h
index 55f6631a2aa..03e7222d8b5 100644
--- a/run-command.h
+++ b/run-command.h
@@ -195,6 +195,11 @@ int is_executable(const char *name);
  */
 int exists_in_PATH(const char *command);
 
+/**
+ * Return the path that is used to execute Unix shell command-lines.
+ */
+char *git_shell_path(void);
+
 /**
  * Start a sub-process. Takes a pointer to a `struct child_process`
  * that specifies the details and returns pipe FDs (if requested).
-- 
gitgitgadget

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

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

On Windows, Unix-like paths like `/bin/sh` make very little sense. In
the best case, they simply don't work, in the worst case they are
misinterpreted as absolute paths that are relative to the drive
associated with the current directory.

To that end, Git does not actually use the path `/bin/sh` that is
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 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      | 3 ++-
 t/t0007-git-var.sh | 2 +-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/builtin/var.c b/builtin/var.c
index 5dc384810c0..e30ff45be1c 100644
--- a/builtin/var.c
+++ b/builtin/var.c
@@ -12,6 +12,7 @@
 #include "refs.h"
 #include "path.h"
 #include "strbuf.h"
+#include "run-command.h"
 
 static const char var_usage[] = "git var (-l | <variable>)";
 
@@ -51,7 +52,7 @@ static char *default_branch(int ident_flag UNUSED)
 
 static char *shell_path(int ident_flag UNUSED)
 {
-	return xstrdup(SHELL_PATH);
+	return git_shell_path();
 }
 
 static char *git_attr_val_system(int ident_flag UNUSED)
diff --git a/t/t0007-git-var.sh b/t/t0007-git-var.sh
index ff4fd9348cc..9fc58823873 100755
--- a/t/t0007-git-var.sh
+++ b/t/t0007-git-var.sh
@@ -157,7 +157,7 @@ test_expect_success POSIXPERM 'GIT_SHELL_PATH points to a valid executable' '
 test_expect_success MINGW 'GIT_SHELL_PATH points to a suitable shell' '
 	shellpath=$(git var GIT_SHELL_PATH) &&
 	case "$shellpath" in
-	*sh) ;;
+	[A-Z]:/*/sh.exe) test -f "$shellpath";;
 	*) return 1;;
 	esac
 '
-- 
gitgitgadget

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

[Phillip Wood]

Hi Johannes

On 13/07/2024 22:08, Johannes Schindelin via GitGitGadget wrote:
> Changes since v2:
> 
>   * Now fspathncmp() is overridden on Windows just like fspathcmp().
>   * The win32_fspath*cmp() functions now respect core.ignoreCase.

These changes in patch 3 look good. Thanks for fixing this

Phillip

> 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 | 37 +++++++++++++++++++++++++++++++++++++
>   compat/win32/path-utils.h |  4 ++++
>   dir.c                     |  4 ++--
>   dir.h                     |  4 ++--
>   git-compat-util.h         |  8 ++++++++
>   run-command.c             | 17 ++++++++++++-----
>   run-command.h             |  5 +++++
>   strvec.c                  |  2 +-
>   strvec.h                  |  3 +++
>   t/t0007-git-var.sh        |  2 +-
>   12 files changed, 78 insertions(+), 13 deletions(-)
> 
> 
> base-commit: 06e570c0dfb2a2deb64d217db78e2ec21672f558
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1760%2Fdscho%2Fgit-var-on-windows-v3
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1760/dscho/git-var-on-windows-v3
> Pull-Request: https://github.com/gitgitgadget/git/pull/1760
> 
> Range-diff vs v2:
> 
>   1:  e0970381042 = 1:  e0970381042 run-command: refactor getting the Unix shell path into its own function
>   2:  91ebccbc39f = 2:  91ebccbc39f strvec: declare the `strvec_push_nodup()` function globally
>   3:  a718183bb3b ! 3:  f46315ac0b2 win32: override `fspathcmp()` with a directory separator-aware version
>       @@ Commit message
>            This means that the paths `a/b` and `a\b` are equivalent, and
>            `fspathcmp()` needs to be made aware of that fact.
>        
>       +    Note that we have to override both `fspathcmp()` and `fspathncmp()`, and
>       +    the former cannot be a mere pre-processor constant that transforms calls
>       +    to `fspathcmp(a, b)` into `fspathncmp(a, b, (size_t)-1)` because the
>       +    function `report_collided_checkout()` in `unpack-trees.c` wants to
>       +    assign `list.cmp = fspathcmp`.
>       +
>       +    Also note that `fspatheq()` does _not_ need to be overridden because it
>       +    calls `fspathcmp()` internally.
>       +
>            Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
>        
>         ## compat/win32/path-utils.c ##
>       +@@
>       + #include "../../git-compat-util.h"
>       ++#include "../../environment.h"
>       +
>       + int win32_has_dos_drive_prefix(const char *path)
>       + {
>        @@ compat/win32/path-utils.c: int win32_offset_1st_component(const char *path)
>         
>         	return pos + is_dir_sep(*pos) - path;
>         }
>        +
>       -+int win32_fspathcmp(const char *a, const char *b)
>       ++int win32_fspathncmp(const char *a, const char *b, size_t count)
>        +{
>        +	int diff;
>        +
>        +	for (;;) {
>       ++		if (!count--)
>       ++			return 0;
>        +		if (!*a)
>       -+			return !*b ? 0 : -1;
>       ++			return *b ? -1 : 0;
>        +		if (!*b)
>        +			return +1;
>        +
>       @@ compat/win32/path-utils.c: int win32_offset_1st_component(const char *path)
>        +		} else if (is_dir_sep(*b))
>        +			return +1;
>        +
>       -+		diff = (unsigned char)tolower(*(a++)) - (unsigned char)tolower(*(b++));
>       ++		diff = ignore_case ?
>       ++			(unsigned char)tolower(*a) - (int)(unsigned char)tolower(*b) :
>       ++			(unsigned char)*a - (int)(unsigned char)*b;
>        +		if (diff)
>        +			return diff;
>       ++		a++;
>       ++		b++;
>        +	}
>       ++}
>       ++
>       ++int win32_fspathcmp(const char *a, const char *b)
>       ++{
>       ++	return win32_fspathncmp(a, b, (size_t)-1);
>        +}
>        
>         ## compat/win32/path-utils.h ##
>       @@ compat/win32/path-utils.h: static inline int win32_has_dir_sep(const char *path)
>         #define offset_1st_component win32_offset_1st_component
>        +int win32_fspathcmp(const char *a, const char *b);
>        +#define fspathcmp win32_fspathcmp
>       ++int win32_fspathncmp(const char *a, const char *b, size_t count);
>       ++#define fspathncmp win32_fspathncmp
>         
>         #endif
>        
>       @@ dir.c: int count_slashes(const char *s)
>         {
>         	return ignore_case ? strcasecmp(a, b) : strcmp(a, b);
>         }
>       +@@ dir.c: int fspatheq(const char *a, const char *b)
>       + 	return !fspathcmp(a, b);
>       + }
>       +
>       +-int fspathncmp(const char *a, const char *b, size_t count)
>       ++int git_fspathncmp(const char *a, const char *b, size_t count)
>       + {
>       + 	return ignore_case ? strncasecmp(a, b, count) : strncmp(a, b, count);
>       + }
>        
>         ## dir.h ##
>        @@ dir.h: int remove_dir_recursively(struct strbuf *path, int flag);
>       @@ dir.h: int remove_dir_recursively(struct strbuf *path, int flag);
>        -int fspathcmp(const char *a, const char *b);
>        +int git_fspathcmp(const char *a, const char *b);
>         int fspatheq(const char *a, const char *b);
>       - int fspathncmp(const char *a, const char *b, size_t count);
>       +-int fspathncmp(const char *a, const char *b, size_t count);
>       ++int git_fspathncmp(const char *a, const char *b, size_t count);
>         unsigned int fspathhash(const char *str);
>       +
>       + /*
>        
>         ## git-compat-util.h ##
>        @@ git-compat-util.h: static inline int git_offset_1st_component(const char *path)
>         #define offset_1st_component git_offset_1st_component
>         #endif
>         
>       -+
>        +#ifndef fspathcmp
>        +#define fspathcmp git_fspathcmp
>        +#endif
>       ++
>       ++#ifndef fspathncmp
>       ++#define fspathncmp git_fspathncmp
>       ++#endif
>        +
>         #ifndef is_valid_path
>         #define is_valid_path(path) 1
>   4:  f04cfd91bd9 = 4:  60fde81d35c mingw(is_msys2_sh): handle forward slashes in the `sh.exe` path, too
>   5:  707daf246bd = 5:  797cf9094ea run-command(win32): resolve the path to the Unix shell early
>   6:  a74a7b4bb11 = 6:  7c53a4f4707 run-command: declare the `git_shell_path()` function globally
>   7:  8bfd23cfa00 = 7:  7c8eba5bfd8 var(win32): do report the GIT_SHELL_PATH that is actually used
> 

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

[phillip.wood123]

Hi Johannes

On 11/07/2024 13:03, Johannes Schindelin wrote:
> Hi Phillip,
> 
> On Tue, 9 Jul 2024, Phillip Wood wrote:
> 
>> On 08/07/2024 14:02, Johannes Schindelin via GitGitGadget wrote:
>>> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>>
>> Thanks for putting a patch together so quickly
>>
>>>    static char *shell_path(int ident_flag UNUSED)
>>>    {
>>> +#ifdef WIN32
>>> +	char *p = locate_in_PATH("sh");
>>
>> If I'm reading is_busybox_applet() (which only exists in git-for-windows)
>> correctly then this will return "busybox.exe" under mingit-busybox rather than
>> ash.exe, so the calling program would have to know to set argv[0] (which is
>> likely not possible unless the calling program is written in C) or pass "sh"
>> as the first argument. As the code to support busybox does not exist upstream
>> I guess that's best handled downstream.
> 
> BusyBox-w32 is unfortunately displaying strange performance patterns. It
> is partially (and expectedly) faster than the MSYS2 Bash, but in other
> scenarios it is substantially slower (which is totally unexpected).
> 
> Some time ago, I tried to make this all work and investigate the
> unexpected performance issues (and hoped to fix them, too), but ran out of
> time [*1*]. That was almost two years ago, and I am unsure whether I will
> ever be able to elevate the BusyBox flavor of MinGit to a non-experimental
> state.
> 
> My original plan was to eventually no longer include `busybox.exe` in
> the mingit-busybox packages, but instead a copy of that executable with
> the name `sh.exe` and thereby have it work without that hack in the Git
> code to call the `busybox.exe` with the `sh` argument inserted before the
> regular command-line arguments.

Thanks for the information.

> In the context of the patch (or now: patch series) at hand, I don't think
> we need to let BusyBox play any role.

Agreed

Best Wishes

Phillip

> Ciao,
> Johannes
> 
> Footnote *1*: Interested parties can find the latest state here:
> https://github.com/git-for-windows/git/compare/main...dscho:git:busybox

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

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 1278 bytes --]

On 2024-07-13 at 21:08:17, Johannes Schindelin via GitGitGadget wrote:
> Changes since v2:
> 
>  * Now fspathncmp() is overridden on Windows just like fspathcmp().
>  * The win32_fspath*cmp() functions now respect core.ignoreCase.
> 
> 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

This series seems reasonable to me as well.  I of course can't speak to
whether the approach for finding sh is the right one, since I'm not a
Windows developer, but I have confidence you know the answer and have
thought through it fully.
-- 
brian m. carlson (they/them or he/him)
Toronto, Ontario, CA

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

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

[Junio C Hamano]

"brian m. carlson" <sandals@crustytoothpaste.net> writes:

>> 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
>
> This series seems reasonable to me as well.  I of course can't speak to
> whether the approach for finding sh is the right one, since I'm not a
> Windows developer, but I have confidence you know the answer and have
> thought through it fully.

... and the series will be in 2.46-rc1

Thanks.