Handle non-existant upstream master branches in x fmt

Author: Noratrieb

Cargo.lock

@@ -5308,7 +5308,6 @@ dependencies = [
 name = "tidy"
 version = "0.1.0"
 dependencies = [
- "build_helper",
  "cargo_metadata 0.14.0",
  "ignore",
  "lazy_static",

src/bootstrap/format.rs

@@ -1,8 +1,8 @@
 //! Runs rustfmt on the repository.
 
 use crate::builder::Builder;
-use crate::util::{output, program_out_of_date, t};
-use build_helper::git::get_rust_lang_rust_remote;
+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};
@@ -79,21 +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(),
-    )
+        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()
+        .map(|s| s.trim().to_owned())
+        .filter(|f| Path::new(f).extension().map_or(false, |ext| ext == "rs"))
+        .collect(),
+    ))
 }
 
 #[derive(serde::Deserialize)]
@@ -130,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
@@ -163,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());
                     }
                 }
             }
@@ -176,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/util.rs

@@ -412,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/src/git.rs

@@ -1,4 +1,4 @@
-use std::process::Command;
+use std::{path::Path, process::Command};
 
 /// Finds the remote for rust-lang/rust.
 /// For example for these remotes it will return `upstream`.
@@ -8,8 +8,11 @@ use std::process::Command;
 /// upstream        https://github.com/rust-lang/rust (fetch)
 /// upstream        https://github.com/rust-lang/rust (push)
 /// ```
-pub fn get_rust_lang_rust_remote() -> Result<String, String> {
+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:?}"))?;
@@ -28,3 +31,45 @@ pub fn get_rust_lang_rust_remote() -> Result<String, String> {
         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())
+}