search

rust-lang / rust

Empowering everyone to build reliable and efficient software.
https://www.rust-lang.org
Other
99.03k stars 12.79k forks source link

tidy should not traverse untracked directories #69291

Open pnkfelix opened 4 years ago

pnkfelix commented 4 years ago

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 commented 4 years ago

@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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 1 year ago

I suspect this was fixed by https://github.com/rust-lang/rust/pull/106440.

jyn514 commented 1 year ago

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: https://github.com/rust-lang/rust/blob/8af42f695d493ed868098e3d1c02318f631998e4/src/tools/tidy/src/walk.rs#L5-L35

Milo123459 commented 1 year ago

@rustbot claim

the8472 commented 1 year ago

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 commented 1 year ago

Removing assignment due to https://github.com/rust-lang/rust/pull/112921#issuecomment-1681164841

NoobProgrammer31 commented 2 months ago

assign it to me , i will look into it

clubby789 commented 2 months ago

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

ismailarilik commented 1 month ago

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: