pack-objects: support `--delta-islands` with `--path-walk`

Since the inception of `--path-walk`, this option has had a documented incompatibility with `--delta-islands`. When discussing those original patches on the list, a message from Stolee in [1] noted the following: this could be remedied by [...] doing a separate walk to identify islands using the normal method In a related portion of the thread, Peff explains[2]: The delta islands code already does its own tree walk to propagate the bits down (it does rely on the base walk's show_commit() to propagate through the commits). Once each object has its island bitmaps, I think however you choose to come up with delta candidates [...] you should be able to use it. It's fundamentally just answering the question of "am I allowed to delta between these two objects". That is similar to what this patch does, and it turns out the cheaper option is sufficient: perform the same island side effects from the path-walk callback rather than doing a second walk. Recall how delta-islands are computed during a normal repack: - `show_commit()` calls `propagate_island_marks()` for each commit, which merges the commit's island bitset onto its root tree object and onto each of its parent commits. - `show_object()` for a tree records the tree's depth derived from the slash-separated pathname. Subsequent `resolve_tree_islands()` uses that depth to walk trees in increasing-depth order, propagating each tree's marks to its children. - At delta-search time, `in_same_island()` enforces that a delta target's island bitmap is a subset of its base's: every island that reaches the target must also reach the base. Path-walk's enumeration callback is `add_objects_by_path()`. It already adds objects to `to_pack`, but until now did not perform the island-related side effects. Two things are needed: - For each commit batch, call `propagate_island_marks()` on commits, exactly as `show_commit()` does. We have to be careful about the order in which we call this function, and we must see a commit before its parents in order to have island marks to propagate. The path-walk batch preserves that order. Path-walk appends commits to its `OBJ_COMMIT` batch as they come back from the same `get_revision()` loop the regular traversal uses, and `add_objects_by_path()` iterates the batch in array order. So every commit reaches `propagate_island_marks()` in the same sequence that `show_commit()` would have seen it, and the descendant-first chain that the algorithm relies on is intact. Skip island propagation for excluded commits to match the regular traversal, whose `show_commit()` callback is only invoked for interesting commits. Boundary commits may still be present in path-walk's callback so they can serve as thin-pack bases, but they should not contribute island marks. - For each tree batch, record the tree's depth from the path. Use the `record_tree_depth()` helper from the previous commit so both callbacks behave identically, including the max-depth-wins behavior when a tree is reached via more than one path. The helper accepts both the `show_object()` path shape ("foo", "foo/bar") and the path-walk shape with a trailing slash ("foo/", "foo/bar/"), so depths recorded from either traversal mode are directly comparable. This is implicit in the implementation sketch from Peff above. `resolve_tree_islands()` sorts trees by `oe->tree_depth` in increasing-depth order before propagating marks down, so that a parent tree's marks are finalized before its children inherit them. Without recording the depth at path-walk time, every path-walk-discovered tree would land at depth 0 in `to_pack`, the sort would lose its ordering, and children could inherit marks from parents whose own contributions had not yet been merged in. With those two pieces in place, `resolve_tree_islands()` receives the same island inputs from path-walk as it would from the regular traversal, so the existing island checks can be reused unchanged. Drop the documented incompatibility between `--path-walk` and `--delta-islands`, and add t5320 coverage for path-walk island repacks with and without bitmap writing, as well as the same-island case where a delta remains allowed. [1]: https://lore.kernel.org/git/9aa2471b-0850-4707-9733-d3b33609f5f2@gmail.com/ [2]: https://lore.kernel.org/git/20240911063203.GA1538586@coredump.intra.peff.net/ Signed-off-by: Taylor Blau <me@ttaylorr.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Taylor Blau committed Jun 21, 2026 at 19:03 UTC 7e6de2ac62817199a2c8dd464f9e33ae0b4966e3
3 files changed +54 -11
Documentation/git-pack-objects.adoc
+7 -7
@@ -402,13 +402,13 @@ will be automatically changed to version `1`.
402 of filenames that cause collisions in Git's default name-hash
403 algorithm.
404 +
405 -Incompatible with `--delta-islands`. When `--use-bitmap-index` is
406 -specified with `--path-walk`, a successful bitmap traversal is used for
407 -object enumeration, with path-walk remaining as the fallback traversal
408 -when the bitmap cannot satisfy the request. The `--path-walk` option
409 -supports the `--filter=<spec>` forms `blob:none`, `blob:limit=<n>`,
410 -`tree:0`, `object:type=<type>`, and `sparse:<oid>`. These supported filter
411 -types can be combined with the `combine:<spec>+<spec>` form.
405 +When `--use-bitmap-index` is specified with `--path-walk`, a successful
406 +bitmap traversal is used for object enumeration, with path-walk
407 +remaining as the fallback traversal when the bitmap cannot satisfy the
408 +request. The `--path-walk` option supports the `--filter=<spec>` forms
409 +`blob:none`, `blob:limit=<n>`, `tree:0`, `object:type=<type>`, and
410 +`sparse:<oid>`. These supported filter types can be combined with the
411 +`combine:<spec>+<spec>` form.
412
413
414 DELTA ISLANDS
builtin/pack-objects.c
+18 -4
@@ -4747,13 +4747,29 @@ static int add_objects_by_path(const char *path,
4747
4748 add_object_entry(oid, type, path, exclude);
4749
4750 - if (type == OBJ_COMMIT && write_bitmap_index) {
4750 + if (type == OBJ_COMMIT) {
4751 struct commit *commit;
4752
4753 + if (!write_bitmap_index && !use_delta_islands)
4754 + continue;
4755 +
4756 commit = lookup_commit(the_repository, oid);
4757 if (!commit)
4758 die(_("could not find commit %s"), oid_to_hex(oid));
4756 - index_commit_for_bitmap(commit);
4759 + if (write_bitmap_index)
4760 + index_commit_for_bitmap(commit);
4761 + /*
4762 + * Skip island propagation for boundary commits.
4763 + * The regular traversal's show_commit() is only
4764 + * called for interesting commits; matching that
4765 + * here keeps path-walk from doing extra work that
4766 + * would only be a no-op anyway (boundary commits
4767 + * are not in island_marks).
4768 + */
4769 + if (use_delta_islands && !exclude)
4770 + propagate_island_marks(the_repository, commit);
4771 + } else if (type == OBJ_TREE && use_delta_islands) {
4772 + record_tree_depth(oid, path);
4773 }
4774 }
4775
@@ -5215,8 +5231,6 @@ int cmd_pack_objects(int argc,
5231 const char *option = NULL;
5232 if (!path_walk_filter_compatible(&filter_options))
5233 option = "--filter";
5218 - else if (use_delta_islands)
5219 - option = "--delta-islands";
5234
5235 if (option) {
5236 warning(_("cannot use %s with %s"),
t/t5320-delta-islands.sh
+29
@@ -53,6 +53,35 @@ test_expect_success 'separate islands disallows delta' '
53 ! is_delta_base $two $one
54 '
55
56 +test_expect_success 'path-walk island repack respects islands' '
57 + GIT_TRACE2_EVENT="$(pwd)/trace.path-walk-islands" \
58 + git -c "pack.island=refs/heads/(.*)" repack -adfi \
59 + --path-walk 2>err &&
60 + test_region pack-objects path-walk trace.path-walk-islands &&
61 + test_grep ! "cannot use --delta-islands with --path-walk" err &&
62 + ! is_delta_base $one $two &&
63 + ! is_delta_base $two $one
64 +'
65 +
66 +test_expect_success 'path-walk island bitmap repack respects islands' '
67 + GIT_TRACE2_EVENT="$(pwd)/trace.path-walk-island-bitmap" \
68 + git -c "pack.island=refs/heads/(.*)" repack -a -d -f -i -b \
69 + --path-walk 2>err &&
70 + test_region pack-objects path-walk trace.path-walk-island-bitmap &&
71 + test_path_is_file .git/objects/pack/*.bitmap &&
72 + git rev-list --test-bitmap --use-bitmap-index one &&
73 + test_grep ! "cannot use --delta-islands with --path-walk" err &&
74 + ! is_delta_base $one $two &&
75 + ! is_delta_base $two $one
76 +'
77 +
78 +test_expect_success 'path-walk same island allows delta' '
79 + GIT_TRACE2_EVENT="$(pwd)/trace.path-walk-same-island" \
80 + git -c "pack.island=refs/heads" repack -adfi --path-walk &&
81 + test_region pack-objects path-walk trace.path-walk-same-island &&
82 + is_delta_base $one $two
83 +'
84 +
85 test_expect_success 'same island allows delta' '
86 git -c "pack.island=refs/heads" repack -adfi &&
87 is_delta_base $one $two