Skip ignored files in check_diff #6794
Conversation
Implement skipping logic just like rustfmt does. rustfmt would ignore these files anyway so we can save some time and effort if we avoid spawning a subprocess for files that we know rustfmt is going to ignore anyway. This saves a significant amount of time, especially for repositories like r-l/rust which ignore a lot of files.
These repos are explicitly skipped by r-l/rust's `rustfmt.toml` so i'm adding them to include them in the diff check.
rustbot
added
A-CI
|
Shaves off another few minutes even after adding two more repos to check: Before: After: |
| } | ||
|
|
||
| pub fn iter(&self) -> impl Iterator<Item = PathBuf> + use<'_, P> { | ||
| WalkDir::new(self.repo.path()) |
There was a problem hiding this comment.
Question: should this be using ignore::Walk instead of walkdir::WalkDir? Difference being ignore::Walk should respect .gitignore while finding the entries too. I don't think this matters much if at all in practice though, I guess an edge case is hypothetically if some random.rs file is broken but in-tree vs feature rustfmt formats broken code differently...
There was a problem hiding this comment.
Thinking about it now, I don't think there's a real difference between walkdir::WalkDir and ignore::Walk in this case since we're cloning these code bases from an upstream git repo and presumably files listed in the .gitignore have already been excluded.
There's always a chance that a file that was supposed to be ignored made it to the upstream repo, but I think that'll be rare and overall we'd be okay even if that happened.
When it comes to the the broken file point, I think it mostly depends on what you consider to be a broken file. I'd define a broken file as one that can't be parsed and formatted by rustfmt. rustlings is full of these broken files and doesn't really impact things. It would probably be nice to know why one or both of the binaries failed, but we can only try to produce a diff when both binaries successfully parse and format a file as shown in the table below (logic defined here):
| main rustfmt | feature rustfmt | can produce a diff? |
|---|---|---|
| ✅ | ✅ | ✅ |
| ❌ | ✅ | ❌ |
| ✅ | ❌ | ❌ |
| ❌ | ❌ | ❌ |
Ultimately, with the diff check job we care less about the actual formatting and more about both binaries agreeing on the formatting.
In the random.rs example you describe both binaries producing different formatting and in that case we'd produce a diff and the Diff Check would fail. In that case, I'd say the Diff Check is doing it's job and that it's a good thing we didn't look at the .gitignore.
rustbot
added
release-notes
|
@jieyouxu Nice to see the |
3 participants
Implement skipping logic just like rustfmt does. rustfmt would ignore these files anyway so we can save some time and effort if we avoid shelling out to rustfmt if we know rustfmt is going to ignore these files anyway.
This saves a significant amount of time, especially for repositories like
r-l/rustwhich ignore a lot of files.