Re: [PATCH v3] fsck: snapshot default refs before object walk
- From
Jeff King <peff@peff.net>
- Date
- Jan 14, 2026, 22:34 UTC
- Message-ID
- <20260114223451.GB1014423@coredump.intra.peff.net>
- In-Reply-To
- <pull.2026.v3.git.1767980953134.gitgitgadget@gmail.com>
On Fri, Jan 09, 2026 at 05:49:13PM +0000, Elijah Newren via GitGitGadget wrote:
Show 6 quoted lines
> -static int fsck_handle_ref(const struct reference *ref, void *cb_data UNUSED)
> +struct ref_snapshot {
> + char *refname;
> + struct object_id oid;
> + /* TODO: Maybe supplement with latest reflog entry info too? */
> +};I think this is an OK place to stop for now, but just a few thoughts for a possible future (that I hope neither of us ever needs to follow up on ;) ).
I wonder if we need to record individual reflog entries at all, or if it would be sufficient to just keep an oidset of reachable tips. In fact, I wondered if we even needed these ref_snapshot structs at all, and couldn't just get away with an oidset. But I guess if we do find that something reachable is missing, we want some better way of pointing to the culprit.
It _might_ be possible to keep a more compact representation like an oidset for the happy path, and then only if we find that something is unreachable, go back and find the culprit. But maybe that gets too complicated and/or racy.
I also wondered if we could just instantiate a "struct object" for each ref tip, and marking it with a reachable flag. That's even _more_ expensive than the object_id you're storing here, but eventually becomes cheaper, since we're going to instantiate all of those objects as we traverse. IIRC fsck traditionally relied on the existence of an object struct as a signal that we saw the object somewhere, but I don't recall if that is still true (a long time ago, I think I tried to convert that into an explicit flag to prevent subtle confusion, but I don't remember how completely I succeeded).
Anyway, all of that is for another time.
Show 8 quoted lines
> +static int fsck_handle_ref(const struct reference *ref, void *cb_data UNUSED)
> +{
> + struct object *obj;
> +
> + obj = parse_object(the_repository, ref->oid);
> obj->flags |= USED;
> fsck_put_object_name(&fsck_walk_options,
> ref->oid, "%s", ref->name);I wonder if this parse_object() can ever fail (say, in a corrupted repo) and we'd segfault here? I wouldn't be surprised if that is already a possibility before your patch, either. ;)
> [...]
I gave a fairly quick read to the rest of it but didn't see anything questionable.
-Peff