From eae432818a72de3b5ae31414de3cf2c5a3139d49 Mon Sep 17 00:00:00 2001
From: Ruslan Kuprieiev <kupruser@gmail.com>
Date: Wed, 18 Mar 2020 19:18:58 +0200
Subject: [PATCH] git: tree: use item.path instead of item.name (#3505)

We are currently using this [0] hack to work around the fact that
item.name
is not always a basename. Even though `IndexObject` has it defined that
way
[1], but `Submodule` - doesn't [2]. So using `item.name` might cause
issues
such as [3], because GitPython doesn't pass name parameter when simply
going through objects [4]. In other places, when working specifically
with
submodules, it sets `_name` attribute explicitly [5].

Note that we still don't walk into submodules when walking through a git
repo. Might want to reconsider that later as for now we haven't heard
anything about it from actual users.

Fixes #3481

[0] https://github.com/iterative/dvc/blob/0.90.0/dvc/scm/git/tree.py#L15
[1]
https://github.com/gitpython-developers/GitPython/blob/3.1.0/git/objects/base.py#L170
[2]
https://github.com/gitpython-developers/GitPython/blob/3.1.0/git/objects/submodule/base.py#L1123
[3] https://github.com/gitpython-developers/GitPython/issues/597
[4]
https://github.com/gitpython-developers/GitPython/blob/3.1.0/git/objects/tree.py#L237
[5]
https://github.com/gitpython-developers/GitPython/blob/3.1.0/git/objects/submodule/base.py#L357
---
 dvc/scm/git/tree.py        | 31 ++++++++++++-------------------
 tests/unit/scm/test_git.py | 22 ++++++++++++++++++++++
 2 files changed, 34 insertions(+), 19 deletions(-)

diff --git a/dvc/scm/git/tree.py b/dvc/scm/git/tree.py
index 7933dee72..bd42d6ca7 100644
--- a/dvc/scm/git/tree.py
+++ b/dvc/scm/git/tree.py
@@ -12,19 +12,11 @@ GIT_MODE_DIR = 0o40000
 GIT_MODE_FILE = 0o644
 
 
-def _iter_tree(tree):
-    # During usual tree iteration with ``for .. in ..`` GitPython returns each
-    # submodule instance without correctly filled ``name`` property. It raises
-    # an exception during accessing such submodule ``name`` later.
-    # The same time repo's submodules property contains correctly initialized
-    # submodules list.
-    # Here we just replace submodule instance from iterator with the
-    # corresponding instance from ``repo.submodules.``
-    submodules = {x.hexsha: x for x in tree.repo.submodules}
-    for node in tree:
-        if node.type == "submodule":
-            node = submodules[node.hexsha]
-        yield node
+def _item_basename(item):
+    # NOTE: `item.name` is not always a basename. See [1] for more details.
+    #
+    # [1] https://github.com/iterative/dvc/issues/3481#issuecomment-600693884
+    return os.path.basename(item.path)
 
 
 class GitTree(BaseTree):
@@ -87,8 +79,8 @@ class GitTree(BaseTree):
             return False
         # see https://github.com/gitpython-developers/GitPython/issues/851
         # `return (i in tree)` doesn't work so here is a workaround:
-        for i in _iter_tree(obj):
-            if i.name == path:
+        for item in obj:
+            if _item_basename(item) == path:
                 return True
         return False
 
@@ -118,11 +110,12 @@ class GitTree(BaseTree):
 
     def _walk(self, tree, topdown=True):
         dirs, nondirs = [], []
-        for i in _iter_tree(tree):
-            if i.mode == GIT_MODE_DIR:
-                dirs.append(i.name)
+        for item in tree:
+            name = _item_basename(item)
+            if item.mode == GIT_MODE_DIR:
+                dirs.append(name)
             else:
-                nondirs.append(i.name)
+                nondirs.append(name)
 
         if topdown:
             yield os.path.normpath(tree.abspath), dirs, nondirs
diff --git a/tests/unit/scm/test_git.py b/tests/unit/scm/test_git.py
index 2874da16b..737c4e52e 100644
--- a/tests/unit/scm/test_git.py
+++ b/tests/unit/scm/test_git.py
@@ -1,5 +1,7 @@
 import os
 
+from dvc.compat import fspath
+
 from tests.basic_env import TestDvcGit
 
 
@@ -15,3 +17,23 @@ class TestGit(TestDvcGit):
     def test_belongs_to_scm_false(self):
         path = os.path.join("some", "non-.git", "file")
         self.assertFalse(self.dvc.scm.belongs_to_scm(path))
+
+
+def test_walk_with_submodules(tmp_dir, scm, git_dir):
+    git_dir.scm_gen(
+        {"foo": "foo", "bar": "bar", "dir": {"data": "data"}},
+        commit="add dir and files",
+    )
+    scm.repo.create_submodule("submodule", "submodule", url=fspath(git_dir))
+    scm.commit("added submodule")
+
+    files = []
+    dirs = []
+    tree = scm.get_tree("HEAD")
+    for _, dnames, fnames in tree.walk("."):
+        dirs.extend(dnames)
+        files.extend(fnames)
+
+    # currently we don't walk through submodules
+    assert not dirs
+    assert set(files) == {".gitmodules", "submodule"}