From 66859260b0e62cf6e1093247648a484a9946660c Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 14 Jun 2026 09:25:27 +0000 Subject: [PATCH] fix(review-diff): discard a fully-staged file back to HEAD MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit File-level Discard (`D`) in Review Diff ran `git checkout -- `, which only rewrites the working tree from the index. When a file's change is entirely staged (clean working tree), that command is a no-op, yet the status bar still reported `Discarded: ` — telling the user an irreversible discard succeeded while the staged change remained in the index, on disk, and in the diff. Reset the file all the way back to its committed state with `git checkout HEAD -- `, which restores both the index entry and the working tree to HEAD, so staged and unstaged edits alike are dropped. Also honor the command's exit code: only claim "Discarded" when it actually succeeded, otherwise surface "Discard failed" rather than a false success. Adds an e2e test that stages a change (clean working tree), discards the file via `D`, and asserts the change disappears from the review diff and that neither the working tree nor the index retains it. The test times out against the old `git checkout -- ` path. Fixes #2318 --- crates/fresh-editor/plugins/audit_mode.ts | 25 ++++-- .../tests/e2e/plugins/audit_mode.rs | 83 +++++++++++++++++++ 2 files changed, 103 insertions(+), 5 deletions(-) diff --git a/crates/fresh-editor/plugins/audit_mode.ts b/crates/fresh-editor/plugins/audit_mode.ts index f5751ffba4..6a1985c8ba 100644 --- a/crates/fresh-editor/plugins/audit_mode.ts +++ b/crates/fresh-editor/plugins/audit_mode.ts @@ -5110,13 +5110,28 @@ editor.on("prompt_confirmed", async (args) => { const f = pendingDiscardFile; if (f) { const cwd = gitCwd(); - if (f.category === 'untracked') { - await editor.spawnProcess("rm", ["--", f.path], cwd); + // "Discard changes in file" means lose the file's changes entirely + // and bring it back to its committed (HEAD) state. For tracked + // files this must reset BOTH the index and the working tree: + // `git checkout -- ` only rewrites the working tree from the + // index, so a fully-staged change (clean working tree) would slip + // through untouched while we still reported success. `git checkout + // HEAD -- ` restores the index entry and the working tree to + // HEAD, dropping staged and unstaged edits alike. + const result = f.category === 'untracked' + ? await editor.spawnProcess("rm", ["--", f.path], cwd) + : await editor.spawnProcess("git", ["checkout", "HEAD", "--", f.path], cwd); + await refreshMagitData(); + // Only claim a discard happened when the command actually + // succeeded — never tell the user an irreversible discard worked + // when it didn't. + if (result.exit_code === 0) { + editor.setStatus(`Discarded: ${f.path}`); } else { - await editor.spawnProcess("git", ["checkout", "--", f.path], cwd); + editor.setStatus( + `Discard failed: ${(result.stderr || "").trim() || f.path}`, + ); } - await refreshMagitData(); - editor.setStatus(`Discarded: ${f.path}`); } } else { editor.setStatus("Discard cancelled"); diff --git a/crates/fresh-editor/tests/e2e/plugins/audit_mode.rs b/crates/fresh-editor/tests/e2e/plugins/audit_mode.rs index 5cfae4e3fd..ecb40cc1eb 100644 --- a/crates/fresh-editor/tests/e2e/plugins/audit_mode.rs +++ b/crates/fresh-editor/tests/e2e/plugins/audit_mode.rs @@ -4450,3 +4450,86 @@ fn test_review_branch_detail_enter_opens_file_at_commit() { .unwrap(); harness.wait_until(|h| !file_view_active(h)).unwrap(); } + +/// Issue #2318: file-level Discard (`D`) on a fully-*staged* file reported +/// "Discarded" but left the staged change intact. The discard ran +/// `git checkout -- `, which only rewrites the working tree from the +/// index — so a change that lives entirely in the index (clean working tree) +/// survived untouched while the status bar still claimed success. The fix +/// resets the file all the way back to HEAD (`git checkout HEAD -- `), +/// dropping both the staged and any unstaged edits. +#[test] +fn test_issue2318_discard_fully_staged_file_reverts_to_head() { + init_tracing_from_env(); + let repo = GitTestRepo::new(); + setup_audit_mode_plugin(&repo); + + // Commit the original file, then make a change and STAGE it so the file + // is fully staged with a clean working tree (`git status` → "M "). + let calc = repo.create_file("calc.py", "def add(a, b):\n return a + b\n"); + repo.git_add_all(); + repo.git_commit("Initial commit"); + repo.create_file("calc.py", "def add(a, b):\n return a + b # CHANGED\n"); + repo.git_add(&["calc.py"]); + + let mut harness = EditorTestHarness::with_config_and_working_dir( + 120, + 30, + Config::default(), + repo.path.clone(), + ) + .unwrap(); + harness.open_file(&calc).unwrap(); + harness.render().unwrap(); + harness + .wait_until(|h| h.screen_to_string().contains("# CHANGED")) + .unwrap(); + + let screen = open_review_diff(&mut harness); + // The change is staged, so it shows under the STAGED section. + assert!( + screen.contains("STAGED") && screen.contains("# CHANGED"), + "The staged change should appear under STAGED. Screen:\n{}", + screen + ); + + // Move the cursor down into the file's hunk body, then file-level + // Discard (`D`). + for _ in 0..5 { + harness + .send_key(KeyCode::Char('j'), KeyModifiers::NONE) + .unwrap(); + } + harness.render().unwrap(); + harness + .send_key(KeyCode::Char('D'), KeyModifiers::SHIFT) + .unwrap(); + harness.wait_for_prompt().unwrap(); + // Default suggestion (index 0) is "Discard changes in file". + harness + .send_key(KeyCode::Enter, KeyModifiers::NONE) + .unwrap(); + harness.wait_for_prompt_closed().unwrap(); + + // After the discard refreshes the view, the change must be gone from the + // diff stream — nothing is left staged or unstaged. + harness + .wait_until(|h| !h.screen_to_string().contains("# CHANGED")) + .expect("Discarding the fully-staged file should remove its change from the review diff"); + + // The discard must reach the index *and* the working tree: the file on + // disk is back to its committed contents and nothing remains staged. + let on_disk = fs::read_to_string(&calc).unwrap(); + assert!( + !on_disk.contains("# CHANGED"), + "Issue #2318: the working tree should be reverted to HEAD. Got: {on_disk:?}" + ); + let cached = git_command(&repo.path) + .args(["diff", "--cached", "--quiet"]) + .status() + .expect("git diff --cached failed to run"); + assert!( + cached.success(), + "Issue #2318: nothing should remain staged after discarding the file" + ); +}