Junio C Hamano <gitster@xxxxxxxxx> writes: > Karthik Nayak <karthik.188@xxxxxxxxx> 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'. > 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
Attachment:
signature.asc
Description: PGP signature
