In gix-testtools, when test fixture scripts are run, the configure_command function customizes their environment. Since 0899c2e (#1444), this includes setting the MSYS variable to make it so that commands in shell scripts such as ln -s will really attempt to create symlinks rather than copies.
If MSYS was already set, the options held in its old value are kept, with the new option appended at the end so it will only suppress an option that conflicts with it, otherwise leaving the effects of preceding options in place.
But this does not happen if the old value of MSYS contained an unpaired surrogate code point. This is because env::var rather than env::var_os is used, so the unwrap_or_default method returns an empty string both when MSYS was absent and when it was present with a value that could not be represented as well-formed UTF-8.
There are at least three scenarios where MSYS may contain an unpaired surrogate:
By accident.
The path to one's debugger that one specifies with error_start: contains one. This seems unlikely, except as a special case of...
Intentionally to test the behavior of some component when MSYS contains an unpaired surrogate.
The impact of this bug is therefore small. However, fixing it would help clarify the intended semantics of how gix-testtools treats the MSYS environment variable when running test fixture scripts. (This may potentially be relevant to some changes made for #1578.)
Expected behavior 🤔
Due to the nature of gix-testtools as a testing facility, both as part of gitoxide's test suite and when the published crate is used, I think any of these behaviors are fine and should be considered to fix the bug:
Always panic when the previous value of MSYS contained an unpaired surrogate, whether or not this is documented.
Always accept and append to the previous value of MSYS when it contains an unpaired surrogate, such as by using env::var_os and calling push (rather than env::var and calling push_str), whether or not this is documented.
If this is done, then unwrap_or_default could still be used, because it would only be falling back to the default empty value when the environment variable was absent.
- let mut msys_for_git_bash_on_windows = std::env::var("MSYS").unwrap_or_default();
- msys_for_git_bash_on_windows.push_str(" winsymlinks:nativestrict");
+ let mut msys_for_git_bash_on_windows = std::env::var_os("MSYS").unwrap_or_default();
+ msys_for_git_bash_on_windows.push(" winsymlinks:nativestrict");
Most other behaviors, including the current behavior, if it is documented.
While any of those would, in my opinion, satisfy users' expectations and eliminate the bug as such, I think the best behavior would be to do (1) or (2). Specifically:
If appending a space followed by nativesymlinks to a preexisting value of MSYS that ends in a high surrogate is effective at causing symlinks to be created, then (2) is preferred.
If that does not work, then (1) is preferred.
I plan to investigate this and open a PR that makes one of those changes. (This investigation has been slightly slowed by the problem described in https://github.com/rust-lang/libz-sys/issues/215, but I should be able to complete it.)
Update: I have opened #1580, which changes it to use OsString instead of String and to append with OsString::push. As noted there, I verified that a value of MSYS produced this way does still have the desired effect on symlink creation.
Git behavior
Not directly applicable.
Steps to reproduce 🕹
The nice way to reproduce this would be to make a fixture that displays the value, or to add a tracing statement, or to use a debugger.
Instead, I just added a panic! to print out whatever MSYS value will be used:
Then I ran a test that, while conceptually unrelated to this issue, exercises that code, ensuring the panic! is hit. The following output was warnings induced by making that change, about unused variables and unreachable code, removed. The full output can be seen in this gist.
First, a control, with well-formed Unicode:
ek@Glub MINGW64 ~/source/repos/gitoxide/tests/tools (main)
$ MSYS='error_start:nonexistent' cargo nextest run configure_command_clears_external_config
Finished `test` profile [unoptimized + debuginfo] target(s) in 0.39s
------------
Nextest run ID e36b5636-bba9-47d5-b9d5-0ae20e5c0f30 with nextest profile: default
Starting 1 test across 3 binaries (6 tests skipped)
FAIL [ 0.015s] gix-testtools tests::configure_command_clears_external_config
--- STDOUT: gix-testtools tests::configure_command_clears_external_config ---
running 1 test
test tests::configure_command_clears_external_config ... FAILED
failures:
failures:
tests::configure_command_clears_external_config
test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 2 filtered out; finished in 0.01s
--- STDERR: gix-testtools tests::configure_command_clears_external_config ---
thread 'tests::configure_command_clears_external_config' panicked at tests\tools\src\lib.rs:598:5:
DEBUG: "error_start:nonexistent winsymlinks:nativestrict"
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
Canceling due to test failure
------------
Summary [ 0.017s] 1 test run: 0 passed, 1 failed, 6 skipped
FAIL [ 0.015s] gix-testtools tests::configure_command_clears_external_config
error: test run failed
A now, when the existing value contains an unpaired high surrogate:
ek@Glub MINGW64 ~/source/repos/gitoxide/tests/tools (main)
$ MSYS=$'error_start:\uD800z' cargo nextest run configure_command_clears_external_config
Finished `test` profile [unoptimized + debuginfo] target(s) in 0.39s
------------
Nextest run ID bc896e5b-fabd-45b5-8823-4889319cf522 with nextest profile: default
Starting 1 test across 3 binaries (6 tests skipped)
FAIL [ 0.024s] gix-testtools tests::configure_command_clears_external_config
--- STDOUT: gix-testtools tests::configure_command_clears_external_config ---
running 1 test
test tests::configure_command_clears_external_config ... FAILED
failures:
failures:
tests::configure_command_clears_external_config
test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 2 filtered out; finished in 0.02s
--- STDERR: gix-testtools tests::configure_command_clears_external_config ---
thread 'tests::configure_command_clears_external_config' panicked at tests\tools\src\lib.rs:598:5:
DEBUG: " winsymlinks:nativestrict"
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
Canceling due to test failure
------------
Summary [ 0.026s] 1 test run: 0 passed, 1 failed, 6 skipped
FAIL [ 0.024s] gix-testtools tests::configure_command_clears_external_config
error: test run failed
Therefore, normally it is able to keep content from the old value:
But when it cannot be converted to UTF-8, its silently discards it:
DEBUG: " winsymlinks:nativestrict"
(The reason I added the z after the unpaired surrogate was to show a situation where it is not at the very end, which is the more common situation. The problem is not limited to when it is at the very end; rather, whether appending ought to be done, rather than panicking, should, I think, be determined by whether program built against the MSYS DLL are able to heed the appended winsymlinks option even when it the space that delimits it follows an unpaired high surrogate.)
Current behavior 😯
In
gix-testtools, when test fixture scripts are run, theconfigure_commandfunction customizes their environment. Since 0899c2e (#1444), this includes setting theMSYSvariable to make it so that commands in shell scripts such asln -swill really attempt to create symlinks rather than copies.If
MSYSwas already set, the options held in its old value are kept, with the new option appended at the end so it will only suppress an option that conflicts with it, otherwise leaving the effects of preceding options in place.But this does not happen if the old value of
MSYScontained an unpaired surrogate code point. This is becauseenv::varrather thanenv::var_osis used, so theunwrap_or_defaultmethod returns an empty string both whenMSYSwas absent and when it was present with a value that could not be represented as well-formed UTF-8.https://github.com/Byron/gitoxide/blob/9bddeeb948397df3d1e391ec48597cc34d10acd0/tests/tools/src/lib.rs#L596-L597
https://github.com/Byron/gitoxide/blob/9bddeeb948397df3d1e391ec48597cc34d10acd0/tests/tools/src/lib.rs#L605
There are at least three scenarios where
MSYSmay contain an unpaired surrogate:error_start:contains one. This seems unlikely, except as a special case of...MSYScontains an unpaired surrogate.The impact of this bug is therefore small. However, fixing it would help clarify the intended semantics of how
gix-testtoolstreats theMSYSenvironment variable when running test fixture scripts. (This may potentially be relevant to some changes made for #1578.)Expected behavior 🤔
Due to the nature of
gix-testtoolsas a testing facility, both as part ofgitoxide's test suite and when the published crate is used, I think any of these behaviors are fine and should be considered to fix the bug:Always panic when the previous value of
MSYScontained an unpaired surrogate, whether or not this is documented.Always accept and append to the previous value of
MSYSwhen it contains an unpaired surrogate, such as by usingenv::var_osand callingpush(rather thanenv::varand callingpush_str), whether or not this is documented.If this is done, then
unwrap_or_defaultcould still be used, because it would only be falling back to the default empty value when the environment variable was absent.Most other behaviors, including the current behavior, if it is documented.
While any of those would, in my opinion, satisfy users' expectations and eliminate the bug as such, I think the best behavior would be to do (1) or (2). Specifically:
nativesymlinksto a preexisting value ofMSYSthat ends in a high surrogate is effective at causing symlinks to be created, then (2) is preferred.I plan to investigate this and open a PR that makes one of those changes. (This investigation has been slightly slowed by the problem described in https://github.com/rust-lang/libz-sys/issues/215, but I should be able to complete it.)
Update: I have opened #1580, which changes it to use
OsStringinstead ofStringand to append withOsString::push. As noted there, I verified that a value ofMSYSproduced this way does still have the desired effect on symlink creation.Git behavior
Not directly applicable.
Steps to reproduce 🕹
The nice way to reproduce this would be to make a fixture that displays the value, or to add a tracing statement, or to use a debugger.
Instead, I just added a
panic!to print out whateverMSYSvalue will be used:Then I ran a test that, while conceptually unrelated to this issue, exercises that code, ensuring the
panic!is hit. The following output was warnings induced by making that change, about unused variables and unreachable code, removed. The full output can be seen in this gist.First, a control, with well-formed Unicode:
A now, when the existing value contains an unpaired high surrogate:
Therefore, normally it is able to keep content from the old value:
But when it cannot be converted to UTF-8, its silently discards it:
(The reason I added the
zafter the unpaired surrogate was to show a situation where it is not at the very end, which is the more common situation. The problem is not limited to when it is at the very end; rather, whether appending ought to be done, rather than panicking, should, I think, be determined by whether program built against the MSYS DLL are able to heed the appendedwinsymlinksoption even when it the space that delimits it follows an unpaired high surrogate.)