| Message ID | 991a3aa27dd7fe67adbed2e03502790932b5059c.1557868134.git.jonathantanmy@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Partial clone fix: handling received REF_DELTA | expand |
Hi Jonathan, On Tue, 14 May 2019, Jonathan Tan wrote: > diff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh > index 9a8f9886b3..7cc0c71556 100755 > --- a/t/t5616-partial-clone.sh > +++ b/t/t5616-partial-clone.sh > @@ -244,11 +244,25 @@ test_expect_success 'fetch what is specified on CLI even if already promised' ' > . "$TEST_DIRECTORY"/lib-httpd.sh > start_httpd > > -# Converts bytes into a form suitable for inclusion in a sed command. For > -# example, "printf 'ab\r\n' | hex_unpack" results in '\x61\x62\x0d\x0a'. > -sed_escape () { > - perl -e '$/ = undef; $input = <>; print unpack("H2" x length($input), $input)' | > - sed 's/\(..\)/\\x\1/g' > +# Converts bytes into their hexadecimal representation. For example, > +# "printf 'ab\r\n' | hex_unpack" results in '61620d0a'. > +hex_unpack () { > + perl -e '$/ = undef; $input = <>; print unpack("H2" x length($input), $input)' > +} > + > +# Inserts $1 at the start of the string and every 2 characters thereafter. > +intersperse () { > + sed 's/\(..\)/'$1'\1/g' > +} > + > +# Create a one-time-sed command to replace the existing packfile with $1. > +replace_packfile () { > + # The protocol requires that the packfile be sent in sideband 1, hence > + # the extra \x01 byte at the beginning. > + printf "1,/packfile/!c %04x\\\\x01%s0000" \ > + "$(($(wc -c <$1) + 5))" \ > + "$(hex_unpack <$1 | intersperse '\\x')" \ > + >"$HTTPD_ROOT_PATH/one-time-sed" > } Urgh. This is not a problem *this* patch introduces, but why on Earth do we have to do complicated computations in shell code using an unholy mix of complex sed and Perl invocations, making things fragile and slow? We do have such a nice facility is the t/test-tool helper... The refactoring itself looks correct to me, of course. Thanks, Dscho > > test_expect_success 'upon cloning, check that all refs point to objects' ' > @@ -270,10 +284,7 @@ test_expect_success 'upon cloning, check that all refs point to objects' ' > # Replace the existing packfile with the crafted one. The protocol > # requires that the packfile be sent in sideband 1, hence the extra > # \x01 byte at the beginning. > - printf "1,/packfile/!c %04x\\\\x01%s0000" \ > - "$(($(wc -c <incomplete.pack) + 5))" \ > - "$(sed_escape <incomplete.pack)" \ > - >"$HTTPD_ROOT_PATH/one-time-sed" && > + replace_packfile incomplete.pack && > > # Use protocol v2 because the sed command looks for the "packfile" > # section header. > @@ -313,10 +324,7 @@ test_expect_success 'when partial cloning, tolerate server not sending target of > # Replace the existing packfile with the crafted one. The protocol > # requires that the packfile be sent in sideband 1, hence the extra > # \x01 byte at the beginning. > - printf "1,/packfile/!c %04x\\\\x01%s0000" \ > - "$(($(wc -c <incomplete.pack) + 5))" \ > - "$(sed_escape <incomplete.pack)" \ > - >"$HTTPD_ROOT_PATH/one-time-sed" && > + replace_packfile incomplete.pack && > > # Use protocol v2 because the sed command looks for the "packfile" > # section header. > -- > 2.21.0.1020.gf2820cf01a-goog > >
> > +# Converts bytes into their hexadecimal representation. For example, > > +# "printf 'ab\r\n' | hex_unpack" results in '61620d0a'. > > +hex_unpack () { > > + perl -e '$/ = undef; $input = <>; print unpack("H2" x length($input), $input)' > > +} > > + > > +# Inserts $1 at the start of the string and every 2 characters thereafter. > > +intersperse () { > > + sed 's/\(..\)/'$1'\1/g' > > +} > > + > > +# Create a one-time-sed command to replace the existing packfile with $1. > > +replace_packfile () { > > + # The protocol requires that the packfile be sent in sideband 1, hence > > + # the extra \x01 byte at the beginning. > > + printf "1,/packfile/!c %04x\\\\x01%s0000" \ > > + "$(($(wc -c <$1) + 5))" \ > > + "$(hex_unpack <$1 | intersperse '\\x')" \ > > + >"$HTTPD_ROOT_PATH/one-time-sed" > > } > > Urgh. This is not a problem *this* patch introduces, but why on Earth do > we have to do complicated computations in shell code using an unholy mix > of complex sed and Perl invocations, making things fragile and slow? We do > have such a nice facility is the t/test-tool helper... This might be a good #leftoverbits. I'm not sure which part you think needs to be replaced - maybe the thing that goes into one-time-sed? > The refactoring itself looks correct to me, of course. Thanks, and thanks for taking a look at this.
diff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh index 9a8f9886b3..7cc0c71556 100755 --- a/t/t5616-partial-clone.sh +++ b/t/t5616-partial-clone.sh @@ -244,11 +244,25 @@ test_expect_success 'fetch what is specified on CLI even if already promised' ' . "$TEST_DIRECTORY"/lib-httpd.sh start_httpd -# Converts bytes into a form suitable for inclusion in a sed command. For -# example, "printf 'ab\r\n' | hex_unpack" results in '\x61\x62\x0d\x0a'. -sed_escape () { - perl -e '$/ = undef; $input = <>; print unpack("H2" x length($input), $input)' | - sed 's/\(..\)/\\x\1/g' +# Converts bytes into their hexadecimal representation. For example, +# "printf 'ab\r\n' | hex_unpack" results in '61620d0a'. +hex_unpack () { + perl -e '$/ = undef; $input = <>; print unpack("H2" x length($input), $input)' +} + +# Inserts $1 at the start of the string and every 2 characters thereafter. +intersperse () { + sed 's/\(..\)/'$1'\1/g' +} + +# Create a one-time-sed command to replace the existing packfile with $1. +replace_packfile () { + # The protocol requires that the packfile be sent in sideband 1, hence + # the extra \x01 byte at the beginning. + printf "1,/packfile/!c %04x\\\\x01%s0000" \ + "$(($(wc -c <$1) + 5))" \ + "$(hex_unpack <$1 | intersperse '\\x')" \ + >"$HTTPD_ROOT_PATH/one-time-sed" } test_expect_success 'upon cloning, check that all refs point to objects' ' @@ -270,10 +284,7 @@ test_expect_success 'upon cloning, check that all refs point to objects' ' # Replace the existing packfile with the crafted one. The protocol # requires that the packfile be sent in sideband 1, hence the extra # \x01 byte at the beginning. - printf "1,/packfile/!c %04x\\\\x01%s0000" \ - "$(($(wc -c <incomplete.pack) + 5))" \ - "$(sed_escape <incomplete.pack)" \ - >"$HTTPD_ROOT_PATH/one-time-sed" && + replace_packfile incomplete.pack && # Use protocol v2 because the sed command looks for the "packfile" # section header. @@ -313,10 +324,7 @@ test_expect_success 'when partial cloning, tolerate server not sending target of # Replace the existing packfile with the crafted one. The protocol # requires that the packfile be sent in sideband 1, hence the extra # \x01 byte at the beginning. - printf "1,/packfile/!c %04x\\\\x01%s0000" \ - "$(($(wc -c <incomplete.pack) + 5))" \ - "$(sed_escape <incomplete.pack)" \ - >"$HTTPD_ROOT_PATH/one-time-sed" && + replace_packfile incomplete.pack && # Use protocol v2 because the sed command looks for the "packfile" # section header.
A subsequent patch will perform the same packfile replacement that is already done twice, so refactor it into its own function. Also, the same subsequent patch will use, in another way, part of the packfile replacement functionality, so extract those out too. Signed-off-by: Jonathan Tan <jonathantanmy@google.com> --- t/t5616-partial-clone.sh | 34 +++++++++++++++++++++------------- 1 file changed, 21 insertions(+), 13 deletions(-)