tidy should not traverse untracked directories

[pnkfelix]

I ran (effectively):

% git pull origin master
...
% git status
On branch moz-master
Your branch is ahead of 'pnk-gh/moz-master' by 5558 commits.
  (use "git push" to publish your local commits)

Untracked files:
  (use "git add <file>..." to include in what will be committed)

	../src/ignore_me/

nothing added to commit but untracked files present (use "git add" to track)
% cat ../src/ignore_me/should_be_ignored.rs
use tidy;
use not_so_smart;

// THIS IS AN ABSURDLY LONG LINE OVER HERE THIS IS AN ABSURDLY LONG LINE OVER HERE THIS IS AN ABSURDLY LONG LINE OVER HERE
% ../x.py test src/tools/tidy
Updating only changed submodules
Submodules updated in 0.08 seconds
    Finished dev [unoptimized] target(s) in 0.16s
Diff in /Users/felixklock/Dev/Mozilla/rust.git/src/ignore_me/should_be_ignored.rs at line 1:
-use tidy;
 use not_so_smart;
+use tidy;
 
 // THIS IS AN ABSURDLY LONG LINE OVER HERE THIS IS AN ABSURDLY LONG LINE OVER HERE THIS IS AN ABSURDLY LONG LINE OVER HERE
 
Running `"/Users/felixklock/Dev/Mozilla/rust.git/objdir-dbgopt/build/x86_64-apple-darwin/stage0/bin/rustfmt" "--config-path" "/Users/felixklock/Dev/Mozilla/rust.git" "--edition" "2018" "--unstable-features" "--skip-children" "--check" "/Users/felixklock/Dev/Mozilla/rust.git/src/ignore_me/should_be_ignored.rs"` failed.
If you're running `tidy`, try again with `--bless` flag. Or, you just want to format code, run `./x.py fmt` instead.
failed to run: /Users/felixklock/Dev/Mozilla/rust.git/objdir-dbgopt/build/bootstrap/debug/bootstrap test src/tools/tidy
Build completed unsuccessfully in 0:00:03
% 

There are often extraneous files and directories in my source tree. And often they are not even files/directories that I had created (see (*) below).

The problem is that tidy does a blanket traversal of the tree under src/, as far as I can tell, and thus will happily flag "problems" in files that are no longer effectively part of the source code.

(My most recent instance of this was tidy flagging a problem in a .rs file under src/stdsimd/.)

Eventually, after several rounds of trying to understand why non-tidy code got checked into the github repo, I'll eventually remember to look at git status and take note of the directories listed under untracked files. And then, usually, delete them.

But this should not be necessary. tidy should be smart enough to run git status itself and use that to drive its traversal.

---

(*) part of the above process is the build system will automatically update submodules. But old directories from old (outdated) submodule checkouts still stick around.

[Mark-Simulacrum]

@pnkfelix Do you know which "part" of tidy is doing this? I suspect rustfmt myself, but am not quite sure :)

Ideally we'd do a global audit I guess for using git ls-files or so vs. direct traversal.

[pnkfelix]

No, I don't know which part is doing it. I was assuming it was src/tools/tidy that was gathering the set of paths to feed into rustfmt, but I have not confirmed that hypothesis.

[Mark-Simulacrum]

Well, I meant specifically what the errors were about -- I assume rustfmt based on your last message?

i.e., these are not (for example) line length or license check warnings, right?

[pnkfelix]

Oh I see.

I've only observed the problem with the rustfmt pass.

I just did a quick test; it seems like only the rustfmt checks are inspecting the untracked files. (And I updated the description to include the output from my quick little test.)

Or at least, an untracked file with a long line does not seem to trigger the line length check.

I'm not clear on how the current license check works; I skimmed the deps.rs code in src/tools/tidy but couldn't work out where I should be finding a vendor/ directory, which seems to be tied into the workings of the tidy::deps::check method.

[Mark-Simulacrum]

I spent a bit of time trying to look at this, but failed to quickly figure out how to get a list of files from git. AFAICT, git ls-files or git ls-tree don't quite do what we want. In particular, I was unable to coax git into listing checked in files but not to list src/test, for example.

It was my impression based on some searching online that this is perhaps even expected behavior. It's possible that we could do some sort of dual listing, where we both apply the current ignore rules, but also only visit files in that list.

I also checked, and it's my impression that all of our checks would hit this problem -- rustfmt is just significantly more likely to do so.

[pnkfelix]

I also checked, and it's my impression that all of our checks would hit this problem -- rustfmt is just significantly more likely to do so.

This is true. (E.g. I just ran into it with untracked files that each had more than one trailing newine.)

[jyn514]

I suspect this was fixed by #106440.

[jyn514]

Ok, the current behavior is that tidy doesn't look at ignored files (i.e. files in .gitignore), but it does look at untracked files. x fmt --check looks at neither.

The fix for this is probably to run git ls-files . --exclude-standard --others and skip files that it outputs in the default filter_dirs function:

rust/src/tools/tidy/src/walk.rs

Lines 5 to 35 in 8af42f6

	/// The default directory filter.
	pub fn filter_dirs(path: &Path) -> bool {
	let skip = [
	"tidy-test-file",
	"compiler/rustc_codegen_cranelift",
	"compiler/rustc_codegen_gcc",
	"src/llvm-project",
	"library/backtrace",
	"library/portable-simd",
	"library/stdarch",
	"src/tools/cargo",
	"src/tools/clippy",
	"src/tools/miri",
	"src/tools/rls",
	"src/tools/rust-analyzer",
	"src/tools/rust-installer",
	"src/tools/rustfmt",
	"src/doc/book",
	"src/doc/edition-guide",
	"src/doc/embedded-book",
	"src/doc/nomicon",
	"src/doc/rust-by-example",
	"src/doc/rustc-dev-guide",
	"src/doc/reference",
	// Filter RLS output directories
	"target/rls",
	"src/bootstrap/target",
	"vendor",
	];
	skip.iter().any(|p| path.ends_with(p))
	}

[Milo123459]

@rustbot claim

[the8472]

Imo tidy should check untracked files. If I run tidy before committing code and then commit+push I'd get CI errors because tidy skipped those.

[clubby789]

Removing assignment due to #112921 (comment)

[NoobProgrammer31]

assign it to me , i will look into it

[clubby789]

@NoobProgrammer31 Done 🙂 In future you can use rustbot to claim issues (see example above )

[ismailarilik]

Imo tidy should check untracked files. If I run tidy before committing code and then commit+push I'd get CI errors because tidy skipped those.

Totally agree. Being forced to stage files before running lint is annoying. Of course, there should be a flow here most developers are agreed on and once someone gets used to it, it would makes sense. I tried hard to think objective below:

• "Tracked" vs "untracked" is for Git and they mean something to Git. They mean nothing to a build or lint tool; we are adding a meaning to it. In that regard, it is not intuitive.
• If it should mean something to a lint tool, it should also mean the same thing to all build tools. For example; x build or x test should be aware of this distinction (Sorry if they already are.).
• We can always use "stash" to hide files, which is very intuitive if we don't want a tool seeing a file.