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

[Kim Altintop <kim@eagain.st>]

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