| Message ID | 20210809175530.75326-2-kim@eagain.st (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | upload-pack: treat want-ref relative to namespace | expand |
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.
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
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
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.
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`?
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.
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
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 --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 &&
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