Closed EliahKagan closed 2 months ago
Thanks a lot!
(I merged in a rush, if I missed anything in the PR description please let me know)
(I merged in a rush, if I missed anything in the PR description please let me know)
You probably did not miss anything. But if you have thoughts on this then I am interested and I'd be pleased to make a follow-up PR if called for:
An alternative fix could be to pass false as the first argument to
apply_environment
, so thatGIT_ASKPASS
is not considered. Which of these approaches is better may depend on exactly what behavior we are trying to test.
(But if you saw that and didn't have a response at this time, that is also definitely fine.)
Two of the askpass tests assume
GIT_ASKPASS
is not set. Before this change, they wrongly fail when it is set, even if the code under test is working correctly.ssh_askpass_is_used_as_fallback
ssh_askpass_does_not_override_current_value
As other tests that deliberately involve
GIT_ASKPASS
(and therefore do not need to be modified) attest, when it is set:SSH_ASKPASS
is not used as a fallback (GIT_ASKPASS
deliberately takes precedence).Although
SSH_ASKPASS
still does not override the current value,GIT_ASKPASS
does override it, so checking for equality to the current value fails.Since those test cases are among those that already have the
serial
attribute and already temporarily modify the environment, this extends that modification by having it also temporarily unsetGIT_ASKPASS
if it was set.An alternative fix could be to pass
false
as the first argument toapply_environment
, so thatGIT_ASKPASS
is not considered. Which of these approaches is better may depend on exactly what behavior we are trying to test. (Maybe even two more test cases should be added, to test it both ways.)Situations where
GIT_ASKPASS
is set include dev containers. I discovered this bug while running tests in the dev container environment added in #1502, where all tests passed except the two mentioned above:All other tests passed already, and all tests pass with this change.