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

From: Taylor Blau <me@ttaylorr.com>
Date: Feb 21, 2022, 03:11 UTC
Message-ID: <YhMC+3FdSEZz22qX@nand.local>
In-Reply-To: <CB2ACEF7-76A9-4253-AD43-7BC842F9576D@gmail.com>

On Wed, Feb 16, 2022 at 04:07:14PM -0500, John Cai wrote:

Show 16 quoted lines

> > 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