Re: [PATCH v4 4/4] parse: separate out parsing functions from config.h

From: Phillip Wood <phillip.wood123@gmail.com>
Date: Oct 10, 2023, 17:58 UTC
Message-ID: <87c16c09-784f-4818-9231-8023fc2c5a02@gmail.com>
In-Reply-To: <20231010174348.2150150-1-jonathantanmy@google.com>

On 10/10/2023 18:43, Jonathan Tan wrote:

Show 27 quoted lines

> phillip.wood123@gmail.com writes:
>> Hi Jonathan
>>
>> On 29/09/2023 22:20, Jonathan Tan wrote:
>>> diff --git a/parse.h b/parse.h
>>> new file mode 100644
>>> index 0000000000..07d2193d69
>>> --- /dev/null
>>> +++ b/parse.h
>>> @@ -0,0 +1,20 @@
>>> +#ifndef PARSE_H
>>> +#define PARSE_H
>>> +
>>> +int git_parse_signed(const char *value, intmax_t *ret, intmax_t max);
>>
>> Previously this function was private to config.c, now it needs to be
>> public because it is still called by
>> git_config_get_expiry_date_in_days(). As this is essentially an internal
>> helper for git_parse_int() and friends it is a bit unfortunate that it
>> is now public. Perhaps we should change
>> git_config_get_expiry_date_in_days() to call git_parse_int() instead.
>> Then we can keep git_parse_signed() and git_parse_unsigned() private to
>> parse.c.
> 
> It could be argued also that it fits in with the rest of
> the parsing functions - this one parses intmax, and we have
> others of various signedness and size.

This one differs from the others because it expects the caller to pass a maximum value, the intmax_t equivalent to git_parse_int() would be

int git_parse_intmax(const char*, intmax_t*);

We now expose git_parse_int64() which covers a similar case.

> I'm open to changing
> git_config_get_expiry_date_in_days() too, though...we probably don't
> need so many days.

Indeed, the existing code passes maximum_signed_value_of_type(int) as the third argument to limit it to INT_MAX already.

Show 26 quoted lines

>>> +/**
>>> + * Same as `git_config_bool`, except that it returns -1 on error rather
>>> + * than dying.
>>> + */
>>> +int git_parse_maybe_bool(const char *);
>>> +int git_parse_maybe_bool_text(const char *value);
>>
>> This used to be private to config.c and now has callers in parse.c and
>> config.c. We should make it clear that non-config code is likely to want
>> git_parse_maybe_bool() rather than this function.
>>
>> Best Wishes
>>
>> Phillip
> 
> The difference between these 2 functions here is that bool_text supports
> only the textual forms (used, for example, in git_config_bool_or_int()
> which accepts both boolean strings and integers), which might be useful
> elsewhere too. But it could be better documented, yes.
> 
> Looking at "What's Cooking", this series is about to be merged to
> master. We could hold off merging that, but I think we don't need to
> - it could be argued that git_parse_maybe_bool_text() could be better
> documented, but even if we wrote it from scratch, I would probably put
> the extra documentation in its own patch anyway (so one patch for moving
> the code, and another for adding documentation).

I agree it's not worth re-rolling just to add some documentation here.

Best Wishes

Phillip