Re: [PATCH v1.1] lib-submodule-update: pass OVERWRITING_FAIL

Denton Liu [off-list ref] 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.

quoted

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.