swellaby / rusty-hook

git hook manager, geared toward Rust projects
MIT License
207 stars 11 forks source link

Don’t corrupt ./.git/hooks #50

Closed EverlastingBugstopper closed 5 years ago

EverlastingBugstopper commented 5 years ago

Description

If rusty-hooks is removed, it will corrupt your ./.git/hooks because there is no longer a .toml file w/hooks. This results in a cryptic “Failed to locate or parse config file” message after removal

Value

The output of rusty-hook should not rely on the existence of artifacts generated by rusty-hook, if rusty-hook is removed from a project, it should not screw up the project.

calebcartwright commented 5 years ago

Hi @EverlastingBugstopper - thanks for reaching out!

If I'm following correctly, you were using rusty-hook at some point but then decided to remove it from your project (sorry to hear that!). I assume that "removing" rusty-hook means that you deleted the .toml file from your repo and then removed rusty-hook from your dev-dependencies and/or ran a cargo uninstall rusty-hook.

However, the various hook files that rusty-hook created in your repo (in the .git/hooks directory) still existed and thus resulted in the referenced error message when the various git hooks were triggered in your local project.

We have plans to add a remove command so that as part of removing rusty-hook, you would be able to run a rusty-hook remove to clean up those generated hooks. For now though, as part of removing rusty-hook from your project, you'll also need to manually delete those hook files rusty-hook created in your .git/hooks directory.

We'll also update our docs to ensure that folks know how to fully remove rusty-hook from a project.

Thanks for giving rusty-hook a try and providing us with feedback!

EverlastingBugstopper commented 5 years ago

Yup, found the fix —> documenting this should be good enough for now, and a remove command is great, but it would be best (in my opinion) if the solution to this allowed for the intuitive removal of the toml file. Perhaps an if statement that simply checks for the existence of the toml before attempting to read from it.

calebcartwright commented 5 years ago

This results in a cryptic “Failed to locate or parse config file” message after removal

Agreed the error message can be more intuitive. Even something as simple as adding a suffix along the lines of If you are trying to remove rusty-hook, you must also remove the hook script files to the error message when the toml file doesn't exist would probably help.

Perhaps an if statement that simply checks for the existence of the toml before attempting to read from it

That's already the current behavior. The problem is that rusty-hook doesn't know why the config file doesn't exist (doesn't know you attempted to remove it). In this scenario, the git hook script files still exist, so they're still getting triggered by git and then still invoking rusty-hook; the only thing missing is the config file.

calebcartwright commented 5 years ago

Thanks again for the feedback @EverlastingBugstopper. I'm going to close this in favor of #51 and #52, if you have any other thoughts or feedback for us please feel free to chime in on those issues.

EverlastingBugstopper commented 5 years ago

So if the behavior is already to check for the existence of the config file and if it doesn’t exist, print an error message, wouldn’t it be better to simply allow git to continue? As it is now if I delete the rusty-hook.toml it completely ruins the git workflow, you can’t commit anything at all. I don’t think that’s the behavior you want. It should just not run anything w/the hook and allow git operations to resume normally.