Re: [PATCH 1/2] config: document git config getter return value.
Jeff King writes: > This is returning the value of git_config_from_file(), which is 0/-1. I > think it should be left where it is in the original, and not part of > this block of functions. > > Other than that, the patch seems sane to me (I think the 0/1 return > value from these "get" functions is unnecessarily inconsistent with the > rest of Git, but changing it would be a pain. Documenting it is at least > a step forward). I do not think changing it would be in the scope of this series. It makes sense to document (1) that zero is success, non-zero is failure, to help those who are reading the current callers and adding new callers, and (2) that we are aware that the exact "non-zero" value chosen for these are not in line with the rest of the system. The latter can just be a "NEEDSWORK" comment.
Re: [PATCH 1/2] config: document git config getter return value.
On Thu, Aug 02, 2018 at 08:29:48PM -0700, Jonathan Nieder wrote: > (cc-ing peff, config whiz) Actually, this is all about the configset caching layer, which I never really worked on. This is mostly from Tanay Abhra , who was a GSoC student mentored by Matthieu Moy . That said... > > + > > +/* > > + * These functions return 1 if not found, and 0 if found, leaving the found > > + * value in the 'dest' pointer. > > + */ > > +extern int git_configset_add_file(struct config_set *cs, const char > > *filename); > > This function doesn't take a 'dest' argument. Is the comment meant to > apply to it? If not, can the comment go below it? This is returning the value of git_config_from_file(), which is 0/-1. I think it should be left where it is in the original, and not part of this block of functions. Other than that, the patch seems sane to me (I think the 0/1 return value from these "get" functions is unnecessarily inconsistent with the rest of Git, but changing it would be a pain. Documenting it is at least a step forward). -Peff
Re: [PATCH 1/2] config: document git config getter return value.
(cc-ing peff, config whiz)
Hi,
Han-Wen Nienhuys wrote:
> Subject: config: document git config getter return value.
micronit: no period at the end of subject line
> ---
> config.h | 10 --
> 1 file changed, 8 insertions(+), 2 deletions(-)
May we forge your sign-off? See Documentation/SubmittingPatches section
[[sign-off]] for what this means.
>
> diff --git a/config.h b/config.h
> index b95bb7649..41700f40b 100644
> --- a/config.h
> +++ b/config.h
> @@ -178,10 +178,16 @@ struct config_set {
> };
>
> extern void git_configset_init(struct config_set *cs);
> -extern int git_configset_add_file(struct config_set *cs, const char
> *filename);
> -extern int git_configset_get_value(struct config_set *cs, const char *key,
> const char **value);
> +
nit: this blank line feels out of place (though I don't particularly
mind it and wouldn't reroll just for that)
> extern const struct string_list *git_configset_get_value_multi(struct
> config_set *cs, const char *key);
> extern void git_configset_clear(struct config_set *cs);
> +
> +/*
> + * These functions return 1 if not found, and 0 if found, leaving the found
> + * value in the 'dest' pointer.
> + */
> +extern int git_configset_add_file(struct config_set *cs, const char
> *filename);
This function doesn't take a 'dest' argument. Is the comment meant to
apply to it? If not, can the comment go below it?
> +extern int git_configset_get_value(struct config_set *cs, const char *key,
> const char **dest);
> extern int git_configset_get_string_const(struct config_set *cs, const char
> *key, const char **dest);
> extern int git_configset_get_string(struct config_set *cs, const char *key,
> char **dest);
> extern int git_configset_get_int(struct config_set *cs, const char *key, int
> *dest);
With a sign-off and whatever subset of the above suggestions makes sense
to you,
Reviewed-by: Jonathan Nieder
Thanks.
Re: [PATCH 1/2] config: document git config getter return value.
Han-Wen Nienhuys writes:
> ---
> config.h | 10 --
> 1 file changed, 8 insertions(+), 2 deletions(-)
Should I forge your sign-off on this patch?
>
> diff --git a/config.h b/config.h
> index b95bb7649..41700f40b 100644
> --- a/config.h
> +++ b/config.h
> @@ -178,10 +178,16 @@ struct config_set {
> };
>
> extern void git_configset_init(struct config_set *cs);
> -extern int git_configset_add_file(struct config_set *cs, const char
> *filename);
> -extern int git_configset_get_value(struct config_set *cs, const char *key,
> const char **value);
> +
> extern const struct string_list *git_configset_get_value_multi(struct
> config_set *cs, const char *key);
> extern void git_configset_clear(struct config_set *cs);
> +
> +/*
> + * These functions return 1 if not found, and 0 if found, leaving the found
> + * value in the 'dest' pointer.
> + */
> +extern int git_configset_add_file(struct config_set *cs, const char
> *filename);
> +extern int git_configset_get_value(struct config_set *cs, const char *key,
> const char **dest);
> extern int git_configset_get_string_const(struct config_set *cs, const char
> *key, const char **dest);
> extern int git_configset_get_string(struct config_set *cs, const char *key,
> char **dest);
> extern int git_configset_get_int(struct config_set *cs, const char *key, int
> *dest);
Re: [PATCH 1/2] config: document git config getter return value.
On Thu, Aug 2, 2018 at 8:13 AM Han-Wen Nienhuys wrote:
> diff --git a/config.h b/config.h
> @@ -178,10 +178,16 @@ struct config_set {
> extern void git_configset_init(struct config_set *cs);
> -extern int git_configset_add_file(struct config_set *cs, const char
> *filename);
> -extern int git_configset_get_value(struct config_set *cs, const char *key,
> const char **value);
> +/*
> + * These functions return 1 if not found, and 0 if found, leaving the found
> + * value in the 'dest' pointer.
> + */
> +extern int git_configset_add_file(struct config_set *cs, const char
> *filename);
> +extern int git_configset_get_value(struct config_set *cs, const char *key,
> const char **dest);
> extern int git_configset_get_string_const(struct config_set *cs, const char
> *key, const char **dest);
> extern int git_configset_get_string(struct config_set *cs, const char *key,
> char **dest);
> extern int git_configset_get_int(struct config_set *cs, const char *key, int
> *dest);
It doesn't seem like git_configset_add_file() fits in this group. It's
neither searching for something (thus won't return "found" / "not
found"), nor is it returning 0 or 1. (It returns 0 on success and -1
on failure.)
