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