From: Karthik Nayak <karthik.188@gmail.com>
Date: Mon, 06 Oct 2025 12:05:25 GMT
Subject: Re: [PATCH v4 7/7] refs/reftable: add fsck check for checking the table name
Message-ID: <CAOLa=ZRU01MQCLwWx4zEZNRtqNBvUJnMo8euE+J86q2a9adSwA@mail.gmail.com>
In-Reply-To: <aN5lrhCLQFnw7qUB@pks.im>

Patrick Steinhardt <ps@pks.im> writes:

> On Fri, Sep 26, 2025 at 09:25:50AM +0200, Karthik Nayak wrote:
>> diff --git a/t/t0614-reftable-fsck.sh b/t/t0614-reftable-fsck.sh
>> new file mode 100755
>> index 0000000000..250d244e66
>> --- /dev/null
>> +++ b/t/t0614-reftable-fsck.sh
>> @@ -0,0 +1,38 @@
>> +#!/bin/sh
>> +
>> +test_description='Test reftable backend consistency check'
>> +
>> +GIT_TEST_DEFAULT_REF_FORMAT=reftable
>> +export GIT_TEST_DEFAULT_REF_FORMAT
>> +
>> +. ./test-lib.sh
>> +
>> +for TABLE_NAME in "foo-bar-e4d12d59.ref" \
>> +	"0x00000000zzzz-0x00000000zzzz-e4d12d59.ref" \
>> +	"0x000000000001-0x000000000002-e4d12d59.abc" \
>> +	"0x000000000001-0x000000000002-e4d12d59.refabc"; do
>> +	test_expect_success "table name $TABLE_NAME should be checked" '
>> +		test_when_finished "rm -rf repo" &&
>> +		git init repo &&
>> +		(
>> +			cd repo &&
>> +			git commit --allow-empty -m initial &&
>> +
>> +			git refs verify 2>err &&
>> +			test_must_be_empty err &&
>> +
>> +			EXISTING_TABLE=$(head -n1 .git/reftable/tables.list) &&
>> +			mv ".git/reftable/$EXISTING_TABLE" ".git/reftable/$TABLE_NAME" &&
>> +			sed "s/${EXISTING_TABLE}/${TABLE_NAME}/g" .git/reftable/tables.list > tables.list &&
>> +			mv tables.list .git/reftable/tables.list &&
>> +
>> +			git refs verify 2>err &&
>> +			cat >expect <<-EOF &&
>> +			warning: ${TABLE_NAME}: badReftableTableName: invalid reftable table name
>> +			EOF
>> +			test_cmp expect err
>> +		)
>> +	'
>> +done
>> +
>> +test_done
>
> Nit: we don't have any test that verifies that `git refs verify` doesn't
> complain with a well-formed stack.

The above test does run `git refs verify` on the repository before
modifying the 'tables.list' file. Do you mean a stack with > 1 tables? I
think that would be worthwhile. Let me do that.

>
> Other than that this series looks good to me, thanks! I think we might
> want to have one final reroll, but once that's out I think this should
> be ready to be merged down.
>
> Patrick

Really appreciate the quick and thorough reviews.

Thanks,
Karthik