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

From: Kim Altintop <kim@eagain.st>
Date: Aug 9, 2021, 21:56 UTC
Message-ID: <CDFBG4S23B1M.141X9FIPP7YHS@schmidt>
In-Reply-To: <YRGEwFauT5QjMSNO@google.com>

Thanks for chiming in!

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

Show 24 quoted lines

> 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.

Show 37 quoted lines

>
> >  	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`?