Re: [PATCH 2/4] object-file: refactor corrupt object diagnosis

From: Jeff King <peff@peff.net>
Date: Nov 30, 2022, 20:47 UTC
Message-ID: <Y4fBdW4IMhVXazm4@coredump.intra.peff.net>
In-Reply-To: <a8c5fcd9f860a0434f974779bae6edf6a8ceeaca.1669839849.git.jonathantanmy@google.com>

On Wed, Nov 30, 2022 at 12:30:47PM -0800, Jonathan Tan wrote:

Show 18 quoted lines

> +void die_if_corrupt(struct repository *r,
> +		    const struct object_id *oid,
> +		    const struct object_id *real_oid)
> +{
> +	const struct packed_git *p;
> +	const char *path;
> +	struct stat st;
>  
>  	obj_read_lock();
>  	if (errno && errno != ENOENT)
>  		die_errno(_("failed to read object %s"), oid_to_hex(oid));
>  
>  	/* die if we replaced an object with one that does not exist */
> -	if (repl != oid)
> +	if (real_oid != oid)
>  		die(_("replacement %s not found for %s"),
> -		    oid_to_hex(repl), oid_to_hex(oid));
> +		    oid_to_hex(real_oid), oid_to_hex(oid));

This kind of pointer comparison is a little subtle. Within a single function, as this code was before this patch, it's probably OK to assume that we use pointer indirection, and a non-replaced object will use the original pointer. But for a public function, it seems like a gotcha that:

  oidcpy(&real_oid, lookup_replace_object(r, &oid));
  die_if_corrupt(r, &oid, &real_oid);

would produce the wrong answer (it would think replacement happened even if it didn't).

So maybe:

  if (!oideq(real_oid, oid))

instead? It's a little slower, but the point of this is to diagnose and die, so it's not exactly a hot path. :)

-Peff