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

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Thu, Mar 09 2023, Glen Choo wrote:

> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>> 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/

I'll fix this, FWIW I was trying to juggle this so that I'd avoid future
churn for a subsequent cleanup of the interface & documentation...

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

...yes, that's why I put the primary documentation on the repo_*()
version here, as with other implicit "the_repository" and "the_index"
migrations I think we should be moving towards using those, and
eventually removing the non-repo_*() ones (in some cases they're almost
unused, or it's easy enough to migrate the rest).

But let's leave that for some future cleanup, but for now I think it's
OK to leave this slight inconsistency in place, with an eye to such
future consolidation.

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

Thanks, hopefully a trivial & upcoming v8 will be the last version...