On Mon, Mar 10, 2025 at 5:29 PM Junio C Hamano <gitster@xxxxxxxxx> wrote:
>
> Christian Couder <christian.couder@xxxxxxxxx> writes:
>
> > Pushing NULL into a 'strvec' results in a segfault, as 'strvec' is a
> > kind of NULL terminated array that is designed to be compatible with
> > 'argv' variables used on the command line.
>
> It is good that you corrected a caller that tries to make a strvec
> into such a state, but shouldn't strvec_push() protect itself with a
> BUG or something, I have to wonder.
Actually strvec_push() uses xstrdup() on the value it is passed and
xstrdup() crashes if that value is NULL. So another way to avoid
crashes would be to make xstrdup() BUG when it's passed NULL. Or maybe
xstrdup() should just return NULL in this case?
Also it looks like strvec_push_nodup() kind of works if it is passed a
NULL. (It adds the NULL to the array and grows it.) So I wonder if the
right solution for strvec_push() would be to make it kind of work in
the same way.
Anyway I think these are separate issues that deserve their own
discussions and can wait for after the 2.49.0 release. Here I am just
providing a hotfix for the "promisor-remote" protocol capability.
> > So when an URL is missing from the config, let's push an empty string
> > instead of NULL into the 'strvec' that stores URLs.
>
> How will these strings in the "names" strvec used? When URLs are
> present, I'm sure we are going to use it as URL, but when they are
> missing, what happens?
The 'names' strvec and 'urls' strvec contain what exists in the client
config. They are only used by the promisor remote code to compare the
names and maybe urls in the config with what the server advertises in
case 'promisor.acceptfromserver' is set to KnownName or KnownUrl. This
is done to decide if an advertised promisor remote is accepted or not
by the client.
When 'promisor.acceptfromserver' is set to KnownUrl, a remote should
be rejected in any of the following cases:
- the server doesn't advertise an URL for that remote,
- the client doesn't have an URL configured for that remote.
When 'promisor.acceptfromserver' is set to KnownName, URLs should not
be taken into account to decide if an advertised promisor remote is
accepted or not.
Note that the promisor remote code added by this series doesn't change
the code that actually uses the remote names and urls to clone or
fetch objects, so there is no change there. In particular, if the
client doesn't have an URL configured for a remote, even if the remote
is accepted and the server provides an URL, the client will not be
able to fetch or clone from the remote as it will not use the server
provided URL. The test called "clone with 'KnownName' and missing URL
in the config" shows that.
> The second hunk of this patch seems to
> ignore such a broken entry with an empty URL, but that smells like
> sweeping a problem under the rug. Shouldn't such a promisor be
> either diagnosed as an error, or skipped when you accumulate URLs to
> be used into the strvec, so that the later code (i.e. the second
> hunk) does not even have to worry about it?
If 'promisor.acceptfromserver' is set to KnownName, I think it is
simpler to just ignore URLs altogether. Such a behavior is easier to
document and implement.
Also if we ever develop a mode where the advertised URL can be used
for cloning or fetching by the client, then it won't matter if no URL
is configured on the client side. In fact it might be the common case.
Skipping remotes with no URL would make it more difficult to explain
why a remote was rejected in the KnownUrl case. In the next version, I
have added checks and warnings to help diagnose why a remote is
rejected when a URL is missing.
I have also added a test case where the LOP URL is not configured on
the server side, so not advertised.
> > @@ -356,7 +360,7 @@ char *promisor_remote_info(struct repository *repo)
> > strbuf_addch(&sb, ';');
> > strbuf_addstr(&sb, "name=");
> > strbuf_addstr_urlencode(&sb, names.v[i], allow_unsanitized);
> > - if (urls.v[i]) {
> > + if (urls.v[i] && *urls.v[i]) {
>
> Why does urls.v[i] need to be checked? Didn't you just make sure
> that the strvec would not have NULL in it?
Yeah, right. I changed this to just `if (*urls.v[i])` in the next version.
Thanks for your review.
