Re: [GSoC PATCH v5 6/6] builtin/repack: add guards for --drop-filtered
- From
Siddharth Shrimali <r.siddharth.shrimali@gmail.com>
- Date
- Sep 4, 2026, 10:51 UTC
- Message-ID
- <CAGWgyh9B=re06aofii9VFB1xOwEeTtxYE=7T14m9WFAx1ORpMg@mail.gmail.com>
- In-Reply-To
- <s0vqzjavw8p.fsf@gmail.com>
Hi Samuel, Thanks for the review and your RFC!
I have a few suggestions:
On Fri, 4 Sept 2026 at 03:24, Samuel Bronson <naesten@gmail.com> wrote:
Show 11 quoted lines
> > + 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.
Show 16 quoted lines
> 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