Code review comment for lp:~vexo/bzr-webdav/bzr-webdav

Revision history for this message
Vincent Ladeuil (vila) wrote :

Thanks for the detailed answer.

>>>>> Reagan Sanders <email address hidden> writes:

    > I initially stumbled on the issues testing https://secure.cloudsafe.com, who
    > appear to use http://www.webdavsystem.com/server with a custom back-end. The
    > MOVE issue might be specific to Cloudsafe in this case, but I think the 200
    > on DELETE "quirk" (still within spec, just unusual) should apply to anything
    > built off of that IT HIT WebDAV frontend. (I've actually had to abandon that
    > project for the moment, as they also don't currently support the
    > PUT-with-content-range appending technique, and as I think you know, without
    > that performance is just abysmal.)

Yeah, I started bzr-webdav when it was the only way for me to host bzr
branches, I've since migrated to ssh which is more secure and performs
better.

    > With regard to the MOVE behavior, any RFC-compliant server should be totally
    > okay with us sending the Overwrite header whether it's required or not. When
    > it comes to the poorly-written servers, this is just my intuition, but it
    > seems likely that it should be much more common to make the mistake of
    > looking for that Overwrite: T header when it isn't actually required,

/me nods

    > Though, since the currently packaged version of bzr-webdav doesn't
    > work with the current package of bzr on Quantal+, I get the
    > feeling there might not be too many other users out there anyways,
    > to worry about breaking some other Exotic WebDAV implementation
    > ;).

Yeah, it's always hard to know...

    > I've tested it against apache2 with mod_dav/davfs without
    > issues.

Yup, using the lp:bzr-local-test-server plugin I did so too, injecting
an apache2 dav server into bzr test suite and running:

  bzr selftest -v -s bp.webdav -s bt.per_transport

which should be enough to cover all webdav plugin uses.

    > Unfortunately, I don't have an IIS server handy to test probably
    > the most common configuration :/.

Same here :-/

    > I'll look into adding some test-cases to attempt to exercise the
    > various server-failure code paths, since I think that's what was
    > lacking to let those slip through,

Exactly, thanks for uncovering them !

    > though I don't have much time during the workweek so you might
    > beat me to it ;).

Right, let's race :)

But to be honest, this shouldn't block landing your patch so I'll try to
do that first.

« Back to merge proposal