You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Philip Martin <ph...@codematters.co.uk> on 2005/09/28 23:42:18 UTC
Re: svn commit: r16329 - in trunk/subversion: libsvn_subr libsvn_wc
dionisos@tigris.org writes:
> Author: dionisos
> Date: Wed Sep 28 15:16:46 2005
> New Revision: 16329
> * subversion/include/svn_io.h,
> subversion/libsvn_subr/io.c (svn_io_file_move): New. Function to move
> files from one path to another, even across device boundaries. Abstracted
> from the change to libsvn_wc/update_editor.c:install_file in r16293.
Some of that information should be in the header file, not the log
message.
>
> * subversion/libsvn_wc/update_editor.c (install_file): Change to use
> svn_io_file_move().
>
>
> Modified: trunk/subversion/include/svn_io.h
> Url: http://svn.collab.net/viewcvs/svn/trunk/subversion/include/svn_io.h?rev=16329&p1=trunk/subversion/include/svn_io.h&p2=trunk/subversion/include/svn_io.h&r1=16328&r2=16329
> ==============================================================================
> --- trunk/subversion/include/svn_io.h (original)
> +++ trunk/subversion/include/svn_io.h Wed Sep 28 15:16:46 2005
> @@ -899,6 +899,14 @@
> apr_pool_t *pool);
>
>
> +/** Move the file from @a from_path to @a to_path.
> + * Overwrite @a to_path if it exists.
How is one supposed to choose whether to use svn_io_file_move rather
than svn_io_file_rename?
> + */
> +svn_error_t *
> +svn_io_file_move (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);
>
> Modified: trunk/subversion/libsvn_subr/io.c
> Url: http://svn.collab.net/viewcvs/svn/trunk/subversion/libsvn_subr/io.c?rev=16329&p1=trunk/subversion/libsvn_subr/io.c&p2=trunk/subversion/libsvn_subr/io.c&r1=16328&r2=16329
> ==============================================================================
> --- trunk/subversion/libsvn_subr/io.c (original)
> +++ trunk/subversion/libsvn_subr/io.c Wed Sep 28 15:16:46 2005
> @@ -2425,6 +2425,61 @@
> }
>
>
> +svn_error_t *
> +svn_io_file_move (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))
> + {
> + const char *tmp_to_path;
> + apr_file_t *fp;
> +
> + svn_error_clear (err);
> +
> + SVN_ERR (svn_io_open_unique_file (&fp, &tmp_to_path,
> + to_path, "tmp", FALSE, pool));
> + apr_file_close (fp);
> +
> + err = svn_io_copy_file (from_path, tmp_to_path, TRUE, pool);
> + if (err)
> + goto failed_tmp;
> +
> + err = svn_io_file_rename (tmp_to_path, to_path, pool);
> + if (err)
> + goto failed_tmp;
> +
> +#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_final;
The error handling is a bit inconsistent, if the above set_read_write
fails then the following remove_file is skipped, but for the
failed_final and failed_tmp cases the remove_file is always attempted.
> +#endif
> +
> + err = svn_io_remove_file (from_path, pool);
> + if (! err)
> + return SVN_NO_ERROR;
> +
> + failed_final:
On non-Win32 systems failed_final is defined but not used, I get a gcc
warning.
> +#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;
> +
> + failed_tmp:
> +#ifdef WIN32
> + svn_error_clear (svn_io_set_file_read_write (tmp_to_path, FALSE, pool));
> +#endif
> + svn_error_clear (svn_io_remove_file (tmp_to_path, pool));
> + }
> +
> + return err;
> +}
> +
--
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r16329 - in trunk/subversion: libsvn_subr libsvn_wc
Posted by Philip Martin <ph...@codematters.co.uk>.
Erik Huelsmann <eh...@gmail.com> writes:
>> > + err = svn_io_file_rename (tmp_to_path, to_path, pool);
>> > + if (err)
>> > + goto failed_tmp;
>> > +
>> > +#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_final;
>>
>> The error handling is a bit inconsistent, if the above set_read_write
>> fails then the following remove_file is skipped, but for the
>> failed_final and failed_tmp cases the remove_file is always attempted.
>
> I hadn't thought of them as inconsistent: The former is part of the
> rename-or-copy codepath (leaving behind the from_path when it fails),
> whereas the latter is in the cleanup path trying to clean up any
> intermediate files as hard as it can.
>
> Does it need changing, as far as you're concerned?
On closer inspection I think that the WIN32 stuff should be removed
from this function since svn_io_remove_file already handles the
svn_io_set_file_read_write.
>
>> > +#endif
>> > +
>> > + err = svn_io_remove_file (from_path, pool);
>> > + if (! err)
>> > + return SVN_NO_ERROR;
>> > +
--
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r16329 - in trunk/subversion: libsvn_subr libsvn_wc
Posted by Erik Huelsmann <eh...@gmail.com>.
>
> > * subversion/include/svn_io.h,
> > subversion/libsvn_subr/io.c (svn_io_file_move): New. Function to move
> > files from one path to another, even across device boundaries. Abstracted
> > from the change to libsvn_wc/update_editor.c:install_file in r16293.
>
> Some of that information should be in the header file, not the log
> message.
Ok. Done.
> >
> > * subversion/libsvn_wc/update_editor.c (install_file): Change to use
> > svn_io_file_move().
> >
> >
> > Modified: trunk/subversion/include/svn_io.h
> > Url: http://svn.collab.net/viewcvs/svn/trunk/subversion/include/svn_io.h?rev=16329&p1=trunk/subversion/include/svn_io.h&p2=trunk/subversion/include/svn_io.h&r1=16328&r2=16329
> > ==============================================================================
> > --- trunk/subversion/include/svn_io.h (original)
> > +++ trunk/subversion/include/svn_io.h Wed Sep 28 15:16:46 2005
> > @@ -899,6 +899,14 @@
> > apr_pool_t *pool);
> >
> >
> > +/** Move the file from @a from_path to @a to_path.
> > + * Overwrite @a to_path if it exists.
>
> How is one supposed to choose whether to use svn_io_file_move rather
> than svn_io_file_rename?
I added a note to the docstring of svn_io_file_move explaining the difference.
> > + err = svn_io_copy_file (from_path, tmp_to_path, TRUE, pool);
> > + if (err)
> > + goto failed_tmp;
> > +
> > + err = svn_io_file_rename (tmp_to_path, to_path, pool);
> > + if (err)
> > + goto failed_tmp;
> > +
> > +#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_final;
>
> The error handling is a bit inconsistent, if the above set_read_write
> fails then the following remove_file is skipped, but for the
> failed_final and failed_tmp cases the remove_file is always attempted.
I hadn't thought of them as inconsistent: The former is part of the
rename-or-copy codepath (leaving behind the from_path when it fails),
whereas the latter is in the cleanup path trying to clean up any
intermediate files as hard as it can.
Does it need changing, as far as you're concerned?
> > +#endif
> > +
> > + err = svn_io_remove_file (from_path, pool);
> > + if (! err)
> > + return SVN_NO_ERROR;
> > +
> > + failed_final:
>
> On non-Win32 systems failed_final is defined but not used, I get a gcc
> warning.
dlr fixed that one.
> > +#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;
> > +
> > + failed_tmp:
> > +#ifdef WIN32
> > + svn_error_clear (svn_io_set_file_read_write (tmp_to_path, FALSE, pool));
> > +#endif
> > + svn_error_clear (svn_io_remove_file (tmp_to_path, pool));
> > + }
> > +
> > + return err;
> > +}
> > +
bye,
Erik.