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

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 25, 2025, 17:05 UTC
Message-ID: <xmqqv7ojvjjs.fsf@gitster.g>
In-Reply-To: <20250625125055.1375596-2-christian.couder@gmail.com>

Christian Couder <christian.couder@gmail.com> writes:

Show 11 quoted lines

> @@ -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.