From: Christian Couder <christian.couder@gmail.com>
Date: Mon, 21 Jul 2025 14:08:24 GMT
Subject: Re: [PATCH v5 1/5] promisor-remote: refactor to get rid of 'struct strvec'
Message-ID: <CAP8UFD0tHnYKh5QeVq_YUYrnk4yZW04_cggbGG-NC_EksHXELA@mail.gmail.com>
In-Reply-To: <xmqqv7ojvjjs.fsf@gitster.g>

On Wed, Jun 25, 2025 at 7:05 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Christian Couder <christian.couder@gmail.com> 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.