diff --git a/_typos.toml b/_typos.toml index ee848258..1b1224be 100644 --- a/_typos.toml +++ b/_typos.toml @@ -13,3 +13,6 @@ nd = "nd" authentification = "authentification" # Hex-like test Change-Id fixture token ("Idead0000…ff"). idead = "idead" +# Latin-1 "café" fixture with the é as a raw byte ("caf\xe9.txt") or +# its replacement char ("caf\u{FFFD}.txt"). +caf = "caf" diff --git a/crates/mergify-ci/src/scopes_detect/changed_files.rs b/crates/mergify-ci/src/scopes_detect/changed_files.rs index 74890690..60ea049e 100644 --- a/crates/mergify-ci/src/scopes_detect/changed_files.rs +++ b/crates/mergify-ci/src/scopes_detect/changed_files.rs @@ -1,13 +1,12 @@ //! `git diff` between two refs, with progressive deepening of a //! shallow clone if no merge base exists yet. //! -//! Mirrors `mergify_cli/ci/scopes/changed_files.py`. The history -//! deepening is necessary in CI: GitHub Actions checkouts default -//! to depth=1, and the merge base between `base` and `head` -//! probably lives further back. We fetch in batches of 100 -//! commits until either a merge base appears or the commit count -//! stops growing (meaning we've reached the root and there's -//! genuinely no common ancestor). +//! The history deepening is necessary in CI: GitHub Actions +//! checkouts default to depth=1, and the merge base between +//! `base` and `head` probably lives further back. We fetch in +//! batches of 100 commits until either a merge base appears or +//! the commit count stops growing (meaning we've reached the +//! root and there's genuinely no common ancestor). use std::path::Path; use std::process::Command; @@ -21,6 +20,9 @@ const FETCHED_REF_PREFIX: &str = "refs/mergify-cli/fetched/"; const COMMITS_BATCH_SIZE: u64 = 100; +/// The remote `ensure_history` deepens from. +const REMOTE: &str = "origin"; + fn is_sha(ref_: &str) -> bool { // Only full 40-char SHAs — abbreviated SHAs would false-match // branch names like "deadbeef" and cause `git fetch` to treat @@ -52,10 +54,25 @@ fn fetch_arg(ref_: &str) -> Option { // `git fetch origin ` only updates `FETCH_HEAD`; // use an explicit refspec so the branch becomes a real // local ref we can name later. - Some(format!("+{ref_}:{}", local_ref(ref_))) + Some(format!("+{}:{}", remote_name(ref_), local_ref(ref_))) } } +/// The name `REMOTE` knows `ref_` by. A remote-tracking name like +/// `origin/main` exists only in a clone, so asking the remote for it +/// fails with "couldn't find remote ref". That is what a depth-1 +/// CI checkout runs into, as it has no `refs/remotes/origin/main` +/// to resolve locally either. Only the `origin/` prefix is +/// stripped: `feature/x` is a branch, not a remote. +fn remote_name(ref_: &str) -> &str { + ref_.strip_prefix("refs/remotes/") + .or_else(|| ref_.strip_prefix("remotes/")) + .unwrap_or(ref_) + .strip_prefix(REMOTE) + .and_then(|rest| rest.strip_prefix('/')) + .unwrap_or(ref_) +} + /// Base `git` command, rooted at `repo_dir` via `-C` when one is /// given. Production always passes `None` (the process cwd); the /// parameter is what lets the tests below drive a fixture @@ -90,6 +107,10 @@ fn run_git(repo_dir: Option<&Path>, args: &[&str]) -> Result { // Untrimmed: `git_changed_files` reads NUL-delimited paths, // and a leading or trailing space is a legal filename // character. Callers that parse a scalar trim it themselves. + // + // Lossy on purpose: see `git_changed_files` for why a path + // that is not UTF-8 must come back with U+FFFD in it rather + // than as raw bytes or an error. Ok(String::from_utf8_lossy(&out.stdout).into_owned()) } @@ -100,6 +121,31 @@ fn has_merge_base(repo_dir: Option<&Path>, base: &str, head: &str) -> bool { .is_ok_and(|o| o.status.success()) } +/// The commits a shallow clone's history stops at, empty for a full +/// clone. Read from the `shallow` file rather than asked of +/// `rev-parse --is-shallow-repository`, which git before 2.15 +/// echoes back instead of answering. +fn shallow_boundary(repo_dir: Option<&Path>) -> Result, CliError> { + let path = run_git(repo_dir, &["rev-parse", "--git-path", "shallow"])?; + let path = Path::new(path.trim()); + let path = repo_dir.map_or_else(|| path.to_path_buf(), |dir| dir.join(path)); + match std::fs::read_to_string(&path) { + Ok(content) => Ok(content.lines().map(str::to_string).collect()), + Err(e) if e.kind() == std::io::ErrorKind::NotFound => Ok(Vec::new()), + Err(e) => Err(CliError::wrap( + format!("could not read {}", path.display()), + e, + )), + } +} + +fn is_ancestor(repo_dir: Option<&Path>, commit: &str, of: &str) -> bool { + git_cmd(repo_dir) + .args(["merge-base", "--is-ancestor", commit, of]) + .output() + .is_ok_and(|o| o.status.success()) +} + fn commits_count(repo_dir: Option<&Path>) -> Result { let out = run_git(repo_dir, &["rev-list", "--count", "--all"])?; let count = out.trim(); @@ -109,7 +155,7 @@ fn commits_count(repo_dir: Option<&Path>) -> Result { } fn fetch(repo_dir: Option<&Path>, depth_flag: &str, fetch_args: &[String]) -> Result<(), CliError> { - let mut args: Vec<&str> = vec!["fetch", "--no-tags", depth_flag, "origin"]; + let mut args: Vec<&str> = vec!["fetch", "--no-tags", depth_flag, REMOTE]; if !fetch_args.is_empty() { args.push("--"); for fa in fetch_args { @@ -146,7 +192,21 @@ pub fn ensure_history( let mut last_count = commits_count(repo_dir)?; while !has_merge_base(repo_dir, &local_base, &local_head) { depth = depth.saturating_mul(2); - fetch(repo_dir, &format!("--deepen={depth}"), &fetch_args)?; + // `--deepen` only moves the boundary of what the fetch asks + // for. A HEAD-relative ref asks for nothing, so a depth-1 + // checkout's HEAD would keep hiding its parents and no merge + // base would ever appear. Asking for every boundary commit + // deepens it too. Only the ones under `base` or `head`: the + // `shallow` file also holds the boundary of anything + // shallow-fetched from another remote, which `origin` may + // not have and would refuse the whole fetch over. Those left + // came from the remote, unlike a local commit on top of the + // checkout, which it would refuse too. + let mut deepen_args = fetch_args.clone(); + deepen_args.extend(shallow_boundary(repo_dir)?.into_iter().filter(|commit| { + is_ancestor(repo_dir, commit, &local_base) || is_ancestor(repo_dir, commit, &local_head) + })); + fetch(repo_dir, &format!("--deepen={depth}"), &deepen_args)?; let count = commits_count(repo_dir)?; if count == last_count { // No new commits this round — we've reached the root @@ -171,10 +231,22 @@ pub fn ensure_history( /// /// A rename yields **both** of its paths, so a scope the file moved /// out of still counts as touched. Paths are not -/// `core.quotePath`-escaped, so the globs see the real name; bytes -/// that aren't valid UTF-8 are still replaced (see `run_git`), and +/// `core.quotePath`-escaped, so the globs see the real name, and /// the result is unescaped, so callers that print a path must /// escape it first. +/// +/// A path that is not valid UTF-8 (a Latin-1 or Shift-JIS name) +/// comes back with each invalid sequence replaced by U+FFFD: +/// `caf\xe9.txt` reads `caf\u{FFFD}.txt`. That is the name the +/// engine matches, not a corruption of it. The engine derives +/// scopes from GitHub's pull request files API, which reports that +/// exact replacement (checked on a sandbox repository holding +/// `caf\xe9.txt` and a Shift-JIS name, MRGFY-8289). An error would +/// fail CI on a pull request the engine scopes without complaint, +/// and no pattern in the UTF-8 YAML config can spell the raw bytes. +/// The name agrees with the engine's; how `?` and `[...]` match +/// its U+FFFD does not yet (globset matches bytes, the engine +/// characters, MRGFY-10066). pub fn git_changed_files( repo_dir: Option<&Path>, base: &str, @@ -190,7 +262,7 @@ pub fn git_changed_files( // two must agree or CLI-uploaded and engine-derived scopes // disagree for the same pull request. // - // `--diff-filter=ACMRTD` matches Python: Added, Modified, + // `--diff-filter=ACMRTD` keeps Added, Modified, // Type-changed, Deleted, plus the Renamed and Copied letters // that `--no-renames` has just made unreachable. They stay so // that removing `--no-renames` degrades to the old @@ -239,24 +311,39 @@ pub fn git_changed_files( #[cfg(test)] mod tests { use super::*; + use std::io::Write as _; + use std::process::Stdio; /// Run `git` in `dir` with the developer's global/system /// config out of the way, so a personal `diff.renames` or /// `core.quotePath` setting can't change what these tests see. /// - /// `GIT_DIR`/`GIT_WORK_TREE` are dropped too: they override - /// `-C`, and this helper is the one that runs `add`/`commit`. - /// Under `git bisect run cargo test` (or a hook, or `git - /// rebase -x`) those are set, and the fixture's `git add -A` - /// would otherwise stage into the developer's real repository. - fn git(dir: &Path, args: &[&str]) { - let ok = Command::new("git") - .arg("-C") + /// `GIT_DIR`, `GIT_WORK_TREE` and the other repository-locating + /// variables are dropped too: they override `-C`, and these + /// helpers run `add`/`commit`. Under `git bisect run cargo + /// test` (or a hook, or `git rebase -x`) they are set, and the + /// fixture's `git add -A` would otherwise stage into the + /// developer's real repository. + fn fixture_git(dir: &Path) -> Command { + let mut cmd = Command::new("git"); + cmd.arg("-C") .arg(dir) .env("GIT_CONFIG_GLOBAL", "/dev/null") - .env("GIT_CONFIG_NOSYSTEM", "1") - .env_remove("GIT_DIR") - .env_remove("GIT_WORK_TREE") + .env("GIT_CONFIG_NOSYSTEM", "1"); + for var in [ + "GIT_DIR", + "GIT_WORK_TREE", + "GIT_INDEX_FILE", + "GIT_OBJECT_DIRECTORY", + "GIT_COMMON_DIR", + ] { + cmd.env_remove(var); + } + cmd + } + + fn git(dir: &Path, args: &[&str]) { + let ok = fixture_git(dir) .args(args) .status() .expect("spawn git") @@ -264,6 +351,42 @@ mod tests { assert!(ok, "git {args:?} failed"); } + /// Stage `content` at a path given as raw bytes, which need + /// not be UTF-8. Goes through the index because macOS refuses + /// such a name on disk. + fn stage_raw_path(dir: &Path, path: &[u8], content: &str) { + let mut hash = fixture_git(dir) + .args(["hash-object", "-w", "--stdin"]) + .stdin(Stdio::piped()) + .stdout(Stdio::piped()) + .spawn() + .expect("spawn git hash-object"); + hash.stdin + .take() + .expect("stdin") + .write_all(content.as_bytes()) + .expect("write blob"); + let out = hash.wait_with_output().expect("wait git hash-object"); + assert!(out.status.success(), "git hash-object failed"); + let blob = String::from_utf8(out.stdout).expect("hex object id"); + + let mut entry = format!("100644 {} 0\t", blob.trim()).into_bytes(); + entry.extend_from_slice(path); + entry.push(b'\n'); + let mut index = fixture_git(dir) + .args(["update-index", "--index-info"]) + .stdin(Stdio::piped()) + .spawn() + .expect("spawn git update-index"); + index + .stdin + .take() + .expect("stdin") + .write_all(&entry) + .expect("write index entry"); + assert!(index.wait().expect("wait git update-index").success()); + } + fn write(dir: &Path, rel: &str, content: &str) { let path = dir.join(rel); std::fs::create_dir_all(path.parent().expect("path has a parent")).expect("mkdir"); @@ -367,6 +490,28 @@ mod tests { assert_eq!(changed(dir), ["critical/quote\"and\\slash.txt"]); } + #[test] + fn non_utf8_path_reads_as_github_reports_it() { + // MRGFY-8289: GitHub's pull request files API, which the + // engine scopes from, reports a Latin-1 `caf\xe9.txt` as + // `caf\u{FFFD}.txt`, and the Shift-JIS `テスト.txt` with one + // U+FFFD per lead byte. An error here would fail CI on a + // pull request the engine scopes fine. + let tmp = fixture(); + let dir = tmp.path(); + stage_raw_path(dir, b"critical/caf\xe9.txt", "latin-1\n"); + stage_raw_path(dir, b"critical/\x83e\x83X\x83g.txt", "shift-jis\n"); + git(dir, &["commit", "-q", "-m", "add non-UTF-8 names"]); + + assert_eq!( + changed(dir), + [ + "critical/caf\u{FFFD}.txt", + "critical/\u{FFFD}e\u{FFFD}X\u{FFFD}g.txt", + ], + ); + } + #[test] fn add_modify_and_delete_are_unaffected() { // Every ordinary status still reaches the caller. This @@ -428,5 +573,125 @@ mod tests { fetch_arg("main"), Some(format!("+main:{FETCHED_REF_PREFIX}main")), ); + // A remote-tracking name is asked of the remote by its + // branch name, and still lands under its own local name. A + // slash alone is not a remote prefix. + for (ref_, remote) in [ + ("origin/main", "main"), + ("origin/feature/x", "feature/x"), + ("remotes/origin/main", "main"), + ("refs/remotes/origin/main", "main"), + ("feature/x", "feature/x"), + ("originals/x", "originals/x"), + ] { + assert_eq!( + fetch_arg(ref_), + Some(format!("+{remote}:{FETCHED_REF_PREFIX}{ref_}")), + "{ref_}", + ); + } + } + + /// An upstream where `feature` touched `docs/readme.md`, `main` + /// moved on after the fork (so the merge base is the tip of + /// neither), and `pull/merge` merges the two, the commit + /// `actions/checkout` checks out for a pull request. + fn upstream() -> tempfile::TempDir { + let tmp = fixture(); + let up = tmp.path(); + write(up, "docs/readme.md", "docs changed\n"); + git(up, &["add", "-A"]); + git(up, &["commit", "-q", "-m", "touch the docs"]); + git(up, &["checkout", "-q", "main"]); + write(up, "ignored/later.txt", "later\n"); + git(up, &["add", "-A"]); + git(up, &["commit", "-q", "-m", "main moves on"]); + git(up, &["checkout", "-q", "-b", "pull/merge"]); + git(up, &["merge", "-q", "--no-edit", "feature"]); + tmp + } + + /// `branch` of `up`, fetched at `depth` and checked out detached + /// the way `actions/checkout` does it: no `refs/remotes/origin/*`. + fn shallow_clone(up: &Path, branch: &str, depth: u32) -> tempfile::TempDir { + let tmp = tempfile::tempdir().expect("tempdir"); + let dir = tmp.path(); + // `file://`, because git ignores `--depth` for a plain + // local path. + let url = format!("file://{}", up.display()); + for args in [ + &["init", "-q"][..], + &["config", "user.email", "t@e.com"], + &["config", "user.name", "T"], + &["remote", "add", REMOTE, &url], + &[ + "fetch", + "-q", + "--no-tags", + &format!("--depth={depth}"), + REMOTE, + branch, + ], + &["checkout", "-q", "--detach", "FETCH_HEAD"], + ] { + git(dir, args); + } + tmp + } + + #[test] + fn remote_tracking_base_on_a_shallow_clone() { + // MRGFY-8290: on a depth-1 checkout, `--base origin/main` + // failed twice over: the remote has no ref by that name + // ("couldn't find remote ref origin/main"), and once it was + // asked for `main` instead, deepening `main` never lifted + // the shallow boundary sitting on HEAD, so the merge base + // stayed hidden behind it. + let up = upstream(); + let clone = shallow_clone(up.path(), "pull/merge", 1); + + let paths = git_changed_files(Some(clone.path()), "origin/main", "HEAD").expect("git diff"); + assert_eq!(paths, ["docs/readme.md"]); + } + + #[test] + fn local_commit_on_a_shallow_clone() { + // A commit made in CI on top of the checkout is unknown to + // the remote, which refuses the whole fetch if it is asked + // for. Deepening goes through the boundary commit below it. + let up = upstream(); + let clone = shallow_clone(up.path(), "feature", 1); + let dir = clone.path(); + write(dir, "ignored/generated.txt", "generated\n"); + git(dir, &["add", "-A"]); + git(dir, &["commit", "-q", "-m", "local only"]); + + let mut paths = git_changed_files(Some(dir), "origin/main", "HEAD").expect("git diff"); + paths.sort(); + assert_eq!(paths, ["docs/readme.md", "ignored/generated.txt"]); + } + + #[test] + fn unrelated_shallow_remote_on_a_shallow_clone() { + // The `shallow` file is per repository, not per remote. A + // boundary that came from another remote is unknown to + // `origin`, which would refuse the whole deepening fetch if + // it were asked for it. + let up = upstream(); + let clone = shallow_clone(up.path(), "pull/merge", 1); + let dir = clone.path(); + let other = fixture(); + write(other.path(), "ignored/other.txt", "other\n"); + git(other.path(), &["add", "-A"]); + git(other.path(), &["commit", "-q", "-m", "only on other"]); + let url = format!("file://{}", other.path().display()); + git(dir, &["remote", "add", "other", &url]); + git( + dir, + &["fetch", "-q", "--no-tags", "--depth=1", "other", "feature"], + ); + + let paths = git_changed_files(Some(dir), "origin/main", "HEAD").expect("git diff"); + assert_eq!(paths, ["docs/readme.md"]); } } diff --git a/crates/mergify-ci/src/scopes_detect/matching.rs b/crates/mergify-ci/src/scopes_detect/matching.rs index e7f210e7..92113607 100644 --- a/crates/mergify-ci/src/scopes_detect/matching.rs +++ b/crates/mergify-ci/src/scopes_detect/matching.rs @@ -18,7 +18,10 @@ //! compiles it inline, the engine expands it into one pattern per //! branch (MRGFY-8359). //! -//! Parity is exact for every path git can report as changed. The +//! Parity is exact for every ASCII path git can report as changed. +//! On a non-ASCII one, `?` and `[...]` differ: globset matches +//! bytes and the engine characters, so `caf?.txt` misses +//! `café.txt` here and hits it there (MRGFY-10066). The other //! residual differences all need a path git never emits: a leading //! `/`, a trailing `/`, an empty segment (`a//b`), or the empty //! string. An unterminated `[` is the one place globset is stricter