Re: [PATCH] env-helper: move this built-in to to "test-tool env-helper"

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 13, 2023, 20:17 UTC
Message-ID: <xmqqpmbi195g.fsf@gitster.g>
In-Reply-To: <Y8A3yGeJl0TCDNqe@coredump.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 8 quoted lines

> Yeah, I am totally fine with this direction. My reservations were:
>
>   1. It was work somebody had to do. But now you've done it.
>
>   2. It's _possible_ that some script somewhere is depending on it. But
>      I think the "--" in the name plus the lack of documentation means
>      that it's unlikely, and that we're morally absolved if somebody's
>      script does break.

That's how we burned some folks who depend on submodule--helper, isn't it? ;-)

> The patch itself looks obviously correct to me.

Show 8 quoted lines

>> -	test_must_fail git env--helper --type=bool --default=false --exit-code MISSING >actual.out 2>actual.err &&
>> +	test_must_fail test-tool env-helper --type=bool --default=false --exit-code MISSING >actual.out 2>actual.err &&
>
> Long lines like these made me wonder if it should simply be "test-tool
> env", which is shorter. We do not need "helper" to avoid polluting the
> main git-command namespace, and everything in test-tool is a helper
> anyway. But it probably doesn't matter much either way. It's not like
> that line wasn't already overly long. :)

I agree that the "-helper" looks a bit irritating, not because the line is long, but because test-tool is by definition about helpers used in tests, so the suffix is redundant.

Let's have a small update before queuing it.

> If we do take this, then my t/interop patch can be dropped, though we
> might want to salvage the error message bit:

Yup, that does make sense. Will queue what is below the scissors line.

Thanks, both.

Show 33 quoted lines

>
> -- >8 --
> Subject: [PATCH] t/interop: report which vanilla git command failed
>
> The interop test library sets up wrappers "git.a" and "git.b" to
> represent the two versions to be tested. It also wraps vanilla "git" to
> report an error, with the goal of catching tests which accidentally fail
> to use one of the version-specific wrappers (which could invalidate the
> tests in a very subtle way).
>
> But when it catches an invocation of vanilla git, it doesn't give any
> details, which makes it very hard to debug exactly which invocation is
> responsible (especially if it's buried in a function invocation, etc).
> Let's report the arguments passed to git, which helps narrow it down.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  t/interop/interop-lib.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/t/interop/interop-lib.sh b/t/interop/interop-lib.sh
> index 3e0a2911d4..62f4481b6e 100644
> --- a/t/interop/interop-lib.sh
> +++ b/t/interop/interop-lib.sh
> @@ -68,7 +68,7 @@ generate_wrappers () {
>  	wrap_git .bin/git.a "$DIR_A" &&
>  	wrap_git .bin/git.b "$DIR_B" &&
>  	write_script .bin/git <<-\EOF &&
> -	echo >&2 fatal: test tried to run generic git
> +	echo >&2 fatal: test tried to run generic git: $*
>  	exit 1
>  	EOF
>  	PATH=$(pwd)/.bin:$PATH