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