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