Re: [PATCH v2 4/4] receive-pack: use batched reference updates

From: Jeff King <peff@peff.net>
Date: May 15, 2025, 18:55 UTC
Message-ID: <20250515185535.GA3309052@coredump.intra.peff.net>
In-Reply-To: <20250515-501-update-git-fetch-1-to-use-partial-transactions-v2-4-80cbaaa55d2e@gmail.com>

On Thu, May 15, 2025 at 04:07:28PM +0200, Karthik Nayak wrote:

Show 7 quoted lines

> +failure:
> +	for (cmd = commands; cmd; cmd = cmd->next) {
> +		if (reported_error)
> +			cmd->error_string = reported_error;
> +		else if (strmap_contains(&failed_refs, cmd->ref_name))
> +			cmd->error_string = xstrdup(strmap_get(&failed_refs, cmd->ref_name));
>  	}

Coverity complains about this code, claiming that strmap_get() could return NULL. At first glance, I thought it was being totally stupid, since we just called strmap_contains() above. But it's only being a little bit stupid: even if the entry exists, it is still possible for it to contain NULL.

However, that won't ever be the case, since it is always fed from the set of ref transaction reasons.

Still, I wondered if:

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index bd0fb729ff..fe001bbfe8 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -1897,10 +1897,11 @@ static void execute_commands_non_atomic(struct command *commands,
 
 failure:
 	for (cmd = commands; cmd; cmd = cmd->next) {
+		const char *reason;
 		if (reported_error)
 			cmd->error_string = reported_error;
-		else if (strmap_contains(&failed_refs, cmd->ref_name))
-			cmd->error_string = xstrdup(strmap_get(&failed_refs, cmd->ref_name));
+		else if ((reason = strmap_get(&failed_refs, cmd->ref_name)))
+			cmd->error_string = xstrdup(reason);
 	}
 
 cleanup:

might be simpler, and skip the extra lookup? I dunno, it is probably
getting into bikeshedding, and it is not like this is the only Coverity
false positive we have seen. So feel free to ignore. ;)

-Peff