Let gix_testtools::Env undo multiple changes to the same var
#1560
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.
I believe
gix_testtools::Envis 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
Envinstance undoes the modifications it has made in first-in first-out order, i.e., in the same order thesetandunsetcalls it is undoing were made. Each call appends to the end of the vector withpush, 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
Envinstance.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
setandunsetcalls were made, by iterating throughaltered_varsin 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_varsis 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:So separate
Envinstances 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
Envin a helper without having to check for overlapping operations; being able to useEnvwhere the variables being unset or unset are (partly) determined at runtime; and being able to useEnvwith 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 sameEnvinstance. 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
Envwho have not looked at the code ofEnvwould 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:
gix_testtools::Env. One of these passed before and after the changes made here. The others failed before the changes and passed afterwards. As mentioned above, the actual bugfix is very simple, consisting just of iterating through the vector in the other direction; most of the code in this PR is the tests.unsetmethod was added. Theunsetmethod's docstring is corrected (it had previously been the same asset), and theEnvstruct docstring is updated to better account for how bothsetandunsetare supported.