From: Patrick Steinhardt <ps@pks.im>
Date: Wed, 24 Sep 2025 05:54:55 GMT
Subject: Re: [PATCH v3 8/8] refs/reftable: add fsck check for checking the table name
Message-ID: <aNOHr7lm4WXUyHa5@pks.im>
In-Reply-To: <20250918-228-reftable-introduce-consistency-checks-v3-8-271af03eb34d@gmail.com>

On Thu, Sep 18, 2025 at 10:11:49AM +0200, Karthik Nayak wrote:
> diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c
> index 2152349cb9..1a18f4bf92 100644
> --- a/refs/reftable-backend.c
> +++ b/refs/reftable-backend.c
> @@ -2707,11 +2709,57 @@ static int reftable_be_reflog_expire(struct ref_store *ref_store,
>  	return ret;
>  }
>  
> -static int reftable_be_fsck(struct ref_store *ref_store UNUSED,
> -			    struct fsck_options *o UNUSED,
> +static void reftable_fsck_verbose_handler(const char *msg, void *cb_data)
> +{
> +	struct fsck_options *o = cb_data;
> +
> +	if (o->verbose)
> +		fprintf_ln(stderr, "%s", msg);
> +}
> +
> +static const enum fsck_msg_id fsck_msg_id_map[] = {
> +	[REFTABLE_FSCK_ERROR_INVALID_FILE_TYPE] = FSCK_MSG_BAD_REFTABLE_FILETYPE,
> +	[REFTABLE_FSCK_ERROR_TABLE_NAME] = FSCK_MSG_BAD_REFTABLE_TABLE_NAME,
> +};
> +
> +static int reftable_fsck_error_handler(struct reftable_fsck_info *info,
> +				       void *cb_data)
> +{
> +	struct fsck_ref_report report = { .path = info->path };
> +	struct fsck_options *o = cb_data;
> +	enum fsck_msg_id msg_id;
> +
> +	if (info->error < 0 || info->error >= REFTABLE_FSCK_MAX_VALUE)
> +		BUG("unknown fsck error: %d", info->error);

`info->error` is an enum, and whether or not it is signed is an
implementation detail of the platform. But I wonder whether this check
may cause some platforms to warn about an impossible condition.

> +
> +	msg_id = fsck_msg_id_map[info->error];
> +
> +	if (!msg_id)
> +		BUG("fsck_msg_id value missing for reftable error: %d", info->error);

Yup, makes sense.

Patrick