Auto merge of #106415 - Nilstrieb:where-is-my-master-branch, r=jyn514

Handle non-existent upstream master branches in `x fmt`

People who do have a remote for `rust-lang/rust` but don't have the master branch checked out there used to get this error when running `x fmt`:
> fatal: ambiguous argument 'rust/master': unknown revision or path not in the working tree.
> Use '--' to separate paths from revisions, like this:
> 'git <command> [<revision>...] -- [<file>...]'
> rust/master

Which is not exactly helpful.

Now, we fall back to `origin/master` (hoping that at least that remote exists) for that case. If there is still some error, we just fall back to `x fmt .` and print a warning.

r? `@jyn514`

Author: bors

Cargo.lock

@@ -271,6 +271,10 @@ dependencies = [
  "toml",
 ]
 
+[[package]]
+name = "build_helper"
+version = "0.1.0"
+
 [[package]]
 name = "bump-stage0"
 version = "0.1.0"

Cargo.toml

@@ -4,6 +4,7 @@ members = [
   "library/std",
   "library/test",
   "src/rustdoc-json-types",
+  "src/tools/build_helper",
   "src/tools/cargotest",
   "src/tools/clippy",
   "src/tools/clippy/clippy_dev",

src/bootstrap/Cargo.lock

@@ -36,6 +36,7 @@ dependencies = [
 name = "bootstrap"
 version = "0.0.0"
 dependencies = [
+ "build_helper",
  "cc",
  "cmake",
  "fd-lock",
@@ -70,6 +71,10 @@ dependencies = [
  "regex-automata",
 ]
 
+[[package]]
+name = "build_helper"
+version = "0.1.0"
+
 [[package]]
 name = "cc"
 version = "1.0.73"

src/bootstrap/Cargo.toml

@@ -30,6 +30,7 @@ path = "bin/sccache-plus-cl.rs"
 test = false
 
 [dependencies]
+build_helper = { path = "../tools/build_helper" }
 cmake = "0.1.38"
 fd-lock = "3.0.8"
 filetime = "0.2"

src/bootstrap/format.rs

@@ -1,7 +1,8 @@
 //! Runs rustfmt on the repository.
 
 use crate::builder::Builder;
-use crate::util::{output, program_out_of_date, t};
+use crate::util::{output, output_result, program_out_of_date, t};
+use build_helper::git::updated_master_branch;
 use ignore::WalkBuilder;
 use std::collections::VecDeque;
 use std::path::{Path, PathBuf};
@@ -78,50 +79,24 @@ fn update_rustfmt_version(build: &Builder<'_>) {
 /// rust-lang/master and what is now on the disk.
 ///
 /// Returns `None` if all files should be formatted.
-fn get_modified_rs_files(build: &Builder<'_>) -> Option<Vec<String>> {
-    let Ok(remote) = get_rust_lang_rust_remote() else { return None; };
+fn get_modified_rs_files(build: &Builder<'_>) -> Result<Option<Vec<String>>, String> {
+    let Ok(updated_master) = updated_master_branch(Some(&build.config.src)) else { return Ok(None); };
+
     if !verify_rustfmt_version(build) {
-        return None;
+        return Ok(None);
     }
 
     let merge_base =
-        output(build.config.git().arg("merge-base").arg(&format!("{remote}/master")).arg("HEAD"));
-    Some(
-        output(build.config.git().arg("diff-index").arg("--name-only").arg(merge_base.trim()))
-            .lines()
-            .map(|s| s.trim().to_owned())
-            .filter(|f| Path::new(f).extension().map_or(false, |ext| ext == "rs"))
-            .collect(),
-    )
-}
-
-/// Finds the remote for rust-lang/rust.
-/// For example for these remotes it will return `upstream`.
-/// ```text
-/// origin  https://github.com/Nilstrieb/rust.git (fetch)
-/// origin  https://github.com/Nilstrieb/rust.git (push)
-/// upstream        https://github.com/rust-lang/rust (fetch)
-/// upstream        https://github.com/rust-lang/rust (push)
-/// ```
-fn get_rust_lang_rust_remote() -> Result<String, String> {
-    let mut git = Command::new("git");
-    git.args(["config", "--local", "--get-regex", "remote\\..*\\.url"]);
-
-    let output = git.output().map_err(|err| format!("{err:?}"))?;
-    if !output.status.success() {
-        return Err("failed to execute git config command".to_owned());
-    }
-
-    let stdout = String::from_utf8(output.stdout).map_err(|err| format!("{err:?}"))?;
-
-    let rust_lang_remote = stdout
+        output_result(build.config.git().arg("merge-base").arg(&updated_master).arg("HEAD"))?;
+    Ok(Some(
+        output_result(
+            build.config.git().arg("diff-index").arg("--name-only").arg(merge_base.trim()),
+        )?
         .lines()
-        .find(|remote| remote.contains("rust-lang"))
-        .ok_or_else(|| "rust-lang/rust remote not found".to_owned())?;
-
-    let remote_name =
-        rust_lang_remote.split('.').nth(1).ok_or_else(|| "remote name not found".to_owned())?;
-    Ok(remote_name.into())
+        .map(|s| s.trim().to_owned())
+        .filter(|f| Path::new(f).extension().map_or(false, |ext| ext == "rs"))
+        .collect(),
+    ))
 }
 
 #[derive(serde::Deserialize)]
@@ -158,6 +133,9 @@ pub fn format(build: &Builder<'_>, check: bool, paths: &[PathBuf]) {
         Ok(status) => status.success(),
         Err(_) => false,
     };
+
+    let mut paths = paths.to_vec();
+
     if git_available {
         let in_working_tree = match build
             .config
@@ -191,10 +169,21 @@ pub fn format(build: &Builder<'_>, check: bool, paths: &[PathBuf]) {
                 ignore_fmt.add(&format!("!/{}", untracked_path)).expect(&untracked_path);
             }
             if !check && paths.is_empty() {
-                if let Some(files) = get_modified_rs_files(build) {
-                    for file in files {
-                        println!("formatting modified file {file}");
-                        ignore_fmt.add(&format!("/{file}")).expect(&file);
+                match get_modified_rs_files(build) {
+                    Ok(Some(files)) => {
+                        for file in files {
+                            println!("formatting modified file {file}");
+                            ignore_fmt.add(&format!("/{file}")).expect(&file);
+                        }
+                    }
+                    Ok(None) => {}
+                    Err(err) => {
+                        println!(
+                            "WARN: Something went wrong when running git commands:\n{err}\n\
+                            Falling back to formatting all files."
+                        );
+                        // Something went wrong when getting the version. Just format all the files.
+                        paths.push(".".into());
                     }
                 }
             }
@@ -204,6 +193,7 @@ pub fn format(build: &Builder<'_>, check: bool, paths: &[PathBuf]) {
     } else {
         println!("Could not find usable git. Skipping git-aware format checks");
     }
+
     let ignore_fmt = ignore_fmt.build().unwrap();
 
     let rustfmt_path = build.initial_rustfmt().unwrap_or_else(|| {

src/bootstrap/lib.rs

@@ -113,6 +113,7 @@ use std::path::{Path, PathBuf};
 use std::process::Command;
 use std::str;
 
+use build_helper::ci::CiEnv;
 use channel::GitInfo;
 use config::{DryRun, Target};
 use filetime::FileTime;
@@ -121,7 +122,7 @@ use once_cell::sync::OnceCell;
 use crate::builder::Kind;
 use crate::config::{LlvmLibunwind, TargetSelection};
 use crate::util::{
-    exe, libdir, mtime, output, run, run_suppressed, symlink_dir, try_run_suppressed, CiEnv,
+    exe, libdir, mtime, output, run, run_suppressed, symlink_dir, try_run_suppressed,
 };
 
 mod bolt;

src/bootstrap/native.rs

@@ -24,6 +24,8 @@ use crate::util::get_clang_cl_resource_dir;
 use crate::util::{self, exe, output, t, up_to_date};
 use crate::{CLang, GitRepo};
 
+use build_helper::ci::CiEnv;
+
 #[derive(Clone)]
 pub struct LlvmResult {
     /// Path to llvm-config binary.
@@ -217,7 +219,7 @@ pub(crate) fn is_ci_llvm_available(config: &Config, asserts: bool) -> bool {
         return false;
     }
 
-    if crate::util::CiEnv::is_ci() {
+    if CiEnv::is_ci() {
         // We assume we have access to git, so it's okay to unconditionally pass
         // `true` here.
         let llvm_sha = detect_llvm_sha(config, true);

src/bootstrap/util.rs

@@ -255,35 +255,6 @@ pub enum CiEnv {
     GitHubActions,
 }
 
-impl CiEnv {
-    /// Obtains the current CI environment.
-    pub fn current() -> CiEnv {
-        if env::var("TF_BUILD").map_or(false, |e| e == "True") {
-            CiEnv::AzurePipelines
-        } else if env::var("GITHUB_ACTIONS").map_or(false, |e| e == "true") {
-            CiEnv::GitHubActions
-        } else {
-            CiEnv::None
-        }
-    }
-
-    pub fn is_ci() -> bool {
-        Self::current() != CiEnv::None
-    }
-
-    /// If in a CI environment, forces the command to run with colors.
-    pub fn force_coloring_in_ci(self, cmd: &mut Command) {
-        if self != CiEnv::None {
-            // Due to use of stamp/docker, the output stream of rustbuild is not
-            // a TTY in CI, so coloring is by-default turned off.
-            // The explicit `TERM=xterm` environment is needed for
-            // `--color always` to actually work. This env var was lost when
-            // compiling through the Makefile. Very strange.
-            cmd.env("TERM", "xterm").args(&["--color", "always"]);
-        }
-    }
-}
-
 pub fn forcing_clang_based_tests() -> bool {
     if let Some(var) = env::var_os("RUSTBUILD_FORCE_CLANG_BASED_TESTS") {
         match &var.to_string_lossy().to_lowercase()[..] {
@@ -441,6 +412,23 @@ pub fn output(cmd: &mut Command) -> String {
     String::from_utf8(output.stdout).unwrap()
 }
 
+pub fn output_result(cmd: &mut Command) -> Result<String, String> {
+    let output = match cmd.stderr(Stdio::inherit()).output() {
+        Ok(status) => status,
+        Err(e) => return Err(format!("failed to run command: {:?}: {}", cmd, e)),
+    };
+    if !output.status.success() {
+        return Err(format!(
+            "command did not execute successfully: {:?}\n\
+             expected success, got: {}\n{}",
+            cmd,
+            output.status,
+            String::from_utf8(output.stderr).map_err(|err| format!("{err:?}"))?
+        ));
+    }
+    Ok(String::from_utf8(output.stdout).map_err(|err| format!("{err:?}"))?)
+}
+
 /// Returns the last-modified time for `path`, or zero if it doesn't exist.
 pub fn mtime(path: &Path) -> SystemTime {
     fs::metadata(path).and_then(|f| f.modified()).unwrap_or(UNIX_EPOCH)

src/tools/build_helper/Cargo.toml

@@ -0,0 +1,8 @@
+[package]
+name = "build_helper"
+version = "0.1.0"
+edition = "2021"
+
+# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html
+
+[dependencies]

src/tools/build_helper/src/ci.rs

@@ -0,0 +1,40 @@
+use std::process::Command;
+
+#[derive(Copy, Clone, PartialEq, Eq, Debug)]
+pub enum CiEnv {
+    /// Not a CI environment.
+    None,
+    /// The Azure Pipelines environment, for Linux (including Docker), Windows, and macOS builds.
+    AzurePipelines,
+    /// The GitHub Actions environment, for Linux (including Docker), Windows and macOS builds.
+    GitHubActions,
+}
+
+impl CiEnv {
+    /// Obtains the current CI environment.
+    pub fn current() -> CiEnv {
+        if std::env::var("TF_BUILD").map_or(false, |e| e == "True") {
+            CiEnv::AzurePipelines
+        } else if std::env::var("GITHUB_ACTIONS").map_or(false, |e| e == "true") {
+            CiEnv::GitHubActions
+        } else {
+            CiEnv::None
+        }
+    }
+
+    pub fn is_ci() -> bool {
+        Self::current() != CiEnv::None
+    }
+
+    /// If in a CI environment, forces the command to run with colors.
+    pub fn force_coloring_in_ci(self, cmd: &mut Command) {
+        if self != CiEnv::None {
+            // Due to use of stamp/docker, the output stream of rustbuild is not
+            // a TTY in CI, so coloring is by-default turned off.
+            // The explicit `TERM=xterm` environment is needed for
+            // `--color always` to actually work. This env var was lost when
+            // compiling through the Makefile. Very strange.
+            cmd.env("TERM", "xterm").args(&["--color", "always"]);
+        }
+    }
+}

src/tools/build_helper/src/git.rs

@@ -0,0 +1,75 @@
+use std::{path::Path, process::Command};
+
+/// Finds the remote for rust-lang/rust.
+/// For example for these remotes it will return `upstream`.
+/// ```text
+/// origin  https://github.com/Nilstrieb/rust.git (fetch)
+/// origin  https://github.com/Nilstrieb/rust.git (push)
+/// upstream        https://github.com/rust-lang/rust (fetch)
+/// upstream        https://github.com/rust-lang/rust (push)
+/// ```
+pub fn get_rust_lang_rust_remote(git_dir: Option<&Path>) -> Result<String, String> {
+    let mut git = Command::new("git");
+    if let Some(git_dir) = git_dir {
+        git.current_dir(git_dir);
+    }
+    git.args(["config", "--local", "--get-regex", "remote\\..*\\.url"]);
+
+    let output = git.output().map_err(|err| format!("{err:?}"))?;
+    if !output.status.success() {
+        return Err("failed to execute git config command".to_owned());
+    }
+
+    let stdout = String::from_utf8(output.stdout).map_err(|err| format!("{err:?}"))?;
+
+    let rust_lang_remote = stdout
+        .lines()
+        .find(|remote| remote.contains("rust-lang"))
+        .ok_or_else(|| "rust-lang/rust remote not found".to_owned())?;
+
+    let remote_name =
+        rust_lang_remote.split('.').nth(1).ok_or_else(|| "remote name not found".to_owned())?;
+    Ok(remote_name.into())
+}
+
+pub fn rev_exists(rev: &str, git_dir: Option<&Path>) -> Result<bool, String> {
+    let mut git = Command::new("git");
+    if let Some(git_dir) = git_dir {
+        git.current_dir(git_dir);
+    }
+    git.args(["rev-parse", rev]);
+    let output = git.output().map_err(|err| format!("{err:?}"))?;
+
+    match output.status.code() {
+        Some(0) => Ok(true),
+        Some(128) => Ok(false),
+        None => {
+            return Err(format!(
+                "git didn't exit properly: {}",
+                String::from_utf8(output.stderr).map_err(|err| format!("{err:?}"))?
+            ));
+        }
+        Some(code) => {
+            return Err(format!(
+                "git command exited with status code: {code}: {}",
+                String::from_utf8(output.stderr).map_err(|err| format!("{err:?}"))?
+            ));
+        }
+    }
+}
+
+/// Returns the master branch from which we can take diffs to see changes.
+/// This will usually be rust-lang/rust master, but sometimes this might not exist.
+/// This could be because the user is updating their forked master branch using the GitHub UI
+/// and therefore doesn't need an upstream master branch checked out.
+/// We will then fall back to origin/master in the hope that at least this exists.
+pub fn updated_master_branch(git_dir: Option<&Path>) -> Result<String, String> {
+    let upstream_remote = get_rust_lang_rust_remote(git_dir)?;
+    let upstream_master = format!("{upstream_remote}/master");
+    if rev_exists(&upstream_master, git_dir)? {
+        return Ok(upstream_master);
+    }
+
+    // We could implement smarter logic here in the future.
+    Ok("origin/master".into())
+}

src/tools/build_helper/src/lib.rs

@@ -0,0 +1,2 @@
+pub mod ci;
+pub mod git;