Repository: iterative/dvc
Pull request: dvc: hooks only act in branches where dvc active
In a branch where DVC hasn't been initialized (as detected by `git ls-files .dvc` not listing any files), don't invoke dvc in the git hooks. This avoids trouble when switching between branches with and without dvc initialized.

Fixes #2208

* [x] Have you followed the guidelines in our
      [Contributing document](https://dvc.org/doc/user-guide/contributing)?

* [x] Does your PR affect documented changes or does it add new functionality
      that should be documented? If yes, have you created a PR for
      [dvc.org](https://github.com/iterative/dvc.org) documenting it or at
      least opened an issue for it? If so, please add a link to it.

Doc issue link: https://github.com/iterative/dvc.org/issues/506

Problem statement:
post-checkout hook breaks checkout and rebase when adopting DVC in a branch
When adopting DVC in an existing repository with PR code-review workflow, it will be landing in a branch that isn't master. This branch then has problematic interactions where work continues on master before the dvc adoption lands there.

Specifically, once you've done `dvc init`, committed that, and then `dvc install`, the post-checkout hook breaks `git checkout` and `git rebase` between the master branch and the adopting-dvc branch.

To reproduce:

```bash
mkdir dvc-test
cd dvc-test
git init
echo "Testing" > README.md
git add README.md
git commit -m "Initial commit"
git checkout -b start-using-dvc
dvc init
git commit -m "dvc init"
dvc install
```

I go on experimenting with dvc, but then I have to pause the dvc adoption to do some work on the master branch.:

```bash
git stash save "experimenting with dvc"
git checkout master
```

This last command succeeds but outputs:

```
Switched to branch 'master'
Adding '.dvc/state' to '.dvc/.gitignore'.
Adding '.dvc/lock' to '.dvc/.gitignore'.
Adding '.dvc/config.local' to '.dvc/.gitignore'.
Adding '.dvc/updater' to '.dvc/.gitignore'.
Adding '.dvc/updater.lock' to '.dvc/.gitignore'.
Adding '.dvc/repos' to '.dvc/.gitignore'.
Adding '.dvc/state-journal' to '.dvc/.gitignore'.
Adding '.dvc/state-wal' to '.dvc/.gitignore'.
Adding '.dvc/cache' to '.dvc/.gitignore'.
```

... uh-oh. I'm on master where dvc hasn't been taken into use, so I (or my tooling) shouldn't be putting anything in `.dvc`.

I continue with the master work and then return to DVC:

```bash
echo "Progress" > README.md
git commit README.md -m "Progress"
git checkout start-using-dvc
```

That last command fails with:

```
error: The following untracked working tree files would be overwritten by checkout:
	.dvc/.gitignore
Please move or remove them before you switch branches.
Aborting
```

... which is not normal in git; with a clean working tree, `git checkout` to switch branches should always work.

I workaround this and want to keep going, rebasing my dvc experiment branch on current tip of master:

```bash
rm .dvc/.gitignore
git checkout start-using-dvc
git rebase master
```

That rebase fails with:

```
First, rewinding head to replay your work on top of it...
Adding '.dvc/state' to '.dvc/.gitignore'.
Adding '.dvc/lock' to '.dvc/.gitignore'.
Adding '.dvc/config.local' to '.dvc/.gitignore'.
Adding '.dvc/updater' to '.dvc/.gitignore'.
Adding '.dvc/updater.lock' to '.dvc/.gitignore'.
Adding '.dvc/repos' to '.dvc/.gitignore'.
Adding '.dvc/state-journal' to '.dvc/.gitignore'.
Adding '.dvc/state-wal' to '.dvc/.gitignore'.
Adding '.dvc/cache' to '.dvc/.gitignore'.
Applying: dvc init
Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
error: The following untracked working tree files would be overwritten by merge:
	.dvc/.gitignore
Please move or remove them before you merge.
Aborting
error: Failed to merge in the changes.
Patch failed at 0001 dvc init
hint: Use 'git am --show-current-patch' to see the failed patch
Resolve all conflicts manually, mark them as resolved with
"git add/rm <conflicted_files>", then run "git rebase --continue".
You can instead skip this commit: run "git rebase --skip".
To abort and get back to the state before "git rebase", run "git rebase --abort".
```

Deleting the post-checkout hook makes the rebase work just fine:

```bash
rm .git/hooks/post-checkout
git rebase master
```

So probably the post-checkout hook should abort with an informative message if you're on a branch where dvc _hasn't_ been adopted. Not sure whether that check is as simple as checking existence of `.dvc` or whether that breaks other legitimate uses of the hook.

My `dvc --version` is 0.50.1, installed with homebrew (`brew cask`).

Hints from issue discussion:
> Not sure whether that check is as simple as checking existence of .dvc

Hm, probably not that, because git-ignored stuff under `.dvc` _should_ stick around even when you checkout a branch outside of dvc control. But maybe `git ls-files  -- .dvc`?
With very minimal testing this post-checkout hook seems to fix at least my immediate problem:

```
#!/bin/sh
LS_FILES=`git ls-files .dvc`
[ "$LS_FILES" = "" ] || exec dvc checkout
```
Same thing happens on `git commit` in different branches. So `pre-commit` hook should also be conditional on this check. Maybe the `pre-push` hook too?
@gthb dvc detects that this is a dvc repository by finding `.dvc` directory. When switching branches, you have some gitignored(in dvc branch) files there, so git leaves them under `.dvc`, and so dvc still detects your non-dvc branch as dvc branch. I don't see any good solution to handle that, except maybe modifying your git-hooks to only run on dvc branch. E.g. `.git/hooks/post-checkout`
```
#!/bin/sh                                                        
                                               
if [ "$(git rev-parse --abrev-ref HEAD)" == "my-dvc-branch" ]; then     
    exec dvc checkout                                            
fi
```
@gthb Oops, sorry, your later messages didn't show up for me for some reason. I like the `ls-files` trick a lot! Very neat! :slightly_smiling_face: Let's think if we could always include it as a header to our git hooks. I can only see a possible problem when you didn't git commit your .dvc files yet, but even that should not be a problem, since you usually git ocmmit that stuff right after `dvc init`. What do you think?
Actually `git ls-files` by default shows files that are in the index (added but not yet committed), and `dvc init` adds the new files to the index, so these hooks will work directly after `dvc init`.

```
$ dvc init
[...]
$ git ls-files .dvc
.dvc/.gitignore
.dvc/config
$ git reset
$ git ls-files .dvc
$
```

But yeah, instructions should probably point out that (with this `ls-files` change) the hooks installed by `dvc install` only act in a “dvc-enabled” branch, meaning one that has tracked files under `.dvc`, and initializing dvc on a branch doesn't affect other branches until the change gets merged to those branches. (So you can safely try it out in a branch before merging to master, for instance.)
@gthb Would you like to contribute a patch for this? :slightly_smiling_face:  You'd simply need to add that to https://github.com/iterative/dvc/blob/0.50.1/dvc/scm/git/__init__.py#L230 .
@gthb Not sure if the message got lost or if I should take this as "no" :slightly_smiling_face: 
Ah, whoops, sorry, you should take it as “away on vacation and not paying
much attention” :) ... I can whip up a simple PR when I'm back home next
weekend.

On Mon, Jul 22, 2019, 23:31 Ruslan Kuprieiev <notifications@github.com>
wrote:

> @gthb <https://github.com/gthb> Not sure if the message got lost or if I
> should take this as "no" :)
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <https://github.com/iterative/dvc/issues/2208?email_source=notifications&email_token=AABFP3HVQOQAZ665CT3GTWTQAY7M5A5CNFSM4H4GTSKKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD2ROSZQ#issuecomment-513993062>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/AABFP3HERHVVK3TS5E7DQG3QAY7M5ANCNFSM4H4GTSKA>
> .
>

