On Thu, Mar 09 2023, Glen Choo wrote: > Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> 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...
