Support running pre-commit hook against staged files

[calebcartwright]

No description provided.

[mainrs]

Would this include the possibility to run cargo fmt on all changed files? Or does a simple pre-commit = "cargo fmt" already work. Is that a good practise to do?

[calebcartwright]

hi @sirwindfield 👋

The change described in this issue is really about the behavior and context that rusty-hook uses when running the configured pre-commit hook script/command specified in the rusty-hook config file (regardless of what that script actually is). This is not about only running rustfmt against staged files.

Right now the commands you specify in your rusty-hook config file are executed against the contents currently on the file system (not what's been staged).

This is fine for most hooks, but for the pre-commit hook it creates a scenario where a user could potentially still commit changes that would fail the pre-commit hook.

This only happens when a user stages changes that would fail the hook (for example if your pre-commit hook is running rustfmt, then staging changes with bad formatting), but then fixes the files on disk and commits, without staging the fixes.

Contrived example:

$ cat src/lib.rs
fn foo(     ) {   }
$ git add lib.rs
$ echo -e "fn foo() {}" > src/lib.rs
$ git commit -m "oops"

This would pass the pre-commit hook, because rustfmt would be executed with the fixed src/lib.rs on disk, but the original/misformatted src/lib.rs version would be committed. The same type of situation could occur with other pre-commit hooks too (clippy, cargo test | build | check, etc.)

What we'd like to have rusty-hook do is prevent this scenario from being possible by running the configured pre-commit hook command/script with the staged changes in context instead of latest on disk.

[calebcartwright]

As it relates to rustfmt in general: rusty-hook isn't affiliated with rustfmt in any capacity, but I also do some work over on rustfmt so will share some rustfmt-specific info and personal preferences.

An important thing to note with v1.x of rustfmt is that it formats entire projects/crates by default.

When running rustfmt directly, one or more input files are provided, this is usually the main entry point like a lib.rs or main.rs. rustfmt then parses that file and recursively generates the complete AST (using the rustc parser), and then formats all the associated files in the workspace.

The cargo fmt subcommand is basically a wrapper around rustfmt that automatically detects a lot of inputs by querying the workspace, and those detected inputs are then passed as args when it invokes rustfmt (examples include the target edition, entry file for each target for each crate in the workspace, etc.). This makes it easier for users to run rustfmt, particularly in larger workspaces.

Neither rustfmt nor the cargo fmt subcommand are "git aware", so they work with all the files in the project (not just the ones that were "changed" since the last commit).

So if you use cargo fmt as your configured hook with rusty-hook:

[hooks]
pre-commit = "cargo fmt -- --check"

cargo fmt is always going to format all the files in your project. it's just that right now with rusty-hook, cargo fmt would be working with the current on-disk contents, and some of those may be different versions than what was staged.

Personally, if I intend to use rustfmt to maintain consistent formatting in my project, I like to enable that up front along with a CI check that will completely prevent misformatted code from sneaking in to the main branch. I also like to use rusty-hook alongside that so that I can catch changes on my local machine and not waste CI time 😄

Trying to retroactively add rustfmt to an existing project is a bit trickier, especially if the project is large and/or highly active and/or has a lot of code rustfmt would change. The best approach for this IMO is to incrementally adopt rustfmt within the codebase by leveraging some of rustfmt's config options (like ignore).

The ignore setting can be used to tell rustfmt to completely ignore specific files or even directories so allows for selectively formatting certain files/directories. This is the approach that was used when the main rust-lang/rust repo started using rustfmt on the compiler tree.

[lroolle]

I found this works for me:

[hooks]
pre-commit = "cargo +nightly fmt -- --check; cargo +nightly fmt"

The check fails, then, re-run fmt, then the committer should re-add the formatted files;

[xamgore]

I have the following piece of code to run migrations, only when there are new/changed files in there:

# If there are any staged changes in `migrations/`,
# regenerate models to check whether we forgot to generate them.
git diff --cached --quiet -- "migrations/**" || ./shortcuts/generate.sh