On Wed, Jul 31, 2024 at 6:16 PM Taylor Blau [off-list ref] wrote:

On Wed, Jul 31, 2024 at 03:40:13PM +0200, Christian Couder wrote:

quoted

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?

Yeah, but, as for the 'url' field where the value is urlencoded, the
value can be encoded if it could contain some special characters.

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 agree but I think urlencoding, or maybe other simple encodings like
base64, should be easy and simple enough to work around this.

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.

I am not sure how NUL bytes would interfere with the pkt-line.[c,h] code though.

quoted

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

The code doesn't seem risky to me as it allows us to treat the case
when git_config_get_string() fails and when it succeeds but possibly
sets 'url' to NULL (not sure if it's possible though as I didn't
check) in the same way.

Yeah, it means that we have to check if each entry in the strvec is
non-NULL, but I think it's quite easy, and honestly I didn't want to
ask myself questions like should we treat an URL of a remote
configured as an empty string in the same way as the URL not
configured. I think it's much simpler to just pass as-is the content,
if any, that we get from git_config_get_string().

Or maybe you need to in order to advertise promisor remotes without
URLs?

Yeah, I think we should advertise promisor remotes that don't have an
URL configured. It might seem strange, but maybe servers might want in
the future to have hidden/secret URLs (URLs that they use, likely
internally on the server, but don't want to pass for some reason to a
client).

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.

The client might already have an URL for the promisor-remote (and it
might be a different one than the one the server would use if
hidden/secret URLs become a thing). That's why patch 4/4 implements
the "KnownName" value for the "promisor.acceptFromServer" config
option.