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

From: phillip.wood123@gmail.com <phillip.wood123@gmail.com>
Date: Oct 10, 2023, 10:00 UTC
Message-ID: <1de0a6f3-e223-4e84-a6d2-51d9b51a02f6@gmail.com>
In-Reply-To: <5d9f0b3de08ab8541482b9b640db06b6d3000b86.1696021277.git.jonathantanmy@google.com>

Hi Jonathan

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

Show 10 quoted lines

> 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.

> +int git_parse_ssize_t(const char *, ssize_t *);
> +int git_parse_ulong(const char *, unsigned long *);
> +int git_parse_int(const char *value, int *ret);
> +int git_parse_int64(const char *value, int64_t *ret);

This was previously private but I think it makes sense for it to be publicly available.

Show 6 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