Different behavior on missing config file

[calebcartwright]

Description

Currently, when the config file is missing, rusty-hook exits with an error code resulting in all the git hooks being blocked/rejected (which prevents a user from being able to commit, or requires them to include the -n or --no-verify flags).

We could update the generated hook script files to handle the "no config file" error scenario differently than other errors so that the git hooks are not rejected.

Value

This would allow for folks to partially remove rusty-hook from their project (only remove config file and dev-dep), but leave the rusty-hook scripts in their hooks directory in such a way that would not cause the git hooks to continue to be failed/rejected.

Still a few things to think through: 1) Should this be the standard/default script behavior, or an init option to let users decide for their own projects? 2) On partial removal, when a config file is missing, should the hooks continue silently or display a warning/message to the user before continuing?

IMO it's not ideal to keep a set of git hook scripts that are triggered on every git hook, running rusty-hook, checking for a config file, and then failing silently. I'd vote that we make this the new default behavior for the "no config file" scenario, and display a warning to the user on the pre-commit hook (not every hook) so that they know they should remove the script files too.

CC @traviskosarek @beverts312

[EverlastingBugstopper]

I definitely think that removal of the config file should not stop normal git flow, a warning (perhaps with instructions to run the new shiny rusty-hook remove command) and allow commits to flow freely w/o a config file makes the most sense to me. My reasoning for this is that from the user’s perspective, once their config file is gone, they don’t want any hooks to be run. My guess is your average user isn’t trying to dig into their .git/hooks folder + wants rusty-hook to take care of everything for them. It follows then that removing their configuration file should remove all hook behavior.

[traviskosarek]

My various opinions here...

Regardless of how the configuration file becomes missing, I think having an error that signals to the user as to why the git hooks are failing is definitely warranted. I can see it being frustrating that things stop working with no specific reasoning given and any help we can provide the user to help them solve their issue makes sense to have.

That said, I think the default behavior for a missing configuration file should simply be a message indicating that the "rusty-hook.toml file is missing" and the process should continue to correctly fail. Specifically, I don't think this use case warrants building in user customization for a single default value.

As far as the removal of rusty-hook, I do think having a documented section/subsection in the README on "how to remove rusty-hook" should cover this scenario well enough for now. While having a command built into the CLI to remove any generated files is neat, it may be overkill when the solution really comes down to "delete all these files" vs "delete this one file"

To that point though, it may also be helpful to make it more clear in the documentation that "both a configuration file and other files/scripts are generated" so that in a "removal" section it is better understood that it is not only the configuration file that needs to be removed.

@calebcartwright Thoughts?

[calebcartwright]

Thanks Travis! The core question here is the behavior we want for the git hooks themselves.

Currently, the hook scripts fail when there's any non-zero return code from rusty-hook which completely exits the git workflow. There's no differentiation between missing config, the configured command (test, lint, etc.) failing, etc. The proposal here is to update the the git hooks to handle the non-zero return codes a bit more intelligently, specifically to handle the "no config file" in such a way that the git hooks are no longer going to fail out when the config file is missing.

It sounds like you are voting against that proposal. Do I have that correct @traviskosarek?

[calebcartwright]

Also

As far as the removal of rusty-hook, I do think having a documented section/subsection in the README on "how to remove rusty-hook" should cover this scenario well enough for now. While having a command built into the CLI to remove any generated files is neat, it may be overkill when the solution really comes down to "delete all these files" vs "delete this one file"

To that point though, it may also be helpful to make it more clear in the documentation that "both a configuration file and other files/scripts are generated" so that in a "removal" section it is better understood that it is not only the configuration file that needs to be removed.

Agreed, see #51

While having a command built into the CLI to remove any generated files is neat, it may be overkill

Disagree with this part, but a little off topic for this issue. Let's discuss the remove command in #5

[traviskosarek]

Ahh I didn't mean to vote against the proposed change. I like following the pattern that husky has set, although I think it is better to give the user a message versus failing silently. Based on the PR #53, I am definitely good with this solution.

[calebcartwright]

@EverlastingBugstopper - we've completely revamped the behavior when the config file does not exist. I believe this makes for a much better experience, so want to thank you again for bringing this to our attention.

If you're interested and willing to test this update (no worries if not!), we'd really appreciate your feedback on this new behavior. To test the new version:

(since you likely have the older version on your system)
cargo install rusty-hook --force 

Then in a new/sample project, either run rusty-hook init or add rusty-hook = "0.9.0" as a dev dependency and run cargo test. Finally, delete the generated .rusty-hook.toml file and then run a git commit.