From: Karthik Nayak <karthik.188@gmail.com>
Date: Mon, 01 Sep 2025 13:40:04 GMT
Subject: Re: [PATCH 3/5] refs/reftable: add fsck check for number of tables
Message-ID: <CAOLa=ZTuDtWC9bCQ4h+tvwGXoL7THxV5fLPxi6a1Z73B=7y+4w@mail.gmail.com>
In-Reply-To: <aK3hwQbO3YwdXa3q@ArchLinux>

shejialuo <shejialuo@gmail.com> writes:

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

Since the reftable library is treated as external one. We can't add and
rely on code outside of the library. That's why you'll see some
duplication here and there.

>>  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`.
>

I tried to rewrite it like you suggested, but I think it still makes
sense to keep the error definitions separate. They help provide
localized context. Otherwise, we'd define the error at the start, then
set individual fields later on. This causes some confusion.

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

Post this patch series we'll dive into block and reference checks, which
will be isolated into individual functions.

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