From: Patrick Steinhardt <ps@pks.im>
Date: Thu, 25 Sep 2025 06:14:27 GMT
Subject: Re: [PATCH v3 7/8] reftable: add code to facilitate consistency checks
Message-ID: <aNTdwzUMlubjcppb@pks.im>
In-Reply-To: <CAOLa=ZQ641MncC9ACm9jfjx0WtQ+nK2shtyucQOxd08LDXDzAw@mail.gmail.com>

On Wed, Sep 24, 2025 at 11:40:31AM -0700, Karthik Nayak wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> > On Thu, Sep 18, 2025 at 10:11:48AM +0200, Karthik Nayak wrote:
> >> diff --git a/reftable/fsck.c b/reftable/fsck.c
> >> new file mode 100644
> >> index 0000000000..785e4b43e8
> >> --- /dev/null
> >> +++ b/reftable/fsck.c
[snip]
> >> +	struct reftable_fsck_info info;
> >> +	struct dirent *d = NULL;
> >> +	uint64_t min, max;
> >> +	int err = 0;
> >> +
> >> +	if (!dir)
> >> +		return 0;
> >> +
> >> +	while ((d = readdir(dir))) {
> >> +		if (!strcmp(d->d_name, "tables.list"))
> >> +			continue;
> >> +
> >> +		if ((d->d_name[0] == '.' &&
> >> +		     (d->d_name[1] == '\0' ||
> >> +		      (d->d_name[1] == '.' && d->d_name[2] == '\0'))))
> >> +			continue;
> >> +
> >> +		if (d->d_type == DT_REG) {
> >> +			if (!valid_table_name(d->d_name, &min, &max)) {
> >> +				info.error = REFTABLE_FSCK_ERROR_TABLE_NAME;
> >> +				info.msg = "file with invalid table name";
> >> +				info.path = d->d_name;
> >> +
> >> +				err |= report_fn(&info, cb_data);
> >> +			}
> >
> > One problem with this is that this is racy with concurrent writers. We
> > don't recognize the "tables.list.lock" file, and neither do we recognize
> > "0x*-0x*.{ref,log}.temp.XXXXXX"-style files.
> >
> > Would it be a better approach be to instead go through table names as
> > loaded by the stack? The reftable code already knows to prune unknown
> > files anyway, so I don't think we should scan for any other files.
> >
> 
> I actually had a more structured code here, where the idea was:
> 
> - For each stack
>   - Run stack level checks
>   - For each table in stack
>     - Run table level checks
>     - For each block in table
>       - Run block level checks
>       - For each ref / log
>         - Run ref / log level checks
> 
> But we move some of my tests to be runtime checks, leaving this as the
> only check remaining. We could still do the first level of what I
> mentioned above. The only reason I didn't was because we wanted to check
> all files in the stack dir. But I think this is much better, having
> unknown files in the reftable directory doesn't affect the repository in
> any way. So I would argue perhaps that we shouldn't even care about it.

Yeah, agreed. As long as we don't know about any edge cases where this
does or did create problems I agree.

Patrick