From: Karthik Nayak <karthik.188@gmail.com>
Date: Tue, 07 Oct 2025 08:45:21 GMT
Subject: Re: [PATCH v5 7/7] refs/reftable: add fsck check for checking the table name
Message-ID: <CAOLa=ZSGsfhUM+cn0XGDJnFHLswxYqSOePPk+LXK0g3cYjaXfA@mail.gmail.com>
In-Reply-To: <20251007023242.GA2747748@coredump.intra.peff.net>

Jeff King <peff@peff.net> writes:

> On Mon, Oct 06, 2025 at 04:23:05PM +0200, Karthik Nayak wrote:
>
>> +test_expect_success "no errors reported on a well formed repository" '
>> +	test_when_finished "rm -rf repo" &&
>> +	git init repo &&
>> +	(
>> +		cd repo &&
>> +		git commit --allow-empty -m initial &&
>> +
>> +		for i in $(test_seq 20)
>> +		do
>> +			git update-ref branch-$i HEAD || return 1
>> +		done &&
>
> Did you mean refs/heads/branch-$i here? As it is written, it creates a
> root ref, and the name does not conform to the usual rules (all-caps,
> and ending in _HEAD). There are some holes in our checks, which is why
> it doesn't barf yet, but I have a series to fix that which I hope to
> send out later this week.
>

Yeah, this was definitely a miss on my side. It works because currently
we haven't yet added reference level checks to reftables.

This series only adds stack/table level checks.

>> +		# The repository should end up with multiple tables.
>> +		test_line_count ">" 1 .git/reftable/tables.list &&
>> +
>> +		git refs verify 2>err &&
>> +		test_must_be_empty err
>> +	)
>
> Arguably this verify command should be complaining about the broken
> names, too.
>

Yes, eventually it will when we implement reference level checks. Since
that's missing, it currently doesn't barf.

It does work as-is and we could leave it at that, until we actually
implement the reference level checks. But I think a quick re-roll will
avoid future confusion.

> -Peff

Thanks for the review. Looking forward to your series.