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
