Created attachment 129286 [details] [review]
Implement native webdav move operation

Hmm, I don't know the dav backend very well, but I don't think this handles overwrite errors correctly. See the API docs for g_file_move, wrt to overwrites for the details of what errors should be reported.

Yes, you're right :-/

Doing simple nautilus tests, it seems that overwriting a file over a directory, or dir over file fails.

Everything else seems to be fine: Dir over dir asks for merging, file over file asks for overwriting.

I'll work on a revised patch...

Setting patch status as for comment #2 and comment #3.

I have done some more investigation into this, and would like some more advice, if someone can give any.

The server I have available (some version of Apache) does not like if you try to overwrite a file with a directory or vice versa.

Overwriting file with directory returns "400 Bad Request"
Overwriting directory with file returns "301 Moved Permanently" (possibly because it requires a trailing slash, but in any case the redirect can not be followed as no Location header is supplied...)

If someone can observe better/different behaviour, I would very much like to know.

In any case, I would recommend (since the added functionality is very useful, e.g. when moving large folders) applying the patch in its current state, with an additional comment like
/* FIXME: Does not handle overwriting file with directory or vice versa */

Any comments on this?
The patch makes it many times more pleasant to move files on WebDav shares - actually makes it usable in my case. I do not see anyone wanting to download 100Mb just to upload them again, when it should be an operation done on the server.

A further comment is that the implementation is heavily borrowed from the ftp backend - which also seems to fail to handle overwrite errors.

May I suggest that the patch be applied, and new bugs filed against the ftp and dav backend re. handling overwrites?

Comment on attachment 129286 [details] [review]
Implement native webdav move operation

Unsetting needs-work flag. Please re-set if my suggested plan is not acceptable.

Created attachment 166533 [details] [review]
Implement native webdav move (update patch to apply to HEAD)

Please, this patch is badly needed!

Review of attachment 166533 [details] [review]:

I made few more tests and found out that overwriting directory with a file results in the old directory being removed with all subfolders and files. Certainly G_IO_ERROR_IS_DIRECTORY should be returned. Same situation with overwriting a file with a directory. Also, directory vs. directory overwrite removes the old one, we should return G_IO_ERROR_WOULD_MERGE here.

I know this is more a server behaviour (or perhaps libsoup) and according to your other concerns I suggest to stat destination before doing an operation.

Note that all my tests were performed on apache2 via gnome-user-share. The behaviour may be different elsewhere but we should avoid any data loss in any case.

::: daemon/gvfsbackenddav.c
@@ +2255,3 @@
+  guint        status;
+
+  /* FIXME: what about G_FILE_COPY_NOFOLLOW_SYMLINKS and G_FILE_COPY_ALL_METADATA? */

My understanding of G_FILE_COPY_ALL_METADATA is that it does matter for copying, not for moving already existing files (which have their attributes).

For symlinks, I guess this is handled by the webserver itself (via some directives) - at least I can't see any symlinks using gnome-user-share. Neither cadaver or gvfsd-dav is showing any of them, they're just ignored.

*** Bug 727096 has been marked as a duplicate of this bug. ***

No previous comments on this issue since 2010 and it results in dataloss (e.g. version history) as per Bug 727096. Is there any likelyhood this will receive any attention?

I can try take a look at this in the next few days.

Created attachment 273033 [details] [review]
dav: Use the native MOVE operation for moving

This saves unnecessary round trips when moving files and directories.
Based on a patch by Mads Chr. Olesen and the GLocalFile implementation.

Backups are not yet implemented.  It tries to cover the various cases of
overwriting files with directories and vice versa by doing what
GLocalFile does.

Review of attachment 273033 [details] [review]:

It looks good, just few notes:

::: daemon/gvfsbackenddav.c
@@ +2740,3 @@
+                                G_IO_ERROR,
+                                G_IO_ERROR_CANT_CREATE_BACKUP,
+                                _("backups not supported yet"));

Would be better to use "Backup file creation failed" to be consistent at least in backend.

@@ +2778,3 @@
+                }
+              else if (source_ft == G_FILE_TYPE_DIRECTORY)
+                {

Is the following code needed? Overwrite header should be enough, shouldn't be?

(In reply to comment #15)
> Review of attachment 273033 [details] [review]:
> 
> It looks good, just few notes:
> 
> ::: daemon/gvfsbackenddav.c
> @@ +2740,3 @@
> +                                G_IO_ERROR,
> +                                G_IO_ERROR_CANT_CREATE_BACKUP,
> +                                _("backups not supported yet"));
> 
> Would be better to use "Backup file creation failed" to be consistent at least
> in backend.
> 
> @@ +2778,3 @@
> +                }
> +              else if (source_ft == G_FILE_TYPE_DIRECTORY)
> +                {
> 
> Is the following code needed? Overwrite header should be enough, shouldn't be?

Yes it is needed. Apache at least chokes if you try and overwrite a file with a dir:

> MOVE /dav/test/ HTTP/1.1
> Soup-Debug-Timestamp: 1396454334
> Soup-Debug: SoupSessionSync 1 (0x11e3100), SoupMessage 6 (0x7f8a500063a0), SoupSocket 3 (0x7f8a5000aa30), restarted
> Host: localhost
> Overwrite: T
> Accept-Encoding: gzip, deflate
> Accept-Language: en-us, en;q=0.9
> Connection: Keep-Alive
> Destination: http://localhost/dav/file/
> User-Agent: gvfs/1.20.1
  
< HTTP/1.1 400 Bad Request
< Soup-Debug-Timestamp: 1396454334
< Soup-Debug: SoupMessage 6 (0x7f8a500063a0)
< Date: Wed, 02 Apr 2014 15:58:54 GMT
< Server: Apache/2.4.9 (Fedora)
< Content-Length: 226
< Connection: close
< Content-Type: text/html; charset=iso-8859-1
< 
< <!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML 2.0//EN">
< <html><head>
< <title>400 Bad Request</title>
< </head><body>
< <h1>Bad Request</h1>
< <p>Your browser sent a request that this server could not understand.<br />
< </p>
< </body></html>

(In reply to comment #15)
> Review of attachment 273033 [details] [review]:
> 
> It looks good, just few notes:
> 
> ::: daemon/gvfsbackenddav.c
> @@ +2740,3 @@
> +                                G_IO_ERROR,
> +                                G_IO_ERROR_CANT_CREATE_BACKUP,
> +                                _("backups not supported yet"));
> 
> Would be better to use "Backup file creation failed" to be consistent at least
> in backend.
> 

OK, I will change that.  I was being consistent with the ftp and sftp backends :-)

(In reply to comment #16)
> (In reply to comment #15)
> > Is the following code needed? Overwrite header should be enough, shouldn't be?
> 
> Yes it is needed. Apache at least chokes if you try and overwrite a file with a

Ok then. I tried it on free public webdav provided by http://www.ajaxfilebrowser.com/ and looked like working...

(In reply to comment #17)
> (In reply to comment #15)
> > Would be better to use "Backup file creation failed" to be consistent at least
> > in backend.
> > 
> 
> OK, I will change that.  I was being consistent with the ftp and sftp backends
> :-)

Unfortunately it is quite inconsistent between backends... :-/

Pushed to master as b4387906ba3d5e73025e19245b5058e0db17c84f.  Thanks!