Re: [PATCH v5 1/5] promisor-remote: refactor to get rid of 'struct strvec'

[Junio C Hamano <gitster@pobox.com>]

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").

Thanks.