Re: [PATCH] 312707: moving a file or folder onto itself destroys it

[Alexander Larsson <alexl redhat com>]

On Thu, 2005-08-25 at 12:38 +0200, Alexander Larsson wrote:
> On Sat, 2005-08-06 at 21:51 +0300, Tuukka Tolvanen wrote:
> > Tuukka Tolvanen wrote:
> > 
> > > The attached patch is not quite a correct fix -- when move+replacing 
> > > identical but separate hardlinks it fails to remove the source.
> > 
> > bah. I threw another candidate on bugzilla that fixes /that/ issue:
> > - check for directory entry name match, then
> > - check for directory entry namespace match (same parent fileinfo)
> > 
> > http://bugzilla.gnome.org/show_bug.cgi?id=312707#c3
> 
> Hmmm. bugzilla down atm...
> 
> > ...but then realized it's not the right solution either -- on e.g. ftp, 
> > we don't get any decent identifying information in file_info so the 
> > same-parent-folder test would be susceptible to false positives.
> > 
> > Besides, the same filesystem could be reachable via multiple methods at 
> > the same time anyhow, which renders fs entry identity comparisons pretty 
> > academic. Temp-renaming targets that are in the way, instead of 
> > deleting, seems to be the only way to really cover all the cases, that I 
> > can think of. Does that sound right?
> 
> I dislike both the initial fix and doing temp renaming. They introduce
> even more stats and other i/o into an already slow gnome_vfs_xfer. Also,
> such solutions sounds inherently racy.
> 
> The kernel allows a move (rename) from parent2/folder to parent1/folder.
> It just returns success and does nothing. I wonder why a gnome-vfs-xfer
> removes the file. Shouldn't it just end up being a gnome_vfs_move (since
> the files are on the same filesystem), which shouldn't remove the file? 

Damn, I tried this out (see attached not-very-tested patch), but it
didn't work out. The problem is that we on name conflicts of directory
moves, if you pick "replace" it actually switches into a merge, which is
done by changing from a move operation to a recursive copy + remove.

