Merge durin42/missing-object-finder.

This fixes #211.

NEWS

@@ -20,6 +20,9 @@
   * Resolve delta refs when pulling into a MemoryRepo.
     (Max Shawabkeh, #256)
 
+  * Fix handling of tags of non-commits in missing object finder.
+    (Augie Fackler, #211)
+
  IMPROVEMENTS
 
   * New public method `Repo.reset_index`. (Jelmer Vernooij)

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,
     )
@@ -91,13 +92,12 @@ class MOFLinearRepoTest(MissingObjectFinderTest):
         self.assertMissingMatch([self.cmt(1).id], [self.cmt(3).id],
             self.missing_1_3)
 
-    def test_bogus_haves_failure(self):
-        """Ensure non-existent SHA in haves are not tolerated"""
+    def test_bogus_haves(self):
+        """Ensure non-existent SHA in haves are tolerated"""
         bogus_sha = self.cmt(2).id[::-1]
         haves = [self.cmt(1).id, bogus_sha]
         wants = [self.cmt(3).id]
-        self.assertRaises(KeyError, self.store.find_missing_objects,
-            self.store, haves, wants)
+        self.assertMissingMatch(haves, wants, self.missing_1_3)
 
     def test_bogus_wants_failure(self):
         """Ensure non-existent SHA in wants are not tolerated"""
@@ -105,7 +105,7 @@ class MOFLinearRepoTest(MissingObjectFinderTest):
         haves = [self.cmt(1).id]
         wants = [self.cmt(3).id, bogus_sha]
         self.assertRaises(KeyError, self.store.find_missing_objects,
-            self.store, haves, wants)
+            haves, wants)
 
     def test_no_changes(self):
         self.assertMissingMatch([self.cmt(3).id], [self.cmt(3).id], [])
@@ -195,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])

dulwich/tests/test_object_store.py

@@ -53,6 +53,7 @@ from dulwich.tests import (
     )
 from dulwich.tests.utils import (
     make_object,
+    make_tag,
     build_pack,
     skipIfPY3,
     )
@@ -84,7 +85,7 @@ class ObjectStoreTests(object):
         self.store.add_objects([])
 
     def test_add_commit(self):
-        # TODO: Argh, no way to construct Git commit objects without 
+        # TODO: Argh, no way to construct Git commit objects without
         # access to a serialized form.
         self.store.add_objects([])
 
@@ -169,10 +170,7 @@ class ObjectStoreTests(object):
         self.assertEqual(expected, list(actual))
 
     def make_tag(self, name, obj):
-        tag = make_object(Tag, name=name, message='',
-                          tag_time=12345, tag_timezone=0,
-                          tagger='Test Tagger <test@example.com>',
-                          object=(object_class(obj.type_name), obj.id))
+        tag = make_tag(obj, name=name)
         self.store.add_object(tag)
         return tag
 
@@ -401,8 +399,6 @@ class TreeLookupPathTests(TestCase):
     def test_lookup_not_tree(self):
         self.assertRaises(NotTreeError, tree_lookup_path, self.get_object, self.tree_id, 'ad/b/j')
 
-# TODO: MissingObjectFinderTests
-
 @skipIfPY3
 class ObjectStoreGraphWalkerTests(TestCase):
 

dulwich/tests/test_server.py

@@ -59,6 +59,7 @@ from dulwich.tests import TestCase
 from dulwich.tests.utils import (
     make_commit,
     make_object,
+    make_tag,
     skipIfPY3,
     )
 from dulwich.protocol import (
@@ -280,10 +281,7 @@ class FindShallowTests(TestCase):
 
     def test_tag(self):
         c1, c2 = self.make_linear_commits(2)
-        tag = make_object(Tag, name='tag', message='',
-                          tagger='Tagger <test@example.com>',
-                          tag_time=12345, tag_timezone=0,
-                          object=(Commit, c2.id))
+        tag = make_tag(c2, name='tag')
         self._store.add_object(tag)
 
         self.assertEqual((set([c1.id]), set([c2.id])),

dulwich/tests/test_web.py

@@ -61,6 +61,7 @@ from dulwich.web import (
 
 from dulwich.tests.utils import (
     make_object,
+    make_tag,
     skipIfPY3,
     )
 
@@ -229,12 +230,7 @@ class DumbHandlersTestCase(WebTestCase):
         blob2 = make_object(Blob, data='2')
         blob3 = make_object(Blob, data='3')
 
-        tag1 = make_object(Tag, name='tag-tag',
-                           tagger='Test <test@example.com>',
-                           tag_time=12345,
-                           tag_timezone=0,
-                           message='message',
-                           object=(Blob, blob2.id))
+        tag1 = make_tag(blob2, name='tag-tag')
 
         objects = [blob1, blob2, blob3, tag1]
         refs = {

dulwich/tests/utils.py

@@ -36,6 +36,8 @@ from dulwich.index import (
 from dulwich.objects import (
     FixedSha,
     Commit,
+    Tag,
+    object_class,
     )
 from dulwich.pack import (
     OFS_DELTA,
@@ -100,6 +102,7 @@ def make_object(cls, **attrs):
         __dict__ instead of __slots__.
         """
         pass
+    TestObject.__name__ = 'TestObject_' + cls.__name__
 
     obj = TestObject()
     for name, value in attrs.items():
@@ -132,6 +135,27 @@ def make_commit(**attrs):
     return make_object(Commit, **all_attrs)
 
 
+def make_tag(target, **attrs):
+    """Make a Tag object with a default set of values.
+
+    :param target: object to be tagged (Commit, Blob, Tree, etc)
+    :param attrs: dict of attributes to overwrite from the default values.
+    :return: A newly initialized Tag object.
+    """
+    target_id = target.id
+    target_type = object_class(target.type_name)
+    default_time = int(time.mktime(datetime.datetime(2010, 1, 1).timetuple()))
+    all_attrs = {'tagger': 'Test Author <test@nodomain.com>',
+                 'tag_time': default_time,
+                 'tag_timezone': 0,
+                 'message': 'Test message.',
+                 'object': (target_type, target_id),
+                 'name': 'Test Tag',
+                 }
+    all_attrs.update(attrs)
+    return make_object(Tag, **all_attrs)
+
+
 def functest_builder(method, func):
     """Generate a test method that tests the given function."""