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

[shejialuo <shejialuo@gmail.com>]

On Tue, Aug 19, 2025 at 02:21:02PM +0200, Karthik Nayak wrote:
> 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.

>  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?

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

Thanks,
Jialuo