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

[Junio C Hamano <gitster@pobox.com>]

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