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

From: Junio C Hamano <gitster@pobox.com>
Date: May 1, 2020, 16:51 UTC
Message-ID: <xmqq8siboayy.fsf@gitster.c.googlers.com>
In-Reply-To: <20200501093554.GA49619@generichostname>

Denton Liu <liu.denton@gmail.com> writes:

Show 38 quoted lines

> 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.

Show 12 quoted lines

>> 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.

Show 5 quoted lines

> 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.