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

quoted

 	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.

quoted

 	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