git: worktree, Fix unreadable subtree handling and recovery by hiddeco · Pull Request #2372 · go-git/go-git

hiddeco · 2026-09-10T13:20:49Z

Missing, corrupt, or malformed tree objects could cause several misleading or unrecoverable outcomes:

Incomplete results reported as success. An unreadable subtree could end enumeration without an error, leaving checkout or archive output incomplete. An index rebuilt from partial results could cause a later commit to record omitted paths as deleted.
A failed operation could still move HEAD. Checkout, reset, or pull could change references before failing to read the target tree. After a failed pull, retrying could incorrectly report that the repository was already up to date.
Hard reset could fail to recover. Even with a healthy target commit, hard reset and forced checkout could fail because they needed to read the unreadable old HEAD tree.

This change reports unreadable subtrees as errors and updates references only after reset processing succeeds. Hard reset and forced checkout identify tracked deletions from the pre-reset index, allowing recovery without reading the old HEAD tree. KeepReset retains its old-tree conflict checks. Index and worktree updates are not atomic.

It also fixes path handling for malformed trees: lookup descends only through directory entries, and walking a directory whose name contains slashes no longer corrupts subsequent sibling paths. Directory-mode descent follows Git’s find_tree_entry. Direct object access remains available for inspection.

Missing subtrees remain errors in partial clones because go-git cannot fetch them lazily. Documentation clarifies validation boundaries, error identities, iterator recovery, and the protections bypassed by direct use of Worktree.Filesystem.

Regression tests cover repeated failures, reference and index preservation, recovery after missing objects become available, and malformed-entry path handling

Path lookup used object types to decide whether to descend, allowing a symlink or gitlink pointing to a tree to expose paths below it. Directory entry modes must bound descent so lookup respects the recorded tree layout. Direct object access remains available for inspecting malformed entries; path validation does not replace structural validation. Assisted-by: Claude Opus 5 Assisted-by: GPT-6 Signed-off-by: Hidde Beydals <hidde@hhh.computer>

Copilot

🟡 Changes recommended

Unresolved critical error-identity and reset-synchronization issues remain, along with a moderate performance regression.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Improves unreadable and malformed Git tree handling, reset recovery, reference preservation, and path traversal.

Changes:

Propagates subtree and malformed-tree errors.
Defers reference updates until reset processing succeeds.
Adds recovery and regression tests.
Documents filesystem safety boundaries.

File summaries

File	Summary
`worktree.go`	Updates reset and recovery logic. Findings: critical `SkipWorktree` synchronization issue; moderate duplicate tree-diff work.
`worktree_test.go`	Adds reset, checkout, pull, and recovery tests.
`worktree_fs.go`	Documents filesystem safety boundaries.
`worktree_fs_test.go`	Tests malformed and missing subtree handling.
`utils/merkletrie/doubleiter.go`	Preserves wrapped traversal errors.
`utils/merkletrie/doubleiter_test.go`	Tests propagated diff errors.
`plumbing/object/tree.go`	Improves tree validation, traversal, and error handling. Finding: critical loss of wrapped missing-object identity.
`plumbing/object/tree_test.go`	Adds tree traversal and malformed-object tests.
`options.go`	Removes obsolete internal reset state.

Review details

Suppressed comments (1)

worktree.go:409

