From 30ccf2fc80d3c34ef56ee1a2babcdd3ad471730c Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 20 Sep 2026 14:25:44 +0000 Subject: [PATCH] fix(shallow): honour the shallow boundary across clone, gc, fsck and rev-list MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A shallow clone records which commits have no parents on disk. bit got that record wrong and then walked past it in four places, the worst of which deleted history without saying so. clone --depth wrote the *wanted ref tips* into .git/shallow rather than the boundary the server reported. Those coincide only at depth 1, so any deeper clone claimed its tip was parentless. `repack -ad` believed it, packed the tip alone and dropped the pack holding the rest: a depth-4 clone lost three commits and exited 0. Record the response's shallow lines instead. The native HTTP clone never wrote the file at all, because prepare_clone_with_http used the fetch variant that discards shallow lines. The repository then reported itself complete, and `fetch --unshallow` — gated on that file existing — short-circuited as up to date, so such a clone could never be completed. Carry the lines through PreparedClone and apply them in both the sync and async writers. gc, prune and repack walked from the ref tips with no boundary and aborted with "Missing commit object"; #183 added the `shallow~` parameter for this and left the call sites unwired. fsck followed the same absent parents and reported a well-formed shallow clone as corrupt. rev-list emitted the boundary's parent, an id with no object behind it, so it counted one commit more than the repository holds while `log` counted correctly. Adds t9021, which covers all six against real git over both transports, and a unit test pinning the fsck graft. Verified by building the pre-fix tree: 9 of its 13 assertions fail there and all pass after. Also runs t902x in the test-subdir task. The filter is a substring match, so `t900` alone had been silently skipping t9020 as well. Fixes #184 Fixes #185 Fixes #186 Fixes #187 Fixes #188 Fixes #189 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01VDVUJHu38YtVAr6sKZtRDB --- Taskfile.pkl | 8 +- modules/bit/cmd/bit/fsck.mbt | 1 + modules/bit/cmd/bit/rev_list.mbt | 11 +- .../bit_io_native/src/upload_pack_process.mbt | 30 +-- modules/bit_lib/src/fsck.mbt | 18 +- modules/bit_lib/src/fsck_test.mbt | 45 ++++ modules/bit_lib/src/gc.mbt | 16 +- .../src/upload_pack_http_common.mbt | 14 +- modules/bit_repo/src/shallow.mbt | 32 +++ t/t9021-shallow-clone-integrity.sh | 193 ++++++++++++++++++ 10 files changed, 337 insertions(+), 31 deletions(-) create mode 100755 t/t9021-shallow-clone-integrity.sh diff --git a/Taskfile.pkl b/Taskfile.pkl index afc51c77..b3b4a75b 100644 --- a/Taskfile.pkl +++ b/Taskfile.pkl @@ -332,8 +332,12 @@ local e2e: Task = new { local testSubdir: Task = new { name = "test-subdir" - description = "subdir-clone/push integration tests (t/ directory)" - cmd = "bash t/run-tests.sh t900" + description = "subdir-clone/push and transport integration tests (t/ directory)" + // The filter is a substring match, so `t900` alone silently skips t902x. + cmd = #""" + bash t/run-tests.sh t900 + bash t/run-tests.sh t902 + """# } local testDistributed: Task = new { diff --git a/modules/bit/cmd/bit/fsck.mbt b/modules/bit/cmd/bit/fsck.mbt index 03bf0ff8..9bb3a1d0 100644 --- a/modules/bit/cmd/bit/fsck.mbt +++ b/modules/bit/cmd/bit/fsck.mbt @@ -84,6 +84,7 @@ async fn handle_fsck(args : Array[String]) -> Unit raise Error { fs, ref_tips, collect_extra=need_extra, + shallow=@repo.read_shallow_boundaries(fs, git_dir), ) errors += result.errors // Report missing objects diff --git a/modules/bit/cmd/bit/rev_list.mbt b/modules/bit/cmd/bit/rev_list.mbt index 87233eb3..5113fa65 100644 --- a/modules/bit/cmd/bit/rev_list.mbt +++ b/modules/bit/cmd/bit/rev_list.mbt @@ -600,6 +600,13 @@ async fn handle_rev_list(args : Array[String]) -> Unit raise Error { } else { Map([]) } + // Commits on the shallow boundary are grafted: their parents were never + // fetched. Following those links emits an id whose object is not in the + // store, so the walk must stop here exactly as `log` does. + let shallow_boundary : Map[String, Bool] = Map([]) + for id in @repo.read_shallow_boundaries(fs, git_dir) { + shallow_boundary[id.to_hex()] = true + } let visited : Map[String, Bool] = Map([]) let result : Array[@bitcore.ObjectId] = [] // BFS/DFS traversal — collect ALL reachable commits, then sort/slice @@ -618,7 +625,9 @@ async fn handle_rev_list(args : Array[String]) -> Unit raise Error { // Get commit and add parents match rev_list_load_commit_info(db, fs, id) { Some(info) => - if first_parent && info.parents.length() > 0 { + if shallow_boundary.contains(hex) { + () + } else if first_parent && info.parents.length() > 0 { if !visited.contains(info.parents[0].to_hex()) { queue.push(info.parents[0]) } diff --git a/modules/bit_io_native/src/upload_pack_process.mbt b/modules/bit_io_native/src/upload_pack_process.mbt index 9721de53..cdf17531 100644 --- a/modules/bit_io_native/src/upload_pack_process.mbt +++ b/modules/bit_io_native/src/upload_pack_process.mbt @@ -1261,7 +1261,10 @@ pub async fn clone_process_to_fs( if wants.length() == 0 { return refs } - let pack = fetch_pack_process(remote, wants, prefer_v2, depth~, filter~) + let fetched = fetch_pack_process_result( + remote, wants, prefer_v2, depth~, filter~, + ) + let pack = fetched.pack let objects = @pack.parse_packfile(pack) let git_dir = @bit.join_path(root, ".git") let pack_id = read_pack_trailer_id(pack) @@ -1290,24 +1293,13 @@ pub async fn clone_process_to_fs( ) } } - if depth > 0 { - let shallow_ids : Array[String] = [] - let seen : Map[String, Bool] = Map([]) - for id in wants { - let hex = id.to_hex() - if seen.contains(hex) { - continue - } - seen[hex] = true - shallow_ids.push(hex) - } - if shallow_ids.length() > 0 { - fs.write_string( - @bit.join_path(git_dir, "shallow"), - shallow_ids.join("\n") + "\n", - ) - } - } + // The boundary is whatever the server reported as shallow, not the refs we + // wanted: those coincide only at depth 1. Recording the tips instead makes + // every deeper clone look parentless at its tip, which then truncates the + // history a repack keeps. + @repo.apply_shallow_updates( + fs, rfs, git_dir, fetched.shallow, fetched.unshallow, + ) refs } diff --git a/modules/bit_lib/src/fsck.mbt b/modules/bit_lib/src/fsck.mbt index 6611fcc5..ad139a29 100644 --- a/modules/bit_lib/src/fsck.mbt +++ b/modules/bit_lib/src/fsck.mbt @@ -18,7 +18,15 @@ pub fn fsck_connectivity_check( fs : &@bit.RepoFileSystem, tips : Array[@bit.ObjectId], collect_extra? : Bool = false, + shallow? : Array[@bit.ObjectId] = [], ) -> FsckResult { + // Commits on the shallow boundary are grafted: their parents were never + // fetched, so following those links would report a well-formed shallow + // clone as corrupt. + let boundary : Map[String, Bool] = Map([]) + for id in shallow { + boundary[id.to_hex()] = true + } let reachable : Map[String, Bool] = Map([]) let missing : Map[String, Bool] = Map([]) let root_commits : Array[String] = [] @@ -68,10 +76,12 @@ pub fn fsck_connectivity_check( if !reachable.contains(tree_hex) { queue.push(info.tree) } - for parent in info.parents { - let phex = parent.to_hex() - if !reachable.contains(phex) { - queue.push(parent) + if !boundary.contains(hex) { + for parent in info.parents { + let phex = parent.to_hex() + if !reachable.contains(phex) { + queue.push(parent) + } } } } diff --git a/modules/bit_lib/src/fsck_test.mbt b/modules/bit_lib/src/fsck_test.mbt index 373e1b10..da5031e4 100644 --- a/modules/bit_lib/src/fsck_test.mbt +++ b/modules/bit_lib/src/fsck_test.mbt @@ -127,3 +127,48 @@ test "fsck_verify_loose_hash valid" { ), ) } + +///| +test "fsck_connectivity_check treats a shallow boundary as grafted" { + let fs = @bit.TestFs::new() + let git_dir = "/repo/.git" + fs.mkdir_p(git_dir) + fs.mkdir_p(git_dir + "/objects") + fs.mkdir_p(git_dir + "/refs") + let (tree_id, tree_bytes) = @bit.create_tree([]) + @bit_lib.write_object_bytes(fs, git_dir, tree_id, tree_bytes) + // A boundary commit whose parent was never fetched, exactly what a + // `clone --depth` leaves behind. + let absent_parent_hex = "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb" + let absent_parent = @bit.ObjectId::from_hex(absent_parent_hex) + let commit = @bit.Commit::new( + tree_id, + [absent_parent], + "A ", + 1000000000L, + "+0000", + "A ", + 1000000000L, + "+0000", + "boundary\n", + ) + let (commit_id, commit_bytes) = @bit.create_commit(commit) + @bit_lib.write_object_bytes(fs, git_dir, commit_id, commit_bytes) + let db = @bit_lib.ObjectDb::load(fs, git_dir) + + // Without the boundary the absent parent is reported as corruption. + let plain = @bit_lib.fsck_connectivity_check(db, fs, [commit_id]) + assert_true(plain.errors > 0) + assert_true(plain.missing.contains(absent_parent_hex)) + + // Declaring the boundary makes the same repository read as clean. + let grafted = @bit_lib.fsck_connectivity_check( + db, + fs, + [commit_id], + shallow=[commit_id], + ) + assert_eq(grafted.errors, 0) + assert_true(!grafted.missing.contains(absent_parent_hex)) + assert_true(grafted.reachable.contains(tree_id.to_hex())) +} diff --git a/modules/bit_lib/src/gc.mbt b/modules/bit_lib/src/gc.mbt index f6066129..f09cf5bf 100644 --- a/modules/bit_lib/src/gc.mbt +++ b/modules/bit_lib/src/gc.mbt @@ -83,7 +83,10 @@ fn repack_git_dir( if roots.length() == 0 { return None } - let objects = collect_reachable_objects_from_commits(db, rfs, roots) + // A shallow clone's boundary commits have no parents on disk. Walking past + // them either aborts the run or, worse, truncates what a repack keeps. + let shallow = @repo.read_shallow_boundaries(rfs, git_dir) + let objects = collect_reachable_objects_from_commits(db, rfs, roots, shallow~) collect_reachable_tag_objects(db, rfs, ref_ids, objects) // Reflog-referenced objects are roots too (git keeps them); repacking without // them would drop reflog-only objects from the new pack. @@ -101,10 +104,11 @@ fn gc_git_dir( let db = ObjectDb::load(rfs, git_dir) let roots = resolve_commit_roots(db, rfs, ref_ids) let prune_enabled = roots.length() > 0 + let shallow = @repo.read_shallow_boundaries(rfs, git_dir) let reachable = if roots.length() == 0 { [] } else { - collect_reachable_objects_from_commits(db, rfs, roots) + collect_reachable_objects_from_commits(db, rfs, roots, shallow~) } // Also include tag objects that refs point to directly (annotated tags). collect_reachable_tag_objects(db, rfs, ref_ids, reachable) @@ -154,7 +158,8 @@ fn prune_git_dir( if roots.length() == 0 { return { pruned: [] } } - let reachable = collect_reachable_objects_from_commits(db, rfs, roots) + let shallow = @repo.read_shallow_boundaries(rfs, git_dir) + let reachable = collect_reachable_objects_from_commits(db, rfs, roots, shallow~) collect_reachable_tag_objects(db, rfs, ref_ids, reachable) // Match gc_git_dir: reflog-referenced objects are protected roots, so a bare // `bit gc --prune` must not delete reflog-only loose objects. @@ -374,6 +379,7 @@ fn collect_reflog_referenced_objects( git_dir : String, objects : Array[@bit.PackObject], ) -> Unit raise @bit.GitError { + let shallow = @repo.read_shallow_boundaries(fs, git_dir) let seen : Map[String, Bool] = Map([]) for obj in objects { seen[@bit.hash_object_content(obj.obj_type, obj.data).to_hex()] = true @@ -391,7 +397,9 @@ fn collect_reflog_referenced_objects( objects.push(obj) // If it's a commit, also collect its tree and blobs if obj.obj_type == @bit.ObjectType::Commit { - let sub = collect_reachable_objects_from_commits(db, fs, [id]) + let sub = collect_reachable_objects_from_commits( + db, fs, [id], shallow~, + ) for s in sub { let sh = @bit.hash_object_content(s.obj_type, s.data).to_hex() if !seen.contains(sh) { diff --git a/modules/bit_protocol/src/upload_pack_http_common.mbt b/modules/bit_protocol/src/upload_pack_http_common.mbt index ced83496..ac4da0c7 100644 --- a/modules/bit_protocol/src/upload_pack_http_common.mbt +++ b/modules/bit_protocol/src/upload_pack_http_common.mbt @@ -253,6 +253,8 @@ priv struct PreparedClone { pack : Bytes pack_id : @bit.ObjectId objects : Array[@bit.PackObject] + shallow : Array[@bit.ObjectId] + unshallow : Array[@bit.ObjectId] } ///| @@ -276,9 +278,13 @@ async fn prepare_clone_with_http( if default_ref is None || wants.length() == 0 { return (refs, None) } - let pack = fetch_pack_with_http( + // Keep the shallow/unshallow lines: without them a --depth clone leaves no + // boundary on disk, so the repository reports itself complete and a later + // `fetch --unshallow` has nothing to deepen from. + let result = fetch_pack_with_http_result( remote, wants, prefer_v2, depth, filter, http_get, http_post, ) + let pack = result.pack ( refs, Some({ @@ -286,6 +292,8 @@ async fn prepare_clone_with_http( pack, pack_id: read_pack_trailer_id_http(pack), objects: @pack.parse_packfile(pack), + shallow: result.shallow, + unshallow: result.unshallow, }), ) } @@ -316,6 +324,9 @@ pub async fn clone_to_fs_with_http( let git_dir = @bit.join_path(root, ".git") fs.mkdir_p(@bit.join_path(git_dir, "objects/pack")) @pack.write_packfile_with_index(fs, git_dir, prepared.pack, prepared.objects) + @repo.apply_shallow_updates( + fs, rfs, git_dir, prepared.shallow, prepared.unshallow, + ) if filter.is_partial() { write_promisor_file(fs, git_dir, remote) write_pack_promisor_marker_http( @@ -384,6 +395,7 @@ pub async fn[ prepared.pack, prepared.objects, ) + @repo.write_shallow_boundaries_async(fs, git_dir, prepared.shallow) if filter.is_partial() { write_promisor_file_async(fs, git_dir, remote) write_pack_promisor_marker_http_async( diff --git a/modules/bit_repo/src/shallow.mbt b/modules/bit_repo/src/shallow.mbt index c5f0c513..a85d0038 100644 --- a/modules/bit_repo/src/shallow.mbt +++ b/modules/bit_repo/src/shallow.mbt @@ -53,3 +53,35 @@ pub fn apply_shallow_updates( let lines = [ for _, id in boundaries => id.to_hex() ] fs.write_string(path, lines.join("\n") + "\n") } + +///| +/// Record the shallow boundary of a freshly cloned repository. +/// +/// A clone starts with no `shallow` file, so there is nothing to merge and no +/// prior boundary an `unshallow` line could lift: writing the reported set is +/// the whole job. `apply_shallow_updates` covers the incremental fetch case, +/// which needs to read what is already there. +pub async fn[FS : @types.AsyncFileSystem] write_shallow_boundaries_async( + fs : FS, + git_dir : String, + shallow : Array[@object.ObjectId], +) -> Unit raise @object.GitError { + if shallow.length() == 0 { + return + } + let seen : Map[String, Bool] = Map([]) + let lines : Array[String] = [] + for id in shallow { + let hex = id.to_hex() + if seen.contains(hex) { + continue + } + seen[hex] = true + lines.push(hex) + } + @types.AsyncFileSystem::write_string( + fs, + join_path(git_dir, "shallow"), + lines.join("\n") + "\n", + ) +} diff --git a/t/t9021-shallow-clone-integrity.sh b/t/t9021-shallow-clone-integrity.sh new file mode 100755 index 00000000..644f3456 --- /dev/null +++ b/t/t9021-shallow-clone-integrity.sh @@ -0,0 +1,193 @@ +#!/bin/sh +# +# Shallow clones must record the boundary the server reported, and every +# history walk must stop there instead of running into objects that were +# never fetched. +# + +test_description='bit shallow clone records its boundary and keeps history intact' + +TEST_DIRECTORY=$(cd "$(dirname "$0")" && pwd) +. "$TEST_DIRECTORY/test-lib.sh" + +if ! test_have_prereq GIT; then + test_skip "shallow clone integrity" "git not found" + test_done +fi + +# `bit` is run with --no-git-fallback throughout: without it clone and gc are +# delegated to the real git binary and none of this exercises bit at all. +bit_cmd() { + "$BIT" --no-git-fallback "$@" +} + +test_expect_success 'setup a six-commit origin' ' + git init -q -b main src && + (cd src && + git config user.email test@example.com && + git config user.name Test && + git config commit.gpgsign false && + for n in 1 2 3 4 5 6; do + echo $n >f$n.txt && + git add . && + git commit -q -m "c$n" || exit 1 + done) && + git clone -q --bare src origin.git && + git -C origin.git symbolic-ref HEAD refs/heads/main && + git -C origin.git config http.receivepack true +' + +# The boundary is the oldest commit fetched, not the ref tip. Recording the tip +# makes the clone look parentless one commit in, so git walks less history than +# the repository actually holds. Comparing the objects on disk against what a +# boundary-honouring walk shows catches that; comparing two walks would not, +# since both would be truncated by the same wrong boundary. +# +# The loop runs in a subshell: `eval` runs a test body in the current shell, so +# a bare `exit` would abandon the whole file. +test_expect_success 'clone --depth keeps every fetched commit walkable' ' + ( for depth in 1 2 3 4; do + rm -rf "d$depth" && + bit_cmd clone -q --depth "$depth" "file://$(pwd)/origin.git" "d$depth" && + present=$(git -C "d$depth" cat-file --batch-all-objects --batch-check | + grep -c commit) && + visible=$(git -C "d$depth" log --oneline | wc -l | tr -d " ") && + test "$present" = "$depth" && + test "$visible" = "$depth" || { + echo "depth $depth: $present commits on disk, $visible walkable" >&2 + exit 1 + } + done ) +' + +test_expect_success 'clone --depth marks the repository shallow' ' + rm -rf d && + bit_cmd clone -q --depth 2 "file://$(pwd)/origin.git" d && + test "$(bit_cmd -C d rev-parse --is-shallow-repository)" = true +' + +# rev-list used to emit the boundary commit'\''s parent, an id with no object +# behind it, so it reported one commit more than the repository holds. +test_expect_success 'rev-list stops at the boundary and matches log' ' + rm -rf d && + bit_cmd clone -q --depth 2 "file://$(pwd)/origin.git" d && + test "$(bit_cmd -C d rev-list --count HEAD)" = 2 && + test "$(bit_cmd -C d rev-list HEAD | wc -l)" = 2 && + test "$(bit_cmd -C d log --oneline | wc -l)" = 2 && + bit_cmd -C d rev-list HEAD | while read -r oid; do + git -C d cat-file -e "$oid" || exit 1 + done +' + +test_expect_success 'gc succeeds in a shallow clone and keeps every commit' ' + rm -rf d && + bit_cmd clone -q --depth 3 "file://$(pwd)/origin.git" d && + bit_cmd -C d gc && + test "$(bit_cmd -C d log --oneline | wc -l)" = 3 +' + +test_expect_success 'prune succeeds in a shallow clone' ' + rm -rf d && + bit_cmd clone -q --depth 3 "file://$(pwd)/origin.git" d && + bit_cmd -C d prune && + test "$(bit_cmd -C d log --oneline | wc -l)" = 3 +' + +# repack -ad used to pack only the tip and then delete the pack holding the +# rest, losing commits with a zero exit status. +test_expect_success 'repack -ad keeps every commit in a shallow clone' ' + ( for depth in 1 2 3 4; do + rm -rf "r$depth" && + bit_cmd clone -q --depth "$depth" "file://$(pwd)/origin.git" "r$depth" && + before=$(git -C "r$depth" cat-file --batch-all-objects --batch-check | + grep -c commit) && + bit_cmd -C "r$depth" repack -ad && + after=$(git -C "r$depth" cat-file --batch-all-objects --batch-check | + grep -c commit) && + test "$before" = "$after" || { + echo "depth $depth: repack lost commits, $before -> $after" >&2 + exit 1 + } + done ) +' + +test_expect_success 'fsck is clean in a shallow clone' ' + rm -rf d && + bit_cmd clone -q --depth 2 "file://$(pwd)/origin.git" d && + bit_cmd -C d fsck +' + +test_expect_success 'committing on top of a shallow tip keeps gc and fsck happy' ' + rm -rf d && + bit_cmd clone -q --depth 2 "file://$(pwd)/origin.git" d && + echo new >d/new.txt && + bit_cmd -C d add . && + bit_cmd -C d commit -q -m "on top of shallow" && + test "$(bit_cmd -C d rev-list --count HEAD)" = 3 && + bit_cmd -C d gc && + bit_cmd -C d fsck +' + +test_expect_success 'fetch --unshallow completes the history' ' + rm -rf d && + bit_cmd clone -q --depth 2 "file://$(pwd)/origin.git" d && + bit_cmd -C d fetch --unshallow && + test "$(bit_cmd -C d rev-list --count HEAD)" = 6 && + test_path_is_missing d/.git/shallow && + bit_cmd -C d fsck +' + +test_expect_success 'fetch --depth deepens to the requested depth' ' + rm -rf d && + bit_cmd clone -q --depth 1 "file://$(pwd)/origin.git" d && + bit_cmd -C d fetch --depth 4 && + test "$(bit_cmd -C d rev-list --count HEAD)" = 4 +' + +# The native HTTP clone used to drop the shallow lines entirely, leaving a +# repository that was shallow in content but reported itself complete, so +# --unshallow afterwards had nothing to deepen from. +if command -v node >/dev/null 2>&1; then + PORT=$((10000 + $$ % 50000)) + SERVER_PID="" + + cleanup_http() { + if test -n "$SERVER_PID"; then + kill "$SERVER_PID" 2>/dev/null || true + SERVER_PID="" + fi + } + trap 'cleanup_http; cleanup' EXIT + + test_expect_success 'start a smart HTTP server' ' + USE_REAL_GIT=1 node "$BIT_BUILD_DIR/tools/http-test-server.js" \ + "$(pwd)/src" "$PORT" >server.log 2>&1 & + SERVER_PID=$! && + ready= && + for _ in 1 2 3 4 5 6 7 8 9 10; do + if git ls-remote "http://localhost:$PORT" >/dev/null 2>&1; then + ready=yes + break + fi + sleep 1 + done && + test -n "$ready" + ' + + # The tear-down must not sit at the end of the && chain: `cmd || true` + # there would swallow every assertion before it and the test could never + # fail. It belongs in the EXIT trap above. + test_expect_success 'http clone --depth records the boundary and can unshallow' ' + rm -rf h && + bit_cmd clone -q --depth 2 "http://localhost:$PORT" h && + test_path_is_file h/.git/shallow && + test "$(bit_cmd -C h rev-parse --is-shallow-repository)" = true && + test "$(bit_cmd -C h rev-list --count HEAD)" = 2 && + bit_cmd -C h fetch --unshallow && + test "$(bit_cmd -C h rev-list --count HEAD)" = 6 + ' +else + test_skip "http shallow clone" "node not found" +fi + +test_done