From 2db7189067110728c8ff1aac6bb67e9160ee3a6e Mon Sep 17 00:00:00 2001 From: Dai Ha Date: Sat, 15 Aug 2026 15:36:06 +0200 Subject: [PATCH] CB-587: seed the snapshot's temp index from the worktree's real index --skip-worktree is an index flag. GitWorktrees.snapshot staged into a fresh empty temp index, which carried none of the real index's skip-worktree bits, so add -A staged local on-disk content for files git status correctly hides (e.g. .mcp.json). Copy the real index (resolved via git rev-parse --git-path index, correct for linked worktrees) into the temp index before staging, so add -A skips exactly what git status skips. --- .../ltms/bridged/session/GitWorktrees.java | 49 ++++++++-- .../bridged/session/GitWorktreesTest.java | 98 +++++++++++++++++++ 2 files changed, 139 insertions(+), 8 deletions(-) diff --git a/bridged/src/main/java/dev/ltms/bridged/session/GitWorktrees.java b/bridged/src/main/java/dev/ltms/bridged/session/GitWorktrees.java index 4d9e8bb..db17250 100644 --- a/bridged/src/main/java/dev/ltms/bridged/session/GitWorktrees.java +++ b/bridged/src/main/java/dev/ltms/bridged/session/GitWorktrees.java @@ -220,20 +220,26 @@ public final class GitWorktrees implements Worktrees { } /** - * CB-578 stage C. Stages into a temporary index (never the worktree's real one, which - * the worker may still be writing to), writes that index to a tree, commits the tree on top of - * the worktree's current HEAD, and points {@code refs/wip/} at the result: + * CB-578 stage C, fixed by CB-587. Stages into a temporary index (never the worktree's + * real one, which the worker may still be writing to) — but that temp index is first seeded + * from the worktree's real one, rather than starting empty: * * 
+     * cp $(git -C worktree rev-parse --git-path index) <temp>
      * GIT_INDEX_FILE=<temp> git -C worktree add -A
      * tree=$(GIT_INDEX_FILE=<temp> git -C worktree write-tree)
      * commit=$(git -C worktree commit-tree $tree -p HEAD -m message)
      * git -C worktree update-ref refs/wip/branch $commit
      *
