From dcfa50319c597e955726f88ab59b37ff158dbf1f Mon Sep 17 00:00:00 2001 From: Valdemar Lorenzen Date: Wed, 14 Oct 2026 15:32:52 +0200 Subject: [PATCH] fix: preserve lazy snapshots for conflicted jj revisions Conflicted revisions read every file through jj file show, which returns empty output for symlinks and causes repository-wide subprocess fanout. Keep ordinary Git blob sources and immutable identities; render only selected conflicted paths through jj. Verified all 905 tests and the original local bookmark comparison with 25 correct paths in 0.522s, plus actual terminal conflict viewing. Strict Clippy is blocked by existing diagnostics outside this change. --- TODO.md | 1 + docs/development.md | 2 + docs/editing.md | 2 + src/repository.rs | 128 +++++++++++++++++++++++--------------------- tests/repository.rs | 96 +++++++++++++++++++++++++++++++++ 5 files changed, 168 insertions(+), 61 deletions(-) diff --git a/TODO.md b/TODO.md index 5ad9a36..b451b5e 100644 --- a/TODO.md +++ b/TODO.md @@ -1,4 +1,5 @@ - [ ] jj conflict resolution with ui akin to the diff viewer +- [ ] make selecting a file in the diff view file picker focus the after buffer instead of the before buffer - [ ] improve the lsp hover floating window: - it should be taller - [ ] add `*` on word to highlights and searches in buffer in one action. diff --git a/docs/development.md b/docs/development.md index 9b41c0c..921a331 100644 --- a/docs/development.md +++ b/docs/development.md @@ -56,6 +56,8 @@ Clipboard checks use isolated providers rather than reading or overwriting the d Diff checks cover staged category/filter navigation, stale request cancellation, row alignment, read-only buffer isolation, and rendered panes. Repository integration fixtures exercise Git and Git-backed jj, advertised but unfetched remote branches/bookmarks/tags, imported historical commit/change IDs, conflicts, unusual paths, binary/mode/link/submodule changes, and unchanged active metadata. Remote fixtures are local bare copies; they never push. Git fixture identities and commit dates are fixed. +Conflicted-revision regressions preserve unchanged symlinks, defer oversized unselected blobs until file selection, compare selected conflict text with jj's projection, and exclude unchanged conflicts when comparing later revisions. Live terminal smoke covers local bookmark selection and opening an actual conflicted file without changing the checkout. + Repository gutter checks cover LF/CRLF normalization, additions/replacements/deletion anchors, staged/disk/unsaved Git text, unborn and missing named files, ignored/unsupported paths, read-only colocated/non-colocated jj queries, stale results and per-buffer isolation, plus rendered modes, colors, current-line precedence, wrap/scroll and narrow/split geometry. Live PTY smoke verifies all four modes, edit/undo refresh, external Git HEAD changes, and jj parent changes without editor restart. Normal-picker checks cover staged/disk/unsaved status, binary and mode changes, exact and ambiguous renames, deleted paths, unborn repositories, aliases and unusual filenames, ignores and safe roots, linked worktrees, and read-only colocated/non-colocated/conflicted jj parents. Catalog/render checks retain unchanged rows, query/selection/expansion and Tree/List navigation, clear stale metadata, restore recreated files, and sanitize rename labels. Live PTY smoke checks A/M/D/R colors in floating and docked pickers with the line gutter disabled, Git HEAD/jj parent refresh, and unchanged jj operation state during polling. diff --git a/docs/editing.md b/docs/editing.md index 21994f8..ca017af 100644 --- a/docs/editing.md +++ b/docs/editing.md @@ -394,6 +394,8 @@ Missing or unprepared historical metadata is reported when selecting the affecte Comparison discovery builds the changed-path/status catalog first. File text, notices, and hunks load asynchronously when a file is selected and remain cached for that comparison. Failed file loads can be retried by selecting the file again; cancelled or superseded results cannot replace the current selection. Working-checkout comparisons still capture working bytes and the historical content needed to identify changes, preserving the frozen Before snapshot and unsaved After buffers. +Conflicted jj revisions also discover files without reading every blob. Ordinary files and symbolic links retain their Git contents; selected conflicted files show jj's rendered conflict projection. Unchanged links and jj's internal merge-tree metadata do not appear as changes. Viewing a conflict does not resolve it or modify the revision. + **Working checkout After follows forward checkouts automatically**, checked asynchronously once per second, including inactive tabs. A descendant jj change or Git HEAD rebuilds the changed-file catalog and captures current working bytes plus eligible unsaved buffers, while keeping the original Before frozen. jj tracking uses change IDs, so snapshot/description rewrites of the same change do not advance the endpoint or prevent following its later descendants. Same, backward, and unrelated checkouts do not rebuild the catalog; live-buffer editing and external reload still work normally. Historical After never follows. Set `config.diff_follow_working_copy = false` to disable following. **Pinned After editability tracks the checkout without moving either endpoint**, checked asynchronously once per second, including inactive tabs and when `config.diff_follow_working_copy = false`. Git matches the selected commit to HEAD; jj matches its change ID so snapshot/description rewrites of the same change remain editable. A matching After uses the shared live buffer. Switching away restores the original read-only After snapshot; switching back reuses the live buffer's unsaved edits and undo history. A failed checkout check revokes pinned After editing until a successful check restores eligibility. diff --git a/src/repository.rs b/src/repository.rs index f16de1e..00c0d5f 100644 --- a/src/repository.rs +++ b/src/repository.rs @@ -1048,6 +1048,8 @@ struct FileData { mode: String, bytes: Vec, object: Option<(PathBuf, String)>, + // Conflicts retain their Git blob identity, but selected contents come from jj. + projection: Option<(PathBuf, String)>, notice: Option, } type Snapshot = BTreeMap; @@ -1766,65 +1768,29 @@ impl Repository { } Ok(None) } - fn conflicted_snapshot(&self, root: &Path, id: &str) -> Result { + fn conflicted_snapshot(&self, root: &Path, store: &Path, id: &str) -> Result { let revset = format!("commit_id({})", serde_json::to_string(id).unwrap()); - let template = r#""[" ++ json(path) ++ "," ++ json(file_type) ++ "," ++ json(executable) ++ "," ++ json(conflict) ++ "]\n""#; + let template = r#""[" ++ json(path) ++ "," ++ json(conflict) ++ "]\n""#; let entries = text(self.jj(root, &["file", "list", "-r", &revset, "-T", template])?)?; + let mut blobs = self.git_snapshot(store, id)?; let mut snapshot = BTreeMap::new(); - let mut total = 0; for entry in entries.lines() { self.check_cancelled()?; - let (path, kind, executable, conflict): (String, String, bool, bool) = - serde_json::from_str(entry) - .map_err(|e| format!("Cannot decode Jujutsu conflict entry: {e}"))?; - let path_buf = PathBuf::from(&path); - safe_path(&path_buf)?; - let mut notice = conflict.then(|| { - "Unresolved Jujutsu conflict: showing CLI-rendered conflict projection".to_owned() - }); - let mode = if kind == "symlink" { - "120000" - } else if kind == "git-submodule" { - "160000" - } else if executable { - "100755" - } else { - "100644" - }; - let bytes = if matches!(kind.as_str(), "file" | "symlink" | "conflict") { - // Use an exact fileset and jj's own conflict materialization. - let pattern = jj_file_pattern(&path); - self.jj( - root, - &["file", "show", "-r", &revset, "-T", "", "--", &pattern], - )? - } else { - notice = Some(format!( - "Unsupported {kind} content in a conflicted revision; nested contents are not traversed" - )); - vec![] - }; - if bytes.len() > FILE_LIMIT { - return Err(format!( - "{} exceeds the 16 MiB comparison file limit", - path_buf.display() - )); - } - total += bytes.len(); - if total > OUTPUT_LIMIT { - return Err( - "Jujutsu conflict projection exceeds the 64 MiB comparison limit".into(), + let (path, conflict): (String, bool) = serde_json::from_str(entry) + .map_err(|e| format!("Cannot decode Jujutsu conflict entry: {e}"))?; + let path = PathBuf::from(path); + safe_path(&path)?; + // jj's logical file list excludes its private merge-tree encoding. + let mut file = blobs + .remove(&path) + .ok_or_else(|| format!("Missing Jujutsu tree entry: {}", path.display()))?; + if conflict { + file.projection = Some((root.to_path_buf(), id.into())); + file.notice = Some( + "Unresolved Jujutsu conflict: showing CLI-rendered conflict projection".into(), ); } - snapshot.insert( - path_buf, - FileData { - mode: mode.into(), - bytes, - object: None, - notice, - }, - ); + snapshot.insert(path, file); } Ok(snapshot) } @@ -1851,9 +1817,13 @@ impl Repository { ], )?)?; if conflict.trim() == "true" { - return self.conflicted_snapshot(root, id); + return self.conflicted_snapshot(root, store, id); } } + self.git_snapshot(store, id) + } + + fn git_snapshot(&self, store: &Path, id: &str) -> Result { let output = self.git(store, &["ls-tree", "-r", "-z", "--full-tree", id])?; let mut snapshot = BTreeMap::new(); for record in output.split(|b| *b == 0).filter(|r| !r.is_empty()) { @@ -1888,6 +1858,7 @@ impl Repository { mode: fields[0].into(), bytes: vec![], object, + projection: None, notice, }, ); @@ -1898,6 +1869,7 @@ impl Repository { fn materialize(&self, snapshot: &mut Snapshot) -> Result<(), String> { let stores: BTreeSet<_> = snapshot .values() + .filter(|file| file.projection.is_none()) .filter_map(|file| file.object.as_ref().map(|(store, _)| store.clone())) .collect(); for store in stores { @@ -1905,6 +1877,7 @@ impl Repository { for file in snapshot.values() { if let Some((source, id)) = &file.object && source == &store + && file.projection.is_none() { request.extend_from_slice(id.as_bytes()); request.push(b'\n'); @@ -1915,10 +1888,11 @@ impl Repository { let data = checked(run(cmd, request, &self.cancelled)?)?; let mut offset = 0; for (path, file) in snapshot.iter_mut() { - if file - .object - .as_ref() - .is_none_or(|(source, _)| source != &store) + if file.projection.is_some() + || file + .object + .as_ref() + .is_none_or(|(source, _)| source != &store) { continue; } @@ -1951,6 +1925,33 @@ impl Repository { offset += size + 1; } } + let mut total = 0; + for (path, file) in snapshot.iter_mut() { + let Some((root, id)) = &file.projection else { + continue; + }; + let revset = format!("commit_id({})", serde_json::to_string(id).unwrap()); + let pattern = jj_file_pattern(path.to_str().ok_or("Non-UTF-8 Jujutsu conflict path")?); + let bytes = self.jj( + root, + &["file", "show", "-r", &revset, "-T", "", "--", &pattern], + )?; + if bytes.len() > FILE_LIMIT { + return Err(format!( + "{} exceeds the 16 MiB comparison file limit", + path.display() + )); + } + total += bytes.len(); + if total > OUTPUT_LIMIT { + return Err( + "Jujutsu conflict projection exceeds the 64 MiB comparison limit".into(), + ); + } + file.bytes = bytes; + file.object = None; + file.projection = None; + } Ok(()) } fn working_snapshot(&self, buffers: &[(PathBuf, String)]) -> Result { @@ -2120,6 +2121,7 @@ impl Repository { mode: "160000".into(), bytes: vec![], object: None, + projection: None, notice: Some(notice), }, ); @@ -2199,6 +2201,7 @@ impl Repository { mode, bytes, object: None, + projection: None, notice, }, ); @@ -2493,8 +2496,8 @@ impl Repository { ) -> Result { let left = &mut prepared.left; let right = &mut prepared.right; - // Frozen checkout/conflict bytes have no object identity. Compare them in a - // single batch, while immutable tree/tree comparisons need only object IDs. + // Frozen checkout bytes have no object identity. Compare them in a single + // batch; immutable blobs and deferred conflict projections retain IDs. let frozen = |file: &FileData| { file.object.is_none() && matches!(file.mode.as_str(), "100644" | "100755" | "120000") }; @@ -2627,10 +2630,12 @@ impl Repository { let mut right = BTreeMap::new(); let before = prepared.left.get(before_path); let after = prepared.right.get(path); - if let Some(data) = before.filter(|data| data.object.is_some()) { + if let Some(data) = before.filter(|data| data.object.is_some() || data.projection.is_some()) + { left.insert(before_path.clone(), clone_file_data(data)); } - if let Some(data) = after.filter(|data| data.object.is_some()) { + if let Some(data) = after.filter(|data| data.object.is_some() || data.projection.is_some()) + { right.insert(path.clone(), clone_file_data(data)); } self.materialize(&mut left)?; @@ -2731,6 +2736,7 @@ fn clone_file_data(file: &FileData) -> FileData { mode: file.mode.clone(), bytes: file.bytes.clone(), object: file.object.clone(), + projection: file.projection.clone(), notice: file.notice.clone(), } } diff --git a/tests/repository.rs b/tests/repository.rs index 8b0f643..8ea55be 100644 --- a/tests/repository.rs +++ b/tests/repository.rs @@ -1053,6 +1053,7 @@ fn jj_conflicted_revisions_show_cli_projection_and_keep_ordinary_files_navigable init_jj(&root, false); fs::write(root.join("conflict"), "base\n").unwrap(); fs::write(root.join("ordinary"), "old\n").unwrap(); + symlink("ordinary", root.join("unchanged-link")).unwrap(); jj(&root, &["describe", "-m", "base"]); let base = jj( &root, @@ -1155,6 +1156,14 @@ fn jj_conflicted_revisions_show_cli_projection_and_keep_ordinary_files_navigable &[], ) .unwrap(); + assert_eq!( + comparison + .files + .iter() + .map(|file| file.path.as_path()) + .collect::>(), + [Path::new("conflict"), Path::new("ordinary")] + ); let conflict = changed(&comparison, Path::new("conflict")); assert_eq!(conflict.after, expected); assert!( @@ -1209,6 +1218,93 @@ fn jj_conflicted_revisions_show_cli_projection_and_keep_ordinary_files_navigable assert_eq!(disk_state(&root), state); } +#[test] +fn conflicted_catalog_preserves_symlinks_and_defers_unselected_contents() { + let fixture = Fixture::new(); + let root = fixture.dir("lazy-conflict"); + init_jj(&root, false); + fs::write(root.join("conflict"), "base\n").unwrap(); + fs::write(root.join("unselected"), "old\n").unwrap(); + symlink("unselected", root.join("unchanged-link")).unwrap(); + jj(&root, &["describe", "-m", "base"]); + let base = jj(&root, &["log", "--no-graph", "-r", "@", "-T", "commit_id"]); + jj(&root, &["new", &base]); + fs::write(root.join("conflict"), "left\n").unwrap(); + fs::write(root.join("unselected"), vec![b'x'; 17 * 1024 * 1024]).unwrap(); + jj(&root, &["describe", "-m", "left"]); + let left = jj(&root, &["log", "--no-graph", "-r", "@", "-T", "commit_id"]); + jj(&root, &["new", &base]); + fs::write(root.join("conflict"), "right\n").unwrap(); + jj(&root, &["describe", "-m", "right"]); + let right = jj(&root, &["log", "--no-graph", "-r", "@", "-T", "commit_id"]); + jj(&root, &["new", &left, &right, "-m", "conflicted merge"]); + let merged = jj(&root, &["log", "--no-graph", "-r", "@", "-T", "commit_id"]); + let state = disk_state(&root); + let mut repository = open(&root); + let comparison = repository + .compare( + 1, + &revision(&base, RepoKind::Jujutsu), + &revision(&merged, RepoKind::Jujutsu), + ComparisonMode::BranchChanges, + &[], + ) + .unwrap(); + assert_eq!( + comparison + .files + .iter() + .map(|file| file.path.as_path()) + .collect::>(), + [Path::new("conflict"), Path::new("unselected")] + ); + let conflict = repository.load_file(1, 0).unwrap(); + assert_eq!(conflict.before, ["base"]); + let projection = jj( + &root, + &[ + "--ignore-working-copy", + "file", + "show", + "-r", + &merged, + "-T", + "", + "--", + "conflict", + ], + ); + assert_eq!(conflict.after, projection.lines().collect::>()); + assert!(!conflict.hunks.is_empty()); + assert!(repository.load_file(1, 1).is_err()); + assert_eq!(disk_state(&root), state); + + jj(&root, &["new", &merged]); + fs::write(root.join("extra"), "extra\n").unwrap(); + jj( + &root, + &["describe", "-m", "unrelated change retaining conflict"], + ); + let next = jj(&root, &["log", "--no-graph", "-r", "@", "-T", "commit_id"]); + let comparison = repository + .compare( + 2, + &revision(&merged, RepoKind::Jujutsu), + &revision(&next, RepoKind::Jujutsu), + ComparisonMode::Snapshots, + &[], + ) + .unwrap(); + assert_eq!( + comparison + .files + .iter() + .map(|file| file.path.as_path()) + .collect::>(), + [Path::new("extra")] + ); +} + #[test] fn unsaved_overlays_are_working_after_only_and_cannot_follow_nonregular_paths() { let fixture = Fixture::new();