From: Siddharth Shrimali <r.siddharth.shrimali@gmail.com>
Date: Fri, 04 Sep 2026 10:51:33 GMT
Subject: Re: [GSoC PATCH v5 6/6] builtin/repack: add guards for --drop-filtered
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:
> > +                     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