On 27 Sep 2005 09:11:19 -0500, kfogel@collab.net <kfogel@collab.net> wrote:
> dionisos@tigris.org writes:
> > Log:
> > Fix merge of added file where cwd is not on the same device as the target wc.
> >
> > * subversion/libsvn_wc/update_editor.c (install_file): Try copying the file
> > when moving fails.
> >
> > --- trunk/subversion/libsvn_wc/update_editor.c
> > +++ trunk/subversion/libsvn_wc/update_editor.c
> > @@ -1943,10 +1943,17 @@
> > pointing to parent_dir/.svn/tmp/text-base/basename. */
> > if (strcmp (final_location, new_text_path))
> > {
> > - SVN_ERR_W (svn_io_file_rename (new_text_path, final_location,
> > - pool),
> > - _("Move failed"));
> > + svn_error_t *err =
> > + svn_io_file_rename (new_text_path, final_location, pool);
> >
> > + if (err)
> > + {
> > + svn_error_clear (err);
> > + SVN_ERR_W (svn_io_copy_file (new_text_path,
> > + final_location, TRUE, pool),
> > + _("Move failed"));
> > + SVN_ERR (svn_io_remove_file (new_text_path, pool));
> > + }
> > new_text_path = final_location;
> > }
> > }
>
> Do we want to check that 'err' is the kind of error we expect (e.g.,
> from a device-crossing failure, or lack of perms), before clearing it
> and going on to the copy-delete method?
Yes, we want to be sure it's only the cross device rename which fails.
I'm working on a patch.
> Now that the move takes place across two calls instead of one, what's
> the justification for using SVN_ERR_W() for the first but SVN_ERR()
> for the second? Also, The "Move failed" message on svn_io_copy_file()
> is less appropriate than it was on svn_io_file_rename()... Maybe it
> would make sense to write two custom wrapping messages, each
> indicating that one half of a non-atomic move has failed, and use
> SVN_ERR_W() for both?
Hmm. Could be. I chose to remove both SVN_ERR_W()'s. That's probably
less informative though... I'll add them.
> Finally, although this might be premature generalization: maybe it
> would be better to wrap the new functionality in a new API,
> svn_io_rename_insist() or something, so that we don't reimplement this
> logic in other places? At least, I know that I won't remember to look
> back here in update_editor.c if this ever comes up again...
Ok. How about the patch below?
BTW: I think it's better to fix diff/merge to use the working copy
.svn/tmp area to store temp files... That way there would be no exdev
renames.
bye,
Erik.
Index: subversion/libsvn_subr/io.c
===================================================================
--- subversion/libsvn_subr/io.c (revision 16265)
+++ subversion/libsvn_subr/io.c (working copy)
@@ -2425,6 +2425,47 @@
}
+svn_error_t *
+svn_io_file_rename_ensure (const char *from_path, const char *to_path,
+ apr_pool_t *pool)
+{
+ svn_error_t *err = svn_io_file_rename (from_path, to_path, pool);
+
+ if (err && APR_STATUS_IS_EXDEV (err->apr_err))
+ {
+ svn_error_clear (err);
+
+ err = svn_io_copy_file (from_path, to_path, TRUE, pool);
+ if (err)
+ {
+ svn_error_clear (svn_io_remove_file (to_path));
+ return err;
+ }
+
+#ifdef WIN32
+ /* Windows won't let us delete a file marked as read-only,
+ so, mark as read+write */
+ err = svn_io_set_file_read_write (from_path, FALSE, pool);
+ if (err)
+ goto failed;
+#endif
+
+ err = svn_io_remove_file (from_path, pool);
+ if (err)
+ goto failed;
+
+ return SVN_NO_ERROR;
+
+ failed:
+#ifdef WIN32
+ svn_error_clear (svn_io_set_file_read_write (to_path, FALSE, pool));
+#endif
+ svn_error_clear (svn_io_remove_file (to_path));
+ }
+
+ return err;
+}
+
/* Common implementation of svn_io_dir_make and svn_io_dir_make_hidden.
HIDDEN determines if the hidden attribute
should be set on the newly created directory. */
Index: subversion/include/svn_io.h
===================================================================
--- subversion/include/svn_io.h (revision 16265)
+++ subversion/include/svn_io.h (working copy)
@@ -899,6 +899,15 @@
apr_pool_t *pool);
+/** Wrapper for svn_io_file_rename(), which falls back to svn_io_copy if
+ * a cross device rename is encountered. @a from_path and @a to_path
+ * are utf8-encoded.
+ */
+svn_error_t *
+svn_io_file_rename_ensure (const char *from_path, const char *to_path,
+ apr_pool_t *pool);
+
+
/** Wrapper for apr_dir_make(), which see. @a path is utf8-encoded. */
svn_error_t *
svn_io_dir_make (const char *path, apr_fileperms_t perm, apr_pool_t *pool);
Received on Tue Sep 27 23:30:34 2005