GitoxideLabs / gitoxide

An idiomatic, lean, fast & safe pure Rust implementation of Git
Apache License 2.0
8.91k stars 303 forks source link

Let `gix_testtools::Env` undo multiple changes to the same var #1560

Closed EliahKagan closed 1 month ago

EliahKagan commented 1 month ago

I believe gix_testtools::Env is meant to undo all changes made through it. But modifying the state of the same environment variable multiple times on the same instance (setting twice, unsetting twice, setting and then unsetting, or unsetting and then setting) breaks this.

This happens because, prior to the changes in this PR, an Env instance undoes the modifications it has made in first-in first-out order, i.e., in the same order the set and unset calls it is undoing were made. Each call appends to the end of the vector with push, and the iteration on drop is done by:

https://github.com/Byron/gitoxide/blob/ec0d03a154777964147aa4a064dd4e5ba38dd78c/tests/tools/src/lib.rs#L860

As a result, the state that was associated with a variable just before its most recent modification is restored, rather than the state associated with it before all modifications through the Env instance.

This PR fixes it by having it undo the modifications in last-in first-out order, i.e., in the opposite of the order the set and unset calls were made, by iterating through altered_vars in reverse. Besides newly introduced tests (which are the bulk of the diff) and some small documentation changes, this PR consists of changing the above iteration to:

https://github.com/Byron/gitoxide/blob/555164f2387f77348ab876d05a77c142a69cacfa/tests/tools/src/lib.rs#L860

altered_vars is thus now an undo stack, though I did not also add any new operations such as undoing the most recent change, because they don't seem to be needed. Furthermore, even though this change makes it easy to add new operations, it likely makes it unnecessary ever to do so, because of a nice property it introduces. The following now have the same effect:

let _env1 = Env::new().set(...).unset(...).set(...);
//      first sequence ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
let _env2 = Env::new().unset(...).set(...).set(...);
//     second sequence ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
let _env = Env::new().set(...).unset(...).set(...)
//     first sequence ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
                     .unset(...).set(...).set(...);
//    second sequence ^^^^^^^^^^^^^^^^^^^^^^^^^^^^

So separate Env instances declared one after the other can now be merged and split back up as needed or desired, without worrying about lasting breakage from operating on the same variable multiple times.

Other use cases include being able to create an Env in a helper without having to check for overlapping operations; being able to use Env where the variables being unset or unset are (partly) determined at runtime; and being able to use Env with variables that differ on Unix but are the same on Windows, because environment variable names in Windows, while case-aware, are case-insensitive.

But all that pales in comparison the main "use case": accidentally modifying the same variable multiple times. This should either have an intuitive effect or cause an error. That way, debugging is much easier than silent success with an unintuitive effect, especially since the unintuitive effect would allow tests cases to contaminate global state used by other test cases, even for tests that are made to run in series.

Because gix-testtools, even if used outside of gitoxide's own test suite, is meant for testing, I think it would be a workable alternative to panic when a variable is modified multiple times with the same Env instance. But this would itself be somewhat unexpected, and it is more complex to implement, especially if it is to be implemented properly to account for case equivalence on Windows.

I think the LIFO behavior introduced here already what any users of Env who have not looked at the code of Env would have expected, in any context, and that this expectation is strong enough that this change should be considered a bugfix, even if one does not consider this expectation to follow from the documentation itself. But I think it is a non-breaking change, because I doubt anyone has relied on the previous behavior (except possibly by accident, introducing a bug into their code).

This PR also includes: