Re: [PATCH v7 3/9] config API: add and use a "git_config_get()" family of functions

From: Glen Choo <chooglen@google.com>
Date: Mar 9, 2023, 18:53 UTC
Message-ID: <kl6ledpxhi3t.fsf@chooglen-macbookpro.roam.corp.google.com>
In-Reply-To: <patch-v7-3.9-a977b7b188f-20230308T090513Z-avarab@gmail.com>

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

Show 10 quoted lines

> diff --git a/config.h b/config.h
> index 7606246531a..7dd62ca81bf 100644
> --- a/config.h
> +++ b/config.h
> @@ -465,6 +465,9 @@ void git_configset_clear(struct config_set *cs);
>   * value in the 'dest' pointer.
>   */
>  
> +RESULT_MUST_BE_USED
> +int git_configset_get(struct config_set *cs, const char *key);

IIRC, feedback on v4 [1] mentioned that since git_configset_get() can return negative values, it probably shouldn't come under this comment:

  /*
  * These functions return 1 if not found, and 0 if found, leaving the found
  * value in the 'dest' pointer.
  */

I think moving it to before the comment would suffice, maybe with a pointer to the corresponding repo_* or git_*.

1. https://lore.kernel.org/git/xmqqv8kjpqoe.fsf@gitster.g/

Show 24 quoted lines

> @@ -485,6 +488,14 @@ int git_configset_get_pathname(struct config_set *cs, const char *key, const cha
>  /* Functions for reading a repository's config */
>  struct repository;
>  void repo_config(struct repository *repo, config_fn_t fn, void *data);
> +
> +/**
> + * Run only the discover part of the repo_config_get_*() functions
> + * below, in addition to 1 if not found, returns negative values on
> + * error (e.g. if the key itself is invalid).
> + */
> +RESULT_MUST_BE_USED
> +int repo_config_get(struct repository *repo, const char *key);
>  int repo_config_get_value(struct repository *repo,
>  			  const char *key, const char **value);
>  const struct string_list *repo_config_get_value_multi(struct repository *repo,
> @@ -521,8 +532,15 @@ void git_protected_config(config_fn_t fn, void *data);
>   * manner, the config API provides two functions `git_config_get_value`
>   * and `git_config_get_value_multi`. They both read values from an internal
>   * cache generated previously from reading the config files.
> + *
> + * For those git_config_get*() functions that aren't documented,
> + * consult the corresponding repo_config_get*() function's
> + * documentation.
>   */

After rereading config.h, I really appreciate comments like this that try to control the documentation load. We have configset*, repo* and git*, _and_ the comments are spread out hapzardly around config.h with no pointers to the corresponding comments. I think we're overdue for reorganization, and this sort of comment helps a lot with that.

As a suggestion, it seems like the git_config_get*() functions are actually the better documented ones - nearly all of them have comments, whereas the repo_config_get_*() ones typically don't, so maybe adding the comment to git_config_get() instead of repo_config_get() would be better for this series:

----- >8 --------- >8 --------- >8 --------- >8 --------- >8 ----
  diff --git a/config.h b/config.h
  index 7dd62ca81b..aa9bdf8df4 100644
  --- a/config.h
  +++ b/config.h
  @@ -489,10 +489,10 @@ int git_configset_get_pathname(struct config_set *cs, const char *key, const cha
  struct repository;
  void repo_config(struct repository *repo, config_fn_t fn, void *data);

  -/**
  - * Run only the discover part of the repo_config_get_*() functions
  - * below, in addition to 1 if not found, returns negative values on
  - * error (e.g. if the key itself is invalid).
  +/*
  + * These repo_config_get*() functions each correspond to to a git_config_get*()
  + * function. Consult the corresponding git_config_get*() documentation for more
  + * information.
    */
  RESULT_MUST_BE_USED
  int repo_config_get(struct repository *repo, const char *key);
  @@ -532,12 +532,13 @@ void git_protected_config(config_fn_t fn, void *data);
    * manner, the config API provides two functions `git_config_get_value`
    * and `git_config_get_value_multi`. They both read values from an internal
    * cache generated previously from reading the config files.
  - *
  - * For those git_config_get*() functions that aren't documented,
  - * consult the corresponding repo_config_get*() function's
  - * documentation.
    */

  +/**
  + * Run only the discover part of the repo_config_get_*() functions
  + * below, in addition to 1 if not found, returns negative values on
  + * error (e.g. if the key itself is invalid).
  + */
  RESULT_MUST_BE_USED
  int git_config_get(const char *key);
----- >8 --------- >8 --------- >8 --------- >8 --------- >8 ----

Though in the long run, I'd prefer having the docs on the more "general" APIs (configset_*, repo_*) instead of the more "specific" ones (git_*). Perhaps you had a similar intent while making this change, but I think this might be better left as a cleanup.

As an aside, I really appreciate your effort in sticking with the config interface work. I think it's grown quite unruly, and it's worth trying to tame it.