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
