[v2,3/6] pack-objects: add GIT_TEST_FULL_NAME_HASH

Commit Message

Derrick Stolee Sept. 18, 2024, 8:46 p.m. UTC

From: Derrick Stolee <[e-mail]>

Add a new environment variable to opt-in to the --full-name-hash option
in 'git pack-objects'. This allows for extra testing of the feature
without repeating all of the test scenarios.

But this option isn't free. There are a few tests that change behavior
with the variable enabled.

First, there are a few tests that are very sensitive to certain delta
bases being picked. These are both involving the generation of thin
bundles and then counting their objects via 'git index-pack --fix-thin'
which pulls the delta base into the new packfile. For these tests,
disable the option as a decent long-term option.

Second, there are two tests in t5616-partial-clone.sh that I believe are
actually broken scenarios. 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.
Changing the name-hash algorithm presents new delta bases and thus
breaks the expectations of these tests. An alternative could be to set
up 'server' as a promisor server with the correct filter enabled. This
may also point out more issues with partial clone being set up as a
remote-based filtering mechanism and not a repository-wide setting. For
now, do the minimal change to make the test work by disabling the test
variable.

Signed-off-by: Derrick Stolee <[e-mail]>
---
 builtin/pack-objects.c    |  6 ++++--
 ci/run-build-and-tests.sh |  1 +
 t/README                  |  4 ++++
 t/t5510-fetch.sh          |  7 ++++++-
 t/t5616-partial-clone.sh  | 26 ++++++++++++++++++++++++--
 t/t6020-bundle-misc.sh    |  6 +++++-
 6 files changed, 44 insertions(+), 6 deletions(-)

Comments

Junio C Hamano Sept. 19, 2024, 10:22 p.m. UTC | #1

"Derrick Stolee via GitGitGadget" <[e-mail]> writes:

> From: Derrick Stolee <[e-mail]>
>
> Add a new environment variable to opt-in to the --full-name-hash option
> in 'git pack-objects'. This allows for extra testing of the feature
> without repeating all of the test scenarios.

This also allows the programmer on the C implementation side to be a
bit lazy, as the --full-name-hash option does not have to be plumbed
through from the end-user facing commands (like "bundle") down to
the underlying "pack-objects" command ;-).

As an end-user facing tweak mechanism, an environment variable is
the most clunky, followed by a configuration variable (which can be
used via "git -c" and exhibits the same clunkiness as an environment
variable), and a command line parameter is the most versatile in
allowing users to customize the behaviour per-invocation of the
commands.  So in the longer term, we probably want to plumb through
the option, like you did for "repack -> pack-objects" call chain,
for all end-user visible commands that call into pack-objects.

But for testing purposes, the solution presented here is of course
good enough.

> Second, there are two tests in t5616-partial-clone.sh that I believe are
> actually broken scenarios. 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.

In short, the tests are based on broken assumption and checking
bogus outcome?  Somebody familiar with the partial clone area should
probably take a look into it and fix the tests if that is the case.

> -	if (write_bitmap_index && use_full_name_hash)
> +	if (write_bitmap_index && use_full_name_hash > 0)
>  		die(_("currently, the --full-name-hash option is incompatible with --write-bitmap-index"));
> +	if (use_full_name_hash < 0)
> +		use_full_name_hash = git_env_bool("GIT_TEST_FULL_NAME_HASH", 0);

OK.

Derrick Stolee Sept. 23, 2024, 1:39 a.m. UTC | #2

On 9/19/24 6:22 PM, Junio C Hamano wrote:
 > "Derrick Stolee via GitGitGadget" <[e-mail]> writes:

 >> Second, there are two tests in t5616-partial-clone.sh that I believe are
 >> actually broken scenarios. 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.
 >
 > In short, the tests are based on broken assumption and checking
 > bogus outcome?  Somebody familiar with the partial clone area should
 > probably take a look into it and fix the tests if that is the case.

That is my understanding, yes. It is a common issue that I've had when
using partial clones with multiple remotes and forgetting to modify the
filter for each, so is also a usability wart that could use mending
(say, by adding the existing filter when adding a new remote).

Thanks,
-Stolee

Patch

diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 4a990d15609..8876fa3d641 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -266,7 +266,7 @@  struct configured_exclusion {
 static struct oidmap configured_exclusions;
 
 static struct oidset excluded_by_config;
-static int use_full_name_hash;
+static int use_full_name_hash = -1;
 
 static inline uint32_t pack_name_hash_fn(const char *name)
 {
@@ -4583,8 +4583,10 @@  int cmd_pack_objects(int argc, const char **argv, const char *prefix)
 	if (pack_to_stdout || !rev_list_all)
 		write_bitmap_index = 0;
 
-	if (write_bitmap_index && use_full_name_hash)
+	if (write_bitmap_index && use_full_name_hash > 0)
 		die(_("currently, the --full-name-hash option is incompatible with --write-bitmap-index"));
+	if (use_full_name_hash < 0)
+		use_full_name_hash = git_env_bool("GIT_TEST_FULL_NAME_HASH", 0);
 
 	if (use_delta_islands)
 		strvec_push(&rp, "--topo-order");
diff --git a/ci/run-build-and-tests.sh b/ci/run-build-and-tests.sh
index 2e28d02b20f..75b40f07bbd 100755
--- a/ci/run-build-and-tests.sh
+++ b/ci/run-build-and-tests.sh
@@ -30,6 +30,7 @@  linux-TEST-vars)
 	export GIT_TEST_NO_WRITE_REV_INDEX=1
 	export GIT_TEST_CHECKOUT_WORKERS=2
 	export GIT_TEST_PACK_USE_BITMAP_BOUNDARY_TRAVERSAL=1
+	export GIT_TEST_FULL_NAME_HASH=1
 	;;
 linux-clang)
 	export GIT_TEST_DEFAULT_HASH=sha1
