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

[Derrick Stolee <derrickstolee@github.com>]

On 11/10/22 8:59 PM, Victoria Dye wrote:
> Æ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?

I think this should be

  When the client issues a `command=bundle-uri` request, the response is...

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

I can elaborate more here, but the intention is that the protocol defines
only how these key-value pairs are delivered, and how the client assigns
meaning to those values and acts upon them is defined elsewhere.

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

"still" is not needed here.

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

Using MUST simplifies things a lot.

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

How about...

"The content at the advertised URIs MUST be one of two types" ?

> 	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?
 
The files do not need that extension. This sentence can be removed.

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

Thanks.

> And to clarify, by "it MAY disconnect", you mean it may disconnect from the
> main repository server? Or the bundle server? 

The main repository server, since the bundle server is not speaking
the Git protocol, but there is definitely room to clarify here.

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

Will do.

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

This is an interesting idea, although the examples of "good" and "bad" are
probably best left as the test cases. Looking through the rest of this
document, this section is already much more verbose than the others, so I
hesitate to add these examples at this point. Perhaps there is room to
improve the whole document with such examples as a follow-up.

Thanks,
-Stolee