client send-pack doesn't follow protocol

[milki]

According to the pack protocol for receive-pack at

https://www.kernel.org/pub/software/scm/git/docs/v1.7.1/technical/pack-protocol.txt

(0)
If the receiving end does not support delete-refs, the sending end MUST
NOT ask for delete command.

(1)
A pack-file MUST be sent if either create or update command is used,
even if the server already has all the necessary objects. In this
case the client MUST send an empty pack-file. The only time this
is likely to happen is if the client is creating
a new branch or a tag that points to an existing obj-id.

In my github

https://github.com/milki/dulwich/tree/send_pack

and the attached patches, I have tackled these two issues.

As (1) suggests, when branches are created/updated to existing
server-side objects and when tags are created to existing server-side
objects, an empty pack-file should be sent.

Currently, the code will only send a pack-file if it is not empty. This
patch adds an additional check - no deletions (aka ZERO-SHAs) in new
refs. 0001-send... will handle this case.

As (2) suggests, delete commands should never be sent if the server
doesn't support them. 0002-Remove... handles this case. Additionally, if
report-status is included, then these commands are reported as failed.
I made a design choice here to match existing implementation regarding
report-status. Existing implementation in dulwich will only
report-status if the server supports it. On the other hand, git-push(1)
will report regardless of the receiver's report-status.

If (2) looks good, I would like to change the behaviour of status-report
as well to match git-push(1). This would allow failed delete commands to
be reported when the server doesn't have delete-refs.

[milki (milki-p)]

Note: patches don't touch the http client

[Jelmer Vernooij (jelmer)]

This introduces a warning in the testsuite for me:

======================================================================
ERROR: test_send_remove_branch (dulwich.tests.compat.test_client.DulwichTCPClientTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "dulwich/tests/compat/test_client.py", line 216, in test_send_remove_branch
    c.send_pack(self._build_path('/dest'), lambda _: sendrefs, gen_pack)
  File "dulwich/client.py", line 485, in send_pack
    progress)
  File "dulwich/client.py", line 322, in _handle_receive_pack_tail
    self._read_side_band64k_data(proto, channel_callbacks)
  File "dulwich/client.py", line 266, in _read_side_band64k_data
    for pkt in proto.read_pkt_seq():
  File "dulwich/protocol.py", line 147, in read_pkt_seq
    pkt = self.read_pkt_line()
  File "dulwich/protocol.py", line 113, in read_pkt_line
    raise GitProtocolError(e)
GitProtocolError: [Errno 104] Connection reset by peer

----------------------------------------------------------------------

[milki (milki-p)]

I'm not seeing that error. I've done the following commands with no errors/warnings:

python setup.py test
python -m unittest dulwich.tests.compat.test_client.DulwichTCPClientTest
gmake check-nocompat

$ uname -a
FreeBSD cibo.ircmylife.com 8.3-RELEASE-p3 FreeBSD 8.3-RELEASE-p3 #0: Tue Jun 12 00:39:29 UTC 2012 <email address hidden>:/usr/obj/usr/src/sys/GENERIC amd64
$ python --version
Python 2.7.3

 I've rebased my branch and fixed pep8 warnings. Can you check again?

[Jelmer Vernooij (jelmer)]

Yep, I can still reproduce it running make check:

gwenhwyvar:~/src/dulwich% make check
python setup.py build
running build
running build_py
copying dulwich/client.py -> build/lib.linux-x86_64-2.7/dulwich
copying dulwich/tests/test_client.py -> build/lib.linux-x86_64-2.7/dulwich/tests
running build_ext
running build_scripts
python setup.py build_ext -i
running build_ext
copying build/lib.linux-x86_64-2.7/dulwich/_objects.so -> dulwich
copying build/lib.linux-x86_64-2.7/dulwich/_pack.so -> dulwich
copying build/lib.linux-x86_64-2.7/dulwich/_diff_tree.so -> dulwich
PYTHONPATH=.: python -m unittest dulwich.tests.test_suite
....................................................................................................................................................s.........................................................................................................................................................................................................................................................s............................s.........................................................................................................................................................................................................................................s.....................................E........................s........
======================================================================
ERROR: test_send_remove_branch (dulwich.tests.compat.test_client.DulwichTCPClientTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "dulwich/tests/compat/test_client.py", line 216, in test_send_remove_branch
    c.send_pack(self._build_path('/dest'), lambda _: sendrefs, gen_pack)
  File "dulwich/client.py", line 461, in send_pack
    progress)
  File "dulwich/client.py", line 326, in _handle_receive_pack_tail
    self._read_side_band64k_data(proto, channel_callbacks)
  File "dulwich/client.py", line 268, in _read_side_band64k_data
    for pkt in proto.read_pkt_seq():
  File "dulwich/protocol.py", line 147, in read_pkt_seq
    pkt = self.read_pkt_line()
  File "dulwich/protocol.py", line 113, in read_pkt_line
    raise GitProtocolError(e)
GitProtocolError: [Errno 104] Connection reset by peer

----------------------------------------------------------------------
Ran 733 tests in 27.058s

FAILED (errors=1, skipped=5)
make: *** [check] Error 1

This is with just your first patch applied.

Note that this happens in the compat tests.

[milki (milki-p)]

On FreeBSD:
python -m unittest dulwich.tests.compat.test_client.DulwichTCPClientTest.test_send_remove_branch

[milki (milki-p)]

On Arch Linux:
python2 -m unittest dulwich.tests.compat.test_client.DulwichTCPClientTest.test_send_remove_branch

[milki (milki-p)]

Notice after the last 0000, FreeBSD goes through the FIN sequence, but on Arch Linux, the connection went straight to RST. This is odd -.-

[milki (milki-p)]

I finally found my errors! I had mistaken assumptions for the behaviour of the filter function and the behaviour of the client-side protocol when determining when an empty pack-file is needed.

My branch is updated and passes make check and make check-compat on linux and fbsd now.

https://github.com/milki/dulwich/compare/master...send_pack

[Jelmer Vernooij (jelmer)]

Merged, thanks!