> -----Original Message-----
> From: stsp_at_apache.org [mailto:stsp_at_apache.org]
> Sent: woensdag 17 augustus 2011 12:33
> To: commits_at_subversion.apache.org
> Subject: svn commit: r1158617 - in /subversion/trunk/subversion: libsvn_wc/
> tests/cmdline/
>
> Author: stsp
> Date: Wed Aug 17 10:33:12 2011
> New Revision: 1158617
>
> URL: http://svn.apache.org/viewvc?rev=1158617&view=rev
> Log:
> Remove reverted copied files and directories from disk, if they weren't
> modified after being copied.
>
> To achieve this we add more columns to rows in the revert list:
> the node kind, the repos_id (which is not NULL for copies),
> the op_root, and, if the node is a file, the pristine checksum.
>
> While restoring the BASE on-disk state, process nodes in the revert list
> marked as copies in a special way: Compare reverted copied files with
> their pristines, and remove the ones that match from disk. Next, remove
> any reverted copied directories which are left empty as a result.
>
> This commit addresses issues #3101 and #876, and my own annoyance at
> unversioned things left behind in my testing WCs (been testing 'svn move'
> with subsequent 'revert' a lot lately).
>
> We could take this further and remove copied files which were modified
> post-copy (after all, we also destroy textual modifications to files which
> were not copied). But that's a decision for another day.
>
> Review by: julianfoad
>
> * subversion/tests/cmdline/merge_tests.py
> (merge_away_subtrees_noninheritable_ranges): No longer need to
> remove
> unversioned files after revert.
>
> * subversion/tests/cmdline/depth_tests.py
> (excluded_path_misc_operation): No longer need to remove unversioned
> files
> after revert.
>
> * subversion/tests/cmdline/revert_tests.py
> (status_of_missing_dir_after_revert_replaced_with_history_dir): No
> longer
> need to remove unversioned files. And don't expect them in status output.
>
> * subversion/libsvn_wc/adm_ops.c
> (revert_restore_handle_copied_file, revert_restore_handle_copied_dirs,
> compare_revert_list_copied_children): New.
> (revert_restore): When reverting copies, remove their remains from disk,
> except for files which were modified after being copied, and directories
> that contain unversioned files.
>
> * subversion/libsvn_wc/wc-queries.sql
> (revert_list): Add new columns OP_DEPTH, REPOS_ID, KIND, CHECKSUM.
> Copy values from rows deleted from the NODES table into them.
> Nothing changes for actual-only nodes.
> (STMT_SELECT_REVERT_LIST): Get the new columns.
> (STMT_SELECT_REVERT_LIST_COPIED_CHILDREN): New statement which
> returns
> all copied children within a given reverted local_relpath.
>
> * subversion/libsvn_wc/wc.h
> (svn_wc__compare_file_with_pristine): Declare.
>
> * subversion/libsvn_wc/questions.c
> (compare_and_verify): Renamed to ...
> (svn_wc__compare_file_with_pristine): ... this, so the revert code can
> access this function. No modifications were made to the implementation.
> (svn_wc__internal_file_modified_p): Track above function rename.
>
> * subversion/libsvn_wc/wc_db.c
> (revert_list_read_baton): Add COPIED_HERE, KIND, and
> PRISTINE_CHECKSUM.
> (revert_list_read): Populate the new baton fields.
> (svn_wc__db_revert_list_read): Add COPIED_HERE, KIND, and
> PRISTINE_CHECKSUM
> output parameters.
> (revert_list_read_copied_children_baton): New.
> (revert_list_read_copied_children): New. Returns a list of all copied
> children which existed within a reverted subtree.
> (svn_wc__db_revert_list_read_copied_children): Exposes the
> revert_list_read_copied_children() function, wrapping it in an sqlite
> transaction.
>
> * subversion/libsvn_wc/wc_db.h
> (svn_wc__db_revert_list_read): Declare new output parameters
> COPIED_HERE,
> KIND, and PRISTINE_CHECKSUM. Update docstring.
> (svn_wc__db_revert_list_read_copied_children): Declare.
>
> Modified:
> subversion/trunk/subversion/libsvn_wc/adm_ops.c
> subversion/trunk/subversion/libsvn_wc/questions.c
> subversion/trunk/subversion/libsvn_wc/wc-queries.sql
> subversion/trunk/subversion/libsvn_wc/wc.h
> subversion/trunk/subversion/libsvn_wc/wc_db.c
> subversion/trunk/subversion/libsvn_wc/wc_db.h
> subversion/trunk/subversion/tests/cmdline/depth_tests.py
> subversion/trunk/subversion/tests/cmdline/merge_tests.py
> subversion/trunk/subversion/tests/cmdline/revert_tests.py
>
> Modified: subversion/trunk/subversion/libsvn_wc/adm_ops.c
> URL:
> http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_wc/adm
> _ops.c?rev=1158617&r1=1158616&r2=1158617&view=diff
> ==========================================================
> ====================
> --- subversion/trunk/subversion/libsvn_wc/adm_ops.c (original)
> +++ subversion/trunk/subversion/libsvn_wc/adm_ops.c Wed Aug 17
> 10:33:12 2011
> @@ -29,6 +29,7 @@
>
>
> #include <string.h>
> +#include <stdlib.h>
>
> #include <apr_pools.h>
> #include <apr_tables.h>
> @@ -1291,6 +1292,186 @@ remove_conflict_file(svn_boolean_t *noti
> }
>
>
> +/* Remove the reverted copied file at LOCAL_ABSPATH from disk if it was
> + * not modified after being copied. Use FILESIZE and PRISTINE_CHECKSUM
> to
> + * detect modification.
> + * If NEW_KIND is not NULL, return the resulting on-disk kind of the node
> + * at LOCAL_ABSPATH in *NEW_KIND. */
> +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?
(compare_and_verify usually accesses properties so I would assume the information is not lost, or your pristine check fails on some conditions like svn:eol-style native)
svn revert is commonly used to resolve missing file problems. I assume this code will never be used in this case?
> +
> +
> +/* Sort copied children obtained from the revert list based on
> + * their paths in descending order (longest paths first). */
> +static int
> +compare_revert_list_copied_children(const void *a, const void *b)
> +{
> + const svn_wc__db_revert_list_copied_child_info_t *ca = a;
> + const svn_wc__db_revert_list_copied_child_info_t *cb = b;
> + int i;
> +
> + i = svn_path_compare_paths(ca->abspath, cb->abspath);
> +
> + /* Reverse the result of svn_path_compare_paths() to achieve
> + * descending order. */
> + return -i;
> +}
> +
> +
> +/* Remove as many reverted copied children as possible from the directory
> at
> + * LOCAL_ABSPATH. If REMOVE_SELF is TRUE, also remove LOCAL_ABSPATH
> itself
> + * if possible (REMOVE_SELF should be set if LOCAL_ABSPATH is itself a
> + * reverted copy).
> + *
> + * All reverted copied file children not modified after being copied are
> + * removed from disk. Reverted copied directories left empty as a result
> + * are also removed from disk.
> + *
> + * If NEW_KIND is not NULL, return the resulting on-disk kind of the node
> + * at LOCAL_ABSPATH in *NEW_KIND. */
> +static svn_error_t *
> +revert_restore_handle_copied_dirs(svn_node_kind_t *new_kind,
> + svn_wc__db_t *db,
> + const char *local_abspath,
> + svn_boolean_t remove_self,
> + svn_cancel_func_t cancel_func,
> + void *cancel_baton,
> + apr_pool_t *scratch_pool)
> +{
> + const apr_array_header_t *copied_children;
> + svn_wc__db_revert_list_copied_child_info_t *child_info;
> + int i;
> + apr_finfo_t finfo;
> + apr_pool_t *iterpool;
> + svn_error_t *err;
> +
> + if (new_kind)
> + *new_kind = svn_node_dir;
> +
> +
> SVN_ERR(svn_wc__db_revert_list_read_copied_children(&copied_children,
> + db, local_abspath,
> + scratch_pool,
> + scratch_pool));
> + iterpool = svn_pool_create(scratch_pool);
> +
> + /* Remove all file children, if possible. */
> + 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_file)
> + continue;
> +
> + svn_pool_clear(iterpool);
> +
> + 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)
> +
> + SVN_ERR(revert_restore_handle_copied_file(NULL, db, child_info-
> >abspath,
> + finfo.size,
> + child_info->pristine_checksum,
> + iterpool));
> + }
> +
> + /* 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).
> + }
> +
> + 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.
> + }
> +
> + svn_pool_destroy(iterpool);
> +
> + return SVN_NO_ERROR;
> +}
> +
> +
> /* Make the working tree under LOCAL_ABSPATH to depth DEPTH match
> the
> versioned tree. This function is called after svn_wc__db_op_revert
> has done the database revert and created the revert list. Notifies
> @@ -1321,6 +1502,9 @@ revert_restore(svn_wc__db_t *db,
> #ifdef HAVE_SYMLINK
> svn_boolean_t special;
> #endif
> + svn_boolean_t copied_here;
> + svn_wc__db_kind_t reverted_kind;
> + const svn_checksum_t *pristine_checksum;
>
> if (cancel_func)
> SVN_ERR(cancel_func(cancel_baton));
> @@ -1328,6 +1512,8 @@ revert_restore(svn_wc__db_t *db,
> SVN_ERR(svn_wc__db_revert_list_read(¬ify_required,
> &conflict_old, &conflict_new,
> &conflict_working, &prop_reject,
> + &copied_here, &reverted_kind,
> + &pristine_checksum,
> db, local_abspath,
> scratch_pool, scratch_pool));
>
> @@ -1342,17 +1528,21 @@ revert_restore(svn_wc__db_t *db,
> {
> svn_error_clear(err);
>
> - if (notify_func && notify_required)
> - notify_func(notify_baton,
> - svn_wc_create_notify(local_abspath, svn_wc_notify_revert,
> - scratch_pool),
> - scratch_pool);
> -
> - if (notify_func)
> - SVN_ERR(svn_wc__db_revert_list_notify(notify_func, notify_baton,
> - db, local_abspath,
> - scratch_pool));
> - return SVN_NO_ERROR;
> + if (!copied_here)
> + {
> + if (notify_func && notify_required)
> + notify_func(notify_baton,
> + svn_wc_create_notify(local_abspath,
> + svn_wc_notify_revert,
> + scratch_pool),
> + scratch_pool);
> +
> + if (notify_func)
> + SVN_ERR(svn_wc__db_revert_list_notify(notify_func, notify_baton,
> + db, local_abspath,
> + scratch_pool));
> + return SVN_NO_ERROR;
> + }
> }
> else if (err)
> return svn_error_trace(err);
> @@ -1387,6 +1577,21 @@ revert_restore(svn_wc__db_t *db,
> #endif
> }
>
> + if (copied_here)
> + {
> + /* The revert target itself is the op-root of a copy. */
> + if (reverted_kind == svn_wc__db_kind_file && on_disk ==
> svn_node_file)
> + SVN_ERR(revert_restore_handle_copied_file(&on_disk, db,
> local_abspath,
> + finfo.size,
> + pristine_checksum,
> + scratch_pool));
> + else if (reverted_kind == svn_wc__db_kind_dir && on_disk ==
> svn_node_dir)
> + SVN_ERR(revert_restore_handle_copied_dirs(&on_disk, db,
> local_abspath,
> + TRUE,
> + cancel_func, cancel_baton,
> + scratch_pool));
> + }
> +
> /* If we expect a versioned item to be present then check that any
> item on disk matches the versioned item, if it doesn't match then
> fix it or delete it. */
> @@ -1567,6 +1772,10 @@ revert_restore(svn_wc__db_t *db,
> const apr_array_header_t *children;
> int i;
>
> + SVN_ERR(revert_restore_handle_copied_dirs(NULL, db, local_abspath,
> FALSE,
> + cancel_func, cancel_baton,
> + iterpool));
> +
> SVN_ERR(svn_wc__db_read_children_of_working_node(&children, db,
> local_abspath,
> scratch_pool,
>
> Modified: subversion/trunk/subversion/libsvn_wc/questions.c
> URL:
> http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_wc/ques
> tions.c?rev=1158617&r1=1158616&r2=1158617&view=diff
> ==========================================================
> ====================
> --- subversion/trunk/subversion/libsvn_wc/questions.c (original)
> +++ subversion/trunk/subversion/libsvn_wc/questions.c Wed Aug 17
> 10:33:12 2011
> @@ -79,38 +79,17 @@
> */
>
>
> -/* Set *MODIFIED_P to TRUE if (after translation)
> VERSIONED_FILE_ABSPATH
> - * (of VERSIONED_FILE_SIZE bytes) differs from PRISTINE_STREAM (of
> - * PRISTINE_SIZE bytes), else to FALSE if not.
> - *
> - * If EXACT_COMPARISON is FALSE, translate VERSIONED_FILE_ABSPATH's
> EOL
> - * style and keywords to repository-normal form according to its properties,
> - * and compare the result with PRISTINE_STREAM. If EXACT_COMPARISON
> is
> - * TRUE, translate PRISTINE_STREAM's EOL style and keywords to working-
> copy
> - * form according to VERSIONED_FILE_ABSPATH's properties, and compare
> the
> - * result with VERSIONED_FILE_ABSPATH.
> - *
> - * HAS_PROPS should be TRUE if the file had properties when it was not
> - * modified, otherwise FALSE.
> - *
> - * PROPS_MOD should be TRUE if the file's properties have been changed,
> - * otherwise FALSE.
> - *
> - * PRISTINE_STREAM will be closed before a successful return.
> - *
> - * DB is a wc_db; use SCRATCH_POOL for temporary allocation.
> - */
> -static svn_error_t *
> -compare_and_verify(svn_boolean_t *modified_p,
> - svn_wc__db_t *db,
> - const char *versioned_file_abspath,
> - svn_filesize_t versioned_file_size,
> - svn_stream_t *pristine_stream,
> - svn_filesize_t pristine_size,
> - svn_boolean_t has_props,
> - svn_boolean_t props_mod,
> - svn_boolean_t exact_comparison,
> - apr_pool_t *scratch_pool)
> +svn_error_t *
> +svn_wc__compare_file_with_pristine(svn_boolean_t *modified_p,
> + svn_wc__db_t *db,
> + const char *versioned_file_abspath,
> + svn_filesize_t versioned_file_size,
> + svn_stream_t *pristine_stream,
> + svn_filesize_t pristine_size,
> + svn_boolean_t has_props,
> + svn_boolean_t props_mod,
> + svn_boolean_t exact_comparison,
> + apr_pool_t *scratch_pool)
> {
> svn_boolean_t same;
> svn_subst_eol_style_t eol_style;
> @@ -337,12 +316,12 @@ svn_wc__internal_file_modified_p(svn_boo
> /* Check all bytes, and verify checksum if requested. */
> {
> svn_error_t *err;
> - err = compare_and_verify(modified_p, db,
> - local_abspath, dirent->filesize,
> - pristine_stream, pristine_size,
> - has_props, props_mod,
> - exact_comparison,
> - scratch_pool);
> + err = svn_wc__compare_file_with_pristine(modified_p, db,
> + local_abspath, dirent->filesize,
> + pristine_stream, pristine_size,
> + has_props, props_mod,
> + exact_comparison,
> + scratch_pool);
>
> /* At this point we already opened the pristine file, so we know that
> the access denied applies to the working copy path */
>
> Modified: subversion/trunk/subversion/libsvn_wc/wc-queries.sql
> URL:
> http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_wc/wc-
> queries.sql?rev=1158617&r1=1158616&r2=1158617&view=diff
> ==========================================================
> ====================
> --- subversion/trunk/subversion/libsvn_wc/wc-queries.sql (original)
> +++ subversion/trunk/subversion/libsvn_wc/wc-queries.sql Wed Aug 17
> 10:33:12 2011
> @@ -1112,14 +1112,20 @@ CREATE TEMPORARY TABLE revert_list (
> conflict_working TEXT,
> prop_reject TEXT,
> notify INTEGER, /* 1 if an actual row had props or tree conflict */
> + op_depth INTEGER,
> + repos_id INTEGER,
> + kind TEXT,
> + checksum TEXT,
> PRIMARY KEY (local_relpath, actual)
> );
> DROP TRIGGER IF EXISTS trigger_revert_list_nodes;
> CREATE TEMPORARY TRIGGER trigger_revert_list_nodes
> BEFORE DELETE ON nodes
> BEGIN
> - INSERT OR REPLACE INTO revert_list(local_relpath, actual)
> - SELECT OLD.local_relpath, 0;
> + INSERT OR REPLACE INTO revert_list(local_relpath, actual, op_depth,
> + repos_id, kind, checksum)
> + SELECT OLD.local_relpath, 0, OLD.op_depth, OLD.repos_id, OLD.kind,
> + OLD.checksum;
> END;
> DROP TRIGGER IF EXISTS trigger_revert_list_actual_delete;
> CREATE TEMPORARY TRIGGER trigger_revert_list_actual_delete
> @@ -1156,11 +1162,20 @@ DROP TRIGGER IF EXISTS trigger_revert_li
> DROP TRIGGER IF EXISTS trigger_revert_list_actual_update
>
> -- STMT_SELECT_REVERT_LIST
> -SELECT conflict_old, conflict_new, conflict_working, prop_reject, notify,
> actual
> +SELECT conflict_old, conflict_new, conflict_working, prop_reject, notify,
> + actual, op_depth, repos_id, kind, checksum
> FROM revert_list
> WHERE local_relpath = ?1
> ORDER BY actual DESC
>
> +-- STMT_SELECT_REVERT_LIST_COPIED_CHILDREN
> +SELECT local_relpath, kind, checksum
> +FROM revert_list
> +WHERE local_relpath LIKE ?1 ESCAPE '#'
> + AND op_depth >= ?2
> + AND repos_id IS NOT NULL
> +ORDER BY local_relpath
> +
> -- STMT_DELETE_REVERT_LIST
> DELETE FROM revert_list WHERE local_relpath = ?1
>
>
> Modified: subversion/trunk/subversion/libsvn_wc/wc.h
> URL:
> http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_wc/wc.h
> ?rev=1158617&r1=1158616&r2=1158617&view=diff
> ==========================================================
> ====================
> --- subversion/trunk/subversion/libsvn_wc/wc.h (original)
> +++ subversion/trunk/subversion/libsvn_wc/wc.h Wed Aug 17 10:33:12 2011
> @@ -725,6 +725,39 @@ svn_wc__perform_file_merge(svn_skel_t **
> apr_pool_t *result_pool,
> apr_pool_t *scratch_pool);
>
> +/* Set *MODIFIED_P to TRUE if (after translation)
> VERSIONED_FILE_ABSPATH
> + * (of VERSIONED_FILE_SIZE bytes) differs from PRISTINE_STREAM (of
> + * PRISTINE_SIZE bytes), else to FALSE if not.
> + *
> + * If EXACT_COMPARISON is FALSE, translate VERSIONED_FILE_ABSPATH's
> EOL
> + * style and keywords to repository-normal form according to its properties,
> + * and compare the result with PRISTINE_STREAM. If EXACT_COMPARISON
> is
> + * TRUE, translate PRISTINE_STREAM's EOL style and keywords to working-
> copy
> + * form according to VERSIONED_FILE_ABSPATH's properties, and compare
> the
> + * result with VERSIONED_FILE_ABSPATH.
> + *
> + * HAS_PROPS should be TRUE if the file had properties when it was not
> + * modified, otherwise FALSE.
> + *
> + * PROPS_MOD should be TRUE if the file's properties have been changed,
> + * otherwise FALSE.
> + *
> + * PRISTINE_STREAM will be closed before a successful return.
> + *
> + * DB is a wc_db; use SCRATCH_POOL for temporary allocation.
> + */
> +svn_error_t *
> +svn_wc__compare_file_with_pristine(svn_boolean_t *modified_p,
> + svn_wc__db_t *db,
> + const char *versioned_file_abspath,
> + svn_filesize_t versioned_file_size,
> + svn_stream_t *pristine_stream,
> + svn_filesize_t pristine_size,
> + svn_boolean_t has_props,
> + svn_boolean_t props_mod,
> + svn_boolean_t exact_comparison,
> + apr_pool_t *scratch_pool);
> +
>
> #ifdef __cplusplus
> }
>
<snip>
Bert
Received on 2011-08-17 13:03:49 CEST