[PATCH] t5702: be more resilient to env quoting

[PATCH] t5702: be more resilient to env quoting

[Jeff King]

A few tests in t5702 check whether we specify the v2 protocol via the
GIT_PROTOCOL environment variable. They do this by redirecting the
output of "env" into a file and grepping it. But our exact pattern
depends on how env quotes its output. GNU coreutils 9.10 will output
this:

  GIT_PROTOCOL=version=2

but version 9.12 will output this:

  GIT_PROTOCOL='version=2'

It doesn't quote every variable, but the presence of "=" now triggers
it. We could loosen our regex to accommodate both versions, but there's
a simpler fix. We only care about the value of one variable, so we can
just print it using the shell invoked for the server-side command.

Note that this new formula is slightly less strict: it allows
GIT_PROTOCOL as an empty string in addition to it being missing. But
that's OK; they both mean the same thing to the child Git process.

Likewise, it's actually more strict when we do expect the v2 protocol:
it checks the contents of the whole GIT_PROTOCOL rather than a partial
grep. In practice it's the same for now (we only ever set protocol=2),
but I think in the long run being strict is better (if we do add
something else, the test will break and we can consider how to handle it
then).

Signed-off-by: Jeff King <peff@peff.net>
---
The new coreutils hit Debian unstable, so I ran into this doing my usual
daily "make test". I wondered what idiot did the overly-complicated
"env" invocation in the first place, but naturally it turns out it was
me.

 t/t5702-protocol-v2.sh | 23 +++++++++++++++--------
 1 file changed, 15 insertions(+), 8 deletions(-)

diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh
index 7e69ba7f84..a66adfbc3b 100755
--- a/t/t5702-protocol-v2.sh
+++ b/t/t5702-protocol-v2.sh
@@ -929,28 +929,35 @@ test_expect_success 'file:// --negotiate-only with protocol v0' '
 test_expect_success 'push with custom path does not request v2' '
 	rm -f env.trace &&
 	git -C client push \
-		--receive-pack="env >../env.trace; git-receive-pack" \
+		--receive-pack="
+			echo GIT_PROTOCOL=\$GIT_PROTOCOL >../env.trace
+			git-receive-pack" \
 		origin HEAD:refs/heads/custom-push-test &&
-	test_path_is_file env.trace &&
-	test_grep ! ^GIT_PROTOCOL env.trace
+	echo GIT_PROTOCOL= >expect &&
+	test_cmp expect env.trace
 '
 
 test_expect_success 'fetch with custom path does request v2' '
 	rm -f env.trace &&
 	git -C client fetch \
-		--upload-pack="env >../env.trace; git-upload-pack" \
+		--upload-pack="
+			echo GIT_PROTOCOL=\$GIT_PROTOCOL >../env.trace
+			git-upload-pack" \
 		origin HEAD &&
-	test_grep ^GIT_PROTOCOL=version=2 env.trace
+	echo GIT_PROTOCOL=version=2 >expect &&
+	test_cmp expect env.trace
 '
 
 test_expect_success 'archive with custom path does not request v2' '
 	rm -f env.trace &&
 	git -C client archive \
-		--exec="env >../env.trace; git-upload-archive" \
+		--exec="
+			echo GIT_PROTOCOL=\$GIT_PROTOCOL >../env.trace
+			git-upload-archive" \
 		--remote=origin \
 		HEAD >/dev/null &&
-	test_path_is_file env.trace &&
-	test_grep ! ^GIT_PROTOCOL env.trace
+	echo "GIT_PROTOCOL=" >expect &&
+	test_cmp expect env.trace
 '
 
 test_expect_success 'reject client packfile-uris if not advertised' '
-- 
2.56.0.613.g16c2199927

Re: [PATCH] t5702: be more resilient to env quoting

[Todd Zullinger]

Jeff King wrote:
> A few tests in t5702 check whether we specify the v2 protocol via the
> GIT_PROTOCOL environment variable. They do this by redirecting the
> output of "env" into a file and grepping it. But our exact pattern
> depends on how env quotes its output. GNU coreutils 9.10 will output
> this:
> 
>   GIT_PROTOCOL=version=2
> 
> but version 9.12 will output this:
> 
>   GIT_PROTOCOL='version=2'
> 
> It doesn't quote every variable, but the presence of "=" now triggers
> it. We could loosen our regex to accommodate both versions, but there's
> a simpler fix. We only care about the value of one variable, so we can
> just print it using the shell invoked for the server-side command.

Per the coreutils-9.12 NEWS¹, it now honors "the
QUOTING_STYLE environment variable, defaulting to
shell-escape style."

Not that I think we should keep the env parsing; as you note
there are some pros to replacing it anyway.  But if we want
to use env somewhere else, we can set QUOTING_STYLE=literal
to get the unquoted values too.

¹ https://github.com/coreutils/coreutils/blob/v9.12/NEWS#L118-L121

  I'd link to the main Savannah cgit or gitweb, but those
  are currently down, thanks to AI scraper bots. :(

> ---
> The new coreutils hit Debian unstable, so I ran into this doing my usual
> daily "make test". I wondered what idiot did the overly-complicated
> "env" invocation in the first place, but naturally it turns out it was
> me.

Haha.  I've had that experience as well.  More often than
I'd like. :)

I didn't properly review or test your changes, though they
do _look_ sane at a glance (albeit a quick Friday night
glance).

-- 
Todd

Re: [PATCH] t5702: be more resilient to env quoting

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> The new coreutils hit Debian unstable, so I ran into this doing my usual
> daily "make test". I wondered what idiot did the overly-complicated
> "env" invocation in the first place, but naturally it turns out it was
> me.

Namely, in eaa0fd6584 (git_connect(): fix corner cases in
downgrading v2 to v0, 2023-03-17).  The log message starts by
discussing dropping a "push" that asks for v2 to v0 because we can
only do v2 for fetches, but the three tests added by the patch
verify that fetch, push, and archive invoked via a custom path do
not request v2.

I can see that the patch correctly checks the value of '$GIT_PROTOCOL'
in the environment when these custom commands are invoked.  I would
have to read the log message for the "head-scratching notes" in
eaa0fd6584 if I wanted to remind myself why the original code wanted
to downgrade a custom upload-pack to v0 in the first place.  Luckily,
I do not have to do that to be convinced that this particular patch
is correct.