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

[Jonathan Nieder <jrnieder@gmail.com>]

Hi,

Kim Altintop wrote:

> Assembling a "raw" fetch command to be fed directly to "test-tool serve-v2"
> is extracted into a test helper.
>
> Suggested-by: Junio C Hamano <gitster@pobox.com>
> Signed-off-by: Kim Altintop <kim@eagain.st>
> ---
>  t/t5703-upload-pack-ref-in-want.sh | 107 ++++++++++++++++++++---------
>  1 file changed, 74 insertions(+), 33 deletions(-)

Thanks!  Interesting.

> diff --git a/t/t5703-upload-pack-ref-in-want.sh b/t/t5703-upload-pack-ref-in-want.sh
> index e9e471621d..cd4744b016 100755
> --- a/t/t5703-upload-pack-ref-in-want.sh
> +++ b/t/t5703-upload-pack-ref-in-want.sh
> @@ -40,6 +40,54 @@ write_command () {
>  	fi
>  }
> 
> +# Write a complete fetch command to stdout, suitable for use with `test-tool
> +# pkt-line`. "want-ref", "want", and "have" values can be given in this order,
> +# with sections separated by "--".
> +#
> +# Examples:
> +#
> +# write_fetch_command refs/heads/main
> +#
> +# write_fetch_command \
> +#	refs/heads/main \
> +#	-- \
> +#	-- \
> +#	$(git rev-parse x)
> +#
> +# write_fetch_command \
> +#	--
> +#	$(git rev-parse a) \
> +#	--
> +#	$(git rev-parse b)
> +write_fetch_command () {

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?

> +	write_command fetch &&
> +	echo "0001" &&
> +	echo "no-progress" || return
> +    while :

Whitespace seems off.  Junio covers this in his review so I'll ignore
other instances.

[...]
> @@ -97,15 +145,13 @@ test_expect_success 'basic want-ref' '
>  	EOF
>  	git rev-parse f >expected_commits &&
> 
> -	oid=$(git rev-parse a) &&
>  	test-tool pkt-line pack >in <<-EOF &&
> -	$(write_command fetch)
> -	0001
> -	no-progress
> -	want-ref refs/heads/main
> -	have $oid
> -	done
> -	0000
> +	$(write_fetch_command \
> +		refs/heads/main \
> +		-- \
> +		-- \
> +		$(git rev-parse a) \
> +	)
>  	EOF
> 
>  	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).

Thanks and hope that helps,
Jonathan