Erik Huelsmann wrote:
> Index: subversion/include/svn_io.h
> ===================================================================
> +/** 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);
I suggest "svn_io_file_move" (like "svn_io_file_rename") or "svn_io_move_file"
(like "svn_io_copy_file") as a better name, since "move" is what it does (in
the most efficient way it can) and it doesn't "ensure" that the file is renamed.
I suggest the following doc string, removing the ambiguity of what "which"
refers to, and talking in terms of functionality rather than implementation.
("svn_io_file_rename" is already documented in terms of "apr_file_rename", and
adding a second level of indirection is too unfriendly.)
/** Move the file @a from_path to @a to_path. Overwrite @a to_path if it
already exists. */
(Question: why do most but not all of the svn_io_... functions document that
the paths must be UTF8? Isn't that assumed, or at least shouldn't it be
documented exactly once at the top of the header file? I expect, for those
functions that are documented as a wrapper around an APR function, that it's a
reminder because it's different from an APR path. But it's bad practice to
document our interface in terms of a third-party one unless it saves a
significant amount of duplication.)
As someone else said, it would be better if it moved the file as atomically as
possible.
> Index: subversion/libsvn_subr/io.c
> ===================================================================
> +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, pool));
> + 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
If we do this (set read-write) before the copy, it simplifies the function
(removes the need for "goto") and allows an early return in this error case
(not that we should optimise for errors, but in this case the benefit is free).
> +
> + 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, pool));
> + }
> +
> + return err;
> +}
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Wed Sep 28 03:00:45 2005