@gthb We would really appreciate that :slightly_smiling_face: Have a great vacation!

Commit message: dvc: hooks only act in branches where dvc active

In a branch where DVC hasn't been initialized (as detected by
`git ls-files .dvc` not listing any files), don't invoke dvc in the git hooks.
This avoids trouble when switching between branches with and without dvc
initialized.

Fixes #2208

Patch to review:
diff --git a/dvc/scm/git/__init__.py b/dvc/scm/git/__init__.py
index c0592e896b..7ed1a56892 100644
--- a/dvc/scm/git/__init__.py
+++ b/dvc/scm/git/__init__.py
@@ -217,17 +217,28 @@ def list_tags(self):
 
     def _install_hook(self, name, cmd):
         command = "dvc {}".format(cmd)
+        dvc_present_check = """
+LS_FILES=`git ls-files .dvc`
+[ "$LS_FILES" = "" ] ||
+        """.strip()
 
         hook = os.path.join(self.root_dir, self.GIT_DIR, "hooks", name)
 
         if os.path.isfile(hook):
             with open(hook, "r+") as fobj:
                 if command not in fobj.read():
-                    fobj.write("exec {command}\n".format(command=command))
+                    fobj.write(
+                        "{check} exec {command}\n".format(
+                            check=dvc_present_check, command=command
+                        )
+                    )
         else:
             with open(hook, "w+") as fobj:
                 fobj.write(
-                    "#!/bin/sh\n" "exec {command}\n".format(command=command)
+                    "#!/bin/sh\n"
+                    "{check} exec {command}\n".format(
+                        check=dvc_present_check, command=command
+                    )
                 )
 
         os.chmod(hook, 0o777)
