Re: [PATCH] fetch-pack: do not mix --pack_header and packfile uri

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 10, 2021, 18:30 UTC
Message-ID: <xmqqa6rakgj3.fsf@gitster.g>
In-Reply-To: <20210310165716.2014235-1-jonathantanmy@google.com>

Jonathan Tan <jonathantanmy@google.com> writes:

> You mentioned --fix-thin (5) and --promisor (8). Why do you think that
> none of these should be given to the "index-pack" that processes the
> packfiles given by URI?

Actually, --fix-thin is probably even worse than that.

As the code processes the in-stream packdata before processing or even downloading the pregenerated URI packfile, the objects necessary to fix a "thin" in-stream packdata are likely to be unavailable (it is exactly the same problem as the one that made us to delay the fsckobjects done in index-pack when URI packfile is involved, isn't it?). Even if the client asks --thin, the server side shouldn't produce a thin pack for in-stream packdata, no?

> Perhaps it could be argued that these extra
> packfiles don't need --fix-thin (but I would say that I think servers
> should be allowed to serve thin packfiles through URI too),

I agree that URI packfile could be thin; after all, the server end chooses, based on what the client claims to have, which pregenerated packfile to hand out, so it is perfectly fine to hand out a pregenerated packfile that is thin if the client asks for a thin pack and says it has base objects missing from that packfile. And because it is (assumed to be) pregenerated, we can make a requirement that no URI packfile should depend on objects that are created later that that (which means it won't depend on in-stream packdata).

But we cannot process a thin in-stream packdata, if we are to process it first, right?

> but I think
> that --promisor is necessary (so that a server could, for example,
> offload all trees and commits to a packfile in a CDN, and offload all
> blobs to a separate packfile in a CDN).

Yes, both packfiles conceptually are given by that same server who promises to be always available to feed us everything we'd need later, so both packfiles should be marked to have come from the same promisor. So this is one example that happens to be sharable between the two.

But I do not see it as an indication that the two packs inherently must be processed with the same options.

> Looking at this list, I think that all the arguments (except 9, which
> has been fixed) are necessary (or at least useful) for indexing a
> packfile given by URI.

I have to say that this is focusing too much on the current need by going through how the current code handles two packs. Of course, if we start from "two must be the same" viewpoint, and restrict what the code can do by "guarding" bits that require the two to be different out based on "if (index_pack_args)", then the resulting code would invoke two index-pack the same way.

I am more worried about the longer term code health, so "currently mostly the same" does not make a convincing argument for the future why the two must be processed the same way.

Show 9 quoted lines

>> Also, because this loop copies everything in cmd.args, if our
>> in-stream packdata is small, cmd.args.v[0] would be "unpack-objects",
>> and we end up asking the command to explode the (presumably large
>> enough to be worth pre-generating and serving via CDN) packfile that
>> is given via the packfile URI mechanism.
>
> I specifically guard against this through the "if (do_keep ||
> args->from_promisor || index_pack_args || fsck_objects) {" line (which
> is a complicated line, unfortunately).

I am aware of that line that forbids the in-stream packdata from getting unpacked into loose objects. But unless we were told to keep the resulting pack, or run fsck-objects via the index-pack, I do not see an inherent reason why the "most recent leftover bits that are not in the pregenerated pack offloaded to CDN" objects must be kept in a separate packfile, especially if the number of objects in it is smaller than the unpack limit threshold. In other words, I view that "guard" as one of the things that blinds us into thinking that the two packs should be handled the same way. It is the other way around---the guard is there only because the code wanted to handle the two packs the same way.

When cloning from a server that offers bulk of old history in a URI packfile and an in-stream packfile, shouldn't the result be like cloning from the server back when it had only the objects in the URI packfile, and then fetching from it again when it acquired objects that came in the in-stream packfile? The objects that come during the second fetch would be left loose if there aren't that many, so that the third and subsequent fetches and local activity can accumulate enough loose objects to be packed into a single new pack, avoiding accumulation of too many tiny packs. And the "guard" breaks that only because this codepath wants to reuse cmd.args that is unrelated to populate index_pack_args. Isn't that an artificial limitation that we may want to eventually fix?

When we want to fix that, the "options are mostly the same when we use the index-pack command for both packdata, so let's copy the entire command line" would come back and haunt us. The person who is doing the fix may be somebody other than you, so it may not matter to you today, but it will hurt somebody tomorrow.

I already said that I think 2aec3bc4 (fetch-pack: do not mix --pack_header and packfile uri, 2021-03-04) is OK as a short-term fix for the upcoming release, but it does not change the fact that it is piling technical debt on top of existing technical debt.

And that is why I am reacting against your earlier mention of "filering out" rather strongly. The approach continues the "keep the single args array in the belief that two must be mostly the same", which I view as a misguided starting point that must be rethought.

Thanks.