Re: [PATCH v2 2/5] refs/reftable: add fsck check for checking the table name

From: Karthik Nayak <karthik.188@gmail.com>
Date: Sep 9, 2025, 13:49 UTC
Message-ID: <CAOLa=ZRzEY7VTcRTLz1u6=A5Mz0XFOhqNWK4s_wr1WOP==B1rg@mail.gmail.com>
In-Reply-To: <aLhx-HdIMCB3c2WK@ArchLinux>

shejialuo <shejialuo@gmail.com> writes:

Show 53 quoted lines

> On Wed, Sep 03, 2025 at 10:07:13AM +0200, Patrick Steinhardt wrote:
>
> [snip]
>
>> > +static int reftable_be_fsck(struct ref_store *ref_store, struct fsck_options *o,
>> >  			    struct worktree *wt UNUSED)
>> >  {
>> > -	return 0;
>> > +	struct reftable_ref_store *refs;
>> > +	struct strmap_entry *entry;
>> > +	struct hashmap_iter iter;
>> > +	int ret = 0;
>> > +
>> > +	refs = reftable_be_downcast(ref_store, REF_STORE_READ, "fsck");
>> > +
>> > +	if (o->verbose)
>> > +		fprintf_ln(stderr, _("Checking references consistency"));
>>
>> This line is duplicate across both backends, right? Maybe it's something
>> that we can do in the generic logic?
>>
>
> That's right, it is duplicate. If we want to remove this, we need to do
> this in the "builtin/refs.c". But I wonder whether we should do this in
> the first place. Should we rather add more detailed information just
> like the following code for packed backend?
>
>     if (o->verbose)
>         fprintf_ln(stderr, "Checking packed-refs file %s", refs->path);
>
> Instead of just using
>
>     Checking references consistency
>
> Could we use
>
>     Checking reftable references consistency
>
> However, I also feel strange about above, :)
>
> [snip]
>
>> > +/* Represents an individual error encountered during the FSCK checks. */
>> > +struct reftable_fsck_info {
>> > +	enum reftable_fsck_error error;
>> > +	const char *msg;
>> > +	const char *path;
>> > +};
>>
>> I wonder whether it should be the reftable library that decides on the
>> severity of each generated finding.
>>
>

I think I did rush while agreeing to do this change and didn't realize the complexity of it.

Show 6 quoted lines

> That's an interesting question. Let's inspect how Git handles the
> severity. When defining the fsck message id, we need to specify its
> severity like the following shows, this happens at compile time:
>
>     FUNC(BAD_REFERENT_NAME, ERROR)
>

This is used to create the enum of all values, but there is a complimentary structure `msg_id_info` which holds the mapping for each message id to its error category.

Both of these could be extended at compile time by including the errors from the reftable header. But to do this in a backend agnostic way, we'd have to receive and re-expose it via `refs.h`.

Show 12 quoted lines

> And we could set the configuration "fsck.[message id]=" to change the
> fsck message severity.
>
> Then let's think if reftable library decides the severity. It means that
> we need to use the API from reftable library to update
> "fsck_option->msg_type" at the runtime. And it is bad because the fsck
> infrastructure would be highly coupled with the reftable library.
>
> So, I don't think it's a good idea for reftable library to choose the
> severity. Instead, reftable library should just provide users with error
> types and let the users decide the severity.
>

So while there are ways to do it, it won't be simple/elegant and I'm not sure it'd be worth it.

> Thanks,
> Jialuo