Re: [PATCH 2/8] bundle-uri: parse bundle.heuristic=creationToken
- From
Derrick Stolee <derrickstolee@github.com>
- Date
- Jan 9, 2023, 14:20 UTC
- Message-ID
- <35bcfd1f-ff54-fc2a-0477-6454bca03a87@github.com>
- In-Reply-To
- <xmqqilhgxwj4.fsf@gitster.g>
On 1/8/2023 9:38 PM, Junio C Hamano wrote:
Show 16 quoted lines
> "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] = {
> ...
> };Yes, I should have used this size indicator.
Show 29 quoted lines
> 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 ...
>
>> @@ -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.You're right. I was following an established pattern of linking enums to values, but I'm not sure that those other examples will loop over the array like this looking for a value.
A safer approach would be to have an array of (enum, string) pairs that could either be iterated in a loop (fast enough for a small number of enum values, such as this case) or used to populate a hashmap at runtime if needed for a large number of queries.
Show 20 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).This is a great example of "doing the typically right thing" but without thinking of _why_ we do that thing. Thanks for pointing this out.
Thanks, -Stolee