On 10/10/2023 18:43, Jonathan Tan wrote:
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.
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.
+/**
+ * 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
