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

[Patrick Steinhardt <ps@pks.im>]

On Tue, Mar 04, 2025 at 07:15:18PM -0500, Taylor Blau wrote:
> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
> index 58a9b161262..79e1e6fb52b 100644
> --- a/builtin/pack-objects.c
> +++ b/builtin/pack-objects.c
> @@ -1502,8 +1503,60 @@ static int have_duplicate_entry(const struct object_id *oid,
>  	return 1;
>  }
>  
> +static int want_cruft_object_mtime(struct repository *r,
> +				   const struct object_id *oid,
> +				   unsigned flags, uint32_t mtime)
> +{
> +	struct packed_git **cache;
> +
> +	for (cache = kept_pack_cache(r, flags); *cache; cache++) {
> +		struct packed_git *p = *cache;
> +		off_t ofs;
> +		uint32_t candidate_mtime;
> +
> +		ofs = find_pack_entry_one(oid, p);
> +		if (!ofs)
> +			continue;
> +
> +		/*
> +		 * We have a copy of the object 'oid' in a non-cruft
> +		 * pack. We can avoid packing an additional copy
> +		 * regardless of what the existing copy's mtime is since
> +		 * it is outside of a cruft pack.
> +		 */
> +		if (!p->is_cruft)
> +			return 0;
> +
> +		/*
> +		 * If we have a copy of the object 'oid' in a cruft
> +		 * pack, then either read the cruft pack's mtime for
> +		 * that object, or, if that can't be loaded, assume the
> +		 * pack's mtime itself.
> +		 */
> +		if (!load_pack_mtimes(p)) {
> +			uint32_t pos;
> +			if (offset_to_pack_pos(p, ofs, &pos) < 0)
> +				continue;
> +			candidate_mtime = nth_packed_mtime(p, pos);
> +		} else {
> +			candidate_mtime = p->mtime;
> +		}
> +
> +		/*
> +		 * We have a surviving copy of the object in a cruft
> +		 * pack whose mtime is greater than or equal to the one
> +		 * we are considering. We can thus avoid packing an
> +		 * additional copy of that object.
> +		 */
> +		if (mtime <= candidate_mtime)
> +			return 0;
> +	}
> +
> +	return -1;
> +}
> +

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.

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

Patrick