Re: [PATCH 04/10] upload-pack: add partial (sparse) fetch

From: Jeff King <peff@peff.net>
Date: Mar 9, 2017, 07:48 UTC
Message-ID: <20170309074849.ktl5vqbzkiwwwbob@sigill.intra.peff.net>
In-Reply-To: <1488994685-37403-5-git-send-email-jeffhost@microsoft.com>

On Wed, Mar 08, 2017 at 05:37:59PM +0000, Jeff Hostetler wrote:

Show 19 quoted lines

> diff --git a/Documentation/technical/pack-protocol.txt b/Documentation/technical/pack-protocol.txt
> index c59ac99..0032729 100644
> --- a/Documentation/technical/pack-protocol.txt
> +++ b/Documentation/technical/pack-protocol.txt
> @@ -212,6 +212,7 @@ out of what the server said it could do with the first 'want' line.
>    upload-request    =  want-list
>  		       *shallow-line
>  		       *1depth-request
> +		       *partial
>  		       flush-pkt
>  
>    want-list         =  first-want
> @@ -223,10 +224,15 @@ out of what the server said it could do with the first 'want' line.
>  		       PKT-LINE("deepen-since" SP timestamp) /
>  		       PKT-LINE("deepen-not" SP ref)
>  
> +  partial           =  PKT-LINE("partial-by-size" SP magnitude) /
> +		       PKT-LINE("partial-special)  
> +

I probably would have added this as a capability coming back from the client, since it only makes sense to send once (the same way we ask for other features like include-tag or ofs-delta). I guess it's six of one, half a dozen of the other, though.

I notice that you require the client to request the "partial" capability _and_ to send these commands. I'm not sure what the client capability response is helping. The server has said "I can do this" and the client either asks for it or not.

Show 10 quoted lines

> +		if (skip_prefix(line, "partial-by-size ", &arg)) {
> +			unsigned long s;
> +			if (!client_requested_partial_capability)
> +				die("git upload-pack: 'partial-by-size' option requires 'partial' capability");
> +			if (!git_parse_ulong(arg, &s))
> +				die("git upload-pack: invalid partial-by-size value: %s", line);
> +			strbuf_addstr(&partial_by_size, "--partial-by-size=");
> +			strbuf_addstr(&partial_by_size, arg);
> +			have_partial_by_size = 1;
> +			continue;

So we parse it here for validation, but then pass the original string on to be parsed again by pack-objects. I think I'd rather see us use the result of our parse here, just to avoid any bugs where the parsing isn't identical (and there is such a bug currently due to the signed/unsigned thing I mentioned).

I also wonder whether the magnitude suffixes are worth exposing across the wire. Anybody touching the list of units in git_parse_ulong() would probably be surprised that the protocol is dependent on them (not that I expect us to really take any away, but it just seems like an unnecessary protocol complication).

-Peff