Re: [PATCH 1/3] t5730: introduce fetch command helper

Thanks for chiming in!

On Mon Aug 9, 2021 at 9:40 PM CEST, Jonathan Nieder wrote:

Hm, for comparison let me see what this looks like without the helper:
after some prior step

 object_format=$(test_oid algo) && # probably just once in a setup step
 x=$(git rev-parse x) &&

we can write

 cat <<-EOF &&
 command=fetch
 object-format=$object_format
 0001
 no-progress
 want-ref refs/heads/main
 have $x
 done
 0000
 EOF

I find that a little _easier_ to read than a write_fetch_command call,
because I don't have to chase the definition and x is labeled as a
'have'.

Is there some additional motivation for this helper?

It was suggested in earlier review rounds. I think it does improve readability
as quite some lines need to be repeated everywhere a fetch command is assembled.
I agree though that not having some sort of "named arguments" is a bit
detrimental.

quoted

 	test-tool serve-v2 --stateless-rpc >out <in &&

@@ -121,16 +167,14 @@ test_expect_success 'multiple want-ref lines' '
 	EOF
 	git rev-parse c d >expected_commits &&

-	oid=$(git rev-parse b) &&
 	test-tool pkt-line pack >in <<-EOF &&
-	$(write_command fetch)
-	0001
-	no-progress
-	want-ref refs/heads/o/foo
-	want-ref refs/heads/o/bar
-	have $oid
-	done
-	0000
+	$(write_fetch_command \
+		refs/heads/o/foo \
+		refs/heads/o/bar \
+		-- \
+		-- \
+		$(git rev-parse b) \
+	)
 	EOF

Here the entirety of the input to "test-tool pkt-line pack" is the
entirety of the output from write_fetch_command, which would suggest
either

a. making write_fetch_command pipe its output to "test-tool pkt-line
pack", or

b. using a pipe instead of a command substitution, like
"write_fetch_command ... | test-tool pkt-line pack >in"

(although as mentioned above, I think it's simpler to inline the
write_fetch_command and even the write_command as well).

Yes, although I believe a pipe cannot be used as we don't have bash's `set -o
pipefail` (ie. the exit status will always be the status of the last command in
the pipe, even if an earlier one failed).

Perhaps an alternative would be:

	write_fetch_command () {
		write_command fetch &&
		echo "0001" &&
		echo "no-progress" &&
		cat /dev/stdin &&
		echo "done" &&
		echo "0000"
 	}


Which would then be called like so:

	write_fetch_command >pkt_cmd <<-EOF &&
	want-ref refs/heads/main
	have $(git rev-parse a)
	EOF
	test-tool pkt-line pack <pkt_cmd >in &&
	test-tool serve-v2 --stateless-rpc >out <in &&


I'm not sure how portable that is, though. Maybe using `while read -r` instead
of `cat /dev/stdin`?