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

From: shejialuo <shejialuo@gmail.com>
Date: Sep 3, 2025, 16:51 UTC
Message-ID: <aLhx-HdIMCB3c2WK@ArchLinux>
In-Reply-To: <aLf3MaKHZSQfnBlT@pks.im>

On Wed, Sep 03, 2025 at 10:07:13AM +0200, Patrick Steinhardt wrote:

[snip]

Show 17 quoted lines

> > +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]

Show 10 quoted lines

> > +/* 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.
>

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)

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.

Thanks, Jialuo