Re: [PATCH] fetch: download bundles once, even with --all

From: Jeff King <peff@peff.net>
Date: Apr 1, 2023, 08:22 UTC
Message-ID: <20230401082233.GA3503828@coredump.intra.peff.net>
In-Reply-To: <pull.1508.git.1680278344173.gitgitgadget@gmail.com>

On Fri, Mar 31, 2023 at 03:59:04PM +0000, Derrick Stolee via GitGitGadget wrote:

Show 16 quoted lines

> diff --git a/bundle-uri.c b/bundle-uri.c
> index 177c1810402..56390651b92 100644
> --- a/bundle-uri.c
> +++ b/bundle-uri.c
> @@ -792,6 +792,15 @@ int fetch_bundle_uri(struct repository *r, const char *uri,
>  
>  	init_bundle_list(&list);
>  
> +	/*
> +	 * Do not fetch a NULL or empty bundle URI. An empty bundle URI
> +	 * could signal that a configured bundle URI has been disabled.
> +	 */
> +	if (!uri || !*uri) {
> +		result = 0;
> +		goto cleanup;
> +	}

Coverity flagged this "!uri"; it can never be true since we unconditionally xstrdup(uri) at the top of the function, and we'd segfault in that case.

I think the existing code that assumes a non-NULL uri is reasonable. Even before the xstrdup() we'd have segfaulted when it was handed to fetch_bundle_uri_internal().

It's not a huge deal, in the sense that you're just being overly defensive, but the comment and conditional here are a little misleading.

-Peff