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

[Jonathan Tan <jonathantanmy@google.com>]

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. I'm open to changing
git_config_get_expiry_date_in_days() too, though...we probably don't
need so many days.

> > +/**
> > + * 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).