Re: [PATCH 3/5] refs/reftable: add fsck check for number of tables

From: shejialuo <shejialuo@gmail.com>
Date: Aug 26, 2025, 16:33 UTC
Message-ID: <aK3hwQbO3YwdXa3q@ArchLinux>
In-Reply-To: <20250819-228-reftable-introduce-consistency-checks-v1-3-8b8f6879fa9e@gmail.com>

On Tue, Aug 19, 2025 at 02:21:02PM +0200, Karthik Nayak wrote:

Show 30 quoted lines

> diff --git a/reftable/fsck.c b/reftable/fsck.c
> index 22ec3c26e9..e92a630276 100644
> --- a/reftable/fsck.c
> +++ b/reftable/fsck.c
> @@ -2,6 +2,28 @@
>  #include "reftable-fsck.h"
>  #include "stack.h"
>  
> +static int reftable_fsck_valid_stack_count(struct reftable_stack *st)
> +{
> +	DIR *dir = opendir(st->reftable_dir);
> +	struct dirent *d = NULL;
> +	unsigned int count = 0;
> +
> +	if (!dir)
> +		return 0;
> +
> +	while ((d = readdir(dir))) {
> +		if (!strcmp(d->d_name, "tables.list"))
> +			continue;
> +
> +		if (d->d_type == DT_REG)
> +			count++;
> +	}
> +
> +	closedir(dir);
> +
> +	return count == st->tables_len;
> +}
> +

The above logic is clear to understand but I think we should our internal interface in "dir-iterator.h" to implement above logic.

Show 16 quoted lines

>  int reftable_fsck_check(struct reftable_stack *stack,
>  			reftable_fsck_report_fn report_fn,
>  			reftable_fsck_verbose_fn verbose_fn,
> @@ -44,6 +66,18 @@ int reftable_fsck_check(struct reftable_stack *stack,
>  		}
>  	}
>  
> +	verbose_fn("Checking reftable tables count", cb_data);
> +
> +	if (!reftable_fsck_valid_stack_count(stack)) {
> +		struct reftable_fsck_info info = {
> +			.error = REFTABLE_FSCK_ERROR_STACK_COUNT,
> +			.path = stack->list_file,
> +			.msg = "mismatch in number of tables"
> +		};
> +

When reading here, I somehow understand the reason why you define this data structure in the loop. But I still think we could just define only one `info`.

BTY, I wonder whether we should define some auxiliary functions for each check instead of adding logic directly in `reftable_fsck_check` function?

Show 6 quoted lines

> +		err = report_fn(info, cb_data);
> +	}
> +
>  out:
>  	free_names(names);
>  	return err;

Thanks, Jialuo