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

[Junio C Hamano <gitster@pobox.com>]

Siddharth Shrimali <r.siddharth.shrimali@gmail.com> writes:

> How it works:
>   * Enumerate promisor objects directly (ODB_FOR_EACH_OBJECT_PROMISOR_ONLY)
>     and select the blobs exceeding the filter threshold. Every enumerated
>     object is a promisor object by construction, so it is guaranteed
>     recoverable and locally-created objects are never candidates.

By 'by construction', do you mean 'It is guaranteed recoverable, as
long as ODB_FOR_EACH_OBJECT_PROMISOR_ONLY is working correctly'?
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.

>   * Rebuild the promisor pack without the selected blobs, reusing the
>     existing repack machinery, so the drop is crash-safe (write, fsync,
>     install, then delete the old pack).

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 ;-)).
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)?

>   * --dry-run lists the candidates and changes nothing.

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.

> Safety guards refuse to run while a merge, rebase, am, cherry-pick,
> revert, or bisect is in progress, and refuse to drop a blob referenced
> by the current index (it would only be lazily re-fetched by the next
> worktree command). Both are skipped for bare repositories.

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?

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?