diff --git a/tests/func/test_install.py b/tests/func/test_install.py
index 5e5efee29b..c06b6830d8 100644
--- a/tests/func/test_install.py
+++ b/tests/func/test_install.py
@@ -36,7 +36,12 @@ def test_should_append_hooks_if_file_already_exists(self, git, dvc_repo):
 
         assert main(["install"]) == 0
 
-        expected_script = "#!/bin/sh\n" "echo hello\n" "exec dvc checkout\n"
+        expected_script = (
+            "#!/bin/sh\n"
+            "echo hello\n"
+            "LS_FILES=`git ls-files .dvc`\n"
+            '[ "$LS_FILES" = "" ] || exec dvc checkout\n'
+        )
 
         with open(self._hook("post-checkout"), "r") as fobj:
             assert fobj.read() == expected_script
@@ -45,7 +50,11 @@ def test_should_be_idempotent(self, git, dvc_repo):
         assert main(["install"]) == 0
         assert main(["install"]) == 0
 
-        expected_script = "#!/bin/sh\n" "exec dvc checkout\n"
+        expected_script = (
+            "#!/bin/sh\n"
+            "LS_FILES=`git ls-files .dvc`\n"
+            '[ "$LS_FILES" = "" ] || exec dvc checkout\n'
+        )
 
         with open(self._hook("post-checkout"), "r") as fobj:
             assert fobj.read() == expected_script


Code review comments:
diff_hunk: @@ -217,17 +217,28 @@ def list_tags(self):
 
     def _install_hook(self, name, cmd):
         command = "dvc {}".format(cmd)
path: dvc/scm/git/__init__.py
text: @user1:
We could to it like this and avoid dvc_present_check and fobj.write() additions down below :) 
```suggestion
         command = "[ -z "$(git ls-files .dvc)" ] && exit 0\n"
         command += "dvc {}".format(cmd)
```

@author:
Done, except:

* make it a oneliner with `||` to avoid the `exit 0`, because that's unfriendly to others who might append to the hook script (like dvc itself does if the script already exists)
* the `fobj.write()` call still needed to be changed because of the `exec`