Repository: gitpython-developers/GitPython
Pull request #947 (2019-10-21 00:45:01)

Fix how Diff handles commits that contain submodule changes
This PR addresses the issue reported in #891

The problem was that `Diff` was receiving the parent `Repo` but commit hashes from the submodule repo, resulting in `Diff` being unable to find the commits. This was due to the output of `git diff-tree <parent_commit_1> <parent_commit_2>` containing the hashes from the submodule:
```
$ git diff-tree 53a7920fe21c49edb4471978361c1f23bf4034b1 d4f583c621da537db510ee19b2198d2192df84a9
:160000 160000 8324c24a8c6d60bd83a5356e81e5278e54ef49fc 5ac7216dccc0c1352d53a17efbc5b3bfafc0fd46 M	sub
```
This implementation fixes that issue by comparing the `a_rawpath` to all of the repo's submodule `path`s and overwriting `repo` to the submodule's repo if there's a match. If there are ideas concerning a more efficient way to identify this situation I'm more than happy to entertain other approaches.

The test added by this PR is based off of the repro provided in #891. While it's a great repro case I don't feel like it's a particularly elegant test so I'm open to suggestions there as well.

Fixed a typo in a comment in `cmd.py` and resolved a deprecation warning within the `test_diff.py` file while I was in there. I also added `git submodule update --init --recursive`  to `init-tests-after-clone.sh` because there are a decent number of tests that fail without `gitdb` and `smmap` cloned.

