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

simp4t7 · 2025-09-27T05:39:17Z

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 · 2025-09-27T05:39:22Z

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 · 2025-09-27T07:59:41Z

src/etc/pre-push.sh

+if [ -n "$(git status --porcelain)" ]; then
+    echo "Stashing local uncommitted changes before running Tidy."
+    # Stash uncommitted changes so that tidy only checks what you are going to push.
+    git stash push -u -q


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 · 2025-09-27T20:30:36Z

@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 · 2025-09-28T14:48:59Z

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

Kobzol · 2025-09-30T12:06:18Z

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 · 2025-10-03T00:11:36Z

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 · 2025-10-03T07:38:35Z

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.

added error handling for git stash and apply fix tidy err abort if stash push fails fix tidy errors... again

rustbot · 2025-10-10T05:07:10Z

The list of allowed third-party dependencies may have been modified! You must ensure that any new dependencies have compatible licenses before merging.

cc @davidtwco, @wesleywiser

tidy extra checks were modified.

cc @lolbinarycat

This PR modifies src/bootstrap/src/core/config.

If appropriate, please update CONFIG_CHANGE_HISTORY in src/bootstrap/src/utils/change_tracker.rs.

rustbot · 2025-10-10T05:07:12Z

This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

simp4t7 · 2025-10-10T05:54:21Z

A bit of an explanation for some of this. I ended up taking some advice from this Zulip thread to get file content from git for running the pre-push tidy check on content from the latest commit. So basically if you pass the --pre-push flag to tidy it will ignore changes in the working dir and get modifed file content from git show HEAD:{file}.

Changed tidy to only run on tracked files. But added a flag --include-untracked to include untracked files. Doesn't seem too useful, but changing the default behavior it's nice to leave an out.

I consolidated --pre-push, --include-untracked, and --bless into a TidyCtx struct and basically just pass it to any check that needs it. I think the slightly cleaner way to do it would be to include TidyCtx with DiagCtx which is automatically passed in but it has a lock and it's not exactly diagnostic so went for this method for now.

Previously for --extra-checks cpp and shell already ignored untracked, and now py and js do as well. spellcheck I have no idea. --extra-checks are not included in the new tidy flags. It's just a bit tricky with the external linters so decided to skip it.

How would I go about testing something like this? I could add some unit tests, but I'm more worried about something like the CI generating files that should be checked but are untracked. I'd just like to know that no checks are being skipped unexpectedly, but not sure how I could get that kind of info.

simp4t7 · 2025-10-10T06:13:36Z

And seeing the CI fail because I modified the walk function signature makes me wonder if maybe there's a better way. Can use the filter for untracked, but it's pretty tedious to manually update for every check and in the case of --pre-push you pretty much have to modify it while it's being walked.

Quick fix would be to make TidyFlags and Option. There was one other tool using it and I just changed it to Default but seems like an option would keep it more easily usable and not have to redo the main logic. But still worth thinking if there's a better way.

rustbot · 2025-10-20T10:01:37Z

These commits modify the Cargo.lock file. Unintentional changes to Cargo.lock can be introduced when switching branches and rebasing PRs.

If this was unintentional then you should revert the changes before this PR is merged.
Otherwise, you can ignore this comment.

simp4t7 · 2025-10-20T23:29:43Z

I merged the DiagCtx and TidyFlags and ended up removing the Option<&TidyFlags and making tidy::walk::walk take &TidyFlags and have downstream functions just use default. There's basically a dozen walk functions in the repo, so I don't hate that they have to take a tidy specific arg if they want to use the tidy walk.

I'm struggling a bit to find a good way to test some of this stuff. I thought I could do tempfiles to basically check the walk function and make sure it's collecting what it should, but the CI is read only so that won't work. --pre-push looks pretty untestable, because would need to modify files or do stuff with a temp branch or something. I could do something like test the walk function and just count the files it collects based on the dir, but as the repo changes it's gonna invalidate it pretty quickly. So, a bit stuck if you have any ideas.

I also ran into an issue I'm not quite sure about. When using --pre-push rustfmt will still run on the working dir before tidy can run. And there are some discrepancies in the style checks between tidy and rustfmt where tidy can pass but rustfmt wil fail. Not totally sure, but checked the content being passed to tidy for --pre-push and seems like it's working as expected. Don't really want to mess with rustfmt, so maybe just leave it as an edge case quirk. But I think testing is the bigger issue still, it would be nice to confirm some basic functionality for tidy both for this PR and perhaps checks in general.

Kobzol

Whoa, that's a lot of changes! Could you please split this into:

A PR that only adds the tidy flags (that would only store bless for now), and propagate the tidy context through tidy? That is most of the mechanical changes in the PR.
A PR that adds --include-untracked?

I'm less sure about the pre-push thing, but we can do that in a third PR.

I don't think that we need to test this in some special way, because tidy already doesn't have a lot of tests. We have some infra in bootstrap to run tests with mocked git contents, but it wouldn't be trivial to add tidy support for that, I think.

View changes since this review

Kobzol · 2025-10-22T07:05:32Z

src/tools/tidy/src/extra_checks/mod.rs


