Re: [PATCH v2 3/4] t5300: move --window clamp test next to unclamped

From: Jonathan Tan <jonathantanmy@google.com>
Date: Nov 15, 2024, 19:55 UTC
Message-ID: <20241115195503.3395744-1-jonathantanmy@google.com>
In-Reply-To: <20241114005652.GC1140565@coredump.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 15 quoted lines

> Where it gets weirder to me is with quarantine directories (and maybe
> this is what you meant above). On receiving a push, we "index --stdin"
> into a temporary quarantine directory. If that kicks off a pack-objects
> run, where does that pack-objects put its new pack? Within the
> quarantined index-pack we set GIT_OBJECT_DIRECTORY to the quarantine and
> add the original repo as an alternate. So I _think_ both the pushed-up
> pack and the repacked promisor pack would go into the quarantine dir,
> and then we'd migrate both (or neither) when we commit to the push.
> 
> Which is OK, but I don't know that I thought that far ahead when writing
> the quarantine stuff long ago.
> 
> It's probably somewhat academic right now, as I'm not sure if you can
> even push reliably into a promisor repo (and it doesn't look like
> receive-pack knows about passing --promisor anyway).

Thanks for this description. Such a push would be an "I am pushing a pack with missing objects to you, and you can later get those missing objects from me" situation. Not completely implausible, but doesn't seem high-priority to me.

Show 16 quoted lines

> We don't quarantine
> on fetch right now, though we have discussed it in the past (and I think
> we should consider doing it).
> 
> So this may become more real in the future. I wonder if there is a way
> to add a test to future-proof against changes to how the quarantine
> system works. The theoretical problem case is if we did quarantine
> fetches, but accidentally wrote the new promisor pack into the main
> repo instead of the quarantine, and then a fetch rejected the incoming
> pack (because of a hook, failed connectivity check, etc). Then we'd end
> up with the new promisor pack when we shouldn't, which I guess could
> move objects from that incoming pack that we rejected into the main
> repo, despite the quarantine?
> 
> I can't think of a way to test that now, without the quarantine-on-fetch
> feature existing.

Quarantine on fetch does seem like a good idea. I also can't think of a way to test that now. Although, for the fetch case, my patch set is not the first time that an extra packfile (that is, a packfile not in the "packfile" section of the fetch response) could be written during a fetch: packfile-uris and bundle-uris already exist. So I would hope that the implementor of the fetch quarantine feature would be aware of at least one of these extra features, and design the test to check that absolutely no packfiles are written if the fetch is rejected. (So I don't think the future needs to be "proofed" so much.)