Skip to content

fix: cli changes diff #1464

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
fayekelmith wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix: cli changes diff #1464

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
117 changes: 55 additions & 62 deletions cli/src/changes.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -494,22 +494,20 @@ async fn run_diff(
canon_repo_str.to_string()
};

let changed_abs: HashSet<String> = after_files
let changed_after_rel: HashSet<String> = 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<String> = before_files
let changed_before_rel: HashSet<String> = 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<String> = after_by_key.keys().cloned().collect();
let before_keys: HashSet<String> = before_by_key.keys().cloned().collect();
Expand Down Expand Up @@ -741,27 +739,41 @@ fn node_signature(node: &Node) -> Option<String> {
})
}

// 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<String> {
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
Expand All @@ -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);
Expand All @@ -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,
Expand All @@ -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;
}
}
Expand All @@ -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(
Expand Down
2 changes: 1 addition & 1 deletion cli/src/git.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<Option<Vec<u8>>> {
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)))?;
Expand Down
154 changes: 154 additions & 0 deletions cli/tests/cli/changes_cmd.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<String> {
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();
Expand Down
Loading