fixup! pack-objects: refactor path-walk delta phase

Author: derrickstolee

builtin/pack-objects.c

@@ -3314,50 +3314,6 @@ static int should_attempt_deltas(struct object_entry *entry)
 	return 1;
 }
 
-static void find_deltas_for_region(struct object_entry *list UNUSED,
-				   struct packing_region *region,
-				   unsigned int *processed)
-{
-	struct object_entry **delta_list;
-	uint32_t delta_list_nr = 0;
-
-	ALLOC_ARRAY(delta_list, region->nr);
-	for (uint32_t i = 0; i < region->nr; i++) {
-		struct object_entry *entry = to_pack.objects + region->start + i;
-		if (should_attempt_deltas(entry))
-			delta_list[delta_list_nr++] = entry;
-	}
-
-	QSORT(delta_list, delta_list_nr, type_size_sort);
-	find_deltas(delta_list, &delta_list_nr, window, depth, processed);
-	free(delta_list);
-}
-
-static void find_deltas_by_region(struct object_entry *list,
-				  struct packing_region *regions,
-				  uint32_t start, uint32_t nr)
-{
-	unsigned int processed = 0;
-	uint32_t progress_nr;
-
-	if (!nr)
-		return;
-
-	progress_nr = regions[nr - 1].start + regions[nr - 1].nr;
-
-	if (progress)
-		progress_state = start_progress(_("Compressing objects by path"),
-						progress_nr);
-
-	while (nr--)
-		find_deltas_for_region(list,
-				       &regions[start++],
-				       &processed);
-
-	display_progress(progress_state, progress_nr);
-	stop_progress(&progress_state);
-}
-
 static void prepare_pack(int window, int depth)
 {
 	struct object_entry **delta_list;
@@ -3382,10 +3338,6 @@ static void prepare_pack(int window, int depth)
 	if (!to_pack.nr_objects || !window || !depth)
 		return;
 
-	if (path_walk)
-		find_deltas_by_region(to_pack.objects, to_pack.regions,
-				      0, to_pack.nr_regions);
-
 	ALLOC_ARRAY(delta_list, to_pack.nr_objects);
 	nr_deltas = n = 0;
 
@@ -4341,8 +4293,10 @@ static int add_objects_by_path(const char *path,
 			       enum object_type type,
 			       void *data)
 {
+	struct object_entry **delta_list;
 	size_t oe_start = to_pack.nr_objects;
 	size_t oe_end;
+	unsigned int sub_list_size;
 	unsigned int *processed = data;
 
 	/*
@@ -4375,17 +4329,32 @@ static int add_objects_by_path(const char *path,
 	if (oe_end == oe_start || !window)
 		return 0;
 
-	ALLOC_GROW(to_pack.regions,
-		   to_pack.nr_regions + 1,
-		   to_pack.nr_regions_alloc);
+	sub_list_size = 0;
+	ALLOC_ARRAY(delta_list, oe_end - oe_start);
 
-	to_pack.regions[to_pack.nr_regions].start = oe_start;
-	to_pack.regions[to_pack.nr_regions].nr = oe_end - oe_start;
-	to_pack.nr_regions++;
+	for (size_t i = 0; i < oe_end - oe_start; i++) {
+		struct object_entry *entry = to_pack.objects + oe_start + i;
 
-	*processed += oids->nr;
-	display_progress(progress_state, *processed);
+		if (!should_attempt_deltas(entry))
+			continue;
 
+		delta_list[sub_list_size++] = entry;
+	}
+
+	/*
+	 * Find delta bases among this list of objects that all match the same
+	 * path. This causes the delta compression to be interleaved in the
+	 * object walk, which can lead to confusing progress indicators. This is
+	 * also incompatible with threaded delta calculations. In the future,
+	 * consider creating a list of regions in the full to_pack.objects array
+	 * that could be picked up by the threaded delta computation.
+	 */
+	if (sub_list_size && window) {
+		QSORT(delta_list, sub_list_size, type_size_sort);
+		find_deltas(delta_list, &sub_list_size, window, depth, processed);
+	}
+
+	free(delta_list);
 	return 0;
 }
 

pack-objects.h

@@ -120,23 +120,11 @@ struct object_entry {
 	unsigned ext_base:1; /* delta_idx points outside packlist */
 };
 
-/**
- * A packing region is a section of the packing_data.objects array
- * as given by a starting index and a number of elements.
- */
-struct packing_region {
-	uint32_t start;
-	uint32_t nr;
-};
-
 struct packing_data {
 	struct repository *repo;
 	struct object_entry *objects;
 	uint32_t nr_objects, nr_alloc;
 
-	struct packing_region *regions;
-	uint32_t nr_regions, nr_regions_alloc;
-
 	int32_t *index;
 	uint32_t index_size;
 

t/t5300-pack-object.sh

@@ -726,9 +726,7 @@ test_expect_success '--name-hash-version=2 and --write-bitmap-index are incompat
 # Basic "repack everything" test
 test_expect_success '--path-walk pack everything' '
 	git -C server rev-parse HEAD >in &&
-	GIT_PROGRESS_DELAY=0 git -C server pack-objects \
-		--stdout --revs --path-walk --progress <in >out.pack 2>err &&
-	grep "Compressing objects by path" err &&
+	git -C server pack-objects --stdout --revs --path-walk <in >out.pack &&
 	git -C server index-pack --stdin <out.pack
 '
 
@@ -738,9 +736,7 @@ test_expect_success '--path-walk thin pack' '
 	$(git -C server rev-parse HEAD)
 	^$(git -C server rev-parse HEAD~2)
 	EOF
-	GIT_PROGRESS_DELAY=0 git -C server pack-objects \
-		--thin --stdout --revs --path-walk --progress <in >out.pack 2>err &&
-	grep "Compressing objects by path" err &&
+	git -C server pack-objects --thin --stdout --revs --path-walk <in >out.pack &&
 	git -C server index-pack --fix-thin --stdin <out.pack
 '
 

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