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

[Jeff Hostetler <git@jeffhostetler.com>]

On 3/9/2017 2:48 AM, Jeff King wrote:
> 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.

True.  I wanted the size argument.  And later want to add
a sparse-file.  It seemed like they better belonged in a
PKT-LINE than in the capability header.

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

Yeah, I wasn't sure if that was necessary or not.
It looked like there were other fields where the
client advised that it wanted to used a capability
that the server had previously advertised.  If we
don't need it, that's fine.


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

Yeah, I'll change this as I described in an earlier sub-thread.

Jeff