dscho
commented
2 months ago /submit
Closed dscho closed 1 month ago
dscho
commented
2 months ago /submit
gitgitgadget[bot]
commented
2 months ago Submitted as pull.1760.git.1720443778074.gitgitgadget@gmail.com
To fetch this version into FETCH_HEAD:
git fetch https://github.com/gitgitgadget/git/ pr-1760/dscho/git-var-on-windows-v1To fetch this version to local tag pr-1760/dscho/git-var-on-windows-v1:
git fetch --no-tags https://github.com/gitgitgadget/git/ tag pr-1760/dscho/git-var-on-windows-v1On the Git mailing list, Junio C Hamano wrote (reply to this):
"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.
On the Git mailing list, Junio C Hamano wrote (reply to this):
"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: 06e570c0dfb2a2deb64d217db78e2ec21672f558On the Git mailing list, "brian m. carlson" wrote (reply to this):
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, CAUser "brian m. carlson" <sandals@crustytoothpaste.net> has been added to the cc: list.
gitgitgadget[bot]
commented
2 months ago On the Git mailing list, Junio C Hamano wrote (reply to this):
"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.
On the Git mailing list, "brian m. carlson" wrote (reply to this):
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, CAOn the Git mailing list, Phillip Wood wrote (reply to this):
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: 06e570c0dfb2a2deb64d217db78e2ec21672f558User Phillip Wood <phillip.wood123@gmail.com> has been added to the cc: list.
gitgitgadget[bot]
commented
2 months ago On the Git mailing list, Phillip Wood wrote (reply to this):
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
PhillipOn the Git mailing list, Junio C Hamano wrote (reply to this):
"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.On the Git mailing list, Johannes Schindelin wrote (reply to this):
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/submit
gitgitgadget[bot]
commented
1 month ago Submitted as pull.1760.v2.git.1720739496.gitgitgadget@gmail.com
To fetch this version into FETCH_HEAD:
git fetch https://github.com/gitgitgadget/git/ pr-1760/dscho/git-var-on-windows-v2To fetch this version to local tag pr-1760/dscho/git-var-on-windows-v2:
git fetch --no-tags https://github.com/gitgitgadget/git/ tag pr-1760/dscho/git-var-on-windows-v2On the Git mailing list, Phillip Wood wrote (reply to this):
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)
> This patch series was integrated into seen via https://github.com/git/git/commit/483844d03c2ffeadc988041553aeaacd26194a7d.
gitgitgadget[bot]
commented
1 month ago This patch series was integrated into seen via https://github.com/git/git/commit/563f9dcae0be3f5c1b8a549ef5893cea62f6ab76.
gitgitgadget[bot]
commented
1 month ago On the Git mailing list, Junio C Hamano wrote (reply to this):
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.This branch is now known as js/var-git-shell-path.
gitgitgadget[bot]
commented
1 month ago There was a status update in the "New Topics" section about the branch js/var-git-shell-path on the Git mailing list:
"git var GIT_SHELL_PATH" should report the path to the shell used to spawn external commands, but it didn't do so on Windows, which has been corrected. Will merge to 'next'. source: <pull.1760.v2.git.1720739496.gitgitgadget@gmail.com>
dscho
commented
1 month ago /submit
gitgitgadget[bot]
commented
1 month ago Submitted as pull.1760.v3.git.1720904905.gitgitgadget@gmail.com
To fetch this version into FETCH_HEAD:
git fetch https://github.com/gitgitgadget/git/ pr-1760/dscho/git-var-on-windows-v3To fetch this version to local tag pr-1760/dscho/git-var-on-windows-v3:
git fetch --no-tags https://github.com/gitgitgadget/git/ tag pr-1760/dscho/git-var-on-windows-v3This patch series was integrated into seen via https://github.com/git/git/commit/2e504c593c163373ba45667594e16f9103f54cdb.
gitgitgadget[bot]
commented
1 month ago This patch series was integrated into next via https://github.com/git/git/commit/2e5b0eea39a78badf7b542b2d3c48a30d509f18d.
gitgitgadget[bot]
commented
1 month ago This patch series was integrated into seen via https://github.com/git/git/commit/c894da8558c18adb74d19939ae8e88919a4e8cdd.
gitgitgadget[bot]
commented
1 month ago There was a status update in the "Cooking" section about the branch js/var-git-shell-path on the Git mailing list:
"git var GIT_SHELL_PATH" should report the path to the shell used to spawn external commands, but it didn't do so on Windows, which has been corrected. Will merge to 'master'. source: <pull.1760.v3.git.1720904905.gitgitgadget@gmail.com>
gitgitgadget[bot]
commented
1 month ago On the Git mailing list, Phillip Wood wrote (reply to this):
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
> On the Git mailing list, phillip.wood123@gmail.com wrote (reply to this):
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:busyboxThis patch series was integrated into seen via https://github.com/git/git/commit/76e018b9a104d4449651f1e3b00b4e0d9a369e87.
gitgitgadget[bot]
commented
1 month ago This patch series was integrated into master via https://github.com/git/git/commit/76e018b9a104d4449651f1e3b00b4e0d9a369e87.
gitgitgadget[bot]
commented
1 month ago This patch series was integrated into next via https://github.com/git/git/commit/76e018b9a104d4449651f1e3b00b4e0d9a369e87.
gitgitgadget[bot]
commented
1 month ago Closed via 76e018b9a104d4449651f1e3b00b4e0d9a369e87.
gitgitgadget[bot]
commented
1 month ago On the Git mailing list, "brian m. carlson" wrote (reply to this):
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, CAOn the Git mailing list, Junio C Hamano wrote (reply to this):
"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.
Changes since v2:
fspathncmp()is overridden on Windows just likefspathcmp().win32_fspath*cmp()functions now respectcore.ignoreCase.Changes since v1:
prepare_shell_cmd()andgit var GIT_SHELL_PATH.cc: "brian m. carlson" sandals@crustytoothpaste.net cc: Phillip Wood phillip.wood123@gmail.com