Re: [PATCH v4 18/21] t0061: fix with --with-dashes and RUNTIME_PREFIX

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 28, 2019, 02:09 UTC
Message-ID: <xmqq36pdcz3o.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <d613c79aff9c65ab9c3723a876c99e6f35a2f133.1548254412.git.gitgitgadget@gmail.com>

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 20 quoted lines

> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> When building Git with RUNTIME_PREFIX and starting a test helper from
> t/helper/, it fails to detect the system prefix correctly.
>
> This is the reason that the warning
>
> 	RUNTIME_PREFIX requested, but prefix computation failed. [...]
>
> to be printed.
>
> In t0061, we did not expect that to happen, and it actually did not
> happen in the normal case, because bin-wrappers/test-tool specifically
> sets GIT_TEXTDOMAINDIR (and as a consequence, nothing in test-tool wants
> to know about the runtime prefix).
>
> However, with --with-dashes, bin-wrappers/test-tool is no longer called,
> but t/helper/test-tool is called directly.
>
> So let's just ignore the RUNTIME_PREFIX warning.

Two questions that would come to the readers' minds are

 - Why "it fails to detect the system prefix correctly"?  Is that a
   bug waiting to hurt end users?

 - Why is it better not to bother fixing that failure?  Is it
   because this happens only in the test helper and won't hurt end
   users?

I do not mind this particular "sweeping it under the rug" if the rationale is "it only is the strange set-up of test-tool that causes it, and we shouldn't burden the code shared with the actual runtime to compute runtime prefix just to fix this bug". I do think it is not productive to bend backwards if it is only done in order to work around unusual setup in the test helper binary and I do agree that ignoring the warning is the right solution.

I just do not like to see that our commits do not explain why we chose to ignore, instead of "fix", the issue.

Show 19 quoted lines

> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  t/t0061-run-command.sh | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/t/t0061-run-command.sh b/t/t0061-run-command.sh
> index 99a614bc7c..5a2d087bf0 100755
> --- a/t/t0061-run-command.sh
> +++ b/t/t0061-run-command.sh
> @@ -166,7 +166,8 @@ test_trace () {
>  	expect="$1"
>  	shift
>  	GIT_TRACE=1 test-tool run-command "$@" run-command true 2>&1 >/dev/null | \
> -		sed -e 's/.* run_command: //' -e '/trace: .*/d' >actual &&
> +		sed -e 's/.* run_command: //' -e '/trace: .*/d' \
> +			-e '/RUNTIME_PREFIX requested/d' >actual &&
>  	echo "$expect true" >expect &&
>  	test_cmp expect actual
>  }