On Mon, Feb 06 2023, Glen Choo wrote: > Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> writes: > >> We already have the basic "git_config_get_value()" function and its >> "repo_*" and "configset" siblings to get a given "key" and assign the >> last key found to a provided "value". >> >> But some callers don't care about that value, but just want to use the >> return value of the "get_value()" function to check whether the key >> exist (or another non-zero return value). > > [...] > >> We could have changed git_configset_get_value_multi() to accept a >> "NULL" as a "dest" for all callers, but let's avoid changing the >> behavior of existing API users. Having an "unused" value that we throw >> away internal to config.c is cheap. > > There is yet another option, which is to teach "git_config_get_value()" > (mentioned earlier) to accept NULL to mean "I just want to know if there > is a value, I don't care what it is". That's what the *_get_<type>() > functions use under the hood (i.e. the ones that return either 0 or 1 or > exit). I've clarified the commit message, but that's the same as what this is describing. I.e. I meant "git_config_get_value_multi() and the functions that are wrapping it", which is "git_config_get_value()" etc. > This amounts to implementing the "*_config_key_exists()" API you > mentioned, but I think this is better fit for the current set of > semantics. At the very least, that would be an easy 1-1 replacement for > the *_get_string[_tmp]() replacements we make here. There's also the > small benefit of saving one function implementation. I think this is the wrong approach, and have updated the commit message further to advocate for this one. >> Another name for this function could have been >> "*_config_key_exists()", as suggested in [1]. That would work for all >> of these callers, and would currently be equivalent to this function, >> as the git_configset_get_value() API normalizes all non-zero return >> values to a "1". >> >> But adding that API would set us up to lose information, as e.g. if >> git_config_parse_key() in the underlying configset_find_element() >> fails we'd like to return -1, not 1. > > We were already 'losing' (or rather, not caring about) this information > with the *_get_<type>() functions. The only reason we'd care about this > is if we using git_configset_get_value_multi() or similar. > > We replace two callers of git_configset_get_value_multi() in this patch, > but they didn't care about the -1 case anyway... > [...] > As Junio pointed out, git_configset_get() can now return -1, so this > isn't so accurate any more. git_configset_get() is really the exception > here, since all the other functions in this section are the > git_configset_get_*() functions that use git_configset_get_value(). I'd > prefer returning only 0 or 1 for consistency. I really prefer not clobbering these return values, but I take your point that the end-state here is inconsistent. I figured that I could fix it for the APIs added here, and follow-up (with a patch I already had mostly ready) after this series to fix the remaining config API warts. I won't fix all of those in the incoming re-roll of this, but I'll fix this issue, i.e. we'll consistently ferry up "ret", and stop normalizing non-zero-non-1 to "return 1".
