eb9022a · j · git.iko.soy

git.iko.soy/j

on master help sign in

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

eb9022aiko3 weeks ago

src/builtins.rs+18−2

compare with master

unifiedsplit

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

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

216}

217

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.

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

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

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

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

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

224 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+ }

222}

229}

223

230

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

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

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

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

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

233 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+ }

228}

238}

229

239

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

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

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

651 out: &mut Vec<Value>,

661 out: &mut Vec<Value>,

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

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

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

663 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;

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

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

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

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

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

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

src/jj.rs+49−37

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

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

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

533 let new_repo = unpublished.leave_unpublished();

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

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

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

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).

536− locked_ws

542+ drop(locked_ws);

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

538− .await

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

540 let _ = op_id;

543 let _ = op_id;

541 let pending = PendingSnapshot {

544 let pending = PendingSnapshot {

542 pre_repo: self.current_repo(),

545 pre_repo: self.current_repo(),

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

1049 break;

1052 break;

1050 }

1053 }

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

1054 (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.

1053 if !m.authored_by_j {

1061 if !m.authored_by_j {

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

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

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

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

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

1057 target_op_id = parent;

1065 target_op_id = parent;

1058 break;

1066 break;

1059 }

1067 }

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

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

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

1069 .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+ };

1062 }

1078 }

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

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

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

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

1065− // redos to the operation ultimately reapplied

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").

1066− target_op_id = if m.authored_by_j {

1084+ if !m.authored_by_j {

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

1085+ target_op_id = m.id;

1068− } else {

1086+ break;

1069− m.id

1087+ }

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

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

1070 };

1090 };

1071− break;

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

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

1072 }

1093 }

1073 }

1094 }

1074 }

1095 }

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

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

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

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

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

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

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

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

1160+ if same {

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

1162+ // only refreshes the mtime cache

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

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

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

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+ }

1142 drop(ws_guard);

1172 drop(ws_guard);

1143 if !same {

1173 if !same {

1144 return Err(dirty);

1174 return Err(dirty);

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

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

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

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

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

1462 /// the undo this redo reversed

1492 /// the undo this redo reversed

1463− Redo { redid: OperationId },

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

1464}

1494}

1465

1495

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

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

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

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

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

1527}

1557}

1528

1558

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−

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

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

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

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

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

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

src/render.rs+12−2

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

1084 }

1085 }

1086 None => {

1087− let n = nfiles;

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+ };

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

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

1089 }

1099 }

1090 };

1100 };

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

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

tests/builtins.rs+21

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

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

282 }

283}

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() {

483 }

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

485}

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+}