Denton Liu <liu.denton@xxxxxxxxx> writes: > Does this commit message increase the clarity? > > lib-submodule-update: pass OVERWRITING_FAIL > > We are using `test_must_fail $command`. However, $command is not > necessarily a git command; it could be a test helper function. > > In an effort to stop using test_must_fail with non-git commands, instead > of invoking `test_must_fail $command`, run > `OVERWRITING_FAIL=test_must_fail $command` instead in > test_submodule_switch_common(). > > In the case where $command is a git command, > test_submodule_switch_common() is called by one of > test_submodule_switch() or test_submodule_forced_switch(). In those two > functions, pass $command like this: > > test_submodule_switch_common "eval \$OVERWRITING_FAIL git $command" > > In the case where $command is a test helper function, increase the > granularity of the test helper function by marking the git invocation > which was meant to fail like this: > > $OVERWRITING_FAIL git checkout "$1" > > This is useful because currently, when we run a test helper function, we > just mark the whole thing as `test_must_fail`. However, it's possible > that the helper function might fail earlier or later than expected due > to an introduced bug. If this happens, then the test case will still > report as passing but it should really be marked as failing since it > didn't actually display the intended behaviour. > > While we're at it, some helper functions have git commands piping into > another git command. Break these pipes up into two separate invocations > with a file buffer so that the return code of the first command is not > lost. > > This patch can be better viewed with `--ignore-all-space`. It may be better, but not all that much. I think it comes from the design that this change is hard to understand. Anybody who wants to write more of these tests would need a much better guidance than "just use OVERWRITING_FAIL=test_must_fail where you would normally write test_must_fail and you'd be OK", as that is not what is going on. Before doing so, they would make sure that the place where they are tempted to write test_must_fail MUST be called by these three wrappers, or this guidance does not apply, for example. >> If we have given up the "single-shot environment export" for >> compatibility reasons (which is a sound decision to follow), we >> should make sure it is clear to our readers that we are not using >> that shell feature. I.e. >> >> export OVERWRITING_FAIL=test_must_fail && >> $command replace_sub1_with_directory && >> unset OVERWRITING_FAIL && > > Makes sense. I would drop the export, though, because $OVERWRITING_FAIL > should only be handled within the shell environment. We're never calling > any external commands that need to know about this variable. Yup, not pretending that this affects anywhere outside of shell by exporting is a bad idea. > On a tangent, I got a response[1] from the dash people about the > "single-shot environment export" propagating past a function. It seems > like in the most updated version of POSIX addresses this and it's up to > the implementers whether or not variables propagate past a function > invocation. Yes, it is the reason why we discourage the unportable use of the pattern in our tests.
