From ba76201f619d32a3606609d14e3bb0f1dfb100b7 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Tue, 18 Feb 2025 14:42:28 -0500 Subject: [PATCH 1/7] feat: Prefer `/` over `\` in `bash.exe` path (for fixtures) Starting in #1712, `gix-testtools` looks for `bash.exe` on Windows in one of its common locations as provided by Git for Windows, by taking the path given by `git --exec-path`, going up by three components, and going down to `bin/bash.exe` under that. But the `bin` and `bash.exe` components were appended in a way that used `\` directory separators. Ordinarily, that would be ideal, since `\` is the primary directory separator on Windows. However, in this case, appending with `\` produces a path that is harder to read (especially in diagostic messages), and that may cause problems if it is processed by a shell or in a way that is intended to operate similarly to shell processing of `\`. A path that does not explicitly prefer `/` but instead uses `PathBuf::push` will have `\` in front of the new components, but will still usually have `/` in front of the old components. This is because, aside from the unusual case that the `GIT_EXEC_PATH` environment vairable is set explicitly and its value uses all `\` separators, the output of `git --exec-path`, which we use to find where `git` installed on Windows, uses `/` separators. The effect of this change seems to be fairly minor, with existing test fixtures having worked before it was done. This is partly because, on Windows, the value of `argv[0]` that the shell actually sees -- and that populates `$0` when no script name is available, as happens in `bash -c '...'` with no subsequent arguments -- is translated by an MSYS DLL such as `msys-2.0.dll` (or, for Cygwin, `cygwin1.dll`) into a Unix-style path meaningful to the shell. This also refactors for clarity, and adds new tests related to the change. --- tests/tools/src/lib.rs | 59 +++++++++++++++++++++++++++++++++--------- 1 file changed, 47 insertions(+), 12 deletions(-) diff --git a/tests/tools/src/lib.rs b/tests/tools/src/lib.rs index 82510d627de..66d98f70664 100644 --- a/tests/tools/src/lib.rs +++ b/tests/tools/src/lib.rs @@ -653,20 +653,27 @@ fn configure_command<'a, I: IntoIterator, S: AsRef>( } fn bash_program() -> &'static Path { - if cfg!(windows) { - // TODO(deps): Once `gix_path::env::shell()` is available, maybe do `shell().parent()?.join("bash.exe")` - static GIT_BASH: Lazy> = Lazy::new(|| { + // TODO(deps): *Maybe* add something to `gix-path` like `env::shell()` that can be used to + // find bash and, once the version `gix-testtools` depends on has it, use it. + static GIT_BASH: Lazy = Lazy::new(|| { + if cfg!(windows) { GIT_CORE_DIR - .parent()? - .parent()? - .parent() - .map(|installation_dir| installation_dir.join("bin").join("bash.exe")) + .ancestors() + .nth(3) + .map(OsString::from) + .map(|mut raw_path| { + // Go down to where `bash.exe` usually is. Keep using `/` separators (not `\`). + raw_path.push("/bin/bash.exe"); + raw_path + }) + .map(PathBuf::from) .filter(|bash| bash.is_file()) - }); - GIT_BASH.as_deref().unwrap_or(Path::new("bash.exe")) - } else { - Path::new("bash") - } + .unwrap_or_else(|| "bash.exe".into()) + } else { + "bash".into() + } + }); + GIT_BASH.as_ref() } fn write_failure_marker(failure_marker: &Path) { @@ -1059,4 +1066,32 @@ mod tests { fn bash_program_ok_for_platform() { assert_eq!(bash_program(), Path::new("bash")); } + + #[test] + fn bash_program_unix_path() { + let path = bash_program() + .to_str() + .expect("This test depends on the bash path being valid Unicode"); + assert!( + !path.contains('\\'), + "The path to bash should have no backslashes, barring very unusual environments" + ); + } + + fn is_rooted_relative(path: impl AsRef) -> bool { + let p = path.as_ref(); + p.is_relative() && p.has_root() + } + + #[test] + #[cfg(windows)] + fn unix_style_absolute_is_rooted_relative() { + assert!(is_rooted_relative("/bin/bash"), "can detect paths like /bin/bash"); + } + + #[test] + fn bash_program_absolute_or_unrooted() { + let bash = bash_program(); + assert!(!is_rooted_relative(bash), "{bash:?}"); + } } From 73d30d7a1e61388a415b27552aa2596abb0238b5 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Sun, 23 Feb 2025 17:52:06 -0500 Subject: [PATCH 2/7] Look for bash in `(git root)/usr/bin`, not `(git root)/bin` This is to help support running them with a shell from the Git for Windows SDK, which, relative to the directory where it's installed in, provides a `usr/bin` directory with a `bash.exe` executable, like the same-named directory in a non-SDK full installation of Git for Windows via the installer or PortableGit or or via package managers such as `scoop` that repackage PortableGit, but unlike those other installations does not provide a `bin` directory. Inside the MSYS2 "Git Bash" provided as part of the Git for Windows SDK, there are both `/bin` and `/usr/bin` directories, but this not related to the `bin` subdirectory of the root of the installation. On both regular Git for Windows "Git Bash" and Git for Windows SDK "Git Bash" environments, `/bin` in the environment is mounted to refer to the same directory as `/usr/bin`. In both, this is separate from `(git root)/bin` as accessed outside that environment. For example, in an SDK "Git Bash": $ mount C:/git-sdk-64 on / type ntfs (binary,noacl,auto) C:/git-sdk-64/usr/bin on /bin type ntfs (binary,noacl,auto) C:/Users/ek/AppData/Local/Temp on /tmp type ntfs (binary,noacl,posix=0,usertemp) C: on /c type ntfs (binary,noacl,posix=0,user,noumount,auto) $ cygpath -w /bin /usr/bin C:\git-sdk-64\usr\bin C:\git-sdk-64\usr\bin And in a non-SDK "Git Bash": $ mount C:/Users/ek/AppData/Local/Temp on /tmp type ntfs (binary,noacl,posix=0,usertemp) C:/Users/ek/scoop/apps/git/2.48.1 on / type ntfs (binary,noacl,auto) C:/Users/ek/scoop/apps/git/2.48.1/usr/bin on /bin type ntfs (binary,noacl,auto) C: on /c type ntfs (binary,noacl,posix=0,user,noumount,auto) $ cygpath -w /bin /usr/bin C:\Users\ek\scoop\apps\git\2.48.1\usr\bin C:\Users\ek\scoop\apps\git\2.48.1\usr\bin The former has another `bin` directory alongside the `usr` directory on disk, containing shims for `git.exe`, `sh.exe`, and `bash.exe`, while the latter does not, but in neither case does `/bin` inside Git Bash refer to it. This other `bin` directory was formerly used to find `bash.exe` to run test fixture scripts. A possible reason to continue using `(git root)/bin` would be if the shims modify the environment in a way that makes the fixtures operate better. --- tests/tools/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/tools/src/lib.rs b/tests/tools/src/lib.rs index 66d98f70664..de52385308e 100644 --- a/tests/tools/src/lib.rs +++ b/tests/tools/src/lib.rs @@ -663,7 +663,7 @@ fn bash_program() -> &'static Path { .map(OsString::from) .map(|mut raw_path| { // Go down to where `bash.exe` usually is. Keep using `/` separators (not `\`). - raw_path.push("/bin/bash.exe"); + raw_path.push("/usr/bin/bash.exe"); raw_path }) .map(PathBuf::from) From d290ad962fe88e2aa28d23d412117f59ee5664c0 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Sun, 16 Mar 2025 04:57:45 -0400 Subject: [PATCH 3/7] Instrument make_remote_repos.sh to view `config` corruption The change in the previous commit of switching to the non-shim `bash.exe` in `(git root)/usr/bin` causes problems, because the environment may not be correct for shell commands and scripts. In particular, the `PATH` might not enable standard POSIX tools to be found, or the tools that are found may interoperate incorrectly with the shell. The latter caused failures in #1862 in an analogous choice of `sh.exe`, which were addressed by preferring the shim when available. See: - https://github.com/GitoxideLabs/gitoxide/pull/1862#issuecomment-2692158831 Here, 90 tests started to fail when the test suite was run locally from PowerShell (i.e. not a Git Bash environment) on a Windows 10 system that, in addition to a full Git for Windows installation, contains a separate non-GfW MSYS2 installation whose `bin` directories are in `PATH` even in non-MSYS2 environments. The failures were described, and most of them investigated, as follows: - https://github.com/GitoxideLabs/gitoxide/pull/1864#issuecomment-2724464883 - https://gist.github.com/EliahKagan/3c5eebd091e66d8c912fddbce0a064cd - https://gist.github.com/EliahKagan/17066ad1f7b0aa98e4fdf4642abe1d93 Most failures, including all those that were unintuitive, were directly or indirectly due to the `make_remote_repos.sh` fixture script encountering the error: fatal: bad config line 10 in file ./config This happened due to the same incorrect behavior of `>>`, when used by a shell that links to one `msys-2.0.dll` running a program that links to a different `msys-2.0.dll` of another version or build, as caused the failure encountered with the non-shim in #1862. (It may be the handful of other failures are also caused by this `>>` problem, but as of now that has not been examined.) This commit temporarily instruments that fixture script so that, when tests are run, the observations and analysis in the last gist above can be confirmed. (These changes are also shown there.) --- gix/tests/fixtures/make_remote_repos.sh | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/gix/tests/fixtures/make_remote_repos.sh b/gix/tests/fixtures/make_remote_repos.sh index 5411fc177c3..1eaccc404cc 100755 --- a/gix/tests/fixtures/make_remote_repos.sh +++ b/gix/tests/fixtures/make_remote_repos.sh @@ -1,5 +1,5 @@ #!/usr/bin/env bash -set -eu -o pipefail +set -eux -o pipefail function tick () { if test -z "${tick+set}" @@ -190,6 +190,7 @@ git init --bare bad-url-rewriting (cd bad-url-rewriting git remote add origin https://github.com/foobar/gitoxide + nl -ba config >&2 cat <> config [remote "origin"] @@ -201,6 +202,7 @@ git init --bare bad-url-rewriting [url "https://github.com/byron/"] insteadOf = https://github.com/foobar/ EOF + nl -ba config >&2 { git remote get-url origin From 0b45bafe2026a2404183f50460d36ec4d40e8e14 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Sun, 16 Mar 2025 08:47:38 -0400 Subject: [PATCH 4/7] feat: Look for bash in `(git root)/bin`, then `(git root)/usr/bin` This changes `bash_program()` so that it will find the `bash.exe` provided by Git for Windows that is most reliable for our use in runinng test fixture scripts, of those that are available. First it uses the shim, but falls back to the non-shim if the shim is not available. If neither is found, then the fallback of using the simple command `bash.exe` (which triggers a path search when run) continues to be used. --- tests/tools/src/lib.rs | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/tests/tools/src/lib.rs b/tests/tools/src/lib.rs index de52385308e..5a0e299a8eb 100644 --- a/tests/tools/src/lib.rs +++ b/tests/tools/src/lib.rs @@ -653,21 +653,25 @@ fn configure_command<'a, I: IntoIterator, S: AsRef>( } fn bash_program() -> &'static Path { - // TODO(deps): *Maybe* add something to `gix-path` like `env::shell()` that can be used to - // find bash and, once the version `gix-testtools` depends on has it, use it. + // TODO(deps): Unify with `gix_path::env::shell()` by having both call a more general function + // in `gix-path`. See https://github.com/GitoxideLabs/gitoxide/issues/1886. static GIT_BASH: Lazy = Lazy::new(|| { if cfg!(windows) { GIT_CORE_DIR .ancestors() .nth(3) - .map(OsString::from) - .map(|mut raw_path| { - // Go down to where `bash.exe` usually is. Keep using `/` separators (not `\`). - raw_path.push("/usr/bin/bash.exe"); - raw_path + .map(OsStr::new) + .iter() + .flat_map(|prefix| { + // Go down to places `bash.exe` usually is. Keep using `/` separators, not `\`. + ["/bin/bash.exe", "/usr/bin/bash.exe"].into_iter().map(|suffix| { + let mut raw_path = (*prefix).to_owned(); + raw_path.push(suffix); + raw_path + }) }) .map(PathBuf::from) - .filter(|bash| bash.is_file()) + .find(|bash| bash.is_file()) .unwrap_or_else(|| "bash.exe".into()) } else { "bash".into() From 9061fc4260fe0d7b2c1ba345ae7923f2d3e37ad4 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Sun, 16 Mar 2025 08:51:42 -0400 Subject: [PATCH 5/7] Revert "Instrument make_remote_repos.sh to view `config` corruption" This reverts commit d290ad962fe88e2aa28d23d412117f59ee5664c0. --- gix/tests/fixtures/make_remote_repos.sh | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/gix/tests/fixtures/make_remote_repos.sh b/gix/tests/fixtures/make_remote_repos.sh index 1eaccc404cc..5411fc177c3 100755 --- a/gix/tests/fixtures/make_remote_repos.sh +++ b/gix/tests/fixtures/make_remote_repos.sh @@ -1,5 +1,5 @@ #!/usr/bin/env bash -set -eux -o pipefail +set -eu -o pipefail function tick () { if test -z "${tick+set}" @@ -190,7 +190,6 @@ git init --bare bad-url-rewriting (cd bad-url-rewriting git remote add origin https://github.com/foobar/gitoxide - nl -ba config >&2 cat <> config [remote "origin"] @@ -202,7 +201,6 @@ git init --bare bad-url-rewriting [url "https://github.com/byron/"] insteadOf = https://github.com/foobar/ EOF - nl -ba config >&2 { git remote get-url origin From 47234b66c5de20b6ea701c3f215d832c86d770c1 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Sun, 16 Mar 2025 08:53:57 -0400 Subject: [PATCH 6/7] feat: Document `gix_testtools::bash_program()` and make it public To make it easier for users of `gix-testtools` to diagnose problems or verify that the fallback for running fixutre scripts without usable shebangs (which is effectively how any fixture shell script is run on Windows), the formerly private `bash_program()` is now public. However, it is not recommend to rely on this specific value or on observed behavior of how it is computed. The details may change at any time. The purpose of `bash_program()` and how it is used internally by `gix-testtools` is also now documented explicitly. Broad details of how it searches or guesses what path to use are likewise documented, with a caveat that changes to them are not considered breaking. --- tests/tools/src/lib.rs | 30 +++++++++++++++++++++++++++++- 1 file changed, 29 insertions(+), 1 deletion(-) diff --git a/tests/tools/src/lib.rs b/tests/tools/src/lib.rs index 5a0e299a8eb..cbe2b42b948 100644 --- a/tests/tools/src/lib.rs +++ b/tests/tools/src/lib.rs @@ -652,7 +652,35 @@ fn configure_command<'a, I: IntoIterator, S: AsRef>( .env("GIT_CONFIG_VALUE_3", "always") } -fn bash_program() -> &'static Path { +/// Get the path attempted as a `bash` interpreter, for fixture scripts having no `#!` we can use. +/// +/// This is rarely called on Unix-like systems, provided that fixture scripts have usable shebang +/// (`#!`) lines and are marked executable. However, Windows does not recognize `#!` when executing +/// a file. If all fixture scripts that cannot be directly executed are `bash` scripts or can be +/// treated as such, fixture generation still works on Windows, as long as this function manages to +/// find or guess a suitable `bash` interpreter. +/// +/// ### Search order +/// +/// This function is used internally. It is public to facilitate diagnostic use. The following +/// details are subject to change without warning, and changes are treated as non-breaking. +/// +/// The `bash.exe` found in a path search is not always suitable on Windows. This is mainly because +/// `bash.exe` in `System32`, which is associated with WSL, would often be found first. But even +/// where that is not the case, the best `bash.exe` to use to run fixture scripts to set up Git +/// repositories for testing is usually one associated with Git for Windows, even if some other +/// `bash.exe` would be found in a path search. Currently, the search order we use is as follows: +/// +/// 1. The shim `bash.exe`, which sets environment variables when run and is, on some systems, +/// needed to find the POSIX utilities that scripts need (or correct versions of them). +/// +/// 2. The non-shim `bash.exe`, which is sometimes available even when the shim is not available. +/// This is mainly because the Git for Windows SDK does not come with a `bash.exe` shim. +/// +/// 3. As a fallback, the simple name `bash.exe`, which triggers a path search when run. +/// +/// On non-Windows systems, the simple name `bash` is used, which triggers a path search when run. +pub fn bash_program() -> &'static Path { // TODO(deps): Unify with `gix_path::env::shell()` by having both call a more general function // in `gix-path`. See https://github.com/GitoxideLabs/gitoxide/issues/1886. static GIT_BASH: Lazy = Lazy::new(|| { From 720a23f873508fc574c0f0e9b212dbd0dd76b8bb Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Sun, 16 Mar 2025 09:04:11 -0400 Subject: [PATCH 7/7] feat: Add `jtt bash-program` (`jtt bp`) to show `bash_program()` This adds a `bash-program` subcommand, abbreviated `bp`, to the `gix-testools` program (`jtt`) to check what the `bash_program()` library function gives. This is intended for diagnostic use and should probably not be used in scripting. Currently it shows the quoted debug repreesentation of the path. --- tests/tools/src/main.rs | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/tests/tools/src/main.rs b/tests/tools/src/main.rs index 4a79bdfda1f..66a485dd360 100644 --- a/tests/tools/src/main.rs +++ b/tests/tools/src/main.rs @@ -1,5 +1,14 @@ use std::{fs, io, io::prelude::*, path::PathBuf}; +fn bash_program() -> io::Result<()> { + use std::io::IsTerminal; + if !std::io::stdout().is_terminal() { + eprintln!("warning: `bash-program` subcommand not meant for scripting, format may change"); + } + println!("{:?}", gix_testtools::bash_program()); + Ok(()) +} + fn mess_in_the_middle(path: PathBuf) -> io::Result<()> { let mut file = fs::OpenOptions::new().read(false).write(true).open(path)?; file.seek(io::SeekFrom::Start(file.metadata()?.len() / 2))?; @@ -17,6 +26,7 @@ fn main() -> Result<(), Box> { let mut args = std::env::args().skip(1); let scmd = args.next().expect("sub command"); match &*scmd { + "bash-program" | "bp" => bash_program()?, "mess-in-the-middle" => mess_in_the_middle(PathBuf::from(args.next().expect("path to file to mess with")))?, #[cfg(unix)] "umask" => umask()?,