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

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 9, 2021, 19:16 UTC
Message-ID: <xmqq8s1afmyh.fsf@gitster.g>
In-Reply-To: <20210809175530.75326-2-kim@eagain.st>

Kim Altintop <kim@eagain.st> writes:

Show 36 quoted lines

> 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(-)
>
> 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)

Have a blank line here (or a line with "#" and nothing else) and it would become easier to read.

> +write_fetch_command () {
> +	write_command fetch &&
> +	echo "0001" &&
> +	echo "no-progress" || return

The "while :" in this helper function are indented with 4 spaces, not a single tab.

Show 24 quoted lines

> +    while :
> +	do
> +		case $# in 0) break ;; esac &&
> +		case "$1" in --) shift; break ;; esac &&
> +		echo "want-ref $1" &&
> +		shift || return
> +	done &&
> +    while :
> +	do
> +		case $# in 0) break ;; esac &&
> +		case "$1" in --) shift; break ;; esac &&
> +		echo "want $1" &&
> +		shift || return
> +	done &&
> +    while :
> +	do
> +		case $# in 0) break ;; esac &&
> +		case "$1" in --) shift; break ;; esac &&
> +		echo "have $1" &&
> +		shift || return
> +	done &&
> +	echo "done" &&
> +	echo "0000"
> +}

The error checking of the helper function seems to be reasonable, but ...

Show 16 quoted lines

>  # c(o/foo) d(o/bar)
>  #        \ /
>  #         b   e(baz)  f(main)
> @@ -97,15 +145,13 @@ test_expect_success 'basic want-ref' '
>  	EOF
>  	git rev-parse f >expected_commits &&
>
>  	test-tool pkt-line pack >in <<-EOF &&
> +	$(write_fetch_command \
> +		refs/heads/main \
> +		-- \
> +		-- \
> +		$(git rev-parse a) \
> +	)
>  	EOF
>  	test-tool serve-v2 --stateless-rpc >out <in &&

... the code that uses the helper needs rewriting to make use of it. A failure in $(write_fetch_command) here will not cause the caller to stop here. If we had any "git" command used in the helper, I would recommand restructuring the caller to do something like:

    write_fetch_command >pkt-src \
	refs/heads/main \
	-- \
	-- \
	$(git rev-parse a) &&
    test-tool pkt-line pack <pkt-src >in &&
    test-tool serve-v2 --stateless-rpc >out <in &&

but it probably may not be necessary (we only use "echo" there, and it probably is not worth worrying about 'echo' failing).

The call to $(git rev-parse a) might fail when rev-parse gets broken, and I think the rewritten version would catch such a breakage while the one inside $(write_fetch_command) in the here-doc would not be.