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

[Jeff King <peff@peff.net>]

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

> +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