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

From: Taylor Blau <me@ttaylorr.com>
Date: Mar 13, 2025, 17:32 UTC
Message-ID: <Z9MWp6+TqwD+LdxE@nand.local>
In-Reply-To: <Z8l5hxNjEOALl_g-@pks.im>

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".

Show 12 quoted lines

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