HardReset computes the full index↔target tree diff here, but resetIndex immediately computes the same diff again before applying it. Every hard reset and forced checkout therefore walks and loads the target tree twice, which is a substantial regression for large repositories. Reuse this precomputed change set when resetting the index (while applying the existing path filter) instead of rebuilding it.

		trackedChanges, err = w.diffTreeWithStaging(t, true)
		if err != nil {
			return err

Files reviewed: 9/9 changed files
Comments generated: 2
Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

+	if errors.Is(cause, io.EOF) {
+		return fmt.Errorf("entry %q has mode %s: %v", e.Name, e.Mode, cause)


+		trackedChanges, err = w.diffTreeWithStaging(t, true)
+		if err != nil {
+			return err


Unreadable subtrees were reported as EOF, allowing checkout and archive generation to succeed with incomplete results. Rebuilding an index from that enumeration could turn omitted paths into deletions. Callers need distinct errors for missing objects, wrong object types, and decode failures, with error identities preserved through tree diffs. EOF causes remain text-only so they cannot signal normal exhaustion. Missing promisor objects also fail because tree enumeration cannot fetch them lazily. Checkout and reset must finish processing before changing references, so a failed tree read does not leave HEAD at the unreadable target. This does not make index and worktree updates atomic. Assisted-by: Claude Opus 5 Assisted-by: GPT-6 Signed-off-by: Hidde Beydals <hidde@hhh.computer>

The path-validation comment incorrectly described CherryPick as a direct validator caller. It also conflated checks on tree names with checks on existing filesystem symlinks, obscuring which boundary provides protection. Assisted-by: Claude Opus 5 Assisted-by: GPT-6 Signed-off-by: Hidde Beydals <hidde@hhh.computer>

The underlying filesystem bypasses the worktree wrapper's path checks. Root containment alone does not protect an in-root .git directory, so callers need that distinction before writing names obtained from trees. Caller-supplied filesystems may provide different containment guarantees. Assisted-by: Claude Opus 5 Assisted-by: GPT-6 Signed-off-by: Hidde Beydals <hidde@hhh.computer>

Pull advanced the current branch before resetting the worktree. An unreadable subtree left HEAD at the fetched commit despite the failure, and retrying reported that the repository was already up to date. The reference update must follow a successful reset while retaining support for pulls into an unborn branch. Assisted-by: GPT-6 Signed-off-by: Hidde Beydals <hidde@hhh.computer>

Ascending from a directory entry named a/b removed only one path component, leaving a root-level sibling z reported as a/z. Malformed entry names can span multiple path components, so ascent must restore the parent path by tree depth. Assisted-by: GPT-6 Signed-off-by: Hidde Beydals <hidde@hhh.computer>

Hard reset and forced checkout could not recover from an unreadable old HEAD tree because they read it to identify tracked deletions. The old index provides those paths, including staged additions absent from the target. KeepReset still needs the old HEAD tree for its conflict checks. Recovery requires a readable target and index; it does not make updates atomic. Assisted-by: GPT-6 Signed-off-by: Hidde Beydals <hidde@hhh.computer>

Resetting the index diffed the target tree against the index as the worktree sees it, which leaves out the entries flagged SkipWorktree. A path outside the sparse directories kept the hash it held before the reset, or stayed in the index after the target dropped it, and the reset still reported success. Committing afterwards would have restored the path's earlier content. The index records every path the target holds, so the diff that brings it to the target reports skipped entries too. Rewriting an entry carries the flag over, because the entries are the only record of the sparse state once a reset does not restate SparseDirs, and the flag still decides which paths reach the worktree. Assisted-by: Claude Opus 5 (1M context) Signed-off-by: Hidde Beydals <hidde@hhh.computer>

hiddeco · 2026-09-10T22:24:28Z

+	// flagged SkipWorktree. Callers that bring the index to a tree need
+	// those entries; callers that compare the index with the worktree must
+	// leave it unset, because a skipped path is absent from disk by design.
+	IgnoreSkipWorktree bool


Not too keen on the naming here, and open to alternatives.

Copilot AI lite review requested due to automatic review settings September 10, 2026 13:20

Copilot started reviewing on behalf of hiddeco September 10, 2026 13:21 View session

hiddeco force-pushed the fix/tree-traversal-and-worktree-recovery branch from 0b22c86 to b8668da Compare September 10, 2026 13:21

Copilot AI reviewed Sep 10, 2026

View reviewed changes

hiddeco added 6 commits September 10, 2026 16:03

hiddeco force-pushed the fix/tree-traversal-and-worktree-recovery branch from b8668da to f6bd1ba Compare September 10, 2026 14:05

hiddeco force-pushed the fix/tree-traversal-and-worktree-recovery branch from f6bd1ba to 105fffb Compare September 10, 2026 14:05

hiddeco commented Sep 10, 2026

View reviewed changes

Navigation Menu

git: worktree, Fix unreadable subtree handling and recovery - #2372

git: worktree, Fix unreadable subtree handling and recovery#2372
hiddeco wants to merge 8 commits into
mainfrom
fix/tree-traversal-and-worktree-recovery

hiddeco commented Sep 10, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

hiddeco Sep 10, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		if errors.Is(cause, io.EOF) {
		return fmt.Errorf("entry %q has mode %s: %v", e.Name, e.Mode, cause)

Conversation

hiddeco commented Sep 10, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Suppressed comments (1)

Uh oh!

hiddeco Sep 10, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants