Skip to content

tidy: check for invalid file names #143957

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.

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

samueltardieu
Copy link
Contributor

@samueltardieu samueltardieu commented Jul 15, 2025

Check for file names added to git with:

  • non-UTF8 filenames (this would fail "fmt check" with a decoding error for the moment, but maybe we should not count on it as it is an accidental failure)
  • control characters (such as "\n" or "\r" in file names)
  • ":" (which is a special character on Windows, made rustdoc-json: Structured attributes #142936 fail in bors while it could have be caught earlier)

It only checks files known by git as a developer might want to have "strange" file names alongside their local repository as long as they don't check them in.

r? jieyouxu
as he stumbled upon such a file in #142936

@rustbot
Copy link
Collaborator

rustbot commented Jul 15, 2025

jieyouxu is currently at their maximum review capacity.
They may take a while to respond.

@rustbot rustbot added A-tidy Area: The tidy tool S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels Jul 15, 2025
@rustbot
Copy link
Collaborator

rustbot commented Jul 15, 2025

There are changes to the tidy tool.

cc @jieyouxu

@Kobzol
Copy link
Member

Kobzol commented Jul 15, 2025

Just out of curiosity, how long does it take to run this on your machine?

@samueltardieu
Copy link
Contributor Author

To run git ls-files in my working directory, around 16ms wall time. Timing x test tidy gives no perceivable difference in walltime (4.22s vs. 4.26s, but it can vary in both directions – 24 virtual cores laptop):

with this PR:

Executed in    4.26 secs    fish           external
   usr time   14.15 secs    3.70 millis   14.15 secs
   sys time    5.47 secs    1.12 millis    5.47 secs

without this PR:

Executed in    4.22 secs    fish           external
   usr time   14.35 secs    0.79 millis   14.35 secs
   sys time    5.40 secs    1.13 millis    5.40 secs

@Kobzol
Copy link
Member

Kobzol commented Jul 15, 2025

Okay, that should be fine. Hopefully it won't be sluggish e.g. on Windows, but I think that tidy does so much I/O anyway that this won't be bad.

@samueltardieu
Copy link
Contributor Author

Yeah, since it uses only file names and not even file contents, there is a good chance that the cache is primed already (or that this primes it for other parts of tidy). If it gets r+, maybe rollup=never would make it easier to roll it back in case it is heavier than expected on some platforms.

@Kobzol
Copy link
Member

Kobzol commented Jul 15, 2025

I think it will be fine, I realized that it won't even read all the filenames from disk, which I first wrongly assumed, just return the filenames from the internal git DB, that should be fine.

The git -C argument wouldn't work on some of our CI runners with old OS, but I think these should be just the dist builders that don't run tests anyway. Let's see.

@bors r+

@bors
Copy link
Collaborator

bors commented Jul 15, 2025

📌 Commit cbaaf15 has been approved by Kobzol

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-tidy Area: The tidy tool S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants