Re: [PATCH 1/7] config: handle NULL value when parsing non-bools

From: Jeff King <peff@peff.net>
Date: Dec 12, 2023, 00:58 UTC
Message-ID: <20231212005807.GC376323@coredump.intra.peff.net>
In-Reply-To: <ZXF-8iNH0qaJSVl9@tanuki>

On Thu, Dec 07, 2023 at 09:14:42AM +0100, Patrick Steinhardt wrote:

Show 6 quoted lines

> >  	if (!strcmp(k, "uploadpack.blobpackfileuri")) {
> [...]
> This isn't part of the diff and not a new issue, but why don't we
> `return 0` when parsing this config correctly? We fall through to
> `git_default_config()` even if we've successfully parsed the config key,
> which seems like a bug to me.

I don't think it's a functional bug, but merely a pessimization. We can return early if we know we've handled the option, but the rest of the code would simply fail to match it. So we are just wasting a few strcmp calls (and an unknown key already wastes the same number).

So I think it is a good practice to return, but not really a bug if we don't.

Show 9 quoted lines

> >  	if (!strcmp(var, "core.checkstat")) {
> > +		if (!value)
> > +			return config_error_nonbool(var);
> >  		if (!strcasecmp(value, "default"))
> >  			check_stat = 1;
> >  		else if (!strcasecmp(value, "minimal"))
> 
> We would ignore `true` here, so should we ignore implicit `true`, as
> well?

IMHO the lack of a final "else" in the strcasecmp if-cascade is a bug (and I sent a fix as part of the "config fixes on top" series). Even if we want to leave it for historical reasons, I think it's still worth returning an error for the NULL case (since we know it would have segfaulted previously).

(I snipped the rest of your mail, as I think my response to the cover letter covers the general discussion).

-Peff