On Thu, Mar 06, 2025 at 11:31:35AM +0100, Patrick Steinhardt wrote: > Minor nit: it is a bit unusual that a negative value, which typically > indicates an error, is used as a boolean value here to indicate that we > don't want to have the object. This matches the convention of the other "do we want this object?" functions, where returning -1 indicates that we don't yet know whether or not we want the object, and should continue looking elsewhere. Returning -1 from 'want_cruft_object_mtime()' doesn't mean that we can necessarily return -1 immediately from its caller in 'want_found_object()' because we might pick it up from further down in that function or in one of its callers. So I think the return values of that function are consistent. Because that function never says "yes, I want this object" and only "no" or "maybe", we could return 0/1 indicating "no" or "maybe". But that feels like a bug waiting to happen if someone later on mistakes "1" (which in this hypothetical would mean "maybe") as "yes". > > diff --git a/t/t7704-repack-cruft.sh b/t/t7704-repack-cruft.sh > > index 959e6e26488..f427150de5b 100755 > > --- a/t/t7704-repack-cruft.sh > > +++ b/t/t7704-repack-cruft.sh > > @@ -304,6 +304,69 @@ test_expect_success '--max-cruft-size with freshened objects (packed)' ' > > ) > > ' > > > > +test_expect_success '--max-cruft-size with freshened objects (previously cruft)' ' > > + git init max-cruft-size-threshold && > > Let's also delete the repository via `test_when_finished`. Eh. The point of naming these uniquely is that we don't have to remember to clean them up afterwords, but I can do so if you want. Thanks, Taylor
