Re: [PATCH 1/2] config: document git config getter return value.

From: Jeff King <peff@peff.net>
Date: Aug 3, 2018, 13:05 UTC
Message-ID: <20180803130509.GB4671@sigill.intra.peff.net>
In-Reply-To: <20180803032948.GG197924@aiede.svl.corp.google.com>

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 <tanayabh@gmail.com>, who was a GSoC student mentored by Matthieu Moy <Matthieu.Moy@imag.fr>.

That said...

Show 9 quoted lines

> > +
> > +/*
> > + * 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