Re: [PATCH 3/4] Add 'promisor-remote' capability to protocol v2
- From
Taylor Blau <me@ttaylorr.com>
- Date
- Jul 31, 2024, 16:16 UTC
- Message-ID
- <ZqpjV/A9ZVPDiGgA@nand.local>
- In-Reply-To
- <20240731134014.2299361-4-christian.couder@gmail.com>
On Wed, Jul 31, 2024 at 03:40:13PM +0200, Christian Couder wrote:
Show 16 quoted lines
> diff --git a/Documentation/gitprotocol-v2.txt b/Documentation/gitprotocol-v2.txt > index 414bc625d5..4d8d3839c4 100644 > --- a/Documentation/gitprotocol-v2.txt > +++ b/Documentation/gitprotocol-v2.txt > @@ -781,6 +781,43 @@ retrieving the header from a bundle at the indicated URI, and thus > save themselves and the server(s) the request(s) needed to inspect the > headers of that bundle or bundles. > > +promisor-remote=<pr-infos> > +~~~~~~~~~~~~~~~~~~~~~~~~~~ > + > +The server may advertise some promisor remotes it is using, if it's OK > +for the server that a client uses them too. In this case <pr-infos> > +should be of the form: > + > + pr-infos = pr-info | pr-infos ";" pr-info
So <pr-infos> uses the ';' character to delimit multiple <pr-info>s, which means that <pr-info> can't use ';' itself. You mention above that <pr-info> is supposed to be generic so that we can add other fields to it in the future. Do you imagine that any of those fields might want to use the ';' in their values?
One that comes to mind is the shared token example you wrote about above. It would be nice to not restrict what characters the token can contain.
I wonder if it would instead be useful to have <pr-infos> first write out how many <pr-info>s it contains, and then write out each <pr-info> separated by a NUL byte, so that none of the files in the <pr-info> itself are restricted in what characters they can use.
Show 14 quoted lines
> +static void promisor_info_vecs(struct repository *repo,
> + struct strvec *names,
> + struct strvec *urls)
> +{
> + struct promisor_remote *r;
> +
> + promisor_remote_init(repo);
> +
> + for (r = repo->promisor_remote_config->promisors; r; r = r->next) {
> + char *url;
> + char *url_key = xstrfmt("remote.%s.url", r->name);
> +
> + strvec_push(names, r->name);
> + strvec_push(urls, git_config_get_string(url_key, &url) ? NULL : url);Do you mean to push NULL onto urls here? It seems risky since you have to check that each entry in the strvec is non-NULL before printing it out (which you do below in promisor_remote_info()).
Or maybe you need to in order to advertise promisor remotes without URLs? If so, I'm not sure what the benefit would be to the client if it doesn't know where to go to retrieve any objects without having a URL.
Thanks, Taylor