Re: [GSoC PATCH v4 4/7] builtin/repack: enumerate promisor blobs for --drop-filtered

From: Christian Couder <christian.couder@gmail.com>
Date: Aug 12, 2026, 17:21 UTC
Message-ID: <CAP8UFD3fF+Ka0Sqs3BSstur_HdqU3s==3ash91Ewwi_F-t2oDA@mail.gmail.com>
In-Reply-To: <20260810174047.6524-5-r.siddharth.shrimali@gmail.com>

On Mon, Aug 10, 2026 at 7:41 PM Siddharth Shrimali <r.siddharth.shrimali@gmail.com> wrote:

Show 21 quoted lines

> +int enumerate_promisor_blobs(struct repository *repo,
> +                            const struct list_objects_filter_options *filter,
> +                            struct oidset *to_drop)
> +{
> +       struct oidset all_promisor_blobs = OIDSET_INIT;
> +       struct collect_cb_data cb = {
> +               .repo = repo,
> +               .set = &all_promisor_blobs
> +       };
> +       int ret = 0;
> +
> +       /*
> +        * The caller (cmd_repack) is responsible for validating that a
> +        * blob:limit filter and a promisor remote are present before
> +        * calling this function.
> +        *
> +        * Walk only promisor objects. every object visited here is a
> +        * promisor object, so it is recoverable from the promisor remote
> +        * as long as the remote still has it, the same assumption the rest
> +        * of partial clone relies on
> +

The sentence that ends with "of partial clone relies on" should be finished with a full stop. And the next line should have a "*" as it is part of a code comment.

Show 24 quoted lines

> +        * We do not use write_filtered_pack() here because git repack
> +        * routes promisor objects through repack_promisor_objects()
> +        * before the filter machinery runs, so the filtered pack never
> +        * contains promisor blobs. Direct enumeration via
> +        * ODB_FOR_EACH_OBJECT_PROMISOR_ONLY is the correct approach.
> +        */
> +       ret = odb_for_each_object(repo->objects, NULL,
> +                       collect_promisor_blob, &cb,
> +                       ODB_FOR_EACH_OBJECT_PROMISOR_ONLY);
> +       if (ret)
> +               goto cleanup;
> +
> +       /*
> +        * Apply the filter to find which blobs exceed the threshold.
> +        */
> +       ret = list_objects_filter__filter_oidset(repo,
> +               (struct list_objects_filter_options *)filter,
> +               &all_promisor_blobs,
> +               to_drop);
> +
> +cleanup:
> +       oidset_clear(&all_promisor_blobs);
> +       return ret;
> +}

[...]

Show 22 quoted lines

> diff --git a/t/t7706-repack-drop-filtered.sh b/t/t7706-repack-drop-filtered.sh
> index f27b09a30e..453053cc18 100755
> --- a/t/t7706-repack-drop-filtered.sh
> +++ b/t/t7706-repack-drop-filtered.sh
> @@ -1,9 +1,36 @@
>  #!/bin/sh
>
> -test_description='git repack --drop-filtered option validation'
> +test_description='git repack --drop-filtered enumerates filtered promisor blobs'
>
>  . ./test-lib.sh
>
> +delete_object () {
> +       local repo="$1" &&
> +       local obj="$2" &&
> +       local path="$repo/.git/objects/$(test_oid_to_path "$obj")" &&
> +       rm "$path"
> +}
> +
> +# pack the objects into a promisor pack inside "repo". it is a pack
> +# accompanied by an empty ".promisor" marker file. objects
> +# in such a pack are treated as recoverable from the promisor remote.

Here and in other places in this and other patches, code comments would read better if they were proper sentences starting with an uppercase letter (and ending with a full stop). Like:

# Pack the objects into a promisor pack inside "repo". It is a pack # accompanied by an empty ".promisor" marker file. Objects # in such a pack are treated as recoverable from the promisor remote.

Show 5 quoted lines

> +pack_as_from_promisor () {
> +       HASH=$(git -C repo pack-objects .git/objects/pack/pack) &&
> +       >repo/.git/objects/pack/pack-$HASH.promisor &&
> +       echo $HASH
> +}