Re: [PATCH] fsck: snapshot default refs before object walk

[Junio C Hamano <gitster@pobox.com>]

"Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> 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?

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.


[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
<old,new,committer,timestamp,tz,msg>, 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.