On 9/28/05, Julian Foad <julianfoad@btopenworld.com> wrote:
> 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.
Ok. Will do.
> 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.)
The UTF-8-ness should probably be added. The point of documenting by
reference is that some of the APR documentation gives you a list of
error returns.
> As someone else said, it would be better if it moved the file as atomically as
> possible.
>
Ok. I thought it wasn't necesssary, but thinking about it some more, I
can see why it is.
> > 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).
But if we do that, the read-only-ness won't be copied with the file in
svn_io_copy_file(): I think it should copy attributes as complete as
possible. That's why I did it this way.
Received on Wed Sep 28 08:47:57 2005