pack-objects: introduce GIT_TEST_PACK_PATH_WALK

There are many tests that validate whether 'git pack-objects' works as
expected. Instead of duplicating these tests, add a new test environment
variable, GIT_TEST_PACK_PATH_WALK, that implies --path-walk by default
when specified.

This was useful in testing the implementation of the --path-walk
implementation, helping to find tests that are overly specific to the
default object walk. These include:

 - t0411-clone-from-partial.sh : One test fetches from a repo that does
   not have the boundary objects. This causes the path-based walk to
   fail. Disable the variable for this test.

 - t5306-pack-nobase.sh : Similar to t0411, one test fetches from a repo
   without a boundary object.

 - t5310-pack-bitmaps.sh : One test compares the case when packing with
   bitmaps to the case when packing without them. Since we disable the
   test variable when writing bitmaps, this causes a difference in the
   object list (the --path-walk option adds an extra object). Specify
   --no-path-walk in both processes for the comparison. Another test
   checks for a specific delta base, but when computing dynamically
   without using bitmaps, the base object it too small to be considered
   in the delta calculations so no base is used.

 - t5316-pack-delta-depth.sh : This script cares about certain delta
   choices and their chain lengths. The --path-walk option changes how
   these chains are selected, and thus changes the results of this test.

 - t5322-pack-objects-sparse.sh : This demonstrates the effectiveness of
   the --sparse option and how it combines with --path-walk.

 - t5332-multi-pack-reuse.sh : This test verifies that the preferred
   pack is used for delta reuse when possible. The --path-walk option is
   not currently aware of the preferred pack at all, so finds a
   different delta base.

 - t7406-submodule-update.sh : When using the variable, the --depth
   option collides with the --path-walk feature, resulting in a warning
   message. Disable the variable so this warning does not appear.

I want to call out one specific test change that is only temporary:

 - t5530-upload-pack-error.sh : One test cares specifically about an
   "unable to read" error message. Since the current implementation
   performs delta calculations within the path-walk API callback, a
   different "unable to get size" error message appears. When this
   is changed in a future refactoring, this test change can be reverted.

Similar to GIT_TEST_NAME_HASH_VERSION, we do not add this option to the
linux-TEST-vars CI build as that's already an overloaded build.

Signed-off-by: Derrick Stolee <stolee@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 59d640d9f2557898bc0da3dca256caf98c95b710..4fd88476dd299a2c9b11c55d52b6e3655957ecf9 100644 (file)
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -226,7 +226,7 @@ static int delta_search_threads;
 static int pack_to_stdout;
 static int sparse;
 static int thin;
-static int path_walk;
+static int path_walk = -1;
 static int num_preferred_base;
 static struct progress *progress_state;
 
