diff mbox series

[1/3] t5730: introduce fetch command helper

Message ID 20210809175530.75326-2-kim@eagain.st (mailing list archive)
State New
Headers show
Series upload-pack: treat want-ref relative to namespace | expand

Commit Message

Kim Altintop Aug. 9, 2021, 5:56 p.m. UTC
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(-)

--
2.32.0

Comments

Junio C Hamano Aug. 9, 2021, 7:16 p.m. UTC | #1
Kim Altintop <kim@eagain.st> writes:

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

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

>  # 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.
Jonathan Nieder Aug. 9, 2021, 7:40 p.m. UTC | #2
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
Kim Altintop Aug. 9, 2021, 9:18 p.m. UTC | #3
On Mon Aug 9, 2021 at 9:16 PM CEST, Junio C Hamano wrote:
> >
> > +# 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.

Noted.

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

Noted, sorry.

I can't seem to be able teach `git diff --check` to exit non-zero on whitespace
errors.

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

Heh, this is turning into an exercise to forget everything about bash :/

I didn't know that an error in a nested command substitution would not cause the
outer one to fail. As we can't use a pipe either (as Jonathan Nieder suggests),
I think a redirection like this is indeed necessary. The only other option I
could think of would be assigning rev-parse to a variable, which arguably is
less readable:

    have=$(git rew-parse a) &&
    test-tool pkt-line pack >in <<-EOF &&
    $(write_fetch_command \
	    refs/heads/main \
	    -- \
	    -- \
	    $have)
    EOF


Thanks
Junio C Hamano Aug. 9, 2021, 9:43 p.m. UTC | #4
Jonathan Nieder <jrnieder@gmail.com> writes:

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

Yeah, write_command was not saving all that much to begin with.  I
was hoping that there were a lot more commonality that we can save
some typing.
Kim Altintop Aug. 9, 2021, 9:56 p.m. UTC | #5
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`?
Junio C Hamano Aug. 9, 2021, 10:03 p.m. UTC | #6
Kim Altintop <kim@eagain.st> writes:

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

If you drop /dev/stdin, the result would be emimently portable.
"cat" without any argument reads from the standard input stream
and copies it to the standard output stream.
Jonathan Nieder Aug. 9, 2021, 11:01 p.m. UTC | #7
Junio C Hamano wrote:
> Kim Altintop <kim@eagain.st> writes:

>> 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`?
>
> If you drop /dev/stdin, the result would be emimently portable.
> "cat" without any argument reads from the standard input stream
> and copies it to the standard output stream.

Yep, with that tweak (using "cat" instead of "cat /dev/stdin") this
looks like a pleasant enough helper.

Jonathan
Kim Altintop Aug. 10, 2021, 9:44 a.m. UTC | #8
Jonathan Nieder <jrnieder@gmail.com> wrote:
>
> Junio C Hamano wrote:
> > Kim Altintop <kim@eagain.st> writes:
>
> >> 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`?
> >
> > If you drop /dev/stdin, the result would be emimently portable.
> > "cat" without any argument reads from the standard input stream
> > and copies it to the standard output stream.
>
> Yep, with that tweak (using "cat" instead of "cat /dev/stdin") this
> looks like a pleasant enough helper.

Great, I'll reroll like this then. Thanks!
diff mbox series

Patch

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 () {
+	write_command fetch &&
+	echo "0001" &&
+	echo "no-progress" || return
+    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"
+}
+
 # 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 &&

-	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

 	test-tool serve-v2 --stateless-rpc >out <in &&
@@ -145,14 +189,13 @@  test_expect_success 'mix want and want-ref' '
 	git rev-parse e f >expected_commits &&

 	test-tool pkt-line pack >in <<-EOF &&
-	$(write_command fetch)
-	0001
-	no-progress
-	want-ref refs/heads/main
-	want $(git rev-parse e)
-	have $(git rev-parse a)
-	done
-	0000
+	$(write_fetch_command \
+		refs/heads/main \
+		-- \
+		$(git rev-parse e) \
+		-- \
+		$(git rev-parse a) \
+	)
 	EOF

 	test-tool serve-v2 --stateless-rpc >out <in &&
@@ -166,15 +209,13 @@  test_expect_success 'want-ref with ref we already have commit for' '
 	EOF
 	>expected_commits &&

-	oid=$(git rev-parse c) &&
 	test-tool pkt-line pack >in <<-EOF &&
-	$(write_command fetch)
-	0001
-	no-progress
-	want-ref refs/heads/o/foo
-	have $oid
-	done
-	0000
+	$(write_fetch_command \
+		refs/heads/o/foo \
+		-- \
+		-- \
+		$(git rev-parse c) \
+	)
 	EOF

 	test-tool serve-v2 --stateless-rpc >out <in &&