Re: [GSoC PATCH v5 6/6] builtin/repack: add guards for --drop-filtered

Hi Samuel,
Thanks for the review and your RFC!

I have a few suggestions:

On Fri, 4 Sept 2026 at 03:24, Samuel Bronson [off-list ref] wrote:

quoted

+                     for (i = 0; i < istate->cache_nr; i++) {
+                             const struct cache_entry *ce = istate->cache[i];
+
+                             if (oidset_contains(&drop_oids, &ce->oid))
+                                     die(_("cannot drop '%s' (%s): it is referenced by the current index"),
+                                             ce->name, oid_to_hex(&ce->oid));

The bad news: dying at this time is *not* convenient, especially after
we've finished that *entire* enumerate_promisor_blobs(), (which is kind
of slow for a step with no progress output, btw).

Thats actually a very good point :)
I agree with this: aborting the whole operation because a single blob
is referenced by the index is a poor trade-off, since it happens only
after the full enumerate_promisor_blobs() walk has already
run.

While I do want to keep the index blobs, I do *not* want to cancel the
whole operation over them.

one caveat: oidset_remove() mutates drop_oids in place, and the
--dry-run printer iterates drop_oids afterwards. So with this change,
--dry-run would stop listing the index-referenced blobs, when it should
still report them as candidates it would skip. Instead, we can collect the index
OIDs into a separate 'skip-set' and have both the dry-run output and the
real drop consult that, rather than removing from drop_oids directly.

As a follow-up note, the planned drop-log work will need to account
for this: a blob skipped here was never dropped, so it must not be
recorded there.

The following seems much more convenient:

-- >8 --
Subject: [RFC] builtin/repack: just don't --drop-filtered index blobs

Instead of dying when we would drop a blob referenced by the index, just
... don't drop it. (Retain the explanatory message as a warning.)

This allows `git repack -a --filter=blob:limit=0 --drop-filtered` to
work in non-bare repositories that have non-trivial files around.

Not done:

  - Fixing the tests to match

  - Allowing `--filter=blob:none`

Thanks,
Siddharth Shrimali