Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Streamline x fmt and improve its output #125699

Merged
merged 5 commits into from
May 30, 2024
Merged

Streamline x fmt and improve its output #125699

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
5cf198d
Remove path choice from `x fmt` and add `--all` option.
nnethercote May 28, 2024
e98740a
Clarify `x fmt` messages.
nnethercote May 29, 2024
3d5d6d2
Adjust `x fmt` printed output.
nnethercote May 29, 2024
a22cfcc
Rename `fmt_override`.
nnethercote May 29, 2024
4dec0a0
Clarify the closure in `rustfmt`.
nnethercote May 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Adjust x fmt printed output. 
Currently, `x fmt` can print two lists of files.
- The untracked files that are skipped. Always done if within a git
  repo.
- The modified files that are formatted.

But if you run with `--all` (or with `GITHUB_ACTIONS=true`) it doesn't
print anything about which files are formatted.

This commit increases consistency.
- The formatted/checked files are now always printed. And it makes it clear why
  a file was formatted, e.g. with "modified".
- It uses the same code for both untracked files and formatted/checked
  files. This means that now if there are a lot of untracked files just
  the number will be printed, which is like the old behaviour for
  modified files.

Example output:
```
fmt: skipped 31 untracked files
fmt: formatted modified file compiler/rustc_mir_transform/src/instsimplify.rs
fmt: formatted modified file compiler/rustc_mir_transform/src/validate.rs
fmt: formatted modified file library/core/src/ptr/metadata.rs
fmt: formatted modified file src/bootstrap/src/core/build_steps/format.rs
```
or (with `--all`):
```
fmt: checked 3148 files
```
  • Loading branch information
@nnethercote
nnethercote committed May 29, 2024
commit 3d5d6d222084db76ac152b6c813a3bc177bec8ce
74 changes: 45 additions & 29 deletions src/bootstrap/src/core/build_steps/format.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ use std::collections::VecDeque;
use std::path::{Path, PathBuf};
use std::process::{Command, Stdio};
use std::sync::mpsc::SyncSender;
use std::sync::Mutex;

fn rustfmt(src: &Path, rustfmt: &Path, paths: &[PathBuf], check: bool) -> impl FnMut(bool) -> bool {
let mut cmd = Command::new(rustfmt);
Expand Down Expand Up @@ -97,6 +98,21 @@ struct RustfmtConfig {
ignore: Vec<String>,
}

// 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(verb: &str, adjective: Option<&str>, paths: &[String]) {
let len = paths.len();
let adjective =
if let Some(adjective) = adjective { format!("{adjective} ") } else { String::new() };
if len <= 10 {
for path in paths {
println!("fmt: {verb} {adjective}file {path}");
}
} else {
println!("fmt: {verb} {len} {adjective}files");
}
}

pub fn format(build: &Builder<'_>, check: bool, all: bool, paths: &[PathBuf]) {
if !paths.is_empty() {
eprintln!("fmt error: path arguments are not accepted");
Expand Down Expand Up @@ -149,6 +165,7 @@ pub fn format(build: &Builder<'_>, check: bool, all: bool, paths: &[PathBuf]) {
Err(_) => false,
};

let mut adjective = None;
if git_available {
let in_working_tree = match build
.config
Expand All @@ -172,45 +189,27 @@ pub fn format(build: &Builder<'_>, check: bool, all: bool, paths: &[PathBuf]) {
.arg("-z")
.arg("--untracked-files=normal"),
);
let untracked_paths = untracked_paths_output.split_terminator('\0').filter_map(
|entry| entry.strip_prefix("?? "), // returns None if the prefix doesn't match
);
let mut untracked_count = 0;
let untracked_paths: Vec<_> = untracked_paths_output
.split_terminator('\0')
.filter_map(
|entry| entry.strip_prefix("?? "), // returns None if the prefix doesn't match
)
.map(|x| x.to_string())
.collect();
print_paths("skipped", Some("untracked"), &untracked_paths);

for untracked_path in untracked_paths {
println!("fmt: skip untracked path {untracked_path} during rustfmt invocations");
// The leading `/` makes it an exact match against the
// repository root, rather than a glob. Without that, if you
// have `foo.rs` in the repository root it will also match
// against anything like `compiler/rustc_foo/src/foo.rs`,
// preventing the latter from being formatted.
untracked_count += 1;
fmt_override.add(&format!("!/{untracked_path}")).expect(untracked_path);
fmt_override.add(&format!("!/{untracked_path}")).expect(&untracked_path);
}
if !all {
adjective = Some("modified");
match get_modified_rs_files(build) {
Ok(Some(files)) => {
if files.len() <= 10 {
for file in &files {
println!("fmt: formatting modified file {file}");
}
} else {
let pluralized = |count| if count > 1 { "files" } else { "file" };
let untracked_msg = if untracked_count == 0 {
"".to_string()
} else {
format!(
", skipped {} untracked {}",
untracked_count,
pluralized(untracked_count),
)
};
println!(
"fmt: formatting {} modified {}{}",
files.len(),
pluralized(files.len()),
untracked_msg
);
}
for file in files {
fmt_override.add(&format!("/{file}")).expect(&file);
}
Expand Down Expand Up @@ -278,16 +277,33 @@ pub fn format(build: &Builder<'_>, check: bool, all: bool, paths: &[PathBuf]) {
}
});

let formatted_paths = Mutex::new(Vec::new());
let formatted_paths_ref = &formatted_paths;
walker.run(|| {
let tx = tx.clone();
Box::new(move |entry| {
let cwd = std::env::current_dir();
let entry = t!(entry);
if entry.file_type().map_or(false, |t| t.is_file()) {
formatted_paths_ref.lock().unwrap().push({
// `into_path` produces an absolute path. Try to strip `cwd` to get a shorter
// relative path.
let mut path = entry.clone().into_path();
if let Ok(cwd) = cwd {
if let Ok(path2) = path.strip_prefix(cwd) {
path = path2.to_path_buf();
}
}
Comment on lines +294 to +298
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small nit:

Suggested change
if let Ok(cwd) = cwd {
if let Ok(path2) = path.strip_prefix(cwd) {
path = path2.to_path_buf();
}
}
if let Ok(path2) = cwd.and_then(|cwd| path.strip_prefix(cwd)) {
path = path2.to_path_buf();
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't work because the error types in the two Results don't match:

error[E0308]: mismatched types
   --> src/core/build_steps/format.rs:292:45
    |
292 |             if let Ok(path2) = cwd.and_then(|cwd| path.strip_prefix(cwd)) {
    |                                                   ^^^^^^^^^^^^^^^^^^^^^^ expected `Result<_, Error>`, found `Result<&Path, ...>`
    |
    = note: expected enum `Result<_, std::io::Error>`
               found enum `Result<&Path, StripPrefixError>`

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm looking forward to let-chains reaching stable :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Me too...

path.display().to_string()
});
t!(tx.send(entry.into_path()));
}
ignore::WalkState::Continue
})
});
let mut paths = formatted_paths.into_inner().unwrap();
paths.sort();
print_paths(if check { "checked" } else { "formatted" }, adjective, &paths);

drop(tx);

Expand Down