Teach git-fetch(1) to use a quarantine directory

Hi,

I've been looking into making git-fetch(1) to use a quarantine directory, but I'm a bit stuck on direction.

I took git-receive-pack(1) as an example how it uses a quarantine directory. It seems it sets the environment variables $GIT_OBJECT_DIRECTORY and $GIT_ALTERNATE_OBJECT_DIRECTORIES so the real object db is used as an alternative, and a temporary is set as the default. Then a sub-process is spawned to uses these. In case of git-receive-pack(1), it calls git-unpack-objects(1).

At the moment git-fetch(1) does not spawn any similar subprocess, so if we want to take the same approach to use the quarantine, we'll need to split up that command.

But then we run into another problem as well. git-fetch(1) updates references, and that is something that's not allowed when using a tmp object directory.

As far as I can tell from the code, fetching packs and updating refs is heavily intertwined, so I'm not sure this approach is the best way forward. So a few questions:

1) Does it even make sense to make use git-fetch(1) use a quarantine
   directory?
2) When making git-fetch(1) use a quarantine directory, what is the
   recommended way to achieve this? Is this by calling a subprocess?
   Maybe git-fetch-pack(1)?

-- Toon

On Mon, Jul 17, 2023 at 12:48:17PM +0200, Toon Claes wrote:

What are you hoping to accomplish? receive-pack quarantines its objects to ensure that the pre-receive hook(s) are all OK before accepting the push. See 722ff7f876c (receive-pack: quarantine objects until pre-receive accepts, 2016-10-03) for more of the details there.

Are you suggesting that fetch be taught the same, so that we can quarantine the pack sent from a remote before moving it into the main repository?

That doesn't seem necessary, you can use `tmp_objdir_add_as_alternate()` to register a temporary directory as an alternate. See the tmp-objdir.h API for more convenience functions.

Thanks, Taylor

On Mon, Jul 17, 2023 at 10:06:30AM -0400, Taylor Blau wrote:

Yes. If we quarantine received objects it becomes easier to reject them in case it's determined that they are bogus in any way.

git-fetch(1) already does perform some checks, namely it will fsck objects when `fetch.fsckObjects` is enabled. I would consider it an improvement by itself already if the incoming objects were quarantined and discarded if we notice that any of them are corrupt. Right now, they will end up in the repository even if the consistency check declares them as broken. This makes the whole option less useful in my opinion.

Also, it makes it easier to prune incoming objects in case Git crashes or gets killed abnormally, e.g. due to a server crash. Mind you, it's not a perfect solution as the operation may be killed after objects have already been migrated or midway through. But it at least makes it easier to clean things up a little bit.

Patrick

Toon Claes <toon@iotcl.com> writes:

Thanks for looking into this!

From a reading of the source code, it may call git-unpack-objects or git-index-pack, I think. (unpack() in builtin/receive-pack.c)

It calls get_pack() in fetch-pack.c, which actually may call one of the two commands above in the same way. (The calling may happen in various ways. IIRC in fetch protocol v2, get_pack() is invoked directly by git- fetch, whereas in earlier protocol versions, it may be the remote helper that calls git-fetch-pack that invokes this function.)

execute_commands() in builtin/receive-pack.c has this:

          /*                                                                                                                          
           * Now we'll start writing out refs, which means the objects need                                                           
           * to be in their final positions so that other processes can see them.                                                     
           */                                                                                                                         
          if (tmp_objdir_migrate(tmp_objdir) < 0) {     

which seems to be a solution to the same problem. I think Taylor wrote something similar [1].

[1] https://lore.kernel.org/git/ZLVK5nzVZU48uvYE@nand.local/

I don't know off-hand if this will work, but this sounds promising.

I also don't know off-hand if this will work, but replicating what git- receive-pack does makes sense to me.

Toon Claes <toon@iotcl.com> writes:

What's the goal here? If the push gets rejected, remove the incoming packfile data together with quarantine repository? Who will be doing such a rejection and how? Does something run in the quarantine repository as its main repository while using the final one as an alternate (presumably some form of hooks) to make that decision? If these goals do not make sense, then no, it does not make sense to teach "fetch" to use a quarantine directory. Otherwise, teaching "fetch" would be a reasonable way to go forward.

It is a separate issue if the mechanisms added (if any) when we tought receive-pack to use a quarantine repository are directly applicable, if if they need some twaking, in order to reuse them in the context of "git fetch". But their needing some tweaking does not mean that it does not make sense to try teaching "fetch" to use quarantine repository.

Thanks.