diff --git a/crates/mergify-ci/src/scopes_detect/mod.rs b/crates/mergify-ci/src/scopes_detect/mod.rs index b85e34f3..10a41086 100644 --- a/crates/mergify-ci/src/scopes_detect/mod.rs +++ b/crates/mergify-ci/src/scopes_detect/mod.rs @@ -40,9 +40,9 @@ use crate::git_refs::ReferencesSource; pub struct ScopesOptions<'a> { /// Explicit `--config `. `None` triggers the - /// fallback chain (env var `MERGIFY_CONFIG_PATH`, then auto- - /// detection of `.mergify.yml` / `.mergify/config.yml` / - /// `.github/mergify.yml`). + /// fallback chain (env var `MERGIFY_CONFIG_PATH`, then the + /// auto-detection over + /// [`mergify_config::paths::DEFAULT_CONFIG_PATHS`]). pub config: Option<&'a Path>, /// Optional `--base`. Combined with `--head` to take the /// "manual" References branch. @@ -78,7 +78,7 @@ pub fn run(opts: ScopesOptions<'_>, output: &mut dyn Output) -> Result<(), CliEr head, write, } = opts; - let config_path = resolve_config_path(config)?; + let config_path = resolve_config_path(config, output)?; let cfg = config::load(&config_path)?; let refs = resolve_refs(base, head, output)?; @@ -111,13 +111,16 @@ pub fn run(opts: ScopesOptions<'_>, output: &mut dyn Output) -> Result<(), CliEr Ok(()) } -/// Auto-detection mirrors Python's -/// `detector.get_mergify_config_path` (which is the same triple -/// `.mergify.yml`, `.mergify/config.yml`, `.github/mergify.yml` -/// that `mergify config validate` uses), with `MERGIFY_CONFIG_PATH` -/// honored ahead of it. Empty env var falls back to auto-detect -/// — matches Python. -fn resolve_config_path(explicit: Option<&Path>) -> Result { +/// Auto-detection defers to [`mergify_config::paths`], so +/// `ci scopes` searches exactly what `mergify config validate` +/// searches — including the duplicate-configuration warning. +/// `MERGIFY_CONFIG_PATH` is honored ahead of it; an empty value +/// falls back to auto-detect, which is what the `gha-mergify-ci` +/// action relies on. +fn resolve_config_path( + explicit: Option<&Path>, + output: &mut dyn Output, +) -> Result { if let Some(path) = explicit { if path.is_file() { return Ok(path.to_path_buf()); @@ -136,7 +139,7 @@ fn resolve_config_path(explicit: Option<&Path>) -> Result { } return Ok(p); } - mergify_config::paths::resolve_config_path(None) + mergify_config::paths::resolve_config_path(None, output) } /// `(base, head)` resolution mirrors Python's branch in @@ -310,7 +313,9 @@ mod tests { #[test] fn resolve_config_path_errors_on_missing_explicit() { - let err = resolve_config_path(Some(Path::new("/no/such/file.yml"))).unwrap_err(); + let mut captured = Captured::human(); + let err = resolve_config_path(Some(Path::new("/no/such/file.yml")), &mut captured.output) + .unwrap_err(); assert!(matches!(err, CliError::Configuration(_))); assert!(err.to_string().contains("does not exist")); } @@ -328,8 +333,9 @@ mod tests { // so this function owns the lookup — and the empty branch // here must fall through to autodetect rather than report // a malformed env var. + let mut captured = Captured::human(); let result = env::testing::with_var("MERGIFY_CONFIG_PATH", Some(""), || { - resolve_config_path(None) + resolve_config_path(None, &mut captured.output) }); // Either autodetect found a real config (cargo test runs // from a workspace that contains `.mergify.yml`, so this is @@ -352,9 +358,10 @@ mod tests { // value that doesn't exist, the error must name the env // var + the bogus path so the user can spot the typo // without having to dig. + let mut captured = Captured::human(); let err = env::testing::with_var("MERGIFY_CONFIG_PATH", Some("/no/such/.mergify.yml"), || { - resolve_config_path(None).unwrap_err() + resolve_config_path(None, &mut captured.output).unwrap_err() }); let msg = err.to_string(); assert!(msg.contains("MERGIFY_CONFIG_PATH="), "got: {msg}"); diff --git a/crates/mergify-cli/src/main.rs b/crates/mergify-cli/src/main.rs index 3fbe63f0..a499dabe 100644 --- a/crates/mergify-cli/src/main.rs +++ b/crates/mergify-cli/src/main.rs @@ -3831,11 +3831,28 @@ struct InternalStackRemoteChangesArgs { author: String, } +/// `--config-file`'s long help. Same reason as +/// [`SCOPES_CONFIG_HELP`]: the list of searched paths belongs to the +/// resolver, not to a doc comment that can fall behind it. +static CONFIG_FILE_HELP: std::sync::LazyLock = std::sync::LazyLock::new(|| { + format!( + "Path to the Mergify configuration file.\n\nWhen omitted, the first of these that \ + exists is used: {}. A repository carrying more than one gets a warning on stderr \ + naming the file in use and the ones ignored.", + mergify_config::paths::DEFAULT_CONFIG_PATHS.join(", "), + ) +}); + #[derive(clap::Args)] struct ConfigArgs { /// Path to the Mergify configuration file (auto-detected if not /// provided). - #[arg(long = "config-file", short = 'f', global = true)] + #[arg( + long = "config-file", + short = 'f', + global = true, + long_help = CONFIG_FILE_HELP.as_str(), + )] config_file: Option, #[command(subcommand)] @@ -3967,12 +3984,20 @@ struct GitRefsCliArgs { #[derive(clap::Args)] struct QueueInfoCliArgs {} +/// `--config`'s long help, built from the resolver's own search +/// list so the two cannot drift apart. clap wants a `&'static str`; +/// a `LazyLock` is what turns a runtime `join` into one. +static SCOPES_CONFIG_HELP: std::sync::LazyLock = std::sync::LazyLock::new(|| { + format!( + "Path to YAML config file.\n\nFalls back to the MERGIFY_CONFIG_PATH environment \ + variable, then auto-detects the first of these that exists: {}.", + mergify_config::paths::DEFAULT_CONFIG_PATHS.join(", "), + ) +}); + #[derive(clap::Args)] struct ScopesCliArgs { - /// Path to YAML config file. Falls back to the - /// `MERGIFY_CONFIG_PATH` env var, then auto-detects - /// `.mergify.yml`, `.mergify/config.yml`, or - /// `.github/mergify.yml`. + /// Path to YAML config file (auto-detected when omitted). // // The env var lookup is intentionally *not* delegated to // clap's `env = ...` attribute: callers (notably the @@ -3989,7 +4014,7 @@ struct ScopesCliArgs { // attribute, which covers this one) and // `resolve_config_path_treats_empty_env_var_as_unset` // (lower-level resolver). - #[arg(long)] + #[arg(long, long_help = SCOPES_CONFIG_HELP.as_str())] config: Option, /// Base git reference to use to look for changed files. diff --git a/crates/mergify-cli/src/snapshots/mergify__tests__cli_schema_golden.snap b/crates/mergify-cli/src/snapshots/mergify__tests__cli_schema_golden.snap index 6e192833..2365e511 100644 --- a/crates/mergify-cli/src/snapshots/mergify__tests__cli_schema_golden.snap +++ b/crates/mergify-cli/src/snapshots/mergify__tests__cli_schema_golden.snap @@ -189,7 +189,7 @@ expression: schema "id": "config_file", "kind": "option", "long": "config-file", - "longHelp": "Path to the Mergify configuration file (auto-detected if not provided)", + "longHelp": "Path to the Mergify configuration file.\n\nWhen omitted, the first of these that exists is used: .mergify.yml, .mergify.yaml, .mergify/config.yml, .mergify/config.yaml, .github/mergify.yml, .github/mergify.yaml. A repository carrying more than one gets a warning on stderr naming the file in use and the ones ignored.", "numArgs": "1", "possibleValues": [], "required": false, @@ -540,11 +540,11 @@ expression: schema "default": null, "env": null, "global": false, - "help": "Path to YAML config file. Falls back to the `MERGIFY_CONFIG_PATH` env var, then auto-detects `.mergify.yml`, `.mergify/config.yml`, or `.github/mergify.yml`", + "help": "Path to YAML config file (auto-detected when omitted)", "id": "config", "kind": "option", "long": "config", - "longHelp": "Path to YAML config file. Falls back to the `MERGIFY_CONFIG_PATH` env var, then auto-detects `.mergify.yml`, `.mergify/config.yml`, or `.github/mergify.yml`", + "longHelp": "Path to YAML config file.\n\nFalls back to the MERGIFY_CONFIG_PATH environment variable, then auto-detects the first of these that exists: .mergify.yml, .mergify.yaml, .mergify/config.yml, .mergify/config.yaml, .github/mergify.yml, .github/mergify.yaml.", "numArgs": "1", "possibleValues": [], "required": false, diff --git a/crates/mergify-config/src/paths.rs b/crates/mergify-config/src/paths.rs index 42b65625..1728dbbf 100644 --- a/crates/mergify-config/src/paths.rs +++ b/crates/mergify-config/src/paths.rs @@ -3,18 +3,65 @@ //! Both `config validate` and `config simulate` accept a //! ``--config-file`` flag and otherwise auto-detect the file from a //! small list of conventional locations. The resolver here is the -//! single source of truth for that behavior. +//! single source of truth for that behavior — `mergify ci scopes` +//! falls back to it too, so there is one search order in the CLI. use std::path::Path; use std::path::PathBuf; use mergify_core::CliError; +use mergify_core::Output; /// Filename patterns the CLI searches for a Mergify configuration, -/// in priority order. Mirrors ``MERGIFY_CONFIG_PATHS`` in -/// ``mergify_cli/ci/detector.py``. -pub const DEFAULT_CONFIG_PATHS: [&str; 3] = - [".mergify.yml", ".mergify/config.yml", ".github/mergify.yml"]; +/// in priority order: the three conventional locations, and at each +/// of them the `.yml` spelling ahead of the `.yaml` one. The first +/// file that exists wins. +/// +/// The engine must search the same list in the same order +/// (MRGFY-9532). A repository carrying several of these files is +/// otherwise validated against one file and merged against another, +/// which is the one failure `config validate` exists to prevent. +pub const DEFAULT_CONFIG_PATHS: [&str; 6] = [ + ".mergify.yml", + ".mergify.yaml", + ".mergify/config.yml", + ".mergify/config.yaml", + ".github/mergify.yml", + ".github/mergify.yaml", +]; + +/// The candidate that won the search, plus the ones it beat. +struct Resolution { + path: PathBuf, + /// Candidates that exist on disk but lose to `path` under the + /// [`DEFAULT_CONFIG_PATHS`] order. Always empty for an explicit + /// `--config-file`: the user named the file, nothing is ignored. + shadowed: Vec, +} + +impl Resolution { + /// The warning to hand the user when the repository carries more + /// than one configuration file, or `None` when it carries one. + /// + /// Naming the losers matters more than naming the winner: an + /// edit to an ignored file looks like Mergify dropping a change. + fn shadow_warning(&self) -> Option { + if self.shadowed.is_empty() { + return None; + } + let ignored = self + .shadowed + .iter() + .map(|p| format!("'{}'", p.display())) + .collect::>() + .join(", "); + Some(format!( + "mergify: warning: several Mergify configuration files found; \ + using '{}' and ignoring {ignored}.", + self.path.display(), + )) + } +} /// Resolve the path of the Mergify configuration file relative to /// the current working directory. @@ -23,9 +70,18 @@ pub const DEFAULT_CONFIG_PATHS: [&str; 3] = /// otherwise the user specified a bad path and we fail loudly with /// [`CliError::Configuration`]. When ``explicit`` is ``None`` the /// resolver walks [`DEFAULT_CONFIG_PATHS`] in order and returns the -/// first match. -pub fn resolve_config_path(explicit: Option<&Path>) -> Result { - resolve_config_path_in(explicit, Path::new(".")) +/// first match, warning through ``output`` about any other candidate +/// it found on the way. +/// +/// # Errors +/// +/// Returns [`CliError::Configuration`] when neither an explicit +/// path nor any default candidate exists. +pub fn resolve_config_path( + explicit: Option<&Path>, + output: &mut dyn Output, +) -> Result { + resolve_config_path_in(explicit, Path::new("."), output) } /// Same as [`resolve_config_path`] but searches relative to @@ -39,54 +95,185 @@ pub fn resolve_config_path(explicit: Option<&Path>) -> Result /// /// Returns [`CliError::Configuration`] when neither an explicit /// path nor any default candidate exists. -pub fn resolve_config_path_in(explicit: Option<&Path>, base: &Path) -> Result { +pub fn resolve_config_path_in( + explicit: Option<&Path>, + base: &Path, + output: &mut dyn Output, +) -> Result { + let resolution = resolve_in(explicit, base)?; + // `Output::status` is the house channel for human chatter, and + // it writes to stderr so `--json` stdout stays a single + // document. It is a no-op in `OutputMode::Json`, which none of + // the config-reading commands run in today. + if let Some(warning) = resolution.shadow_warning() { + output.status(&warning)?; + } + Ok(resolution.path) +} + +fn resolve_in(explicit: Option<&Path>, base: &Path) -> Result { if let Some(path) = explicit { if path.is_file() { - return Ok(path.to_path_buf()); + return Ok(Resolution { + path: path.to_path_buf(), + shadowed: Vec::new(), + }); } return Err(CliError::Configuration(format!( "Configuration file not found: {}", path.display(), ))); } - for candidate in DEFAULT_CONFIG_PATHS { - let path = base.join(candidate); - if path.is_file() { - return Ok(path); - } - } - Err(CliError::Configuration(format!( - "Mergify configuration file not found. Looked in: {}", - DEFAULT_CONFIG_PATHS.join(", "), - ))) + let mut found = DEFAULT_CONFIG_PATHS + .iter() + .map(|candidate| base.join(candidate)) + .filter(|path| path.is_file()); + let Some(path) = found.next() else { + return Err(CliError::Configuration(format!( + "Mergify configuration file not found. Looked in: {}", + DEFAULT_CONFIG_PATHS.join(", "), + ))); + }; + Ok(Resolution { + path, + shadowed: found.collect(), + }) } #[cfg(test)] mod tests { use std::fs; + use mergify_test_support::Captured; + use super::*; + /// Resolve under `base` with a capturing output, returning the + /// winner and whatever was written to stderr. + fn resolve(base: &Path) -> (Result, String) { + let mut captured = Captured::human(); + let got = resolve_config_path_in(None, base, &mut captured.output); + (got, captured.stderr()) + } + #[test] fn finds_dotmergify_yml() { let tmp = tempfile::tempdir().unwrap(); fs::write(tmp.path().join(".mergify.yml"), "").unwrap(); - let got = resolve_config_path_in(None, tmp.path()).unwrap(); - assert_eq!(got, tmp.path().join(".mergify.yml")); + let (got, stderr) = resolve(tmp.path()); + assert_eq!(got.unwrap(), tmp.path().join(".mergify.yml")); + assert_eq!(stderr, ""); + } + + #[test] + fn finds_dotmergify_yaml() { + let tmp = tempfile::tempdir().unwrap(); + fs::write(tmp.path().join(".mergify.yaml"), "").unwrap(); + let (got, stderr) = resolve(tmp.path()); + assert_eq!(got.unwrap(), tmp.path().join(".mergify.yaml")); + assert_eq!(stderr, ""); + } + + #[test] + fn finds_yaml_in_every_conventional_location() { + for candidate in [ + ".mergify.yaml", + ".mergify/config.yaml", + ".github/mergify.yaml", + ] { + let tmp = tempfile::tempdir().unwrap(); + let path = tmp.path().join(candidate); + fs::create_dir_all(path.parent().unwrap()).unwrap(); + fs::write(&path, "").unwrap(); + let (got, _) = resolve(tmp.path()); + assert_eq!(got.unwrap(), path, "looking for {candidate}"); + } + } + + #[test] + fn yml_wins_over_yaml_at_the_same_location() { + let tmp = tempfile::tempdir().unwrap(); + fs::write(tmp.path().join(".mergify.yml"), "").unwrap(); + fs::write(tmp.path().join(".mergify.yaml"), "").unwrap(); + let (got, stderr) = resolve(tmp.path()); + assert_eq!(got.unwrap(), tmp.path().join(".mergify.yml")); + assert_eq!( + stderr, + format!( + "mergify: warning: several Mergify configuration files found; \ + using '{}' and ignoring '{}'.\n", + tmp.path().join(".mergify.yml").display(), + tmp.path().join(".mergify.yaml").display(), + ), + ); + } + + /// The location order outranks the extension order: a `.yaml` + /// higher up beats a `.yml` lower down. + #[test] + fn location_order_outranks_extension_order() { + let tmp = tempfile::tempdir().unwrap(); + fs::write(tmp.path().join(".mergify.yaml"), "").unwrap(); + fs::create_dir_all(tmp.path().join(".github")).unwrap(); + fs::write(tmp.path().join(".github/mergify.yml"), "").unwrap(); + let (got, _) = resolve(tmp.path()); + assert_eq!(got.unwrap(), tmp.path().join(".mergify.yaml")); + } + + /// Every loser is named, in search order, on one line. + #[test] + fn warning_names_every_ignored_candidate() { + let tmp = tempfile::tempdir().unwrap(); + fs::write(tmp.path().join(".mergify.yml"), "").unwrap(); + fs::write(tmp.path().join(".mergify.yaml"), "").unwrap(); + fs::create_dir_all(tmp.path().join(".github")).unwrap(); + fs::write(tmp.path().join(".github/mergify.yaml"), "").unwrap(); + let (_, stderr) = resolve(tmp.path()); + assert_eq!( + stderr, + format!( + "mergify: warning: several Mergify configuration files found; \ + using '{}' and ignoring '{}', '{}'.\n", + tmp.path().join(".mergify.yml").display(), + tmp.path().join(".mergify.yaml").display(), + tmp.path().join(".github/mergify.yaml").display(), + ), + ); + } + + #[test] + fn explicit_path_is_never_reported_as_shadowing() { + let tmp = tempfile::tempdir().unwrap(); + fs::write(tmp.path().join(".mergify.yml"), "").unwrap(); + let explicit = tmp.path().join(".mergify.yaml"); + fs::write(&explicit, "").unwrap(); + let mut captured = Captured::human(); + let got = resolve_config_path_in(Some(&explicit), tmp.path(), &mut captured.output); + assert_eq!(got.unwrap(), explicit); + assert_eq!(captured.stderr(), ""); } #[test] fn errors_when_no_file_and_no_explicit() { let tmp = tempfile::tempdir().unwrap(); - let err = resolve_config_path_in(None, tmp.path()).unwrap_err(); + let (got, _) = resolve(tmp.path()); + let err = got.unwrap_err(); assert!(matches!(err, CliError::Configuration(_))); assert!(err.to_string().contains("not found")); + // The failure lists what was searched, so a user who spelled + // the file `.yaml` can see that spelling is accepted. + assert!(err.to_string().contains(".mergify.yaml")); } #[test] fn errors_on_explicit_missing_file() { - let err = resolve_config_path_in(Some(Path::new("/nonexistent/path.yml")), Path::new(".")) - .unwrap_err(); + let mut captured = Captured::human(); + let err = resolve_config_path_in( + Some(Path::new("/nonexistent/path.yml")), + Path::new("."), + &mut captured.output, + ) + .unwrap_err(); assert!(matches!(err, CliError::Configuration(_))); } } diff --git a/crates/mergify-config/src/simulate.rs b/crates/mergify-config/src/simulate.rs index aca627a9..1f2b729c 100644 --- a/crates/mergify-config/src/simulate.rs +++ b/crates/mergify-config/src/simulate.rs @@ -50,7 +50,7 @@ pub struct SimulateOptions<'a> { /// Run the `config simulate` command. pub async fn run(opts: SimulateOptions<'_>, output: &mut dyn Output) -> Result<(), CliError> { - let config_path = resolve_config_path(opts.config_file)?; + let config_path = resolve_config_path(opts.config_file, output)?; let mergify_yml = std::fs::read_to_string(&config_path).map_err(|e| { CliError::Configuration(format!("cannot read {}: {e}", config_path.display())) })?; diff --git a/crates/mergify-config/src/validate.rs b/crates/mergify-config/src/validate.rs index 1bdee53c..ba540d71 100644 --- a/crates/mergify-config/src/validate.rs +++ b/crates/mergify-config/src/validate.rs @@ -3,8 +3,7 @@ //! //! The command: //! 1. Resolves the config file (explicit `--config-file` or the -//! first of `.mergify.yml`, `.mergify/config.yml`, -//! `.github/mergify.yml`). +//! first candidate in [`crate::paths::DEFAULT_CONFIG_PATHS`]). //! 2. Parses it as YAML. //! 3. Fetches `https://docs.mergify.com/mergify-configuration-schema.json`. //! 4. Validates the config against the schema using the @@ -36,7 +35,7 @@ const SCHEMA_PATH: &str = "/mergify-configuration-schema.json"; /// user provided one; otherwise the command searches the default /// locations. pub async fn run(explicit_path: Option<&Path>, output: &mut dyn Output) -> Result<(), CliError> { - let config_path = resolve_config_path(explicit_path)?; + let config_path = resolve_config_path(explicit_path, output)?; let config_value = load_yaml(&config_path)?; output.status(&format!("Fetching schema from {SCHEMA_HOST}…"))?; diff --git a/skills/mergify-config/SKILL.md b/skills/mergify-config/SKILL.md index 41b1b5cf..1a30f595 100644 --- a/skills/mergify-config/SKILL.md +++ b/skills/mergify-config/SKILL.md @@ -18,10 +18,16 @@ mergify config simulate PULL_REQUEST_URL # Simulate actions on a PR ## Configuration File Detection -Mergify CLI auto-detects the configuration file from standard locations: -- `.mergify.yml` -- `.mergify/config.yml` -- `.github/mergify.yml` +Mergify CLI auto-detects the configuration file from standard locations, in +this order — the first one that exists wins: +- `.mergify.yml`, then `.mergify.yaml` +- `.mergify/config.yml`, then `.mergify/config.yaml` +- `.github/mergify.yml`, then `.github/mergify.yaml` + +Both extensions are read at every location, and `.yml` wins when a location +carries both. A repository holding more than one configuration file gets a +warning on stderr naming the file in use and the ones ignored — the engine +applies the same order, so the CLI reads whatever production reads. Override with `--config-file` / `-f`: ```bash