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

phillip.wood123@gmail.com writes:

Hi Jonathan

On 29/09/2023 22:20, Jonathan Tan wrote:

quoted

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.

quoted

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