Fixes issue #88 in a brute manner

- Just to get the core details done to ensure the issued is nailed down
  before the cleanup.

- `MultiAckDetailedGraphWalkerImpl` rebuilt to ensure output generated
  matches the reference implementation (i.e. follow the behavior of
  git-http-backend).

- While I would like to split out above into separate commit, the pieces
  needed seems to be scattered all over the place so I will leave this
  unisolated simply this change does not make sense without the full
  correction in place (also this was already done to isolate the actual
  root cause and building the fix).  This also required minor changes to
  the related TEST case.

- Another issue that is complicit to issue 88 is that the http server
  behaves almost as if no-done was implemented, however that capability
  is not reported to the client, i.e. the PACK section was sent without
  the expected done, which the client dutifully sends in a subsequent
  HTTP request, but instead it already got the PACK and thus silently
  errors out without completing the pull request, even with the first
  issue corrected.  If the no-done capability was specified the client
  will automatically call merge if configured to do so.

- There is another issue where attempts to pull an unrelated repo, the
  various combination of the previous flaws made it difficult to send
  the correct number of NAKs.  Removing the walker.send_ack call from
  the walker implementation and just ensure the ack is the only method
  that will call send_ack simplifies the process of correcting this.

- A state variable is needed to track whether the client expects the
  server to be processing the have lines as the reference implementation
  errors out (with `expected ACK/NAK got` something else error).  This
  was triggered by `dulwich.object_store.MissingObjectFinder.next`, and
  the flag controls whether `progress` reports progress.  This was the
  issue that was isolated by correcting the above (or some other way
  around, correction to this implementation has been rather difficult
  due to lack of discipline on where things are called, which leads
  to the next point).

- Finally, server needs to send the final ACK obj-id before the PACK
  iff client has already send done.  Otherwise this causes similar
  issues as a misplaced PACK.  Unfortunately due to poor cohesion
  between the walker and handler this was implemented in a hacky
  manner which must  be fixed in a subsequent changeset.  Also the
  definition of scoping and object ownership (e.g. certain method calls
  should not be done at where they are) needs to be corrected (i.e
  define a more common methods across all implementations).

dulwich/server.py

@@ -203,6 +203,10 @@ class Handler(object):
         self.proto = proto
         self.http_req = http_req
         self._client_capabilities = None
+        # used for UploadPackHandler, leaving this here until it is 
+        # determined that it is the only user of this.
+        self._done_required = True
+        self._done_received = False
 
     @classmethod
     def capability_line(cls):
@@ -329,6 +333,11 @@ class UploadPackHandler(Handler):
         # band data now.
         self._processing_have_lines = False
 
+        if self._done_required and not self._done_received:
+            # we are not done, especially when done is required; skip
+            # the pack for this request.
+            return
+
         self.progress(b"dul-daemon says what\n")
         self.progress(("counting objects: %d, done.\n" % len(objects_iter)).encode('ascii'))
         write_pack_objects(ProtocolFile(None, write), objects_iter)
@@ -707,17 +716,12 @@ class MultiAckDetailedGraphWalkerImpl(object):
 
     def __init__(self, walker):
         self.walker = walker
-        self._found_base = False
         self._common = []
 
     def ack(self, have_ref):
+        # Should only be called iff have_ref is common
         self._common.append(have_ref)
-        if not self._found_base:
-            self.walker.send_ack(have_ref, b'common')
-            if self.walker.all_wants_satisfied(self._common):
-                self._found_base = True
-                self.walker.send_ack(have_ref, b'ready')
-        # else we blind ack within next
+        self.walker.send_ack(have_ref, b'common')
 
     def next(self):
         while True:
@@ -731,21 +735,45 @@ class MultiAckDetailedGraphWalkerImpl(object):
                     # is a short circuit (also to make the test case
                     # as written happy).
                     return None
-                continue
             elif command == COMMAND_DONE:
-                # don't nak unless no common commits were found, even if not
-                # everything is satisfied
-                if self._common:
-                    self.walker.send_ack(self._common[-1])
-                else:
-                    self.walker.send_nak()
-                return None
+                # This whole mess really needs to be properly redefined
+                # in terms of flags and states and have a better way to
+                # signal things as required rather than poking these
+                # seemingly random variables outside of this class and
+                # hope things work in the intended manner.
+                # XXX walker should have a done notification method.
+                # XXX we don't know if handler will implement that.
+                try:
+                    self.walker.handler._done_received = True
+                except:
+                    pass
+                break
             elif command == COMMAND_HAVE:
-                if self._found_base:
-                    # blind ack; can happen if the client has more requests
-                    # inflight
-                    self.walker.send_ack(sha, b'ready')
+                # return the sha and let the caller ACK it with the
+                # above ack method.
                 return sha
+        # don't nak unless no common commits were found, even if not
+        # everything is satisfied
+        # XXX shouldn't we do this if "no-done"?
+        # TODO implement no-done.
+        if self.walker.all_wants_satisfied(self._common):
+            self.walker.send_ack(self._common[-1], b'ready')
+
+        # XXX if the next part after this can be done by walker.
+        try:
+            # XXX again, this should be handled in the walker
+            if (self.walker.handler._done_required and
+                    not self.walker.handler._done_received):
+                # do not ready.
+                return None
+        except:
+            pass
+
+        if self._common:
+            self.walker.send_ack(self._common[-1])
+        else:
+            self.walker.send_nak()
+        return None
 
     __next__ = next
 

dulwich/tests/test_server.py

@@ -723,14 +723,14 @@ class MultiAckDetailedGraphWalkerImplTestCase(AckGraphWalkerImplTestCase):
         self.assertNextEquals(ONE)
         self._walker.done = True
         self._impl.ack(ONE)
-        self.assertAcks([(ONE, b'common'), (ONE, b'ready')])
+        self.assertAck(ONE, b'common')
 
         self.assertNextEquals(THREE)
         self._impl.ack(THREE)
-        self.assertAck(THREE, b'ready')
+        self.assertAck(THREE, b'common')
 
         self.assertNextEquals(None)
-        self.assertAck(THREE)
+        self.assertAcks([(THREE, 'ready'), (THREE, '')])
 
     def test_multi_ack_partial(self):
         self.assertNextEquals(TWO)
@@ -764,14 +764,14 @@ class MultiAckDetailedGraphWalkerImplTestCase(AckGraphWalkerImplTestCase):
 
         self._walker.done = True
         self._impl.ack(ONE)
-        self.assertAcks([(ONE, b'common'), (ONE, b'ready')])
+        self.assertAck(ONE, b'common')
 
         self.assertNextEquals(THREE)
         self._impl.ack(THREE)
-        self.assertAck(THREE, b'ready')
+        self.assertAck(THREE, b'common')
 
         self.assertNextEquals(None)
-        self.assertAck(THREE)
+        self.assertAcks([(THREE, 'ready'), (THREE, '')])
 
     def test_multi_ack_nak(self):
         self.assertNextEquals(TWO)