{"thread":{"id":"61753","subject":"[PATCH] var(win32): do report the GIT_SHELL_PATH that is actually used","startedAt":"2024-07-08T13:03:01Z","lastAt":"2024-07-17T22:51:27Z","messageCount":35,"participants":["Johannes Schindelin via GitGitGadget","Junio C Hamano","brian m. carlson","Phillip Wood","Johannes Schindelin","phillip.wood123@gmail.com"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"498245","messageId":"pull.1760.git.1720443778074.gitgitgadget@gmail.com","threadId":"61753","inReplyTo":null,"subject":"[PATCH] var(win32): do report the GIT_SHELL_PATH that is actually used","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-07-08T13:02:57Z","receivedAt":"2024-07-08T13:03:01Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nOn Windows, Unix-like paths like `/bin/sh` make very little sense. In\nthe best case, they simply don't work, in the worst case they are\nmisinterpreted as absolute paths that are relative to the drive\nassociated with the current directory.\n\nTo that end, Git does not actually use the path `/bin/sh` that is\nrecorded e.g. in Unix shell scripts' hash-bang lines. Instead, as of\n776297548e (Do not use SHELL_PATH from build system in prepare_shell_cmd\non Windows, 2012-04-17), it re-interprets `/bin/sh` as \"look up `sh` on\nthe `PATH` and use the result instead\".\n\nHowever, when 1e65721227 (var: add support for listing the shell,\n2023-06-27) introduced support for `git var GIT_SHELL_PATH`, Windows was\nnot special-cased as above, which is why it outputs `/bin/sh` even\nthough that disagrees with what Git actually uses.\n\nLet's fix this, and also adjust the corresponding test case to verify\nthat it actually finds a working executable.\n\nReported-by: Phillip Wood \nSigned-off-by: Johannes Schindelin \n---\n var(win32): do report the GIT_SHELL_PATH that is actually used\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1760%2Fdscho%2Fgit-var-on-windows-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1760/dscho/git-var-on-windows-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1760\n\n builtin/var.c | 6 ++++++\n t/t0007-git-var.sh | 2 +-\n 2 files changed, 7 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/var.c b/builtin/var.c\nindex 5dc384810c0..f4b1f34e403 100644\n--- a/builtin/var.c\n+++ b/builtin/var.c\n@@ -51,7 +51,13 @@ static char *default_branch(int ident_flag UNUSED)\n \n static char *shell_path(int ident_flag UNUSED)\n {\n+#ifdef WIN32\n+\tchar *p = locate_in_PATH(\"sh\");\n+\tconvert_slashes(p);\n+\treturn p;\n+#else\n \treturn xstrdup(SHELL_PATH);\n+#endif\n }\n \n static char *git_attr_val_system(int ident_flag UNUSED)\ndiff --git a/t/t0007-git-var.sh b/t/t0007-git-var.sh\nindex ff4fd9348cc..9fc58823873 100755\n--- a/t/t0007-git-var.sh\n+++ b/t/t0007-git-var.sh\n@@ -157,7 +157,7 @@ test_expect_success POSIXPERM 'GIT_SHELL_PATH points to a valid executable' '\n test_expect_success MINGW 'GIT_SHELL_PATH points to a suitable shell' '\n \tshellpath=$(git var GIT_SHELL_PATH) &&\n \tcase \"$shellpath\" in\n-\t*sh) ;;\n+\t[A-Z]:/*/sh.exe) test -f \"$shellpath\";;\n \t*) return 1;;\n \tesac\n '\n\nbase-commit: 06e570c0dfb2a2deb64d217db78e2ec21672f558\n-- \ngitgitgadget\n"},{"id":"498304","messageId":"xmqqed83g1e6.fsf@gitster.g","threadId":"61753","inReplyTo":"pull.1760.git.1720443778074.gitgitgadget@gmail.com","subject":"Re: [PATCH] var(win32): do report the GIT_SHELL_PATH that is actually used","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-07-08T18:54:41Z","receivedAt":"2024-07-08T18:54:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" \nwrites:\n\n> On Windows, Unix-like paths like `/bin/sh` make very little sense. In\n> the best case, they simply don't work, in the worst case they are\n> misinterpreted as absolute paths that are relative to the drive\n> associated with the current directory.\n>\n> To that end, Git does not actually use the path `/bin/sh` that is\n> recorded e.g. in Unix shell scripts' hash-bang lines. Instead, as of\n> 776297548e (Do not use SHELL_PATH from build system in prepare_shell_cmd\n> on Windows, 2012-04-17), it re-interprets `/bin/sh` as \"look up `sh` on\n> the `PATH` and use the result instead\".\n\nThis asks for a few naïve questions.\n\nIf there is a fixed path the \"git\" binary was compiled for, which\ncan be referenced with a single variable GIT_SHELL_PATH, even though\non non-POSIX systems it won't be like /bin/sh, wouldn't there be a\npath like \"C:\\Program Files\\Git for Windows\\bin\\sh\" (I do not do\nWindows, so you may be installing somewhere completely different)\nand wouldn't such a path work regardless of which drive is\nassociated with the current directory?\n\nI would actually understand that, with relocatable build where\neverything is relative to the installed directory, a single\nGIT_SHELL_PATH that is defined at the compile-time may not make much\nsense, and when you need to interpret a shell script, you may need\nto recompute the actual path, relative to the installation\ndirectory.\n\nBut I wonder why the replacement shell that is spawned is searched\nfor in PATH, not where you installed it (which of course would be\ndifferent from /bin/sh). In other words, when running script that\ncalls for \"#!/bin/sh\", looking for \"sh\" on PATH might be a workable\nhack, and it might even yield the same result, especially if you\nprepend the path you installed git and bash as part of your\ninstallation package to the user's PATH, but wouldn't it make more\nsense to compute it just like how \"git --exec-path\" is recomputed\nwith the relocatable build?\n\nThe \"look on the %PATH%\" strategy does not make any sense as an\nimplementation for getting GIT_SHELL_PATH, which answers \"what is\nthe shell this instanciation of Git was built to work with?\", at\nleast to me. Maybe I am missing some knowledge on limitations on\nWindows and Git for Windows why it is done that way.\n\nThanks.\n\n\n"},{"id":"498305","messageId":"xmqq7cdvg105.fsf@gitster.g","threadId":"61753","inReplyTo":"pull.1760.git.1720443778074.gitgitgadget@gmail.com","subject":"Re: [PATCH] var(win32): do report the GIT_SHELL_PATH that is actually used","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-07-08T19:03:06Z","receivedAt":"2024-07-08T19:03:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" \nwrites:\n\n> diff --git a/builtin/var.c b/builtin/var.c\n> index 5dc384810c0..f4b1f34e403 100644\n> --- a/builtin/var.c\n> +++ b/builtin/var.c\n> @@ -51,7 +51,13 @@ static char *default_branch(int ident_flag UNUSED)\n> \n> static char *shell_path(int ident_flag UNUSED)\n> {\n> +#ifdef WIN32\n> +\tchar *p = locate_in_PATH(\"sh\");\n> +\tconvert_slashes(p);\n> +\treturn p;\n> +#else\n> \treturn xstrdup(SHELL_PATH);\n> +#endif\n> }\n\nWe have done well without any ugly conditional compilation in this\nfile so far. If we needed to report dynamic path on certain\nplatforms, we'd need something a bit cleaner, like\n\n * rename \"shell_path()\" to something that is unlikely to conflict\n with others (e.g., \"git_shell_path()\") and make it extern\n\n * allow it to be overridden from compat/\n\nwhich is often how we hide such an implementation quirk from the\ngeneral code paths.\n\n> static char *git_attr_val_system(int ident_flag UNUSED)\n> diff --git a/t/t0007-git-var.sh b/t/t0007-git-var.sh\n> index ff4fd9348cc..9fc58823873 100755\n> --- a/t/t0007-git-var.sh\n> +++ b/t/t0007-git-var.sh\n> @@ -157,7 +157,7 @@ test_expect_success POSIXPERM 'GIT_SHELL_PATH points to a valid executable' '\n> test_expect_success MINGW 'GIT_SHELL_PATH points to a suitable shell' '\n> \tshellpath=$(git var GIT_SHELL_PATH) &&\n> \tcase \"$shellpath\" in\n> -\t*sh) ;;\n> +\t[A-Z]:/*/sh.exe) test -f \"$shellpath\";;\n> \t*) return 1;;\n> \tesac\n> '\n>\n> base-commit: 06e570c0dfb2a2deb64d217db78e2ec21672f558\n"},{"id":"498330","messageId":"Zox48sVp-PybvLxi@tapette.crustytoothpaste.net","threadId":"61753","inReplyTo":"xmqqed83g1e6.fsf@gitster.g","subject":"Re: [PATCH] var(win32): do report the GIT_SHELL_PATH that is actually used","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2024-07-08T23:40:34Z","receivedAt":"2024-07-08T23:40:37Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2024-07-08 at 18:54:41, Junio C Hamano wrote:\n> This asks for a few naïve questions.\n> \n> If there is a fixed path the \"git\" binary was compiled for, which\n> can be referenced with a single variable GIT_SHELL_PATH, even though\n> on non-POSIX systems it won't be like /bin/sh, wouldn't there be a\n> path like \"C:\\Program Files\\Git for Windows\\bin\\sh\" (I do not do\n> Windows, so you may be installing somewhere completely different)\n> and wouldn't such a path work regardless of which drive is\n> associated with the current directory?\n> \n> I would actually understand that, with relocatable build where\n> everything is relative to the installed directory, a single\n> GIT_SHELL_PATH that is defined at the compile-time may not make much\n> sense, and when you need to interpret a shell script, you may need\n> to recompute the actual path, relative to the installation\n> directory.\n\nI'll jump in here, and Dscho can correct me if I'm wrong, but I believe\nthere's one build that's always relocatable (well, there's MinGit and\nthe regular, but ignoring that). You can install to almost any drive\nand location, so it's not known at compile time.\n\n> But I wonder why the replacement shell that is spawned is searched\n> for in PATH, not where you installed it (which of course would be\n> different from /bin/sh). In other words, when running script that\n> calls for \"#!/bin/sh\", looking for \"sh\" on PATH might be a workable\n> hack, and it might even yield the same result, especially if you\n> prepend the path you installed git and bash as part of your\n> installation package to the user's PATH, but wouldn't it make more\n> sense to compute it just like how \"git --exec-path\" is recomputed\n> with the relocatable build?\n> \n> The \"look on the %PATH%\" strategy does not make any sense as an\n> implementation for getting GIT_SHELL_PATH, which answers \"what is\n> the shell this instanciation of Git was built to work with?\", at\n> least to me. Maybe I am missing some knowledge on limitations on\n> Windows and Git for Windows why it is done that way.\n\nWell, it may be that that's the approach that Git for Windows takes to\nlook up the shell. (I don't know for certain.) If that _is_ what it\ndoes, then that's absolutely the value we want because we want to use\nwhatever shell Git for Windows uses. I will say it's a risky approach\nbecause it could well also find a Cygwin or MINGW shell (or, if it were\ncalled bash, WSL), but we really want whatever Git for Windows does\nhere.\n\nThat's because external users who rely on Git for Windows to furnish a\nPOSIX shell will want to know the path, and this variable is the best\nway to do that (and the reason I added it).\n-- \nbrian m. carlson (they/them or he/him)\nToronto, Ontario, CA\n"},{"id":"498331","messageId":"xmqqr0c3h21e.fsf@gitster.g","threadId":"61753","inReplyTo":"Zox48sVp-PybvLxi@tapette.crustytoothpaste.net","subject":"Re: [PATCH] var(win32): do report the GIT_SHELL_PATH that is actually used","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-07-08T23:55:25Z","receivedAt":"2024-07-08T23:55:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"brian m. carlson\" writes:\n\n>> The \"look on the %PATH%\" strategy does not make any sense as an\n>> implementation for getting GIT_SHELL_PATH, which answers \"what is\n>> the shell this instanciation of Git was built to work with?\", at\n>> least to me. Maybe I am missing some knowledge on limitations on\n>> Windows and Git for Windows why it is done that way.\n>\n> Well, it may be that that's the approach that Git for Windows takes to\n> look up the shell. (I don't know for certain.)\n\n> If that _is_ what it does, then that's absolutely the value we\n> want because we want to use whatever shell Git for Windows uses.\n> I will say it's a risky approach because it could well also find a\n> Cygwin or MINGW shell (or, if it were called bash, WSL), but we\n> really want whatever Git for Windows does here.\n\nYeah, absolutely it is risky unless it is doing the \"we are\nrelocatable, so where is the 'sh' _we_ installed?\", which is what I\nwould expect GIT_SHELL_PATH to be.\n\n> That's because external users who rely on Git for Windows to furnish a\n> POSIX shell will want to know the path, and this variable is the best\n> way to do that (and the reason I added it).\n\nIf Git for Windows furnishes programs other than POSIX shell, paths\nto which external users would also want to learn, what happens?\nGIT_PERL_PATH, GIT_WISH_PATH, etc.? Locate them on PATH themselves\nshouldn't be rocket science (and instead of locating, just spawning\nthem themselves would be even easier, I would presume).\n\nThanks.\n\n"},{"id":"498333","messageId":"ZoyDlrcmyXUX_dki@tapette.crustytoothpaste.net","threadId":"61753","inReplyTo":"xmqqr0c3h21e.fsf@gitster.g","subject":"Re: [PATCH] var(win32): do report the GIT_SHELL_PATH that is actually used","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2024-07-09T00:25:58Z","receivedAt":"2024-07-09T00:26:01Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2024-07-08 at 23:55:25, Junio C Hamano wrote:\n> \"brian m. carlson\" writes:\n> \n> > That's because external users who rely on Git for Windows to furnish a\n> > POSIX shell will want to know the path, and this variable is the best\n> > way to do that (and the reason I added it).\n> \n> If Git for Windows furnishes programs other than POSIX shell, paths\n> to which external users would also want to learn, what happens?\n> GIT_PERL_PATH, GIT_WISH_PATH, etc.? Locate them on PATH themselves\n> shouldn't be rocket science (and instead of locating, just spawning\n> them themselves would be even easier, I would presume).\n\nIn my experience, they have not always been on PATH. It may be that\nthey are now, which is fantastic, but I seem to remember that at some\npoint bash.exe and git.exe were on PATH, but sh.exe and other commands\nwere not. (There's also a different path under Git Bash than the\nregular Windows PATH.) You might say, \"Well, why not use bash.exe?\" and\nthat works great until you realize that there's also a bash.exe that is\npart of WSL and will attempt to spawn WSL for you.\n\nHowever, that doesn't always work, because sometimes WSL isn't installed\nor is disabled or broken, and so the operation fails with an error\nmessage. Also, users will typically have wanted Git for Windows's bash\nand not a Linux environment's bash, since the two environments will\ntypically have different software available.\n\nWhy not add Perl or Wish or something else? Because once you have the\nGit for Windows sh, you can use a fixed Unix path to look them up and\nget a canonical Windows path with cygpath -w. Thus, this is just the\nminimal bootstrapping functionality to get that information.\n\nOf course, on Unix, there can still be multiple Perl implementations,\nbut you're typically either going to build against one in particular,\nwhich may or may not be what Git was built against, or you're going to\nuse /usr/bin/env and choose whatever's first in PATH. In that case,\nit's very unlikely that anyone cares what version of Perl Git actually\nuses.\n\nThe only major benefit of using Git's shell on Unix is that you know it\nsupports POSIX functionality (which /bin/sh does not on some proprietary\nUnices) and it also supports local, which may be convenient for\nscripting.\n-- \nbrian m. carlson (they/them or he/him)\nToronto, Ontario, CA\n"},{"id":"498349","messageId":"8612a47b-18c4-4071-8827-56b4f3bd0d80@gmail.com","threadId":"61753","inReplyTo":"pull.1760.git.1720443778074.gitgitgadget@gmail.com","subject":"Re: [PATCH] var(win32): do report the GIT_SHELL_PATH that is actually used","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-07-09T08:55:13Z","receivedAt":"2024-07-09T08:55:12Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Johannes\n\nOn 08/07/2024 14:02, Johannes Schindelin via GitGitGadget wrote:\n> From: Johannes Schindelin \n\nThanks for putting a patch together so quickly\n\n> static char *shell_path(int ident_flag UNUSED)\n> {\n> +#ifdef WIN32\n> +\tchar *p = locate_in_PATH(\"sh\");\n\nIf I'm reading is_busybox_applet() (which only exists in \ngit-for-windows) correctly then this will return \"busybox.exe\" under \nmingit-busybox rather than ash.exe, so the calling program would have to \nknow to set argv[0] (which is likely not possible unless the calling \nprogram is written in C) or pass \"sh\" as the first argument. As the code \nto support busybox does not exist upstream I guess that's best handled \ndownstream.\n\nBest Wishes\n\nPhillip\n\n> +\tconvert_slashes(p);\n> +\treturn p;\n> +#else\n> \treturn xstrdup(SHELL_PATH);\n> +#endif\n> }\n> \n> static char *git_attr_val_system(int ident_flag UNUSED)\n> diff --git a/t/t0007-git-var.sh b/t/t0007-git-var.sh\n> index ff4fd9348cc..9fc58823873 100755\n> --- a/t/t0007-git-var.sh\n> +++ b/t/t0007-git-var.sh\n> @@ -157,7 +157,7 @@ test_expect_success POSIXPERM 'GIT_SHELL_PATH points to a valid executable' '\n> test_expect_success MINGW 'GIT_SHELL_PATH points to a suitable shell' '\n> \tshellpath=$(git var GIT_SHELL_PATH) &&\n> \tcase \"$shellpath\" in\n> -\t*sh) ;;\n> +\t[A-Z]:/*/sh.exe) test -f \"$shellpath\";;\n> \t*) return 1;;\n> \tesac\n> '\n> \n> base-commit: 06e570c0dfb2a2deb64d217db78e2ec21672f558\n"},{"id":"498365","messageId":"9e2e32d2-4f52-43db-b2bd-8cf2df1af10c@gmail.com","threadId":"61753","inReplyTo":"Zox48sVp-PybvLxi@tapette.crustytoothpaste.net","subject":"Re: [PATCH] var(win32): do report the GIT_SHELL_PATH that is actually used","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-07-09T13:53:11Z","receivedAt":"2024-07-09T13:53:09Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 09/07/2024 00:40, brian m. carlson wrote:\n> On 2024-07-08 at 18:54:41, Junio C Hamano wrote:\n>> The \"look on the %PATH%\" strategy does not make any sense as an\n>> implementation for getting GIT_SHELL_PATH, which answers \"what is\n>> the shell this instanciation of Git was built to work with?\", at\n>> least to me. Maybe I am missing some knowledge on limitations on\n>> Windows and Git for Windows why it is done that way.\n> \n> Well, it may be that that's the approach that Git for Windows takes to\n> look up the shell. (I don't know for certain.) If that _is_ what it\n> does, then that's absolutely the value we want because we want to use\n> whatever shell Git for Windows uses.\n\nAs I understand it this is the approach Git for Windows takes\n\n> I will say it's a risky approach\n> because it could well also find a Cygwin or MINGW shell (or, if it were\n> called bash, WSL), but we really want whatever Git for Windows does\n> here.\n\nGit for Windows prepends the MINGW system directories to $PATH via some \nextra downstream code in setup_windows_environment() so looking up the \nshell in $PATH will find the correct executable.\n\nBest Wishes\n\nPhillip\n"},{"id":"498368","messageId":"xmqqy16afrwr.fsf@gitster.g","threadId":"61753","inReplyTo":"ZoyDlrcmyXUX_dki@tapette.crustytoothpaste.net","subject":"Re: [PATCH] var(win32): do report the GIT_SHELL_PATH that is actually used","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-07-09T16:31:48Z","receivedAt":"2024-07-09T16:32:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"brian m. carlson\" writes:\n\n> Why not add Perl or Wish or something else? Because once you have the\n> Git for Windows sh, you can use a fixed Unix path to look them up and\n> get a canonical Windows path with cygpath -w. Thus, this is just the\n> minimal bootstrapping functionality to get that information.\n\nBesides, perl and wish are not part of Git. The same thing can be\nsaid that shell is not part of Git. \n\nSo stepping back and thinking why we have \"git var GIT_SHELL_PATH\",\nwhat should it mean to begin with? It is obviously not to tell\nwhere we installed the thing as a part of \"Git\" suite, even though\nGit for Windows installer may let users install the shell and other\nstuff (similar to how \"apt\" lets users install package on Debian\nderived systems).\n\nHence, I can accept that \"git var GIT_SHELL_PATH\" is not (and does\nnot have to be) about where we installed what we shipped. It is\nwhere we use the shell from (i.e., when we need \"sh\", we use that\nshell).\n\n The documentation for GIT_SHELL_PATH is already good. Sorry for\n my earlier confusion---I should have looked at it first. It says\n it is not about what we ship, but what we use when we need to\n shell out.\n\nI am OK with GIT_SHELL_PATH computed differently depending on the\nplatform, as different platform apparently use different mechanism\nto decide which shell to spawn. On POSIX systems, it is the one we\ncompiled to use, while on Windows it is the one that happens to be\non the end-user's PATH.\n\nBut then the comment I made in my earlier review still stands that\nsuch a platform dependent logic should be implemented a bit more\ncleanly than the posted patch.\n\n\"Which shell do we use at runtime\" should influence a lot of what\nthe things in run-command.c do, so perhaps\n\n - we remove builtin/var.c:shell_path()\n\n - We create run-command.c:git_shell_path() immediately above\n run-command.c:prepare_shell_cmd(). We will add conditional\n compilation there (i.e. #ifdef GIT_WINDOWS_NATIVE). The default\n implementation is to return SHELL_PATH, and on Windows it looks\n for \"sh\" on %PATH%.\n\n - The entry for GIT_SHELL_PATH in builtin/var.c:git_vars[] should\n be updated to point at git_shell_path() in run-command.c\n\n - Near the beginning of run-command.c:prepare_shell_cmd(), there is\n a conditional compilation. If we can remove it by using\n git_shell_path(), that would be a nice bonus.\n\nwould give us a good approach for implementation?\n\nThanks.\n"},{"id":"498508","messageId":"66345b2d-6662-710a-5513-6cc70507b4d0@gmx.de","threadId":"61753","inReplyTo":"8612a47b-18c4-4071-8827-56b4f3bd0d80@gmail.com","subject":"Re: [PATCH] var(win32): do report the GIT_SHELL_PATH that is actually used","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2024-07-11T12:03:23Z","receivedAt":"2024-07-11T12:03:38Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Phillip,\n\nOn Tue, 9 Jul 2024, Phillip Wood wrote:\n\n> On 08/07/2024 14:02, Johannes Schindelin via GitGitGadget wrote:\n> > From: Johannes Schindelin \n>\n> Thanks for putting a patch together so quickly\n>\n> > static char *shell_path(int ident_flag UNUSED)\n> > {\n> > +#ifdef WIN32\n> > +\tchar *p = locate_in_PATH(\"sh\");\n>\n> If I'm reading is_busybox_applet() (which only exists in git-for-windows)\n> correctly then this will return \"busybox.exe\" under mingit-busybox rather than\n> ash.exe, so the calling program would have to know to set argv[0] (which is\n> likely not possible unless the calling program is written in C) or pass \"sh\"\n> as the first argument. As the code to support busybox does not exist upstream\n> I guess that's best handled downstream.\n\nBusyBox-w32 is unfortunately displaying strange performance patterns. It\nis partially (and expectedly) faster than the MSYS2 Bash, but in other\nscenarios it is substantially slower (which is totally unexpected).\n\nSome time ago, I tried to make this all work and investigate the\nunexpected performance issues (and hoped to fix them, too), but ran out of\ntime [*1*]. That was almost two years ago, and I am unsure whether I will\never be able to elevate the BusyBox flavor of MinGit to a non-experimental\nstate.\n\nMy original plan was to eventually no longer include `busybox.exe` in\nthe mingit-busybox packages, but instead a copy of that executable with\nthe name `sh.exe` and thereby have it work without that hack in the Git\ncode to call the `busybox.exe` with the `sh` argument inserted before the\nregular command-line arguments.\n\nIn the context of the patch (or now: patch series) at hand, I don't think\nwe need to let BusyBox play any role.\n\nCiao,\nJohannes\n\nFootnote *1*: Interested parties can find the latest state here:\nhttps://github.com/git-for-windows/git/compare/main...dscho:git:busybox\n"},{"id":"498544","messageId":"pull.1760.v2.git.1720739496.gitgitgadget@gmail.com","threadId":"61753","inReplyTo":"pull.1760.git.1720443778074.gitgitgadget@gmail.com","subject":"[PATCH v2 0/7] var(win32): do report the GIT_SHELL_PATH that is actually used","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-07-11T23:11:29Z","receivedAt":"2024-07-11T23:11:40Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Changes since v1:\n\n * This patch series now shares the logic that determines the path of the\n Unix shell that Git uses between prepare_shell_cmd() and git var\n GIT_SHELL_PATH.\n\nJohannes Schindelin (7):\n run-command: refactor getting the Unix shell path into its own\n function\n strvec: declare the `strvec_push_nodup()` function globally\n win32: override `fspathcmp()` with a directory separator-aware version\n mingw(is_msys2_sh): handle forward slashes in the `sh.exe` path, too\n run-command(win32): resolve the path to the Unix shell early\n run-command: declare the `git_shell_path()` function globally\n var(win32): do report the GIT_SHELL_PATH that is actually used\n\n builtin/var.c | 3 ++-\n compat/mingw.c | 2 +-\n compat/win32/path-utils.c | 25 +++++++++++++++++++++++++\n compat/win32/path-utils.h | 2 ++\n dir.c | 2 +-\n dir.h | 2 +-\n git-compat-util.h | 5 +++++\n run-command.c | 17 ++++++++++++-----\n run-command.h | 5 +++++\n strvec.c | 2 +-\n strvec.h | 3 +++\n t/t0007-git-var.sh | 2 +-\n 12 files changed, 59 insertions(+), 11 deletions(-)\n\n\nbase-commit: 06e570c0dfb2a2deb64d217db78e2ec21672f558\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1760%2Fdscho%2Fgit-var-on-windows-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1760/dscho/git-var-on-windows-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/1760\n\nRange-diff vs v1:\n\n -: ----------- > 1: e0970381042 run-command: refactor getting the Unix shell path into its own function\n -: ----------- > 2: 91ebccbc39f strvec: declare the `strvec_push_nodup()` function globally\n -: ----------- > 3: a718183bb3b win32: override `fspathcmp()` with a directory separator-aware version\n -: ----------- > 4: f04cfd91bd9 mingw(is_msys2_sh): handle forward slashes in the `sh.exe` path, too\n -: ----------- > 5: 707daf246bd run-command(win32): resolve the path to the Unix shell early\n -: ----------- > 6: a74a7b4bb11 run-command: declare the `git_shell_path()` function globally\n 1: ef62c3fc122 ! 7: 8bfd23cfa00 var(win32): do report the GIT_SHELL_PATH that is actually used\n @@ Commit message\n associated with the current directory.\n \n To that end, Git does not actually use the path `/bin/sh` that is\n - recorded e.g. in Unix shell scripts' hash-bang lines. Instead, as of\n - 776297548e (Do not use SHELL_PATH from build system in prepare_shell_cmd\n - on Windows, 2012-04-17), it re-interprets `/bin/sh` as \"look up `sh` on\n - the `PATH` and use the result instead\".\n + recorded e.g. when `run_command()` is called with a Unix shell\n + command-line. Instead, as of 776297548e (Do not use SHELL_PATH from\n + build system in prepare_shell_cmd on Windows, 2012-04-17), it\n + re-interprets `/bin/sh` as \"look up `sh` on the `PATH` and use the\n + result instead\".\n +\n + This is the logic users expect to be followed when running `git var\n + GIT_SHELL_PATH`.\n \n However, when 1e65721227 (var: add support for listing the shell,\n 2023-06-27) introduced support for `git var GIT_SHELL_PATH`, Windows was\n not special-cased as above, which is why it outputs `/bin/sh` even\n though that disagrees with what Git actually uses.\n \n - Let's fix this, and also adjust the corresponding test case to verify\n - that it actually finds a working executable.\n + Let's fix this by using the exact same logic as `prepare_shell_cmd()`,\n + adjusting the Windows-specific `git var GIT_SHELL_PATH` test case to\n + verify that it actually finds a working executable.\n \n Reported-by: Phillip Wood \n Signed-off-by: Johannes Schindelin \n \n ## builtin/var.c ##\n +@@\n + #include \"refs.h\"\n + #include \"path.h\"\n + #include \"strbuf.h\"\n ++#include \"run-command.h\"\n + \n + static const char var_usage[] = \"git var (-l | )\";\n + \n @@ builtin/var.c: static char *default_branch(int ident_flag UNUSED)\n \n static char *shell_path(int ident_flag UNUSED)\n {\n -+#ifdef WIN32\n -+\tchar *p = locate_in_PATH(\"sh\");\n -+\tconvert_slashes(p);\n -+\treturn p;\n -+#else\n - \treturn xstrdup(SHELL_PATH);\n -+#endif\n +-\treturn xstrdup(SHELL_PATH);\n ++\treturn git_shell_path();\n }\n \n static char *git_attr_val_system(int ident_flag UNUSED)\n\n-- \ngitgitgadget\n"},{"id":"498545","messageId":"e0970381042e4e35074c5f095959d3379aad6d9d.1720739496.git.gitgitgadget@gmail.com","threadId":"61753","inReplyTo":"pull.1760.v2.git.1720739496.gitgitgadget@gmail.com","subject":"[PATCH v2 1/7] run-command: refactor getting the Unix shell path into its own function","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-07-11T23:11:30Z","receivedAt":"2024-07-11T23:11:40Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nThis encapsulates the platform-specific logic better.\n\nSigned-off-by: Johannes Schindelin \n---\n run-command.c | 15 ++++++++++-----\n 1 file changed, 10 insertions(+), 5 deletions(-)\n\ndiff --git a/run-command.c b/run-command.c\nindex d9f80fabe6e..59e433bf91c 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -274,17 +274,22 @@ int sane_execvp(const char *file, char * const argv[])\n \treturn -1;\n }\n \n+static const char *git_shell_path(void)\n+{\n+#ifndef GIT_WINDOWS_NATIVE\n+\treturn SHELL_PATH;\n+#else\n+\treturn \"sh\";\n+#endif\n+}\n+\n static const char **prepare_shell_cmd(struct strvec *out, const char **argv)\n {\n \tif (!argv[0])\n \t\tBUG(\"shell command is empty\");\n \n \tif (strcspn(argv[0], \"|&;<>()$`\\\\\\\"' \\t\\n*?[#~=%\") != strlen(argv[0])) {\n-#ifndef GIT_WINDOWS_NATIVE\n-\t\tstrvec_push(out, SHELL_PATH);\n-#else\n-\t\tstrvec_push(out, \"sh\");\n-#endif\n+\t\tstrvec_push(out, git_shell_path());\n \t\tstrvec_push(out, \"-c\");\n \n \t\t/*\n-- \ngitgitgadget\n\n"},{"id":"498546","messageId":"91ebccbc39f21fa73af8bc8c81af721f1ca201bd.1720739496.git.gitgitgadget@gmail.com","threadId":"61753","inReplyTo":"pull.1760.v2.git.1720739496.gitgitgadget@gmail.com","subject":"[PATCH v2 2/7] strvec: declare the `strvec_push_nodup()` function globally","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-07-11T23:11:31Z","receivedAt":"2024-07-11T23:11:42Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nThis function differs from `strvec_push()` in that it takes ownership of\nthe allocated string that is passed as second argument.\n\nThis is useful when appending elements to the string array that have\nbeen freshly allocated and serve no further other purpose after that.\n\nWithout declaring this function globally, call sites would allocate the\nmemory, only to have `strvec_push()` duplicate the string, and then the\nfirst copy would need to be released. Having this function globally\navoids that kind of unnecessary work.\n\nSigned-off-by: Johannes Schindelin \n---\n strvec.c | 2 +-\n strvec.h | 3 +++\n 2 files changed, 4 insertions(+), 1 deletion(-)\n\ndiff --git a/strvec.c b/strvec.c\nindex d4073ec9fa2..f712070f574 100644\n--- a/strvec.c\n+++ b/strvec.c\n@@ -10,7 +10,7 @@ void strvec_init(struct strvec *array)\n \tmemcpy(array, &blank, sizeof(*array));\n }\n \n-static void strvec_push_nodup(struct strvec *array, const char *value)\n+void strvec_push_nodup(struct strvec *array, char *value)\n {\n \tif (array->v == empty_strvec)\n \t\tarray->v = NULL;\ndiff --git a/strvec.h b/strvec.h\nindex 6c7e8b7d503..4b73c1f092e 100644\n--- a/strvec.h\n+++ b/strvec.h\n@@ -46,6 +46,9 @@ void strvec_init(struct strvec *);\n /* Push a copy of a string onto the end of the array. */\n const char *strvec_push(struct strvec *, const char *);\n \n+/* Push an allocated string onto the end of the array, taking ownership. */\n+void strvec_push_nodup(struct strvec *array, char *value);\n+\n /**\n * Format a string and push it onto the end of the array. This is a\n * convenience wrapper combining `strbuf_addf` and `strvec_push`.\n-- \ngitgitgadget\n\n"},{"id":"498547","messageId":"a718183bb3b8eabd5ced274d8db124909bcdf493.1720739496.git.gitgitgadget@gmail.com","threadId":"61753","inReplyTo":"pull.1760.v2.git.1720739496.gitgitgadget@gmail.com","subject":"[PATCH v2 3/7] win32: override `fspathcmp()` with a directory separator-aware version","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-07-11T23:11:32Z","receivedAt":"2024-07-11T23:11:42Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nOn Windows, the backslash is the directory separator, even if the\nforward slash can be used, too, at least since Windows NT.\n\nThis means that the paths `a/b` and `a\\b` are equivalent, and\n`fspathcmp()` needs to be made aware of that fact.\n\nSigned-off-by: Johannes Schindelin \n---\n compat/win32/path-utils.c | 25 +++++++++++++++++++++++++\n compat/win32/path-utils.h | 2 ++\n dir.c | 2 +-\n dir.h | 2 +-\n git-compat-util.h | 5 +++++\n 5 files changed, 34 insertions(+), 2 deletions(-)\n\ndiff --git a/compat/win32/path-utils.c b/compat/win32/path-utils.c\nindex ebf2f12eb66..af7ef957bbf 100644\n--- a/compat/win32/path-utils.c\n+++ b/compat/win32/path-utils.c\n@@ -50,3 +50,28 @@ int win32_offset_1st_component(const char *path)\n \n \treturn pos + is_dir_sep(*pos) - path;\n }\n+\n+int win32_fspathcmp(const char *a, const char *b)\n+{\n+\tint diff;\n+\n+\tfor (;;) {\n+\t\tif (!*a)\n+\t\t\treturn !*b ? 0 : -1;\n+\t\tif (!*b)\n+\t\t\treturn +1;\n+\n+\t\tif (is_dir_sep(*a)) {\n+\t\t\tif (!is_dir_sep(*b))\n+\t\t\t\treturn -1;\n+\t\t\ta++;\n+\t\t\tb++;\n+\t\t\tcontinue;\n+\t\t} else if (is_dir_sep(*b))\n+\t\t\treturn +1;\n+\n+\t\tdiff = (unsigned char)tolower(*(a++)) - (unsigned char)tolower(*(b++));\n+\t\tif (diff)\n+\t\t\treturn diff;\n+\t}\n+}\ndiff --git a/compat/win32/path-utils.h b/compat/win32/path-utils.h\nindex 65fa3b9263a..e3c2a79df74 100644\n--- a/compat/win32/path-utils.h\n+++ b/compat/win32/path-utils.h\n@@ -29,5 +29,7 @@ static inline int win32_has_dir_sep(const char *path)\n #define has_dir_sep(path) win32_has_dir_sep(path)\n int win32_offset_1st_component(const char *path);\n #define offset_1st_component win32_offset_1st_component\n+int win32_fspathcmp(const char *a, const char *b);\n+#define fspathcmp win32_fspathcmp\n \n #endif\ndiff --git a/dir.c b/dir.c\nindex b7a6625ebda..37d8e266b2c 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -95,7 +95,7 @@ int count_slashes(const char *s)\n \treturn cnt;\n }\n \n-int fspathcmp(const char *a, const char *b)\n+int git_fspathcmp(const char *a, const char *b)\n {\n \treturn ignore_case ? strcasecmp(a, b) : strcmp(a, b);\n }\ndiff --git a/dir.h b/dir.h\nindex 69a76d8bdd3..947e3d77442 100644\n--- a/dir.h\n+++ b/dir.h\n@@ -541,7 +541,7 @@ int remove_dir_recursively(struct strbuf *path, int flag);\n */\n int remove_path(const char *path);\n \n-int fspathcmp(const char *a, const char *b);\n+int git_fspathcmp(const char *a, const char *b);\n int fspatheq(const char *a, const char *b);\n int fspathncmp(const char *a, const char *b, size_t count);\n unsigned int fspathhash(const char *str);\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex ca7678a379d..ac564a68987 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -506,6 +506,11 @@ static inline int git_offset_1st_component(const char *path)\n #define offset_1st_component git_offset_1st_component\n #endif\n \n+\n+#ifndef fspathcmp\n+#define fspathcmp git_fspathcmp\n+#endif\n+\n #ifndef is_valid_path\n #define is_valid_path(path) 1\n #endif\n-- \ngitgitgadget\n\n"},{"id":"498548","messageId":"f04cfd91bd9a3a462294ea9125e72b3e6bb64865.1720739496.git.gitgitgadget@gmail.com","threadId":"61753","inReplyTo":"pull.1760.v2.git.1720739496.gitgitgadget@gmail.com","subject":"[PATCH v2 4/7] mingw(is_msys2_sh): handle forward slashes in the `sh.exe` path, too","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-07-11T23:11:33Z","receivedAt":"2024-07-11T23:11:44Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nWhether the full path to the MSYS2 Bash is specified using backslashes\nor forward slashes, in either case the command-line arguments need to be\nquoted in the MSYS2-specific manner instead of using regular Win32\ncommand-line quoting rules.\n\nIn preparation for `prepare_shell_cmd()` to use the full path to\n`sh.exe` (with forward slashes for consistency), let's teach the\n`is_msys2_sh()` function about this; Otherwise 5580.4 'clone with\nbackslashed path' would fail once `prepare_shell_cmd()` uses the full\npath instead of merely `sh`.\n\nThis patch relies on the just-introduced fix where `fspathcmp()` handles\nbackslashes and forward slashes as equivalent on Windows.\n\nSigned-off-by: Johannes Schindelin \n---\n compat/mingw.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/compat/mingw.c b/compat/mingw.c\nindex 6097b8f9e60..29d3f09768c 100644\n--- a/compat/mingw.c\n+++ b/compat/mingw.c\n@@ -1546,7 +1546,7 @@ static int is_msys2_sh(const char *cmd)\n \t\treturn ret;\n \t}\n \n-\tif (ends_with(cmd, \"\\\\sh.exe\")) {\n+\tif (ends_with(cmd, \"\\\\sh.exe\") || ends_with(cmd, \"/sh.exe\")) {\n \t\tstatic char *sh;\n \n \t\tif (!sh)\n-- \ngitgitgadget\n\n"},{"id":"498549","messageId":"707daf246bd74f05428fc95c5b84b4836b06c648.1720739496.git.gitgitgadget@gmail.com","threadId":"61753","inReplyTo":"pull.1760.v2.git.1720739496.gitgitgadget@gmail.com","subject":"[PATCH v2 5/7] run-command(win32): resolve the path to the Unix shell early","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-07-11T23:11:34Z","receivedAt":"2024-07-11T23:11:45Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nIn 776297548e (Do not use SHELL_PATH from build system in\nprepare_shell_cmd on Windows, 2012-04-17), the hard-coded path to the\nUnix shell was replaced by passing `sh` instead when executing Unix\nshell scripts in Git.\n\nThis was done because the hard-coded path to the Unix shell is incorrect\non Windows because it not only is a Unix-style absolute path instead of\na Windows one, but Git uses the runtime prefix feature on Windows, i.e.\nthe correct path cannot be hard-coded.\n\nNaturally, the `sh` argument will be resolved to the full path of said\nexecutable eventually.\n\nTo help fixing the bug where `git var GIT_SHELL_PATH` currently does not\nreflect that logic, but shows that incorrect hard-coded Unix-style\nabsolute path, let's resolve the full path to the `sh` executable early\nin the `git_shell_path()` function so that we can use it in `git var`,\ntoo, and be sure that the output is equivalent to what `run_command()`\ndoes when it is asked to execute a command-line using a Unix shell.\n\nSigned-off-by: Johannes Schindelin \n---\n run-command.c | 10 ++++++----\n 1 file changed, 6 insertions(+), 4 deletions(-)\n\ndiff --git a/run-command.c b/run-command.c\nindex 59e433bf91c..60a79db8f0e 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -274,12 +274,14 @@ int sane_execvp(const char *file, char * const argv[])\n \treturn -1;\n }\n \n-static const char *git_shell_path(void)\n+static char *git_shell_path(void)\n {\n #ifndef GIT_WINDOWS_NATIVE\n-\treturn SHELL_PATH;\n+\treturn xstrdup(SHELL_PATH);\n #else\n-\treturn \"sh\";\n+\tchar *p = locate_in_PATH(\"sh\");\n+\tconvert_slashes(p);\n+\treturn p;\n #endif\n }\n \n@@ -289,7 +291,7 @@ static const char **prepare_shell_cmd(struct strvec *out, const char **argv)\n \t\tBUG(\"shell command is empty\");\n \n \tif (strcspn(argv[0], \"|&;<>()$`\\\\\\\"' \\t\\n*?[#~=%\") != strlen(argv[0])) {\n-\t\tstrvec_push(out, git_shell_path());\n+\t\tstrvec_push_nodup(out, git_shell_path());\n \t\tstrvec_push(out, \"-c\");\n \n \t\t/*\n-- \ngitgitgadget\n\n"},{"id":"498550","messageId":"a74a7b4bb1129a33230e63ba72c656f52adb0e61.1720739496.git.gitgitgadget@gmail.com","threadId":"61753","inReplyTo":"pull.1760.v2.git.1720739496.gitgitgadget@gmail.com","subject":"[PATCH v2 6/7] run-command: declare the `git_shell_path()` function globally","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-07-11T23:11:35Z","receivedAt":"2024-07-11T23:11:46Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nThe intention is to use it in `git var GIT_SHELL_PATH`, therefore we\nneed this function to stop being file-local only.\n\nSigned-off-by: Johannes Schindelin \n---\n run-command.c | 2 +-\n run-command.h | 5 +++++\n 2 files changed, 6 insertions(+), 1 deletion(-)\n\ndiff --git a/run-command.c b/run-command.c\nindex 60a79db8f0e..45ba5449323 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -274,7 +274,7 @@ int sane_execvp(const char *file, char * const argv[])\n \treturn -1;\n }\n \n-static char *git_shell_path(void)\n+char *git_shell_path(void)\n {\n #ifndef GIT_WINDOWS_NATIVE\n \treturn xstrdup(SHELL_PATH);\ndiff --git a/run-command.h b/run-command.h\nindex 55f6631a2aa..03e7222d8b5 100644\n--- a/run-command.h\n+++ b/run-command.h\n@@ -195,6 +195,11 @@ int is_executable(const char *name);\n */\n int exists_in_PATH(const char *command);\n \n+/**\n+ * Return the path that is used to execute Unix shell command-lines.\n+ */\n+char *git_shell_path(void);\n+\n /**\n * Start a sub-process. Takes a pointer to a `struct child_process`\n * that specifies the details and returns pipe FDs (if requested).\n-- \ngitgitgadget\n\n"},{"id":"498551","messageId":"8bfd23cfa00c351ffdcc25bd29f3b84089544a56.1720739496.git.gitgitgadget@gmail.com","threadId":"61753","inReplyTo":"pull.1760.v2.git.1720739496.gitgitgadget@gmail.com","subject":"[PATCH v2 7/7] var(win32): do report the GIT_SHELL_PATH that is actually used","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-07-11T23:11:36Z","receivedAt":"2024-07-11T23:11:48Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nOn Windows, Unix-like paths like `/bin/sh` make very little sense. In\nthe best case, they simply don't work, in the worst case they are\nmisinterpreted as absolute paths that are relative to the drive\nassociated with the current directory.\n\nTo that end, Git does not actually use the path `/bin/sh` that is\nrecorded e.g. when `run_command()` is called with a Unix shell\ncommand-line. Instead, as of 776297548e (Do not use SHELL_PATH from\nbuild system in prepare_shell_cmd on Windows, 2012-04-17), it\nre-interprets `/bin/sh` as \"look up `sh` on the `PATH` and use the\nresult instead\".\n\nThis is the logic users expect to be followed when running `git var\nGIT_SHELL_PATH`.\n\nHowever, when 1e65721227 (var: add support for listing the shell,\n2023-06-27) introduced support for `git var GIT_SHELL_PATH`, Windows was\nnot special-cased as above, which is why it outputs `/bin/sh` even\nthough that disagrees with what Git actually uses.\n\nLet's fix this by using the exact same logic as `prepare_shell_cmd()`,\nadjusting the Windows-specific `git var GIT_SHELL_PATH` test case to\nverify that it actually finds a working executable.\n\nReported-by: Phillip Wood \nSigned-off-by: Johannes Schindelin \n---\n builtin/var.c | 3 ++-\n t/t0007-git-var.sh | 2 +-\n 2 files changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/var.c b/builtin/var.c\nindex 5dc384810c0..e30ff45be1c 100644\n--- a/builtin/var.c\n+++ b/builtin/var.c\n@@ -12,6 +12,7 @@\n #include \"refs.h\"\n #include \"path.h\"\n #include \"strbuf.h\"\n+#include \"run-command.h\"\n \n static const char var_usage[] = \"git var (-l | )\";\n \n@@ -51,7 +52,7 @@ static char *default_branch(int ident_flag UNUSED)\n \n static char *shell_path(int ident_flag UNUSED)\n {\n-\treturn xstrdup(SHELL_PATH);\n+\treturn git_shell_path();\n }\n \n static char *git_attr_val_system(int ident_flag UNUSED)\ndiff --git a/t/t0007-git-var.sh b/t/t0007-git-var.sh\nindex ff4fd9348cc..9fc58823873 100755\n--- a/t/t0007-git-var.sh\n+++ b/t/t0007-git-var.sh\n@@ -157,7 +157,7 @@ test_expect_success POSIXPERM 'GIT_SHELL_PATH points to a valid executable' '\n test_expect_success MINGW 'GIT_SHELL_PATH points to a suitable shell' '\n \tshellpath=$(git var GIT_SHELL_PATH) &&\n \tcase \"$shellpath\" in\n-\t*sh) ;;\n+\t[A-Z]:/*/sh.exe) test -f \"$shellpath\";;\n \t*) return 1;;\n \tesac\n '\n-- \ngitgitgadget\n"},{"id":"498578","messageId":"a1913542-ea19-49d7-a694-42540c71df03@gmail.com","threadId":"61753","inReplyTo":"a718183bb3b8eabd5ced274d8db124909bcdf493.1720739496.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 3/7] win32: override `fspathcmp()` with a directory separator-aware version","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-07-12T13:46:27Z","receivedAt":"2024-07-12T13:46:32Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Johannes\n\nOn 12/07/2024 00:11, Johannes Schindelin via GitGitGadget wrote:\n> From: Johannes Schindelin \n> \n> On Windows, the backslash is the directory separator, even if the\n> forward slash can be used, too, at least since Windows NT.\n> \n> This means that the paths `a/b` and `a\\b` are equivalent, and\n> `fspathcmp()` needs to be made aware of that fact.\n\nHow does fspathncmp() behave? It would be good for the two to match. \nI've left a couple of thoughts below but I'm not sure they're worth \nre-rolling for\n\n> Signed-off-by: Johannes Schindelin \n> ---\n> compat/win32/path-utils.c | 25 +++++++++++++++++++++++++\n> compat/win32/path-utils.h | 2 ++\n> dir.c | 2 +-\n> dir.h | 2 +-\n> git-compat-util.h | 5 +++++\n> 5 files changed, 34 insertions(+), 2 deletions(-)\n> \n> diff --git a/compat/win32/path-utils.c b/compat/win32/path-utils.c\n> index ebf2f12eb66..af7ef957bbf 100644\n> --- a/compat/win32/path-utils.c\n> +++ b/compat/win32/path-utils.c\n> @@ -50,3 +50,28 @@ int win32_offset_1st_component(const char *path)\n> \n> \treturn pos + is_dir_sep(*pos) - path;\n> }\n> +\n> +int win32_fspathcmp(const char *a, const char *b)\n> +{\n> +\tint diff;\n> +\n> +\tfor (;;) {\n> +\t\tif (!*a)\n> +\t\t\treturn !*b ? 0 : -1;\n\nPersonally I'd find this easier to read as\n\n\treturn *b ? -1 : 0;\n\n> +\t\tif (!*b)\n> +\t\t\treturn +1;\n> +\n> +\t\tif (is_dir_sep(*a)) {\n> +\t\t\tif (!is_dir_sep(*b))\n> +\t\t\t\treturn -1;\n> +\t\t\ta++;\n> +\t\t\tb++;\n> +\t\t\tcontinue;\n> +\t\t} else if (is_dir_sep(*b))\n> +\t\t\treturn +1;\n> +\n> +\t\tdiff = (unsigned char)tolower(*(a++)) - (unsigned char)tolower(*(b++));\n\nThere is a subtle difference between this and strcasecmp() because the \nlatter is locale dependent but our tolower() is not because we override \nthe standard library's version. As they're both useless on multibyte \nlocales it probably doesn't make much difference in practice.\n\nThanks\n\nPhillip\n\n> +\t\tif (diff)\n> +\t\t\treturn diff;\n> +\t}\n> +}\n> diff --git a/compat/win32/path-utils.h b/compat/win32/path-utils.h\n> index 65fa3b9263a..e3c2a79df74 100644\n> --- a/compat/win32/path-utils.h\n> +++ b/compat/win32/path-utils.h\n> @@ -29,5 +29,7 @@ static inline int win32_has_dir_sep(const char *path)\n> #define has_dir_sep(path) win32_has_dir_sep(path)\n> int win32_offset_1st_component(const char *path);\n> #define offset_1st_component win32_offset_1st_component\n> +int win32_fspathcmp(const char *a, const char *b);\n> +#define fspathcmp win32_fspathcmp\n> \n> #endif\n> diff --git a/dir.c b/dir.c\n> index b7a6625ebda..37d8e266b2c 100644\n> --- a/dir.c\n> +++ b/dir.c\n> @@ -95,7 +95,7 @@ int count_slashes(const char *s)\n> \treturn cnt;\n> }\n> \n> -int fspathcmp(const char *a, const char *b)\n> +int git_fspathcmp(const char *a, const char *b)\n> {\n> \treturn ignore_case ? strcasecmp(a, b) : strcmp(a, b);\n> }\n> diff --git a/dir.h b/dir.h\n> index 69a76d8bdd3..947e3d77442 100644\n> --- a/dir.h\n> +++ b/dir.h\n> @@ -541,7 +541,7 @@ int remove_dir_recursively(struct strbuf *path, int flag);\n> */\n> int remove_path(const char *path);\n> \n> -int fspathcmp(const char *a, const char *b);\n> +int git_fspathcmp(const char *a, const char *b);\n> int fspatheq(const char *a, const char *b);\n> int fspathncmp(const char *a, const char *b, size_t count);\n> unsigned int fspathhash(const char *str);\n> diff --git a/git-compat-util.h b/git-compat-util.h\n> index ca7678a379d..ac564a68987 100644\n> --- a/git-compat-util.h\n> +++ b/git-compat-util.h\n> @@ -506,6 +506,11 @@ static inline int git_offset_1st_component(const char *path)\n> #define offset_1st_component git_offset_1st_component\n> #endif\n> \n> +\n> +#ifndef fspathcmp\n> +#define fspathcmp git_fspathcmp\n> +#endif\n> +\n> #ifndef is_valid_path\n> #define is_valid_path(path) 1\n> #endif\n"},{"id":"498579","messageId":"ba8c96ce-4249-4000-8efd-3db73cf10001@gmail.com","threadId":"61753","inReplyTo":"f04cfd91bd9a3a462294ea9125e72b3e6bb64865.1720739496.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 4/7] mingw(is_msys2_sh): handle forward slashes in the `sh.exe` path, too","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-07-12T13:49:09Z","receivedAt":"2024-07-12T13:49:14Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"\n\nOn 12/07/2024 00:11, Johannes Schindelin via GitGitGadget wrote:\n> From: Johannes Schindelin \n> \n> Whether the full path to the MSYS2 Bash is specified using backslashes\n> or forward slashes, in either case the command-line arguments need to be\n> quoted in the MSYS2-specific manner instead of using regular Win32\n> command-line quoting rules.\n> \n> In preparation for `prepare_shell_cmd()` to use the full path to\n> `sh.exe` (with forward slashes for consistency), let's teach the\n> `is_msys2_sh()` function about this; Otherwise 5580.4 'clone with\n> backslashed path' would fail once `prepare_shell_cmd()` uses the full\n> path instead of merely `sh`.\n\nMakes sense\n\n> This patch relies on the just-introduced fix where `fspathcmp()` handles\n> backslashes and forward slashes as equivalent on Windows.\n\nThat dependency isn't obvious from the patch but there is a call to \nfspathcmp() a couple of lines below the last context line.\n\nThanks\n\nPhillip\n\n> Signed-off-by: Johannes Schindelin \n> ---\n> compat/mingw.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n> \n> diff --git a/compat/mingw.c b/compat/mingw.c\n> index 6097b8f9e60..29d3f09768c 100644\n> --- a/compat/mingw.c\n> +++ b/compat/mingw.c\n> @@ -1546,7 +1546,7 @@ static int is_msys2_sh(const char *cmd)\n> \t\treturn ret;\n> \t}\n> \n> -\tif (ends_with(cmd, \"\\\\sh.exe\")) {\n> +\tif (ends_with(cmd, \"\\\\sh.exe\") || ends_with(cmd, \"/sh.exe\")) {\n> \t\tstatic char *sh;\n> \n> \t\tif (!sh)\n"},{"id":"498580","messageId":"3b687707-7aa7-4f4f-8043-0e31fb0c09ce@gmail.com","threadId":"61753","inReplyTo":"pull.1760.v2.git.1720739496.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 0/7] var(win32): do report the GIT_SHELL_PATH that is actually used","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-07-12T13:51:37Z","receivedAt":"2024-07-12T13:51:42Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Johannes\n\nOn 12/07/2024 00:11, Johannes Schindelin via GitGitGadget wrote:\n> Changes since v1:\n> \n> * This patch series now shares the logic that determines the path of the\n> Unix shell that Git uses between prepare_shell_cmd() and git var\n> GIT_SHELL_PATH.\n\nThis was a pleasant read, each step was well described and easy to \nfollow. I've left a couple of comments but I think this is good as it is.\n\nThanks\n\nPhillip\n\n> Johannes Schindelin (7):\n> run-command: refactor getting the Unix shell path into its own\n> function\n> strvec: declare the `strvec_push_nodup()` function globally\n> win32: override `fspathcmp()` with a directory separator-aware version\n> mingw(is_msys2_sh): handle forward slashes in the `sh.exe` path, too\n> run-command(win32): resolve the path to the Unix shell early\n> run-command: declare the `git_shell_path()` function globally\n> var(win32): do report the GIT_SHELL_PATH that is actually used\n> \n> builtin/var.c | 3 ++-\n> compat/mingw.c | 2 +-\n> compat/win32/path-utils.c | 25 +++++++++++++++++++++++++\n> compat/win32/path-utils.h | 2 ++\n> dir.c | 2 +-\n> dir.h | 2 +-\n> git-compat-util.h | 5 +++++\n> run-command.c | 17 ++++++++++++-----\n> run-command.h | 5 +++++\n> strvec.c | 2 +-\n> strvec.h | 3 +++\n> t/t0007-git-var.sh | 2 +-\n> 12 files changed, 59 insertions(+), 11 deletions(-)\n> \n> \n> base-commit: 06e570c0dfb2a2deb64d217db78e2ec21672f558\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1760%2Fdscho%2Fgit-var-on-windows-v2\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1760/dscho/git-var-on-windows-v2\n> Pull-Request: https://github.com/gitgitgadget/git/pull/1760\n> \n> Range-diff vs v1:\n> \n> -: ----------- > 1: e0970381042 run-command: refactor getting the Unix shell path into its own function\n> -: ----------- > 2: 91ebccbc39f strvec: declare the `strvec_push_nodup()` function globally\n> -: ----------- > 3: a718183bb3b win32: override `fspathcmp()` with a directory separator-aware version\n> -: ----------- > 4: f04cfd91bd9 mingw(is_msys2_sh): handle forward slashes in the `sh.exe` path, too\n> -: ----------- > 5: 707daf246bd run-command(win32): resolve the path to the Unix shell early\n> -: ----------- > 6: a74a7b4bb11 run-command: declare the `git_shell_path()` function globally\n> 1: ef62c3fc122 ! 7: 8bfd23cfa00 var(win32): do report the GIT_SHELL_PATH that is actually used\n> @@ Commit message\n> associated with the current directory.\n> \n> To that end, Git does not actually use the path `/bin/sh` that is\n> - recorded e.g. in Unix shell scripts' hash-bang lines. Instead, as of\n> - 776297548e (Do not use SHELL_PATH from build system in prepare_shell_cmd\n> - on Windows, 2012-04-17), it re-interprets `/bin/sh` as \"look up `sh` on\n> - the `PATH` and use the result instead\".\n> + recorded e.g. when `run_command()` is called with a Unix shell\n> + command-line. Instead, as of 776297548e (Do not use SHELL_PATH from\n> + build system in prepare_shell_cmd on Windows, 2012-04-17), it\n> + re-interprets `/bin/sh` as \"look up `sh` on the `PATH` and use the\n> + result instead\".\n> +\n> + This is the logic users expect to be followed when running `git var\n> + GIT_SHELL_PATH`.\n> \n> However, when 1e65721227 (var: add support for listing the shell,\n> 2023-06-27) introduced support for `git var GIT_SHELL_PATH`, Windows was\n> not special-cased as above, which is why it outputs `/bin/sh` even\n> though that disagrees with what Git actually uses.\n> \n> - Let's fix this, and also adjust the corresponding test case to verify\n> - that it actually finds a working executable.\n> + Let's fix this by using the exact same logic as `prepare_shell_cmd()`,\n> + adjusting the Windows-specific `git var GIT_SHELL_PATH` test case to\n> + verify that it actually finds a working executable.\n> \n> Reported-by: Phillip Wood \n> Signed-off-by: Johannes Schindelin \n> \n> ## builtin/var.c ##\n> +@@\n> + #include \"refs.h\"\n> + #include \"path.h\"\n> + #include \"strbuf.h\"\n> ++#include \"run-command.h\"\n> +\n> + static const char var_usage[] = \"git var (-l | )\";\n> +\n> @@ builtin/var.c: static char *default_branch(int ident_flag UNUSED)\n> \n> static char *shell_path(int ident_flag UNUSED)\n> {\n> -+#ifdef WIN32\n> -+\tchar *p = locate_in_PATH(\"sh\");\n> -+\tconvert_slashes(p);\n> -+\treturn p;\n> -+#else\n> - \treturn xstrdup(SHELL_PATH);\n> -+#endif\n> +-\treturn xstrdup(SHELL_PATH);\n> ++\treturn git_shell_path();\n> }\n> \n> static char *git_attr_val_system(int ident_flag UNUSED)\n> \n"},{"id":"498586","messageId":"xmqqikxaoc7s.fsf@gitster.g","threadId":"61753","inReplyTo":"8bfd23cfa00c351ffdcc25bd29f3b84089544a56.1720739496.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 7/7] var(win32): do report the GIT_SHELL_PATH that is actually used","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-07-12T15:35:03Z","receivedAt":"2024-07-12T15:35:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" \nwrites:\n\n> From: Johannes Schindelin \n>\n> On Windows, Unix-like paths like `/bin/sh` make very little sense. In\n> the best case, they simply don't work, in the worst case they are\n> misinterpreted as absolute paths that are relative to the drive\n> associated with the current directory.\n> To that end, ...\n\nThis does not quite explain why a hardcoded full path does not make\nsense, though. If you do not want \"relative to the current drive\",\nyou can even hardcode the path including the drive letter and now it\nno longer falls into that \"worst case\" because there is no way to\nmisinterpret it as such. I think the real reason is that the port's\n\"relocatable\" nature does not mix well with the \"let's make sure we\nknow what we use exactly by deciding the paths to various tools at\ncompile time\" mentality. So if I were rerolling this I would say\n\n\tGit ported to Windows expect to be installed at user-chosen\n\tplaces, which means hardcoded paths to tools decided at\n\tcompile time are bad fit.\n\nor something.\n\nBut it does not matter if the stated reason is to use the \"find 'sh'\nthat is on PATH\" approach was taken tells the whole story or not.\nThe important thing is that we do ...\n\n> Git does not actually use the path `/bin/sh` that is\n> recorded e.g. when `run_command()` is called with a Unix shell\n> command-line. Instead, as of 776297548e (Do not use SHELL_PATH from\n> build system in prepare_shell_cmd on Windows, 2012-04-17), it\n> re-interprets `/bin/sh` as \"look up `sh` on the `PATH` and use the\n> result instead\".\n\n... this. And with the design constraints that it has to be\ninstallable anywhere and cannot rely on compile-time determined\nhardcoded paths, the above design decision is a very valid one.\n\n> This is the logic users expect to be followed when running `git var\n> GIT_SHELL_PATH`.\n\nAnd matching `git var GIT_SHELL_PATH` to what the code path does\nmake sense.\n\n> diff --git a/builtin/var.c b/builtin/var.c\n> index 5dc384810c0..e30ff45be1c 100644\n> --- a/builtin/var.c\n> +++ b/builtin/var.c\n> @@ -12,6 +12,7 @@\n> #include \"refs.h\"\n> #include \"path.h\"\n> #include \"strbuf.h\"\n> +#include \"run-command.h\"\n> \n> static const char var_usage[] = \"git var (-l | )\";\n> \n> @@ -51,7 +52,7 @@ static char *default_branch(int ident_flag UNUSED)\n> \n> static char *shell_path(int ident_flag UNUSED)\n> {\n> -\treturn xstrdup(SHELL_PATH);\n> +\treturn git_shell_path();\n> }\n\nSimple and clear. Nicely concluded.\n\nThanks. Will queue.\n\n"},{"id":"498599","messageId":"xmqq5xtal0hf.fsf@gitster.g","threadId":"61753","inReplyTo":"3b687707-7aa7-4f4f-8043-0e31fb0c09ce@gmail.com","subject":"Re: [PATCH v2 0/7] var(win32): do report the GIT_SHELL_PATH that is actually used","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-07-12T22:16:44Z","receivedAt":"2024-07-12T22:16:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood writes:\n\n> Hi Johannes\n>\n> On 12/07/2024 00:11, Johannes Schindelin via GitGitGadget wrote:\n>> Changes since v1:\n>> * This patch series now shares the logic that determines the path\n>> of the\n>> Unix shell that Git uses between prepare_shell_cmd() and git var\n>> GIT_SHELL_PATH.\n>\n> This was a pleasant read, each step was well described and easy to\n> follow.\n\nDitto.\n\n> I've left a couple of comments but I think this is good as it\n> is.\n>\n> Thanks\n\nThanks, both.\n"},{"id":"498642","messageId":"pull.1760.v3.git.1720904905.gitgitgadget@gmail.com","threadId":"61753","inReplyTo":"pull.1760.v2.git.1720739496.gitgitgadget@gmail.com","subject":"[PATCH v3 0/7] var(win32): do report the GIT_SHELL_PATH that is actually used","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-07-13T21:08:17Z","receivedAt":"2024-07-13T21:08:29Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Changes since v2:\n\n * Now fspathncmp() is overridden on Windows just like fspathcmp().\n * The win32_fspath*cmp() functions now respect core.ignoreCase.\n\nChanges since v1:\n\n * This patch series now shares the logic that determines the path of the\n Unix shell that Git uses between prepare_shell_cmd() and git var\n GIT_SHELL_PATH.\n\nJohannes Schindelin (7):\n run-command: refactor getting the Unix shell path into its own\n function\n strvec: declare the `strvec_push_nodup()` function globally\n win32: override `fspathcmp()` with a directory separator-aware version\n mingw(is_msys2_sh): handle forward slashes in the `sh.exe` path, too\n run-command(win32): resolve the path to the Unix shell early\n run-command: declare the `git_shell_path()` function globally\n var(win32): do report the GIT_SHELL_PATH that is actually used\n\n builtin/var.c | 3 ++-\n compat/mingw.c | 2 +-\n compat/win32/path-utils.c | 37 +++++++++++++++++++++++++++++++++++++\n compat/win32/path-utils.h | 4 ++++\n dir.c | 4 ++--\n dir.h | 4 ++--\n git-compat-util.h | 8 ++++++++\n run-command.c | 17 ++++++++++++-----\n run-command.h | 5 +++++\n strvec.c | 2 +-\n strvec.h | 3 +++\n t/t0007-git-var.sh | 2 +-\n 12 files changed, 78 insertions(+), 13 deletions(-)\n\n\nbase-commit: 06e570c0dfb2a2deb64d217db78e2ec21672f558\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1760%2Fdscho%2Fgit-var-on-windows-v3\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1760/dscho/git-var-on-windows-v3\nPull-Request: https://github.com/gitgitgadget/git/pull/1760\n\nRange-diff vs v2:\n\n 1: e0970381042 = 1: e0970381042 run-command: refactor getting the Unix shell path into its own function\n 2: 91ebccbc39f = 2: 91ebccbc39f strvec: declare the `strvec_push_nodup()` function globally\n 3: a718183bb3b ! 3: f46315ac0b2 win32: override `fspathcmp()` with a directory separator-aware version\n @@ Commit message\n This means that the paths `a/b` and `a\\b` are equivalent, and\n `fspathcmp()` needs to be made aware of that fact.\n \n + Note that we have to override both `fspathcmp()` and `fspathncmp()`, and\n + the former cannot be a mere pre-processor constant that transforms calls\n + to `fspathcmp(a, b)` into `fspathncmp(a, b, (size_t)-1)` because the\n + function `report_collided_checkout()` in `unpack-trees.c` wants to\n + assign `list.cmp = fspathcmp`.\n +\n + Also note that `fspatheq()` does _not_ need to be overridden because it\n + calls `fspathcmp()` internally.\n +\n Signed-off-by: Johannes Schindelin \n \n ## compat/win32/path-utils.c ##\n +@@\n + #include \"../../git-compat-util.h\"\n ++#include \"../../environment.h\"\n + \n + int win32_has_dos_drive_prefix(const char *path)\n + {\n @@ compat/win32/path-utils.c: int win32_offset_1st_component(const char *path)\n \n \treturn pos + is_dir_sep(*pos) - path;\n }\n +\n -+int win32_fspathcmp(const char *a, const char *b)\n ++int win32_fspathncmp(const char *a, const char *b, size_t count)\n +{\n +\tint diff;\n +\n +\tfor (;;) {\n ++\t\tif (!count--)\n ++\t\t\treturn 0;\n +\t\tif (!*a)\n -+\t\t\treturn !*b ? 0 : -1;\n ++\t\t\treturn *b ? -1 : 0;\n +\t\tif (!*b)\n +\t\t\treturn +1;\n +\n @@ compat/win32/path-utils.c: int win32_offset_1st_component(const char *path)\n +\t\t} else if (is_dir_sep(*b))\n +\t\t\treturn +1;\n +\n -+\t\tdiff = (unsigned char)tolower(*(a++)) - (unsigned char)tolower(*(b++));\n ++\t\tdiff = ignore_case ?\n ++\t\t\t(unsigned char)tolower(*a) - (int)(unsigned char)tolower(*b) :\n ++\t\t\t(unsigned char)*a - (int)(unsigned char)*b;\n +\t\tif (diff)\n +\t\t\treturn diff;\n ++\t\ta++;\n ++\t\tb++;\n +\t}\n ++}\n ++\n ++int win32_fspathcmp(const char *a, const char *b)\n ++{\n ++\treturn win32_fspathncmp(a, b, (size_t)-1);\n +}\n \n ## compat/win32/path-utils.h ##\n @@ compat/win32/path-utils.h: static inline int win32_has_dir_sep(const char *path)\n #define offset_1st_component win32_offset_1st_component\n +int win32_fspathcmp(const char *a, const char *b);\n +#define fspathcmp win32_fspathcmp\n ++int win32_fspathncmp(const char *a, const char *b, size_t count);\n ++#define fspathncmp win32_fspathncmp\n \n #endif\n \n @@ dir.c: int count_slashes(const char *s)\n {\n \treturn ignore_case ? strcasecmp(a, b) : strcmp(a, b);\n }\n +@@ dir.c: int fspatheq(const char *a, const char *b)\n + \treturn !fspathcmp(a, b);\n + }\n + \n +-int fspathncmp(const char *a, const char *b, size_t count)\n ++int git_fspathncmp(const char *a, const char *b, size_t count)\n + {\n + \treturn ignore_case ? strncasecmp(a, b, count) : strncmp(a, b, count);\n + }\n \n ## dir.h ##\n @@ dir.h: int remove_dir_recursively(struct strbuf *path, int flag);\n @@ dir.h: int remove_dir_recursively(struct strbuf *path, int flag);\n -int fspathcmp(const char *a, const char *b);\n +int git_fspathcmp(const char *a, const char *b);\n int fspatheq(const char *a, const char *b);\n - int fspathncmp(const char *a, const char *b, size_t count);\n +-int fspathncmp(const char *a, const char *b, size_t count);\n ++int git_fspathncmp(const char *a, const char *b, size_t count);\n unsigned int fspathhash(const char *str);\n + \n + /*\n \n ## git-compat-util.h ##\n @@ git-compat-util.h: static inline int git_offset_1st_component(const char *path)\n #define offset_1st_component git_offset_1st_component\n #endif\n \n -+\n +#ifndef fspathcmp\n +#define fspathcmp git_fspathcmp\n +#endif\n ++\n ++#ifndef fspathncmp\n ++#define fspathncmp git_fspathncmp\n ++#endif\n +\n #ifndef is_valid_path\n #define is_valid_path(path) 1\n 4: f04cfd91bd9 = 4: 60fde81d35c mingw(is_msys2_sh): handle forward slashes in the `sh.exe` path, too\n 5: 707daf246bd = 5: 797cf9094ea run-command(win32): resolve the path to the Unix shell early\n 6: a74a7b4bb11 = 6: 7c53a4f4707 run-command: declare the `git_shell_path()` function globally\n 7: 8bfd23cfa00 = 7: 7c8eba5bfd8 var(win32): do report the GIT_SHELL_PATH that is actually used\n\n-- \ngitgitgadget\n"},{"id":"498643","messageId":"e0970381042e4e35074c5f095959d3379aad6d9d.1720904905.git.gitgitgadget@gmail.com","threadId":"61753","inReplyTo":"pull.1760.v3.git.1720904905.gitgitgadget@gmail.com","subject":"[PATCH v3 1/7] run-command: refactor getting the Unix shell path into its own function","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-07-13T21:08:18Z","receivedAt":"2024-07-13T21:08:29Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nThis encapsulates the platform-specific logic better.\n\nSigned-off-by: Johannes Schindelin \n---\n run-command.c | 15 ++++++++++-----\n 1 file changed, 10 insertions(+), 5 deletions(-)\n\ndiff --git a/run-command.c b/run-command.c\nindex d9f80fabe6e..59e433bf91c 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -274,17 +274,22 @@ int sane_execvp(const char *file, char * const argv[])\n \treturn -1;\n }\n \n+static const char *git_shell_path(void)\n+{\n+#ifndef GIT_WINDOWS_NATIVE\n+\treturn SHELL_PATH;\n+#else\n+\treturn \"sh\";\n+#endif\n+}\n+\n static const char **prepare_shell_cmd(struct strvec *out, const char **argv)\n {\n \tif (!argv[0])\n \t\tBUG(\"shell command is empty\");\n \n \tif (strcspn(argv[0], \"|&;<>()$`\\\\\\\"' \\t\\n*?[#~=%\") != strlen(argv[0])) {\n-#ifndef GIT_WINDOWS_NATIVE\n-\t\tstrvec_push(out, SHELL_PATH);\n-#else\n-\t\tstrvec_push(out, \"sh\");\n-#endif\n+\t\tstrvec_push(out, git_shell_path());\n \t\tstrvec_push(out, \"-c\");\n \n \t\t/*\n-- \ngitgitgadget\n\n"},{"id":"498644","messageId":"91ebccbc39f21fa73af8bc8c81af721f1ca201bd.1720904905.git.gitgitgadget@gmail.com","threadId":"61753","inReplyTo":"pull.1760.v3.git.1720904905.gitgitgadget@gmail.com","subject":"[PATCH v3 2/7] strvec: declare the `strvec_push_nodup()` function globally","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-07-13T21:08:19Z","receivedAt":"2024-07-13T21:08:30Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nThis function differs from `strvec_push()` in that it takes ownership of\nthe allocated string that is passed as second argument.\n\nThis is useful when appending elements to the string array that have\nbeen freshly allocated and serve no further other purpose after that.\n\nWithout declaring this function globally, call sites would allocate the\nmemory, only to have `strvec_push()` duplicate the string, and then the\nfirst copy would need to be released. Having this function globally\navoids that kind of unnecessary work.\n\nSigned-off-by: Johannes Schindelin \n---\n strvec.c | 2 +-\n strvec.h | 3 +++\n 2 files changed, 4 insertions(+), 1 deletion(-)\n\ndiff --git a/strvec.c b/strvec.c\nindex d4073ec9fa2..f712070f574 100644\n--- a/strvec.c\n+++ b/strvec.c\n@@ -10,7 +10,7 @@ void strvec_init(struct strvec *array)\n \tmemcpy(array, &blank, sizeof(*array));\n }\n \n-static void strvec_push_nodup(struct strvec *array, const char *value)\n+void strvec_push_nodup(struct strvec *array, char *value)\n {\n \tif (array->v == empty_strvec)\n \t\tarray->v = NULL;\ndiff --git a/strvec.h b/strvec.h\nindex 6c7e8b7d503..4b73c1f092e 100644\n--- a/strvec.h\n+++ b/strvec.h\n@@ -46,6 +46,9 @@ void strvec_init(struct strvec *);\n /* Push a copy of a string onto the end of the array. */\n const char *strvec_push(struct strvec *, const char *);\n \n+/* Push an allocated string onto the end of the array, taking ownership. */\n+void strvec_push_nodup(struct strvec *array, char *value);\n+\n /**\n * Format a string and push it onto the end of the array. This is a\n * convenience wrapper combining `strbuf_addf` and `strvec_push`.\n-- \ngitgitgadget\n\n"},{"id":"498645","messageId":"f46315ac0b24879283846682878139641203d4ae.1720904905.git.gitgitgadget@gmail.com","threadId":"61753","inReplyTo":"pull.1760.v3.git.1720904905.gitgitgadget@gmail.com","subject":"[PATCH v3 3/7] win32: override `fspathcmp()` with a directory separator-aware version","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-07-13T21:08:20Z","receivedAt":"2024-07-13T21:08:31Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nOn Windows, the backslash is the directory separator, even if the\nforward slash can be used, too, at least since Windows NT.\n\nThis means that the paths `a/b` and `a\\b` are equivalent, and\n`fspathcmp()` needs to be made aware of that fact.\n\nNote that we have to override both `fspathcmp()` and `fspathncmp()`, and\nthe former cannot be a mere pre-processor constant that transforms calls\nto `fspathcmp(a, b)` into `fspathncmp(a, b, (size_t)-1)` because the\nfunction `report_collided_checkout()` in `unpack-trees.c` wants to\nassign `list.cmp = fspathcmp`.\n\nAlso note that `fspatheq()` does _not_ need to be overridden because it\ncalls `fspathcmp()` internally.\n\nSigned-off-by: Johannes Schindelin \n---\n compat/win32/path-utils.c | 37 +++++++++++++++++++++++++++++++++++++\n compat/win32/path-utils.h | 4 ++++\n dir.c | 4 ++--\n dir.h | 4 ++--\n git-compat-util.h | 8 ++++++++\n 5 files changed, 53 insertions(+), 4 deletions(-)\n\ndiff --git a/compat/win32/path-utils.c b/compat/win32/path-utils.c\nindex ebf2f12eb66..b658ca3f811 100644\n--- a/compat/win32/path-utils.c\n+++ b/compat/win32/path-utils.c\n@@ -1,4 +1,5 @@\n #include \"../../git-compat-util.h\"\n+#include \"../../environment.h\"\n \n int win32_has_dos_drive_prefix(const char *path)\n {\n@@ -50,3 +51,39 @@ int win32_offset_1st_component(const char *path)\n \n \treturn pos + is_dir_sep(*pos) - path;\n }\n+\n+int win32_fspathncmp(const char *a, const char *b, size_t count)\n+{\n+\tint diff;\n+\n+\tfor (;;) {\n+\t\tif (!count--)\n+\t\t\treturn 0;\n+\t\tif (!*a)\n+\t\t\treturn *b ? -1 : 0;\n+\t\tif (!*b)\n+\t\t\treturn +1;\n+\n+\t\tif (is_dir_sep(*a)) {\n+\t\t\tif (!is_dir_sep(*b))\n+\t\t\t\treturn -1;\n+\t\t\ta++;\n+\t\t\tb++;\n+\t\t\tcontinue;\n+\t\t} else if (is_dir_sep(*b))\n+\t\t\treturn +1;\n+\n+\t\tdiff = ignore_case ?\n+\t\t\t(unsigned char)tolower(*a) - (int)(unsigned char)tolower(*b) :\n+\t\t\t(unsigned char)*a - (int)(unsigned char)*b;\n+\t\tif (diff)\n+\t\t\treturn diff;\n+\t\ta++;\n+\t\tb++;\n+\t}\n+}\n+\n+int win32_fspathcmp(const char *a, const char *b)\n+{\n+\treturn win32_fspathncmp(a, b, (size_t)-1);\n+}\ndiff --git a/compat/win32/path-utils.h b/compat/win32/path-utils.h\nindex 65fa3b9263a..a561c700e75 100644\n--- a/compat/win32/path-utils.h\n+++ b/compat/win32/path-utils.h\n@@ -29,5 +29,9 @@ static inline int win32_has_dir_sep(const char *path)\n #define has_dir_sep(path) win32_has_dir_sep(path)\n int win32_offset_1st_component(const char *path);\n #define offset_1st_component win32_offset_1st_component\n+int win32_fspathcmp(const char *a, const char *b);\n+#define fspathcmp win32_fspathcmp\n+int win32_fspathncmp(const char *a, const char *b, size_t count);\n+#define fspathncmp win32_fspathncmp\n \n #endif\ndiff --git a/dir.c b/dir.c\nindex b7a6625ebda..5a23376bdae 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -95,7 +95,7 @@ int count_slashes(const char *s)\n \treturn cnt;\n }\n \n-int fspathcmp(const char *a, const char *b)\n+int git_fspathcmp(const char *a, const char *b)\n {\n \treturn ignore_case ? strcasecmp(a, b) : strcmp(a, b);\n }\n@@ -105,7 +105,7 @@ int fspatheq(const char *a, const char *b)\n \treturn !fspathcmp(a, b);\n }\n \n-int fspathncmp(const char *a, const char *b, size_t count)\n+int git_fspathncmp(const char *a, const char *b, size_t count)\n {\n \treturn ignore_case ? strncasecmp(a, b, count) : strncmp(a, b, count);\n }\ndiff --git a/dir.h b/dir.h\nindex 69a76d8bdd3..a3a2f00f5d9 100644\n--- a/dir.h\n+++ b/dir.h\n@@ -541,9 +541,9 @@ int remove_dir_recursively(struct strbuf *path, int flag);\n */\n int remove_path(const char *path);\n \n-int fspathcmp(const char *a, const char *b);\n+int git_fspathcmp(const char *a, const char *b);\n int fspatheq(const char *a, const char *b);\n-int fspathncmp(const char *a, const char *b, size_t count);\n+int git_fspathncmp(const char *a, const char *b, size_t count);\n unsigned int fspathhash(const char *str);\n \n /*\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex ca7678a379d..71b4d23f038 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -506,6 +506,14 @@ static inline int git_offset_1st_component(const char *path)\n #define offset_1st_component git_offset_1st_component\n #endif\n \n+#ifndef fspathcmp\n+#define fspathcmp git_fspathcmp\n+#endif\n+\n+#ifndef fspathncmp\n+#define fspathncmp git_fspathncmp\n+#endif\n+\n #ifndef is_valid_path\n #define is_valid_path(path) 1\n #endif\n-- \ngitgitgadget\n\n"},{"id":"498646","messageId":"60fde81d35ce58975307e507debbb27ade10853d.1720904905.git.gitgitgadget@gmail.com","threadId":"61753","inReplyTo":"pull.1760.v3.git.1720904905.gitgitgadget@gmail.com","subject":"[PATCH v3 4/7] mingw(is_msys2_sh): handle forward slashes in the `sh.exe` path, too","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-07-13T21:08:21Z","receivedAt":"2024-07-13T21:08:32Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nWhether the full path to the MSYS2 Bash is specified using backslashes\nor forward slashes, in either case the command-line arguments need to be\nquoted in the MSYS2-specific manner instead of using regular Win32\ncommand-line quoting rules.\n\nIn preparation for `prepare_shell_cmd()` to use the full path to\n`sh.exe` (with forward slashes for consistency), let's teach the\n`is_msys2_sh()` function about this; Otherwise 5580.4 'clone with\nbackslashed path' would fail once `prepare_shell_cmd()` uses the full\npath instead of merely `sh`.\n\nThis patch relies on the just-introduced fix where `fspathcmp()` handles\nbackslashes and forward slashes as equivalent on Windows.\n\nSigned-off-by: Johannes Schindelin \n---\n compat/mingw.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/compat/mingw.c b/compat/mingw.c\nindex 6097b8f9e60..29d3f09768c 100644\n--- a/compat/mingw.c\n+++ b/compat/mingw.c\n@@ -1546,7 +1546,7 @@ static int is_msys2_sh(const char *cmd)\n \t\treturn ret;\n \t}\n \n-\tif (ends_with(cmd, \"\\\\sh.exe\")) {\n+\tif (ends_with(cmd, \"\\\\sh.exe\") || ends_with(cmd, \"/sh.exe\")) {\n \t\tstatic char *sh;\n \n \t\tif (!sh)\n-- \ngitgitgadget\n\n"},{"id":"498647","messageId":"797cf9094ea39e9bedb2e60ef86fe3ca089009ba.1720904905.git.gitgitgadget@gmail.com","threadId":"61753","inReplyTo":"pull.1760.v3.git.1720904905.gitgitgadget@gmail.com","subject":"[PATCH v3 5/7] run-command(win32): resolve the path to the Unix shell early","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-07-13T21:08:22Z","receivedAt":"2024-07-13T21:08:33Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nIn 776297548e (Do not use SHELL_PATH from build system in\nprepare_shell_cmd on Windows, 2012-04-17), the hard-coded path to the\nUnix shell was replaced by passing `sh` instead when executing Unix\nshell scripts in Git.\n\nThis was done because the hard-coded path to the Unix shell is incorrect\non Windows because it not only is a Unix-style absolute path instead of\na Windows one, but Git uses the runtime prefix feature on Windows, i.e.\nthe correct path cannot be hard-coded.\n\nNaturally, the `sh` argument will be resolved to the full path of said\nexecutable eventually.\n\nTo help fixing the bug where `git var GIT_SHELL_PATH` currently does not\nreflect that logic, but shows that incorrect hard-coded Unix-style\nabsolute path, let's resolve the full path to the `sh` executable early\nin the `git_shell_path()` function so that we can use it in `git var`,\ntoo, and be sure that the output is equivalent to what `run_command()`\ndoes when it is asked to execute a command-line using a Unix shell.\n\nSigned-off-by: Johannes Schindelin \n---\n run-command.c | 10 ++++++----\n 1 file changed, 6 insertions(+), 4 deletions(-)\n\ndiff --git a/run-command.c b/run-command.c\nindex 59e433bf91c..60a79db8f0e 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -274,12 +274,14 @@ int sane_execvp(const char *file, char * const argv[])\n \treturn -1;\n }\n \n-static const char *git_shell_path(void)\n+static char *git_shell_path(void)\n {\n #ifndef GIT_WINDOWS_NATIVE\n-\treturn SHELL_PATH;\n+\treturn xstrdup(SHELL_PATH);\n #else\n-\treturn \"sh\";\n+\tchar *p = locate_in_PATH(\"sh\");\n+\tconvert_slashes(p);\n+\treturn p;\n #endif\n }\n \n@@ -289,7 +291,7 @@ static const char **prepare_shell_cmd(struct strvec *out, const char **argv)\n \t\tBUG(\"shell command is empty\");\n \n \tif (strcspn(argv[0], \"|&;<>()$`\\\\\\\"' \\t\\n*?[#~=%\") != strlen(argv[0])) {\n-\t\tstrvec_push(out, git_shell_path());\n+\t\tstrvec_push_nodup(out, git_shell_path());\n \t\tstrvec_push(out, \"-c\");\n \n \t\t/*\n-- \ngitgitgadget\n\n"},{"id":"498648","messageId":"7c53a4f4707ec1fcc0e0bf6ca049e632922b0bb3.1720904905.git.gitgitgadget@gmail.com","threadId":"61753","inReplyTo":"pull.1760.v3.git.1720904905.gitgitgadget@gmail.com","subject":"[PATCH v3 6/7] run-command: declare the `git_shell_path()` function globally","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-07-13T21:08:23Z","receivedAt":"2024-07-13T21:08:35Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nThe intention is to use it in `git var GIT_SHELL_PATH`, therefore we\nneed this function to stop being file-local only.\n\nSigned-off-by: Johannes Schindelin \n---\n run-command.c | 2 +-\n run-command.h | 5 +++++\n 2 files changed, 6 insertions(+), 1 deletion(-)\n\ndiff --git a/run-command.c b/run-command.c\nindex 60a79db8f0e..45ba5449323 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -274,7 +274,7 @@ int sane_execvp(const char *file, char * const argv[])\n \treturn -1;\n }\n \n-static char *git_shell_path(void)\n+char *git_shell_path(void)\n {\n #ifndef GIT_WINDOWS_NATIVE\n \treturn xstrdup(SHELL_PATH);\ndiff --git a/run-command.h b/run-command.h\nindex 55f6631a2aa..03e7222d8b5 100644\n--- a/run-command.h\n+++ b/run-command.h\n@@ -195,6 +195,11 @@ int is_executable(const char *name);\n */\n int exists_in_PATH(const char *command);\n \n+/**\n+ * Return the path that is used to execute Unix shell command-lines.\n+ */\n+char *git_shell_path(void);\n+\n /**\n * Start a sub-process. Takes a pointer to a `struct child_process`\n * that specifies the details and returns pipe FDs (if requested).\n-- \ngitgitgadget\n\n"},{"id":"498649","messageId":"7c8eba5bfd82944402e90eaaaf5150e9d7e7d8a3.1720904905.git.gitgitgadget@gmail.com","threadId":"61753","inReplyTo":"pull.1760.v3.git.1720904905.gitgitgadget@gmail.com","subject":"[PATCH v3 7/7] var(win32): do report the GIT_SHELL_PATH that is actually used","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-07-13T21:08:24Z","receivedAt":"2024-07-13T21:08:36Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nOn Windows, Unix-like paths like `/bin/sh` make very little sense. In\nthe best case, they simply don't work, in the worst case they are\nmisinterpreted as absolute paths that are relative to the drive\nassociated with the current directory.\n\nTo that end, Git does not actually use the path `/bin/sh` that is\nrecorded e.g. when `run_command()` is called with a Unix shell\ncommand-line. Instead, as of 776297548e (Do not use SHELL_PATH from\nbuild system in prepare_shell_cmd on Windows, 2012-04-17), it\nre-interprets `/bin/sh` as \"look up `sh` on the `PATH` and use the\nresult instead\".\n\nThis is the logic users expect to be followed when running `git var\nGIT_SHELL_PATH`.\n\nHowever, when 1e65721227 (var: add support for listing the shell,\n2023-06-27) introduced support for `git var GIT_SHELL_PATH`, Windows was\nnot special-cased as above, which is why it outputs `/bin/sh` even\nthough that disagrees with what Git actually uses.\n\nLet's fix this by using the exact same logic as `prepare_shell_cmd()`,\nadjusting the Windows-specific `git var GIT_SHELL_PATH` test case to\nverify that it actually finds a working executable.\n\nReported-by: Phillip Wood \nSigned-off-by: Johannes Schindelin \n---\n builtin/var.c | 3 ++-\n t/t0007-git-var.sh | 2 +-\n 2 files changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/var.c b/builtin/var.c\nindex 5dc384810c0..e30ff45be1c 100644\n--- a/builtin/var.c\n+++ b/builtin/var.c\n@@ -12,6 +12,7 @@\n #include \"refs.h\"\n #include \"path.h\"\n #include \"strbuf.h\"\n+#include \"run-command.h\"\n \n static const char var_usage[] = \"git var (-l | )\";\n \n@@ -51,7 +52,7 @@ static char *default_branch(int ident_flag UNUSED)\n \n static char *shell_path(int ident_flag UNUSED)\n {\n-\treturn xstrdup(SHELL_PATH);\n+\treturn git_shell_path();\n }\n \n static char *git_attr_val_system(int ident_flag UNUSED)\ndiff --git a/t/t0007-git-var.sh b/t/t0007-git-var.sh\nindex ff4fd9348cc..9fc58823873 100755\n--- a/t/t0007-git-var.sh\n+++ b/t/t0007-git-var.sh\n@@ -157,7 +157,7 @@ test_expect_success POSIXPERM 'GIT_SHELL_PATH points to a valid executable' '\n test_expect_success MINGW 'GIT_SHELL_PATH points to a suitable shell' '\n \tshellpath=$(git var GIT_SHELL_PATH) &&\n \tcase \"$shellpath\" in\n-\t*sh) ;;\n+\t[A-Z]:/*/sh.exe) test -f \"$shellpath\";;\n \t*) return 1;;\n \tesac\n '\n-- \ngitgitgadget\n"},{"id":"498830","messageId":"93ed998e-d331-4d16-a4e5-1ae449763205@gmail.com","threadId":"61753","inReplyTo":"pull.1760.v3.git.1720904905.gitgitgadget@gmail.com","subject":"Re: [PATCH v3 0/7] var(win32): do report the GIT_SHELL_PATH that is actually used","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-07-17T14:51:40Z","receivedAt":"2024-07-17T14:51:41Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Johannes\n\nOn 13/07/2024 22:08, Johannes Schindelin via GitGitGadget wrote:\n> Changes since v2:\n> \n> * Now fspathncmp() is overridden on Windows just like fspathcmp().\n> * The win32_fspath*cmp() functions now respect core.ignoreCase.\n\nThese changes in patch 3 look good. Thanks for fixing this\n\nPhillip\n\n> Changes since v1:\n> \n> * This patch series now shares the logic that determines the path of the\n> Unix shell that Git uses between prepare_shell_cmd() and git var\n> GIT_SHELL_PATH.\n> \n> Johannes Schindelin (7):\n> run-command: refactor getting the Unix shell path into its own\n> function\n> strvec: declare the `strvec_push_nodup()` function globally\n> win32: override `fspathcmp()` with a directory separator-aware version\n> mingw(is_msys2_sh): handle forward slashes in the `sh.exe` path, too\n> run-command(win32): resolve the path to the Unix shell early\n> run-command: declare the `git_shell_path()` function globally\n> var(win32): do report the GIT_SHELL_PATH that is actually used\n> \n> builtin/var.c | 3 ++-\n> compat/mingw.c | 2 +-\n> compat/win32/path-utils.c | 37 +++++++++++++++++++++++++++++++++++++\n> compat/win32/path-utils.h | 4 ++++\n> dir.c | 4 ++--\n> dir.h | 4 ++--\n> git-compat-util.h | 8 ++++++++\n> run-command.c | 17 ++++++++++++-----\n> run-command.h | 5 +++++\n> strvec.c | 2 +-\n> strvec.h | 3 +++\n> t/t0007-git-var.sh | 2 +-\n> 12 files changed, 78 insertions(+), 13 deletions(-)\n> \n> \n> base-commit: 06e570c0dfb2a2deb64d217db78e2ec21672f558\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1760%2Fdscho%2Fgit-var-on-windows-v3\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1760/dscho/git-var-on-windows-v3\n> Pull-Request: https://github.com/gitgitgadget/git/pull/1760\n> \n> Range-diff vs v2:\n> \n> 1: e0970381042 = 1: e0970381042 run-command: refactor getting the Unix shell path into its own function\n> 2: 91ebccbc39f = 2: 91ebccbc39f strvec: declare the `strvec_push_nodup()` function globally\n> 3: a718183bb3b ! 3: f46315ac0b2 win32: override `fspathcmp()` with a directory separator-aware version\n> @@ Commit message\n> This means that the paths `a/b` and `a\\b` are equivalent, and\n> `fspathcmp()` needs to be made aware of that fact.\n> \n> + Note that we have to override both `fspathcmp()` and `fspathncmp()`, and\n> + the former cannot be a mere pre-processor constant that transforms calls\n> + to `fspathcmp(a, b)` into `fspathncmp(a, b, (size_t)-1)` because the\n> + function `report_collided_checkout()` in `unpack-trees.c` wants to\n> + assign `list.cmp = fspathcmp`.\n> +\n> + Also note that `fspatheq()` does _not_ need to be overridden because it\n> + calls `fspathcmp()` internally.\n> +\n> Signed-off-by: Johannes Schindelin \n> \n> ## compat/win32/path-utils.c ##\n> +@@\n> + #include \"../../git-compat-util.h\"\n> ++#include \"../../environment.h\"\n> +\n> + int win32_has_dos_drive_prefix(const char *path)\n> + {\n> @@ compat/win32/path-utils.c: int win32_offset_1st_component(const char *path)\n> \n> \treturn pos + is_dir_sep(*pos) - path;\n> }\n> +\n> -+int win32_fspathcmp(const char *a, const char *b)\n> ++int win32_fspathncmp(const char *a, const char *b, size_t count)\n> +{\n> +\tint diff;\n> +\n> +\tfor (;;) {\n> ++\t\tif (!count--)\n> ++\t\t\treturn 0;\n> +\t\tif (!*a)\n> -+\t\t\treturn !*b ? 0 : -1;\n> ++\t\t\treturn *b ? -1 : 0;\n> +\t\tif (!*b)\n> +\t\t\treturn +1;\n> +\n> @@ compat/win32/path-utils.c: int win32_offset_1st_component(const char *path)\n> +\t\t} else if (is_dir_sep(*b))\n> +\t\t\treturn +1;\n> +\n> -+\t\tdiff = (unsigned char)tolower(*(a++)) - (unsigned char)tolower(*(b++));\n> ++\t\tdiff = ignore_case ?\n> ++\t\t\t(unsigned char)tolower(*a) - (int)(unsigned char)tolower(*b) :\n> ++\t\t\t(unsigned char)*a - (int)(unsigned char)*b;\n> +\t\tif (diff)\n> +\t\t\treturn diff;\n> ++\t\ta++;\n> ++\t\tb++;\n> +\t}\n> ++}\n> ++\n> ++int win32_fspathcmp(const char *a, const char *b)\n> ++{\n> ++\treturn win32_fspathncmp(a, b, (size_t)-1);\n> +}\n> \n> ## compat/win32/path-utils.h ##\n> @@ compat/win32/path-utils.h: static inline int win32_has_dir_sep(const char *path)\n> #define offset_1st_component win32_offset_1st_component\n> +int win32_fspathcmp(const char *a, const char *b);\n> +#define fspathcmp win32_fspathcmp\n> ++int win32_fspathncmp(const char *a, const char *b, size_t count);\n> ++#define fspathncmp win32_fspathncmp\n> \n> #endif\n> \n> @@ dir.c: int count_slashes(const char *s)\n> {\n> \treturn ignore_case ? strcasecmp(a, b) : strcmp(a, b);\n> }\n> +@@ dir.c: int fspatheq(const char *a, const char *b)\n> + \treturn !fspathcmp(a, b);\n> + }\n> +\n> +-int fspathncmp(const char *a, const char *b, size_t count)\n> ++int git_fspathncmp(const char *a, const char *b, size_t count)\n> + {\n> + \treturn ignore_case ? strncasecmp(a, b, count) : strncmp(a, b, count);\n> + }\n> \n> ## dir.h ##\n> @@ dir.h: int remove_dir_recursively(struct strbuf *path, int flag);\n> @@ dir.h: int remove_dir_recursively(struct strbuf *path, int flag);\n> -int fspathcmp(const char *a, const char *b);\n> +int git_fspathcmp(const char *a, const char *b);\n> int fspatheq(const char *a, const char *b);\n> - int fspathncmp(const char *a, const char *b, size_t count);\n> +-int fspathncmp(const char *a, const char *b, size_t count);\n> ++int git_fspathncmp(const char *a, const char *b, size_t count);\n> unsigned int fspathhash(const char *str);\n> +\n> + /*\n> \n> ## git-compat-util.h ##\n> @@ git-compat-util.h: static inline int git_offset_1st_component(const char *path)\n> #define offset_1st_component git_offset_1st_component\n> #endif\n> \n> -+\n> +#ifndef fspathcmp\n> +#define fspathcmp git_fspathcmp\n> +#endif\n> ++\n> ++#ifndef fspathncmp\n> ++#define fspathncmp git_fspathncmp\n> ++#endif\n> +\n> #ifndef is_valid_path\n> #define is_valid_path(path) 1\n> 4: f04cfd91bd9 = 4: 60fde81d35c mingw(is_msys2_sh): handle forward slashes in the `sh.exe` path, too\n> 5: 707daf246bd = 5: 797cf9094ea run-command(win32): resolve the path to the Unix shell early\n> 6: a74a7b4bb11 = 6: 7c53a4f4707 run-command: declare the `git_shell_path()` function globally\n> 7: 8bfd23cfa00 = 7: 7c8eba5bfd8 var(win32): do report the GIT_SHELL_PATH that is actually used\n> \n"},{"id":"498831","messageId":"fdb26d61-d755-4daf-ab5d-6e1a36b7b871@gmail.com","threadId":"61753","inReplyTo":"66345b2d-6662-710a-5513-6cc70507b4d0@gmx.de","subject":"Re: [PATCH] var(win32): do report the GIT_SHELL_PATH that is actually used","fromName":"","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-07-17T14:55:46Z","receivedAt":"2024-07-17T14:55:47Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Johannes\n\nOn 11/07/2024 13:03, Johannes Schindelin wrote:\n> Hi Phillip,\n> \n> On Tue, 9 Jul 2024, Phillip Wood wrote:\n> \n>> On 08/07/2024 14:02, Johannes Schindelin via GitGitGadget wrote:\n>>> From: Johannes Schindelin \n>>\n>> Thanks for putting a patch together so quickly\n>>\n>>> static char *shell_path(int ident_flag UNUSED)\n>>> {\n>>> +#ifdef WIN32\n>>> +\tchar *p = locate_in_PATH(\"sh\");\n>>\n>> If I'm reading is_busybox_applet() (which only exists in git-for-windows)\n>> correctly then this will return \"busybox.exe\" under mingit-busybox rather than\n>> ash.exe, so the calling program would have to know to set argv[0] (which is\n>> likely not possible unless the calling program is written in C) or pass \"sh\"\n>> as the first argument. As the code to support busybox does not exist upstream\n>> I guess that's best handled downstream.\n> \n> BusyBox-w32 is unfortunately displaying strange performance patterns. It\n> is partially (and expectedly) faster than the MSYS2 Bash, but in other\n> scenarios it is substantially slower (which is totally unexpected).\n> \n> Some time ago, I tried to make this all work and investigate the\n> unexpected performance issues (and hoped to fix them, too), but ran out of\n> time [*1*]. That was almost two years ago, and I am unsure whether I will\n> ever be able to elevate the BusyBox flavor of MinGit to a non-experimental\n> state.\n> \n> My original plan was to eventually no longer include `busybox.exe` in\n> the mingit-busybox packages, but instead a copy of that executable with\n> the name `sh.exe` and thereby have it work without that hack in the Git\n> code to call the `busybox.exe` with the `sh` argument inserted before the\n> regular command-line arguments.\n\nThanks for the information.\n\n> In the context of the patch (or now: patch series) at hand, I don't think\n> we need to let BusyBox play any role.\n\nAgreed\n\nBest Wishes\n\nPhillip\n\n> Ciao,\n> Johannes\n> \n> Footnote *1*: Interested parties can find the latest state here:\n> https://github.com/git-for-windows/git/compare/main...dscho:git:busybox\n"},{"id":"498883","messageId":"ZphKBe5gmWKJnrqm@tapette.crustytoothpaste.net","threadId":"61753","inReplyTo":"pull.1760.v3.git.1720904905.gitgitgadget@gmail.com","subject":"Re: [PATCH v3 0/7] var(win32): do report the GIT_SHELL_PATH that is actually used","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2024-07-17T22:47:33Z","receivedAt":"2024-07-17T22:47:41Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2024-07-13 at 21:08:17, Johannes Schindelin via GitGitGadget wrote:\n> Changes since v2:\n> \n> * Now fspathncmp() is overridden on Windows just like fspathcmp().\n> * The win32_fspath*cmp() functions now respect core.ignoreCase.\n> \n> Changes since v1:\n> \n> * This patch series now shares the logic that determines the path of the\n> Unix shell that Git uses between prepare_shell_cmd() and git var\n> GIT_SHELL_PATH.\n> \n> Johannes Schindelin (7):\n> run-command: refactor getting the Unix shell path into its own\n> function\n> strvec: declare the `strvec_push_nodup()` function globally\n> win32: override `fspathcmp()` with a directory separator-aware version\n> mingw(is_msys2_sh): handle forward slashes in the `sh.exe` path, too\n> run-command(win32): resolve the path to the Unix shell early\n> run-command: declare the `git_shell_path()` function globally\n> var(win32): do report the GIT_SHELL_PATH that is actually used\n\nThis series seems reasonable to me as well. I of course can't speak to\nwhether the approach for finding sh is the right one, since I'm not a\nWindows developer, but I have confidence you know the answer and have\nthought through it fully.\n-- \nbrian m. carlson (they/them or he/him)\nToronto, Ontario, CA\n"},{"id":"498884","messageId":"xmqqa5if3a57.fsf@gitster.g","threadId":"61753","inReplyTo":"ZphKBe5gmWKJnrqm@tapette.crustytoothpaste.net","subject":"Re: [PATCH v3 0/7] var(win32): do report the GIT_SHELL_PATH that is actually used","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-07-17T22:51:16Z","receivedAt":"2024-07-17T22:51:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"brian m. carlson\" writes:\n\n>> Johannes Schindelin (7):\n>> run-command: refactor getting the Unix shell path into its own\n>> function\n>> strvec: declare the `strvec_push_nodup()` function globally\n>> win32: override `fspathcmp()` with a directory separator-aware version\n>> mingw(is_msys2_sh): handle forward slashes in the `sh.exe` path, too\n>> run-command(win32): resolve the path to the Unix shell early\n>> run-command: declare the `git_shell_path()` function globally\n>> var(win32): do report the GIT_SHELL_PATH that is actually used\n>\n> This series seems reasonable to me as well. I of course can't speak to\n> whether the approach for finding sh is the right one, since I'm not a\n> Windows developer, but I have confidence you know the answer and have\n> thought through it fully.\n\n... and the series will be in 2.46-rc1\n\nThanks.\n"}]}