From: Jeff Hostetler Date: Thu, 09 Mar 2017 18:34:32 GMT Subject: Re: [PATCH 04/10] upload-pack: add partial (sparse) fetch Message-ID: In-Reply-To: <20170309074849.ktl5vqbzkiwwwbob@sigill.intra.peff.net> 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