codecov-io: # [Codecov](https://codecov.io/gh/gitpython-developers/GitPython/pull/947?src=pr&el=h1) Report
> Merging [#947](https://codecov.io/gh/gitpython-developers/GitPython/pull/947?src=pr&el=desc) into [master](https://codecov.io/gh/gitpython-developers/GitPython/commit/6fd090293792884f5a0d05f69109da1c970c3cab?src=pr&el=desc) will **increase** coverage by `0.03%`.
> The diff coverage is `100%`.

[![Impacted file tree graph](https://codecov.io/gh/gitpython-developers/GitPython/pull/947/graphs/tree.svg?width=650&token=mTmKwYbezP&height=150&src=pr)](https://codecov.io/gh/gitpython-developers/GitPython/pull/947?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master     #947      +/-   ##
==========================================
+ Coverage   93.65%   93.68%   +0.03%     
==========================================
  Files          59       59              
  Lines        9851     9887      +36     
==========================================
+ Hits         9226     9263      +37     
+ Misses        625      624       -1
```


| [Impacted Files](https://codecov.io/gh/gitpython-developers/GitPython/pull/947?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [git/cmd.py](https://codecov.io/gh/gitpython-developers/GitPython/pull/947/diff?src=pr&el=tree#diff-Z2l0L2NtZC5weQ==) | `82.94% <ø> (ø)` | :arrow_up: |
| [git/test/test\_diff.py](https://codecov.io/gh/gitpython-developers/GitPython/pull/947/diff?src=pr&el=tree#diff-Z2l0L3Rlc3QvdGVzdF9kaWZmLnB5) | `100% <100%> (ø)` | :arrow_up: |
| [git/diff.py](https://codecov.io/gh/gitpython-developers/GitPython/pull/947/diff?src=pr&el=tree#diff-Z2l0L2RpZmYucHk=) | `98.03% <100%> (+0.03%)` | :arrow_up: |
| [git/objects/submodule/base.py](https://codecov.io/gh/gitpython-developers/GitPython/pull/947/diff?src=pr&el=tree#diff-Z2l0L29iamVjdHMvc3VibW9kdWxlL2Jhc2UucHk=) | `93.25% <0%> (+0.16%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/gitpython-developers/GitPython/pull/947?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/gitpython-developers/GitPython/pull/947?src=pr&el=footer). Last update [6fd0902...dd546a1](https://codecov.io/gh/gitpython-developers/GitPython/pull/947?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).


Patch:
diff --git a/git/cmd.py b/git/cmd.py
index 78319c75..263c8ba7 100644
--- a/git/cmd.py
+++ b/git/cmd.py
@@ -67,7 +67,7 @@ __all__ = ('Git',)
 
 def handle_process_output(process, stdout_handler, stderr_handler,
                           finalizer=None, decode_streams=True):
-    """Registers for notifications to lean that process output is ready to read, and dispatches lines to
+    """Registers for notifications to learn that process output is ready to read, and dispatches lines to
     the respective line handlers.
     This function returns once the finalizer returns
 
diff --git a/git/diff.py b/git/diff.py
index 0150d675..3dbe0866 100644
--- a/git/diff.py
+++ b/git/diff.py
@@ -278,6 +278,14 @@ class Diff(object):
         if self.b_mode:
             self.b_mode = mode_str_to_int(self.b_mode)
 
+        # Determine whether this diff references a submodule, if it does then
+        # we need to overwrite "repo" to the corresponding submodule's repo instead
+        if repo and a_rawpath:
+            for submodule in repo.submodules:
+                if submodule.path == a_rawpath.decode("utf-8"):
+                    repo = submodule.module()
+                    break
+
         if a_blob_id is None or a_blob_id == self.NULL_HEX_SHA:
             self.a_blob = None
         else:


Test patch:
diff --git a/git/test/test_diff.py b/git/test/test_diff.py
index e1b345b5..56e51289 100644
--- a/git/test/test_diff.py
+++ b/git/test/test_diff.py
@@ -5,12 +5,15 @@
 # This module is part of GitPython and is released under
 # the BSD License: http://www.opensource.org/licenses/bsd-license.php
 import ddt
+import shutil
+import tempfile
 from git import (
     Repo,
     GitCommandError,
     Diff,
     DiffIndex,
     NULL_TREE,
+    Submodule,
 )
 from git.cmd import Git
 from git.test.lib import (
@@ -19,7 +22,6 @@ from git.test.lib import (
     fixture,
     assert_equal,
     assert_true,
-
 )
 from git.test.lib import with_rw_directory
 
@@ -29,9 +31,15 @@ import os.path as osp
 @ddt.ddt
 class TestDiff(TestBase):
 
+    def setUp(self):
+        self.repo_dir = tempfile.mkdtemp()
+        self.submodule_dir = tempfile.mkdtemp()
+
     def tearDown(self):
         import gc
         gc.collect()
+        shutil.rmtree(self.repo_dir)
+        shutil.rmtree(self.submodule_dir)
 
     def _assert_diff_format(self, diffs):
         # verify that the format of the diff is sane
@@ -68,7 +76,8 @@ class TestDiff(TestBase):
         r.git.commit(all=True, message="change on topic branch")
 
         # there must be a merge-conflict
-        self.failUnlessRaises(GitCommandError, r.git.cherry_pick, 'master')
+        with self.assertRaises(GitCommandError):
+            r.git.cherry_pick('master')
 
         # Now do the actual testing - this should just work
         self.assertEqual(len(r.index.diff(None)), 2)
@@ -267,6 +276,43 @@ class TestDiff(TestBase):
         self.assertIsNone(diff_index[0].a_path, repr(diff_index[0].a_path))
         self.assertEqual(diff_index[0].b_path, u'file with spaces', repr(diff_index[0].b_path))
 
+    def test_diff_submodule(self):
+        """Test that diff is able to correctly diff commits that cover submodule changes"""
+        # Init a temp git repo that will be referenced as a submodule
+        sub = Repo.init(self.submodule_dir)
+        with open(self.submodule_dir + "/subfile", "w") as sub_subfile:
+            sub_subfile.write("")
+        sub.index.add(["subfile"])
+        sub.index.commit("first commit")
+
+        # Init a temp git repo that will incorporate the submodule
+        repo = Repo.init(self.repo_dir)
+        with open(self.repo_dir + "/test", "w") as foo_test:
+            foo_test.write("")
+        repo.index.add(['test'])
+        Submodule.add(repo, "subtest", "sub", url="file://" + self.submodule_dir)
+        repo.index.commit("first commit")
+        repo.create_tag('1')
+
+        # Add a commit to the submodule
+        submodule = repo.submodule('subtest')
+        with open(self.repo_dir + "/sub/subfile", "w") as foo_sub_subfile:
+            foo_sub_subfile.write("blub")
+        submodule.module().index.add(["subfile"])
+        submodule.module().index.commit("changed subfile")
+        submodule.binsha = submodule.module().head.commit.binsha
+
+        # Commit submodule updates in parent repo
+        repo.index.add([submodule])
+        repo.index.commit("submodule changed")
+        repo.create_tag('2')
+
+        diff = repo.commit('1').diff(repo.commit('2'))[0]
+        # If diff is unable to find the commit hashes (looks in wrong repo) the *_blob.size
+        # property will be a string containing exception text, an int indicates success
+        self.assertIsInstance(diff.a_blob.size, int)
+        self.assertIsInstance(diff.b_blob.size, int)
+
     def test_diff_interface(self):
         # test a few variations of the main diff routine
         assertion_map = {}
diff --git a/init-tests-after-clone.sh b/init-tests-after-clone.sh
index 0d445891..e852f3cd 100755
--- a/init-tests-after-clone.sh
+++ b/init-tests-after-clone.sh
@@ -12,4 +12,5 @@ git checkout master || git checkout -b master
 git reset --hard HEAD~1
 git reset --hard HEAD~1
 git reset --hard HEAD~1
-git reset --hard __testing_point__
\ No newline at end of file
+git reset --hard __testing_point__
+git submodule update --init --recursive
\ No newline at end of file