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

Kim Altintop [off-list ref] writes:

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

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

On Mon Aug 9, 2021 at 9:16 PM CEST, Junio C Hamano wrote:

quoted

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

quoted

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

quoted

 # 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