diff --git a/src/incremental.rs b/src/incremental.rs index 18b8e42..1a50137 100644 --- a/src/incremental.rs +++ b/src/incremental.rs @@ -6,9 +6,12 @@ //! commits. This module works the diff out where the scan is run. //! //! Finding what to diff *against* is the same in both directions: ask the -//! server for the newest completed scan of this branch, then of trunk if this -//! branch has never been scanned. What changed since it can then be measured -//! two ways. +//! server for the newest completed scan of this branch. If this branch has +//! never been scanned -- or the checkout names none, the detached HEAD a CI +//! checkout usually lands on -- take the scan of the nearest commit in this +//! checkout's recent history, whatever branch it was recorded under, and only +//! then the newest scan of trunk. What changed since the baseline can then be +//! measured two ways. //! //! The first is the file checksums that scan uploaded, fetched and subtracted //! from this run's. It needs no git history, so it is the only one that works @@ -47,11 +50,18 @@ use crate::manifest::{Manifest, MANIFEST_VERSION}; use crate::scanners::blast::{classify_scan_status, ScanState}; use crate::utils::api::{self, ScanResponse}; use git2::Repository; -use std::collections::BTreeSet; +use std::collections::{BTreeSet, HashMap, VecDeque}; /// How many of the project's scans to read at a time, newest first. const SCAN_LOOKUP_PAGE_SIZE: u16 = 30; +/// How many of this checkout's nearest commits a baseline is looked for among. +/// Must stay within the 50 the scan list's `sha` filter takes in one request. +const ANCESTOR_LOOKUP_COMMITS: usize = 10; +/// The most scans per page the scan list returns, so one page normally holds +/// every scan of those commits. +const ANCESTOR_LOOKUP_PAGE_SIZE: u16 = 50; + /// Backstop on pages walked looking for a baseline. /// /// The server filters out scans that cannot be a baseline, so the answer is @@ -139,10 +149,10 @@ impl IncrementalPlan { /// booleans and a caller listing them positionally can swap two without the /// compiler noticing — which would quietly change how the scan is scoped. pub struct DiffSources<'a> { - /// Branch the upload reports. Names where a baseline may come from, and - /// the git diff needs it to have one at all. + /// Branch the upload reports. Its own last scan is the preferred baseline. pub branch: Option<&'a str>, - /// Commit the upload reports, the near side of a git diff. + /// Commit the upload reports: what a baseline found by ancestry has to be + /// an ancestor of, and the near side of a git diff. pub head_sha: Option<&'a str>, /// Whether the worktree holds edits a commit-to-commit diff cannot see. pub worktree_dirty: bool, @@ -189,30 +199,74 @@ fn plan_diff( // Optional, because a checksum diff needs no repository. Only the git diff // below does, and it reports its absence by its real name. let repo = Repository::discover(".").ok(); + let checksums_usable = sources.manifest.is_some(); // Branch names are only meaningful where there is a clone to read them // from. Without one, take the project's newest usable scan whatever branch // it names -- including none, which is what a scan uploaded from a // directory with no git records, and so what the earlier runs of *this* // pipeline recorded. - let candidates = repo - .as_ref() - .map(|repo| baseline_branches(repo, sources.branch.filter(|name| !name.is_empty()))); - - let lookup = find_baseline( - config, - project_name, - candidates.as_deref(), - sources.manifest.is_some(), - ); + let branch = sources.branch.filter(|name| !name.is_empty()); + let candidates = repo.as_ref().map(|repo| baseline_branches(repo, branch)); + let ancestry = match (repo.as_ref(), sources.head_sha) { + (Some(repo), Some(head)) => Ancestry::walk(repo, head), + _ => None, + }; + + // Whether the lookup by commit answered, so the refusal below only claims + // none of those commits was scanned when the server actually said so. + let mut commits_searched = false; + let lookup = match &candidates { + Some(candidates) => { + // `baseline_branches` puts the branch being scanned first. + let (own, trunks) = candidates.split_at(usize::from(branch.is_some())); + let mut lookup = find_baseline(config, project_name, Some(own), checksums_usable); + // Between this branch's own history and trunk's newest scan, + // because the scan of a commit this one descends from differs by + // only what this history has done since. Trunk's newest also + // differs by whatever trunk did after this branch forked, and may + // not be this branch's trunk at all where that is not main or + // master. It also finds the earlier runs of a detached CI pipeline, + // which recorded no branch for a name to match. + if let (BaselineLookup::NotFound, Some(ancestry)) = (&lookup, &ancestry) { + lookup = match find_ancestor_baseline( + config, + project_name, + ancestry, + checksums_usable, + git_diff_refusal(sources).is_none(), + ) { + // A backend that cannot answer a list of commits must not + // cost the trunk lookup every earlier release made. If the + // endpoint itself is down, that lookup fails and says so. + BaselineLookup::LookupFailed => BaselineLookup::NotFound, + answered => { + commits_searched = true; + answered + } + }; + } + if lookup == BaselineLookup::NotFound { + lookup = find_baseline(config, project_name, Some(trunks), checksums_usable); + } + lookup + } + None => find_baseline(config, project_name, None, checksums_usable), + }; let baseline = match lookup { BaselineLookup::Found(scan) => scan, BaselineLookup::NotFound => { return Err(format!( "project '{project_name}' has no completed scan {} that could be diffed \ against, so there is nothing to compare this one to", - match &candidates { - Some(candidates) => format!("on {}", join_or(candidates)), - None => "of its whole state".to_string(), + match (&candidates, ancestry.as_ref().filter(|_| commits_searched)) { + (Some(candidates), Some(ancestry)) => format!( + "on {}, nor of any of the {} most recent commits in this checkout's \ + history", + join_or(candidates), + ancestry.shas.len() + ), + (Some(candidates), None) => format!("on {}", join_or(candidates)), + (None, _) => "of its whole state".to_string(), } )) } @@ -272,50 +326,28 @@ fn plan_git_diff( repo: Option<&Repository>, sources: &DiffSources<'_>, ) -> Result<(IncrementalPlan, String), String> { - // git diffs the repository, and --exclude means the archive is not it. An - // excluded file git reports unchanged is left off the list, so the server - // copies its findings forward over a file this upload does not contain -- - // and no later run under the same --exclude will look at it either. First, - // because these runs report dirty whatever the worktree holds, so the check - // below would otherwise answer for a tree with nothing uncommitted in it. - if sources.exclude_narrowed { - return Err( - "--exclude held files back from this archive, so a git diff of the repository \ - would not describe it (its stored file checksums would, on the next run)" - .to_string(), - ); + if let Some(refusal) = git_diff_refusal(sources) { + return Err(refusal); } - - // A commit-to-commit diff cannot see uncommitted edits, so on a dirty tree - // it leaves modified files off the list and their old findings are copied - // forward as current. --ignore-dirty-worktree does not paper over that; it - // switches the diff to measure the working tree, so those files are named - // and rescanned like any other change. let covers_worktree = sources.worktree_dirty; - if sources.worktree_dirty && !sources.ignore_dirty_worktree { - return Err( - "this worktree has uncommitted changes that a commit-to-commit diff cannot \ - see. Pass --ignore-dirty-worktree to diff the working tree instead" - .to_string(), - ); - } // Nothing to diff from. Covers a non-git directory, a repo with no commit, - // a detached HEAD, and a scan started below the repo root — none of which - // report RepoInfo to the upload either. - let (Some(_branch), Some(head_sha), Some(repo), Some(base_sha)) = ( - sources.branch, - sources.head_sha, - repo, - baseline.sha.as_deref(), - ) else { + // and a scan started below the repo root — none of which report RepoInfo + // to the upload either. A detached HEAD is not one of them: a diff between + // two commits needs no branch. + let (Some(head_sha), Some(repo)) = (sources.head_sha, repo) else { return Err( - "there is no git branch and commit to diff from either (not a git \ - repository, no commit yet, a detached HEAD, or a scan started below the \ - repository root)" + "there is no git commit to diff from either (not a git repository, no \ + commit yet, or a scan started below the repository root)" .to_string(), ); }; + let Some(base_sha) = baseline.sha.as_deref() else { + return Err(format!( + "the {} recorded no commit for a git diff to start from", + baseline.describe() + )); + }; let changed_files = changed_files_since(repo, base_sha, head_sha, covers_worktree)?; let since = if covers_worktree { @@ -401,11 +433,38 @@ struct BaselineScan { /// Digest of the file manifest it stored, `None` when it stored none. manifest_root: Option, manifest_version: Option, + /// Commits between its commit and this run's, when it was picked for + /// being in this run's history rather than for the branch it names. + commits_back: Option, } impl BaselineScan { + fn from_response(scan: &ScanResponse) -> Self { + BaselineScan { + id: scan.id.clone(), + sha: scan.git_sha.clone().filter(|sha| !sha.is_empty()), + branch: scan.branch.clone(), + manifest_root: scan + .file_manifest_root + .clone() + .filter(|root| !root.is_empty()), + manifest_version: scan.file_manifest_version.clone(), + commits_back: None, + } + } + /// How to refer to this scan in the one line the run prints. fn describe(&self) -> String { + if let (Some(commits_back), Some(sha)) = (self.commits_back, &self.sha) { + return match commits_back { + 0 => format!("scan of this commit ({})", short_sha(sha)), + 1 => format!("scan of ancestor commit {} (1 commit back)", short_sha(sha)), + n => format!( + "scan of ancestor commit {} ({n} commits back)", + short_sha(sha) + ), + }; + } let what = match &self.branch { Some(branch) => format!("last scan of {branch}"), None => "last scan of this project".to_string(), @@ -623,16 +682,175 @@ fn branch_baseline( } else { newest? }; - Some(BaselineScan { - id: scan.id.clone(), - sha: scan.git_sha.clone().filter(|sha| !sha.is_empty()), - branch: scan.branch.clone(), - manifest_root: scan - .file_manifest_root - .clone() - .filter(|root| !root.is_empty()), - manifest_version: scan.file_manifest_version.clone(), - }) + Some(BaselineScan::from_response(scan)) +} + +/// Why this run cannot diff from any baseline's commit, only from stored +/// checksums. One function for both `plan_git_diff` and the ancestor lookup, +/// which skips commit-only baselines when this says no. +fn git_diff_refusal(sources: &DiffSources<'_>) -> Option { + // git diffs the repository, and --exclude means the archive is not it. An + // excluded file git reports unchanged is left off the list, so the server + // copies its findings forward over a file this upload does not contain -- + // and no later run under the same --exclude will look at it either. First, + // because these runs report dirty whatever the worktree holds, so the check + // below would otherwise answer for a tree with nothing uncommitted in it. + if sources.exclude_narrowed { + return Some( + "--exclude held files back from this archive, so a git diff of the repository \ + would not describe it (its stored file checksums would, on the next run)" + .to_string(), + ); + } + + // A commit-to-commit diff cannot see uncommitted edits, so on a dirty tree + // it leaves modified files off the list and their old findings are copied + // forward as current. --ignore-dirty-worktree does not paper over that; it + // switches the diff to measure the working tree, so those files are named + // and rescanned like any other change. + if sources.worktree_dirty && !sources.ignore_dirty_worktree { + return Some( + "this worktree has uncommitted changes that a commit-to-commit diff cannot \ + see. Pass --ignore-dirty-worktree to diff the working tree instead" + .to_string(), + ); + } + None +} + +/// HEAD and the commits nearest behind it in this clone, with how far back +/// each sits. +struct Ancestry { + /// Nearest first, HEAD leading. + shas: Vec, + commits_back: HashMap, +} + +impl Ancestry { + /// Breadth-first over every parent, so the far side of a merge counts as + /// history too, stopping at `ANCESTOR_LOOKUP_COMMITS`. None when + /// `head_sha` is not a commit this clone holds. A shallow clone's history + /// ends at its depth, and so does this. + fn walk(repo: &Repository, head_sha: &str) -> Option { + let head = repo.find_commit(git2::Oid::from_str(head_sha).ok()?).ok()?; + let mut ancestry = Ancestry { + shas: vec![head.id().to_string()], + commits_back: HashMap::from([(head.id().to_string(), 0)]), + }; + let mut queue = VecDeque::from([(head, 0)]); + while let Some((commit, depth)) = queue.pop_front() { + for parent_id in commit.parent_ids() { + if ancestry.shas.len() >= ANCESTOR_LOOKUP_COMMITS { + return Some(ancestry); + } + let sha = parent_id.to_string(); + if ancestry.commits_back.contains_key(&sha) { + continue; + } + let Ok(parent) = repo.find_commit(parent_id) else { + continue; + }; + ancestry.commits_back.insert(sha.clone(), depth + 1); + ancestry.shas.push(sha); + queue.push_back((parent, depth + 1)); + } + } + Some(ancestry) + } +} + +/// The scan whose commit is nearest behind HEAD among this checkout's recent +/// commits, on any branch. +/// +/// Any branch, including none, because ancestry settles what the branch name +/// was standing in for. A scan of a commit HEAD descends from covered a tree +/// this history produced, so the diff from it is exactly what this history has +/// done since -- whether the scan was recorded under trunk, under the branch +/// this one forked from, or, from a detached CI checkout, under no branch at +/// all. The last is what every earlier run of a pipeline that checks out +/// detached records, and a lookup keyed on names never finds them. +/// +/// Nearest wins because it leaves the fewest files to rescan. The commits +/// travel in the request, so a page normally holds every candidate. Stops at +/// the first page holding any: the pages after it are for a backend that +/// ignored the filter and is listing the whole history. +fn find_ancestor_baseline( + config: &Config, + project_name: &str, + ancestry: &Ancestry, + checksums_usable: bool, + git_diffable: bool, +) -> BaselineLookup { + let url = config.get_url(); + for page in 1..=SCAN_LOOKUP_MAX_PAGES { + let response = match api::query_baseline_scans_at_commits( + &url, + project_name, + BLAST_ENGINE, + &ancestry.shas, + // Only a checksum baseline may have been dirty, so without one + // the server can drop what could never qualify. + !checksums_usable, + page, + ANCESTOR_LOOKUP_PAGE_SIZE, + ) { + Ok(response) => response, + Err(e) => { + crate::log::debug(&format!( + "Baseline lookup by commit failed, trying trunk instead: {e}" + )); + return BaselineLookup::LookupFailed; + } + }; + let scans = response.scans.unwrap_or_default(); + if scans.is_empty() { + break; + } + if let Some(scan) = + nearest_ancestor_baseline(&scans, ancestry, checksums_usable, git_diffable) + { + return BaselineLookup::Found(scan); + } + if response + .total_pages + .is_some_and(|total| u32::from(page) >= total) + { + break; + } + } + BaselineLookup::NotFound +} + +/// The scan on this page nearest behind HEAD that this run can diff from. +/// +/// Matched against the walked commits client-side too: a backend that ignored +/// the filter would otherwise hand back a scan of any commit. A scan only +/// diffable by its commit is no use to a run that cannot git diff (a dirty +/// tree without `--ignore-dirty-worktree`, or an `--exclude`d archive), so +/// there it has to carry checksums. Between scans of the same commit, +/// checksums win, then recency. +fn nearest_ancestor_baseline( + scans: &[ScanResponse], + ancestry: &Ancestry, + checksums_usable: bool, + git_diffable: bool, +) -> Option { + scans + .iter() + .enumerate() + .filter(|(_, scan)| is_usable_baseline(scan, checksums_usable)) + .filter(|(_, scan)| git_diffable || (checksums_usable && has_readable_checksums(scan))) + .filter_map(|(position, scan)| { + let sha = scan.git_sha.as_deref()?; + let commits_back = *ancestry.commits_back.get(sha)?; + let without_checksums = !(checksums_usable && has_readable_checksums(scan)); + Some(((commits_back, without_checksums, position), scan)) + }) + .min_by_key(|(rank, _)| *rank) + .map(|((commits_back, _, _), scan)| BaselineScan { + commits_back: Some(commits_back), + ..BaselineScan::from_response(scan) + }) } /// Whether `scan` may be diffed against, given what this run can diff with. @@ -1227,6 +1445,208 @@ mod tests { assert!(err.contains("submodule vendor"), "{err}"); } + /// Commit with `parents`, adding `name` to the first parent's tree. + /// Leaves HEAD alone, so a test can grow side branches off any commit. + fn commit_on(repo: &Repository, parents: &[git2::Oid], name: &str) -> git2::Oid { + let sig = git2::Signature::now("t", "t@example.com").expect("sig"); + let parents: Vec = parents + .iter() + .map(|oid| repo.find_commit(*oid).expect("parent")) + .collect(); + let parent_tree = parents.first().map(|c| c.tree().expect("parent tree")); + let mut builder = repo.treebuilder(parent_tree.as_ref()).expect("treebuilder"); + let blob = repo.blob(name.as_bytes()).expect("blob"); + builder.insert(name, blob, 0o100644).expect("insert"); + let tree = repo + .find_tree(builder.write().expect("write tree")) + .expect("find tree"); + let parent_refs: Vec<&git2::Commit> = parents.iter().collect(); + repo.commit(None, &sig, &sig, name, &tree, &parent_refs) + .expect("commit") + } + + /// root -> near -> head on one line, and `side` forked off root. + struct Lineage { + _dir: tempfile::TempDir, + repo: Repository, + root: String, + near: String, + head: String, + side: String, + } + + fn lineage() -> Lineage { + let dir = tempfile::tempdir().expect("tempdir"); + let repo = Repository::init(dir.path()).expect("init"); + let root = commit_on(&repo, &[], "root.txt"); + let near = commit_on(&repo, &[root], "near.txt"); + let head = commit_on(&repo, &[near], "head.txt"); + let side = commit_on(&repo, &[root], "side.txt"); + Lineage { + _dir: dir, + repo, + root: root.to_string(), + near: near.to_string(), + head: head.to_string(), + side: side.to_string(), + } + } + + /// A scan uploaded from a detached CI checkout: a commit, no branch. + fn detached_scan(sha: &str) -> ScanResponse { + let mut scan = scan("unused", sha); + scan.branch = None; + scan + } + + #[test] + fn the_nearest_ancestor_scan_wins_whatever_branch_it_names() { + let l = lineage(); + let ancestry = Ancestry::walk(&l.repo, &l.head).expect("head in clone"); + // Newest first: a scan of a sibling branch, one of a commit this clone + // never fetched, then two ancestors -- the nearer recorded detached. + let scans = vec![ + scan("feature", &l.side), + scan("other", &"f".repeat(40)), + scan("main", &l.root), + detached_scan(&l.near), + ]; + + let picked = nearest_ancestor_baseline(&scans, &ancestry, false, true).expect("a baseline"); + + assert_eq!(picked.sha.as_deref(), Some(l.near.as_str())); + assert_eq!(picked.commits_back, Some(1)); + assert_eq!( + picked.describe(), + format!("scan of ancestor commit {} (1 commit back)", &l.near[..7]) + ); + } + + #[test] + fn a_scan_of_head_itself_is_zero_commits_back() { + let l = lineage(); + let ancestry = Ancestry::walk(&l.repo, &l.head).expect("head in clone"); + let scans = vec![scan("main", &l.near), detached_scan(&l.head)]; + + let picked = nearest_ancestor_baseline(&scans, &ancestry, false, true).expect("a baseline"); + + assert_eq!(picked.commits_back, Some(0)); + } + + #[test] + fn no_ancestor_on_the_page_means_no_ancestry_baseline() { + let l = lineage(); + let ancestry = Ancestry::walk(&l.repo, &l.head).expect("head in clone"); + let scans = vec![scan("feature", &l.side), scan_without_git("no-git")]; + + assert_eq!( + nearest_ancestor_baseline(&scans, &ancestry, true, true), + None + ); + } + + #[test] + fn between_scans_of_one_commit_the_one_with_checksums_wins() { + let l = lineage(); + let ancestry = Ancestry::walk(&l.repo, &l.head).expect("head in clone"); + let mut plain = detached_scan(&l.near); + plain.id = "plain".to_string(); + let mut hashed = with_checksums(detached_scan(&l.near)); + hashed.id = "hashed".to_string(); + + let picked = + nearest_ancestor_baseline(&[plain, hashed], &ancestry, true, true).expect("a baseline"); + + assert_eq!(picked.id, "hashed"); + } + + #[test] + fn a_run_that_cannot_git_diff_needs_an_ancestor_with_checksums() { + // A dirty tree without --ignore-dirty-worktree, or an --exclude run: + // the nearer scan could only be diffed from its commit. + let l = lineage(); + let ancestry = Ancestry::walk(&l.repo, &l.head).expect("head in clone"); + let scans = vec![ + detached_scan(&l.near), + with_checksums(detached_scan(&l.root)), + ]; + + let picked = nearest_ancestor_baseline(&scans, &ancestry, true, false).expect("a baseline"); + + assert_eq!(picked.sha.as_deref(), Some(l.root.as_str())); + assert_eq!(picked.commits_back, Some(2)); + } + + #[test] + fn a_head_this_clone_does_not_hold_offers_no_ancestry() { + let l = lineage(); + assert!(Ancestry::walk(&l.repo, &"0".repeat(40)).is_none()); + assert!(Ancestry::walk(&l.repo, "not-a-sha").is_none()); + } + + #[test] + fn the_walk_follows_both_sides_of_a_merge_and_skips_other_branches() { + let l = lineage(); + let side_tip = commit_on(&l.repo, &[git2::Oid::from_str(&l.side).unwrap()], "tip.txt"); + let merge = commit_on( + &l.repo, + &[git2::Oid::from_str(&l.near).unwrap(), side_tip], + "merge.txt", + ); + + let ancestry = Ancestry::walk(&l.repo, &merge.to_string()).expect("head in clone"); + + let back = |sha: &str| ancestry.commits_back.get(sha).copied(); + assert_eq!(back(&l.near), Some(1)); + assert_eq!(back(&side_tip.to_string()), Some(1)); + assert_eq!(back(&l.side), Some(2)); + assert_eq!(back(&l.root), Some(2)); + assert_eq!(back(&l.head), None); + assert_eq!(ancestry.shas.first(), Some(&merge.to_string())); + } + + #[test] + fn the_walk_stops_at_what_one_request_can_ask_about() { + let dir = tempfile::tempdir().expect("tempdir"); + let repo = Repository::init(dir.path()).expect("init"); + let mut tip = commit_on(&repo, &[], "0"); + for i in 1..=ANCESTOR_LOOKUP_COMMITS + 5 { + tip = commit_on(&repo, &[tip], &i.to_string()); + } + + let ancestry = Ancestry::walk(&repo, &tip.to_string()).expect("head in clone"); + + assert_eq!(ancestry.shas.len(), ANCESTOR_LOOKUP_COMMITS); + assert_eq!( + ancestry.commits_back.get(ancestry.shas.last().unwrap()), + Some(&(ANCESTOR_LOOKUP_COMMITS - 1)) + ); + } + + #[test] + fn a_git_diff_needs_no_branch() { + // A detached HEAD reports a commit and no branch; a diff between two + // commits needs nothing more. + let (_dir, repo, base, head) = repo_with_history(); + let baseline = BaselineScan::from_response(&detached_scan(&base)); + let sources = DiffSources { + branch: None, + head_sha: Some(&head), + worktree_dirty: false, + ignore_dirty_worktree: false, + exclude_narrowed: false, + manifest: None, + }; + + let (plan, _) = plan_git_diff(&baseline, Some(&repo), &sources).expect("a plan"); + + assert_eq!(plan.base, BaselineRef::Commit(base)); + assert_eq!( + plan.changed_files, + vec!["added.txt", "edit.txt", "gone.txt"] + ); + } + #[test] fn a_base_commit_this_clone_does_not_have_is_reported_not_panicked() { let (_dir, repo, _base, head) = repo_with_history(); diff --git a/src/utils/api.rs b/src/utils/api.rs index a31b547..01c94b0 100644 --- a/src/utils/api.rs +++ b/src/utils/api.rs @@ -1241,6 +1241,41 @@ pub fn query_baseline_scans( page: u16, page_size: u16, ) -> Result> { + let mut query_params = baseline_scan_params(project, engine, require_clean, page, page_size); + if let Some(branch) = branch { + query_params.push(("branch", branch.to_string())); + } + request_scan_list(url, query_params) +} + +/// One page of the project's scans of any of `shas` that could be diffed +/// against, on any branch, newest first. +/// +/// A backend that takes a single `sha` reads the list as one literal commit +/// and matches nothing. One predating the `sha` filter ignores it and answers +/// with scans at any commit, so callers must re-check `git_sha` on every scan +/// they act on. +pub fn query_baseline_scans_at_commits( + url: &str, + project: &str, + engine: &str, + shas: &[String], + require_clean: bool, + page: u16, + page_size: u16, +) -> Result> { + let mut query_params = baseline_scan_params(project, engine, require_clean, page, page_size); + query_params.push(("sha", shas.join(","))); + request_scan_list(url, query_params) +} + +fn baseline_scan_params( + project: &str, + engine: &str, + require_clean: bool, + page: u16, + page_size: u16, +) -> Vec<(&'static str, String)> { let mut query_params = vec![ ("page", page.to_string()), ("page_size", page_size.to_string()), @@ -1252,13 +1287,10 @@ pub fn query_baseline_scans( // so copying forward from one would drop everything else. ("full_project_state", "true".to_string()), ]; - if let Some(branch) = branch { - query_params.push(("branch", branch.to_string())); - } if require_clean { query_params.push(("worktree_dirty", "false".to_string())); } - request_scan_list(url, query_params) + query_params } /// One page of the project's scans at exactly `sha`, newest first. diff --git a/tests/cloud_commands_e2e/common/mod.rs b/tests/cloud_commands_e2e/common/mod.rs index b67b041..1639164 100644 --- a/tests/cloud_commands_e2e/common/mod.rs +++ b/tests/cloud_commands_e2e/common/mod.rs @@ -343,26 +343,87 @@ pub(crate) fn plan_step(plan: &[ExpectedRequest], label: &str) -> usize { /// Every baseline lookup a fixture repo makes when nothing turns up. /// -/// Two walks over the branch candidates: the one being scanned, whose last scan -/// is the nearest baseline there could be, then the two trunk names, since the -/// fixture records no origin/HEAD. The first walk takes a baseline of either -/// kind, so it cannot let the server drop scans that are not known-clean; the -/// second asks for exactly those once no checksums have been found, so a clean -/// scan behind a page budget's worth of dirty ones is still reachable. +/// The branch being scanned first, whose last scan is the nearest baseline +/// there could be; then the scans of this checkout's recent commits on any +/// branch; then the two trunk names, since the fixture records no origin/HEAD. +/// Each branch lookup is two walks. The first takes a baseline of either kind, +/// so it cannot let the server drop scans that are not known-clean; the second +/// asks for exactly those once no checksums have been found, so a clean scan +/// behind a page budget's worth of dirty ones is still reachable. pub(crate) fn baseline_lookups_finding_nothing(project: &'static str) -> Vec { - let mut lookups = Vec::new(); - for require_clean in [false, true] { - for branch in BASELINE_BRANCH_ORDER { - lookups.push(expected_request( - "look up a baseline scan to diff against", - move |request| { - assert_baseline_lookup_request(request, project, Some(branch), require_clean) - }, - json_response(scans_response(Vec::new())), - )); + let (own, trunks) = BASELINE_BRANCH_ORDER.split_at(1); + let branch_walks = |branches: &[&'static str]| -> Vec { + [false, true] + .into_iter() + .flat_map(|require_clean| { + branches.iter().map(move |&branch| { + expected_request( + "look up a baseline scan to diff against", + move |request| { + assert_baseline_lookup_request( + request, + project, + Some(branch), + require_clean, + ) + }, + json_response(scans_response(Vec::new())), + ) + }) + }) + .collect() + }; + let mut lookups = branch_walks(own); + lookups.push(expected_request( + "look up a baseline scan of a recent commit", + move |request| assert_ancestor_lookup_request(request, project, None, false), + json_response(scans_response(Vec::new())), + )); + lookups.extend(branch_walks(trunks)); + lookups +} + +/// The lookup by commit: the checkout's recent commits in one `sha` list, on +/// any branch. `shas`, when given, is the exact list expected, nearest first; +/// otherwise any list of full commit hashes passes. +pub(crate) fn assert_ancestor_lookup_request( + request: &CapturedRequest, + project: &str, + shas: Option<&[String]>, + require_clean: bool, +) -> Result<(), String> { + assert_authenticated_request(request, Method::GET, "/api/v1/scans")?; + assert_query(request, "page", "1")?; + assert_query(request, "page_size", "50")?; + assert_query(request, "project", project)?; + assert_query(request, "engine", "corgea-blast")?; + assert_query(request, "status", "complete")?; + assert_query(request, "exclude_pull_requests", "true")?; + assert_query(request, "full_project_state", "true")?; + if let Ok(branch) = query_value(request, "branch") { + return Err(format!( + "a lookup by commit must not ask for branch {branch}" + )); + } + match (require_clean, query_value(request, "worktree_dirty")) { + (true, Ok(value)) if value == "false" => {} + (false, Err(_)) => {} + (_, value) => return Err(format!("unexpected worktree_dirty filter {value:?}")), + } + let sent = query_value(request, "sha")?; + match shas { + Some(shas) if sent != shas.join(",") => { + Err(format!("expected sha={}, got {sent}", shas.join(","))) + } + Some(_) => Ok(()), + None if sent + .split(',') + .all(|sha| sha.len() == 40 && sha.bytes().all(|b| b.is_ascii_hexdigit())) => + { + Ok(()) } + None => Err(format!("expected a list of commit hashes, got sha={sent}")), } - lookups } /// One baseline lookup an incremental scan makes before uploading. diff --git a/tests/cloud_commands_e2e/scan_incremental.rs b/tests/cloud_commands_e2e/scan_incremental.rs index 67beae1..0663911 100644 --- a/tests/cloud_commands_e2e/scan_incremental.rs +++ b/tests/cloud_commands_e2e/scan_incremental.rs @@ -115,6 +115,15 @@ fn clean_baseline_lookup(branch: &'static str, scans: Vec) -> ExpectedReq ) } +/// The lookup by commit, over this checkout's recent history on any branch. +fn ancestor_lookup(shas: Option>, scans: Vec) -> ExpectedRequest { + expected_request( + "look up a baseline scan of a recent commit", + move |request| assert_ancestor_lookup_request(request, PROJECT, shas.as_deref(), false), + json_response(scans_response(scans)), + ) +} + /// One page of the baseline lookup, for the walk an old backend forces. fn baseline_lookup_page( branch: &'static str, @@ -661,8 +670,8 @@ fn a_project_with_no_baseline_scan_uploads_without_a_diff() { assert_eq!(output.status.code(), Some(0), "{context}"); assert!( stdout.contains(&format!( - "has no completed scan on {FIXTURE_BRANCH}, main or master that could be diffed \ - against" + "has no completed scan on {FIXTURE_BRANCH}, main or master, nor of any of the 2 \ + most recent commits in this checkout's history that could be diffed against" )), "{context}" ); @@ -686,11 +695,14 @@ fn the_branch_being_scanned_is_preferred_over_trunk_and_falls_back_to_it() { let mut scan = baseline_scan(&base_sha); scan["branch"] = json!(baseline_branch); // The branch being scanned is asked about first either way. When the - // baseline is trunk's, that lookup comes back empty and the next one - // finds it -- so the plan is the assertion that the order is what it is. + // baseline is trunk's, that lookup and the one by commit come back + // empty and the next one finds it -- so the plan is the assertion that + // the order is what it is. let mut plan = vec![verify_request(), scan_settings_request(PROJECT)]; if baseline_branch != FIXTURE_BRANCH { plan.push(baseline_lookup(FIXTURE_BRANCH, vec![])); + plan.push(clean_baseline_lookup(FIXTURE_BRANCH, vec![])); + plan.push(ancestor_lookup(None, vec![])); } let expected_base = base_sha.clone(); plan.extend([ @@ -723,6 +735,151 @@ fn the_branch_being_scanned_is_preferred_over_trunk_and_falls_back_to_it() { } } +/// A branch with no scan of its own diffs against the scan of the commit it +/// forked from, whatever branch that scan names -- here a trunk called +/// `develop`, which the name lookup never asks about. A detached HEAD, which +/// names no branch to look up at all, gets the same answer. +#[test] +fn a_scan_of_a_recent_commit_is_the_baseline_when_the_branch_has_none() { + for detached in [false, true] { + let project = git_project(); + let base_sha = project.sha.clone(); + let head_sha = second_commit(&project); + if detached { + run_git(project.path(), &["checkout", "--detach"]); + } + + let (mut scan, manifest) = + baseline_scan_with_checksums(&base_sha, &[("main.py", SOURCE_BODY)]); + scan["branch"] = json!("develop"); + let mut plan = vec![verify_request(), scan_settings_request(PROJECT)]; + if !detached { + plan.push(baseline_lookup(FIXTURE_BRANCH, vec![])); + plan.push(clean_baseline_lookup(FIXTURE_BRANCH, vec![])); + } + plan.extend([ + ancestor_lookup(Some(vec![head_sha.clone(), base_sha.clone()]), vec![scan]), + checksum_download(manifest), + start_upload(), + expected_request( + "upload BLAST archive with the checksum diff", + |request| { + assert_authenticated_request( + request, + Method::PATCH, + "/api/v1/start-scan/transfer-123/", + )?; + assert_multipart_text_field( + request, + "incremental_base_scan_id", + BASELINE_SCAN, + )?; + assert_multipart_text_field( + request, + "incremental_changed_files", + r#"["helper.py","main.py"]"#, + ) + }, + json_response(json!({"scan_id": "blast-scan-123", "project_id": 91})), + ), + ]); + plan.extend(scan_tail()); + + let api = ApiStub::start(plan); + let (mut command, _home) = cloud_command(&api, project.path()); + command.args(["scan", "blast", "--project-name", PROJECT]); + + let output = run_with_timeout(command, &api); + let transcript = api.assert_finished(); + let context = output_context(&output, &transcript); + let stdout = String::from_utf8_lossy(&output.stdout); + + assert_eq!(output.status.code(), Some(0), "{context}"); + assert!( + stdout.contains(&format!( + "Incremental scan: 2 files changed since the scan of ancestor commit {} (1 \ + commit back).", + &base_sha[..7] + )), + "{context}" + ); + } +} + +/// A detached HEAD has no branch lookup to make, and when none of its recent +/// commits was scanned it still reaches trunk -- through a git diff here, which +/// needs the two commits and no branch. +/// +/// So does a lookup by commit that fails. A backend that cannot answer a list +/// of commits must not cost the trunk baseline every earlier release found. +#[test] +fn a_detached_head_with_no_scanned_recent_commit_falls_back_to_trunk() { + for lookup_by_commit_fails in [false, true] { + assert_detached_head_falls_back_to_trunk(lookup_by_commit_fails); + } +} + +fn assert_detached_head_falls_back_to_trunk(lookup_by_commit_fails: bool) { + let project = git_project(); + let base_sha = project.sha.clone(); + let head_sha = second_commit(&project); + run_git(project.path(), &["checkout", "--detach"]); + + let mut scan = baseline_scan(&base_sha); + scan["branch"] = json!("main"); + let expected_base = base_sha.clone(); + let shas = vec![head_sha, base_sha]; + let lookup_by_commit = if lookup_by_commit_fails { + expected_request( + "fail the baseline lookup by commit", + move |request| assert_ancestor_lookup_request(request, PROJECT, Some(&shas), false), + json_response_with_status(StatusCode::BAD_REQUEST, json!({"error": "bad sha"})), + ) + } else { + ancestor_lookup(Some(shas), vec![]) + }; + let mut plan = vec![ + verify_request(), + scan_settings_request(PROJECT), + lookup_by_commit, + baseline_lookup("main", vec![scan]), + start_upload(), + expected_request( + "upload BLAST archive with the git diff", + move |request| { + assert_authenticated_request( + request, + Method::PATCH, + "/api/v1/start-scan/transfer-123/", + )?; + assert_multipart_text_field(request, "incremental_base_sha", &expected_base)?; + assert_multipart_text_field( + request, + "incremental_changed_files", + r#"["helper.py","main.py"]"#, + ) + }, + json_response(json!({"scan_id": "blast-scan-123", "project_id": 91})), + ), + ]; + plan.extend(scan_tail()); + + let api = ApiStub::start(plan); + let (mut command, _home) = cloud_command(&api, project.path()); + command.args(["scan", "blast", "--project-name", PROJECT]); + + let output = run_with_timeout(command, &api); + let transcript = api.assert_finished(); + let context = output_context(&output, &transcript); + let stdout = String::from_utf8_lossy(&output.stdout); + + assert_eq!(output.status.code(), Some(0), "{context}"); + assert!( + stdout.contains("Incremental scan: 2 files changed since commit"), + "{context}" + ); +} + /// A backend predating the server-side filters returns scans of every kind, so /// a page can hold nothing usable. The walk is what stops that project from /// being permanently unable to find a baseline it has. @@ -941,8 +1098,6 @@ fn a_clean_baseline_is_found_even_when_dirty_scans_come_back_first() { verify_request(), scan_settings_request(PROJECT), baseline_lookup(FIXTURE_BRANCH, vec![dirty]), - baseline_lookup("main", vec![]), - baseline_lookup("master", vec![]), clean_baseline_lookup(FIXTURE_BRANCH, vec![baseline_scan(&base_sha)]), start_upload(), expected_request(