@@ -4230,7 +4230,7 @@ static int add_objects_by_path(const char *path,
                struct object_id *oid = &oids->oid[i];
 
                /* Skip objects that do not exist locally. */
-               if (exclude_promisor_objects &&
+               if ((exclude_promisor_objects || arg_missing_action != MA_ERROR) &&
                    oid_object_info_extended(the_repository, oid, &oi,
                                             OBJECT_INFO_FOR_PREFETCH) < 0)
                        continue;
@@ -4648,6 +4648,14 @@ int cmd_pack_objects(int argc,
        if (pack_to_stdout != !base_name || argc)
                usage_with_options(pack_usage, pack_objects_options);
 
+       if (path_walk < 0) {
+               if (use_bitmap_index > 0 ||
+                   !use_internal_rev_list)
+                       path_walk = 0;
+               else
+                       path_walk = git_env_bool("GIT_TEST_PACK_PATH_WALK", 0);
+       }
+
        if (depth < 0)
                depth = 0;
        if (depth >= (1 << OE_DEPTH_BITS)) {

diff --git a/t/README b/t/README
index 53e5b4a7107416c8e9c6c5340153b8f86041720f..ae06e62881503dfc2a825850a03216ea7c0115f7 100644 (file)
--- a/t/README
+++ b/t/README
@@ -415,6 +415,10 @@ GIT_TEST_PACK_SPARSE=<boolean> if disabled will default the pack-objects
 builtin to use the non-sparse object walk. This can still be overridden by
 the --sparse command-line argument.
 
+GIT_TEST_PACK_PATH_WALK=<boolean> if enabled will default the pack-objects
+builtin to use the path-walk API for the object walk. This can still be
+overridden by the --no-path-walk command-line argument.
+
 GIT_TEST_PRELOAD_INDEX=<boolean> exercises the preload-index code path
 by overriding the minimum number of cache entries required per thread.
 

diff --git a/t/t0411-clone-from-partial.sh b/t/t0411-clone-from-partial.sh
index 196fc617843cb94125903410adb4b847f8c3d48a..9e6bca56255be9edd6024132743b08754db144f9 100755 (executable)
--- a/t/t0411-clone-from-partial.sh
+++ b/t/t0411-clone-from-partial.sh
@@ -59,6 +59,12 @@ test_expect_success 'pack-objects should fetch from promisor remote and execute
 
 test_expect_success 'clone from promisor remote does not lazy-fetch by default' '
        rm -f script-executed &&
+
+       # The --path-walk feature of "git pack-objects" is not
+       # compatible with this kind of fetch from an incomplete repo.
+       GIT_TEST_PACK_PATH_WALK=0 &&
+       export GIT_TEST_PACK_PATH_WALK &&
+
        test_must_fail git clone evil no-lazy 2>err &&
        test_grep "lazy fetching disabled" err &&
        test_path_is_missing script-executed

diff --git a/t/t5306-pack-nobase.sh b/t/t5306-pack-nobase.sh
index 805d60ff3179ce522219b8ba46e13f8bbf1c80ab..609399d54fbb5ba0e1af1158dcd5c5744811c705 100755 (executable)
--- a/t/t5306-pack-nobase.sh
+++ b/t/t5306-pack-nobase.sh
@@ -59,6 +59,11 @@ test_expect_success 'indirectly clone patch_clone' '
         git pull ../.git &&
         test $(git rev-parse HEAD) = $B &&
 
+       # The --path-walk feature of "git pack-objects" is not
+       # compatible with this kind of fetch from an incomplete repo.
+       GIT_TEST_PACK_PATH_WALK=0 &&
+       export GIT_TEST_PACK_PATH_WALK &&
+
         git pull ../patch_clone/.git &&
         test $(git rev-parse HEAD) = $C
        )

diff --git a/t/t5310-pack-bitmaps.sh b/t/t5310-pack-bitmaps.sh
index 621bbbdd26ed211855a741a4e96400a8fe90b2b3..e01df807a62d784f9e983d14cf75cfba95e79282 100755 (executable)
--- a/t/t5310-pack-bitmaps.sh
+++ b/t/t5310-pack-bitmaps.sh
@@ -158,8 +158,9 @@ test_bitmap_cases () {
                ls .git/objects/pack/ | grep bitmap >output &&
                test_line_count = 1 output &&
                # verify equivalent packs are generated with/without using bitmap index
-               packasha1=$(git pack-objects --no-use-bitmap-index --all packa </dev/null) &&
-               packbsha1=$(git pack-objects --use-bitmap-index --all packb </dev/null) &&
+               # Be careful to not use the path-walk option in either case.
+               packasha1=$(git pack-objects --no-use-bitmap-index --no-path-walk --all packa </dev/null) &&
+               packbsha1=$(git pack-objects --use-bitmap-index --no-path-walk --all packb </dev/null) &&
                list_packed_objects packa-$packasha1.idx >packa.objects &&
                list_packed_objects packb-$packbsha1.idx >packb.objects &&
                test_cmp packa.objects packb.objects
@@ -388,6 +389,14 @@ test_bitmap_cases () {
                git init --bare client.git &&
                (
                        cd client.git &&
+
+                       # This test relies on reusing a delta, but if the
+                       # path-walk machinery is engaged, the base object
+                       # is considered too small to use during the
+                       # dynamic computation, so is not used.
+                       GIT_TEST_PACK_PATH_WALK=0 &&
+                       export GIT_TEST_PACK_PATH_WALK &&
+
                        git config transfer.unpackLimit 1 &&
                        git fetch .. delta-reuse-old:delta-reuse-old &&
                        git fetch .. delta-reuse-new:delta-reuse-new &&

diff --git a/t/t5316-pack-delta-depth.sh b/t/t5316-pack-delta-depth.sh
index 32cf4227451ff762414f53f415560399275ee8a3..167c3a352345763b1dd466705c8ca24174e7e145 100755 (executable)
--- a/t/t5316-pack-delta-depth.sh
+++ b/t/t5316-pack-delta-depth.sh
@@ -89,15 +89,18 @@ max_chain() {
 # adjusted (or scrapped if the heuristics have become too unreliable)
 test_expect_success 'packing produces a long delta' '
        # Use --window=0 to make sure we are seeing reused deltas,
-       # not computing a new long chain.
-       pack=$(git pack-objects --all --window=0 </dev/null pack) &&
+       # not computing a new long chain. (Also avoid the --path-walk
+       # option as it may break delta chains.)
+       pack=$(git pack-objects --all --window=0 --no-path-walk </dev/null pack) &&
        echo 9 >expect &&
        max_chain pack-$pack.pack >actual &&
        test_cmp expect actual
 '
 
 test_expect_success '--depth limits depth' '
-       pack=$(git pack-objects --all --depth=5 </dev/null pack) &&
+       # Avoid --path-walk to avoid breaking delta chains across path
+       # boundaries.
+       pack=$(git pack-objects --all --depth=5 --no-path-walk </dev/null pack) &&
        echo 5 >expect &&
        max_chain pack-$pack.pack >actual &&
        test_cmp expect actual

diff --git a/t/t5332-multi-pack-reuse.sh b/t/t5332-multi-pack-reuse.sh
index 57cad7708f80f96bac87323b7dc47b18a9ad577a..395d09444ced72d219ba5f19ae7e3b729aef5590 100755 (executable)
--- a/t/t5332-multi-pack-reuse.sh
+++ b/t/t5332-multi-pack-reuse.sh
@@ -7,6 +7,13 @@ test_description='pack-objects multi-pack reuse'
 
 GIT_TEST_MULTI_PACK_INDEX=0
 GIT_TEST_MULTI_PACK_INDEX_WRITE_INCREMENTAL=0
+
+# The --path-walk option does not consider the preferred pack
+# at all for reusing deltas, so this variable changes the
+# behavior of this test, if enabled.
+GIT_TEST_PACK_PATH_WALK=0
+export GIT_TEST_PACK_PATH_WALK
+
 objdir=.git/objects
 packdir=$objdir/pack
 

diff --git a/t/t5530-upload-pack-error.sh b/t/t5530-upload-pack-error.sh
index 558eedf25a4c9b9f2bac8dca0d69b1b7eaebf381..8eb6fea839a63f514c1583e755d7d87e6435271b 100755 (executable)
--- a/t/t5530-upload-pack-error.sh
+++ b/t/t5530-upload-pack-error.sh
@@ -34,6 +34,12 @@ test_expect_success 'upload-pack fails due to error in pack-objects packing' '
        hexsz=$(test_oid hexsz) &&
        printf "%04xwant %s\n00000009done\n0000" \
                $(($hexsz + 10)) $head >input &&
+
+       # The current implementation of path-walk causes a different
+       # error message. This will be changed by a future refactoring.
+       GIT_TEST_PACK_PATH_WALK=0 &&
+       export GIT_TEST_PACK_PATH_WALK &&
+
        test_must_fail git upload-pack . <input >/dev/null 2>output.err &&
        test_grep "unable to read" output.err &&
        test_grep "pack-objects died" output.err

diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index c562bad042ab2d4d0f82cb8b57a1eadbe24044d1..ab76d4b6dc41b8d4e9253525b3ea187e23535639 100755 (executable)
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -1095,12 +1095,15 @@ test_expect_success 'submodule update --quiet passes quietness to fetch with a s
        (cd super5 &&
         # This test var can mess with the stderr output checked in this test.
         GIT_TEST_NAME_HASH_VERSION=1 \
+        GIT_TEST_PACK_PATH_WALK=0 \
                git submodule update --quiet --init --depth=1 submodule3 >out 2>err &&
         test_must_be_empty out &&
         test_must_be_empty err
        ) &&
        git clone super4 super6 &&
        (cd super6 &&
+        # This test variable will create a "warning" message to stderr
+        GIT_TEST_PACK_PATH_WALK=0 \
         git submodule update --init --depth=1 submodule3 >out 2>err &&
         test_file_not_empty out &&
         test_file_not_empty err