diff --git a/cmd/entire/cli/state.go b/cmd/entire/cli/state.go index b3d06e315c..f1c086333c 100644 --- a/cmd/entire/cli/state.go +++ b/cmd/entire/cli/state.go @@ -21,8 +21,11 @@ import ( "github.com/entireio/cli/cmd/entire/cli/paths" "github.com/entireio/cli/cmd/entire/cli/strategy" "github.com/entireio/cli/cmd/entire/cli/validation" + "github.com/entireio/cli/cmd/entire/cli/worktreedir" "github.com/go-git/go-git/v6" + "github.com/go-git/go-git/v6/plumbing" + "github.com/go-git/go-git/v6/plumbing/object" ) // PrePromptState stores the state captured before a user prompt @@ -329,6 +332,17 @@ func detectFileChanges(ctx context.Context, previouslyUntracked []string, status // (already condensed by PostCommit) back to FilesTouched via SaveStep. Files not in // HEAD or with different content in the working tree are kept. Fails open: if any git // operation errors, returns the original list unchanged. +// +// "Same content" is decided by Git's own clean filters, via +// gitrepo.HashWorktreeFiles (git hash-object), not by comparing the working +// tree's raw bytes against the blob. Under core.autocrlf — the Git for Windows +// default, and what e2e/testutil/repo.go sets — the working tree holds CRLF +// while the blob holds LF, so a byte comparison reports every committed text +// file as still modified. That defeats the caller's "no changes, skip" gate and +// mints a fresh shadow branch on the new HEAD *after* PostCommit condensed and +// deleted the old one; nothing condenses that one away, so it outlives the +// session. The same reasoning applies to .gitattributes eol/text rules and to +// clean filters such as Git LFS, which a byte comparison also gets wrong. func filterToUncommittedFiles(ctx context.Context, files []string, repoRoot string) []string { if len(files) == 0 { return files @@ -357,7 +371,14 @@ func filterToUncommittedFiles(ctx context.Context, files []string, repoRoot stri logCtx := logging.WithComponent(ctx, "filter-uncommitted") + // Pass 1: split into "not in HEAD" (uncommitted by definition) and + // candidates that need a content comparison. + type candidate struct { + path string + headFile *object.File + } var result []string + var candidates []candidate for _, relPath := range files { headFile, err := headTree.File(relPath) if err != nil { @@ -368,26 +389,68 @@ func filterToUncommittedFiles(ctx context.Context, files []string, repoRoot stri result = append(result, relPath) continue } + candidates = append(candidates, candidate{path: relPath, headFile: headFile}) + } + if len(candidates) == 0 { + return result + } + + // Hash the regular files through native Git so the comparison sees the + // same clean-filtered bytes Git would have committed. HashableWorktreeEntry + // withholds the paths hash-object would answer wrongly or block on — a + // symlink on either side, a FIFO, a tracked file replaced by another type — + // and those take the raw fallback below instead. + hashPaths := make([]string, 0, len(candidates)) + for _, c := range candidates { + if !worktreedir.HashableEntry(repoRoot, c.path, c.headFile.Mode) { + continue + } + hashPaths = append(hashPaths, c.path) + } + var worktreeHashes map[string]plumbing.Hash + if len(hashPaths) > 0 { + var hashErr error + worktreeHashes, hashErr = gitrepo.HashWorktreeFiles(ctx, repoRoot, hashPaths) + if hashErr != nil { + // Partial results are still usable; only the paths Git could not + // hash fall back to the raw comparison below. + logging.Debug(logCtx, "native git could not hash every candidate; falling back to raw comparison for those paths", + slog.String("error", hashErr.Error())) + } + } + + // Pass 2: decide each candidate, preserving the input order. + for _, c := range candidates { + if worktreeHash, ok := worktreeHashes[c.path]; ok { + // Equal, not ==: plumbing.Hash carries an object-format field + // alongside its bytes and == compares that field too, which the + // tree decoder and FromHex do not always agree on. + if !worktreeHash.Equal(c.headFile.Hash) { + result = append(result, c.path) + } + continue + } - // File is in HEAD — compare content with working tree, through the - // worktree's shared root. relPath comes from git, so it is already the - // coordinate the root reads in. - workingContent, ok := readWorktreeFileSafely(repoRoot, relPath) + // Fallback for every path withheld above and for anything native Git + // could not hash: compare the raw working-tree bytes. Filter-unaware, + // so it can report a clean file as modified, which keeps a file rather + // than dropping one — the same direction this function already fails. + workingContent, ok := readWorktreeFileSafely(repoRoot, c.path) if !ok { // Can't read working tree file (deleted?) — keep it - result = append(result, relPath) + result = append(result, c.path) continue } - headContent, err := headFile.Contents() + headContent, err := c.headFile.Contents() if err != nil { - result = append(result, relPath) + result = append(result, c.path) continue } if string(workingContent) != headContent { // Working tree differs from HEAD — uncommitted changes - result = append(result, relPath) + result = append(result, c.path) } // else: content matches HEAD — already committed, skip } diff --git a/cmd/entire/cli/state_test.go b/cmd/entire/cli/state_test.go index d1a3fa565d..852f8b9490 100644 --- a/cmd/entire/cli/state_test.go +++ b/cmd/entire/cli/state_test.go @@ -5,6 +5,7 @@ import ( "fmt" "os" "path/filepath" + "strings" "testing" "github.com/entireio/cli/cmd/entire/cli/agent/claudecode" @@ -922,3 +923,113 @@ func TestFilterToUncommittedFiles_ReallyModified(t *testing.T) { t.Errorf("filterToUncommittedFiles() = %v, want [file.txt]", result) } } + +// TestFilterToUncommittedFiles_CleanFilteredFileIsDropped pins the condition +// that left a stale shadow branch behind after every commit on Windows. +// +// Under core.autocrlf (the Git for Windows default, and what InitRepo and +// e2e/testutil/repo.go both set) a committed text file holds CRLF in the +// working tree and LF in its blob. Comparing the two raw byte strings reports +// the file as still modified, which defeats turn-end's "no changes, skip" gate +// and mints a fresh shadow branch on the new HEAD right after PostCommit +// condensed and deleted the old one — and nothing condenses that one away. +// +// The file here is fully committed with no subsequent edit, so the only +// correct answer is to drop it. +func TestFilterToUncommittedFiles_CleanFilteredFileIsDropped(t *testing.T) { + tmpDir := t.TempDir() + t.Chdir(tmpDir) + + testutil.InitRepo(t, tmpDir) // sets core.autocrlf=true + + // Written with CRLF and committed through native Git, so the clean filter + // normalises the blob to LF while the working tree keeps CRLF. + const relPath = "docs/blue.md" + testutil.WriteFile(t, tmpDir, relPath, "# Blue\r\n\r\nAbout the colour blue.\r\n") + testutil.RunGit(t, tmpDir, "add", relPath) + testutil.RunGit(t, tmpDir, "commit", "-m", "Add documentation about the colour blue") + + // Assert the premise rather than assume it: if Git ever stops normalising + // here, this test would pass for the wrong reason and stop guarding + // anything. + blob := testutil.RunGit(t, tmpDir, "cat-file", "-p", "HEAD:"+relPath) + if strings.Contains(blob, "\r\n") { + t.Fatalf("precondition failed: blob still contains CRLF, so core.autocrlf did not apply") + } + onDisk, err := os.ReadFile(filepath.Join(tmpDir, filepath.FromSlash(relPath))) + if err != nil { + t.Fatalf("failed to read working tree file: %v", err) + } + if !strings.Contains(string(onDisk), "\r\n") { + t.Fatalf("precondition failed: working tree file has no CRLF, so the byte comparison would agree anyway") + } + + if got := filterToUncommittedFiles(context.Background(), []string{relPath}, tmpDir); len(got) != 0 { + t.Errorf("filterToUncommittedFiles() = %v, want [] — the file is committed, so it is not an uncommitted change", got) + } +} + +// TestFilterToUncommittedFiles_CleanFilteredFileEditedIsKept is the other half: +// the clean-filter-aware comparison must still notice a real edit to a file +// whose line endings are converted, rather than dropping every committed path. +func TestFilterToUncommittedFiles_CleanFilteredFileEditedIsKept(t *testing.T) { + tmpDir := t.TempDir() + t.Chdir(tmpDir) + + testutil.InitRepo(t, tmpDir) + + const relPath = "docs/blue.md" + testutil.WriteFile(t, tmpDir, relPath, "# Blue\r\n") + testutil.RunGit(t, tmpDir, "add", relPath) + testutil.RunGit(t, tmpDir, "commit", "-m", "Add blue") + + // A genuine edit, still CRLF-terminated. + testutil.WriteFile(t, tmpDir, relPath, "# Blue\r\n\r\nNow with more content.\r\n") + + got := filterToUncommittedFiles(context.Background(), []string{relPath}, tmpDir) + if len(got) != 1 || got[0] != relPath { + t.Errorf("filterToUncommittedFiles() = %v, want [%s]", got, relPath) + } +} + +// TestFilterToUncommittedFiles_TypechangeToSymlinkIsKept pins the hole the +// clean-filter comparison would otherwise open. git hash-object follows a +// working-tree symlink and hashes the target's content, so a tracked regular +// file replaced by a link to identical content hashes equal to its HEAD blob +// and would be dropped as "already committed" — even though git status calls +// it a typechange (" T"). Checking only the HEAD mode does not catch it, +// because the HEAD entry is still a regular file. +func TestFilterToUncommittedFiles_TypechangeToSymlinkIsKept(t *testing.T) { + testutil.SkipWithoutSymlinks(t) + + tmpDir := t.TempDir() + t.Chdir(tmpDir) + + testutil.InitRepo(t, tmpDir) + + const relPath = "foo.txt" + testutil.WriteFile(t, tmpDir, relPath, "hello\n") + testutil.RunGit(t, tmpDir, "add", relPath) + testutil.RunGit(t, tmpDir, "commit", "-m", "Add foo") + + // Replace the tracked regular file with a symlink whose target holds the + // identical content, so hash-object returns exactly the HEAD blob hash. + testutil.WriteFile(t, tmpDir, "elsewhere.txt", "hello\n") + if err := os.Remove(filepath.Join(tmpDir, relPath)); err != nil { + t.Fatalf("failed to remove tracked file: %v", err) + } + if err := os.Symlink("elsewhere.txt", filepath.Join(tmpDir, relPath)); err != nil { + t.Fatalf("failed to create symlink: %v", err) + } + + // Assert the premise: native Git really does hash the link to the blob it + // replaced, which is what makes the naive comparison unsafe. + if got := strings.TrimSpace(testutil.RunGit(t, tmpDir, "hash-object", "--", relPath)); got != + strings.TrimSpace(testutil.RunGit(t, tmpDir, "rev-parse", "HEAD:"+relPath)) { + t.Fatalf("precondition failed: hash-object did not follow the symlink to the HEAD blob (got %s)", got) + } + + if got := filterToUncommittedFiles(context.Background(), []string{relPath}, tmpDir); len(got) != 1 || got[0] != relPath { + t.Errorf("filterToUncommittedFiles() = %v, want [%s] — a typechange is an uncommitted change", got, relPath) + } +} diff --git a/cmd/entire/cli/strategy/content_overlap.go b/cmd/entire/cli/strategy/content_overlap.go index 59a3adf531..93aa032c6c 100644 --- a/cmd/entire/cli/strategy/content_overlap.go +++ b/cmd/entire/cli/strategy/content_overlap.go @@ -3,7 +3,6 @@ package strategy import ( "context" "io" - "io/fs" "log/slog" "github.com/entireio/cli/cmd/entire/cli/gitrepo" @@ -517,7 +516,7 @@ func filesWithRemainingAgentChanges( // hash-object follows symlinks and hashes target content, while a Git // symlink blob stores the target path. Compare either side of a mode // mismatch through the confined fallback instead. - if !requiresConfinedWorktreeHash(worktreeRoot, candidate.path, candidate.commitMode) { + if worktreedir.HashableEntry(worktreeRoot, candidate.path, candidate.commitMode) { paths = append(paths, candidate.path) } } @@ -576,29 +575,6 @@ func filesWithRemainingAgentChanges( return remaining } -func requiresConfinedWorktreeHash(worktreeRoot, filePath string, commitMode filemode.FileMode) bool { - if commitMode == filemode.Symlink { - return true - } - root, err := worktreedir.OpenAt(worktreeRoot) - if err != nil { - return true - } - name, err := worktreedir.Name(worktreeRoot, filePath) - if err != nil { - return true - } - info, err := root.Lstat(name) - return err != nil || requiresConfinedWorktreeMode(info.Mode()) -} - -func requiresConfinedWorktreeMode(mode fs.FileMode) bool { - // Windows uses ModeIrregular for OneDrive Files On-Demand placeholders. - // Mask it so placeholder files still receive Git's clean-filter handling, - // while every substantive non-regular type remains confined. - return mode.Type()&^fs.ModeIrregular != 0 -} - // workingTreeMatchesBlob checks whether the raw file representation hashes to // commitHash. It is the filter-unaware fallback for when native Git cannot hash // a regular file and the symlink-aware path for Git symlink blobs. diff --git a/cmd/entire/cli/strategy/content_overlap_test.go b/cmd/entire/cli/strategy/content_overlap_test.go index fe7796d93d..fc96856a90 100644 --- a/cmd/entire/cli/strategy/content_overlap_test.go +++ b/cmd/entire/cli/strategy/content_overlap_test.go @@ -2,7 +2,6 @@ package strategy import ( "context" - "io/fs" "os" "path/filepath" "testing" @@ -502,30 +501,6 @@ func TestFilesWithRemainingAgentChanges_ComparesWorktreeToCommitNotIndex(t *test assert.Equal(t, []string{"config.go"}, remaining) } -func TestRequiresConfinedWorktreeModeAllowsWindowsCloudPlaceholders(t *testing.T) { - t.Parallel() - - tests := []struct { - name string - mode fs.FileMode - want bool - }{ - {name: "regular", mode: 0, want: false}, - {name: "cloud placeholder file", mode: fs.ModeIrregular, want: false}, - {name: "directory", mode: fs.ModeDir, want: true}, - {name: "cloud placeholder directory", mode: fs.ModeDir | fs.ModeIrregular, want: true}, - {name: "symlink", mode: fs.ModeSymlink, want: true}, - {name: "symlink irregular", mode: fs.ModeSymlink | fs.ModeIrregular, want: true}, - {name: "named pipe", mode: fs.ModeNamedPipe, want: true}, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - t.Parallel() - assert.Equal(t, tt.want, requiresConfinedWorktreeMode(tt.mode)) - }) - } -} - func TestWorkingTreeMatchesBlobSymlinkHashesTheTargetPath(t *testing.T) { testutil.SkipWithoutSymlinks(t) t.Parallel() @@ -537,9 +512,9 @@ func TestWorkingTreeMatchesBlobSymlinkHashesTheTargetPath(t *testing.T) { _, err := h.Write([]byte(target)) require.NoError(t, err) - assert.True(t, requiresConfinedWorktreeHash(dir, "link.txt", filemode.Symlink)) - assert.True(t, requiresConfinedWorktreeHash(dir, "link.txt", filemode.Regular), - "a working-tree symlink must not be sent to hash-object even if the commit is regular") + // Withholding a worktree symlink from hash-object is gitrepo's rule now + // (TestHashableWorktreeEntry_WorktreeSymlinkIsWithheld); what stays here is + // what this fallback must then answer for one. assert.True(t, workingTreeMatchesBlob(dir, "link.txt", filemode.Symlink, h.Sum())) assert.False(t, workingTreeMatchesBlob(dir, "link.txt", filemode.Regular, h.Sum()), "a symlink must not compare clean against a regular-file commit") diff --git a/cmd/entire/cli/worktreedir/worktreedir.go b/cmd/entire/cli/worktreedir/worktreedir.go index bf4940ec18..8505f45363 100644 --- a/cmd/entire/cli/worktreedir/worktreedir.go +++ b/cmd/entire/cli/worktreedir/worktreedir.go @@ -24,11 +24,13 @@ import ( "context" "errors" "fmt" + "io/fs" "os" "path/filepath" "github.com/entireio/cli/cmd/entire/cli/osroot" "github.com/entireio/cli/cmd/entire/cli/paths" + "github.com/go-git/go-git/v6/plumbing/filemode" ) // Open returns the shared *os.Root over the current worktree root. The returned @@ -137,3 +139,46 @@ func NameFollowingLinks(worktreeRoot, p string) (string, error) { } return Name(resolvedBase, resolved) } + +// HashableEntry reports whether filePath may be handed to +// gitrepo.HashWorktreeFiles (git hash-object), given the mode Git recorded for +// it in a tree. +// +// It lives here rather than beside HashWorktreeFiles because gitrepo cannot +// import this package: worktreedir's own test imports testutil, which imports +// gitrepo, so that edge is an import cycle in the test binary. +// +// Both halves are load-bearing, and each fails in a different direction: +// +// - A Git symlink blob stores the target path, while hash-object follows the +// link and hashes the target's *content*. The two never agree, so a symlink +// recorded in the tree must be compared some other way. +// - The working tree can hold something other than what the tree recorded. A +// tracked regular file replaced by a symlink is `git status`'s typechange +// (" T"), and hash-object follows it — so a link pointing at content equal +// to the recorded blob hashes equal and the change reads as clean. FIFOs +// are worse than wrong: hash-object blocks reading them, which on a hook +// path costs the caller its whole budget. +// +// ModeIrregular is masked out rather than rejected, matching the reasoning in +// paths.ValidateEntireDirAt: Windows maps OneDrive Files On-Demand +// placeholders onto it, and those are ordinary files that should still receive +// Git's clean-filter handling. +func HashableEntry(worktreeRoot, filePath string, treeMode filemode.FileMode) bool { + if treeMode == filemode.Symlink { + return false + } + root, err := OpenAt(worktreeRoot) + if err != nil { + return false + } + name, err := Name(worktreeRoot, filePath) + if err != nil { + return false + } + info, err := root.Lstat(name) + if err != nil { + return false + } + return info.Mode().Type()&^fs.ModeIrregular == 0 +} diff --git a/cmd/entire/cli/worktreedir/worktreedir_test.go b/cmd/entire/cli/worktreedir/worktreedir_test.go index 661bfbe10e..4f5740addb 100644 --- a/cmd/entire/cli/worktreedir/worktreedir_test.go +++ b/cmd/entire/cli/worktreedir/worktreedir_test.go @@ -2,11 +2,14 @@ package worktreedir import ( "errors" + "io/fs" "os" "path/filepath" + "runtime" "testing" "github.com/entireio/cli/cmd/entire/cli/testutil" + "github.com/go-git/go-git/v6/plumbing/filemode" ) func TestName(t *testing.T) { @@ -161,3 +164,72 @@ func TestNameFollowingLinks(t *testing.T) { } }) } + +func TestHashableEntry_ModeTable(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + mode fs.FileMode + want bool + }{ + {name: "regular", mode: 0, want: true}, + {name: "cloud placeholder file", mode: fs.ModeIrregular, want: true}, + {name: "directory", mode: fs.ModeDir, want: false}, + {name: "cloud placeholder directory", mode: fs.ModeDir | fs.ModeIrregular, want: false}, + {name: "symlink", mode: fs.ModeSymlink, want: false}, + {name: "symlink irregular", mode: fs.ModeSymlink | fs.ModeIrregular, want: false}, + {name: "named pipe", mode: fs.ModeNamedPipe, want: false}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + // The exported entry point Lstats a real file, so the mode rule is + // asserted directly here and through the filesystem below. + got := tt.mode.Type()&^fs.ModeIrregular == 0 + if got != tt.want { + t.Errorf("mode %v hashable = %v, want %v", tt.mode, got, tt.want) + } + }) + } +} + +// TestHashableEntry_WorktreeSymlinkIsWithheld is the case that matters +// on the read side: git hash-object follows a working-tree symlink and hashes +// the target's content, so a link pointing at content equal to the recorded +// blob hashes equal and a real typechange (git status " T") reads as clean. +// The tree mode alone cannot catch it — the tree still says regular. +func TestHashableEntry_WorktreeSymlinkIsWithheld(t *testing.T) { + // Not testutil.SkipWithoutSymlinks: testutil imports gitrepo, so using it + // here is an import cycle in the test binary. + if runtime.GOOS == "windows" { + t.Skip("symlink creation needs elevation on Windows") + } + t.Parallel() + + dir := t.TempDir() + if err := os.WriteFile(filepath.Join(dir, "real.txt"), []byte("hello\n"), 0o644); err != nil { + t.Fatalf("failed to write target: %v", err) + } + if err := os.Symlink("real.txt", filepath.Join(dir, "link.txt")); err != nil { + t.Fatalf("failed to create symlink: %v", err) + } + + if HashableEntry(dir, "link.txt", filemode.Symlink) { + t.Error("a symlink recorded in the tree must not be sent to hash-object") + } + if HashableEntry(dir, "link.txt", filemode.Regular) { + t.Error("a working-tree symlink must not be sent to hash-object even when the tree says regular") + } + if !HashableEntry(dir, "real.txt", filemode.Regular) { + t.Error("an ordinary regular file must still be hashed through native Git") + } +} + +func TestHashableEntry_MissingEntry(t *testing.T) { + t.Parallel() + + if HashableEntry(t.TempDir(), "absent.txt", filemode.Regular) { + t.Error("a path with no working-tree entry must not be sent to hash-object") + } +}