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

[Jeff King <peff@peff.net>]

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

> 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.

> +		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