Re: [PATCH 3/7] pack-objects: add GIT_TEST_FULL_NAME_HASH

From: Jonathan Tan <jonathantanmy@google.com>
Date: Nov 22, 2024, 01:13 UTC
Message-ID: <20241122011308.2743517-1-jonathantanmy@google.com>
In-Reply-To: <259734e0bcea952c2c09b0fb3a017e139922b975.1730775908.git.gitgitgadget@gmail.com>

"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:

> Second, there are two tests in t5616-partial-clone.sh that I believe are
> actually broken scenarios.

I took a look...this is a tricky one.

Show 6 quoted lines

> While the client is set up to clone the
> 'promisor-server' repo via a treeless partial clone filter (tree:0),
> that filter does not translate to the 'server' repo. Thus, fetching from
> these repos causes the server to think that the client has all reachable
> trees and blobs from the commits advertised as 'haves'. This leads the
> server to providing a thin pack assuming those objects as delta bases.

It is expected that the server sometimes sends deltas based on objects that the client doesn't have. In fact, this test tests the ability of Git to lazy-fetch delta bases.

> Changing the name-hash algorithm presents new delta bases and thus
> breaks the expectations of these tests.

To be precise, the change resulted in no deltas being sent (before this change, one delta was sent). Here's what is meant to happen. The server has:

 commitB - treeB - file1 ("make the tree big\nanother line\n"), file2...file100
  |
 commitA - treeA - file1...file100 ("make the tree big\n")

The client only has commitA. (The client does not have treeA or any blob, since it was cloned with --filter=tree:0.)

When GIT_TEST_FULL_NAME_HASH=0 (matching the current behavior), the server sends a non-delta commitB, a delta treeB (with base treeA), and a non-delta blob "make the tree big\nanother line\n". This triggers a lazy fetch of treeA, and thus treeB is inflated successfully. During the subsequent connectivity check (with --exclude-promisor-objects, see connected.c), it is noticed that the "make the tree big\n" blob is missing, but since it is a promisor object (referenced by treeA, which was fetched from the promisor remote), the connectivity check since passes.

When GIT_TEST_FULL_NAME_HASH=1, the server sends a non-delta commitB, a non-delta treeB, and a non-delta blob "make the tree big\nanother line\n". No lazy fetch is triggered. During the subsequent connectivity check, the "make the tree big\n" blob (referenced by treeB) is missing. There is nothing that can vouch for it (the client does not have treeA, remember) so the client does not consider it a promisor object, and thus the connectivity check fails.

Investigating this was made a bit harder due to a missing "git -C promisor-remote config --local uploadpack.allowfilter 1" in the test. The above behavior is after this is included in the test.

I think the solution is to have an algorithm that preserves the property that treeB is sent as a delta object - if not, we need to find another way to test the lazy-fetch of delta bases. My proposal in [1] does do that.

[1] https://lore.kernel.org/git/20241121235014.2554033-1-jonathantanmy@google.com/