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

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 14, 2024, 06:41 UTC
Message-ID: <xmqqiksq71x7.fsf@gitster.g>
In-Reply-To: <20241114005652.GC1140565@coredump.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 24 quoted lines

>> As far as I know, index-pack, when run as part of fetch, indexes a pack
>> that's not in the repository's object store; it indexes a packfile in a
>> temp directory. (So I don't think this is a strange thing to do.)
>
> When fetching (or receiving a push), we use "index-pack --stdin" and do
> write the resulting pack into the repository (and the command will
> complain if there is no repository).
> ...
>> We definitely should prevent the segfault, but I think that's better
>> done by making --promisor only work if we run index-pack from within a
>> repo. I don't think we can restrict the repacking to run only if we're
>> indexing a pack within the repo, because in our fetch case, we're
>> indexing a new pack - not one within the repo.
>
> I think the "--stdin" thing above neatly solves this.
> ...
> Yeah, I guess the fundamental thing here is that anybody who isn't
> passing "--promisor" is not going to be affected, so that at least
> limits the opportunity for surprise.
>
> The quarantine discussion above is an example of how there could be
> unexpected consequences. I _think_ it's OK based on what I wrote, but
> hopefully that explains my general feeling of surprise. I dunno. It
> still may be the least bad thing.

Tying this extra processing to the use of "--stdin" is not exactly intuitive, in that a "--stdin" user is not necessarily doing a fetch (even though a fetch may always use "--stdin"), but I guess it is a good enough approximation (and the best one easily available to us) if we want to safeguard the use of this "--promisor" logic only to fetch client.

As to future potential mis-interaction between quarantined fetch and the effect of this "repack local objects that can be reached by objects in a promisor pack" feature, I do not offhand think of a good way to future-proof it with tests.

Thanks for the discussion, both of you.