Re: [GSoC PATCH v2 0/7] repack: add --drop-filtered to reclaim space in partial clones

From: Siddharth Shrimali <r.siddharth.shrimali@gmail.com>
Date: Aug 1, 2026, 18:19 UTC
Message-ID: <CAGWgyh8EPSufBZrk0xCqTr4gz6MtJHkfCy6JQKxCqKSPZ3gEgw@mail.gmail.com>
In-Reply-To: <xmqqcxw3dvh5.fsf@gitster.g>

Hi, thanks for the review Junio!

On Fri, 31 Jul 2026 at 21:04, Junio C Hamano <gitster@pobox.com> wrote:

> By 'by construction', do you mean 'It is guaranteed recoverable, as
> long as ODB_FOR_EACH_OBJECT_PROMISOR_ONLY is working correctly'?

yes, that is what I meant. the object is recoverable because it came from a promisor pack (due to a .promisor file), so the remote has promised to give it back. "by construction" means "recoverable as long as the promisor-only walk correctly picks out promisor objects".

> Since I do not use it, I do not personally trust promisor-based
> traversal all that much, and it would be great if we could hear from
> other practitioners that this really works well.

i'll also be glad to hear from people who actually use partial clone about whether leaning on the promisor-only walk here is a good idea for now, until the remote-object-info side of the cat-file protocol lands, which would let us verify against the remote directly.

> This is sensible, as long as this repacking is done only with
> locally available data, without dynamically pulling in lazy objects
> from the promisor (which would defeat the whole point ;-)).

right, i made sure of that :) enumeration passes OBJECT_INFO_SKIP_FETCH_OBJECT on every object-info lookup, so it never triggers a lazy fetch. The rebuild is local too: it only repacks promisor objects that are already present. I confirmed this by tracing a real drop and by moving the promisor remote away entirely before a drop, it still completed, so it clearly did not need the remote

> Presumably, this rebuilding is done without an extra traversal,
> driven instead by the list of enumerated promisor objects we
> constructed above (excluding the unwanted ones)?

not quite, there are two walks right now. First, enumerate_promisor_blobs() walks the promisor objects to figure out what to drop. Then repack_promisor_objects() does its own promisor-only walk to rebuild the pack, skipping anything in that drop set. so the rebuild does use the drop set, but through a second walk, not by directly reusing the first list.

Show 8 quoted lines

> I wonder whether size is the only criterion we would want to use
> when choosing what to discard among objects we know the promisor can
> give us on-demand.  It is, of course, perfectly fine to make it the
> only condition in this first effort, but it would help to imagine
> what other criteria we might want in the future and how they would
> fit into the framework you establish with this series.  Ensuring
> that the framework is easily extensible with a future set of rules
> will keep us from painting ourselves into a corner.

true, i agree.. size (blob:limit) is the only rule for now, but its easy to imagine others: how old an object is or when it was last used, its path, its type, or whether its still reachable from the current branch. The design should handle those without much trouble. Enumeration builds a set of promisor objects, and then one step narrows that set down to what we actually drop. Right now that step is just the blob:limit filter. A new rule would plug in at the same spot, narrowing the same set, so the overall "list them, then pick what to drop" shape would not change

Show 8 quoted lines

> I assume you do not mean a race where an operation wants to write a
> blob, finds that an identical one that came from the promisor remote
> already exists locally, refrains from writing another copy, and the
> drop-filtered operation removes the blob at the right moment.
> Rather, you likely have in mind an operation that stops, gives
> control back to the user, and, while the user ponders the situation,
> the drop-filtered operation kicks in and removes the blobs involved
> in the operation in progress.  Am I reading you correctly?

um yes, the case i had in mind is the second one: an operation stops halfway, hands control back to the user, and drop-filtered runs in that gap and removes blobs the paused operation was using

Show 6 quoted lines

> Even in either of these situations, I do not quite see why the
> safeguards are necessary.  The operation completes, or stays stopped
> in the middle.  The user's next move (whether they issue a new
> command after completion or resume the interrupted operation) will
> automatically lazy-refetch what the drop-filtered operation
> discarded as needed, will it not?

yes, you got that right. Since the objects are promised, whatever gets dropped will just be lazy-refetched when the user runs the next command or resumes the operation. the guards avoid immediately re-downloading something we just dropped, (which we can call as some wasted work : )), and a network fetch in the middle of, say, resolving a merge. The index guard is the same story- the blob it protects would just be re-fetched by the next command anyway. So they are a convenience to avoid pointless re-fetching, not a correctness measure. I am happy to drop them or keep them clearly documented as just that, whichever the list prefers.

Thanks, Siddharth Shrimali