:(

Not sure what a good solution is here.

=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
 Alexander Larsson                                            Red Hat, Inc 
                   alexl redhat com    alla lysator liu se 
He's a world-famous overambitious firefighter on his last day in the job. 
She's a virginal tempestuous wrestler with an MBA from Harvard. They fight 
crime! 

Index: libgnomevfs/gnome-vfs-xfer.c
===================================================================
RCS file: /cvs/gnome/gnome-vfs/libgnomevfs/gnome-vfs-xfer.c,v
retrieving revision 1.125
diff -u -p -r1.125 gnome-vfs-xfer.c
--- libgnomevfs/gnome-vfs-xfer.c	8 Jul 2005 08:53:18 -0000	1.125
+++ libgnomevfs/gnome-vfs-xfer.c	25 Aug 2005 15:26:39 -0000
@@ -2325,7 +2325,7 @@ gnome_vfs_xfer_uri_internal (const GList
 	GList *source_uri_list, *target_uri_list;
 	GList *source_uri, *target_uri;
 	GList *source_uri_list_copied;
-	GList *merge_source_uri_list, *merge_target_uri_list;
+	GList *move_source_uri_list, *move_target_uri_list;
 	GnomeVFSURI *target_dir_uri;
 	gboolean move, link;
 	GnomeVFSFileSize free_bytes;
@@ -2363,28 +2363,37 @@ gnome_vfs_xfer_uri_internal (const GList
 	 */
 	source_uri_list = gnome_vfs_uri_list_copy ((GList *)source_uris);
 	target_uri_list = gnome_vfs_uri_list_copy ((GList *)target_uris);
-	merge_source_uri_list = NULL;
-	merge_target_uri_list = NULL;
+	move_source_uri_list = NULL;
+	move_target_uri_list = NULL;
 
-	if ((xfer_options & GNOME_VFS_XFER_USE_UNIQUE_NAMES) == 0) {
+	if (move &&
+	    (xfer_options & GNOME_VFS_XFER_USE_UNIQUE_NAMES) == 0) {
 		/* see if moved files are on the same file system so that we decide to do
 		 * a simple move or have to do a copy/remove
 		 */
-		for (source_uri = source_uri_list, target_uri = target_uri_list;
-			source_uri != NULL;
-			source_uri = source_uri->next, target_uri = target_uri->next) {
+		source_uri = source_uri_list;
+		target_uri = target_uri_list;
+		while (source_uri != NULL) {
+			GList *source_item_to_remove;
+			GList *target_item_to_remove;
 			gboolean same_fs;
 
 			g_assert (target_dir_uri != NULL);
 
 			result = gnome_vfs_check_same_fs_uris ((GnomeVFSURI *)source_uri->data, 
-				target_dir_uri, &same_fs);
+							       target_dir_uri, &same_fs);
 
-			if (result != GNOME_VFS_OK) {
-				break;
+			source_item_to_remove = source_uri;
+			target_item_to_remove = target_uri;
+			source_uri = source_uri->next;
+			target_uri = target_uri->next;
+
+			if (result == GNOME_VFS_OK && same_fs) {
+				move_source_uri_list = g_list_prepend (move_source_uri_list, (GnomeVFSURI *)source_item_to_remove->data);
+				move_target_uri_list = g_list_prepend (move_target_uri_list, (GnomeVFSURI *)target_item_to_remove->data);
+				source_uri_list = g_list_remove_link (source_uri_list, source_item_to_remove);
+				target_uri_list = g_list_remove_link (target_uri_list, target_item_to_remove);
 			}
-
-			move &= same_fs;
 		}
 	}
 
@@ -2393,9 +2402,18 @@ gnome_vfs_xfer_uri_internal (const GList
 		target_dir_uri = NULL;
 	}
 	
+	call_progress (progress, GNOME_VFS_XFER_PHASE_COLLECTING);
+	if (move_source_uri_list != NULL) {
+		result = count_items_and_size (move_source_uri_list, xfer_options, progress, TRUE, link);
+		if (result != GNOME_VFS_ERROR_INTERRUPTED) {
+			/* Ignore anything but interruptions here -- we will deal with the errors
+			 * during the actual copy
+			 */
+			result = GNOME_VFS_OK;
+		}
+	}
 	if (result == GNOME_VFS_OK) {
-		call_progress (progress, GNOME_VFS_XFER_PHASE_COLLECTING);
-		result = count_items_and_size (source_uri_list, xfer_options, progress, move, link);
+		result = count_items_and_size (source_uri_list, xfer_options, progress, FALSE, link);
 		if (result != GNOME_VFS_ERROR_INTERRUPTED) {
 			/* Ignore anything but interruptions here -- we will deal with the errors
 			 * during the actual copy
@@ -2408,11 +2426,13 @@ gnome_vfs_xfer_uri_internal (const GList
 		/* Calculate free space on destination. If an error is returned, we have a non-local
 		 * file system, so we just forge ahead and hope for the best 
 		 */
-		target_dir_uri = gnome_vfs_uri_get_parent ((GnomeVFSURI *)target_uri_list->data);
+
+		target_uri = (target_uri_list != NULL) ? target_uri_list : move_target_uri_list;
+		target_dir_uri = gnome_vfs_uri_get_parent ((GnomeVFSURI *)target_uri->data);
 		result = gnome_vfs_get_volume_free_space (target_dir_uri, &free_bytes);
 
 		if (result == GNOME_VFS_OK) {
-			if (!move && !link && progress->progress_info->bytes_total > free_bytes) {
+			if (!link && progress->progress_info->bytes_total > free_bytes) {
 				result = GNOME_VFS_ERROR_NO_SPACE;
 				progress_set_source_target_uris (progress, NULL, target_dir_uri);
 				handle_error (&result, progress, &error_mode, &skip);
@@ -2428,11 +2448,12 @@ gnome_vfs_xfer_uri_internal (const GList
 		}
 
 		if (result != GNOME_VFS_OK) {
-			return result;
+			goto error;
 		}
 			
 		if ((xfer_options & GNOME_VFS_XFER_USE_UNIQUE_NAMES) == 0) {
-		
+			GList *merge_source_uri_list, *merge_target_uri_list;
+			
 			/* Save the preflight numbers, handle_name_conflicts would overwrite them */
 			bytes_total = progress->progress_info->bytes_total;
 			files_total = progress->progress_info->files_total;
@@ -2443,18 +2464,31 @@ gnome_vfs_xfer_uri_internal (const GList
 			progress->progress_info->bytes_total = 0;
 			progress->progress_info->files_total = 0;
 
-			result = handle_name_conflicts (&source_uri_list, &target_uri_list,
-						        xfer_options, &error_mode, &overwrite_mode,
-						        progress,
-							move,
-							link,
-							&merge_source_uri_list,
-							&merge_target_uri_list);
+			merge_source_uri_list = NULL;
+			merge_target_uri_list = NULL;
+		
+			if (move_source_uri_list != NULL) {
+				result = handle_name_conflicts (&move_source_uri_list, &move_target_uri_list,
+								xfer_options, &error_mode, &overwrite_mode,
+								progress,
+								TRUE,
+								link,
+								&merge_source_uri_list,
+								&merge_target_uri_list);
+			}
+			if (result == GNOME_VFS_OK) {
+				result = handle_name_conflicts (&source_uri_list, &target_uri_list,
+								xfer_options, &error_mode, &overwrite_mode,
+								progress,
+								FALSE,
+								link,
+								NULL, NULL);
+			}
 
 			progress->progress_info->bytes_total = 0;
 			progress->progress_info->files_total = 0;
 			
-			if (result == GNOME_VFS_OK && move && merge_source_uri_list != NULL) {
+			if (result == GNOME_VFS_OK && merge_source_uri_list != NULL) {
 				/* Some moves was converted to copy,
 				   remove previously added non-recursive sizes,
 				   and add recursive sizes */
@@ -2488,7 +2522,7 @@ gnome_vfs_xfer_uri_internal (const GList
 					result = gnome_vfs_get_volume_free_space (target_dir_uri, &free_bytes);
 					
 					if (result == GNOME_VFS_OK) {
-						if (progress->progress_info->bytes_total > free_bytes) {
+						if (progress->progress_info->bytes_total + bytes_total > free_bytes) {
 							result = GNOME_VFS_ERROR_NO_SPACE;
 							progress_set_source_target_uris (progress, NULL, target_dir_uri);
 						}
@@ -2502,6 +2536,10 @@ gnome_vfs_xfer_uri_internal (const GList
 						target_dir_uri = NULL;
 					}
 				}
+
+				/* Now make the merge items normal copy ops */
+				source_uri_list = g_list_concat (source_uri_list, merge_source_uri_list);
+				target_uri_list = g_list_concat (target_uri_list, merge_target_uri_list);
 			}
 
 			/* Add previous size (non-copy size in the case of a move) */
@@ -2523,42 +2561,40 @@ gnome_vfs_xfer_uri_internal (const GList
 		} else {
 			call_progress (progress, GNOME_VFS_XFER_PHASE_READYTOGO);
 
-			if (move) {
-				g_assert (!link);
-				result = move_items (source_uri_list, target_uri_list,
-						     xfer_options, &error_mode, &overwrite_mode, 
-						     progress);
-				if (result == GNOME_VFS_OK && merge_source_uri_list != NULL) {
-					result = copy_items (merge_source_uri_list, merge_target_uri_list,
+			if (!link) {
+				if (move_source_uri_list != NULL) {
+					result = move_items (move_source_uri_list, move_target_uri_list,
+							     xfer_options, &error_mode, &overwrite_mode, 
+							     progress);
+				}
+				if (result == GNOME_VFS_OK && source_uri_list != NULL) {
+					result = copy_items (source_uri_list, target_uri_list,
 							     xfer_options, &error_mode, overwrite_mode, 
 							     progress, &source_uri_list_copied);
 				}
-			} else if (link) {
+				if (result == GNOME_VFS_OK &&
+				    xfer_options & GNOME_VFS_XFER_REMOVESOURCE) {
+					call_progress (progress, GNOME_VFS_XFER_PHASE_CLEANUP);
+					result = gnome_vfs_xfer_delete_items_common (source_uri_list_copied,
+										     error_mode, xfer_options, progress);
+				}
+
+			} else {
 				result = link_items (source_uri_list, target_uri_list,
 						     xfer_options, &error_mode, &overwrite_mode,
 						     progress);
-			} else {
-				result = copy_items (source_uri_list, target_uri_list,
-						     xfer_options, &error_mode, overwrite_mode, 
-						     progress, &source_uri_list_copied);
 			}
 			
-			if (result == GNOME_VFS_OK) {
-				if (xfer_options & GNOME_VFS_XFER_REMOVESOURCE
-				    && !link) {
-					call_progress (progress, GNOME_VFS_XFER_PHASE_CLEANUP);
-					result = gnome_vfs_xfer_delete_items_common ( 
-						 	source_uri_list_copied,
-						 	error_mode, xfer_options, progress);
-				}
-			}
 		}
 	}
 
+
+ error:
+	
 	gnome_vfs_uri_list_free (source_uri_list);
 	gnome_vfs_uri_list_free (target_uri_list);
-	gnome_vfs_uri_list_free (merge_source_uri_list);
-	gnome_vfs_uri_list_free (merge_target_uri_list);
+	gnome_vfs_uri_list_free (move_source_uri_list);
+	gnome_vfs_uri_list_free (move_target_uri_list);
 	gnome_vfs_uri_list_free (source_uri_list_copied);
 
 	return result;