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

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 9, 2024, 16:31 UTC
Message-ID: <xmqqy16afrwr.fsf@gitster.g>
In-Reply-To: <ZoyDlrcmyXUX_dki@tapette.crustytoothpaste.net>

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