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

From: Karthik Nayak <karthik.188@gmail.com>
Date: Sep 1, 2025, 13:40 UTC
Message-ID: <CAOLa=ZTuDtWC9bCQ4h+tvwGXoL7THxV5fLPxi6a1Z73B=7y+4w@mail.gmail.com>
In-Reply-To: <aK3hwQbO3YwdXa3q@ArchLinux>

shejialuo <shejialuo@gmail.com> writes:

Show 35 quoted lines

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

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

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.

Show 5 quoted lines

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

Show 9 quoted lines

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