Send an empty pack to clients if they requested objects, even if they

already have those objects. Fixes #781

Thanks to Martijn Pieters for the detailed bug report.

NEWS

@@ -3,6 +3,10 @@
  * Print a clearer exception when setup.py is executed on Python < 3.5.
    (Jelmer Vernooij, #783)
 
+ * Send an empty pack to clients if they requested objects, even if they
+   already have those objects. Thanks to Martijn Pieters for
+   the detailed bug report. (Jelmer Vernooij, #781)
+
 0.20.3	2020-06-14
 
  * Add support for remembering remote refs after push/pull.

dulwich/server.py

@@ -355,8 +355,14 @@ class UploadPackHandler(PackHandler):
         graph_walker = _ProtocolGraphWalker(
                 self, self.repo.object_store, self.repo.get_peeled,
                 self.repo.refs.get_symrefs)
+        wants = []
+
+        def wants_wrapper(refs):
+            wants.extend(graph_walker.determine_wants(refs))
+            return wants
+
         objects_iter = self.repo.fetch_objects(
-            graph_walker.determine_wants, graph_walker, self.progress,
+            wants_wrapper, graph_walker, self.progress,
             get_tagged=self.get_tagged)
 
         # Note the fact that client is only processing responses related
@@ -370,7 +376,7 @@ class UploadPackHandler(PackHandler):
         # with a graph walker with an implementation that talks over the
         # wire (which is this instance of this class) this will actually
         # iterate through everything and write things out to the wire.
-        if len(objects_iter) == 0:
+        if len(wants) == 0:
             return
 
         # The provided haves are processed, and it is safe to send side-
@@ -548,7 +554,7 @@ class _ProtocolGraphWalker(object):
         """Determine the wants for a set of heads.
 
         The given heads are advertised to the client, who then specifies which
-        refs he wants using 'want' lines. This portion of the protocol is the
+        refs they want using 'want' lines. This portion of the protocol is the
         same regardless of ack type, and in fact is used to set the ack type of
         the ProtocolGraphWalker.
 

dulwich/tests/compat/test_client.py

@@ -262,6 +262,22 @@ class DulwichClientTestBase(object):
                 dest.refs.set_if_equals(r[0], None, r[1])
             self.assertDestEqualsSrc()
 
+    def test_fetch_empty_pack(self):
+        c = self._client()
+        with repo.Repo(os.path.join(self.gitroot, 'dest')) as dest:
+            result = c.fetch(self._build_path('/server_new.export'), dest)
+            for r in result.refs.items():
+                dest.refs.set_if_equals(r[0], None, r[1])
+            self.assertDestEqualsSrc()
+            def dw(refs):
+                return list(refs.values())
+            result = c.fetch(
+                self._build_path('/server_new.export'), dest,
+                determine_wants=dw)
+            for r in result.refs.items():
+                dest.refs.set_if_equals(r[0], None, r[1])
+            self.assertDestEqualsSrc()
+
     def test_incremental_fetch_pack(self):
         self.test_fetch_pack()
         dest, dummy = self.disable_ff_and_make_dummy_commit()

dulwich/tests/compat/test_server.py

@@ -85,7 +85,7 @@ class GitServerSideBand64kTestCase(GitServerTestCase):
 
     def setUp(self):
         super(GitServerSideBand64kTestCase, self).setUp()
-        # side-band-64k is broken in the widows client.
+        # side-band-64k is broken in the windows client.
         # https://github.com/msysgit/git/issues/101
         # Fix has landed for the 1.9.3 release.
         if os.name == 'nt':

dulwich/tests/test_server.py

@@ -33,6 +33,7 @@ from dulwich.errors import (
     UnexpectedCommandError,
     HangupException,
     )
+from dulwich.objects import Tree
 from dulwich.object_store import (
     MemoryObjectStore,
     )
@@ -215,6 +216,35 @@ class UploadPackHandlerTestCase(TestCase):
         self._handler.set_client_capabilities(caps)
         self.assertEqual({}, self._handler.get_tagged(refs, repo=self._repo))
 
+    def test_nothing_to_do_but_wants(self):
+        # Just the fact that the client claims to want an object is enough
+        # for sending a pack. Even if there turns out to be nothing.
+        refs = {b'refs/tags/tag1': ONE}
+        tree = Tree()
+        self._repo.object_store.add_object(tree)
+        self._repo.object_store.add_object(make_commit(id=ONE, tree=tree))
+        self._repo.refs._update(refs)
+        self._handler.proto.set_output(
+            [b'want ' + ONE + b' side-band-64k thin-pack ofs-delta',
+             None, b'have ' + ONE, b'done', None])
+        self._handler.handle()
+        # The server should always send a pack, even if it's empty.
+        self.assertTrue(
+            self._handler.proto.get_received_line(1).startswith(b'PACK'))
+
+    def test_nothing_to_do_no_wants(self):
+        # Don't send a pack if the client didn't ask for anything.
+        refs = {b'refs/tags/tag1': ONE}
+        tree = Tree()
+        self._repo.object_store.add_object(tree)
+        self._repo.object_store.add_object(make_commit(id=ONE, tree=tree))
+        self._repo.refs._update(refs)
+        self._handler.proto.set_output([None])
+        self._handler.handle()
+        # The server should not send a pack, since the client didn't ask for
+        # anything.
+        self.assertEqual([], self._handler.proto._received[1])
+
 
 class FindShallowTests(TestCase):
 
@@ -320,6 +350,7 @@ class ReceivePackHandlerTestCase(TestCase):
 
 
 class ProtocolGraphWalkerEmptyTestCase(TestCase):
+
     def setUp(self):
         super(ProtocolGraphWalkerEmptyTestCase, self).setUp()
         self._repo = MemoryRepo.init_bare([], {})