Re: [PATCH v2] reftable/writer: ensure valid range for log's update_index

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 6, 2024, 23:11 UTC
Message-ID: <xmqqwmgcv23b.fsf@gitster.g>
In-Reply-To: <20241206-424-reftable-writer-add-check-for-limits-v2-1-82ca350b10be@gmail.com>

Karthik Nayak <karthik.188@gmail.com> writes:

Show 12 quoted lines

> The corresponding check for reflogs in `reftable_writer_add_log` is
> however missing. Add a similar check, but only check for the upper
> limit. This is because reflogs are treated a bit differently than refs.
> Each reflog entry in reftable has an associated update_index and we also
> allow expiring entries in the middle, which is done by simply writing a
> new reflog entry with the same update_index. This means, writing reflog
> entries with update_index lesser than the writer's update_index is an
> expected scenario.
>
> Add a new unit test to check for the limits and fix some of the existing
> tests, which were setting arbitrary values for the update_index by
> ensuring they stay within the now checked limits.

Interesting. I am a little curious how this was found. As long as the other parts of the system (i.e. the callers) are not buggy, the update pointer will stay within the range, and that is why I do not think I can write an end-to-end test to trigger an existing bug that would be caught by this "fix". Fixing the existing unit tests that feed a wrong input and expect some right outcome is good, but would it be a good to also have a new unit test that validates that such an incorrect precondition for calling is caught and the caller gets the expected REFTABLE_API_ERROR as a result, I wonder? Being able to trigger a condition that is harder to do with the end-to-end test is one good thing about having unit test framework, no?

Will queue.  Thanks.