MissingObjectFinder: fix tags of non-commits

Prior to this change, MissingObjectFinder would explode if a Tag object
pointed at anything other than a commit object. This appears to have
been an unintentional regression in
c1787f51f4eb03a5a4d56d5a34429ae7639d2093. Includes tests for the
following cases:

 * A tag pointing to a commit
 * A tag pointing to a tag pointing to a commit
 * A tag pointing to a tree
 * A tag pointing to a blob
 * A tag pointing to a tag pointing to a blob

Only the first case worked prior to this change.

dulwich/object_store.py

@@ -911,7 +911,7 @@ def _collect_filetree_revs(obj_store, tree_sha, kset):
 
 
 def _split_commits_and_tags(obj_store, lst, ignore_unknown=False):
-    """Split object id list into two list with commit SHA1s and tag SHA1s.
+    """Split object id list into three lists with commit, tag, and other SHAs.
 
     Commits referenced by tags are included into commits
     list as well. Only SHA1s known in this repository will get
@@ -922,10 +922,11 @@ def _split_commits_and_tags(obj_store, lst, ignore_unknown=False):
     :param lst: Collection of commit and tag SHAs
     :param ignore_unknown: True to skip SHA1 missing in the repository
         silently.
-    :return: A tuple of (commits, tags) SHA1s
+    :return: A tuple of (commits, tags, others) SHA1s
     """
     commits = set()
     tags = set()
+    others = set()
     for e in lst:
         try:
             o = obj_store[e]
@@ -937,10 +938,15 @@ def _split_commits_and_tags(obj_store, lst, ignore_unknown=False):
                 commits.add(e)
             elif isinstance(o, Tag):
                 tags.add(e)
-                commits.add(o.object[1])
+                tagged = o.object[1]
+                c, t, o = _split_commits_and_tags(
+                    obj_store, [tagged], ignore_unknown=ignore_unknown)
+                commits |= c
+                tags |= t
+                others |= o
             else:
-                raise KeyError('Not a commit or a tag: %s' % e)
-    return (commits, tags)
+                others.add(e)
+    return (commits, tags, others)
 
 
 class MissingObjectFinder(object):
@@ -966,9 +972,9 @@ class MissingObjectFinder(object):
         # and such SHAs would get filtered out by _split_commits_and_tags,
         # wants shall list only known SHAs, and otherwise
         # _split_commits_and_tags fails with KeyError
-        have_commits, have_tags = (
+        have_commits, have_tags, have_others = (
             _split_commits_and_tags(object_store, haves, True))
-        want_commits, want_tags = (
+        want_commits, want_tags, want_others = (
             _split_commits_and_tags(object_store, wants, False))
         # all_ancestors is a set of commits that shall not be sent
         # (complete repository up to 'haves')
@@ -993,9 +999,11 @@ class MissingObjectFinder(object):
             self.sha_done.add(t)
 
         missing_tags = want_tags.difference(have_tags)
-        # in fact, what we 'want' is commits and tags
+        missing_others = want_others.difference(have_others)
+        # in fact, what we 'want' is commits, tags, and others
         # we've found missing
         wants = missing_commits.union(missing_tags)
+        wants = wants.union(missing_others)
 
         self.objects_to_send = set([(w, None, False) for w in wants])
 

dulwich/tests/test_missing_obj_finder.py

@@ -25,6 +25,7 @@ from dulwich.objects import (
 from dulwich.tests import TestCase
 from dulwich.tests.utils import (
     make_object,
+    make_tag,
     build_commit_graph,
     skipIfPY3,
     )
@@ -194,3 +195,58 @@ class MOFMergeForkRepoTest(MissingObjectFinderTest):
               self.cmt(7).id, self.cmt(6).id, self.cmt(4).id,
               self.cmt(7).tree, self.cmt(6).tree, self.cmt(4).tree,
               self.f1_4_id])
+
+
+class MOFTagsTest(MissingObjectFinderTest):
+    def setUp(self):
+        super(MOFTagsTest, self).setUp()
+        f1_1 = make_object(Blob, data='f1')
+        commit_spec = [[1]]
+        trees = {1: [('f1', f1_1)]}
+        self.commits = build_commit_graph(self.store, commit_spec, trees)
+
+        self._normal_tag = make_tag(self.cmt(1))
+        self.store.add_object(self._normal_tag)
+
+        self._tag_of_tag = make_tag(self._normal_tag)
+        self.store.add_object(self._tag_of_tag)
+
+        self._tag_of_tree = make_tag(self.store[self.cmt(1).tree])
+        self.store.add_object(self._tag_of_tree)
+
+        self._tag_of_blob = make_tag(f1_1)
+        self.store.add_object(self._tag_of_blob)
+
+        self._tag_of_tag_of_blob = make_tag(self._tag_of_blob)
+        self.store.add_object(self._tag_of_tag_of_blob)
+
+        self.f1_1_id = f1_1.id
+
+    def test_tagged_commit(self):
+        # The user already has the tagged commit, all they want is the tag,
+        # so send them only the tag object.
+        self.assertMissingMatch([self.cmt(1).id], [self._normal_tag.id],
+                                [self._normal_tag.id])
+
+    # The remaining cases are unusual, but do happen in the wild.
+    def test_tagged_tag(self):
+        # User already has tagged tag, send only tag of tag
+        self.assertMissingMatch([self._normal_tag.id], [self._tag_of_tag.id],
+                                [self._tag_of_tag.id])
+        # User needs both tags, but already has commit
+        self.assertMissingMatch([self.cmt(1).id], [self._tag_of_tag.id],
+                                [self._normal_tag.id, self._tag_of_tag.id])
+
+    def test_tagged_tree(self):
+        self.assertMissingMatch(
+            [], [self._tag_of_tree.id],
+            [self._tag_of_tree.id, self.cmt(1).tree, self.f1_1_id])
+
+    def test_tagged_blob(self):
+        self.assertMissingMatch([], [self._tag_of_blob.id],
+                                [self._tag_of_blob.id, self.f1_1_id])
+
+    def test_tagged_tagged_blob(self):
+        self.assertMissingMatch([], [self._tag_of_tag_of_blob.id],
+                                [self._tag_of_tag_of_blob.id,
+                                 self._tag_of_blob.id, self.f1_1_id])