Re: [PATCH 1/2] t5616: refactor packfile replacement

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi Jonathan,

On Tue, 14 May 2019, Jonathan Tan wrote:

> diff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh
> index 9a8f9886b3..7cc0c71556 100755
> --- a/t/t5616-partial-clone.sh
> +++ b/t/t5616-partial-clone.sh
> @@ -244,11 +244,25 @@ test_expect_success 'fetch what is specified on CLI even if already promised' '
>  . "$TEST_DIRECTORY"/lib-httpd.sh
>  start_httpd
>
> -# Converts bytes into a form suitable for inclusion in a sed command. For
> -# example, "printf 'ab\r\n' | hex_unpack" results in '\x61\x62\x0d\x0a'.
> -sed_escape () {
> -	perl -e '$/ = undef; $input = <>; print unpack("H2" x length($input), $input)' |
> -		sed 's/\(..\)/\\x\1/g'
> +# Converts bytes into their hexadecimal representation. For example,
> +# "printf 'ab\r\n' | hex_unpack" results in '61620d0a'.
> +hex_unpack () {
> +	perl -e '$/ = undef; $input = <>; print unpack("H2" x length($input), $input)'
> +}
> +
> +# Inserts $1 at the start of the string and every 2 characters thereafter.
> +intersperse () {
> +	sed 's/\(..\)/'$1'\1/g'
> +}
> +
> +# Create a one-time-sed command to replace the existing packfile with $1.
> +replace_packfile () {
> +	# The protocol requires that the packfile be sent in sideband 1, hence
> +	# the extra \x01 byte at the beginning.
> +	printf "1,/packfile/!c %04x\\\\x01%s0000" \
> +		"$(($(wc -c <$1) + 5))" \
> +		"$(hex_unpack <$1 | intersperse '\\x')" \
> +		>"$HTTPD_ROOT_PATH/one-time-sed"
>  }

Urgh. This is not a problem *this* patch introduces, but why on Earth do
we have to do complicated computations in shell code using an unholy mix
of complex sed and Perl invocations, making things fragile and slow? We do
have such a nice facility is the t/test-tool helper...

The refactoring itself looks correct to me, of course.

Thanks,
Dscho

>
>  test_expect_success 'upon cloning, check that all refs point to objects' '
> @@ -270,10 +284,7 @@ test_expect_success 'upon cloning, check that all refs point to objects' '
>  	# Replace the existing packfile with the crafted one. The protocol
>  	# requires that the packfile be sent in sideband 1, hence the extra
>  	# \x01 byte at the beginning.
> -	printf "1,/packfile/!c %04x\\\\x01%s0000" \
> -		"$(($(wc -c <incomplete.pack) + 5))" \
> -		"$(sed_escape <incomplete.pack)" \
> -		>"$HTTPD_ROOT_PATH/one-time-sed" &&
> +	replace_packfile incomplete.pack &&
>
>  	# Use protocol v2 because the sed command looks for the "packfile"
>  	# section header.
> @@ -313,10 +324,7 @@ test_expect_success 'when partial cloning, tolerate server not sending target of
>  	# Replace the existing packfile with the crafted one. The protocol
>  	# requires that the packfile be sent in sideband 1, hence the extra
>  	# \x01 byte at the beginning.
> -	printf "1,/packfile/!c %04x\\\\x01%s0000" \
> -		"$(($(wc -c <incomplete.pack) + 5))" \
> -		"$(sed_escape <incomplete.pack)" \
> -		>"$HTTPD_ROOT_PATH/one-time-sed" &&
> +	replace_packfile incomplete.pack &&
>
>  	# Use protocol v2 because the sed command looks for the "packfile"
>  	# section header.
> --
> 2.21.0.1020.gf2820cf01a-goog
>
>