diff --git a/t/README b/t/README
index 44c02d81298..eaaf9fcd57e 100644
--- a/t/README
+++ b/t/README
@@ -488,6 +488,10 @@  a test and then fails then the whole test run will abort. This can help to make
 sure the expected tests are executed and not silently skipped when their
 dependency breaks or is simply not present in a new environment.
 
+GIT_TEST_FULL_NAME_HASH=<boolean>, when true, sets the default name-hash
+function in 'git pack-objects' to be the one used by the --full-name-hash
+option.
+
 Naming Tests
 ------------
 
diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
index 0890b9f61c5..be79c72495e 100755
--- a/t/t5510-fetch.sh
+++ b/t/t5510-fetch.sh
@@ -1062,7 +1062,12 @@  test_expect_success 'all boundary commits are excluded' '
 	test_tick &&
 	git merge otherside &&
 	ad=$(git log --no-walk --format=%ad HEAD) &&
-	git bundle create twoside-boundary.bdl main --since="$ad" &&
+
+	# If the --full-name-hash function is used here, then no delta
+	# pair is found and the bundle does not expand to three objects
+	# when fixing the thin object.
+	GIT_TEST_FULL_NAME_HASH=0 \
+		git bundle create twoside-boundary.bdl main --since="$ad" &&
 	test_bundle_object_count --thin twoside-boundary.bdl 3
 '
 
diff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh
index 8415884754b..69d5bab73cd 100755
--- a/t/t5616-partial-clone.sh
+++ b/t/t5616-partial-clone.sh
@@ -515,7 +515,18 @@  test_expect_success 'fetch lazy-fetches only to resolve deltas' '
 	# Exercise to make sure it works. Git will not fetch anything from the
 	# promisor remote other than for the big tree (because it needs to
 	# resolve the delta).
-	GIT_TRACE_PACKET="$(pwd)/trace" git -C client \
+	#
+	# TODO: the --full-name-hash option is disabled here, since this test
+	# is fundamentally broken! When GIT_TEST_FULL_NAME_HASH=1, the server
+	# recognizes delta bases in a different way and then sends a _blob_ to
+	# the client with a delta base that the client does not have! This is
+	# because the client is cloned from "promisor-server" with tree:0 but
+	# is now fetching from "server" withot any filter. This is violating the
+	# promise to the server that all reachable objects exist and could be
+	# used as delta bases!
+	GIT_TRACE_PACKET="$(pwd)/trace" \
+	GIT_TEST_FULL_NAME_HASH=0 \
+		git -C client \
 		fetch "file://$(pwd)/server" main &&
 
 	# Verify the assumption that the client needed to fetch the delta base
@@ -534,7 +545,18 @@  test_expect_success 'fetch lazy-fetches only to resolve deltas, protocol v2' '
 	# Exercise to make sure it works. Git will not fetch anything from the
 	# promisor remote other than for the big blob (because it needs to
 	# resolve the delta).
-	GIT_TRACE_PACKET="$(pwd)/trace" git -C client \
+	#
+	# TODO: the --full-name-hash option is disabled here, since this test
+	# is fundamentally broken! When GIT_TEST_FULL_NAME_HASH=1, the server
+	# recognizes delta bases in a different way and then sends a _blob_ to
+	# the client with a delta base that the client does not have! This is
+	# because the client is cloned from "promisor-server" with tree:0 but
+	# is now fetching from "server" withot any filter. This is violating the
+	# promise to the server that all reachable objects exist and could be
+	# used as delta bases!
+	GIT_TRACE_PACKET="$(pwd)/trace" \
+	GIT_TEST_FULL_NAME_HASH=0 \
+		git -C client \
 		fetch "file://$(pwd)/server" main &&
 
 	# Verify that protocol version 2 was used.
diff --git a/t/t6020-bundle-misc.sh b/t/t6020-bundle-misc.sh
index 34b5cd62c20..553a20d10e9 100755
--- a/t/t6020-bundle-misc.sh
+++ b/t/t6020-bundle-misc.sh
@@ -247,7 +247,11 @@  test_expect_success 'create bundle with --since option' '
 	EOF
 	test_cmp expect actual &&
 
-	git bundle create since.bdl \
+	# If the --full-name-hash option is used, then one fewer
+	# delta base is found and this counts a different number
+	# of objects after performing --fix-thin.
+	GIT_TEST_FULL_NAME_HASH=0 \
+		git bundle create since.bdl \
 		--since "Thu Apr 7 15:27:00 2005 -0700" \
 		--all &&