Modify the pre-push githook to run Tidy on only committed changes.

[simp4t7]

I think this is a fairly simple solution for #125654. The basic idea is just to stash local changes, run tidy, and then pop the changes back after Tidy runs for the pre-push git hook. It sort of got a bit more complicated with the error handling, but still seems like a reasonable solution.

A few things to note:

I opted to fail the whole check if git stash push -u fails instead of continuing to run Tidy on the unstashed changes. My thinking is git stash doesn't really fail easily so if it does something is probably pretty wrong and this way there's no inconsistency on what is being checked.

In general I think it's unlikely for git stash apply to fail because Tidy does not actually change files during its checks so there shouldn't be any risk of merge conflicts. If it does fail, it's a bit of a pain for the user, but handled and nothing is lost.

[rustbot]

r? @Mark-Simulacrum

rustbot has assigned @Mark-Simulacrum.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[Kobzol]

I think that doing anything with untracked files might be a bit dangerous. I personally wouldn't want these scripts ever touching anything untracked by git.

[simp4t7]

@Kobzol

So I think the best way to avoid stashing untracked files would probably be to have Tidy only run on tracked files. fmt already does this, so it sort of makes sense.

I did a little bit of digging anyways, and don't really think stashing untracked is extra dangerous though. Stash is basically all or nothing so on failure the working dir won't be affected. And once stashed it's still a commit under the hood so it's recoverable even if you explicitly drop the stash. But it is still stored locally, so there's always some risk I suppose. Just a note that --include-untracked will still respect the .gitignore.

Another option that could work well is throwing in a prompt asking if you want to stash changes before continuing. So if you're really worried you can back out and handle it manually if you prefer. I think it'd still have to include untracked unless you changed Tidy to ignore untracked first.

[bors]

☔ The latest upstream changes (presumably #147118) made this pull request unmergeable. Please resolve the merge conflicts.

[Kobzol]

I think there are two approaches that we could do here:

• Have an optional flag that will tell tidy to ignore untracked files by autostashing. I would implement this in tidy directly, rather than in a bash script.
• Examine Tidy to figure out all the places where it runs code essentially on *.[ext], and then modify it to either not do that, or first gather all the files, and then use git to filter out only files that are actually tracked.

I would very much prefer the second solution, if we figure out that it's doable.

[simp4t7]

So I looked into this a bit and option 2 seems doable. There's already a get_git_untracked_files that can be reused. The most straightforward way is just to modify walk_no_read to filter untracked files before each check. Then each of the extra-checks needs its own filter for its respective linter which should be doable as well.

I think setting tidy to ignore untracked files as the default and adding an optional flag to include untracked is an option and a bit more user-friendly but adds some complexity. It's plausible people want to check untracked files and having to git add instead of using a tidy flag is awkward.

I think keeping the stash in the bash script makes sense and just remove the --include-untracked flag. Assuming tidy already ignores untracked I think the only time you'd want to stash before checking is in the pre-push githook. But you still need the stash because you don't want to check on uncommited changes on tracked files for the githook.

[Kobzol]

I agree that ignoring untracked files by default is a good idea, I often run into tidy errors because I have a bunch of crap laying on my disk in the source root 😆 When we do that, we might as well ignore uncommitted files in the git index, but this only makes sense in the pre-push hook.

Still unsure about the stashing though, I think that some people (myself included) might be unhappy about the hook messing with their git state. I asked about this on our Zulip.