On Wed, Aug 17, 2011 at 01:03:09PM +0200, Bert Huijben wrote:
> > +static svn_error_t *
> > +revert_restore_handle_copied_file(svn_node_kind_t *new_kind,
> > + svn_wc__db_t *db,
> > + const char *local_abspath,
> > + svn_filesize_t filesize,
> > + const svn_checksum_t *pristine_checksum,
> > + apr_pool_t *scratch_pool)
> > +{
> > + svn_stream_t *pristine_stream;
> > + svn_filesize_t pristine_size;
> > + svn_boolean_t has_text_mods;
> > +
> > + if (new_kind)
> > + *new_kind = svn_node_file;
> > +
> > + SVN_ERR(svn_wc__db_pristine_read(&pristine_stream, &pristine_size,
> > + db, local_abspath, pristine_checksum,
> > + scratch_pool, scratch_pool));
> > + SVN_ERR(svn_wc__compare_file_with_pristine(&has_text_mods, db,
> > + local_abspath,
> > + filesize,
> > + pristine_stream,
> > + pristine_size,
> > + FALSE, FALSE, FALSE,
> > + scratch_pool));
> > +
> > + if (!has_text_mods)
> > + {
> > + SVN_ERR(svn_io_remove_file2(local_abspath, FALSE, scratch_pool));
> > + if (new_kind)
> > + *new_kind = svn_node_none;
> > + }
> > +
> > + return SVN_NO_ERROR;
> > +}
>
> Why do you reimplement svn_wc__internal_file_modified_p() here without the timestamp optimization?
> Is the recorded timestamp + size already lost?
> Or can we move this check a bit to allow using the existing functions?
The file has already been nuked from the DB at this point.
It is unversioned. The existing svn_wc__internal_file_modified_p()
implementation doesn't like that.
I see your point about the timestamp check. However, Julian's comment at
http://subversion.tigris.org/issues/show_bug.cgi?id=3101#desc6
got me convinced that we should just nuke the copied files, modified or not.
Revert is _supposed_ to destroy local mods, and tip-toeing around this
just because the file was copied is silly.
So this entire function will be replaced by svn_io_remove_file2().
> svn revert is commonly used to resolve missing file problems. I assume this code will never be used in this case?
No, it's only run on unversioned files which were copied before being
reverted.
> > + err = svn_io_stat(&finfo, child_info->abspath,
> > + APR_FINFO_TYPE | APR_FINFO_SIZE,
> > + iterpool);
> > + if (err && (APR_STATUS_IS_ENOENT(err->apr_err)
> > + || SVN__APR_STATUS_IS_ENOTDIR(err->apr_err)))
> > + {
> > + svn_error_clear(err);
> > + continue;
> > + }
> > + else
> > + SVN_ERR(err);
> > +
> > + if (finfo.filetype != APR_REG)
> > + continue;
>
> Using svn_io_stat_dirent would make this code much easier (and you could probably forward the dirent)
Thanks, I'll look into that.
> > + /* Try to delete every child directory, ignoring errors.
> > + * This is a bit crude but good enough for our purposes.
> > + *
> > + * We cannot delete children recursively since we want to keep any files
> > + * that still exist on disk. So sort the children list such that longest
> > + * paths come first and try to remove each child directory in order. */
> > + qsort(copied_children->elts, copied_children->nelts,
> > + sizeof(svn_wc__db_revert_list_copied_child_info_t *),
> > + compare_revert_list_copied_children);
> > + for (i = 0; i < copied_children->nelts; i++)
> > + {
> > + child_info = APR_ARRAY_IDX(
> > + copied_children, i,
> > + svn_wc__db_revert_list_copied_child_info_t *);
> > +
> > + if (cancel_func)
> > + SVN_ERR(cancel_func(cancel_baton));
> > +
> > + if (child_info->kind != svn_wc__db_kind_dir)
> > + continue;
> > +
> > + svn_pool_clear(iterpool);
> > +
> > + svn_error_clear(svn_io_dir_remove_nonrecursive(child_info->abspath,
> > + iterpool));
>
> Please handle explicit errors instead of ignoring all errors.
>
> It is not nice that your operating system warns you that your filesystem is broken while the client just goes on as if nothing happened.
> (Common problem in Subversion 1.6 on Windows 7 pre servicepack 1).
OK, will do.
> > + if (remove_self)
> > + {
> > + apr_hash_t *remaining_children;
> > +
> > + /* Delete LOCAL_ABSPATH itself if no children are left. */
> > + SVN_ERR(svn_io_get_dirents3(&remaining_children, local_abspath,
> > TRUE,
> > + iterpool, iterpool));
> > + if (apr_hash_count(remaining_children) == 0)
> > + {
> > + SVN_ERR(svn_io_remove_dir2(local_abspath, FALSE, NULL, NULL,
> > + iterpool));
> > + if (new_kind)
> > + *new_kind = svn_node_none;
> > + }
>
> This is an inefficient reimplementation of svn_io_dir_empty() and a call to svn_io_dir_remove_nonrecursive() would handle this check for you.
Right, a simple call to svn_io_dir_remove_nonrecursive() will do. Thanks!
Received on 2011-08-17 15:31:38 CEST