{"thread":{"id":"59994","subject":"Teach git-fetch(1) to use a quarantine directory","startedAt":"2023-07-17T11:20:21Z","lastAt":"2023-07-19T07:24:55Z","messageCount":5,"participants":["Toon Claes","Taylor Blau","Jonathan Tan","Junio C Hamano","Patrick Steinhardt"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"479572","messageId":"87edl6bx7o.fsf@iotcl.com","threadId":"59994","inReplyTo":null,"subject":"Teach git-fetch(1) to use a quarantine directory","fromName":"Toon Claes","fromEmail":"toon@iotcl.com","sentAt":"2023-07-17T10:48:17Z","receivedAt":"2023-07-17T11:20:21Z","isPatch":false,"sender":{"key":"toon@iotcl.com","avatar":"https://avatars.githubusercontent.com/u/121621?v=4"},"body":"Hi,\n\nI've been looking into making git-fetch(1) to use a quarantine\ndirectory, but I'm a bit stuck on direction.\n\nI took git-receive-pack(1) as an example how it uses a quarantine\ndirectory. It seems it sets the environment variables\n$GIT_OBJECT_DIRECTORY and $GIT_ALTERNATE_OBJECT_DIRECTORIES so the real\nobject db is used as an alternative, and a temporary is set as the\ndefault. Then a sub-process is spawned to uses these. In case of\ngit-receive-pack(1), it calls git-unpack-objects(1).\n\nAt the moment git-fetch(1) does not spawn any similar subprocess, so if\nwe want to take the same approach to use the quarantine, we'll need to\nsplit up that command.\n\nBut then we run into another problem as well. git-fetch(1) updates\nreferences, and that is something that's not allowed when using a tmp\nobject directory.\n\nAs far as I can tell from the code, fetching packs and updating refs is\nheavily intertwined, so I'm not sure this approach is the best way\nforward. So a few questions:\n\n1) Does it even make sense to make use git-fetch(1) use a quarantine\n directory?\n2) When making git-fetch(1) use a quarantine directory, what is the\n recommended way to achieve this? Is this by calling a subprocess?\n Maybe git-fetch-pack(1)?\n\n--\nToon\n"},{"id":"479574","messageId":"ZLVK5nzVZU48uvYE@nand.local","threadId":"59994","inReplyTo":"87edl6bx7o.fsf@iotcl.com","subject":"Re: Teach git-fetch(1) to use a quarantine directory","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-07-17T14:06:30Z","receivedAt":"2023-07-17T14:06:56Z","isPatch":false,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Jul 17, 2023 at 12:48:17PM +0200, Toon Claes wrote:\n> Hi,\n>\n> I've been looking into making git-fetch(1) to use a quarantine\n> directory, but I'm a bit stuck on direction.\n\nWhat are you hoping to accomplish? receive-pack quarantines its objects\nto ensure that the pre-receive hook(s) are all OK before accepting the\npush. See 722ff7f876c (receive-pack: quarantine objects until\npre-receive accepts, 2016-10-03) for more of the details there.\n\nAre you suggesting that fetch be taught the same, so that we can\nquarantine the pack sent from a remote before moving it into the main\nrepository?\n\n> I took git-receive-pack(1) as an example how it uses a quarantine\n> directory. It seems it sets the environment variables\n> $GIT_OBJECT_DIRECTORY and $GIT_ALTERNATE_OBJECT_DIRECTORIES so the real\n> object db is used as an alternative, and a temporary is set as the\n> default. Then a sub-process is spawned to uses these. In case of\n> git-receive-pack(1), it calls git-unpack-objects(1).\n>\n> At the moment git-fetch(1) does not spawn any similar subprocess, so if\n> we want to take the same approach to use the quarantine, we'll need to\n> split up that command.\n\nThat doesn't seem necessary, you can use `tmp_objdir_add_as_alternate()`\nto register a temporary directory as an alternate. See the tmp-objdir.h\nAPI for more convenience functions.\n\nThanks,\nTaylor\n"},{"id":"479591","messageId":"20230717234704.1906465-1-jonathantanmy@google.com","threadId":"59994","inReplyTo":"87edl6bx7o.fsf@iotcl.com","subject":"Re: Teach git-fetch(1) to use a quarantine directory","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2023-07-17T23:47:04Z","receivedAt":"2023-07-17T23:47:13Z","isPatch":false,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Toon Claes writes:\n> Hi,\n> \n> I've been looking into making git-fetch(1) to use a quarantine\n> directory, but I'm a bit stuck on direction.\n\nThanks for looking into this!\n\n> I took git-receive-pack(1) as an example how it uses a quarantine\n> directory. It seems it sets the environment variables\n> $GIT_OBJECT_DIRECTORY and $GIT_ALTERNATE_OBJECT_DIRECTORIES so the real\n> object db is used as an alternative, and a temporary is set as the\n> default. Then a sub-process is spawned to uses these. In case of\n> git-receive-pack(1), it calls git-unpack-objects(1).\n\nFrom a reading of the source code, it may call git-unpack-objects or\ngit-index-pack, I think. (unpack() in builtin/receive-pack.c)\n\n> At the moment git-fetch(1) does not spawn any similar subprocess, so if\n> we want to take the same approach to use the quarantine, we'll need to\n> split up that command.\n\nIt calls get_pack() in fetch-pack.c, which actually may call one of the\ntwo commands above in the same way. (The calling may happen in various\nways. IIRC in fetch protocol v2, get_pack() is invoked directly by git-\nfetch, whereas in earlier protocol versions, it may be the remote helper\nthat calls git-fetch-pack that invokes this function.)\n\n> But then we run into another problem as well. git-fetch(1) updates\n> references, and that is something that's not allowed when using a tmp\n> object directory.\n\nexecute_commands() in builtin/receive-pack.c has this:\n\n /* \n * Now we'll start writing out refs, which means the objects need \n * to be in their final positions so that other processes can see them. \n */ \n if (tmp_objdir_migrate(tmp_objdir) < 0) { \n\nwhich seems to be a solution to the same problem. I think Taylor wrote\nsomething similar [1].\n\n[1] https://lore.kernel.org/git/ZLVK5nzVZU48uvYE@nand.local/\n\n> As far as I can tell from the code, fetching packs and updating refs is\n> heavily intertwined, so I'm not sure this approach is the best way\n> forward. So a few questions:\n> \n> 1) Does it even make sense to make use git-fetch(1) use a quarantine\n> directory?\n\nI don't know off-hand if this will work, but this sounds promising.\n\n> 2) When making git-fetch(1) use a quarantine directory, what is the\n> recommended way to achieve this? Is this by calling a subprocess?\n> Maybe git-fetch-pack(1)?\n\nI also don't know off-hand if this will work, but replicating what git-\nreceive-pack does makes sense to me.\n"},{"id":"479592","messageId":"xmqqcz0qdre2.fsf@gitster.g","threadId":"59994","inReplyTo":"87edl6bx7o.fsf@iotcl.com","subject":"Re: Teach git-fetch(1) to use a quarantine directory","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-07-17T23:56:05Z","receivedAt":"2023-07-17T23:56:19Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Toon Claes writes:\n\n> 1) Does it even make sense to make use git-fetch(1) use a quarantine\n> directory?\n\nWhat's the goal here? If the push gets rejected, remove the\nincoming packfile data together with quarantine repository? Who\nwill be doing such a rejection and how? Does something run in the\nquarantine repository as its main repository while using the final\none as an alternate (presumably some form of hooks) to make that\ndecision? If these goals do not make sense, then no, it does not\nmake sense to teach \"fetch\" to use a quarantine directory.\nOtherwise, teaching \"fetch\" would be a reasonable way to go forward.\n\nIt is a separate issue if the mechanisms added (if any) when we\ntought receive-pack to use a quarantine repository are directly\napplicable, if if they need some twaking, in order to reuse them in\nthe context of \"git fetch\". But their needing some tweaking does\nnot mean that it does not make sense to try teaching \"fetch\" to use\nquarantine repository.\n\nThanks.\n"},{"id":"479629","messageId":"7tprum2so74wdccd3vmzgrt7d4ds6lgsq2y6bsxcxflbxw7ds4@qv3sqnph7gyc","threadId":"59994","inReplyTo":"ZLVK5nzVZU48uvYE@nand.local","subject":"Re: Teach git-fetch(1) to use a quarantine directory","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-07-19T07:24:36Z","receivedAt":"2023-07-19T07:24:55Z","isPatch":false,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Jul 17, 2023 at 10:06:30AM -0400, Taylor Blau wrote:\n> On Mon, Jul 17, 2023 at 12:48:17PM +0200, Toon Claes wrote:\n> > Hi,\n> >\n> > I've been looking into making git-fetch(1) to use a quarantine\n> > directory, but I'm a bit stuck on direction.\n> \n> What are you hoping to accomplish? receive-pack quarantines its objects\n> to ensure that the pre-receive hook(s) are all OK before accepting the\n> push. See 722ff7f876c (receive-pack: quarantine objects until\n> pre-receive accepts, 2016-10-03) for more of the details there.\n\n> Are you suggesting that fetch be taught the same, so that we can\n> quarantine the pack sent from a remote before moving it into the main\n> repository?\n\nYes. If we quarantine received objects it becomes easier to reject them\nin case it's determined that they are bogus in any way. \n\ngit-fetch(1) already does perform some checks, namely it will fsck\nobjects when `fetch.fsckObjects` is enabled. I would consider it an\nimprovement by itself already if the incoming objects were quarantined\nand discarded if we notice that any of them are corrupt. Right now, they\nwill end up in the repository even if the consistency check declares\nthem as broken. This makes the whole option less useful in my opinion.\n\nAlso, it makes it easier to prune incoming objects in case Git crashes\nor gets killed abnormally, e.g. due to a server crash. Mind you, it's\nnot a perfect solution as the operation may be killed after objects have\nalready been migrated or midway through. But it at least makes it easier\nto clean things up a little bit.\n\nPatrick\n"}]}