Re: [PATCH v3 1/1] builtin/pack-objects.c: freshen objects from existing cruft packs

[Taylor Blau <me@ttaylorr.com>]

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