From 3bd0d514459b35e18926fcbb2e28251c8a5976e0 Mon Sep 17 00:00:00 2001 From: Cormac Relf Date: Sat, 12 Oct 2024 21:18:23 +1100 Subject: [PATCH] view: failing test for deprecated protobuf fields using the CLI This actually revealed another bug re HEAD imports, fixed in the following commit. --- Cargo.lock | 1 + cli/Cargo.toml | 1 + cli/tests/test_git_colocated.rs | 185 +++++++++++++++++++++++++++++--- lib/src/simple_op_store.rs | 12 +++ 4 files changed, 187 insertions(+), 12 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index dcfb3ed6ab..8c5518398f 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1855,6 +1855,7 @@ dependencies = [ "pest", "pest_derive", "pollster", + "prost", "rayon", "regex", "rpassword", diff --git a/cli/Cargo.toml b/cli/Cargo.toml index bf9b4de4c7..975cf2592d 100644 --- a/cli/Cargo.toml +++ b/cli/Cargo.toml @@ -102,6 +102,7 @@ async-trait = { workspace = true } insta = { workspace = true } test-case = { workspace = true } testutils = { workspace = true } +prost = { workspace = true } # https://github.com/rust-lang/cargo/issues/2911#issuecomment-1483256987 jj-cli = { path = ".", features = ["test-fakes"], default-features = false } diff --git a/cli/tests/test_git_colocated.rs b/cli/tests/test_git_colocated.rs index 7837023e27..e4a8ef68ce 100644 --- a/cli/tests/test_git_colocated.rs +++ b/cli/tests/test_git_colocated.rs @@ -17,6 +17,10 @@ use std::process::Command; use assert_cmd::assert::OutputAssertExt; use git2::Oid; +use itertools::Itertools; +use jj_lib::object_id::ObjectId; +use jj_lib::op_store::RefTarget; +use jj_lib::op_store::ViewId; use crate::common::TestEnvironment; @@ -1064,6 +1068,18 @@ fn test_colocated_workspace_fail_existing_git_worktree() { let _ = test_env.jj_cmd_ok(&repo_path, &["workspace", "add", "--colocate", "../second"]); } +fn log_heads(test_env: &TestEnvironment, repo_path: &Path) -> String { + let (stdout, stderr) = test_env.jj_cmd_ok( + repo_path, + &[ + "log", + "-rgit_heads()", + "-Tseparate(\" \", description.first_line(), working_copies)", + ], + ); + eprint!("{}", stderr); + stdout +} #[test] fn test_colocated_workspace_add_forget_add() { // TODO: Better way to disable the test if git command couldn't be executed @@ -1087,17 +1103,7 @@ fn test_colocated_workspace_add_forget_add() { std::fs::write(second_path.join("file"), "edit").unwrap(); let _ = test_env.jj_cmd_ok(&second_path, &["commit", "-m", "edit"]); - let log_heads = || { - test_env.jj_cmd_success( - &repo_path, - &[ - "log", - "-rgit_heads()", - "-Tseparate(\" \", description.first_line(), working_copies)", - ], - ) - }; - insta::assert_snapshot!(log_heads(), @r#" + insta::assert_snapshot!(log_heads(&test_env, &repo_path), @r#" ○ edit ○ initial commit │ @@ -1107,7 +1113,7 @@ fn test_colocated_workspace_add_forget_add() { // This should remove the git_heads entry, so the `edit` commit should disappear let _ = test_env.jj_cmd_ok(&repo_path, &["workspace", "forget", "second"]); - insta::assert_snapshot!(log_heads(), @r#" + insta::assert_snapshot!(log_heads(&test_env, &repo_path), @r#" ○ initial commit │ ~ @@ -1159,3 +1165,158 @@ fn test_colocated_workspace_forget_locked() { let _ = test_env.jj_cmd_ok(&repo_path, &["workspace", "forget", "second"]); } + +#[test] +fn test_colocated_view_proto_deprecated() { + let test_env = TestEnvironment::default(); + let repo_path = test_env.env_root().join("repo"); + let second_path = test_env.env_root().join("second"); + + // Not colocated, because we don't want default to appear in the working copies + let _ = test_env.jj_cmd_ok(test_env.env_root(), &["git", "init", "--colocate", "repo"]); + + std::fs::write(repo_path.join("file"), "content").unwrap(); + let _ = test_env.jj_cmd_ok(&repo_path, &["commit", "-m", "renamed"]); + + let _ = test_env.jj_cmd_ok(&repo_path, &["workspace", "rename", "renamed"]); + let _ = test_env.jj_cmd_ok(&repo_path, &["workspace", "add", "--colocate", "../second"]); + + std::fs::write(second_path.join("file"), "edit").unwrap(); + let _ = test_env.jj_cmd_ok(&second_path, &["commit", "-m", "second"]); + + insta::assert_snapshot!(log_heads(&test_env, &repo_path), @r#" + ○ second + ○ renamed + │ + ~ + "#); + + let views_path = repo_path + .join(".jj") + .join("repo") + .join("op_store") + .join("views"); + + let current_view_id = || { + let ops_path = repo_path + .join(".jj") + .join("repo") + .join("op_store") + .join("operations"); + let op_id = test_env + .jj_cmd_ok( + &second_path, + &[ + "op", + "log", + "--ignore-working-copy", + "--limit", + "1", + "--no-graph", + "--template=id", + ], + ) + .0 + .trim() + .to_owned(); + use prost::Message; + let op_path = ops_path.join(&op_id); + let buf = std::fs::read(op_path).unwrap(); + let op_proto = jj_lib::protos::op_store::Operation::decode(&*buf).unwrap(); + ViewId::new(op_proto.view_id) + }; + + #[allow(deprecated)] + let dbg_view = || { + use prost::Message; + let view_path = views_path.join(current_view_id().hex()); + let buf = std::fs::read(&view_path).expect("Layout of op_store changed"); + let proto = jj_lib::protos::op_store::View::decode(&*buf).unwrap(); + let working_copies = proto.wc_commit_ids.keys().sorted().collect_vec(); + let git_head_is_some = proto.git_head.is_some(); + let git_heads = proto + .git_heads + .into_iter() + .sorted_by_key(|(k, _)| k.clone()) + .map(|(k, v)| { + //.. + ( + k, + RefTarget::_from_proto(v) + .as_normal() + .map(|cid| cid.hex()[..8].to_owned()), + ) + }) + .collect_vec(); + format!( + "---- git_head_is_some: {git_head_is_some:?};\n git_heads: {git_heads:?}\n \ + working_copies: {working_copies:?}" + ) + }; + + // Now, simulate a previous version of JJ having written these views + for dentry in views_path.read_dir().unwrap() { + use prost::Message; + let view_path = dentry.unwrap().path(); + let buf = std::fs::read(&view_path).expect("Layout of op_store changed"); + let mut proto = jj_lib::protos::op_store::View::decode(&*buf).unwrap(); + #[allow(deprecated)] + { + proto.git_head = std::mem::take(&mut proto.git_heads).remove("renamed"); + } + std::fs::write(&view_path, proto.encode_to_vec()).unwrap(); + } + + insta::assert_snapshot!(dbg_view(), @r#" + ---- git_head_is_some: true; + git_heads: [] + working_copies: ["renamed", "second"] + "#); + + // A bit strange: apparently we are finding HEAD for the + // default workspace (aka renamed) even when it is not present in the view + insta::assert_snapshot!(log_heads(&test_env, &repo_path), @r#" + ○ renamed + │ + ~ + "#); + + insta::assert_snapshot!(dbg_view(), @r#" + ---- git_head_is_some: false; + git_heads: [("renamed", Some("65186856"))] + working_copies: ["renamed", "second"] + "#); + + // Even weirder: even in the other workspace. + insta::assert_snapshot!(log_heads(&test_env, &second_path), @r#" + ○ second + ○ renamed + │ + ~ + "#); + + insta::assert_snapshot!(dbg_view(), @r#" + ---- git_head_is_some: false; + git_heads: [("renamed", Some("65186856")), ("second", Some("f8398ffd"))] + working_copies: ["renamed", "second"] + "#); + + let (_, stderr) = test_env.jj_cmd_ok(&second_path, &["commit", "-m", "second 2"]); + eprint!("{}", stderr); + + insta::assert_snapshot!(dbg_view(), @r#" + ---- git_head_is_some: false; + git_heads: [("renamed", Some("65186856")), ("second", Some("1bc3c6d5"))] + working_copies: ["renamed", "second"] + "#); + + // It's ok if we can do better than this and get HEADS for all the workspaces + // We are testing that our best effort recovery actually does something. + insta::assert_snapshot!(log_heads(&test_env, &repo_path), @r#" + ○ second 2 + ~ (elided revisions) + ○ renamed + │ + ~ + "#); +} diff --git a/lib/src/simple_op_store.rs b/lib/src/simple_op_store.rs index a0e8277943..83be57c4f1 100644 --- a/lib/src/simple_op_store.rs +++ b/lib/src/simple_op_store.rs @@ -542,11 +542,13 @@ fn view_from_proto(proto: crate::protos::op_store::View) -> View { let default = WorkspaceId::default(); if let Ok(only_one) = view.wc_commit_ids.keys().exactly_one().cloned() { + eprintln!("only one workspace id: {only_one}"); // Easy. The user has renamed the default workspace. view.git_heads = hashmap! { only_one => fallback, }; } else if view.wc_commit_ids.contains_key(&default) { + eprintln!("guessed default workspace id as default"); // If one of them is named default, assume it is the default, and behave the // most correctly we can -- only one git head is set, and we wait for the // other workspaces to reset their git heads on their own in normal operation. @@ -562,6 +564,7 @@ fn view_from_proto(proto: crate::protos::op_store::View) -> View { // So we do nothing. Eventually JJ will add git_heads entries for // any workspaces you interact with. So it's not the end // of the world. + eprintln!("Could not guess default workspace id") } } @@ -728,6 +731,12 @@ fn ref_target_to_proto_legacy(value: &RefTarget) -> Option Self { + ref_target_from_proto(Some(proto)) + } +} fn ref_target_from_proto(maybe_proto: Option) -> RefTarget { // TODO: Delete legacy format handling when we decide to drop support for views // saved by jj <= 0.8. @@ -1148,6 +1157,7 @@ mod tests { ..default.clone() }; let view = view_from_proto(proto); + assert_eq!(view.git_heads.len(), 1); assert!(view.git_heads.contains_key(&wsd)); } let rt = RefTarget::normal(CommitId::new(b"abc".to_vec())); @@ -1161,6 +1171,7 @@ mod tests { ..default.clone() }; let view = view_from_proto(proto); + assert_eq!(view.git_heads.len(), 1); assert!(view.git_heads.contains_key(&wsd)); } { @@ -1173,6 +1184,7 @@ mod tests { ..default.clone() }; let view = view_from_proto(proto); + assert_eq!(view.git_heads.len(), 1); assert!(view.git_heads.contains_key(&wsd)); } }