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

[Jeff King <peff@peff.net>]

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

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

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