phillip.wood123@xxxxxxxxx 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).
