Re: [PATCH v2 0/4] [RFC] repack: add --filter=

[Taylor Blau <me@ttaylorr.com>]

On Wed, Feb 16, 2022 at 04:07:14PM -0500, John Cai wrote:
> > I don't know whether that is just around naming (--delete-filter /
> > --drop-filter /
> > --expire-filter ?), and/or making the documentation very explicit that
> > this isn't so
> > much "omitting certain objects from a packfile" as irretrievably
> > deleting objects.
>
> Yeah, making the name very clear (I kind of like --delete-filter) would certainly help.
> Also, to have more protection we can either
>
> 1. add a config value that needs to be set to true for repack to remove
> objects (repack.allowDestroyFilter).
>
> 2. --filter is dry-run by default and prints out objects that would have been removed,
> and it has to be combined with another flag --destroy in order for it to actually remove
> objects from the odb.

I share the same concern as Robert and Stolee do. But I think this issue
goes deeper than just naming.

Even if we called this `git repack --delete-filter` and only ran it with
`--i-know-what-im-doing` flag, we would still be leaving repository
corruption on the table, just making it marginally more difficult to
achieve.

I'm not familiar enough with the proposal to comment authoritatively,
but it seems like we should be verifying that there is a promisor remote
which promises any objects that we are about to filter out of the
repository.

I think that this is basically what `pack-objects`'s
`--missing=allow-promisor` does, though I don't think that's the right
tool for this job, either. Because we pack-objects also knows the object
filter, by the time we are ready to construct a pack, we're traversing
the filtered list of objects.

So we don't even bother to call show_object (or, in this case,
builtin/pack-objects.c::show_objecT__ma_allow_promisor) on them.

So I wonder what your thoughts are on having pack-objects only allow an
object to get "filtered out" if a copy of it is promised by some
promisor remote. Alternatively, and perhaps a more straight-forward
option might be to have `git repack` look at any objects that exist in a
pack we're about to delete, but don't exist in any of the packs we are
going to leave around, and make sure that any of those objects are
either unreachable or exist on a promisor remote.

But as it stands right now, I worry that this feature is too easily
misused and could result in unintended repository corruption.

I think verifying that that any objects we're about to delete exist
somewhere should make this safer to use, though even then, I think we're
still open to a TOCTOU race whereby the promisor has the objects when
we're about to delete them (convincing Git that deleting those objects
is OK to do) but gets rid of them after objects have been deleted from
the local copy (leaving no copies of the object around).

So, I don't know exactly what the right path forward is. But I'm curious
to get your thoughts on the above.

Thanks,
Taylor