Re: [PATCH v2 6/9] Documentation: add Packfile URIs design doc

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Nov 25, 2020, 09:15 UTC
Message-ID: <87zh35okzy.fsf@evledraar.gmail.com>
In-Reply-To: <6344c225897de1a2d8aa86d610e9eaf1c6ec82b4.1591821067.git.jonathantanmy@google.com>

On Wed, Jun 10 2020, Jonathan Tan wrote:

Show 5 quoted lines

> +This is the implementation: a feature, marked experimental, that allows the
> +server to be configured by one or more `uploadpack.blobPackfileUri=<sha1>
> +<uri>` entries. Whenever the list of objects to be sent is assembled, all such
> +blobs are excluded, replaced with URIs. The client will download those URIs,
> +expecting them to each point to packfiles containing single blobs.

I was poking at this recently to see whether I could change it into the more dumb method I noted in https://public-inbox.org/git/87k1hv6eel.fsf@evledraar.gmail.com/

As an aside on a protocol level could that be supported with this current verb by having the client say "packfile-uris=early" or something like that instead of "packfile-uris"? The server advertising the same, and the client then just requesting packfile-uris before ls-refs or whatever? The server would need to be stateful about what's requested when and serve up something different than the current one-blob-per-pack. Any pointers to where/how to implement that would be welcome, I got lost in the non-linearity of the connect.c/fetch-pack.c/upload-pack.c code yesterday.

But I'm mainly replying here to ask if it's intentional that clients are
tolerant of the server sending whatever it pleases in the supposedly
"single blob" packs. As demonstrated by the tests passing with this
patch:
    
    diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh
    index 7d5b17909bb..4fe2030f4c1 100755
    --- a/t/t5702-protocol-v2.sh
    +++ b/t/t5702-protocol-v2.sh
    @@ -797,11 +797,12 @@ test_expect_success 'when server does not send "ready", expect FLUSH' '
     
     configure_exclusion () {
     	git -C "$1" hash-object "$2" >objh &&
    +	echo -n shattered | git -C "$1" hash-object --stdin -w >>objh &&
     	git -C "$1" pack-objects "$HTTPD_DOCUMENT_ROOT_PATH/mypack" <objh >packh &&
     	git -C "$1" config --add \
     		"uploadpack.blobpackfileuri" \
    -		"$(cat objh) $(cat packh) $HTTPD_URL/dumb/mypack-$(cat packh).pack" &&
    -	cat objh
    +		"$(head -n 1 objh) $(cat packh) $HTTPD_URL/dumb/mypack-$(cat packh).pack" &&
    +	head -n 1 objh
     }
     
     test_expect_success 'part of packfile response provided as URI' '
    @@ -820,10 +821,11 @@ test_expect_success 'part of packfile response provided as URI' '
     	configure_exclusion "$P" my-blob >h &&
     	configure_exclusion "$P" other-blob >h2 &&
     
    -	GIT_TRACE=1 GIT_TRACE_PACKET="$(pwd)/log" GIT_TEST_SIDEBAND_ALL=1 \
    -	git -c protocol.version=2 \
    +	GIT_TRACE=1 GIT_TRACE2="$(pwd)/log" GIT_TRACE_PACKET="$(pwd)/log" GIT_TEST_SIDEBAND_ALL=1 \
    +	CHECK_SHATTERED=1 git -c protocol.version=2 \
     		-c fetch.uriprotocols=http,https \
     		clone "$HTTPD_URL/smart/http_parent" http_child &&
    +	cp "$(pwd)/log" /tmp/clone.log &&
     
     	# Ensure that my-blob and other-blob are in separate packfiles.
     	for idx in http_child/.git/objects/pack/*.idx
    @@ -832,7 +834,7 @@ test_expect_success 'part of packfile response provided as URI' '
     		{
     			grep "^[0-9a-f]\{16,\} " out || :
     		} >out.objectlist &&
    -		if test_line_count = 1 out.objectlist
    +		if true
     		then
     			if grep $(cat h) out
     			then

As you may guess from the "shattered" I was trying to find if the particulars around the partial fsck allowed me to exploit this somehow, I haven't found a way to do that, just be annoying by sending the client more than they asked for, but I could also do that with the normal dialog. Just wondering if the client should be opening the pack and barfing if it has more than one object, or not care.