Fix various Windows test bugs #2324
Merged
+151
−48
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Unlike on Unix-like systems, it is common for a Windows system to
have no `sh` or `sh.exe` in `PATH`. We use `gix_path::env::shell()`
in most cases to find the appropriate path. On Windows, this finds
a suitable `sh` command in the Git for Windows installation, if
applicable, and otherwise falls back to `sh.exe`. However, this was
not being done in some `gix-credentials` tests.
The affected tests use shell script credential helper fixtures that
need to be invoked in a way that forces them to be run by a shell
(without being able to pass through parameters into `gix-command`
that are guaranteed to force this). To do this, `sh ` is prepended
to the command. Since Windows need not be able to find `sh`, this
breaks on some Windows systems.
Because this specific code is in the test suite and only affects
tests, it should be acceptable to make some imperfect assumptions
about how the required concatenation can be performed. Assume:
1. That the string returned by `gix_path::env::shell()` is valid
Unicode, or that it is acceptable for these tests to panic.
2. That the string either already has all separators as `/` (we try
weakly to achieve this in `shell()` but don't force it), or they
can be changed to `/` in a straightforward way that usually
works on files in Git repositories themselves but may not always
work (due, for example, to `\\?\` and `//?/` paths having
different rules for the interpretation of `/` appearing later).
This is not expeted to be safe enough for general use, but it ought
to be okay for the test suite.
This commit makes those assumptions and uses
- paths for `sh`, with added conversion of `\` to `/`, and
- its script path argument, which already had that conversion,
adding POSIX shell single-quoting to each. Quoting the path to
`sh` is needed if it is under a Git installation directory like
`C:/Program Files/Git/bin`; not doing so would introduce a test
suite regression at least as severe as the test bug being fixed.
Quoting the path to its argument is done to avoid future problems
or possibly fix an existing test bug.
This technique shoud allow all `gix-credentials` tests that are
broken in the way described above to work on affected Windows
systems. But for now it is applied in only one of two places
where the code would need to be changed, so it only fixes these
tests:
gix-credentials::credentials helper::invoke::get
gix-credentials::credentials helper::invoke::program::path_to_helper_as_script_to_workaround_executable_bits
gix-credentials::credentials helper::invoke::store_and_reject
It may at first seem as though adding the quoting would be enough
by itself to cause the commands to be run in a shell, because shell
quoting is being used. However, that is only enough to cause the
`command_may_be_shell_script()` call in `Program::to_command()` to
set `use_shell` to `true`. It usually does not cause the command to
be run in a shell on Windows, where `allow_manual_arg_splitting`
defaults to `true`. The manual argument splitting is able to do
both field splitting and quote removal in simple cases, so no shell
gets used.
Although it's possible to suppress manual argument splitting
indirectly by including even more characters treated specially by a
shell, including some that manual argument splitting can't handle,
to do so would be inelegant, unclear, and possibly brittle.
Therefore, this passes a pretty good path to a likely usable
`sh.exe`.
This builds on the partial test suite fix in the preceding commit
to fix the remaining affected tests. The previously fixed tests
were in `helper::invoke`. These are in `helper::cascade::invoke`:
gix-credentials::credentials helper::cascade::invoke::credentials_are_filled_in_one_by_one_and_stop_when_complete
gix-credentials::credentials helper::cascade::invoke::expired_credentials_are_not_returned
gix-credentials::credentials helper::cascade::invoke::failing_helpers_for_filling_dont_interrupt
gix-credentials::credentials helper::cascade::invoke::helpers_can_quit_and_their_creds_are_taken_if_complete
gix-credentials::credentials helper::cascade::invoke::bogus_password_overrides_any_helper_and_helper_overrides_username_in_url
gix-credentials::credentials helper::cascade::invoke::helpers_can_set_any_context_value
gix-credentials::credentials helper::cascade::invoke::helpers_can_set_any_context_value_using_the_url_only
gix-credentials::credentials helper::cascade::invoke::partial_credentials_can_be_overwritten_by_complete_ones
gix-credentials::credentials helper::cascade::invoke::urls_are_split_in_get_but_can_skip_the_path_in_host_only_urls
gix-credentials::credentials helper::cascade::invoke::urls_are_split_in_get_to_support_scripts
gix-credentials::credentials helper::cascade::invoke::usernames_in_urls_are_kept_if_the_helper_does_not_overwrite_it
However, the fix may still benefit from refactoring.
- Move `script_helper()` up to the `helper` test module, so both modules that use it reach upward for it, rather than one reaching to its "cousin" to access a test helper function that appears to be for only its own module. - Remove imports that became unused in the preceding commit or from the changes in this one.
These shell scripts are all written for `sh` and they are currently always invoked as arguments to an explicit `sh` interpreter (either `sh`, or a dynamically determined `sh.exe` path on Windows). But they had a `bash` shebang. This changes their shebang to the simple shebang for `sh` to avoid confusion. This is not expected to change the behavior of the test suite; it is mainly for clarity. (This change shouldn't be made to most other test fixture scripts; they're intended for `bash`, often using `bash`-specific features.)
Two tests of `gix-command` for external commands used `ls` on Unix-like systems and `dir.exe` on Windows. But Windows does not ship a `dir.exe` command, as `dir` in the `cmd.exe` shell is a builtin and `dir` in PowerShell is an alias to a builtin cmdlet. The reason `dir.exe` often worked is that Git Bash and other MSYS2 (or MSYS or Cygwin) environments provide GNU `ls`, which actually supplies three commands, `ls`, `dir`, and `vdir`, which differ by their default options but otherwise behave the same. When no such environment's `bin` directories are in `PATH`, attempting to run `dir.exe` will generally fail, causing the tests to fail. This replaces `dir.exe` with `attrib.exe` and, in the test that passes an argument, adjusts the argument accordingly. (The argument substitution does not preserve meaning, but it is a reasonable argument to pass to `attrib.exe`.) Although `attrib.exe` can be used to set attributes, these commands do not do so (and also avoid appearing to do so). Two commands that are typically present in `System32`, including in native Windows containers and even Nano Server containers, are, very roughly speaking, similar to `ls`, in that they provide filesystem metadata and will do so both implicitly on the current directory or explicitly on specified entries. One is `icacls`, which has the advantage of returning nonzero when it runs but fails, but the disadvantage that it fails when run on an entry in a 9P share accessed from the Windows side. (Shares appearing as network shares that provide access to installed WSL2 distributions' filesystems are 9P shares. There might also be more uses of 9P in Windows in the future.) The other is `attrib`, which has the disavantage of returning zero even when it runs but fails, but the advantage that it work in 9P shares. `attrib` does usually fail when passed the root directory of a filesystem (which is strange), but this is both fairly unlikely in the current use and also acceptable since it still returns zero. Failing to run the command at all will of course not appear like success, which is what really matters. Another command that is sometimes useful for testing the ability to run external commands, and that is present even in Nano Server containers, is `hostname`. However, this can only reasonably be called with zero arguments for testing, since calling it with more arguments fails if the Windows `hostname` command is found, but may do something else such as attempting to set the hostname if the Git Bash (or other MSYS2, etc.) version is found.
+ For a test case that typically runs external commands, note an edge case where a builtin of the same name exists.
So we have zero, one, and two-argument test cases for running commands that are available as external commands and also typically (though not always) as builtins.
This moves the test case that really always must use a shell below those that don't need to (and in practice don't) use a shell.
These are like the tests originally named that way (and renamed to reflect that the commands they run are not shell-specific), but with `:;` prepended to the command to force a shell to really run it, even on Windows.
These should work in all environments (with `sh` available).
This makes the `shell_builtin_or_command_in_path*` tests only run on Unix-like systems. Although they're able to pass in some commonly used Windows environments, including Git Bash and the environment used in GitHub Actions runners, they actually rely on `printf` (and, for the recently added zero-argument one, `echo`) being present in a `PATH` directory. This should always be the case on Unix-like systems (at least if attempting to abide by the POSIX requirement that such commands be available externally whether or nor they are also shell builtins), but it is often not the case on Windows, and when it isn't, this doesn't indicate any problem with the Windows environment. Variants of these tests have recently been added that ensure a shell is actually used: both implicitly, by prepending `:;` to the commands so manual argument splitting cannot be used instead of running a shell, and (in other test cases) explicitly, by calling `with_shell()`, which suppreses argument splitting. Therefore, sufficient coverage for the functionality being tested here is present on Windows even without the versions that take the form of calling a builtin but actually in practice rely on an external command. This turns those off on Windows, but keeps them on Unix-like systems since they may guard against regressions if an attempt is ever made to automatically detect efforts to call a shell builtin and suppress implicit argument splitting. (However, this is not to say that such a feature ought to be implemented, nor to predict that it will: most likely this would not make sense to add, since it would be hard to get right, but also beacuse it would diverge from the behavior of Git. But if Git ever does this, then the regression tests against doing it incorrectly may be helpful.)
The `extract_delayed_key()` function is only used by test cases in the same module.
Two test cases use `cat` as a clean and smudge filter, to check for regressions of GitoxideLabs#2080 (fixed in GitoxideLabs#2262). These tests passed on Unix-like systems. They also passed in Windows environments where a `cat` command is in `PATH`, including Git Bash environments and the full environments on Windows CI runners. However, they failed with "program not found" errors in Windows environments where `cat` is not in a `PATH` directory. The purpose of the two tests cases is not to make sure the exact command `cat` can be used successfully as a clean and smudge filter on all systems, but instead to produce a condition that caused a deadlock (GitoxideLabs#2080) prior to the improvements in GitoxideLabs#2262. Therefore, it can be considered a test bug that the tests were often unable to specify a usable `cat` command on Windows (even when one existed). This commit fixes that test bug, by resolving `cat` to a full path that uses `/` separators but that is otherwise a Windows-style path, and by single quoting the path for use in a shell, passing that instead of the literal command `cat`. If it cannot be found, `cat.exe` is used as a fallback. Non-Windows systems continue always to use `cat`. The following tests that previously passed on Windows when `cat` is in `PATH`, but failed on Windows when `cat` is not in `PATH`, now pass on Windows whether `cat` is in `PATH` or not: gix-filter::filter driver::apply::large_file_with_cat_filter_does_not_hang gix-filter::filter driver::apply::large_file_with_cat_filter_early_drop The technique used to resolve `cat` to the a usable full path is somewhat ad-hoc. At least for now, it runs a command in a shell that uses the shell to look up `cat` and when uses `cygpath` to resolve it. (`cygpath` supports making Windows paths with `/` separators with the `--mixed`/`-m` option.)
Byron
reviewed
Jan 1, 2026
| #[test] | ||
| fn sh_shell_specific_script_code_with_multiple_extra_args() -> crate::Result { | ||
| let out = gix_command::prepare("printf") | ||
| let out = gix_command::prepare(":;printf") |
Member
There was a problem hiding this comment.
A nice way to force a shell to be used!
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This fixes a few bugs in the test suite, which either primarily or solely affect Windows environments. These bugs do not currently appear to cause any tests to wrongly fail or wrongly pass on CI, but in some common local Windows setups they prevent a few tests from passing that ought to pass. See the commit messages for complete details.