Re: [PATCH 2/8] bundle-uri: parse bundle.heuristic=creationToken

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 9, 2023, 02:38 UTC
Message-ID: <xmqqilhgxwj4.fsf@gitster.g>
In-Reply-To: <9007249b9488c23f00c2d498ffd520e4af8b37a4.1673037405.git.gitgitgadget@gmail.com>

"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:

> +static const char *heuristics[] = {
> +	[BUNDLE_HEURISTIC_NONE] = "",
> +	[BUNDLE_HEURISTIC_CREATIONTOKEN] = "creationToken",
> +};

Ideally it would require the least amount of maintenance if we could define BUNDLE_HEURISTIC__COUNT as ARRAY_SIZE() of this thing, but it being a file scope static, it might not be easy to arrange that. As a lessor altenative, would it make it safer to size this array more explicitly using BUNDLE_HEURISTIC__COUNT macro?

	static const char *heuristics[BUNDLE_HEURISTIC__COUNT] = {
		...
	};

or is it more-or-less moot point to aim for safety because nobody enforces that these [indices] used to define the contents of this array are dense?

That is ...

Show 12 quoted lines

> @@ -142,6 +150,19 @@ static int bundle_list_update(const char *key, const char *value,
>  			return 0;
>  		}
>  
> +		if (!strcmp(subkey, "heuristic")) {
> +			int i;
> +			for (i = 0; i < BUNDLE_HEURISTIC__COUNT; i++) {
> +				if (!strcmp(value, heuristics[i])) {
> +					list->heuristic = i;
> +					return 0;
> +				}
> +			}

... this strcmp() will segfault if heuristics[] array is sparse, or BUNDLE_HEURISTIC__COUNT is larger than the array (i.e. you add a new heuristic in "enum bundle_heuristic" before the __COUNT sentinel, but forget to add it to the heuristics[] array).

"You are worrying too much. Our developers would notice a segfault and the current code, which may look risky to you, is something they can live with", is a perfectly acceptable response, but somehow I have this nagging feeling that we should be able to make it easier to maintain without incurring extra runtime cost.

Show 15 quoted lines

> diff --git a/bundle-uri.h b/bundle-uri.h
> index d5e89f1671c..ad82174112d 100644
> --- a/bundle-uri.h
> +++ b/bundle-uri.h
> @@ -52,6 +52,14 @@ enum bundle_list_mode {
>  	BUNDLE_MODE_ANY
>  };
>  
> +enum bundle_list_heuristic {
> +	BUNDLE_HEURISTIC_NONE = 0,
> +	BUNDLE_HEURISTIC_CREATIONTOKEN,
> +
> +	/* Must be last. */
> +	BUNDLE_HEURISTIC__COUNT,
> +};

The only reason to leave a trailing comma is to make it easy to append new values at the end. By omitting the trailing comma, you can doubly signal "Must be last" here (not suggesting to remove the comment; suggesting to remove the trailing comma).

Thanks.