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

[Jeff King <peff@peff.net>]

On Thu, Jan 12, 2023 at 05:03:21PM +0100, Ævar Arnfjörð Bjarmason wrote:

> > Arguably "git env--helper" should be "test-tool", which wouldn't run
> > into this problem, but it's probably not worth refactoring it for the
> > sake of these tests.
> 
> I think it's worth doing that, i.e. to take this alternate
> approach. When I removed the GIT_TEST_GETTEXT_POISON facility I didn't
> want to make that series any larger, and keeping it a built-in seemed
> harmless, as there wasn't any practical difference between an
> undocumented built-in and a test-tool.
> 
> But as your patch shows there is, I think we should just pay down that
> technical debt, rather than adding to it by accumulating workarounds
> (however small those are...).

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.

>  .gitignore                                    |  1 -
>  Makefile                                      |  2 +-
>  git.c                                         |  1 -
>  .../helper/test-env-helper.c                  | 24 +++----
>  t/helper/test-tool.c                          |  1 +
>  t/helper/test-tool.h                          |  1 +
>  t/t0017-env-helper.sh                         | 62 +++++++++----------
>  t/test-lib-functions.sh                       |  2 +-
>  t/test-lib.sh                                 |  6 +-
>  9 files changed, 50 insertions(+), 50 deletions(-)
>  rename builtin/env--helper.c => t/helper/test-env-helper.c (71%)

The patch itself looks obviously correct to me.

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

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

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