Fix handling of 'done' in graph walker and implement the 'no-done' capability. (#88)

The gist of this gnarly problem is that it looks like the implementation over HTTP behaves as-if the `no-done` capability was enabled but unspecified so the reference client would make a second round trip back with more `haves` but then it doesn't know what to do with the `PACK` that was already sent. Alternatively there are cases where the side-band was used when it was expecting ACK/NAK. There were multiple problems with the code base that made this issue rather hard to stomp out, and here are the fixes (some verbatim from commit messages).

- `MultiAckDetailedGraphWalkerImpl` rebuilt to ensure output generated
  matches the reference implementation, like the git-http-backend. (Even though this is not strictly necessary, repeatedly calling `all_wants_specified` doesn't really change much given that there's only one place where the commits can be added in, which is by the object store through the `ack` method.

- Correction to the communication after all `have` lines have been processed. As a freebie I got `no-done` capability implemented since that's what it looked like before.

- 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.

- Added various state variables to the handler to track whether the `done` token is expected.

- Added a couple methods to deal with the handling of the above state variables, which the walker implementations themselves have access to through a method provided by the generic protocol walker class

- Added a method to the graph walker that takes in the state variables provided by the handler to delegate the dealing of the final ACK/NAK line that ends the section to permit the PACK section to begin, which then gets delegated to the Impl instances (which also have been normalized to address this).

- Finally, cleaned up the test cases and added further tests where relevant. Naturally
one of the earlier commits introduce some test examples that demonstrates this problem.

NEWS

@@ -9,6 +9,11 @@
   * Support 'git.bat' in SubprocessGitClient on Windows.
     (Stefan Zimmermann)
 
+ BUG FIXES
+
+  * Fix handling of 'done' in graph walker and implement the
+    'no-done' capability. (Tommy Yu, #88)
+
 0.10.1  2015-03-25
 
  BUG FIXES

dulwich/protocol.py

@@ -49,6 +49,7 @@ CAPABILITY_DELETE_REFS = b'delete-refs'
 CAPABILITY_INCLUDE_TAG = b'include-tag'
 CAPABILITY_MULTI_ACK = b'multi_ack'
 CAPABILITY_MULTI_ACK_DETAILED = b'multi_ack_detailed'
+CAPABILITY_NO_DONE = b'no-done'
 CAPABILITY_NO_PROGRESS = b'no-progress'
 CAPABILITY_OFS_DELTA = b'ofs-delta'
 CAPABILITY_REPORT_STATUS = b'report-status'

dulwich/repo.py

@@ -263,6 +263,9 @@ class BaseRepo(object):
 
             return []
 
+        # If the graph walker is set up with an implementation that can
+        # ACK/NAK to the wire, it will write data to the client through
+        # this call as a side-effect.
         haves = self.object_store.find_common_revisions(graph_walker)
 
         # Deal with shallow requests separately because the haves do

dulwich/server.py

@@ -72,6 +72,7 @@ from dulwich.protocol import (
     CAPABILITY_INCLUDE_TAG,
     CAPABILITY_MULTI_ACK_DETAILED,
     CAPABILITY_MULTI_ACK,
+    CAPABILITY_NO_DONE,
     CAPABILITY_NO_PROGRESS,
     CAPABILITY_OFS_DELTA,
     CAPABILITY_REPORT_STATUS,
@@ -205,6 +206,8 @@ class Handler(object):
         self.proto = proto
         self.http_req = http_req
         self._client_capabilities = None
+        # Flags needed for the no-done capability
+        self._done_received = False
 
     @classmethod
     def capability_line(cls):
@@ -244,6 +247,9 @@ class Handler(object):
                                    'before asking client' % cap)
         return cap in self._client_capabilities
 
+    def notify_done(self):
+        self._done_received = True
+
 
 class UploadPackHandler(Handler):
     """Protocol handler for uploading a pack to the server."""
@@ -264,7 +270,7 @@ class UploadPackHandler(Handler):
         return (CAPABILITY_MULTI_ACK_DETAILED, CAPABILITY_MULTI_ACK,
                 CAPABILITY_SIDE_BAND_64K, CAPABILITY_THIN_PACK,
                 CAPABILITY_OFS_DELTA, CAPABILITY_NO_PROGRESS,
-                CAPABILITY_INCLUDE_TAG, CAPABILITY_SHALLOW)
+                CAPABILITY_INCLUDE_TAG, CAPABILITY_SHALLOW, CAPABILITY_NO_DONE)
 
     @classmethod
     def required_capabilities(cls):
@@ -331,6 +337,10 @@ class UploadPackHandler(Handler):
         # band data now.
         self._processing_have_lines = False
 
+        if not graph_walker.handle_done(
+                not self.has_capability(CAPABILITY_NO_DONE), self._done_received):
+            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)
@@ -607,6 +617,10 @@ class ProtocolGraphWalker(object):
 
         self.proto.write_pkt_line(None)
 
+    def notify_done(self):
+        # relay the message down to the handler.
+        self.handler.notify_done()
+
     def send_ack(self, sha, ack_type=b''):
         if ack_type:
             ack_type = b' ' + ack_type
@@ -615,6 +629,10 @@ class ProtocolGraphWalker(object):
     def send_nak(self):
         self.proto.write_pkt_line(b'NAK\n')
 
+    def handle_done(self, done_required, done_received):
+        # Delegate this to the implementation.
+        return self._impl.handle_done(done_required, done_received)
+
     def set_wants(self, wants):
         self._wants = wants
 
@@ -644,24 +662,44 @@ class SingleAckGraphWalkerImpl(object):
 
     def __init__(self, walker):
         self.walker = walker
-        self._sent_ack = False
+        self._common = []
 
     def ack(self, have_ref):
-        if not self._sent_ack:
+        if not self._common:
             self.walker.send_ack(have_ref)
-            self._sent_ack = True
+            self._common.append(have_ref)
 
     def next(self):
         command, sha = self.walker.read_proto_line(_GRAPH_WALKER_COMMANDS)
         if command in (None, COMMAND_DONE):
-            if not self._sent_ack:
-                self.walker.send_nak()
+            # defer the handling of done
+            self.walker.notify_done()
             return None
         elif command == COMMAND_HAVE:
             return sha
 
     __next__ = next
 
+    def handle_done(self, done_required, done_received):
+        if not self._common:
+            self.walker.send_nak()
+
+        if done_required and not done_received:
+            # we are not done, especially when done is required; skip
+            # the pack for this request and especially do not handle
+            # the done.
+            return False
+
+        if not done_received and not self._common:
+            # Okay we are not actually done then since the walker picked
+            # up no haves.  This is usually triggered when client attempts
+            # to pull from a source that has no common base_commit.
+            # See: test_server.MultiAckDetailedGraphWalkerImplTestCase.\
+            #          test_multi_ack_stateless_nodone
+            return False
+
+        return True
+
 
 class MultiAckGraphWalkerImpl(object):
     """Graph walker implementation that speaks the multi-ack protocol."""
@@ -688,12 +726,7 @@ class MultiAckGraphWalkerImpl(object):
                 # flush but more have lines are still coming
                 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()
+                self.walker.notify_done()
                 return None
             elif command == COMMAND_HAVE:
                 if self._found_base:
@@ -703,49 +736,94 @@ class MultiAckGraphWalkerImpl(object):
 
     __next__ = next
 
+    def handle_done(self, done_required, done_received):
+        if done_required and not done_received:
+            # we are not done, especially when done is required; skip
+            # the pack for this request and especially do not handle
+            # the done.
+            return False
+
+        if not done_received and not self._common:
+            # Okay we are not actually done then since the walker picked
+            # up no haves.  This is usually triggered when client attempts
+            # to pull from a source that has no common base_commit.
+            # See: test_server.MultiAckDetailedGraphWalkerImplTestCase.\
+            #          test_multi_ack_stateless_nodone
+            return False
+
+        # 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 True
+
 
 class MultiAckDetailedGraphWalkerImpl(object):
     """Graph walker implementation speaking the multi-ack-detailed protocol."""
 
     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:
             command, sha = self.walker.read_proto_line(_GRAPH_WALKER_COMMANDS)
             if command is None:
+                if self.walker.all_wants_satisfied(self._common):
+                    self.walker.send_ack(self._common[-1], b'ready')
                 self.walker.send_nak()
                 if self.walker.http_req:
+                    # The HTTP version of this request a flush-pkt always
+                    # signifies an end of request, so we also return
+                    # nothing here as if we are done (but not really, as
+                    # it depends on whether no-done capability was
+                    # specified and that's handled in handle_done which
+                    # may or may not call post_nodone_check depending on
+                    # that).
                     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
+                # Let the walker know that we got a done.
+                self.walker.notify_done()
+                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
 
     __next__ = next
 
+    def handle_done(self, done_required, done_received):
+        if done_required and not done_received:
+            # we are not done, especially when done is required; skip
+            # the pack for this request and especially do not handle
+            # the done.
+            return False
+
+        if not done_received and not self._common:
+            # Okay we are not actually done then since the walker picked
+            # up no haves.  This is usually triggered when client attempts
+            # to pull from a source that has no common base_commit.
+            # See: test_server.MultiAckDetailedGraphWalkerImplTestCase.\
+            #          test_multi_ack_stateless_nodone
+            return False
+
+        # 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 True
+
 
 class ReceivePackHandler(Handler):
     """Protocol handler for downloading a pack from the client."""
@@ -758,7 +836,8 @@ class ReceivePackHandler(Handler):
 
     @classmethod
     def capabilities(cls):
-        return (CAPABILITY_REPORT_STATUS, CAPABILITY_DELETE_REFS, CAPABILITY_SIDE_BAND_64K)
+        return (CAPABILITY_REPORT_STATUS, CAPABILITY_DELETE_REFS,
+                CAPABILITY_SIDE_BAND_64K, CAPABILITY_NO_DONE)
 
     def _apply_pack(self, refs):
         all_exceptions = (IOError, OSError, ChecksumMismatch, ApplyDeltaError,

dulwich/tests/compat/server_utils.py

@@ -221,6 +221,43 @@ class ServerTests(object):
         self.assertEqual([], _get_shallow(clone))
         self.assertReposEqual(clone, self._source_repo)
 
+    def test_fetch_from_dulwich_issue_88_standard(self):
+        # Basically an integration test to see that the ACK/NAK
+        # generation works on repos with common head.
+        self._source_repo = self.import_repo('issue88_expect_ack_nak_server.export')
+        self._client_repo = self.import_repo('issue88_expect_ack_nak_client.export')
+        port = self._start_server(self._source_repo)
+
+        run_git_or_fail(['fetch', self.url(port), 'master',],
+                        cwd=self._client_repo.path)
+        self.assertObjectStoreEqual(
+            self._source_repo.object_store,
+            self._client_repo.object_store)
+
+    def test_fetch_from_dulwich_issue_88_alternative(self):
+        # likewise, but the case where the two repos have no common parent
+        self._source_repo = self.import_repo('issue88_expect_ack_nak_other.export')
+        self._client_repo = self.import_repo('issue88_expect_ack_nak_client.export')
+        port = self._start_server(self._source_repo)
+
+        self.assertRaises(KeyError, self._client_repo.get_object,
+            b'02a14da1fc1fc13389bbf32f0af7d8899f2b2323')
+        run_git_or_fail(['fetch', self.url(port), 'master',],
+                        cwd=self._client_repo.path)
+        self.assertEqual(b'commit', self._client_repo.get_object(
+            b'02a14da1fc1fc13389bbf32f0af7d8899f2b2323').type_name)
+
+    def test_push_to_dulwich_issue_88_standard(self):
+        # Same thing, but we reverse the role of the server/client
+        # and do a push instead.
+        self._source_repo = self.import_repo('issue88_expect_ack_nak_client.export')
+        self._client_repo = self.import_repo('issue88_expect_ack_nak_server.export')
+        port = self._start_server(self._source_repo)
+
+        run_git_or_fail(['push', self.url(port), 'master',],
+                        cwd=self._client_repo.path)
+        self.assertReposEqual(self._source_repo, self._client_repo)
+
 
 # TODO(dborowitz): Come up with a better way of testing various permutations of
 # capabilities. The only reason it is the way it is now is that side-band-64k

dulwich/tests/compat/test_web.py

@@ -30,6 +30,8 @@ import sys
 
 from dulwich.server import (
     DictBackend,
+    UploadPackHandler,
+    ReceivePackHandler,
     )
 from dulwich.tests import (
     SkipTest,
@@ -106,6 +108,18 @@ class SmartWebTestCase(WebTests, CompatTestCase):
         return app
 
 
+def patch_capabilities(handler, caps_removed):
+    # Patch a handler's capabilities by specifying a list of them to be
+    # removed, and return the original classmethod for restoration.
+    original_capabilities = handler.capabilities
+    filtered_capabilities = tuple(
+        i for i in original_capabilities() if i not in caps_removed)
+    def capabilities(cls):
+        return filtered_capabilities
+    handler.capabilities = classmethod(capabilities)
+    return original_capabilities
+
+
 @skipIf(sys.platform == 'win32', 'Broken on windows, with very long fail time.')
 @skipIfPY3
 class SmartWebSideBand64kTestCase(SmartWebTestCase):
@@ -114,6 +128,16 @@ class SmartWebSideBand64kTestCase(SmartWebTestCase):
     # side-band-64k in git-receive-pack was introduced in git 1.7.0.2
     min_git_version = (1, 7, 0, 2)
 
+    def setUp(self):
+        self.o_uph_cap = patch_capabilities(UploadPackHandler, ("no-done",))
+        self.o_rph_cap = patch_capabilities(ReceivePackHandler, ("no-done",))
+        super(SmartWebSideBand64kTestCase, self).setUp()
+
+    def tearDown(self):
+        super(SmartWebSideBand64kTestCase, self).tearDown()
+        UploadPackHandler.capabilities = self.o_uph_cap
+        ReceivePackHandler.capabilities = self.o_rph_cap
+
     def _handlers(self):
         return None  # default handlers include side-band-64k
 
@@ -121,6 +145,25 @@ class SmartWebSideBand64kTestCase(SmartWebTestCase):
         receive_pack_handler_cls = app.handlers[b'git-receive-pack']
         caps = receive_pack_handler_cls.capabilities()
         self.assertTrue(b'side-band-64k' in caps)
+        self.assertFalse(b'no-done' in caps)
+
+
+class SmartWebSideBand64kNoDoneTestCase(SmartWebTestCase):
+    """Test cases for smart HTTP server with side-band-64k and no-done
+    support.
+    """
+
+    # no-done was introduced in git 1.7.4
+    min_git_version = (1, 7, 4)
+
+    def _handlers(self):
+        return None  # default handlers include side-band-64k
+
+    def _check_app(self, app):
+        receive_pack_handler_cls = app.handlers[b'git-receive-pack']
+        caps = receive_pack_handler_cls.capabilities()
+        self.assertTrue(b'side-band-64k' in caps)
+        self.assertTrue(b'no-done' in caps)
 
 
 @skipIf(sys.platform == 'win32', 'Broken on windows, with very long fail time.')
@@ -153,3 +196,7 @@ class DumbWebTestCase(WebTests, CompatTestCase):
         # Note: remove this if C git and dulwich implement dumb web shallow
         # clones.
         raise SkipTest('Dumb web shallow cloning not supported.')
+
+    def test_push_to_dulwich_issue_88_standard(self):
+        raise SkipTest('Dumb web pushing not supported.')
+

dulwich/tests/compat/utils.py

@@ -152,7 +152,7 @@ def import_repo_to_dir(name):
     """Import a repo from a fast-export file in a temporary directory.
 
     These are used rather than binary repos for compat tests because they are
-    more compact an human-editable, and we already depend on git.
+    more compact and human-editable, and we already depend on git.
 
     :param name: The name of the repository export file, relative to
         dulwich/tests/data/repos.

dulwich/tests/data/repos/issue88_expect_ack_nak_client.export

@@ -0,0 +1,260 @@
+reset refs/heads/master
+commit refs/heads/master
+mark :1
+author User <user@localhost> 1427183369 +1300
+committer User <user@localhost> 1427183369 +1300
+data 6
+empty
+
+blob
+mark :2
+data 35
+We will reproduce a problem here.
+
+commit refs/heads/master
+mark :3
+author User <user@localhost> 1427183376 +1300
+committer User <user@localhost> 1427183376 +1300
+data 11
+demo file.
+from :1
+M 100644 :2 demo.txt
+
+blob
+mark :4
+data 62
+We will reproduce a problem here.
+
+This will take some time.
+
+commit refs/heads/master
+mark :5
+author User <user@localhost> 1427185135 +1300
+committer User <user@localhost> 1427185135 +1300
+data 13
+added a line
+from :3
+M 100644 :4 demo.txt
+
+blob
+mark :6
+data 57
+We will reproduce a problem here.
+
+We will change these.
+
+commit refs/heads/master
+mark :7
+author User <user@localhost> 1427185245 +1300
+committer User <user@localhost> 1427185245 +1300
+data 14
+replace a line
+from :5
+M 100644 :6 demo.txt
+
+blob
+mark :8
+data 52
+We will change these.
+
+Then issues will be proven.
+
+commit refs/heads/master
+mark :9
+author User <user@localhost> 1427185343 +1300
+committer User <user@localhost> 1427185343 +1300
+data 13
+Yes we will.
+from :7
+M 100644 :8 demo.txt
+
+blob
+mark :10
+data 69
+We will change these. 
+
+Then issues will be proven once and for all.
+
+commit refs/heads/master
+mark :11
+author User <user@localhost> 1427185440 +1300
+committer User <user@localhost> 1427185440 +1300
+data 6
+sure.
+from :9
+M 100644 :10 demo.txt
+
+blob
+mark :12
+data 0
+
+commit refs/heads/master
+mark :13
+author User <user@localhost> 1427185512 +1300
+committer User <user@localhost> 1427185516 +1300
+data 26
+not an actual readme, yet
+from :11
+M 100644 :12 readme.txt
+
+blob
+mark :14
+data 61
+This will for sure we will prove a problem exist somewhere.
+
+blob
+mark :15
+data 49
+okay fine add something here this is only a test
+
+commit refs/heads/master
+mark :16
+author User <user@localhost> 1427185569 +1300
+committer User <user@localhost> 1427185569 +1300
+data 12
+more things
+from :13
+M 100644 :14 demo.txt
+M 100644 :15 readme.txt
+
+blob
+mark :17
+data 100
+This will for sure we will prove a problem exist somewhere. 
+
+Just that we need a few more commits.
+
+commit refs/heads/master
+mark :18
+author User <user@localhost> 1427185659 +1300
+committer User <user@localhost> 1427185659 +1300
+data 13
+one more try
+from :16
+M 100644 :17 demo.txt
+
+blob
+mark :19
+data 54
+It might have something to do with number of commits?
+
+commit refs/heads/master
+mark :20
+author User <user@localhost> 1427185905 +1300
+committer User <user@localhost> 1427185905 +1300
+data 18
+is this number 9?
+from :18
+M 100644 :19 commitcount
+
+blob
+mark :21
+data 123
+This will for sure we will prove a problem exist somewhere. 
+
+Just that we need a few more commits.
+
+Hey look we need more
+
+commit refs/heads/master
+mark :22
+author User <user@localhost> 1427185922 +1300
+committer User <user@localhost> 1427185922 +1300
+data 5
+cool
+from :20
+M 100644 :21 demo.txt
+
+blob
+mark :23
+data 50
+Okay fine add something here this is only a test.
+
+commit refs/heads/master
+mark :24
+author User <user@localhost> 1427185936 +1300
+committer User <user@localhost> 1427185936 +1300
+data 7
+readme
+from :22
+M 100644 :23 readme.txt
+
+blob
+mark :25
+data 74
+Okay come on this is getting boring.
+
+Yes I went and edit all the things.
+
+commit refs/heads/master
+mark :26
+author User <user@localhost> 1427185954 +1300
+committer User <user@localhost> 1427185954 +1300
+data 14
+remove a line
+from :24
+M 100644 :25 demo.txt
+
+blob
+mark :27
+data 186
+Okay come on this is getting boring. 
+
+Yes I went and edit all the things. 
+
+Of course, making test data can be somewhat tedious, especially a
+minimum set that can be easily reproduced.
+
+commit refs/heads/master
+mark :28
+author User <user@localhost> 1427185996 +1300
+committer User <user@localhost> 1427185996 +1300
+data 25
+Getting serious mode on.
+from :26
+M 100644 :27 demo.txt
+
+blob
+mark :29
+data 48
+This is taking a bit longer than I remembered.
+
+commit refs/heads/master
+mark :30
+author User <user@localhost> 1427186065 +1300
+committer User <user@localhost> 1427186065 +1300
+data 40
+At least we will have things minimized.
+from :28
+M 100644 :29 demo.txt
+
+blob
+mark :31
+data 11
+there yet?
+
+commit refs/heads/master
+mark :32
+author User <user@localhost> 1427186080 +1300
+committer User <user@localhost> 1427186080 +1300
+data 7
+are we
+from :30
+M 100644 :31 demo.txt
+
+blob
+mark :33
+data 237
+This should be the head commit for the client repo for testing out
+the failure case reported in issue 88.  Just do a git pull from the
+repo that includes the following commit that is hosted with dulwich.
+The issue should be reproduced.
+
+commit refs/heads/master
+mark :34
+author User <user@localhost> 1427186109 +1300
+committer User <user@localhost> 1427186109 +1300
+data 6
+okay?
+from :32
+M 100644 :33 readme.txt

dulwich/tests/data/repos/issue88_expect_ack_nak_other.export

@@ -0,0 +1,293 @@
+blob
+mark :1
+data 33
+We will sneak in a blob like so.
+
+reset refs/heads/master
+commit refs/heads/master
+mark :2
+author User <user@localhost> 1427183369 +1300
+committer User <user@localhost> 1427183369 +1300
+data 7
+sneaky
+M 100644 :1 problem.questionmark
+
+blob
+mark :3
+data 35
+We will introduce a problem here.
+
+
+commit refs/heads/master
+mark :4
+author User <user@localhost> 1427183376 +1300
+committer User <user@localhost> 1427183376 +1300
+data 11
+demo file.
+from :2
+M 100644 :3 demo.rst
+
+blob
+mark :5
+data 62
+We will introduce a problem here.
+
+This will take some time.
+
+
+commit refs/heads/master
+mark :6
+author User <user@localhost> 1427185135 +1300
+committer User <user@localhost> 1427185135 +1300
+data 13
+added a line
+from :4
+M 100644 :5 demo.rst
+
+blob
+mark :7
+data 57
+We will introduce a problem here.
+
+We will change these.
+
+commit refs/heads/master
+mark :8
+author User <user@localhost> 1427185245 +1300
+committer User <user@localhost> 1427185245 +1300
+data 14
+replace a linefrom :6
+M 100644 :7 demo.rst
+
+blob
+mark :9
+data 52
+We will change these.
+
+Then issues will be proven.
+
+
+commit refs/heads/master
+mark :10
+author User <user@localhost> 1427185343 +1300
+committer User <user@localhost> 1427185343 +1300
+data 13
+Yes we will.
+from :8
+M 100644 :9 demo.rst
+
+blob
+mark :11
+data 72
+We will change these. 
+
+Then issues will be construed once and for all.
+
+commit refs/heads/master
+mark :12
+author User <user@localhost> 1427185440 +1300
+committer User <user@localhost> 1427185440 +1300
+data 6
+sure.
+from :10
+M 100644 :11 demo.rst
+
+blob
+mark :13
+data 0
+
+commit refs/heads/master
+mark :14
+author User <user@localhost> 1427185512 +1300
+committer User <user@localhost> 1427185516 +1300
+data 26
+not an actual readme, yet
+from :12
+M 100644 :13 emdaer.txt
+
+blob
+mark :15
+data 58
+This will for sure we will prove issues exist somewhere.
+
+
+blob
+mark :16
+data 49
+okay fine add something here this is only a test
+
+commit refs/heads/master
+mark :17
+author User <user@localhost> 1427185569 +1300
+committer User <user@localhost> 1427185569 +1300
+data 12
+more things
+from :14
+M 100644 :15 demo.rst
+M 100644 :16 emdaer.txt
+
+blob
+mark :18
+data 97
+This will for sure prove issue exist somewhere.
+
+Just that we need a few more commits as usual.
+
+
+commit refs/heads/master
+mark :19
+author User <user@localhost> 1427185659 +1300
+committer User <user@localhost> 1427185659 +1300
+data 13
+one more try
+from :17
+M 100644 :18 demo.rst
+
+blob
+mark :20
+data 54
+It might have something to do with number of commits?
+
+commit refs/heads/master
+mark :21
+author User <user@localhost> 1427185905 +1300
+committer User <user@localhost> 1427185905 +1300
+data 18
+is this number 9?
+from :19
+M 100644 :20 count
+
+blob
+mark :22
+data 119
+This will for sure we will prove issues exist somewhere.
+
+Just that we need a few more commits.
+
+Hey look we need more
+
+commit refs/heads/master
+mark :23
+author User <user@localhost> 1427185922 +1300
+committer User <user@localhost> 1427185922 +1300
+data 5
+cool
+from :21
+M 100644 :22 demo.rst
+
+blob
+mark :24
+data 50
+Okay fine add something here this is only a test.
+
+commit refs/heads/master
+mark :25
+author User <user@localhost> 1427185936 +1300
+committer User <user@localhost> 1427185936 +1300
+data 7
+readme
+from :23
+M 100644 :24 emdaer.txt
+
+blob
+mark :26
+data 74
+Okay come on this is getting boring.
+
+Yes I went and edit all the things.
+
+commit refs/heads/master
+mark :27
+author User <user@localhost> 1427185954 +1300
+committer User <user@localhost> 1427185954 +1300
+data 14
+remove a line
+from :25
+M 100644 :26 demo.rst
+
+blob
+mark :28
+data 186
+Okay come on this is getting boring. 
+
+Yes I went and edit all the things. 
+
+Of course, making test data can be somewhat tedious, especially a
+minimum set that can be easily reproduced.
+
+commit refs/heads/master
+mark :29
+author User <user@localhost> 1427185996 +1300
+committer User <user@localhost> 1427185996 +1300
+data 25
+Getting serious mode on.
+from :27
+M 100644 :28 demo.rst
+
+blob
+mark :30
+data 48
+This is taking a bit longer than I remembered.
+
+
+commit refs/heads/master
+mark :31
+author User <user@localhost> 1427186065 +1300
+committer User <user@localhost> 1427186065 +1300
+data 40
+At least we will have things minimized.
+from :29
+M 100644 :30 demo.rst
+
+blob
+mark :32
+data 11
+there yet?
+
+commit refs/heads/master
+mark :33
+author User <user@localhost> 1427186080 +1300
+committer User <user@localhost> 1427186080 +1300
+data 7
+are we
+from :31
+M 100644 :32 demo.rst
+
+blob
+mark :34
+data 237
+This should be the head commit for the client repo for testing out
+the failure case reported in issue 88.  Just do a git pull from the
+repo that includes the following commit that is hosted with dulwich.
+The issue should be reproduced.
+
+
+commit refs/heads/master
+mark :35
+author User <user@localhost> 1427186109 +1300
+committer User <user@localhost> 1427186109 +1300
+data 6
+okay?
+from :33
+M 100644 :34 emdaer.txt
+
+blob
+mark :36
+data 394
+This should be the commit that will trigger the bug noted in issue 88
+(https://github.com/jelmer/dulwich/issues/88).  To reproduce, run git
+fast-import using this fast-export and host this using dulwich, and
+then make a copy of this, strip out this blob and the following commit
+block, import to another git repo and then git clone from the previous.
+
+Naturally, this is part of the test case.
+
+commit refs/heads/master
+mark :37
+author User <user@localhost> 1427244891 +1300
+committer User <user@localhost> 1427248186 +1300
+data 49
+Added instructions on how to use this to readme.
+from :35
+M 100644 :36 emdaer.txt
+

dulwich/tests/data/repos/issue88_expect_ack_nak_server.export

@@ -0,0 +1,281 @@
+reset refs/heads/master
+commit refs/heads/master
+mark :1
+author User <user@localhost> 1427183369 +1300
+committer User <user@localhost> 1427183369 +1300
+data 6
+empty
+
+blob
+mark :2
+data 35
+We will reproduce a problem here.
+
+commit refs/heads/master
+mark :3
+author User <user@localhost> 1427183376 +1300
+committer User <user@localhost> 1427183376 +1300
+data 11
+demo file.
+from :1
+M 100644 :2 demo.txt
+
+blob
+mark :4
+data 62
+We will reproduce a problem here.
+
+This will take some time.
+
+commit refs/heads/master
+mark :5
+author User <user@localhost> 1427185135 +1300
+committer User <user@localhost> 1427185135 +1300
+data 13
+added a line
+from :3
+M 100644 :4 demo.txt
+
+blob
+mark :6
+data 57
+We will reproduce a problem here.
+
+We will change these.
+
+commit refs/heads/master
+mark :7
+author User <user@localhost> 1427185245 +1300
+committer User <user@localhost> 1427185245 +1300
+data 14
+replace a line
+from :5
+M 100644 :6 demo.txt
+
+blob
+mark :8
+data 52
+We will change these.
+
+Then issues will be proven.
+
+commit refs/heads/master
+mark :9
+author User <user@localhost> 1427185343 +1300
+committer User <user@localhost> 1427185343 +1300
+data 13
+Yes we will.
+from :7
+M 100644 :8 demo.txt
+
+blob
+mark :10
+data 69
+We will change these. 
+
+Then issues will be proven once and for all.
+
+commit refs/heads/master
+mark :11
+author User <user@localhost> 1427185440 +1300
+committer User <user@localhost> 1427185440 +1300
+data 6
+sure.
+from :9
+M 100644 :10 demo.txt
+
+blob
+mark :12
+data 0
+
+commit refs/heads/master
+mark :13
+author User <user@localhost> 1427185512 +1300
+committer User <user@localhost> 1427185516 +1300
+data 26
+not an actual readme, yet
+from :11
+M 100644 :12 readme.txt
+
+blob
+mark :14
+data 61
+This will for sure we will prove a problem exist somewhere.
+
+blob
+mark :15
+data 49
+okay fine add something here this is only a test
+
+commit refs/heads/master
+mark :16
+author User <user@localhost> 1427185569 +1300
+committer User <user@localhost> 1427185569 +1300
+data 12
+more things
+from :13
+M 100644 :14 demo.txt
+M 100644 :15 readme.txt
+
+blob
+mark :17
+data 100
+This will for sure we will prove a problem exist somewhere. 
+
+Just that we need a few more commits.
+
+commit refs/heads/master
+mark :18
+author User <user@localhost> 1427185659 +1300
+committer User <user@localhost> 1427185659 +1300
+data 13
+one more try
+from :16
+M 100644 :17 demo.txt
+
+blob
+mark :19
+data 54
+It might have something to do with number of commits?
+
+commit refs/heads/master
+mark :20
+author User <user@localhost> 1427185905 +1300
+committer User <user@localhost> 1427185905 +1300
+data 18
+is this number 9?
+from :18
+M 100644 :19 commitcount
+
+blob
+mark :21
+data 123
+This will for sure we will prove a problem exist somewhere. 
+
+Just that we need a few more commits.
+
+Hey look we need more
+
+commit refs/heads/master
+mark :22
+author User <user@localhost> 1427185922 +1300
+committer User <user@localhost> 1427185922 +1300
+data 5
+cool
+from :20
+M 100644 :21 demo.txt
+
+blob
+mark :23
+data 50
+Okay fine add something here this is only a test.
+
+commit refs/heads/master
+mark :24
+author User <user@localhost> 1427185936 +1300
+committer User <user@localhost> 1427185936 +1300
+data 7
+readme
+from :22
+M 100644 :23 readme.txt
+
+blob
+mark :25
+data 74
+Okay come on this is getting boring.
+
+Yes I went and edit all the things.
+
+commit refs/heads/master
+mark :26
+author User <user@localhost> 1427185954 +1300
+committer User <user@localhost> 1427185954 +1300
+data 14
+remove a line
+from :24
+M 100644 :25 demo.txt
+
+blob
+mark :27
+data 186
+Okay come on this is getting boring. 
+
+Yes I went and edit all the things. 
+
+Of course, making test data can be somewhat tedious, especially a
+minimum set that can be easily reproduced.
+
+commit refs/heads/master
+mark :28
+author User <user@localhost> 1427185996 +1300
+committer User <user@localhost> 1427185996 +1300
+data 25
+Getting serious mode on.
+from :26
+M 100644 :27 demo.txt
+
+blob
+mark :29
+data 48
+This is taking a bit longer than I remembered.
+
+commit refs/heads/master
+mark :30
+author User <user@localhost> 1427186065 +1300
+committer User <user@localhost> 1427186065 +1300
+data 40
+At least we will have things minimized.
+from :28
+M 100644 :29 demo.txt
+
+blob
+mark :31
+data 11
+there yet?
+
+commit refs/heads/master
+mark :32
+author User <user@localhost> 1427186080 +1300
+committer User <user@localhost> 1427186080 +1300
+data 7
+are we
+from :30
+M 100644 :31 demo.txt
+
+blob
+mark :33
+data 237
+This should be the head commit for the client repo for testing out
+the failure case reported in issue 88.  Just do a git pull from the
+repo that includes the following commit that is hosted with dulwich.
+The issue should be reproduced.
+
+commit refs/heads/master
+mark :34
+author User <user@localhost> 1427186109 +1300
+committer User <user@localhost> 1427186109 +1300
+data 6
+okay?
+from :32
+M 100644 :33 readme.txt
+
+blob
+mark :35
+data 394
+This should be the commit that will trigger the bug noted in issue 88
+(https://github.com/jelmer/dulwich/issues/88).  To reproduce, run git
+fast-import using this fast-export and host this using dulwich, and
+then make a copy of this, strip out this blob and the following commit
+block, import to another git repo and then git clone from the previous.
+
+Naturally, this is part of the test case.
+
+commit refs/heads/master
+mark :36
+author User <user@localhost> 1427244891 +1300
+committer User <user@localhost> 1427248186 +1300
+data 49
+Added instructions on how to use this to readme.
+from :34
+M 100644 :35 readme.txt
+

dulwich/tests/test_porcelain.py

@@ -644,8 +644,8 @@ class ReceivePackTests(PorcelainTestCase):
         exitcode = porcelain.receive_pack(self.repo.path, BytesIO(b"0000"), outf)
         outlines = outf.getvalue().splitlines()
         self.assertEqual([
-            b'005a9e65bdcf4a22cdd4f3700604a275cd2aaf146b23 HEAD\x00report-status '
-            b'delete-refs side-band-64k',
+            b'00629e65bdcf4a22cdd4f3700604a275cd2aaf146b23 HEAD\x00report-status '
+            b'delete-refs side-band-64k no-done',
             b'003f9e65bdcf4a22cdd4f3700604a275cd2aaf146b23 refs/heads/master',
             b'0000'], outlines)
         self.assertEqual(0, exitcode)

dulwich/tests/test_server.py

@@ -515,9 +515,14 @@ class TestProtocolGraphWalker(object):
     def __init__(self):
         self.acks = []
         self.lines = []
-        self.done = False
+        self.wants_satisified = False
         self.http_req = None
         self.advertise_refs = False
+        self._impl = None
+        self.done_required = True
+        self.done_received = False
+        self._empty = False
+        self.pack_sent = False
 
     def read_proto_line(self, allowed):
         command, sha = self.lines.pop(0)
@@ -532,13 +537,26 @@ class TestProtocolGraphWalker(object):
         self.acks.append((None, b'nak'))
 
     def all_wants_satisfied(self, haves):
-        return self.done
+        if haves:
+            return self.wants_satisified
 
     def pop_ack(self):
         if not self.acks:
             return None
         return self.acks.pop(0)
 
+    def handle_done(self):
+        if not self._impl:
+            return
+        # Whether or not PACK is sent after is determined by this, so
+        # record this value.
+        self.pack_sent = self._impl.handle_done(self.done_required,
+            self.done_received)
+        return self.pack_sent
+
+    def notify_done(self):
+        self.done_received = True
+
 
 class AckGraphWalkerImplTestCase(TestCase):
     """Base setup and asserts for AckGraphWalker tests."""
@@ -553,6 +571,7 @@ class AckGraphWalkerImplTestCase(TestCase):
           (b'done', None),
           ]
         self._impl = self.impl_cls(self._walker)
+        self._walker._impl = self._impl
 
     def assertNoAck(self):
         self.assertEqual(None, self._walker.pop_ack())
@@ -571,6 +590,15 @@ class AckGraphWalkerImplTestCase(TestCase):
     def assertNextEquals(self, sha):
         self.assertEqual(sha, next(self._impl))
 
+    def assertNextEmpty(self):
+        # This is necessary because of no-done - the assumption that it
+        # it safe to immediately send out the final ACK is no longer
+        # true but the test is still needed for it.  TestProtocolWalker
+        # does implement the handle_done which will determine whether
+        # the final confirmation can be sent.
+        self.assertRaises(IndexError, next, self._impl)
+        self._walker.handle_done()
+
 
 class SingleAckGraphWalkerImplTestCase(AckGraphWalkerImplTestCase):
 
@@ -581,7 +609,6 @@ class SingleAckGraphWalkerImplTestCase(AckGraphWalkerImplTestCase):
         self.assertNoAck()
 
         self.assertNextEquals(ONE)
-        self._walker.done = True
         self._impl.ack(ONE)
         self.assertAck(ONE)
 
@@ -600,7 +627,6 @@ class SingleAckGraphWalkerImplTestCase(AckGraphWalkerImplTestCase):
         self.assertNoAck()
 
         self.assertNextEquals(ONE)
-        self._walker.done = True
         self._impl.ack(ONE)
         self.assertAck(ONE)
 
@@ -621,6 +647,7 @@ class SingleAckGraphWalkerImplTestCase(AckGraphWalkerImplTestCase):
         self.assertNoAck()
 
         self.assertNextEquals(None)
+        self.assertNextEmpty()
         self.assertNak()
 
     def test_single_ack_nak_flush(self):
@@ -637,6 +664,7 @@ class SingleAckGraphWalkerImplTestCase(AckGraphWalkerImplTestCase):
         self.assertNoAck()
 
         self.assertNextEquals(None)
+        self.assertNextEmpty()
         self.assertNak()
 
 
@@ -649,7 +677,6 @@ class MultiAckGraphWalkerImplTestCase(AckGraphWalkerImplTestCase):
         self.assertNoAck()
 
         self.assertNextEquals(ONE)
-        self._walker.done = True
         self._impl.ack(ONE)
         self.assertAck(ONE, b'continue')
 
@@ -658,6 +685,7 @@ class MultiAckGraphWalkerImplTestCase(AckGraphWalkerImplTestCase):
         self.assertAck(THREE, b'continue')
 
         self.assertNextEquals(None)
+        self.assertNextEmpty()
         self.assertAck(THREE)
 
     def test_multi_ack_partial(self):
@@ -672,7 +700,7 @@ class MultiAckGraphWalkerImplTestCase(AckGraphWalkerImplTestCase):
         self.assertNoAck()
 
         self.assertNextEquals(None)
-        # done, re-send ack of last common
+        self.assertNextEmpty()
         self.assertAck(ONE)
 
     def test_multi_ack_flush(self):
@@ -689,7 +717,6 @@ class MultiAckGraphWalkerImplTestCase(AckGraphWalkerImplTestCase):
         self.assertNextEquals(ONE)
         self.assertNak()  # nak the flush-pkt
 
-        self._walker.done = True
         self._impl.ack(ONE)
         self.assertAck(ONE, b'continue')
 
@@ -698,6 +725,7 @@ class MultiAckGraphWalkerImplTestCase(AckGraphWalkerImplTestCase):
         self.assertAck(THREE, b'continue')
 
         self.assertNextEquals(None)
+        self.assertNextEmpty()
         self.assertAck(THREE)
 
     def test_multi_ack_nak(self):
@@ -711,6 +739,7 @@ class MultiAckGraphWalkerImplTestCase(AckGraphWalkerImplTestCase):
         self.assertNoAck()
 
         self.assertNextEquals(None)
+        self.assertNextEmpty()
         self.assertNak()
 
 
@@ -723,16 +752,88 @@ class MultiAckDetailedGraphWalkerImplTestCase(AckGraphWalkerImplTestCase):
         self.assertNoAck()
 
         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')
 
+        # done is read.
+        self._walker.wants_satisified = True
         self.assertNextEquals(None)
-        self.assertAck(THREE)
+        self._walker.lines.append((None, None))
+        self.assertNextEmpty()
+        self.assertAcks([(THREE, b'ready'), (None, b'nak'), (THREE, b'')])
+        # PACK is sent
+        self.assertTrue(self._walker.pack_sent)
+
+    def test_multi_ack_nodone(self):
+        self._walker.done_required = False
+        self.assertNextEquals(TWO)
+        self.assertNoAck()
+
+        self.assertNextEquals(ONE)
+        self._impl.ack(ONE)
+        self.assertAck(ONE, b'common')
+
+        self.assertNextEquals(THREE)
+        self._impl.ack(THREE)
+        self.assertAck(THREE, b'common')
+
+        # done is read.
+        self._walker.wants_satisified = True
+        self.assertNextEquals(None)
+        self._walker.lines.append((None, None))
+        self.assertNextEmpty()
+        self.assertAcks([(THREE, b'ready'), (None, b'nak'), (THREE, b'')])
+        # PACK is sent
+        self.assertTrue(self._walker.pack_sent)
+
+    def test_multi_ack_flush_end(self):
+        # transmission ends with a flush-pkt without a done but no-done is
+        # assumed.
+        self._walker.lines[-1] = (None, None)
+        self.assertNextEquals(TWO)
+        self.assertNoAck()
+
+        self.assertNextEquals(ONE)
+        self._impl.ack(ONE)
+        self.assertAck(ONE, b'common')
+
+        self.assertNextEquals(THREE)
+        self._impl.ack(THREE)
+        self.assertAck(THREE, b'common')
+
+        # no done is read
+        self._walker.wants_satisified = True
+        self.assertNextEmpty()
+        self.assertAcks([(THREE, b'ready'), (None, b'nak')])
+        # PACK is NOT sent
+        self.assertFalse(self._walker.pack_sent)
+
+    def test_multi_ack_flush_end_nodone(self):
+        # transmission ends with a flush-pkt without a done but no-done is
+        # assumed.
+        self._walker.lines[-1] = (None, None)
+        self._walker.done_required = False
+        self.assertNextEquals(TWO)
+        self.assertNoAck()
+
+        self.assertNextEquals(ONE)
+        self._impl.ack(ONE)
+        self.assertAck(ONE, b'common')
+
+        self.assertNextEquals(THREE)
+        self._impl.ack(THREE)
+        self.assertAck(THREE, b'common')
+
+        # no done is read, but pretend it is (last 'ACK 'commit_id' '')
+        self._walker.wants_satisified = True
+        self.assertNextEmpty()
+        self.assertAcks([(THREE, b'ready'), (None, b'nak'), (THREE, b'')])
+        # PACK is sent
+        self.assertTrue(self._walker.pack_sent)
 
     def test_multi_ack_partial(self):
         self.assertNextEquals(TWO)
@@ -746,7 +847,7 @@ class MultiAckDetailedGraphWalkerImplTestCase(AckGraphWalkerImplTestCase):
         self.assertNoAck()
 
         self.assertNextEquals(None)
-        # done, re-send ack of last common
+        self.assertNextEmpty()
         self.assertAck(ONE)
 
     def test_multi_ack_flush(self):
@@ -757,6 +858,7 @@ class MultiAckDetailedGraphWalkerImplTestCase(AckGraphWalkerImplTestCase):
           (b'have', ONE),
           (b'have', THREE),
           (b'done', None),
+          (None, None),
           ]
         self.assertNextEquals(TWO)
         self.assertNoAck()
@@ -764,16 +866,17 @@ class MultiAckDetailedGraphWalkerImplTestCase(AckGraphWalkerImplTestCase):
         self.assertNextEquals(ONE)
         self.assertNak()  # nak the flush-pkt
 
-        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._walker.wants_satisified = True
         self.assertNextEquals(None)
-        self.assertAck(THREE)
+        self.assertNextEmpty()
+        self.assertAcks([(THREE, b'ready'), (None, b'nak'), (THREE, b'')])
 
     def test_multi_ack_nak(self):
         self.assertNextEquals(TWO)
@@ -785,8 +888,31 @@ class MultiAckDetailedGraphWalkerImplTestCase(AckGraphWalkerImplTestCase):
         self.assertNextEquals(THREE)
         self.assertNoAck()
 
+        # Done is sent here.
+        self.assertNextEquals(None)
+        self.assertNextEmpty()
+        self.assertNak()
+        self.assertNextEmpty()
+        self.assertTrue(self._walker.pack_sent)
+
+    def test_multi_ack_nak_nodone(self):
+        self._walker.done_required = False
+        self.assertNextEquals(TWO)
+        self.assertNoAck()
+
+        self.assertNextEquals(ONE)
+        self.assertNoAck()
+
+        self.assertNextEquals(THREE)
+        self.assertNoAck()
+
+        # Done is sent here.
+        self.assertFalse(self._walker.pack_sent)
         self.assertNextEquals(None)
+        self.assertNextEmpty()
+        self.assertTrue(self._walker.pack_sent)
         self.assertNak()
+        self.assertNextEmpty()
 
     def test_multi_ack_nak_flush(self):
         # same as nak test but contains a flush-pkt in the middle
@@ -807,6 +933,7 @@ class MultiAckDetailedGraphWalkerImplTestCase(AckGraphWalkerImplTestCase):
         self.assertNoAck()
 
         self.assertNextEquals(None)
+        self.assertNextEmpty()
         self.assertNak()
 
     def test_multi_ack_stateless(self):
@@ -823,9 +950,38 @@ class MultiAckDetailedGraphWalkerImplTestCase(AckGraphWalkerImplTestCase):
         self.assertNextEquals(THREE)
         self.assertNoAck()
 
+        self.assertFalse(self._walker.pack_sent)
+        self.assertNextEquals(None)
+        self.assertNak()
+
+        self.assertNextEmpty()
+        self.assertNoAck()
+        self.assertFalse(self._walker.pack_sent)
+
+    def test_multi_ack_stateless_nodone(self):
+        self._walker.done_required = False
+        # transmission ends with a flush-pkt
+        self._walker.lines[-1] = (None, None)
+        self._walker.http_req = True
+
+        self.assertNextEquals(TWO)
+        self.assertNoAck()
+
+        self.assertNextEquals(ONE)
+        self.assertNoAck()
+
+        self.assertNextEquals(THREE)
+        self.assertNoAck()
+
+        self.assertFalse(self._walker.pack_sent)
         self.assertNextEquals(None)
         self.assertNak()
 
+        self.assertNextEmpty()
+        self.assertNoAck()
+        # PACK will still not be sent.
+        self.assertFalse(self._walker.pack_sent)
+
 
 class FileSystemBackendTests(TestCase):
     """Tests for FileSystemBackend."""