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

[Christian Couder <christian.couder@gmail.com>]

On Mon, Feb 21, 2022 at 4:11 AM Taylor Blau <me@ttaylorr.com> wrote:
>
> 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.

I am ok with making the name and doc very clear that it deletes
objects and they might be lost if they haven't been saved elsewhere
first.

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

I don't think it's of much value. We don't have such config values for
other possibly destructive operations.

> > 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 am not sure it's of much value either compared to naming it
--filter-destroy. It's likely to just make things more difficult for
users to understand.

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

My opinion on this is that the promisor object mechanism assumes by
design that some objects are outside a repo, and that this repo
shouldn't care much about these objects possibly being corrupted.

It's the same for git LFS. As only a pointer file is stored in Git and
the real file is stored elsewhere, the Git repo doesn't care by design
about possible corruption of the real file.

I am not against a name and some docs that strongly state that users
should be very careful when using such a command, but otherwise I
think such a command is perfectly ok. We have other commands that by
design could lead to some objects or data being lost.

> 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 it could be a follow up mode that could be useful and safe,
but there should be no requirement for such a mode. In some cases you
know very much what you want and you don't want checks. For example if
you have taken proper care to transfer large objects to another
remote, you might just not need other possibly expansive checks.

[...]

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

Are you worrying about the UI or about what it does?

I am ok with improving the UI, but I think what it does is reasonable.

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

Possible TOCTOU races are a good reason why something with no check is
perhaps a better goal for now.