* - * {@code add -A} (never {@code -f}) respects {@code .gitignore} exactly as it would in the real - * index — a gitignored file staying ignored is what keeps secrets and local config out of the - * snapshot's tree. The temporary index file is removed afterwards regardless of outcome. + * A fresh empty index carries none of the real index's {@code --skip-worktree} / + * {@code --assume-unchanged} bits, so {@code add -A} into it stages a skip-worktree file's local + * on-disk content even though {@code git status --porcelain} correctly hides that file (CB-587). + * Seeding from the real index preserves those bits, so {@code add -A} then skips exactly what + * {@code git status} skips. {@code add -A} (never {@code -f}) also still respects + * {@code .gitignore} exactly as it would in the real index — a gitignored file staying ignored is + * what keeps secrets and local config out of the snapshot's tree. The temporary index file is + * removed afterwards regardless of outcome; the worker's real index is never opened for writing. */ @Override public Optional snapshot(String worktreePath, String branch, String message) { @@ -244,13 +250,18 @@ public final class GitWorktrees implements Worktrees { Path tempIndex; try { tempIndex = Files.createTempFile("bridged-wip-index-", ".tmp"); - Files.delete(tempIndex); // git creates it fresh under GIT_INDEX_FILE; a stale empty - // file at that path is otherwise treated as a corrupt index. } catch (IOException e) { throw new WorktreeException("cannot create a temporary index for snapshot: " + e.getMessage(), e); } Map indexEnv = Map.of("GIT_INDEX_FILE", tempIndex.toAbsolutePath().toString()); try { + Path realIndex = resolveRealIndex(worktreePath); + try { + Files.copy(realIndex, tempIndex, StandardCopyOption.REPLACE_EXISTING); + } catch (IOException e) { + throw new WorktreeException("cannot copy the worktree's real index (" + realIndex + + ") into the temporary snapshot index: " + e.getMessage(), e); + } exec(indexEnv, "git", "-C", worktreePath, "add", "-A"); String tree = exec(indexEnv, "git", "-C", worktreePath, "write-tree").trim(); String commit = exec("git", "-C", worktreePath, "commit-tree", tree, "-p", "HEAD", "-m", message).trim(); @@ -266,6 +277,28 @@ public final class GitWorktrees implements Worktrees { } } + /** + * Resolve the path of {@code worktreePath}'s real index. Never assume {@code /.git/index}: + * in a linked worktree {@code .git} is a file pointing at the main repo's + * {@code worktrees//} directory, and that is where the real per-worktree index lives. + * {@code git rev-parse --git-path index} resolves this correctly for both a linked worktree and + * the main checkout. Throws {@link WorktreeException} — same as every other failure in this + * class — if the command fails or the resolved path does not exist, rather than silently + * snapshotting from an empty index. + */ + private Path resolveRealIndex(String worktreePath) { + String out = exec("git", "-C", worktreePath, "rev-parse", "--git-path", "index").trim(); + Path index = Path.of(out); + if (!index.isAbsolute()) { + index = Path.of(worktreePath).resolve(index).normalize(); + } + if (!Files.exists(index)) { + throw new WorktreeException("worktree's real index not found at resolved path " + index + + " (git rev-parse --git-path index reported '" + out + "')"); + } + return index; + } + /** Resolve the directory that will hold per-session worktree checkouts. */ private Path resolveRoot(String repoRoot) { if (configuredRoot != null && !configuredRoot.isBlank()) { diff --git a/bridged/src/test/java/dev/ltms/bridged/session/GitWorktreesTest.java b/bridged/src/test/java/dev/ltms/bridged/session/GitWorktreesTest.java index a90df7a..86f9f61 100644 --- a/bridged/src/test/java/dev/ltms/bridged/session/GitWorktreesTest.java +++ b/bridged/src/test/java/dev/ltms/bridged/session/GitWorktreesTest.java @@ -5,8 +5,10 @@ import org.junit.jupiter.api.io.TempDir; import java.nio.file.Files; import java.nio.file.Path; +import java.util.HashSet; import java.util.List; import java.util.Optional; +import java.util.Set; import java.util.concurrent.TimeUnit; import static org.junit.jupiter.api.Assertions.*; @@ -97,6 +99,36 @@ class GitWorktreesTest { return out; } + /** The set of paths in {@code git diff --name-only from..to} — used to check exactly what a + * snapshot's tree changed relative to its parent, the same shape {@code git status --porcelain} + * reports for the worktree it was taken from. */ + private static Set diffNameOnly(Path cwd, String from, String to) throws Exception { + Process p = new ProcessBuilder("git", "-C", cwd.toString(), "diff", "--name-only", from, to) + .redirectErrorStream(true).start(); + String out = new String(p.getInputStream().readAllBytes()); + assertTrue(p.waitFor(30, TimeUnit.SECONDS), "git diff timed out"); + assertEquals(0, p.exitValue(), "git diff " + from + ".." + to + " failed:\n" + out); + Set paths = new HashSet<>(); + for (String line : out.split("\\R")) { + if (!line.isBlank()) { + paths.add(line.trim()); + } + } + return paths; + } + + /** The set of paths a {@code git status --porcelain} listing names, stripping the two-char status + * code prefix each line carries. */ + private static Set porcelainPaths(String porcelain) { + Set paths = new HashSet<>(); + for (String line : porcelain.split("\\R")) { + if (!line.isBlank()) { + paths.add(line.substring(3).trim()); + } + } + return paths; + } + private static String forEachRef(Path cwd, String pattern) throws Exception { Process p = new ProcessBuilder("git", "-C", cwd.toString(), "for-each-ref", pattern) .redirectErrorStream(true).start(); @@ -376,4 +408,70 @@ class GitWorktreesTest { assertTrue(gitWorktrees.snapshot(gone, "some-branch", "msg").isEmpty(), "a missing worktree has nothing to snapshot, and must not throw"); } + + /** + * CB-587, acceptance criteria 1 and 2. A file with a REAL {@code --skip-worktree} bit set in the + * worktree's own index must never enter the snapshot, even though its on-disk content has locally + * diverged from what is committed — that is exactly the divergence {@code --skip-worktree} exists + * to hide from {@code git status}, and a snapshot built from a fresh empty temp index (the bug) + * stages that local content anyway because the fresh index carries none of the real index's flags. + * The snapshot's diff against its parent must list exactly what {@code git status --porcelain} + * reports for the worktree — no more, no less. + */ + @Test + void dirtySnapshotHonoursARealSkipWorktreeBit(@TempDir Path tmp) throws Exception { + Path repo = tmp.resolve("repo"); + Files.createDirectories(repo); + git(repo, "init", "-q", "-b", "main"); + git(repo, "config", "user.email", "test@example.invalid"); + git(repo, "config", "user.name", "Test"); + Files.writeString(repo.resolve("protected.cfg"), "committed-value\n"); + Files.writeString(repo.resolve("README.md"), "seed\n"); + git(repo, "add", "protected.cfg", "README.md"); + git(repo, "commit", "-q", "-m", "seed"); + + GitWorktrees gitWorktrees = new GitWorktrees(tmp.resolve("wts").toString()); + String branch = "cb-587-a"; + String wt = gitWorktrees.add(repo.toString(), branch, "HEAD"); + + git(Path.of(wt), "update-index", "--skip-worktree", "protected.cfg"); + Files.writeString(Path.of(wt).resolve("protected.cfg"), "locally-diverged-never-commit\n"); + Files.writeString(Path.of(wt).resolve("untracked.txt"), "draft that was never added\n"); + Files.writeString(Path.of(wt).resolve("README.md"), "edited tracked file\n"); + + String porcelain = fullStatus(Path.of(wt)); + assertFalse(porcelain.contains("protected.cfg"), + "test setup invalid — protected.cfg must not show in git status once skip-worktree is set:\n" + + porcelain); + + Optional ref = gitWorktrees.snapshot(wt, branch, "test snapshot"); + + assertTrue(ref.isPresent(), "a dirty worktree snapshot returns a commit sha"); + Set diffPaths = diffNameOnly(repo, "HEAD", "refs/wip/" + branch); + assertFalse(diffPaths.contains("protected.cfg"), + "a --skip-worktree file's local drift leaked into the snapshot:\n" + diffPaths); + assertEquals(porcelainPaths(porcelain), diffPaths, + "snapshot diff must list exactly what git status --porcelain reports, no more, no less"); + } + + /** + * CB-587, acceptance criterion 6. If the worktree's real index cannot be resolved/read, snapshot + * must fail loudly (throw) rather than silently falling back to an empty temp index and producing + * a wrong snapshot. SessionManager's caller already catches and WARNs on any exception here — this + * only needs to confirm the failure is not swallowed inside snapshot() itself. + */ + @Test + void snapshotThrowsWhenTheWorktreesRealIndexCannotBeResolved(@TempDir Path tmp) throws Exception { + Path repo = initRepo(tmp.resolve("repo")); + GitWorktrees gitWorktrees = new GitWorktrees(tmp.resolve("wts").toString()); + String branch = "cb-587-b"; + String wt = gitWorktrees.add(repo.toString(), branch, "HEAD"); + + // Break git's ability to resolve the worktree's real index by removing the linked worktree's + // `.git` file (which normally points at the main repo's worktrees// directory). + Files.delete(Path.of(wt).resolve(".git")); + + assertThrows(WorktreeException.class, () -> gitWorktrees.snapshot(wt, branch, "test snapshot"), + "an unresolvable real index must fail loudly, not silently snapshot from an empty index"); + } }