Dvc: hooks: dvc appends hooks and breaks the existing hook

[shcheklein]

If git hook is a python file users end up with a broken hook - a mix of python and bash code. We need to find a way to mitigate this.

[efiop]

We originally didn't account for a hook being anything than a shell script and so we were just appending to it. I guess we could check the shebang to verify that it is #!/bin/sh(or a variation of it) and if it is - append, if not - throw an error or a warning(or a prompt?). That would be some pretty nasty heuristics, but I see no other way of solving this. Also, we could do the next step with the heuristics and append to pre-commit config file :slightly_smiling_face: But that also sounds as a pretty awful magic :sparkles: :sparkles: :sparkles:

[shcheklein]

it's really annoying that Git hooks are implemented this way. I think we should stop appending if there is the is a file already - it's not nice that the top level command dvc install breaks the environment for users. Or at least ask a confirmation with some explanation of what's going on.

Are there some general frameworks that can be used to install hooks? Something like Husky, but not JS-specific?

[efiop]

@shcheklein

it's really annoying that Git hooks are implemented this way. I think we should stop appending if there is the is a file already - it's not nice that the top level command dvc install breaks the environment for users. Or at least ask a confirmation with some explanation of what's going on.

We've actually added that append functionality not that long ago in https://github.com/iterative/dvc/issues/1964 , before that we just errored-out if the hook existed already.

Are there some general frameworks that can be used to install hooks? Something like Husky, but not JS-specific?

Looks like it is pre-commit after all. But in any case, different projects might have different git hooks frameworks and it doesn't look like we would be able to suit them all. We could start detecting and supporting those one-by-one, but that would be complicated, and it feels like we could simply write the instructions for now, that would tell people to use our commands in their hooks.

[shcheklein]

@efiop looks like the solution was wrong even for that ticket. Original author of that ticket has the problem with dvc breaking the existing hook, which makes experience even worse.

[ghost]

@shcheklein , what about changing the dvc install command (which edits the hook with some heuristics as @efiop noted above) to dvc hook that will output the content to be executed in a script.

For example, hub offers a similar interface: hub alias -s that outputs alias git=hub, and the intended use is to append eval "$(hub alias -s)" to a .*rc file.

With this interface, users can use dvc hook post-checkout > .git/hooks/checkout or dvc hook post-checkout >> .git/hooks/checkout or let the users inspect the dvc hook itself and adapt it to their existent script.

What I mean, is to delegate the installation to the users and provide the script itself

[shcheklein]

@mroutis I like the idea of delegating to the user and for the command to output the content. May be we can add an option to write directly to the file. We can also add some options to output it in a few languages - --python, --shell, etc

I'm not sure about the naming - dvc hook can conflict in the future with DVC hooks. Even though I understand that dvc install looks a bit weird - a global command just to manipulate hooks feels too much.

[AlJohri]

I just ran into this as well. I was looking to use the black autoformatter pre-commit hook along with dvc. Is it possible to support .pre-commit-config.yaml directly? Seems like it can support multiple hook types.

[efiop]

@AlJohri It does support multiple hooks, but not all :( Currently it supports pre-commit,pre-push,prepare-commit-msg,commit-msg, and we need post-checkout in addition to that. We could ask pre-commit guys to implement post-checkout so we are covered and/or temporarily use a hybrid solution.

[efiop]

First step is to detect if git hook already exists and then refuse to install our hook into it, to avoid nasty situations.

[efiop]

Let's first check that no hooks that we want to install exist. If they do - refuse to install anything (and refer to a documentation). If they don't exist - install.