EliahKagan
commented
1 month ago that (at least by now)
OsStringhas some ways of altering it
It occurs to me that I do not always check when features are introduced. I am hoping that if this feature came in later than the project MSRV, then one of the clippy checks on CI would catch it and fail. (If that is not the case, please let me know.)
Fixes #1574
When the
MSYSvariable is absent, we treat this the same as when it is present but empty. However, as described in #1574, anMSYSvariable that is present but whose value contains an unpaired surrogate would also be replaced entirely, rather than appending to its old value. This changes that, to instead append, retaining whatever was there even if it was ill-formed Unicode.An alternative change could be to panic when the old value is ill-formed Unicode. The change made here allows and appends to the old value, rather than panicking or keeping and documenting the previous behavior of discarding the old value, because the appended sequence
winsymlinks:nativestrictis effective at causing fixture scripts to attempt to create actual symlinks even if the preceding code point is an unpaired Unicode high surrogate.To avoid confusion related to #1358, I tested without setting
GIX_TEST_IGNORE_ARCHIVES, relying on how some fixtures must still run because archive generation is suppressed for them in.gitignorefiles. That failure would occur in connection with theMSYSvariable even when run this way was observed in #1443 and #1444, but I have also verified that.This PR adds and then removes tests, which were unsuitable for the reasons noted in #1578, and which I removed rather than attempting to fix due to the concerns in #1577 combined with there being little doubt, in this case, that the code using
OsString::pushworks as it seems. If those abandoned tests are not wanted in the history, I have no objection to rebasing to remove those commits, which I would be pleased to do on request.This gist contains transcripts of the manual verification done to decide on and check over this approach. The scripts run in a normal environment, as well as in an environment in which the old value of
MSYShas a trailing unpaired Unicode high surrogate. Failures occur due to the absence ofMSYSwhen the code that sets it is removed, which verifies both that this code does not break setting it, and that the appended space character andwinsymlinksoption takes effect properly even if it immediately follows an unpaired high surrogate that "tries" to combine with the next code point. Testing with apanic!temporarily added, by the same technique as #1574, confirms that we really are concatenating.All full test runs have an unrelated failure of
jj_realistic_needs_to_be_more_cleverlocally, which is due to #1575 and can therefore be disregarded in the transcripts of local test runs here. Between tests, I removed generated fixture script output by using the technique discussed in #1435 of runninggix clean -xd -m '*generated*' -e, which is shown in the gist transcripts. I did it that way rather than by running the stronger commandgix clean -xdeso the unpaired surrogate in theMSYSenvironment variable would not stall the build due to https://github.com/rust-lang/libz-sys/issues/215. The command used is strong enough, but to doubly ensure insufficient cleaning would not cause misinterpreted results, I ran the tests with the expected failure modification last of all the full runs, so that if left-over fixture output was going to cause tests to wrongly pass, then it would happen with the tests I was verifying should fail.