with the change at https://go-review.googlesource.com/#/c/10942/, there's no problem anymore connecting with MiniRedir.

I agree: this is a strong indicator that your MiniRedir version indeed has a problem with namespace prefixes. If so, this must have been fixed in Windows 8 since I do not have the same issue with MiniRedir version 6.3.9600 (http://pastebin.com/X7ggpht5).

Still, I would like to see what is going over the wire. Could you please dump the HTTP traffic between the WebDAV handshake of your Windows client with the current, vanilla litmus server in the net/webdav package? The log I dumped at pastebin is from http://github.com/rsto/logproxy but you might just as well use netcat or any other reverse proxy.

I can confirm that this change makes the DAV server work with Windows 7 and Windows Server 2008 (both is Microsoft-WebDAV-MiniRedir/6.1.7601), and does not interfere with gvfs (still works with pcmanfm).

@rsto Ah right, I hadn't understood what you meant about a litmus server in the CL. Here's what happens when trying to connect:

Server-side:

% go run litmus_test_server.go
Serving :9999
OPTIONS /
OPTIONS /
PROPFIND /
PROPFIND /

From logproxy:

$ logproxy -address :8080 -target http://localhost:9999
2015/06/12 16:17:18 Forwarding from :8080 to http://localhost:9999
==> OPTIONS / HTTP/1.1
Host: 192.168.0.9:8080
Translate: f
User-Agent: DavClnt
X-Forwarded-For: 192.168.0.12

<== HTTP/1.1 200 OK
Allow: OPTIONS, LOCK, GET, HEAD, POST, DELETE, PROPPATCH, COPY, MOVE, UNLOCK, PROPFIND
Content-Type: text/plain; charset=utf-8
Date: Fri, 12 Jun 2015 14:17:31 GMT
Dav: 1, 2
Ms-Author-Via: DAV
Content-Length: 0

==> OPTIONS / HTTP/1.1
Host: 192.168.0.9:8080
Translate: f
User-Agent: Microsoft-WebDAV-MiniRedir/6.1.7601
X-Forwarded-For: 192.168.0.12

<== HTTP/1.1 200 OK
Allow: OPTIONS, LOCK, GET, HEAD, POST, DELETE, PROPPATCH, COPY, MOVE, UNLOCK, PROPFIND
Content-Type: text/plain; charset=utf-8
Date: Fri, 12 Jun 2015 14:17:37 GMT
Dav: 1, 2
Ms-Author-Via: DAV
Content-Length: 0

==> PROPFIND / HTTP/1.1
Host: 192.168.0.9:8080
Depth: 0
Translate: f
User-Agent: Microsoft-WebDAV-MiniRedir/6.1.7601
X-Forwarded-For: 192.168.0.12

<== HTTP/1.1 207 status code 207
Content-Length: 552
Content-Type: text/xml; charset=utf-8
Date: Fri, 12 Jun 2015 14:17:37 GMT

/0Fri, 12 Jun 2015 16:15:13 GMTtext/plain; charset=utf-8HTTP/1.1 200 OK

==> PROPFIND / HTTP/1.1
Host: 192.168.0.9:8080
Depth: 0
Translate: f
User-Agent: Microsoft-WebDAV-MiniRedir/6.1.7601
X-Forwarded-For: 192.168.0.12

<== HTTP/1.1 207 status code 207
Content-Length: 552
Content-Type: text/xml; charset=utf-8
Date: Fri, 12 Jun 2015 14:17:37 GMT

/text/plain; charset=utf-80Fri, 12 Jun 2015 16:15:13 GMTHTTP/1.1 200 OK

reposting logproxy in a pastebin since github ate the xml:

http://pastebin.com/q6HUGJKU

Thanks. So it just stops after the second PROPFIND attempt, whereas in version 6.3.9600 it continues to PROPFIND / with Depth 1.

I wonder: Do we have to declare namespace prefixes on all elements?

Case in point: Since MiniRedir version 6.1.7601 works with your CL it is clear that it does not expect a namespace prefix on all elements. The following, hardcoded payload was not amended in your CL (it's defined here https://github.com/golang/net/blob/master/webdav/prop.go#L380).

<lockentry xmlns="DAV:">
   <lockscope><exclusive/></lockscope>
   <locktype><write/></locktype>
</lockentry>

and yet, MiniRedir seems just to be fine with it. Maybe it only expects a namespace prefix on the multistatus root element? Or the propstat or href? Would it be possible for you to test this? (I don't have Windows 7 at hand.)

I agree that to make this work with Windows 7, fixing the namespace prefixes is the way to go forward. I just want to minimise the impact this has on the XML marshalling and testing internals; that's wobbly enough without picky clients like MiniRedir, already.

Yeah, I had noticed that I had missed the lockentry and co. ones.

I don't know for sure which ones it needs, but I'm almost sure it's more than just the multistatus one. I think I actually went the other way around when I tested, and when I had the multistatus, propstat and props already prefixed, but not the top ones in Response (such as href) yet, it was still not working.

I'll try and do a more systematic probing of that. It's a pain we don't have an exact doc of what MiniRedir wants :/

Regarding the wobblyness, that was partly the sense of my question in the CL commit msg: do you think we could only prefix the tags when we detect the user-agent matches a "faulty" MiniRedir? That way, not only do we emit "cleaner" XML in the general case, but we also wouldn't have to fix as many tests; we could just add a propfind test specific to the faulty MiniRedir case.
With the approach I took it would be very impractical, since we would have to duplicate the structs types and use one or the other depending on what the client is. But maybe there's another/better way to prefix the XML elements than what I did (i.e. a way that would not change the xml tags of the structs) ? What if we didn't touch xml.go at all, and instead try to do some prepending on the fly everywhere, like I did in prop.go and webdav.go ?

when I had the multistatus, propstat and props already prefixed, but not the top ones in Response (such as href) yet, it was still not working.

OK, so most probably it requires everything prefixed.

I'd be fine to prefix all XML elements that are in direct control of the webdav package (but not any dead properties), as long as it is valid XML. There should be no need for checking for MiniRedir versions in the runtime. I'll look again at the CL and will maybe do some own testing.

Let's wait what @nigeltao has to say on this topic.

Thanks.

ok. In the meantime I should soon be able to retest more thoroughly what needs to be prefixed and report back in a few hours.

I've just tried "enabling" prefixes one by one, and the conclusion seems to be that MinRedir simply ignores anything that isn't prefixed. So for example, if you prefix only multistatus, href and propstat in the response, which gives you something like that:

<?xml version="1.0" encoding="UTF-8"?><D:multistatus xmlns="DAV:" xmlns:D="DAV:"><D:response><D:href>/</D:href><D:propstat><prop><resourcetype><collection xmlns="DAV:"/></resourcetype><getcontentlength>0</getcontentlength><getlastmodified>Fri, 12 Jun 2015 21:35:15 GMT</getlastmodified><displayname></displayname><getcontenttype>text/plain; charset=utf-8</getcontenttype><supportedlock><lockentry xmlns="DAV:"><lockscope><exclusive/></lockscope><locktype><write/></locktype></lockentry></supportedlock></prop><status>HTTP/1.1 200 OK</status></D:propstat></D:response></D:multistatus>

you don't get any error message, and the window seems to display the network location, but you don't get any contents displayed, because none of the props are prefixed.

I've just updated the CL (patchset 2) with what I found to be the minimum for things to appear working. But of course I might be missing some piece of the story, or some detail. Like, I didn't notice their influence, but maybe we do need to prefix the "not found" properties as well? I leave it to your appreciation before starting to update the tests accordingly.

Thanks for the thorough trial and error testing. If we prefix anything to make this work with Windows 7 I'd opt for also prefixing the 404 properties.

With regards to tests: please don't currently spend effort in updating them. These "golden XML" tests turn out to be a time sink and the current issue might be the final reason to use a general XML normalisation in the package tests. There's a related CL (10827) baking, so once merged it might make sense to refactor it for use with other tests. Let's see what Nigel has to say on this.

oh I know. I often enough have to update similar golden JSON outputs and it's painful enough as it is. Those XML ones seemed even worse so I had no intention to do it until you guys said the approach in the code looked at least remotely sound :-)

What @rsto said, prefixing all XML elements that we're responsible for, SGTM if the resultant code isn't too ugly.

I'll let the two of you figure out how to manage changing behavior and making the tests less brittle.

What's the next step? Should I review https://go-review.googlesource.com/#/c/10942/ or is somebody cooking up a counter-proposal or are we waiting for @rsto to make the tests more lenient?

I'm fine either way. I can make the tests pass whenever you guys feel the CL looks reasonable enough, or I can wait for whatever @rsto has in mind.

I just submitted a CL to unify the golden XML tests [1]. If merged, it would allow to tweak the XML outputs of the property system further without breaking the current package tests.

@mpl I had a look at your CL and will comment soon. I played a bit with the latest xml package namespace feature and am somewhat puzzled that it doesn't suffice to declare the D prefix for the DAV: namespace at the start of the multistatus tag. I'll do some further testing but am inclined to reach out to Roger Peppe if he could spare a minute to comment.

[1] https://go-review.googlesource.com/#/c/11032/

@nigeltao I suggest to first look at https://go-review.googlesource.com/#/c/11032/, it should make the package tests more stable irrespective of the current issue. After that, I'd like to review https://go-review.googlesource.com/#/c/10942

I just submitted https://go-review.googlesource.com/#/c/11074 for the encoding/xml package. It leverages the namespace prefix feature of CL 2660. IIUC if this or any similar counter-proposal gets accepted we should just have to declare the namespace prefix for "DAV:" once at the top of the multistatus element.

Cool, thanks. I'll keep an eye on it and pull as soon as it gets in. When I can confirm that it does work as you say, and that it fixes the issue for MiniRedir on win 7, shall I change CL 10942 to do just that?

@mpl Let's see if CL 11074 gets a chance. I commented on your CL and marked the actions that make sense to wait for a decision on CL 11074. The other stuff is fine to update now, since it shouldn't depend on how we do the namespace prefixing, anyways. Thanks.