Re: [PATCH 1/9] protocol v2: add server-side "bundle-uri" skeleton

> Ævar Arnfjörð Bjarmason via GitGitGadget wrote:
>> +
>> +PROTOCOL for bundle-uri
>> +^^^^^^^^^^^^^^^^^^^^^^^
>> +
>> +A `bundle-uri` request takes no arguments, and as noted above does not
>> +currently advertise a capability value. Both may be added in the
>> +future.
>> +
>> +When the client issues a `command=bundle-uri` the response is a list of
> 
> nit: comma after `command=bundle-uri`
> 
> I misread this a couple times dropping the "the", so it read like the
> `command=bundle-uri` was the *response*, not the request. I think the comma
> would help avoid that?

>> +key-value pairs provided as packet lines with value `<key>=<value>`. The
>> +meaning of these key-value pairs are provided by the config keys in the
>> +`bundle.*` namespace (see linkgit:git-config[1]).
> 
> What does this ("the meaning of these key-value pairs are provided by the
> config keys...") mean? Are the response keys in the `bundle.*` namespace? Or
> do the client-side `bundle.*` keys provide some kind of translation of what
> the keys mean?

>> +
>> +Clients are still expected to fully parse the line according to the
>> +above format, lines that do not conform to the format SHOULD be
>> +discarded. The user MAY be warned in such a case.
> 
> Why "still" - is there some reason they *wouldn't* parse the response line? 

> Is "the above format" referring to `<key>=<value>` in general, or restricted
> to/guaranteed that the `<key>`'s defined by the `bundle.*` namespace? I'm
> guessing "still expected to fully parse" == "MUST parse" (using
> MUST/SHOULD/MAY nomeclature), it would help to call that out explicitly to
> be consistent with the rest of this doc.

>> +
>> +bundle-uri CLIENT AND SERVER EXPECTATIONS
>> +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>> +
>> +URI CONTENTS::
>> +The advertised URIs MUST be in one of two possible formats.
>> ++
>> +The first possible format is a bundle file that `git bundle verify`
> 
> I don't think "format" is the right word to describe this (I'd reserve
> "format" for the literal format of the URI string). Maybe something like
> this?
> 
> 	The advertised URIs MUST contain one of two types of content.

> 	The advertised URI may contain a bundle file that `git bundle
> 	verify` would accept...
> 
> 	...
> 	
> 	Alternatively, the advertised URI may provide a plaintext file...
> 
>> +would accept. I.e. they MUST contain one or more reference tips for
>> +use by the client, MUST indicate prerequisites (in any) with standard
>> +"-" prefixes, and MUST indicate their "object-format", if
>> +applicable. Create "*.bundle" files with `git bundle create`.
> 
> The last sentence doesn't add anything that you don't know from the `git
> bundle verify` note in the first doesn't already tell you, and feels like a
> bit of a non-sequitur as a result. Although, it tangentially raises a
> question: do bundle files *have to* have the '.bundle' suffix to pass `git
> bundle verify`? If not, are they expected to when coming from these URIs?

>> +WHEN ADVERTISED BUNDLE(S) REQUIRE NO FURTHER NEGOTIATION::
>> +If after issuing `bundle-uri` and `ls-refs`, and getting the header(s)
>> +of the bundle(s) the client finds that the ref tips it wants can be
>> +retrieved entirety from advertised bundle(s), it MAY disconnect. The
> 
> s/entirety/entirely

>> +bundle-uri PROTOCOL FEATURES
>> +^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>> +
>> +As noted above the `<key>=<value>` definitions are documented by the
>> +`bundle.*` config namespace.
> 
> Same comment as earlier - this is a confusing way to phrase this. If you
> mean "the keys are part of the `bundle.*` namespace documented in
> linkgit:git-config[1]", I think you can just say that directly. If not, it
> would help to clarify the relationship between the `bundle.*` namespace and
> these keys.

> Overall, this document is quite thorough, especially with respect to edge
> cases/error handling. I found it a bit confusing at times (at least
> partially due to my unfamiliarity with protocol v2), including some
> potentially ambiguous phrasing or scenarios (especially those in the
> disconnection & error recovery sections) that are difficult to clearly
> describe in generic terms.
> 
> I think some sections (especially "PROTOCOL for bundle-uri" and "bundle-uri
> CLIENT AND SERVER EXPECTATIONS") would benefit from examples of what "good"
> and "bad" request/response values & behaviors look like; they would help
> illustrate some of those more complex situations. The rest of the patch (the
> implementation & tests) looked good to me.