eb9022a · j · git.iko.soy

git.iko.soy/j

Stop a discarded snapshot from saving working-copy state; fix four more bugs

`snapshot_repo` deliberately leaves its operation unpublished, but still called `locked_ws.finish(...)`, which writes the scanned tree state to disk. That recorded the working directory as already snapshotted while the published head still had the old wc tree, so the next run's incremental snapshot found nothing to do and built the Repo from the stale tree — and a persisting run then wrote that stale tree and checked it back out. Editing a file, running `j files`, then `j describe` left the edit reverted and a new file deleted from disk. `check_wc_clean` had the same pattern, so a refused `j undo` on a dirty tree triggered it too. Both now release the lock without finishing; the state is advanced only by `persist`'s checkout, which finishes against the operation that actually recorded the tree. The four others, each invisible to the suite before: - `tree` marked commits empty whenever the parent's file list had not been materialized: `empty` fell back to `nfiles == 0`, and `nfiles` is 0 precisely when the list was not loaded. It now asks the backend's O(1) answer first, as the `Some(parent)` arm already did. - The second `redo` in a row restored the wrong view. Undoing a redo stopped at the undo it reversed rather than past it to the operation it reapplied, stepping back one operation too far; redoing a redo re-targeted the already-reversed undo instead of looking outward for one still standing. Both now follow §7.7. `chain_through_redos` is unused and removed. - `(&&)` and `(||)` used as values returned their second argument unconditionally, so `foldl (&&) true [false true]` was `true`. Infix short-circuiting is handled by the evaluator and is unaffected. - `extract` walked past a `Value::Thunk` without forcing it, so `extract Entry repo` found nothing on a real repository while finding everything on MemBackend. Commit file lists are lazy for large-repo performance, but that must stay invisible to the language.

commiteb9022a5c202ddd9a9fba937d502ff5a481b02f0 authoriko <ilyakooo0@gmail.com> dateMon 21 Sep 2026, 18:39 (3 weeks ago) parent7800b49

5 files changed+222−41

compare with master

unifiedsplit

src/builtins.rs+18−2 src/jj.rs+49−37 src/render.rs+12−2 tests/builtins.rs+21 tests/cli.rs+122

src/builtins.rs+18−2

@@ -215,16 +215,26 @@ fn b_ne(_i: &mut Interp, args: &[Value]) -> BResult {

215215 Ok(Value::Bool(!value_eq(&args[0], &args[1])?))

216216}

217217

218+// `&&` and `||` short-circuit when written infix (the evaluator handles

219+// `Expr::BinOp` directly). These are the same operators used as *values* —

220+// `foldl (&&) true xs`, `map ((||) b) xs` — where both arguments are already

221+// evaluated, so they must still combine them rather than return the second.

218222fn b_and(_i: &mut Interp, args: &[Value]) -> BResult {

219223 want!(args, 0, Value::Bool(_), "Bool");

220224 want!(args, 1, Value::Bool(_), "Bool");

221− Ok(args[1].clone())

225+ match (&args[0], &args[1]) {

226+ (Value::Bool(a), Value::Bool(b)) => Ok(Value::Bool(*a && *b)),

227+ _ => unreachable!("checked above"),

228+ }

222229}

223230

224231fn b_or(_i: &mut Interp, args: &[Value]) -> BResult {

225232 want!(args, 0, Value::Bool(_), "Bool");

226233 want!(args, 1, Value::Bool(_), "Bool");

227− Ok(args[1].clone())

234+ match (&args[0], &args[1]) {

235+ (Value::Bool(a), Value::Bool(b)) => Ok(Value::Bool(*a || *b)),

236+ _ => unreachable!("checked above"),

237+ }

228238}

229239

