On Thu, May 15, 2025 at 04:07:28PM +0200, Karthik Nayak wrote:
> +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