+    let untracked;
+    if let Ok(Some(untracked_files)) = get_git_untracked_files(Some(root_path)) {
+        untracked = format!("--exclude={}", untracked_files.join(","));


Does this also work with directories?

Yeah, this does work with directories. There's already a big ignore list in tidy/config/ruff.toml

Kobzol · 2025-10-22T07:08:16Z

src/tools/tidy/src/diagnostics.rs


+#[derive(Clone, Default, Debug)]
+pub struct TidyFlags {
+    pub bless: bool,


Could you please add doc comments on top of these fields to explain what do they do?

Kobzol · 2025-10-22T07:10:53Z

src/tools/tidy/src/diagnostics.rs

+            _ => HashSet::new(),
+        };
+
+        flags.pre_push_content = pre_push_content;


Umm, I don't like storing these things in the flags. If we want to cache them, I'd put them into the tidy context.

So one issue here is other tools using tidy's walk function. The new TidyCtx (old DiagCtx) cannot be very neatly initialized to pass into functions using walk outside of tidy. Going back to an Option<&TidyCtx> would be the easiest. So would look like:

struct TidyCtx { diag_ctx: ... tidy_flags: TidyFlags, tidy_git_ctx: TidyGitCtx, }

Kobzol · 2025-10-22T07:11:11Z

src/tools/tidy/src/diagnostics.rs

-pub struct DiagCtx(Arc<Mutex<DiagCtxInner>>);
+pub struct TidyCtx {
+    diag_ctx: Arc<Mutex<DiagCtx>>,
+    pub tidy_flags: TidyFlags,


Let's make that a method accessor to avoid changing the flags after being created.

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 27, 2025

rustbot assigned Mark-Simulacrum Sep 27, 2025

Kobzol reviewed Sep 27, 2025

View reviewed changes

Mark-Simulacrum assigned Kobzol and unassigned Mark-Simulacrum Sep 28, 2025

simp4t7 added 5 commits October 10, 2025 04:28

add a stash/pop to the tidy pre-push git-hook

616eb44

added error handling for git stash and apply fix tidy err abort if stash push fails fix tidy errors... again

tidy always ignore untracked

bdd0224

ignore untracked with extra-checks

2607f16

add tidy flags and ignore untracked

9f5cfdf

remove println

5bb1bc3

simp4t7 force-pushed the tidy-git-hook branch from 72aaf67 to 5bb1bc3 Compare October 10, 2025 05:07

rustbot added A-testsuite Area: The testsuite used to check the correctness of rustc A-tidy Area: The tidy tool T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels Oct 10, 2025

This comment has been minimized.

Sign in to view

change TidyFlags to Option and add git context

7b5111a

This comment has been minimized.

Sign in to view

fix CI fails hopefully

da22db6

This comment has been minimized.

Sign in to view

fixed logic with None

8300eca

simp4t7 added 4 commits October 17, 2025 23:34

merge tidyctx and diagctx

3563ca2

update Readme and cosmetic change to walk

51cd406

added some simple tests

a9fac09

fix tidy error

25dff8e

This comment has been minimized.

Sign in to view

add Cargo.lock

b42bae4

This comment has been minimized.

Sign in to view

remove tempfile tests

43e378d

This comment has been minimized.

Sign in to view

remove unused

c70e565

This comment has been minimized.

Sign in to view

update tools using Tidy

df638fb

Kobzol reviewed Oct 22, 2025

View reviewed changes

simp4t7 closed this Oct 23, 2025

rustbot removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 23, 2025

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

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

Uh oh!

Conversation

simp4t7 commented Sep 27, 2025 • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustbot commented Sep 27, 2025

Uh oh!

Kobzol Sep 27, 2025

Choose a reason for hiding this comment

Uh oh!

simp4t7 commented Sep 27, 2025

Uh oh!

bors commented Sep 28, 2025

Uh oh!

Kobzol commented Sep 30, 2025

Uh oh!

simp4t7 commented Oct 3, 2025

Uh oh!

Kobzol commented Oct 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustbot commented Oct 10, 2025

Uh oh!

rustbot commented Oct 10, 2025

Uh oh!

This comment has been minimized.

simp4t7 commented Oct 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

simp4t7 commented Oct 10, 2025

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

rustbot commented Oct 20, 2025

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

simp4t7 commented Oct 20, 2025

Uh oh!

Kobzol left a comment • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Kobzol Oct 22, 2025

Choose a reason for hiding this comment

Uh oh!

simp4t7 Oct 22, 2025

Choose a reason for hiding this comment

Uh oh!

Kobzol Oct 22, 2025

Choose a reason for hiding this comment

Uh oh!

Kobzol Oct 22, 2025

Choose a reason for hiding this comment

Uh oh!

simp4t7 Oct 22, 2025

Choose a reason for hiding this comment

Uh oh!

Kobzol Oct 22, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

simp4t7 commented Sep 27, 2025 •

edited by rustbot

Loading

Kobzol commented Oct 3, 2025 •

edited

Loading

simp4t7 commented Oct 10, 2025 •

edited

Loading

Kobzol left a comment •

edited by rustbot

Loading