230240fn b_not(_i: &mut Interp, args: &[Value]) -> BResult {

@@ -651,6 +661,12 @@ fn extract_into(

651661 out: &mut Vec<Value>,

652662) -> Result<(), Crash> {

653663 use crate::value::{PrimKind, ShapeKind};

664+ // a commit's `files` is a thunk until something asks for it; the language

665+ // must not see the difference, so force before matching and recursing —

666+ // otherwise `extract Entry repo` finds nothing on a real repository while

667+ // finding everything on the in-memory backend

668+ let forced = v.forced()?;

669+ let v = &forced;

654670 let matches = match (&shape.kind, v) {

655671 (ShapeKind::Prim(PrimKind::Int), Value::Int(_)) => true,

656672 (ShapeKind::Prim(PrimKind::Text), Value::Text(_)) => true,

src/jj.rs+49−37

@@ -531,12 +531,15 @@ impl JjBackend {

531531 .map_err(|e| (2, format!("cannot write the snapshot operation: {}", e)))?;

532532 let op_id = unpublished.operation().id().clone();

533533 let new_repo = unpublished.leave_unpublished();

534− // the working-copy state tracks the snapshot tree, against the

535− // current (published) head; persist will finish against its own op

536− locked_ws

537− .finish(self.current_repo().operation().id().clone())

538− .await

539− .map_err(|e| (2, format!("cannot finish the snapshot: {}", e)))?;

534+ // Release the lock *without* finishing: `finish` saves the scanned

535+ // tree state to disk, which would record the working directory as

536+ // already snapshotted while the published head still has the old wc

537+ // tree. The next run's incremental snapshot would then find nothing

538+ // to do and build the Repo from the stale tree — and a persisting run

539+ // would check that stale tree back out, destroying the edits. The

540+ // state is advanced only by `persist`'s checkout, which finishes

541+ // against the operation that actually recorded the tree (§7.4/§7.7).

542+ drop(locked_ws);

540543 let _ = op_id;

541544 let pending = PendingSnapshot {

542545 pre_repo: self.current_repo(),

@@ -1049,7 +1052,12 @@ impl JjBackend {

10491052 break;

10501053 }

10511054 (Some(m), false) => {

1052− // a redo: move past the undo it reversed

1055+ // a redo: "move past the operation it reapplied" (§7.7).

1056+ // The redo marker names the *undo* it reversed; the

1057+ // operation that undo discarded is the one this redo put

1058+ // back, so that is where the walk continues. Stopping at

1059+ // the undo instead would make the loop treat it as an undo

1060+ // to follow and step back one operation too far.

10531061 if !m.authored_by_j {

10541062 let Some(parent) = cur.parent_ids().first().cloned() else {

10551063 return Err((1, "nothing to undo".to_string()));

@@ -1057,18 +1065,31 @@ impl JjBackend {

10571065 target_op_id = parent;

10581066 break;

10591067 }

1060− cur = block_on(loader.load_operation(&m.id))

1068+ let undo_op = block_on(loader.load_operation(&m.id))

10611069 .map_err(|e| (2, format!("cannot load an operation: {}", e)))?;

1070+ cur = match op_marker(undo_op.metadata()) {

1071+ Some(u) if u.kind == "undo" && u.authored_by_j => {

1072+ block_on(loader.load_operation(&u.id))

1073+ .map_err(|e| (2, format!("cannot load an operation: {}", e)))?

1074+ }

1075+ // cannot follow the marker: degrade to the undo itself

1076+ _ => undo_op,

1077+ };

10621078 }

10631079 (Some(m), true) => {

1064− // a redo: reverse the undo it reversed, following further

1065− // redos to the operation ultimately reapplied

1066− target_op_id = if m.authored_by_j {

1067− block_on(chain_through_redos(&loader, &m.id))?

1068− } else {

1069− m.id

1080+ // a redo: the undo it reversed is already accounted for,

1081+ // so keep looking outward from that undo's parent for an

1082+ // undo that has not been reversed yet (§7.7: the head must

1083+ // be an undo, "possibly reached through redos").

1084+ if !m.authored_by_j {

1085+ target_op_id = m.id;

1086+ break;

1087+ }

1088+ let Some(prev) = op_parent_id(&loader, &m.id)? else {

1089+ return Err((1, "nothing to redo".to_string()));

10701090 };

1071− break;

1091+ cur = block_on(loader.load_operation(&prev))

1092+ .map_err(|e| (2, format!("cannot load an operation: {}", e)))?;

10721093 }

10731094 }

10741095 }

@@ -1136,9 +1157,18 @@ impl JjBackend {

11361157 let (new_tree, _stats) = block_on(locked_ws.locked_wc().snapshot(&options))

11371158 .map_err(|e| (2, format!("cannot read the working copy: {}", e)))?;

11381159 let same = new_tree.tree_ids() == wc_commit.tree().tree_ids();

1139− // discard: finish against the current op so no state changes

1140− block_on(locked_ws.finish(base.operation().id().clone()))

1141− .map_err(|e| (2, format!("cannot finish: {}", e)))?;

1160+ if same {

1161+ // the scanned tree is the one already recorded: saving the state

1162+ // only refreshes the mtime cache

1163+ block_on(locked_ws.finish(base.operation().id().clone()))

1164+ .map_err(|e| (2, format!("cannot finish: {}", e)))?;

1165+ } else {

1166+ // the working copy differs from @ and this command records

1167+ // nothing: drop the lock without saving, or the state would claim

1168+ // the directory was snapshotted and hide the changes from the next

1169+ // run (§7.7)

1170+ drop(locked_ws);

1171+ }

11421172 drop(ws_guard);

11431173 if !same {

11441174 return Err(dirty);

@@ -1460,7 +1490,7 @@ enum OpKind {

14601490 /// the operation this undo discarded the view of

14611491 Undo { #[allow(dead_code)] undid: OperationId },

14621492 /// the undo this redo reversed

1463− Redo { redid: OperationId },

1493+ Redo { #[allow(dead_code)] redid: OperationId },

14641494}

14651495

14661496/// a marker parsed from an operation description (§7.7 kinds)

@@ -1526,24 +1556,6 @@ fn op_parent_id(

15261556 Ok(op.parent_ids().first().cloned())

15271557}

15281558

1529−/// follow a chain of redos to the operation that was ultimately reapplied

1530−async fn chain_through_redos(

1531− loader: &jj_lib::repo::RepoLoader,

1532− first: &OperationId,

1533−) -> Result<OperationId, OpenError> {

1534− let mut id = first.clone();

1535− loop {

1536− let op = loader

1537− .load_operation(&id)

1538− .await

1539− .map_err(|e| (2, format!("cannot load an operation: {}", e)))?;

1540− match op_kind(op.metadata()) {

1541− OpKind::Redo { redid } => id = redid,

1542− _ => return Ok(id),

1543− }

1544− }

1545−}

1546−

15471559/// parse the push-record list (§7.6): records of shape {id, name} set a

15481560/// bookmark, records of shape {delete} remove one; duplicates merge, the same

15491561/// name with different effects crashes (exit 1)

src/render.rs+12−2

@@ -1084,8 +1084,18 @@ fn build_info(

10841084 }

10851085 }

10861086 None => {

1087− let n = nfiles;

1088− (n == 0 && id != ROOT_ID, None, Vec::new())

1087+ // No parent snapshot to compare against — either this is the top

1088+ // of the history, or the parent's file list was never materialized

1089+ // because nothing needed it. `nfiles` is 0 in that second case

1090+ // simply because the list was not loaded, so it cannot stand in

1091+ // for emptiness: ask the backend first, and fall back to the file

1092+ // count only when the list really was loaded (which is exactly

1093+ // when the backend could not answer).

1094+ let empty = match backend_empty {

1095+ Some(e) => e && id != ROOT_ID,

1096+ None => nfiles == 0 && id != ROOT_ID,

1097+ };

1098+ (empty, None, Vec::new())

10891099 }

10901100 };

10911101 let meta = interp.backend.meta(&id).ok();

tests/builtins.rs+21

@@ -281,3 +281,24 @@ labelled = \_ _ -> []

281281 Err(c) => assert!(c.msg.contains("unbound"), "{}", c.msg),

282282 }

283283}

284+

285+#[test]

286+fn boolean_operators_as_values() {

287+ // `&&` and `||` short-circuit as infix operators, which the evaluator

288+ // handles directly. Used as values — a section, or passed to foldl/map —

289+ // both arguments are already evaluated, and the builtin must still

290+ // combine them rather than return the second one.

291+ let (mut i, cfg) = make_interp();

292+ check!(i, cfg, "foldl (&&) true [true true]", Value::Bool(true));

293+ check!(i, cfg, "foldl (&&) true [true false]", Value::Bool(false));

294+ check!(i, cfg, "foldl (&&) true [false true]", Value::Bool(false));

295+ check!(i, cfg, "foldl (||) false [false false]", Value::Bool(false));

296+ check!(i, cfg, "foldl (||) false [false true]", Value::Bool(true));

297+ check!(i, cfg, "foldl (||) false [true false]", Value::Bool(true));

298+ // partially applied sections

299+ check!(i, cfg, "map ((&&) false) [true true]", Value::list(vec![Value::Bool(false); 2]));

300+ check!(i, cfg, "map ((||) true) [false false]", Value::list(vec![Value::Bool(true); 2]));

301+ // infix still short-circuits: the rhs is never evaluated

302+ check!(i, cfg, "false && (crash \"boom\")", Value::Bool(false));

303+ check!(i, cfg, "true || (crash \"boom\")", Value::Bool(true));

304+}

tests/cli.rs+122

@@ -483,3 +483,125 @@ fn missing_config_is_created_editable() {

483483 }

484484 let _ = std::fs::remove_dir_all(&dir);

485485}

486+

487+// ----------------------------------------------------------------------

488+// regressions

489+// ----------------------------------------------------------------------

490+

491+#[test]

492+fn read_only_run_does_not_hide_working_copy_changes() {

493+ // A printing run snapshots the working directory but records no

494+ // operation. It must not save the scanned tree state either: doing so

495+ // told the next run's incremental snapshot that the directory was already

496+ // recorded, so it rebuilt the Repo from the stale tree in the working-copy

497+ // commit — and a later persisting run then checked that stale tree back

498+ // out, discarding the edits from disk (§7.4/§7.7).

499+ let r = setup();

500+ r.write("a.txt", "one\n");

501+ r.j(&["id"]).ok();

502+ r.write("a.txt", "edited content\n");

503+ r.write("b.txt", "brand new\n");

504+ for _ in 0..3 {

505+ let out = r.j(&["files"]).ok().stdout;

506+ assert!(out.contains("a.txt"), "{}", out);

507+ assert!(out.contains("b.txt"), "b.txt missing from: {}", out);

508+ }

509+ // and the edits survive the next persisting run, on disk and in the commit

510+ r.j(&["describe \"after reads\""]).ok();

511+ assert_eq!(r.read("a.txt"), "edited content\n");

512+ assert_eq!(r.read("b.txt"), "brand new\n");

513+ let files = r.j(&["files"]).ok().stdout;

514+ assert!(files.contains("b.txt"), "{}", files);

515+}

516+

517+#[test]

518+fn refused_reserved_command_does_not_hide_working_copy_changes() {

519+ // `undo` refuses a dirty working copy; the snapshot it took to decide

520+ // that must not be saved either, for the same reason

521+ let r = setup();

522+ r.write("a.txt", "one\n");

523+ r.j(&["id"]).ok();

524+ r.write("a.txt", "edited content\n");

525+ assert_eq!(r.j(&["undo"]).code, 1);

526+ let out = r.j(&["files"]).ok().stdout;

527+ assert!(out.contains("a.txt"), "{}", out);

528+ // the size column reflects the edited file, not the recorded one

529+ assert!(out.contains("15 B"), "expected the edited size in: {}", out);

530+}

531+

532+#[test]

533+fn tree_does_not_mark_commits_empty() {

534+ // `empty` was read off a file count that is 0 whenever the parent's file

535+ // list was never materialized, so every commit whose parent skipped

536+ // loading files rendered with the empty glyph

537+ let r = setup();

538+ for n in ["a", "b", "c", "d"] {

539+ r.write(&format!("{}.txt", n), &format!("content {}\n", n));

540+ r.j(&[&format!("describe \"commit {}\"", n)]).ok();

541+ r.j(&["new"]).ok();

542+ }

543+ let out = r.j(&["tree"]).ok().stdout;

544+ // each commit that added a file must not carry the empty glyph (the

545+ // legend at the bottom names it too, so check the commit rows only)

546+ for n in ["a", "b", "c", "d"] {

547+ let row = out

548+ .lines()

549+ .find(|l| l.contains(&format!("commit {}", n)))

550+ .unwrap_or_else(|| panic!("no row for commit {} in:\n{}", n, out));

551+ assert!(!row.contains('◌'), "commit {} marked empty: {:?}", n, row);

552+ }

553+}

554+

555+#[test]

556+fn redo_twice_in_a_row() {

557+ // the second redo followed the redo marker to the undo it reversed and

558+ // restored *that* view, instead of looking outward for an undo that had

559+ // not been reversed yet (§7.7)

560+ let r = setup();

561+ r.write("a.txt", "x\n");

562+ // two operations on the same commit, so undoing both clears the message

563+ r.j(&["describe \"one\""]).ok();

564+ r.j(&["describe \"two\""]).ok();

565+ r.j(&["undo"]).ok();

566+ assert!(r.j(&["status"]).ok().stdout.contains("one"));

567+ r.j(&["undo"]).ok();

568+ let cleared = r.j(&["status"]).ok().stdout;

569+ assert!(!cleared.contains("one"), "{}", cleared);

570+ r.j(&["redo"]).ok();

571+ let first = r.j(&["status"]).ok().stdout;

572+ assert!(first.contains("one"), "first redo: {}", first);

573+ r.j(&["redo"]).ok();

574+ let second = r.j(&["status"]).ok().stdout;

575+ assert!(second.contains("two"), "second redo did not restore it:\n{}", second);

576+}

577+

578+#[test]

579+fn undo_redo_cycles_are_stable() {

580+ // alternating undo/redo must keep returning to the same two states

581+ let r = setup();

582+ r.write("a.txt", "x\n");

583+ r.j(&["describe \"one\""]).ok();

584+ for i in 0..3 {

585+ r.j(&["undo"]).ok();

586+ let after = r.j(&["status"]).ok().stdout;

587+ assert!(!after.contains("one"), "cycle {}: {}", i, after);

588+ r.j(&["redo"]).ok();

589+ let back = r.j(&["status"]).ok().stdout;

590+ assert!(back.contains("one"), "cycle {}: {}", i, back);

591+ }

592+}

593+

594+#[test]

595+fn extract_sees_through_lazy_file_lists() {

596+ // a commit's `files` is a thunk until something asks for it; `extract`

597+ // walked past it, so it found no entries at all on a real repository

598+ // while finding them all on the in-memory backend

599+ let r = setup();

600+ r.write("a.txt", "x\n");

601+ r.write("b.txt", "y\n");

602+ r.j(&["id"]).ok();

603+ let entries = r.j(&["length . extract Entry"]).ok().stdout;

604+ assert_eq!(entries.trim(), "2", "extract Entry: {}", entries);

605+ let blobs = r.j(&["length . extract Blob"]).ok().stdout;

606+ assert_eq!(blobs.trim(), "2", "extract Blob: {}", blobs);

607+}