From 910190f6e52488f530ef0180f81090039915e4f6 Mon Sep 17 00:00:00 2001 From: kelmith Date: Sat, 11 Jul 2026 08:53:55 +0530 Subject: [PATCH] fix: cli changes diff --- cli/src/changes.rs | 117 +++++++++++++------------- cli/src/git.rs | 2 +- cli/tests/cli/changes_cmd.rs | 154 +++++++++++++++++++++++++++++++++++ 3 files changed, 210 insertions(+), 63 deletions(-) diff --git a/cli/src/changes.rs b/cli/src/changes.rs index 0fe7ba251..ba7e4cd20 100644 --- a/cli/src/changes.rs +++ b/cli/src/changes.rs @@ -494,22 +494,20 @@ async fn run_diff( canon_repo_str.to_string() }; - let changed_abs: HashSet = after_files + let changed_after_rel: HashSet = after_files .iter() - .filter_map(|f| std::fs::canonicalize(f).ok()) - .map(|p| p.to_string_lossy().to_string()) + .filter_map(|f| rel_from_root(f, &canon_after_root)) .collect(); - let changed_tmp: HashSet = before_files + let changed_before_rel: HashSet = before_files .iter() - .filter_map(|f| std::fs::canonicalize(f).ok()) - .map(|p| p.to_string_lossy().to_string()) + .filter_map(|f| rel_from_root(f, canon_tmp_str)) .collect(); - let after_by_key = index_graph_by_norm_key(&after_graph, &canon_after_root, &changed_abs); - let before_by_key = index_graph_by_norm_key(&before_graph, canon_tmp_str, &changed_tmp); + let after_by_key = index_graph_by_norm_key(&after_graph, &canon_after_root, &changed_after_rel); + let before_by_key = index_graph_by_norm_key(&before_graph, canon_tmp_str, &changed_before_rel); - let after_edge_by_key = index_edges_by_key(&after_graph, &canon_after_root, &changed_abs); - let before_edge_by_key = index_edges_by_key(&before_graph, canon_tmp_str, &changed_tmp); + let after_edge_by_key = index_edges_by_key(&after_graph, &canon_after_root, &changed_after_rel); + let before_edge_by_key = index_edges_by_key(&before_graph, canon_tmp_str, &changed_before_rel); let after_keys: HashSet = after_by_key.keys().cloned().collect(); let before_keys: HashSet = before_by_key.keys().cloned().collect(); @@ -741,27 +739,41 @@ fn node_signature(node: &Node) -> Option { }) } +// Node file paths may have had their `/tmp/` (or `/private/tmp/`) prefix +// stripped by the ast builder, so try the root, the tmp-stripped root, and +// finally symlink resolution before giving up. +fn rel_from_root(file: &str, root: &str) -> Option { + if root.is_empty() { + return None; + } + if let Some(rel) = file.strip_prefix(root) { + return Some(rel.trim_start_matches('/').to_string()); + } + let tmp_stripped = root + .strip_prefix("/private/tmp/") + .or_else(|| root.strip_prefix("/tmp/")) + .unwrap_or(root); + if tmp_stripped != root { + if let Some(rel) = file.strip_prefix(tmp_stripped) { + return Some(rel.trim_start_matches('/').to_string()); + } + } + std::fs::canonicalize(file).ok().and_then(|c| { + c.to_str() + .and_then(|cs| cs.strip_prefix(root)) + .map(|rel| rel.trim_start_matches('/').to_string()) + }) +} + fn norm_key(node: &Node, root: &str) -> String { let file = &node.node_data.file; - let rel_file = if root.is_empty() { - file.as_str() - } else { - file.strip_prefix(root) - .map(|s| s.trim_start_matches('/')) - .unwrap_or(file.as_str()) - }; + let rel_file = rel_from_root(file, root).unwrap_or_else(|| file.clone()); format!("{}-{}-{}", node.node_type, node.node_data.name, rel_file) } fn norm_key_from_ref(node_ref: &ast::lang::graphs::NodeRef, root: &str) -> String { let file = &node_ref.node_data.file; - let rel_file = if root.is_empty() { - file.as_str() - } else { - file.strip_prefix(root) - .map(|s| s.trim_start_matches('/')) - .unwrap_or(file.as_str()) - }; + let rel_file = rel_from_root(file, root).unwrap_or_else(|| file.clone()); format!( "{}-{}-{}", node_ref.node_type, node_ref.node_data.name, rel_file @@ -787,11 +799,9 @@ fn index_graph_by_norm_key<'a>( continue; } if !allowed_files.is_empty() { - let canon = std::fs::canonicalize(&node.node_data.file) - .map(|p| p.to_string_lossy().to_string()) - .unwrap_or_default(); - if !allowed_files.contains(&canon) { - continue; + match rel_from_root(&node.node_data.file, root) { + Some(rel) if allowed_files.contains(&rel) => {} + _ => continue, } } let key = norm_key(node, root); @@ -803,22 +813,8 @@ fn index_graph_by_norm_key<'a>( fn edge_key(edge: &Edge, src_root: &str, tgt_root: &str) -> String { let src_file = &edge.source.node_data.file; let tgt_file = &edge.target.node_data.file; - let rel_src = if src_root.is_empty() { - src_file.as_str() - } else { - src_file - .strip_prefix(src_root) - .map(|s| s.trim_start_matches('/')) - .unwrap_or(src_file.as_str()) - }; - let rel_tgt = if tgt_root.is_empty() { - tgt_file.as_str() - } else { - tgt_file - .strip_prefix(tgt_root) - .map(|s| s.trim_start_matches('/')) - .unwrap_or(tgt_file.as_str()) - }; + let rel_src = rel_from_root(src_file, src_root).unwrap_or_else(|| src_file.clone()); + let rel_tgt = rel_from_root(tgt_file, tgt_root).unwrap_or_else(|| tgt_file.clone()); format!( "{}-{}-{}→{}-{}-{}", edge.source.node_type, @@ -842,13 +838,13 @@ fn index_edges_by_key<'a>( } // Only include edges where at least one endpoint is in the changed files set if !allowed_files.is_empty() { - let src_canon = std::fs::canonicalize(&edge.source.node_data.file) - .map(|p| p.to_string_lossy().to_string()) - .unwrap_or_default(); - let tgt_canon = std::fs::canonicalize(&edge.target.node_data.file) - .map(|p| p.to_string_lossy().to_string()) - .unwrap_or_default(); - if !allowed_files.contains(&src_canon) && !allowed_files.contains(&tgt_canon) { + let src_ok = rel_from_root(&edge.source.node_data.file, root) + .map(|rel| allowed_files.contains(&rel)) + .unwrap_or(false); + let tgt_ok = rel_from_root(&edge.target.node_data.file, root) + .map(|rel| allowed_files.contains(&rel)) + .unwrap_or(false); + if !src_ok && !tgt_ok { continue; } } @@ -859,16 +855,13 @@ fn index_edges_by_key<'a>( } fn rel_path(file: &str, root: &str) -> String { - Path::new(file) - .strip_prefix(root) - .map(|p| p.to_string_lossy().to_string()) - .unwrap_or_else(|_| { - Path::new(file) - .file_name() - .and_then(|n| n.to_str()) - .unwrap_or(file) - .to_string() - }) + rel_from_root(file, root).unwrap_or_else(|| { + Path::new(file) + .file_name() + .and_then(|n| n.to_str()) + .unwrap_or(file) + .to_string() + }) } fn print_delta( diff --git a/cli/src/git.rs b/cli/src/git.rs index ecc0e493f..e37cceb2b 100644 --- a/cli/src/git.rs +++ b/cli/src/git.rs @@ -117,7 +117,7 @@ pub fn list_commits_for_paths( pub fn read_file_at_rev(repo_path: &str, rev: &str, file_path: &str) -> Result>> { let spec = format!("{}:{}", rev, file_path); let output = std::process::Command::new("git") - .args(["show", "--", &spec]) + .args(["show", "--end-of-options", &spec]) .current_dir(repo_path) .output() .map_err(|e| Error::internal(format!("Failed to run git show: {}", e)))?; diff --git a/cli/tests/cli/changes_cmd.rs b/cli/tests/cli/changes_cmd.rs index 3eaa33335..2c655be8a 100644 --- a/cli/tests/cli/changes_cmd.rs +++ b/cli/tests/cli/changes_cmd.rs @@ -217,6 +217,160 @@ fn changes_diff_scope_normalizes_dot_slash_and_trailing_slash() { ); } +fn init_git_repo_with_committed_node_changes() -> tempfile::TempDir { + let dir = tempfile::tempdir().expect("tempdir failed"); + let root = dir.path(); + + run_cmd(root, &["git", "init"]); + run_cmd(root, &["git", "config", "user.email", "test@example.com"]); + run_cmd(root, &["git", "config", "user.name", "Test User"]); + + write_file( + root, + "src/lib.rs", + "pub fn one() -> i32 {\n 1\n}\n\npub fn gone() -> i32 {\n 0\n}\n", + ); + run_cmd(root, &["git", "add", "."]); + run_cmd(root, &["git", "commit", "-m", "initial"]); + + write_file( + root, + "src/lib.rs", + "pub fn one() -> i32 {\n 2\n}\n\npub fn two() -> i32 {\n one()\n}\n", + ); + run_cmd(root, &["git", "add", "."]); + run_cmd(root, &["git", "commit", "-m", "second"]); + + dir +} + +fn node_names(payload: &Value, key: &str) -> Vec { + payload["data"][key] + .as_array() + .expect("node array") + .iter() + .filter_map(|n| { + n["name"] + .as_str() + .or_else(|| n["after"]["name"].as_str()) + .map(str::to_string) + }) + .collect() +} + +#[test] +fn changes_diff_last_reports_removed_and_modified_nodes() { + let repo = init_git_repo_with_committed_node_changes(); + let cwd = repo.path().to_string_lossy().to_string(); + let out = run_stakgraph_in_cwd(&cwd, &["--json", "changes", "diff", "--last", "1"]); + + assert_eq!(out.exit_code, 0, "stderr: {}", out.stderr); + let payload: Value = serde_json::from_str(&out.stdout).expect("valid json stdout"); + assert_eq!(payload["ok"], true); + + let added = node_names(&payload, "added_nodes"); + let removed = node_names(&payload, "removed_nodes"); + let modified = node_names(&payload, "modified_nodes"); + + assert!( + added.iter().any(|n| n == "two"), + "fn two must be reported added; stdout: {}", + out.stdout + ); + assert!( + removed.iter().any(|n| n == "gone"), + "fn gone must be reported removed; stdout: {}", + out.stdout + ); + assert!( + modified.iter().any(|n| n == "one"), + "fn one must be reported modified; stdout: {}", + out.stdout + ); + assert!( + !added.iter().any(|n| n == "one"), + "fn one must not be reported added; stdout: {}", + out.stdout + ); + assert!( + payload["data"]["summary"]["nodes_removed"].as_u64().unwrap_or(0) >= 1 + && payload["data"]["summary"]["nodes_modified"].as_u64().unwrap_or(0) >= 1, + "summary must count removed and modified nodes; stdout: {}", + out.stdout + ); +} + +fn init_git_repo_with_rewired_call() -> tempfile::TempDir { + let dir = tempfile::tempdir().expect("tempdir failed"); + let root = dir.path(); + + run_cmd(root, &["git", "init"]); + run_cmd(root, &["git", "config", "user.email", "test@example.com"]); + run_cmd(root, &["git", "config", "user.name", "Test User"]); + + write_file( + root, + "src/lib.rs", + "pub fn one() -> i32 {\n 1\n}\n\npub fn caller() -> i32 {\n one()\n}\n", + ); + run_cmd(root, &["git", "add", "."]); + run_cmd(root, &["git", "commit", "-m", "initial"]); + + write_file( + root, + "src/lib.rs", + "pub fn one() -> i32 {\n 1\n}\n\npub fn helper() -> i32 {\n 5\n}\n\npub fn caller() -> i32 {\n helper()\n}\n", + ); + run_cmd(root, &["git", "add", "."]); + run_cmd(root, &["git", "commit", "-m", "rewire caller"]); + + dir +} + +fn edge_pairs(payload: &Value, key: &str) -> Vec<(String, String)> { + payload["data"][key] + .as_array() + .expect("edge array") + .iter() + .filter_map(|e| { + Some(( + e["source_name"].as_str()?.to_string(), + e["target_name"].as_str()?.to_string(), + )) + }) + .collect() +} + +#[test] +fn changes_diff_range_reports_edge_changes() { + let repo = init_git_repo_with_rewired_call(); + let cwd = repo.path().to_string_lossy().to_string(); + let out = run_stakgraph_in_cwd( + &cwd, + &["--json", "changes", "diff", "--range", "HEAD~1..HEAD"], + ); + + assert_eq!(out.exit_code, 0, "stderr: {}", out.stderr); + let payload: Value = serde_json::from_str(&out.stdout).expect("valid json stdout"); + + let removed = edge_pairs(&payload, "removed_edges"); + let added = edge_pairs(&payload, "added_edges"); + assert!( + removed + .iter() + .any(|(s, t)| s == "caller" && t == "one"), + "call edge caller -> one must be reported removed; stdout: {}", + out.stdout + ); + assert!( + added + .iter() + .any(|(s, t)| s == "caller" && t == "helper"), + "call edge caller -> helper must be reported added; stdout: {}", + out.stdout + ); +} + #[test] fn changes_list_json_outputs_machine_readable_payload() { let repo = init_git_repo();