dulwich-users team mailing list archive

[dborowitz@xxxxxxxxxx]

From: Dave Borowitz <dborowitz@xxxxxxxxxx>

There are two distinct cases where determine_wants might return
"nothing":
 - The client may have finished its request, in which case it is
   expecting a 0-object pack.
 - We may be in the middle of a stateless RPC packfile negotiation
   request, in which case we should close the connection without writing
   a packfile.

This change distinguishes between these two cases by allowing
determine_wants to return either None, indicating that no pack should
be sent, or an empty iterator, indicating that an empty pack should be
sent.

Added a compat test for the no-op case that failed before.

Change-Id: Ia24f86cf8aa30d040eaf2f06a58e1d610cdeb32f
---
 NEWS                                 |    3 +++
 dulwich/repo.py                      |    6 ++++--
 dulwich/server.py                    |   14 +++++++++++---
 dulwich/tests/compat/server_utils.py |   12 ++++++++++++
 4 files changed, 30 insertions(+), 5 deletions(-)

diff --git a/NEWS b/NEWS
index 75fe560..2405824 100644
--- a/NEWS
+++ b/NEWS
@@ -11,6 +11,9 @@
 
   * Provide strnlen() on mingw32 which doesn't have it. (Hans Kolek)
 
+  * Correct short-circuiting operation for no-op fetches in the server.
+    (Dave Borowitz)
+
  FEATURES
 
   * Use slots for core objects to save up on memory. (Jelmer Vernooij)
diff --git a/dulwich/repo.py b/dulwich/repo.py
index 54a5546..40c18bf 100644
--- a/dulwich/repo.py
+++ b/dulwich/repo.py
@@ -833,8 +833,10 @@ class BaseRepo(object):
         :return: iterator over objects, with __len__ implemented
         """
         wants = determine_wants(self.get_refs())
-        if not wants:
-            return []
+        if wants is None:
+            # TODO(dborowitz): find a way to short-circuit that doesn't change
+            # this interface.
+            return None
         haves = self.object_store.find_common_revisions(graph_walker)
         return self.object_store.iter_shas(
           self.object_store.find_missing_objects(haves, wants, progress,
diff --git a/dulwich/server.py b/dulwich/server.py
index 7a97e64..a734628 100644
--- a/dulwich/server.py
+++ b/dulwich/server.py
@@ -264,8 +264,9 @@ class UploadPackHandler(Handler):
           graph_walker.determine_wants, graph_walker, self.progress,
           get_tagged=self.get_tagged)
 
-        # Do they want any objects?
-        if len(objects_iter) == 0:
+        # Did the process short-circuit (e.g. in a stateless RPC call)? Note
+        # that the client still expects a 0-object pack in most cases.
+        if objects_iter is None:
             return
 
         self.progress("dul-daemon says what\n")
@@ -367,7 +368,7 @@ class ProtocolGraphWalker(object):
             self.proto.write_pkt_line(None)
 
             if self.advertise_refs:
-                return []
+                return None
 
         # Now client will sending want want want commands
         want = self.proto.read_pkt_line()
@@ -388,6 +389,13 @@ class ProtocolGraphWalker(object):
             command, sha = self.read_proto_line(allowed)
 
         self.set_wants(want_revs)
+
+        if self.stateless_rpc and self.proto.eof():
+            # The client may close the socket at this point, expecting a
+            # flush-pkt from the server. We might be ready to send a packfile at
+            # this point, so we need to explicitly short-circuit in this case.
+            return None
+
         return want_revs
 
     def ack(self, have_ref):
diff --git a/dulwich/tests/compat/server_utils.py b/dulwich/tests/compat/server_utils.py
index 0c5dce3..4a9da06 100644
--- a/dulwich/tests/compat/server_utils.py
+++ b/dulwich/tests/compat/server_utils.py
@@ -88,6 +88,18 @@ class ServerTests(object):
         self._old_repo.object_store._pack_cache = None
         self.assertReposEqual(self._old_repo, self._new_repo)
 
+    def test_fetch_from_dulwich_no_op(self):
+        self._old_repo = import_repo('server_old.export')
+        self._new_repo = import_repo('server_old.export')
+        self.assertReposEqual(self._old_repo, self._new_repo)
+        port = self._start_server(self._new_repo)
+
+        run_git_or_fail(['fetch', self.url(port)] + self.branch_args(),
+                        cwd=self._old_repo.path)
+        # flush the pack cache so any new packs are picked up
+        self._old_repo.object_store._pack_cache = None
+        self.assertReposEqual(self._old_repo, self._new_repo)
+
 
 class ShutdownServerMixIn:
     """Mixin that allows serve_forever to be shut down.
-- 
1.7.2