From: Elijah Newren Date: Tue, 06 Jan 2026 23:19:22 GMT Subject: Re: [PATCH] fsck: snapshot default refs before object walk Message-ID: In-Reply-To: On Mon, Dec 29, 2025 at 4:46 PM Junio C Hamano wrote: > > "Elijah Newren via GitGitGadget" writes: > > > This problem doesn't occur when refs are specified on the command line > > for us to check, since we use those specified refs for both walking and > > checking. Using the same refs for walking and checking seems to just > > make sense, so modify the existing code to do the same when refs aren't > > specified. > > Excellent analysis and good approach. > > > Snapshot the refs at the beginning, and also ignore all > > reflog entries since the time of our snapshot (while this technically > > means we could ignore a reflog entry created before the fsck process > > if the local clock is weird, since reflogs are local-only there are not > > concerns about differences between clocks on different machines). > > Repository on a network filesystem being accessed by hosts with > broken clock? Oh, indeed. > I do not think our reflog API has (1) give me some token to mark > your current state (2) here is the token you gave me earlier, now > iterate and yield entries but ignore entries added after you gave me > that token, so going by the reflog timestamp is probably the best we > could do. Any approach may get confused when the user tries to be > cute and issues "reflog delete" or "reflog expire" in the middle > anyway, I suspect ;-) > > > While worries about live updates while running fsck is likely of most > > interest for forge operators, it will likely also benefit those with > > automated jobs (such as git maintenance) or even casual users who want > > to do other work in their clone while fsck is running. > > Great. Will queue. Thanks. > > > @@ -509,6 +510,9 @@ static int fsck_handle_reflog_ent(const char *refname, > > timestamp_t timestamp, int tz UNUSED, > > const char *message UNUSED, void *cb_data UNUSED) > > { > > + if (now && timestamp > now) > > + return 0; > > + > > if (verbose) > > fprintf_ln(stderr, _("Checking reflog %s->%s"), > > oid_to_hex(ooid), oid_to_hex(noid)); > > @@ -567,14 +571,53 @@ static int fsck_head_link(const char *head_ref_name, > > const char **head_points_at, > > struct object_id *head_oid); > > > > -static void get_default_heads(void) > > +struct ref_snapshot { > > + size_t nr; > > + size_t name_alloc; > > + size_t oid_alloc; > > + char **refname; > > + struct object_id *oid; > > +}; > > This data structure is somewhat unexpected. Instead of a struct > that holds two arrays, I would have rather expected an array of > "struct { refname, oid }", with the possiblity to add a "token to > mark the latest reflog entry" to the mix I alluded to earlier when > such an API function materializes. Yeah, that makes sense. It'll mean that there won't be anything left of Matthew's original patch that I was trying to upstream (especially with the further changes Peff highlighted elsewhere in this thread), but I can just take the authorship and note Matthew's contribution in a trailer. > [Footnote] > > We could call refs_for_each_reflog_ent_reverse(), grab the > parameters that each_reflog_ent_fn receives as that "token" for the > latest reflog entry and stop. That way, we will learn the value of > , which should be a robust > enough unique key. > > After that when iterating over the reflog, we know we should stop > after processing the reflog entry that holds the recorded value. Interesting. The global timestamp for reflogs seems good enough for me (network filesystems with a broken clock feel niche to me), but I can leave a TODO in the code for those that want to pursue improving the reflog handling further.