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

From: karthik nayak <karthik.188@gmail.com>
Date: Dec 7, 2024, 09:54 UTC
Message-ID: <CAOLa=ZRYDqhAPMiuAS-HGaC1huiUGgLW3hL_dDNwrFA5c8ewEQ@mail.gmail.com>
In-Reply-To: <xmqqwmgcv23b.fsf@gitster.g>

Junio C Hamano <gitster@pobox.com> writes:

Show 20 quoted lines

> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> 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".

I was reading the reftable code and noticed it. So mostly luck. Agreed with what you're saying, I'd say this is mostly a 'safeguard'.

Show 5 quoted lines

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

I'm confused, I did add a _new_ test in this patch to do exactly what you're suggesting.

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

Totally, these kind of specific changes are perfect for unit-tests. Plus it was very simple to add one too.

> Will queue.  Thanks.

Thanks!

Karthik