Re: [PATCH v3 1/1] builtin/pack-objects.c: freshen objects from existing cruft packs
- From
Patrick Steinhardt <ps@pks.im>
- Date
- Mar 6, 2025, 10:31 UTC
- Message-ID
- <Z8l5hxNjEOALl_g-@pks.im>
- In-Reply-To
- <6e93471f9a8e6a3dde36342088748ba17e4f7f95.1741133712.git.me@ttaylorr.com>
On Tue, Mar 04, 2025 at 07:15:18PM -0500, Taylor Blau wrote:
Show 60 quoted lines
> 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.
Show 10 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`.
Patrick