diff --git a/Cargo.lock b/Cargo.lock index 6fca48f..6ebf8a3 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -369,7 +369,7 @@ checksum = "773648b94d0e5d620f64f280777445740e61fe701025087ec8b57f45c791888b" [[package]] name = "corgea" -version = "1.16.1" +version = "1.16.2" dependencies = [ "chrono", "clap", diff --git a/Cargo.toml b/Cargo.toml index 55a82d3..a43f433 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "corgea" -version = "1.16.1" +version = "1.16.2" edition = "2021" # See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html diff --git a/src/incremental.rs b/src/incremental.rs index 1a50137..bee1afd 100644 --- a/src/incremental.rs +++ b/src/incremental.rs @@ -6,12 +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. 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. +//! server for scans of the commits nearest behind HEAD in this checkout's +//! history, whatever branch they were recorded under, and take the one of the +//! closest commit. Only when none of those was scanned -- or there is no +//! history to walk -- fall back to the newest completed scan of this branch, +//! then 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 @@ -50,14 +50,16 @@ 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, HashMap, VecDeque}; +use std::collections::{BTreeSet, HashMap, HashSet, 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; +const ANCESTOR_LOOKUP_COMMITS: usize = 20; +/// How many of those commits one request asks about, nearest first. Must stay +/// within the 50 the scan list's `sha` filter takes in one request. +const ANCESTOR_LOOKUP_BATCH: 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; @@ -215,43 +217,49 @@ fn plan_diff( // 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 - } - }; + // Ancestry before any branch name, because the scan of a commit HEAD + // descends from differs by only what this history has done since. A + // branch's newest scan may be of a commit this checkout is behind or was + // rebased away from, and trunk's newest also differs by whatever trunk did + // after this branch forked. It also finds the earlier runs of a detached + // CI pipeline, which recorded no branch for a name to match. + let mut lookup = BaselineLookup::NotFound; + if let Some(ancestry) = &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 branch lookups every earlier release made. If the endpoint + // itself is down, those fail too and say so. + BaselineLookup::LookupFailed => BaselineLookup::NotFound, + answered => { + commits_searched = true; + answered } - if lookup == BaselineLookup::NotFound { - lookup = find_baseline(config, project_name, Some(trunks), checksums_usable); + }; + } + if lookup == BaselineLookup::NotFound { + lookup = match &candidates { + Some(candidates) => { + // `baseline_branches` puts the branch being scanned first. Its + // own two walks finish before trunk is asked about, so a clean + // scan of this branch behind a page of dirty ones still beats + // trunk's. + let (own, trunks) = candidates.split_at(usize::from(branch.is_some())); + match find_baseline(config, project_name, Some(own), checksums_usable) { + BaselineLookup::NotFound => { + find_baseline(config, project_name, Some(trunks), checksums_usable) + } + found => found, + } } - lookup - } - None => find_baseline(config, project_name, None, checksums_usable), - }; + None => find_baseline(config, project_name, None, checksums_usable), + }; + } let baseline = match lookup { BaselineLookup::Found(scan) => scan, BaselineLookup::NotFound => { @@ -260,15 +268,22 @@ fn plan_diff( against, so there is nothing to compare this one to", 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() + "{}, nor on {},", + match ancestry.shas.len() { + // Only when no parent is reachable: a shallow + // clone's depth, or a repository's first commit. + 1 => "of the only commit in this checkout's history".to_string(), + n => format!( + "of any of the {n} most recent commits in this checkout's \ + history" + ), + }, + join_or(candidates) ), (Some(candidates), None) => format!("on {}", join_or(candidates)), (None, _) => "of its whole state".to_string(), } - )) + )); } BaselineLookup::LookupFailed => { return Err(format!( @@ -436,8 +451,15 @@ struct BaselineScan { /// 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, + /// Whether its commit is on HEAD's first-parent line, when it was picked + /// for being in this run's history. + on_first_parent: bool, } +/// `BaselineScan::ancestor_rank`: commits back, off the first-parent line, +/// diffable only without checksums. Lowest is best. +type AncestorRank = (usize, bool, bool); + impl BaselineScan { fn from_response(scan: &ScanResponse) -> Self { BaselineScan { @@ -450,9 +472,28 @@ impl BaselineScan { .filter(|root| !root.is_empty()), manifest_version: scan.file_manifest_version.clone(), commits_back: None, + on_first_parent: false, } } + /// Order among scans found by ancestry, lowest best: fewest commits back, + /// then one on HEAD's first-parent line, then one this run can diff by its + /// checksums over one it cannot. + /// + /// The first-parent line breaks the tie a merge makes, whose parents are + /// both one commit back. The diff from the first parent is the merge + /// itself; the diff from the other is everything the first line did since + /// the fork, which can outgrow an incremental scan. + fn ancestor_rank(&self, checksums_usable: bool) -> AncestorRank { + let readable_checksums = self.manifest_root.is_some() + && self.manifest_version.as_deref() == Some(MANIFEST_VERSION); + ( + self.commits_back.unwrap_or(usize::MAX), + !self.on_first_parent, + !(checksums_usable && readable_checksums), + ) + } + /// 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) { @@ -488,19 +529,22 @@ enum BaselineLookup { LookupFailed, } -/// The branches a baseline may come from, best first. +/// The branches a baseline may come from, best first, once no scan of a recent +/// commit was found. /// -/// The branch being scanned leads. Its last scan is the nearest ancestor of -/// this one that exists, so the diff against it is the smallest honest one and -/// the findings carried forward are this branch's own. A long-lived branch that -/// has diverged from trunk gets the biggest reduction: against trunk every file -/// it has touched since it forked is "changed", against its own last scan only -/// what moved since that scan is. +/// The branch being scanned leads. Its last scan is usually an ancestor of this +/// one further back than the commit lookup reaches, so the diff against it is +/// the smallest left and the findings carried forward are this branch's own. A +/// long-lived branch that has diverged from trunk gets the biggest reduction: +/// against trunk every file it has touched since it forked is "changed", +/// against its own last scan only what moved since that scan is. /// /// Trunk follows, because a branch on its first scan has no history of its own -/// and trunk is the line it descends from. *Other* branches never qualify: a -/// scan of someone else's feature branch is a baseline whose contents nobody -/// can predict, and the findings copied forward would be that branch's. +/// and trunk is the line it descends from. *Other* branches never qualify by +/// name: the newest scan of someone else's feature branch is a baseline whose +/// contents nobody can predict, and the findings copied forward would be that +/// branch's. The commit lookup may take one, because ancestry proves it covered +/// a tree this history produced. /// /// `origin/HEAD` records what the remote advertised as its default when this /// clone was made. It is absent from single-branch and `actions/checkout` @@ -724,6 +768,8 @@ struct Ancestry { /// Nearest first, HEAD leading. shas: Vec, commits_back: HashMap, + /// Those of `shas` reached from HEAD through first parents alone. + first_parent: HashSet, } impl Ancestry { @@ -736,7 +782,18 @@ impl Ancestry { let mut ancestry = Ancestry { shas: vec![head.id().to_string()], commits_back: HashMap::from([(head.id().to_string(), 0)]), + first_parent: HashSet::from([head.id().to_string()]), }; + // A missing parent ends the line, which is where a shallow clone's + // history ends too. + let mut tip = head.clone(); + while ancestry.first_parent.len() < ANCESTOR_LOOKUP_COMMITS { + let Ok(parent) = tip.parent(0) else { + break; + }; + ancestry.first_parent.insert(parent.id().to_string()); + tip = parent; + } let mut queue = VecDeque::from([(head, 0)]); while let Some((commit, depth)) = queue.pop_front() { for parent_id in commit.parent_ids() { @@ -770,10 +827,10 @@ impl Ancestry { /// 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. +/// Nearest wins because it leaves the fewest files to rescan. The commits are +/// asked about `ANCESTOR_LOOKUP_BATCH` at a time, nearest first, and the walk +/// stops at the first batch with any usable scan: every commit in a later batch +/// is at least as far back, so none of its scans could be closer. fn find_ancestor_baseline( config: &Config, project_name: &str, @@ -782,12 +839,55 @@ fn find_ancestor_baseline( git_diffable: bool, ) -> BaselineLookup { let url = config.get_url(); + for batch in ancestry.shas.chunks(ANCESTOR_LOOKUP_BATCH) { + match search_ancestor_batch( + &url, + project_name, + ancestry, + batch, + checksums_usable, + git_diffable, + ) { + BaselineLookup::NotFound => continue, + answered => return answered, + } + } + BaselineLookup::NotFound +} + +/// The nearest usable scan of any commit in `batch`. +/// +/// The server orders by recency, not by distance from HEAD, so a page full of +/// re-runs of one commit can push a nearer commit's scan onto the next page. +/// Pages are read until none is left, the budget runs out, or the best so far +/// is a scan of the batch's nearest commit, which nothing later can beat. +fn search_ancestor_batch( + url: &str, + project_name: &str, + ancestry: &Ancestry, + batch: &[String], + checksums_usable: bool, + git_diffable: bool, +) -> BaselineLookup { + // Over the whole batch, not its first commit: that one can sit off the + // first-parent line at the same distance as one on it. + let unbeatable: Option = batch + .iter() + .filter_map(|sha| { + let commits_back = *ancestry.commits_back.get(sha)?; + Some((commits_back, !ancestry.first_parent.contains(sha))) + }) + .min() + .map(|(commits_back, off_first_parent)| { + (commits_back, off_first_parent, !checksums_usable) + }); + let mut best: Option = None; for page in 1..=SCAN_LOOKUP_MAX_PAGES { let response = match api::query_baseline_scans_at_commits( - &url, + url, project_name, BLAST_ENGINE, - &ancestry.shas, + batch, // Only a checksum baseline may have been dirty, so without one // the server can drop what could never qualify. !checksums_usable, @@ -796,10 +896,9 @@ fn find_ancestor_baseline( ) { Ok(response) => response, Err(e) => { - crate::log::debug(&format!( - "Baseline lookup by commit failed, trying trunk instead: {e}" - )); - return BaselineLookup::LookupFailed; + crate::log::debug(&format!("Baseline lookup by commit failed: {e}")); + // A scan already read is still a scan of this history. + return best.map_or(BaselineLookup::LookupFailed, BaselineLookup::Found); } }; let scans = response.scans.unwrap_or_default(); @@ -809,7 +908,19 @@ fn find_ancestor_baseline( if let Some(scan) = nearest_ancestor_baseline(&scans, ancestry, checksums_usable, git_diffable) { - return BaselineLookup::Found(scan); + // Strictly better only: earlier pages are newer, and recency + // breaks a tie. + if best.as_ref().is_none_or(|kept| { + scan.ancestor_rank(checksums_usable) < kept.ancestor_rank(checksums_usable) + }) { + best = Some(scan); + } + } + if best + .as_ref() + .is_some_and(|kept| Some(kept.ancestor_rank(checksums_usable)) == unbeatable) + { + break; } if response .total_pages @@ -818,7 +929,7 @@ fn find_ancestor_baseline( break; } } - BaselineLookup::NotFound + best.map_or(BaselineLookup::NotFound, BaselineLookup::Found) } /// The scan on this page nearest behind HEAD that this run can diff from. @@ -843,14 +954,18 @@ fn nearest_ancestor_baseline( .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)) + let baseline = BaselineScan { + commits_back: Some(commits_back), + on_first_parent: ancestry.first_parent.contains(sha), + ..BaselineScan::from_response(scan) + }; + Some(( + (baseline.ancestor_rank(checksums_usable), position), + baseline, + )) }) .min_by_key(|(rank, _)| *rank) - .map(|((commits_back, _, _), scan)| BaselineScan { - commits_back: Some(commits_back), - ..BaselineScan::from_response(scan) - }) + .map(|(_, baseline)| baseline) } /// Whether `scan` may be diffed against, given what this run can diff with. @@ -1577,6 +1692,41 @@ mod tests { assert_eq!(picked.commits_back, Some(2)); } + /// A shallow clone holds its boundary commit's parent ids but not the + /// parents, so the walk ends at the depth instead of reporting commits + /// nothing could diff against. + #[test] + fn a_shallow_clone_offers_only_the_commits_it_holds() { + let dir = tempfile::tempdir().expect("tempdir"); + let origin = dir.path().join("origin"); + let repo = Repository::init(&origin).expect("init"); + let mut tip = commit_on(&repo, &[], "0"); + for i in 1..5 { + tip = commit_on(&repo, &[tip], &i.to_string()); + } + repo.reference("refs/heads/main", tip, true, "test") + .expect("branch"); + repo.set_head("refs/heads/main").expect("HEAD"); + + for depth in [1, 2] { + let clone = dir.path().join(format!("depth-{depth}")); + let status = std::process::Command::new("git") + .args(["clone", "--quiet", "--depth", &depth.to_string()]) + .arg(format!("file://{}", origin.display())) + .arg(&clone) + .status() + .expect("run git clone"); + assert!(status.success(), "git clone --depth {depth} failed"); + let shallow = Repository::open(&clone).expect("open clone"); + assert!(shallow.is_shallow()); + + let ancestry = Ancestry::walk(&shallow, &tip.to_string()).expect("head in clone"); + + assert_eq!(ancestry.shas.len(), depth); + assert_eq!(ancestry.shas.first(), Some(&tip.to_string())); + } + } + #[test] fn a_head_this_clone_does_not_hold_offers_no_ancestry() { let l = lineage(); @@ -1603,10 +1753,47 @@ mod tests { assert_eq!(back(&l.root), Some(2)); assert_eq!(back(&l.head), None); assert_eq!(ancestry.shas.first(), Some(&merge.to_string())); + + let on_first_parent = |sha: &str| ancestry.first_parent.contains(sha); + assert!(on_first_parent(&merge.to_string())); + assert!(on_first_parent(&l.near)); + assert!(on_first_parent(&l.root)); + assert!(!on_first_parent(&side_tip.to_string())); + assert!(!on_first_parent(&l.side)); + } + + /// A merge's parents are both one commit back. The diff from the first is + /// the merge; the diff from the other is everything the first line did + /// since the fork, so the first wins even when the other was scanned later. + #[test] + fn between_the_parents_of_a_merge_the_first_parent_wins() { + 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"); + // Newest first, so the merged-in side would win on recency. + let other_side = with_checksums(scan("feature", &side_tip.to_string())); + let scans = vec![other_side, scan("main", &l.near)]; + + for checksums_usable in [false, true] { + let picked = nearest_ancestor_baseline(&scans, &ancestry, checksums_usable, true) + .expect("a baseline"); + assert_eq!(picked.sha.as_deref(), Some(l.near.as_str())); + assert_eq!(picked.commits_back, Some(1)); + } + + // Still the baseline when the first parent was never scanned. + let picked = + nearest_ancestor_baseline(&scans[..1], &ancestry, true, true).expect("a baseline"); + assert_eq!(picked.sha, Some(side_tip.to_string())); } #[test] - fn the_walk_stops_at_what_one_request_can_ask_about() { + fn the_walk_stops_at_the_commit_lookup_limit() { let dir = tempfile::tempdir().expect("tempdir"); let repo = Repository::init(dir.path()).expect("init"); let mut tip = commit_on(&repo, &[], "0"); diff --git a/tests/cloud_commands_e2e/common/mod.rs b/tests/cloud_commands_e2e/common/mod.rs index 1639164..5c209df 100644 --- a/tests/cloud_commands_e2e/common/mod.rs +++ b/tests/cloud_commands_e2e/common/mod.rs @@ -343,10 +343,11 @@ pub(crate) fn plan_step(plan: &[ExpectedRequest], label: &str) -> usize { /// Every baseline lookup a fixture repo makes when nothing turns up. /// -/// 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, +/// The scans of this checkout's recent commits on any branch first, since the +/// nearest scanned ancestor is the best baseline there could be; the fixture's +/// history fits in one batch. Then the branch being scanned, 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. @@ -373,12 +374,12 @@ pub(crate) fn baseline_lookups_finding_nothing(project: &'static str) -> Vec>, scans: Vec) -> ExpectedRequ ) } +/// One page of the lookup by commit, for the cases that read more than one. +fn ancestor_lookup_page( + shas: Vec, + page: u16, + total_pages: u32, + scans: Vec, +) -> ExpectedRequest { + expected_request( + "look up a baseline scan of a recent commit", + move |request| { + assert_authenticated_request(request, Method::GET, "/api/v1/scans")?; + assert_query(request, "project", PROJECT)?; + assert_query(request, "page", &page.to_string())?; + assert_query(request, "page_size", "50")?; + assert_query(request, "sha", &shas.join(",")) + }, + json_response(json!({ + "status": "ok", + "page": page, + "total_pages": total_pages, + "scans": scans, + })), + ) +} + +/// The lookup by commit every git run makes first, answered with nothing, for +/// the cases about what happens once it falls back to branch names. +fn no_scanned_recent_commit() -> ExpectedRequest { + ancestor_lookup(None, vec![]) +} + /// One page of the baseline lookup, for the walk an old backend forces. fn baseline_lookup_page( branch: &'static str, @@ -208,6 +239,7 @@ fn the_baselines_stored_checksums_are_used_in_preference_to_a_git_diff() { let mut plan = vec![ verify_request(), scan_settings_request(PROJECT), + no_scanned_recent_commit(), baseline_lookup(FIXTURE_BRANCH, vec![scan]), checksum_download(manifest), start_upload(), @@ -326,6 +358,7 @@ fn a_dirty_worktree_is_scanned_incrementally_from_the_stored_checksums() { let mut plan = vec![ verify_request(), scan_settings_request(PROJECT), + no_scanned_recent_commit(), baseline_lookup(FIXTURE_BRANCH, vec![scan]), checksum_download(manifest), start_upload(), @@ -403,6 +436,7 @@ fn an_excluded_run_diffs_its_checksums_and_names_what_it_holds_back() { let mut plan = vec![ verify_request(), scan_settings_request(PROJECT), + no_scanned_recent_commit(), baseline_lookup(FIXTURE_BRANCH, vec![scan]), checksum_download(manifest), start_upload(), @@ -474,6 +508,7 @@ fn an_excluded_run_with_no_checksums_to_diff_scans_everything() { let mut plan = vec![ verify_request(), scan_settings_request(PROJECT), + no_scanned_recent_commit(), baseline_lookup(FIXTURE_BRANCH, vec![baseline_scan(&base_sha)]), start_upload(), expected_request( @@ -539,6 +574,7 @@ fn checksums_that_do_not_match_their_digest_fall_back_to_the_git_diff() { let mut plan = vec![ verify_request(), scan_settings_request(PROJECT), + no_scanned_recent_commit(), baseline_lookup(FIXTURE_BRANCH, vec![scan]), checksum_download(manifest), start_upload(), @@ -585,6 +621,7 @@ fn the_upload_carries_the_baseline_commit_and_the_files_that_changed_since_it() let mut plan = vec![ verify_request(), scan_settings_request(PROJECT), + no_scanned_recent_commit(), baseline_lookup(FIXTURE_BRANCH, vec![baseline_scan(&base_sha)]), start_upload(), expected_request( @@ -670,15 +707,15 @@ 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, nor of any of the 2 \ - most recent commits in this checkout's history that could be diffed against" + "has no completed scan of any of the 2 most recent commits in this checkout's \ + history, nor on {FIXTURE_BRANCH}, main or master, that could be diffed against" )), "{context}" ); } -/// The branch being scanned is asked about first, and its own last scan wins -/// over trunk's. +/// Once no recent commit turns out to have been scanned, the branch being +/// scanned is asked about next, and its own last scan wins over trunk's. /// /// A branch that has been scanned before has a nearer ancestor than trunk does: /// everything it changed before that scan was already analyzed, so diffing @@ -694,15 +731,18 @@ 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 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)]; + // The lookup by commit comes first either way, then the branch being + // scanned. When the baseline is trunk's, those 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), + no_scanned_recent_commit(), + ]; 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([ @@ -735,12 +775,12 @@ 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. +/// The scan of a recent commit is asked about before any branch name, and +/// taken whatever branch it names -- here a trunk called `develop`, which the +/// name lookup never asks about. No branch lookup is made at all once it is +/// found, whether or not the checkout names a branch. #[test] -fn a_scan_of_a_recent_commit_is_the_baseline_when_the_branch_has_none() { +fn a_scan_of_a_recent_commit_is_the_baseline_before_any_branch_is_asked_about() { for detached in [false, true] { let project = git_project(); let base_sha = project.sha.clone(); @@ -753,10 +793,6 @@ fn a_scan_of_a_recent_commit_is_the_baseline_when_the_branch_has_none() { 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), @@ -806,6 +842,209 @@ fn a_scan_of_a_recent_commit_is_the_baseline_when_the_branch_has_none() { } } +/// Commits `count` new files on top of the fixture, one per commit, and +/// returns this checkout's history nearest first. +fn commit_steps(project: &GitProject, count: usize) -> Vec { + for step in 0..count { + let name = format!("step{step}.py"); + std::fs::write(project.path().join(&name), format!("print({step})\n")).expect("write step"); + run_git(project.path(), &["add", &name]); + run_git(project.path(), &["commit", "-m", &name]); + } + String::from_utf8(run_git(project.path(), &["rev-list", "HEAD"]).stdout) + .expect("UTF-8 SHAs") + .lines() + .map(str::to_string) + .collect() +} + +/// Upload asserting the checksum diff named [`BASELINE_SCAN`]. +fn checksum_diff_upload() -> ExpectedRequest { + 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) + }, + json_response(json!({"scan_id": "blast-scan-123", "project_id": 91})), + ) +} + +/// Commits are asked about ten at a time, nearest first, so a scan of a commit +/// further back than the first ten is still found -- by a second request for +/// the next ten, and before any branch is asked about. +#[test] +fn a_scan_beyond_the_first_ten_commits_is_found_by_the_second_batch() { + let project = git_project(); + let history = commit_steps(&project, 11); + let base_sha = history.last().expect("fixture commit").clone(); + assert_eq!(history.len(), 12); + + let (scan, manifest) = baseline_scan_with_checksums(&base_sha, &[("main.py", SOURCE_BODY)]); + let mut plan = vec![ + verify_request(), + scan_settings_request(PROJECT), + ancestor_lookup(Some(history[..10].to_vec()), vec![]), + ancestor_lookup(Some(history[10..].to_vec()), vec![scan]), + checksum_download(manifest), + start_upload(), + checksum_diff_upload(), + ]; + 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: 11 files changed since the scan of ancestor commit {} (11 \ + commits back).", + &base_sha[..7] + )), + "{context}" + ); +} + +/// The server lists scans newest first, not nearest first, so the scan of the +/// closer commit can come back on a later page than one further back. The +/// batch is read to the end, and the closer commit wins. +#[test] +fn the_closest_commit_wins_even_when_its_scan_is_on_a_later_page() { + let project = git_project(); + let history = commit_steps(&project, 2); + let (near_sha, far_sha) = (history[1].clone(), history[2].clone()); + + let (mut far, _) = baseline_scan_with_checksums(&far_sha, &[("main.py", SOURCE_BODY)]); + far["id"] = json!("farther-scan"); + let (near, manifest) = baseline_scan_with_checksums( + &near_sha, + &[("main.py", SOURCE_BODY), ("step0.py", "print(0)\n")], + ); + let mut plan = vec![ + verify_request(), + scan_settings_request(PROJECT), + ancestor_lookup_page(history.clone(), 1, 2, vec![far]), + ancestor_lookup_page(history.clone(), 2, 2, vec![near]), + checksum_download(manifest), + start_upload(), + checksum_diff_upload(), + ]; + 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: 1 file changed since the scan of ancestor commit {} (1 commit \ + back).", + &near_sha[..7] + )), + "{context}" + ); +} + +/// A depth-1 clone, the `actions/checkout` default, holds only HEAD. The lookup +/// by commit asks about that one commit rather than history the clone does not +/// have, then falls back to the branch. The branch's scan is of a commit this +/// clone cannot reach, so only its stored checksums can diff against it; without +/// them the run scans everything and says why. +#[test] +fn a_shallow_clone_asks_about_the_commits_it_holds_then_falls_back_to_the_branch() { + for stored_checksums in [true, false] { + let project = git_project(); + let base_sha = project.sha.clone(); + let head_sha = second_commit(&project); + let clone = tempfile::TempDir::new().expect("create clone"); + run_git( + project.path(), + &[ + "clone", + "--quiet", + "--depth", + "1", + "--branch", + FIXTURE_BRANCH, + &format!("file://{}", project.path().display()), + &clone.path().display().to_string(), + ], + ); + + let mut plan = vec![ + verify_request(), + scan_settings_request(PROJECT), + ancestor_lookup(Some(vec![head_sha.clone()]), vec![]), + ]; + if stored_checksums { + let (scan, manifest) = + baseline_scan_with_checksums(&base_sha, &[("main.py", SOURCE_BODY)]); + plan.extend([ + baseline_lookup(FIXTURE_BRANCH, vec![scan]), + checksum_download(manifest), + start_upload(), + checksum_diff_upload(), + ]); + } else { + plan.extend([ + baseline_lookup(FIXTURE_BRANCH, vec![baseline_scan(&base_sha)]), + start_upload(), + expected_request( + "upload BLAST archive with no diff", + |request| { + assert_authenticated_request( + request, + Method::PATCH, + "/api/v1/start-scan/transfer-123/", + )?; + assert_no_multipart_field(request, "incremental_base_sha")?; + assert_no_multipart_field(request, "incremental_changed_files") + }, + 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, clone.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}"); + let expected = if stored_checksums { + format!( + "Incremental scan: 2 files changed since the last scan of {FIXTURE_BRANCH} ({})", + &base_sha[..7] + ) + } else { + "A shallow clone cannot diff against it".to_string() + }; + assert!(stdout.contains(&expected), "{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. @@ -896,6 +1135,7 @@ fn a_baseline_on_a_later_page_is_still_found() { let mut plan = vec![ verify_request(), scan_settings_request(PROJECT), + no_scanned_recent_commit(), baseline_lookup_page(FIXTURE_BRANCH, 1, 2, vec![unusable]), baseline_lookup_page(FIXTURE_BRANCH, 2, 2, vec![baseline_scan(&base_sha)]), start_upload(), @@ -936,6 +1176,7 @@ fn a_failed_lookup_is_not_reported_as_a_missing_baseline() { let mut plan = vec![ verify_request(), scan_settings_request(PROJECT), + no_scanned_recent_commit(), expected_request( "fail the baseline lookup", |request| assert_baseline_lookup_request(request, PROJECT, Some(FIXTURE_BRANCH), false), @@ -1097,6 +1338,7 @@ fn a_clean_baseline_is_found_even_when_dirty_scans_come_back_first() { let mut plan = vec![ verify_request(), scan_settings_request(PROJECT), + no_scanned_recent_commit(), baseline_lookup(FIXTURE_BRANCH, vec![dirty]), clean_baseline_lookup(FIXTURE_BRANCH, vec![baseline_scan(&base_sha)]), start_upload(), @@ -1194,6 +1436,7 @@ fn ignore_dirty_worktree_diffs_the_working_tree_instead_of_refusing() { let mut plan = vec![ verify_request(), scan_settings_request(PROJECT), + no_scanned_recent_commit(), baseline_lookup(FIXTURE_BRANCH, vec![baseline_scan(&base_sha)]), start_upload(), expected_request( @@ -1260,6 +1503,7 @@ fn a_dirty_worktree_with_no_stored_checksums_scans_everything() { let mut plan = vec![ verify_request(), scan_settings_request(PROJECT), + no_scanned_recent_commit(), baseline_lookup(FIXTURE_BRANCH, vec![baseline_scan(&base_sha)]), start_upload(), expected_request(