Re: [PATCH 2/2] fetch-pack.c: do not declare local commits as "have" in partial repos

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 22, 2024, 16:41 UTC
Message-ID: <xmqqh6a78wv6.fsf@gitster.g>
In-Reply-To: <xmqqr09c89id.fsf@gitster.g>

Junio C Hamano <gitster@pobox.com> writes:

Show 30 quoted lines

> Calvin Wan <calvinwan@google.com> writes:
>
>> In a partial repository, creating a local commit and then fetching
>> causes the following state to occur:
>>
>> commit  tree  blob
>>  C3 ---- T3 -- B3 (fetched from remote, in promisor pack)
>>  |
>>  C2 ---- T2 -- B2 (created locally, in non-promisor pack)
>>  |
>>  C1 ---- T1 -- B1 (fetched from remote, in promisor pack)
>>
>> During garbage collection, parents of promisor objects are marked as
>> UNINTERESTING and are subsequently garbage collected. In this case, C2
>> would be deleted and attempts to access that commit would result in "bad
>> object" errors (originally reported here[1]).
>
> Understandable.
> ...
>> When promisor objects are fetched, the state of the repository
>> should ensure that the above holds true. Therefore, do not declare local
>> commits as "have" in partial repositores so they can be fetched into a
>> promisor pack.
> ...
> We pretend that C2 and anything it reaches do not exist locally, to
> force them to be fetched from the remote?  We'd end up having two
> copies of C2 (one that we created locally and had before starting
> this fetch, the other we fetched when we fetched C3 from them)?
> This sounds like it is awfully inefficient both network bandwidth-
> and local disk-wise.

One related thing that worries me is what happens after we make a large push, either directly to the remote, or what we pushed elsewhere were fetched by the remote, and then we need to fetch what they created on top. The history may look like this:

1. we fetch from promisor remote.  C is in promisor packs

 ---C

2. we build on top. 'x' are local.

 ---C---x---x---x---x---x---x---x---x

3. 'x' we created above ends up to be a the promisor side,
   and others build a few commits on top.

 ---C---x---x---x---x---x---x---x---x---o---o

4. Now we try to fetch from them.  I.e. a repository that has
   history illustrated in 2. fetches the history illustrated in 3.

Because this change forbids the fetching side to tell the other side that it has 'x', the first "have" we are allowed to send is 'C', even though we only need to fetch two commits 'o' from them.

And 'x' could be numerous in distributed development workflows, as these "local" commits do not have to be ones you created locally yourself. You may have fetched and merged these commits from elsewhere where the active development is happening. The only criterion that qualifies a commit to be "local" (and causes us to omit them from "have" communication) is that we didn't obtain it directly from our promisor remote, so you may end up fetching a large portion of the history you already have from the promisor remote, just to have them into a promisor pack.

If we cannot change the definition of "is-promisor-object" for the purpose of "gc" (and it is probably I am missing what you, JTan, and HanYang thought about that I do not see he reason why), I wonder if there is a way to somehow avoid the refetching but still "move" these 'x' objects purely locally into a promisor pack?

That is, the current "git fetch" without this patch would only fetch two 'o' commits (and its associated trees and blobs) into a new promisor pack, but because we know that commits 'x' have now become re-fetchable from the promisor, we can make them promisor objects by repacking locally them and mark the resulting pack a promisor pack, without incurring the cost to the remote to prepare and send 'x' again to us. That would give us the same protection the patch under discussion offers, wouldn't it?

I however still think fixing "gc" would give us a lot more intuitive behaviour, though.

Thanks.