On Wed, Jun 25, 2025 at 7:05 PM Junio C Hamano <gitster@xxxxxxxxx> wrote:
>
> Christian Couder <christian.couder@xxxxxxxxx> writes:
>
> > @@ -414,11 +439,15 @@ static int should_accept_remote(enum accept_promisor accept,
> > return 0;
> > }
> >
> > - if (!strcmp(urls->v[i], remote_url))
> > + if (!p->url)
> > + BUG("bad config_info (URL is NULL) for remote '%s'",
> > + remote_name);
> > +
> > + if (!strcmp(p->url, remote_url))
> > return 1;
>
> The code seems to trust string_list enough that once an earlier call
> to string_list_lookup() in this code path finds the entry for the
> remote_name, item->util is assumed to be non-NULL and points at a
> valid promisor_info instance. But it does not seem to be defensive
> against p being NULL, but it does so for p->url being NULL, which
> smells fishy. Once promisor_config_info_list() reads the data from
> the configuration, do we ever update the contents and the risk of us
> corrupting p->url while we are running is great enough to warrant
> being defensive?
>
> My inclination is to suggest removing the check and BUG() here, but
> I may be missing something obvious (like "p->url gets NULL'ed out in
> this function, but then such a remote is never checked with this
> function because such and such logic in that function").
Yeah, right, if we trust promisor_config_info_list() to properly
populate the `config_info` string list (which is not modified
anywhere), let's trust it fully. So I have removed the `if (!p->url)`
check and the associated `BUG(...)`, in the v6.
Thanks.
