Open calebcartwright opened 5 years ago
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.
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.
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;
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
Would this include the possibility to run
cargo fmt
on all changed files? Or does a simplepre-commit = "cargo fmt"
already work. Is that a good practise to do?