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/git_refs.rs b/crates/mergify-ci/src/git_refs.rs index 091e178c..82388468 100644 --- a/crates/mergify-ci/src/git_refs.rs +++ b/crates/mergify-ci/src/git_refs.rs @@ -83,8 +83,8 @@ pub struct References { pub source: ReferencesSource, /// The scopes the merge queue decided for this batch, read off /// the engine's git note: see [`batch_scopes`]. `None` everywhere - /// else, the pull request body included, which anyone opening a - /// pull request titled `merge queue: ` writes. + /// else, the pull request body included, which whoever opens the + /// pull request writes. pub batch_scopes: Option, } @@ -240,7 +240,10 @@ fn detect_from_pull_request_event( .and_then(|pr| pr.head.as_ref()) .map_or_else(|| "HEAD".to_string(), |r| r.sha.clone()); + // A fork can serve the note too, when the workflow checks it out + // as `origin`, so the note needs the same admission as the body. if let Some(pr) = &event.pull_request + && pr.is_from_base_repository() && let Some(head_ref) = &pr.head && let Some(branch) = head_ref.r#ref.as_deref() && let Some(note) = notes_reader(branch, &head_ref.sha) @@ -255,8 +258,9 @@ fn detect_from_pull_request_event( } if let Some(meta) = extract_from_event(event, output)? { - // Anyone who can open a pull request writes this body, so the - // value only becomes a base once it looks like one. Either way + // Anyone who can open a pull request from a branch of the + // repository writes this body, so the value only becomes a base + // once it looks like one. Either way // a payload we can't use is not metadata: fall through to the // PR base below, which is the *wrong* base for a genuine batch // build, so warn rather than let it be diagnosed silently. @@ -361,8 +365,9 @@ pub fn real_notes_reader(branch: &str, head_sha: &str) -> Option` in the body and git-refs // emitted it as `base`, where the workflow's next git call // read it as an option. A value that is not an object name is - // not metadata: fall through to the PR base and say so. + // not metadata: fall through to the PR base and say so. Forks + // are refused since MRGFY-8854, but a branch of the repository + // still writes the body. let dir = tempfile::tempdir().unwrap(); let path = write_event( &dir, @@ -806,8 +851,8 @@ mod tests { "pull_request": { "title": "merge queue: batch", "body": "```yaml\nchecking_base_sha: --output=/home/runner/.gitconfig\n```", - "head": {"sha": "mq-head", "ref": "mq/main/0"}, - "base": {"sha": "pr-base"}, + "head": {"sha": "mq-head", "ref": "mq/main/0", "repo": {"id": 1}}, + "base": {"sha": "pr-base", "repo": {"id": 1}}, }, }), ); @@ -842,8 +887,8 @@ mod tests { "pull_request": { "title": "merge queue: batch", "body": "```yaml\nchecking_base_sha: \"cafef00d\\n::error::pwned\"\n```", - "head": {"sha": "mq-head", "ref": "mq/main/0"}, - "base": {"sha": "pr-base"}, + "head": {"sha": "mq-head", "ref": "mq/main/0", "repo": {"id": 1}}, + "base": {"sha": "pr-base", "repo": {"id": 1}}, }, }), ); @@ -877,7 +922,8 @@ mod tests { "pull_request": { "title": "merge queue: batch", "body": format!("```yaml\nchecking_base_sha: {MQ_BASE}\n```"), - "head": {"sha": "mq-head", "ref": "mq/main/0"}, + "head": {"sha": "mq-head", "ref": "mq/main/0", "repo": {"id": 1}}, + "base": {"sha": "pr-base", "repo": {"id": 1}}, }, }), ); @@ -943,8 +989,8 @@ mod tests { #[test] fn mq_note_carries_batch_scopes_but_body_does_not() { - // The body is written by whoever opens a pull request titled - // `merge queue: `, so only the engine's note decides scopes. + // The body is written by whoever opens the pull request, so + // only the engine's note decides scopes. let dir = tempfile::tempdir().unwrap(); let path = write_event( &dir, @@ -952,7 +998,8 @@ mod tests { "pull_request": { "title": "merge queue: batch", "body": format!("```yaml\nchecking_base_sha: {MQ_BASE}\nscopes: [body]\n```"), - "head": {"sha": "mq-head", "ref": "mq/main/0"}, + "head": {"sha": "mq-head", "ref": "mq/main/0", "repo": {"id": 1}}, + "base": {"sha": "pr-base", "repo": {"id": 1}}, }, }), ); diff --git a/crates/mergify-ci/src/github_event.rs b/crates/mergify-ci/src/github_event.rs index 4495e9fc..bca24fb7 100644 --- a/crates/mergify-ci/src/github_event.rs +++ b/crates/mergify-ci/src/github_event.rs @@ -15,6 +15,17 @@ pub struct GitRef { pub sha: String, #[serde(default)] pub r#ref: Option, + /// `null` on a pull request's `head` once its fork is deleted. + #[serde(default)] + pub repo: Option, +} + +#[derive(Debug, Clone, Default, Deserialize)] +pub struct RefRepository { + /// Optional so that a `repo` without it costs only the trust in + /// [`PullRequest::is_from_base_repository`], not the whole event. + #[serde(default)] + pub id: Option, } #[derive(Debug, Clone, Default, Deserialize)] @@ -31,6 +42,31 @@ pub struct PullRequest { pub head: Option, } +impl PullRequest { + /// Whether the head branch lives in the base repository, which is + /// where the engine always opens its merge queue drafts. + /// + /// This is what lets the CLI trust merge queue metadata, never the + /// `merge queue: ` title, which the author picks (MRGFY-8854). It + /// rules out a fork, whose author writes the code, the body, and, + /// when the workflow checks the fork out as `origin`, the git note + /// too. It does not rule out a reader opening a pull request from a + /// branch someone else pushed: they write its title and body, but + /// not its code. The branch prefix and the author are no better: + /// `queue_branch_prefix` and `draft_bot_account` are configurable. + /// + /// `false` when either side has no repository id, including the + /// `null` head of a deleted fork. + #[must_use] + pub fn is_from_base_repository(&self) -> bool { + let repo_id = |r: &Option| r.as_ref()?.repo.as_ref()?.id; + matches!( + (repo_id(&self.head), repo_id(&self.base)), + (Some(head), Some(base)) if head == base + ) + } +} + #[derive(Debug, Clone, Default, Deserialize)] pub struct Repository { #[serde(default)] @@ -94,6 +130,21 @@ mod tests { assert_eq!(ev.pull_request.unwrap().number, Some(7)); } + #[test] + fn is_from_base_repository_needs_both_ids_and_equal() { + let pr = |raw: &str| serde_json::from_str::(raw).unwrap(); + assert!(pr(r#"{"head": {"sha": "h", "repo": {"id": 1}}, "base": {"sha": "b", "repo": {"id": 1}}}"#).is_from_base_repository()); + for raw in [ + r#"{"head": {"sha": "h", "repo": {"id": 2}}, "base": {"sha": "b", "repo": {"id": 1}}}"#, + r#"{"head": {"sha": "h", "repo": null}, "base": {"sha": "b", "repo": {"id": 1}}}"#, + r#"{"head": {"sha": "h", "repo": {"id": 1}}, "base": {"sha": "b"}}"#, + r#"{"head": {"sha": "h"}, "base": {"sha": "b"}}"#, + r#"{"head": {"sha": "h", "repo": {}}, "base": {"sha": "b", "repo": {}}}"#, + ] { + assert!(!pr(raw).is_from_base_repository(), "admitted {raw}"); + } + } + #[test] fn deserialize_push_event_shape() { let raw = r#"{"before": "a", "after": "b", "repository": {"default_branch": "main"}}"#; diff --git a/crates/mergify-ci/src/queue_metadata.rs b/crates/mergify-ci/src/queue_metadata.rs index dfa9c0b4..1816bc7d 100644 --- a/crates/mergify-ci/src/queue_metadata.rs +++ b/crates/mergify-ci/src/queue_metadata.rs @@ -21,6 +21,8 @@ use mergify_core::Output; use crate::github_event::GitHubEvent; +#[cfg(doc)] +use crate::github_event::PullRequest; /// Parse the first ```yaml``` fenced block out of `body` into a /// generic value, keeping every field. Returns `None` when the body @@ -51,9 +53,10 @@ pub fn parse_yaml_block(body: &str) -> Option { /// Extract MQ metadata from an event payload's pull-request body. /// -/// Emits a warning on `output` (stderr for human mode) when the PR is -/// an MQ draft but the body is missing or lacks the fenced block — -/// matches Python's stderr warnings. +/// The title only nominates the pull request: the body is read when +/// [`PullRequest::is_from_base_repository`] also holds. Emits a warning +/// on `output` (stderr for human mode) when it does not, or when the +/// body is missing or lacks the fenced block. pub fn extract_from_event( ev: &GitHubEvent, output: &mut dyn Output, @@ -67,6 +70,12 @@ pub fn extract_from_event( if !title.starts_with("merge queue: ") { return Ok(None); } + if !pr.is_from_base_repository() { + output.status( + "WARNING: pull request titled like a merge queue draft does not come from the base repository, skipping metadata extraction", + )?; + return Ok(None); + } let Some(body) = pr.body.as_deref() else { output.status("WARNING: MQ pull request without body, skipping metadata extraction")?; return Ok(None); @@ -154,16 +163,31 @@ mod tests { assert!(cap.stderr().is_empty()); } - #[test] - fn extract_warns_on_mq_pr_without_body() { - let ev = GitHubEvent { + /// A `merge queue: ` pull request whose head and base live in the + /// repositories with these ids (`None`: the payload's `repo` is + /// `null`, as on the head of a deleted fork). + fn mq_event(head_repo: Option, base_repo: Option, body: Option<&str>) -> GitHubEvent { + let git_ref = |id: Option| crate::github_event::GitRef { + repo: id.map(|id| crate::github_event::RefRepository { id: Some(id) }), + ..Default::default() + }; + GitHubEvent { pull_request: Some(crate::github_event::PullRequest { - title: Some("merge queue: deploy".into()), - body: None, + title: Some("merge queue: batch".into()), + body: body.map(Into::into), + head: Some(git_ref(head_repo)), + base: Some(git_ref(base_repo)), ..Default::default() }), ..Default::default() - }; + } + } + + const BODY: &str = "blah\n```yaml\nchecking_base_sha: deadbeef\n```"; + + #[test] + fn extract_warns_on_mq_pr_without_body() { + let ev = mq_event(Some(1), Some(1), None); let mut cap = Captured::human(); let result = extract_from_event(&ev, &mut cap.output).unwrap(); assert!(result.is_none()); @@ -173,17 +197,32 @@ mod tests { #[test] fn extract_returns_metadata_for_mq_pr() { - let body = "blah\n```yaml\nchecking_base_sha: deadbeef\n```"; - let ev = GitHubEvent { - pull_request: Some(crate::github_event::PullRequest { - title: Some("merge queue: batch".into()), - body: Some(body.into()), - ..Default::default() - }), - ..Default::default() - }; + let ev = mq_event(Some(1), Some(1), Some(BODY)); let mut cap = Captured::human(); let meta = extract_from_event(&ev, &mut cap.output).unwrap().unwrap(); assert_eq!(meta["checking_base_sha"], "deadbeef"); } + + #[test] + fn extract_ignores_mq_title_outside_the_base_repository() { + // MRGFY-8854: anyone can title a pull request `merge queue: `. + // From a fork, from a deleted fork, or with no repository to + // compare, the body is not the engine's. + for (head, base) in [ + (Some(2), Some(1)), + (None, Some(1)), + (Some(1), None), + (None, None), + ] { + let ev = mq_event(head, base, Some(BODY)); + let mut cap = Captured::human(); + let result = extract_from_event(&ev, &mut cap.output).unwrap(); + assert!(result.is_none(), "{head:?} -> {base:?} admitted"); + let stderr = cap.stderr(); + assert!( + stderr.contains("does not come from the base repository"), + "got: {stderr:?}" + ); + } + } } 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