Rollup merge of rust-lang#139867 - WaffleLapkin:tidypaper, r=jieyouxu

Fix some tidy paper cuts

The main thing this fixes is that currently, if you run `x t tidy` it will format ~6K files, even though it's supposed to format only modified files (whatever this is a useful optimization or not is besides the point). The problem is that `x t tidy` never writes the `rustfmt` stamp, so it always assumes `rustfmt` that was last used is outdated and we need to recheck everything. This PR fixes it by actually writing the stamp.

There are also some minor tweaks to comments/diagnostics. cc `@Kobzol` this probably conflicts with rust-lang#138591. I didn't fix anything, just tried to document better the status quo.

r? `@jieyouxu`

Author: Zalathar

src/bootstrap/src/core/build_steps/format.rs

@@ -81,14 +81,19 @@ fn update_rustfmt_version(build: &Builder<'_>) {
     let Some((version, stamp_file)) = get_rustfmt_version(build) else {
         return;
     };
-    t!(std::fs::write(stamp_file.path(), version))
+
+    t!(stamp_file.add_stamp(version).write());
 }
 
-/// Returns the Rust files modified between the `merge-base` of HEAD and
-/// rust-lang/master and what is now on the disk. Does not include removed files.
+/// Returns the Rust files modified between the last merge commit and what is now on the disk.
+/// Does not include removed files.
 ///
 /// Returns `None` if all files should be formatted.
 fn get_modified_rs_files(build: &Builder<'_>) -> Result<Option<Vec<String>>, String> {
+    // In CI `get_git_modified_files` returns something different to normal environment.
+    // This shouldn't be called in CI anyway.
+    assert!(!build.config.is_running_on_ci);
+
     if !verify_rustfmt_version(build) {
         return Ok(None);
     }
@@ -103,7 +108,7 @@ struct RustfmtConfig {
 
 // Prints output describing a collection of paths, with lines such as "formatted modified file
 // foo/bar/baz" or "skipped 20 untracked files".
-fn print_paths(build: &Builder<'_>, verb: &str, adjective: Option<&str>, paths: &[String]) {
+fn print_paths(verb: &str, adjective: Option<&str>, paths: &[String]) {
     let len = paths.len();
     let adjective =
         if let Some(adjective) = adjective { format!("{adjective} ") } else { String::new() };
@@ -114,9 +119,6 @@ fn print_paths(build: &Builder<'_>, verb: &str, adjective: Option<&str>, paths:
     } else {
         println!("fmt: {verb} {len} {adjective}files");
     }
-    if len > 1000 && !build.config.is_running_on_ci {
-        println!("hint: if this number seems too high, try running `git fetch origin master`");
-    }
 }
 
 pub fn format(build: &Builder<'_>, check: bool, all: bool, paths: &[PathBuf]) {
@@ -189,7 +191,7 @@ pub fn format(build: &Builder<'_>, check: bool, all: bool, paths: &[PathBuf]) {
                 )
                 .map(|x| x.to_string())
                 .collect();
-            print_paths(build, "skipped", Some("untracked"), &untracked_paths);
+            print_paths("skipped", Some("untracked"), &untracked_paths);
 
             for untracked_path in untracked_paths {
                 // The leading `/` makes it an exact match against the
@@ -212,7 +214,13 @@ pub fn format(build: &Builder<'_>, check: bool, all: bool, paths: &[PathBuf]) {
                             override_builder.add(&format!("/{file}")).expect(&file);
                         }
                     }
-                    Ok(None) => {}
+                    Ok(None) => {
+                        // NOTE: `Ok(None)` signifies that we need to format all files.
+                        // The tricky part here is that if `override_builder` isn't given any white
+                        // list files (i.e. files to be formatted, added without leading `!`), it
+                        // will instead look for *all* files. So, by doing nothing here, we are
+                        // actually making it so we format all files.
+                    }
                     Err(err) => {
                         eprintln!("fmt warning: Something went wrong running git commands:");
                         eprintln!("fmt warning: {err}");
@@ -318,7 +326,7 @@ pub fn format(build: &Builder<'_>, check: bool, all: bool, paths: &[PathBuf]) {
     });
     let mut paths = formatted_paths.into_inner().unwrap();
     paths.sort();
-    print_paths(build, if check { "checked" } else { "formatted" }, adjective, &paths);
+    print_paths(if check { "checked" } else { "formatted" }, adjective, &paths);
 
     drop(tx);
 
@@ -328,7 +336,10 @@ pub fn format(build: &Builder<'_>, check: bool, all: bool, paths: &[PathBuf]) {
         crate::exit!(1);
     }
 
-    if !check {
-        update_rustfmt_version(build);
-    }
+    // Update `build/.rustfmt-stamp`, allowing this code to ignore files which have not been changed
+    // since last merge.
+    //
+    // NOTE: Because of the exit above, this is only reachable if formatting / format checking
+    // succeeded. So we are not commiting the version if formatting was not good.
+    update_rustfmt_version(build);
 }

src/build_helper/src/git.rs

@@ -114,7 +114,9 @@ fn git_upstream_merge_base(
     Ok(output_result(git.arg("merge-base").arg(&updated_master).arg("HEAD"))?.trim().to_owned())
 }
 
-/// Searches for the nearest merge commit in the repository that also exists upstream.
+/// Searches for the nearest merge commit in the repository.
+///
+/// **In CI** finds the nearest merge commit that *also exists upstream*.
 ///
 /// It looks for the most recent commit made by the merge bot by matching the author's email
 /// address with the merge bot's email.
@@ -165,7 +167,7 @@ pub fn get_closest_merge_commit(
     Ok(output_result(&mut git)?.trim().to_owned())
 }
 
-/// Returns the files that have been modified in the current branch compared to the master branch.
+/// Returns the files that have been modified in the current branch compared to the last merge.
 /// The `extensions` parameter can be used to filter the files by their extension.
 /// Does not include removed files.
 /// If `extensions` is empty, all files will be returned.

src/tools/tidy/src/deps.rs

@@ -682,8 +682,10 @@ pub static CRATES: &[&str] = &[
 pub fn has_missing_submodule(root: &Path, submodules: &[&str]) -> bool {
     !CiEnv::is_ci()
         && submodules.iter().any(|submodule| {
+            let path = root.join(submodule);
+            !path.exists()
             // If the directory is empty, we can consider it as an uninitialized submodule.
-            read_dir(root.join(submodule)).unwrap().next().is_none()
+            || read_dir(path).unwrap().next().is_none()
         })
 }