| Message ID | 20230622195059.320593-2-sandals@crustytoothpaste.net (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Additional variables for git var | expand |
On Thu, Jun 22, 2023 at 4:03 PM brian m. carlson <sandals@crustytoothpaste.net> wrote: > On most Unix systems, finding a suitable shell is easy: one simply uses > "sh" with an appropriate PATH value. However, in many Windows > environments, the shell is shipped alongside Git, and it may or may not > be in PATH, even if Git is. > > In such an environment, it can be very helpful to query Git for the > shell it's using, since other tools may want to use the same shell as > well. To help them out, let's add a variable, GIT_SHELL_PATH, that > points to the location of the shell. > > On Unix, we know our shell must be executable to be functional, so > assume that the distributor has correctly configured their environment, > and use that as a basic test. On Git for Windows, we know that our > shell will be one of a few fixed values, all of which end in "sh" (such > as "bash"). This seems like it might be a nice test on Unix as well, > since it is customary for all shells to end in "sh", but there probably > exist such systems that don't have such a configuration, so be careful > here not to break them. > > Signed-off-by: brian m. carlson <bk2204@github.com> > --- > diff --git a/t/t0007-git-var.sh b/t/t0007-git-var.sh > @@ -147,6 +147,18 @@ test_expect_success 'get GIT_SEQUENCE_EDITOR with configuration and environment > +test_expect_success POSIXPERM 'GIT_SHELL_PATH points to a valid executable' ' > + git var GIT_SHELL_PATH >shell && > + test -x "$(cat shell)" > +' This can be implemented more simply without a temporary file: shpath=$(git var GIT_SHELL_PATH) && test -x "$shpath" This is safe since the exit code of the Git command is preserved across the `shpath=...` assignment. > +# We know in this environment that our shell will be one of a few fixed values > +# that all end in "sh". > +test_expect_success MINGW 'GIT_SHELL_PATH points to a suitable shell' ' > + git var GIT_SHELL_PATH >shell && > + grep "sh\$" shell > +' Similarly, there is no need for a temporary file or an extra process. This can all be done entirely in the shell itself: shpath=$(git var GIT_SHELL_PATH) && case "$shpath" in *sh) ;; *) return 1 esac
Eric Sunshine <sunshine@sunshineco.com> writes: > This can be implemented more simply without a temporary file: > > shpath=$(git var GIT_SHELL_PATH) && > test -x "$shpath" > > This is safe since the exit code of the Git command is preserved > across the `shpath=...` assignment. Correct. I also suspect that we want to add test_path_is_executable helper next to test_path_is_{file,dir,missing} helpers and list it in t/README. One downside of your approach is that the output from the command is only in $shpath and cannot be observed easily in the $TRASH_DIRECTORY after the test fails, but with such a helper we can report the problematic path when the expectation fails. Thanks.
On Thu, Jun 22, 2023 at 5:05 PM Junio C Hamano <gitster@pobox.com> wrote: > Eric Sunshine <sunshine@sunshineco.com> writes: > > This can be implemented more simply without a temporary file: > > > > shpath=$(git var GIT_SHELL_PATH) && > > test -x "$shpath" > > > > This is safe since the exit code of the Git command is preserved > > across the `shpath=...` assignment. > > Correct. I also suspect that we want to add test_path_is_executable > helper next to test_path_is_{file,dir,missing} helpers and list it > in t/README. One downside of your approach is that the output from > the command is only in $shpath and cannot be observed easily in the > $TRASH_DIRECTORY after the test fails, but with such a helper we can > report the problematic path when the expectation fails. Good idea. A test_path_is_executable() function would also be a good place to encapsulate the Windows-specific hackiness.
On 2023-06-22 at 20:42:31, Eric Sunshine wrote: > This can be implemented more simply without a temporary file: > > shpath=$(git var GIT_SHELL_PATH) && > test -x "$shpath" > > This is safe since the exit code of the Git command is preserved > across the `shpath=...` assignment. I can do this in v2. > Similarly, there is no need for a temporary file or an extra process. > This can all be done entirely in the shell itself: > > shpath=$(git var GIT_SHELL_PATH) && > case "$shpath" in > *sh) ;; > *) return 1 > esac That's true, but this is much uglier and harder to read.
On 2023-06-22 at 21:05:39, Junio C Hamano wrote: > Correct. I also suspect that we want to add test_path_is_executable > helper next to test_path_is_{file,dir,missing} helpers and list it > in t/README. One downside of your approach is that the output from > the command is only in $shpath and cannot be observed easily in the > $TRASH_DIRECTORY after the test fails, but with such a helper we can > report the problematic path when the expectation fails. At first glance, I thought that was a good idea, too, but unfortunately there is no way to make that work on Windows. That's why all of our tests skip those assertions with POSIXPERM, and why my tests specifically look for something different on Windows. We could in theory just make it always succeed there, but my concern with writing such a function is that people will think it works generally, when in fact it does not. That's why, typically throughout the codebase, we specifically use "test -x".
"brian m. carlson" <sandals@crustytoothpaste.net> writes: > On 2023-06-22 at 21:05:39, Junio C Hamano wrote: >> Correct. I also suspect that we want to add test_path_is_executable >> helper next to test_path_is_{file,dir,missing} helpers and list it >> in t/README. One downside of your approach is that the output from >> the command is only in $shpath and cannot be observed easily in the >> $TRASH_DIRECTORY after the test fails, but with such a helper we can >> report the problematic path when the expectation fails. > > At first glance, I thought that was a good idea, too, but unfortunately > there is no way to make that work on Windows. That's why all of our > tests skip those assertions with POSIXPERM, and why my tests > specifically look for something different on Windows. > > We could in theory just make it always succeed there, but my concern > with writing such a function is that people will think it works > generally, when in fact it does not. That's why, typically throughout > the codebase, we specifically use "test -x". Hmph. I would have thought that test_path_is_executable that is based on "test -x" and gives a diagnosis when "test -x" fails would be better than using bare "test -x" and be silent, even on Windows.
diff --git a/Documentation/git-var.txt b/Documentation/git-var.txt index f40202b8e3..f0f647e14a 100644 --- a/Documentation/git-var.txt +++ b/Documentation/git-var.txt @@ -71,6 +71,9 @@ endif::git-default-pager[] GIT_DEFAULT_BRANCH:: The name of the first branch created in newly initialized repositories. +GIT_SHELL_PATH:: + The path of the binary providing the POSIX shell for commands which use the shell. + SEE ALSO -------- linkgit:git-commit-tree[1] diff --git a/builtin/var.c b/builtin/var.c index 2149998980..f97178eed1 100644 --- a/builtin/var.c +++ b/builtin/var.c @@ -36,6 +36,11 @@ static const char *default_branch(int flag) return git_default_branch_name(1); } +static const char *shell_path(int flag) +{ + return SHELL_PATH; +} + struct git_var { const char *name; const char *(*read)(int); @@ -47,6 +52,7 @@ static struct git_var git_vars[] = { { "GIT_SEQUENCE_EDITOR", sequence_editor }, { "GIT_PAGER", pager }, { "GIT_DEFAULT_BRANCH", default_branch }, + { "GIT_SHELL_PATH", shell_path }, { "", NULL }, }; diff --git a/t/t0007-git-var.sh b/t/t0007-git-var.sh index eeb8539c1b..270bd4e512 100755 --- a/t/t0007-git-var.sh +++ b/t/t0007-git-var.sh @@ -147,6 +147,18 @@ test_expect_success 'get GIT_SEQUENCE_EDITOR with configuration and environment ) ' +test_expect_success POSIXPERM 'GIT_SHELL_PATH points to a valid executable' ' + git var GIT_SHELL_PATH >shell && + test -x "$(cat shell)" +' + +# We know in this environment that our shell will be one of a few fixed values +# that all end in "sh". +test_expect_success MINGW 'GIT_SHELL_PATH points to a suitable shell' ' + git var GIT_SHELL_PATH >shell && + grep "sh\$" shell +' + # For git var -l, we check only a representative variable; # testing the whole output would make our test too brittle with # respect to unrelated changes in the test suite's environment.