On Sun, Apr 04, 2010 at 01:20:09PM +0200, Stefan Sperling wrote: > On Sun, Apr 04, 2010 at 10:55:26AM -0000, dan...@apache.org wrote: > > Author: dannas > > Date: Sun Apr 4 10:55:25 2010 > > New Revision: 930662 > > > > URL: http://svn.apache.org/viewvc?rev=930662&view=rev > > Log: > > Make 'svn patch' able to remove dirs beeing empty after patching, taking > > into account both versioned and unversioned files and dirs. > > > > > + > > +/* Compare A and B and return an integer greater than, equal to, or less > > + * than 0, according to whether A has less subdirs, just as many or more > > + * subdir than B. That means that the path with the most subdirs is placed > > + * first. */ > > +static int > > +sort_compare_nr_of_path_elements(const svn_sort__item_t *a, > > + const svn_sort__item_t *b) > > +{ > > + const char *astr, *bstr; > > + int n_a = 0, n_b = 0, i; > > + > > + astr = a->key; > > + bstr = b->key; > > + > > + for (i = 0; i < a->klen; i++) > > + if (astr[i] == '/') > > + n_a++; > > Drive-by review (will look more closely later):
Looking more closely, I ended up rewriting a lot of this (I played with making small changes to make sure I understand the problem, and then I got carried away :) Below is a log message and a patch. I moved the code around, partly because this makes it easier for you to read the new implementation. I'd rather not rip out a lot of code you wrote without your consent, so please review this and let me know you what you think. Thanks, Stefan [[[ On the svn-patch-improvements branch, revamp handling of deleted directories. While not originally intended, this is almost a complete rewrite of the logic handling directory deletions. Compared to the previous implementation: - It does not modify lists in place while iterating them. - It does not add empty directories which can be deleted as 'fake' patch targets, but deletes them directly, and skips targets which were deleted together with the parent directories. - It splits the overall task into more distinct steps, mainly for clarity to the reader. * subversion/libsvn_client/patch.c (patch_target_t): New field PARENT_DIR_DELETED, to trigger special processing of targets which are within deleted directories. (init_patch_target): Initialise PARENT_DIR_DELETED. (is_dir_empty): Rename to ... (check_dir_empty): ... this, and in addition to the list of deleted targets, accept a list of directories we know will be removed. (add_target_to_hash_keyed_by_parent_dir, sort_compare_path_component_count, condense_deleted_targets): Remove. (push_if_unique): New helper function, needed because we do not use a hash table anymore to ensure path uniqueness. (delete_empty_dirs): New function, which determines which directories will be empty after patching, deletes them, and marks any targets which are children of the deleted directories. (install_patched_target): Skip targets which had their parent dir removed from above them. (apply_patches): Call delete_empty_dirs() instead of the removed condense_deleted_targets(). Do not notify about targets which were deleted together with their parent directories. * subversion/tests/cmdline/patch_tests.py (patch_remove_empty_dir): Rename to ... (patch_remove_empty_dirs): .. this, and adjust expected output because notification order has changed. (test_list): Track renamed function. ]]] Index: subversion/libsvn_client/patch.c =================================================================== --- subversion/libsvn_client/patch.c (revision 932869) +++ subversion/libsvn_client/patch.c (working copy) @@ -158,6 +158,10 @@ typedef struct { /* True if the target's parent directory exists. */ svn_boolean_t parent_dir_exists; + /* True if the target's parent directory is being deleted + * as a result of the patching process. */ + svn_boolean_t parent_dir_deleted; + /* The keywords of the target. */ apr_hash_t *keywords; @@ -516,6 +520,7 @@ init_patch_target(patch_target_t **patch_target, target->had_rejects = FALSE; target->deleted = FALSE; target->eof = FALSE; + target->parent_dir_deleted = FALSE; target->pool = result_pool; target->lines = apr_array_make(result_pool, 0, sizeof(svn_stream_mark_t *)); @@ -1234,241 +1239,6 @@ apply_one_patch(patch_target_t **patch_target, svn return SVN_NO_ERROR; } -/* Baton for find_existing_children() */ -struct status_baton -{ - apr_array_header_t *existing_targets; - const char *parent_path; - apr_pool_t *result_pool; -}; - -/* Implements svn_wc_status_func4_t. */ -static svn_error_t * -find_existing_children(void *baton, - const char *abspath, - const svn_wc_status2_t *status, - apr_pool_t *pool) -{ - struct status_baton *btn = baton; - - if (status->text_status != svn_wc_status_none - && status->text_status != svn_wc_status_deleted - && strcmp(abspath, btn->parent_path)) - { - APR_ARRAY_PUSH(btn->existing_targets, - const char *) = apr_pstrdup(btn->result_pool, - abspath); - } - - return SVN_NO_ERROR; -} - -/* Check if PARENT_DIR_ABSPATH has any versioned or unversioned children if - * we ignore the ones in TARGETS_TO_BE_DELETED. Return the answer in - * EMPTY. */ -static svn_error_t * -is_dir_empty(svn_boolean_t *empty, const char *parent_dir_abspath, - svn_wc_context_t *wc_ctx, - apr_array_header_t *targets_to_be_deleted, - apr_pool_t *result_pool, apr_pool_t *scratch_pool) -{ - struct status_baton btn; - svn_opt_revision_t revision; - int i; - - btn.existing_targets = apr_array_make(scratch_pool, 0, - sizeof(patch_target_t *)); - btn.parent_path = parent_dir_abspath; - btn.result_pool = scratch_pool; - revision.kind = svn_opt_revision_unspecified; - - SVN_ERR(svn_wc_walk_status(wc_ctx, parent_dir_abspath, svn_depth_immediates, - TRUE, TRUE, TRUE, NULL, find_existing_children, - &btn, NULL, NULL, NULL, NULL, scratch_pool)); - *empty = TRUE; - - /* Do we delete all targets? */ - for (i = 0; i < btn.existing_targets->nelts; i++) - { - int j; - const char *found = APR_ARRAY_IDX(btn.existing_targets, i, const char *); - svn_boolean_t deleted = FALSE; - - for (j = 0; j < targets_to_be_deleted->nelts; j++) - { - patch_target_t *to_be_del; - to_be_del = APR_ARRAY_IDX(targets_to_be_deleted, j, patch_target_t *); - if (! strcmp(found, to_be_del->abs_path)) - { - deleted = TRUE; - break; - } - } - if (! deleted) - { - *empty = FALSE; - break; - } - } - - return SVN_NO_ERROR; -} - -/* Add TARGET to the array of targets keyed by their parent dir in - * TARGETS_TO_BE_DELETED. If there is no array for the parent dir a new one - * is created. All allocations are done in RESULT_POOL. */ -static svn_error_t * -add_target_to_hash_keyed_by_parent_dir(apr_hash_t *targets_to_be_deleted, - patch_target_t *target, - apr_pool_t *result_pool) -{ - apr_array_header_t * deleted_targets_in_dir; - - /* We're using the abs_path of the target. The abs_path is not - * present if the path is a symlink pointing outside the wc but we - * know that's not the case. */ - const char *dirname = svn_dirent_dirname(target->abs_path, - result_pool); - - deleted_targets_in_dir = apr_hash_get(targets_to_be_deleted, - dirname, APR_HASH_KEY_STRING); - - if (! deleted_targets_in_dir) - { - deleted_targets_in_dir = apr_array_make(result_pool, 0, - sizeof(patch_target_t *)); - apr_hash_set(targets_to_be_deleted, - dirname, APR_HASH_KEY_STRING, deleted_targets_in_dir); - } - - APR_ARRAY_PUSH(deleted_targets_in_dir, patch_target_t *) = target; - - return SVN_NO_ERROR; -} - -/* Compare A and B and return an integer greater than, equal to, or less - * than 0, according to whether A has less subdirs, just as many or more - * subdir than B. That means that the path with the most subdirs is placed - * first. */ -static int -sort_compare_path_component_count(const svn_sort__item_t *a, - const svn_sort__item_t *b) -{ - apr_size_t n_a = 0, n_b = 0; - - n_a = svn_path_component_count(a->key); - n_b = svn_path_component_count(b->key); - - if (n_a > n_b) - return -1; - else if (n_a == n_b) - return 0; - else - return 1; -} - -/* Condense the list of TARGETS such that there is only a single entry for - * each common parent directory of deleted targets. Use WC_CTX for checking - * if a dir is empty. Re-allocate TARGETS in RESULT_POOL if deleted targets - * exists. Do temporary allocations in SCRATCH_POOL. */ -static svn_error_t * -condense_deleted_targets(apr_array_header_t **targets, svn_wc_context_t - *wc_ctx, apr_pool_t *result_pool, apr_pool_t - *scratch_pool) -{ - int i; - apr_array_header_t *new_targets; - apr_array_header_t *sorted_keys; - apr_hash_t *targets_to_be_deleted; - - /* We have deleted targets, time to do some allocations. */ - new_targets = apr_array_make(result_pool, 0, sizeof(patch_target_t *)); - targets_to_be_deleted = apr_hash_make(result_pool); - - /* First we collect the targets that should be deleted ... */ - for (i = 0; i < (*targets)->nelts; i++) - { - patch_target_t *target = APR_ARRAY_IDX(*targets, i, patch_target_t *); - - if (! target->deleted) - APR_ARRAY_PUSH(new_targets, patch_target_t *) = target; - else - SVN_ERR(add_target_to_hash_keyed_by_parent_dir(targets_to_be_deleted, - target, result_pool)); - } - - /* ... Then we sort the keys to allow us to detect when multiple subdirs - * should be deleted. */ - sorted_keys = svn_sort__hash(targets_to_be_deleted, - sort_compare_path_component_count, - scratch_pool); - - /* For each directory that contains targets to be deleted determine if the - * entire dir is empty and in that case mark it to be deleted. Else just - * add the individual targets in it to 'new_targets'. */ - for (i = 0; i < sorted_keys->nelts ; i++) - { - svn_sort__item_t *item; - svn_boolean_t empty; - char *parent_dir; - apr_array_header_t *targets_array; - int j; - - item = &APR_ARRAY_IDX(sorted_keys, i, svn_sort__item_t); - parent_dir = (char *) item->key; - targets_array = (apr_array_header_t *) item->value; - - SVN_ERR(is_dir_empty(&empty, parent_dir, wc_ctx, targets_array, - result_pool, scratch_pool)); - if (empty) - { - patch_target_t *parent_target = apr_palloc(result_pool, - sizeof(*parent_target)); - parent_target->deleted = TRUE; - parent_target->kind = svn_node_dir; - parent_target->abs_path = apr_pstrdup(result_pool, parent_dir); - - /* Since the dir was empty we'll use a parent_dir target instead - * of the child targets. Time to close unused streams! */ - for (j = 0; j < targets_array->nelts; j++) - { - patch_target_t *child_target; - - child_target = APR_ARRAY_IDX(targets_array, j, patch_target_t *); - - if (child_target->patch) - SVN_ERR(svn_diff_close_patch(child_target->patch)); - } - - /* Marking one dir as to be deleted may mean that it's parent dir - * will be empty too and thus should be marked for deletion. */ - SVN_ERR(add_target_to_hash_keyed_by_parent_dir(targets_to_be_deleted, - parent_target, - result_pool)); - - /* Hack! Since the added target of the parent dir has a shorter - * path, we're guarenteed that it will be inserted later. We do - * the sort and just continue our iteration. */ - sorted_keys = svn_sort__hash(targets_to_be_deleted, - sort_compare_path_component_count, - scratch_pool); - } - else - { - /* No empty dir, just add the targets to be deleted */ - for (j = 0; j < targets_array->nelts; j++) - { - patch_target_t *target = APR_ARRAY_IDX(targets_array, j, - patch_target_t *); - APR_ARRAY_PUSH(new_targets, patch_target_t *) = target; - } - } - } - *targets = new_targets; - - return SVN_NO_ERROR; -} - /* Install a patched TARGET into the working copy at ABS_WC_PATH. * Use client context CTX to retrieve WC_CTX, and possibly doing * notifications. If DRY_RUN is TRUE, don't modify the working copy. @@ -1480,7 +1250,7 @@ install_patched_target(patch_target_t *target, con { if (target->deleted) { - if (! dry_run) + if (! dry_run && ! target->parent_dir_deleted) { /* Schedule the target for deletion. Suppress * notification, we'll do it manually in a minute. @@ -1648,6 +1418,289 @@ install_patched_target(patch_target_t *target, con return SVN_NO_ERROR; } +/* Baton for find_existing_children() */ +struct status_baton +{ + apr_array_header_t *existing_targets; + const char *parent_path; + apr_pool_t *result_pool; +}; + +/* Implements svn_wc_status_func4_t. */ +static svn_error_t * +find_existing_children(void *baton, + const char *abspath, + const svn_wc_status2_t *status, + apr_pool_t *pool) +{ + struct status_baton *btn = baton; + + if (status->text_status != svn_wc_status_none + && status->text_status != svn_wc_status_deleted + && strcmp(abspath, btn->parent_path)) + { + APR_ARRAY_PUSH(btn->existing_targets, + const char *) = apr_pstrdup(btn->result_pool, + abspath); + } + + return SVN_NO_ERROR; +} + +/* Indicate in *EMPTY whether the directory at LOCAL_ABSPATH has any + * versioned or unversioned children. Consider any DELETED_TARGETS, + * as well as paths listed in DELETED_ABSPATHS_LIST (which may be NULL) + * as already deleted. Use WC_CTX as the working copy context. + * Do temporary allocations in SCRATCH_POOL. */ +static svn_error_t * +check_dir_empty(svn_boolean_t *empty, const char *local_abspath, + svn_wc_context_t *wc_ctx, + apr_array_header_t *deleted_targets, + apr_array_header_t *deleted_abspath_list, + apr_pool_t *scratch_pool) +{ + struct status_baton btn; + svn_opt_revision_t revision; + svn_error_t *err; + int i; + + btn.existing_targets = apr_array_make(scratch_pool, 0, + sizeof(patch_target_t *)); + btn.parent_path = local_abspath; + btn.result_pool = scratch_pool; + revision.kind = svn_opt_revision_unspecified; + + err = svn_wc_walk_status(wc_ctx, local_abspath, svn_depth_immediates, + TRUE, TRUE, TRUE, NULL, find_existing_children, + &btn, NULL, NULL, NULL, NULL, scratch_pool); + if (err) + { + if (err->apr_err == SVN_ERR_WC_NOT_WORKING_COPY) + { + /* The patch has deleted the entire working copy. */ + svn_error_clear(err); + *empty = FALSE; + return SVN_NO_ERROR; + } + else + svn_error_return(err); + } + + *empty = TRUE; + + /* Do we delete all targets? */ + for (i = 0; i < btn.existing_targets->nelts; i++) + { + int j; + const char *found; + svn_boolean_t deleted; + + deleted = FALSE; + found = APR_ARRAY_IDX(btn.existing_targets, i, const char *); + + for (j = 0; j < deleted_targets->nelts; j++) + { + patch_target_t *target; + + target = APR_ARRAY_IDX(deleted_targets, j, patch_target_t *); + if (! svn_path_compare_paths(found, target->abs_path)) + { + deleted = TRUE; + break; + } + } + if (! deleted && deleted_abspath_list) + { + for (j = 0; j < deleted_abspath_list->nelts; j++) + { + const char *abspath; + + abspath = APR_ARRAY_IDX(deleted_abspath_list, j, const char *); + if (! svn_path_compare_paths(found, abspath)) + { + deleted = TRUE; + break; + } + } + } + if (! deleted) + { + *empty = FALSE; + break; + } + } + + return SVN_NO_ERROR; +} + +/* Push a copy of EMPTY_DIR, allocated in RESULT_POOL, onto the EMPTY_DIRS + * array if no directory matching EMPTY_DIR is already in the array. */ +static void +push_if_unique(apr_array_header_t *empty_dirs, const char *empty_dir, + apr_pool_t *result_pool) +{ + svn_boolean_t is_unique; + int i; + + is_unique = TRUE; + for (i = 0; i < empty_dirs->nelts; i++) + { + const char *empty_dir2; + + empty_dir2 = APR_ARRAY_IDX(empty_dirs, i, const char *); + if (strcmp(empty_dir, empty_dir2) == 0) + { + is_unique = FALSE; + break; + } + } + + if (is_unique) + APR_ARRAY_PUSH(empty_dirs, const char *) = apr_pstrdup(result_pool, + empty_dir); +} + +/* Delete all directories from the working copy which are left empty + * by deleted TARGETS. Use client context CTX. + * If DRY_RUN is TRUE, do not modify the working copy. + * Do temporary allocations in SCRATCH_POOL. */ +static svn_error_t * +delete_empty_dirs(apr_array_header_t *targets, svn_client_ctx_t *ctx, + svn_boolean_t dry_run, apr_pool_t *scratch_pool) +{ + apr_array_header_t *empty_dirs; + apr_array_header_t *deleted_targets; + apr_pool_t *iterpool; + svn_boolean_t again; + int i; + + /* Get a list of all deleted targets. */ + deleted_targets = apr_array_make(scratch_pool, 0, sizeof(patch_target_t *)); + for (i = 0; i < targets->nelts; i++) + { + patch_target_t *target; + + target = APR_ARRAY_IDX(targets, i, patch_target_t *); + if (target->deleted) + APR_ARRAY_PUSH(deleted_targets, patch_target_t *) = target; + } + + /* We have nothing to do if there aren't any deleted targets. */ + if (deleted_targets->nelts == 0) + return SVN_NO_ERROR; + + /* Look for empty parent directories of deleted targets. */ + empty_dirs = apr_array_make(scratch_pool, 0, sizeof(const char *)); + iterpool = svn_pool_create(scratch_pool); + for (i = 0; i < targets->nelts; i++) + { + svn_boolean_t parent_empty; + patch_target_t *target; + const char *parent; + + svn_pool_clear(iterpool); + + target = APR_ARRAY_IDX(targets, i, patch_target_t *); + parent = svn_dirent_dirname(target->abs_path, iterpool); + SVN_ERR(check_dir_empty(&parent_empty, parent, ctx->wc_ctx, + deleted_targets, NULL, iterpool)); + if (parent_empty) + { + APR_ARRAY_PUSH(empty_dirs, const char *) = + apr_pstrdup(scratch_pool, parent); + } + } + + /* We have nothing to do if there aren't any empty directories. */ + if (empty_dirs->nelts == 0) + { + svn_pool_destroy(iterpool); + return SVN_NO_ERROR; + } + + /* Determine the minimal set of empty directories we need to delete. */ + do + { + apr_array_header_t *empty_dirs_copy; + + svn_pool_clear(iterpool); + + /* Rebuild the empty dirs list, replacing empty dirs which have + * an empty parent with their parent. */ + again = FALSE; + empty_dirs_copy = apr_array_copy(iterpool, empty_dirs); + apr_array_clear(empty_dirs); + for (i = 0; i < empty_dirs_copy->nelts; i++) + { + svn_boolean_t parent_empty; + const char *empty_dir; + const char *parent; + + empty_dir = APR_ARRAY_IDX(empty_dirs_copy, i, const char *); + parent = svn_dirent_dirname(empty_dir, iterpool); + SVN_ERR(check_dir_empty(&parent_empty, parent, ctx->wc_ctx, + deleted_targets, empty_dirs_copy, + iterpool)); + if (parent_empty) + { + again = TRUE; + push_if_unique(empty_dirs, parent, scratch_pool); + } + else + push_if_unique(empty_dirs, empty_dir, scratch_pool); + } + } + while (again); + + /* Mark all children of empty parent directories. */ + for (i = 0; i < targets->nelts ; i++) + { + patch_target_t *target; + int j; + + target = APR_ARRAY_IDX(targets, i, patch_target_t *); + for (j = 0; j < empty_dirs->nelts; j++) + { + const char *empty_dir; + + empty_dir = APR_ARRAY_IDX(empty_dirs, j, const char *); + if (svn_dirent_is_ancestor(empty_dir, target->abs_path)) + { + target->parent_dir_deleted = TRUE; + break; + } + } + } + + /* Finally, delete empty directories. */ + for (i = 0; i < empty_dirs->nelts; i++) + { + const char *empty_dir; + + svn_pool_clear(iterpool); + + if (ctx->cancel_func) + SVN_ERR(ctx->cancel_func(ctx->cancel_baton)); + + empty_dir = APR_ARRAY_IDX(empty_dirs, i, const char *); + if (ctx->notify_func2) + { + svn_wc_notify_t *notify; + + notify = svn_wc_create_notify(empty_dir, svn_wc_notify_delete, + iterpool); + (*ctx->notify_func2)(ctx->notify_baton2, notify, iterpool); + } + if (! dry_run) + SVN_ERR(svn_wc_delete4(ctx->wc_ctx, empty_dir, FALSE, FALSE, + NULL, NULL, /* suppress cancellation */ + NULL, NULL, /* suppress notification */ + iterpool)); + } + + return SVN_NO_ERROR; +} + /* Baton for apply_patches(). */ typedef struct { /* The path to the patch file. */ @@ -1741,8 +1794,8 @@ apply_patches(void *baton, } while (patch); - SVN_ERR(condense_deleted_targets(&targets, btn->ctx->wc_ctx, - scratch_pool, result_pool)); + /* Delete directories which are empty after patching, if any. */ + SVN_ERR(delete_empty_dirs(targets, btn->ctx, btn->dry_run, scratch_pool)); /* Install patched targets into the working copy. */ for (i = 0; i < targets->nelts; i++) @@ -1751,7 +1804,6 @@ apply_patches(void *baton, svn_pool_clear(iterpool); - if (btn->ctx->cancel_func) SVN_ERR(btn->ctx->cancel_func(btn->ctx->cancel_baton)); @@ -1759,7 +1811,8 @@ apply_patches(void *baton, if (! target->skipped) SVN_ERR(install_patched_target(target, btn->abs_wc_path, btn->ctx, btn->dry_run, iterpool)); - SVN_ERR(send_patch_notification(target, btn->ctx, iterpool)); + if (! target->parent_dir_deleted) + SVN_ERR(send_patch_notification(target, btn->ctx, iterpool)); if (target->patch) SVN_ERR(svn_diff_close_patch(target->patch)); } Index: subversion/tests/cmdline/patch_tests.py =================================================================== --- subversion/tests/cmdline/patch_tests.py (revision 932894) +++ subversion/tests/cmdline/patch_tests.py (working copy) @@ -1003,7 +1003,7 @@ def patch_add_new_dir(sbox): None, # expected err 1, # check-props 1) # dry-run -def patch_remove_empty_dir(sbox): +def patch_remove_empty_dirs(sbox): "patch deleting all children of a directory" sbox.build() @@ -1068,9 +1068,9 @@ def patch_add_new_dir(sbox): os.remove(os.path.join(wc_dir, 'A', 'D', 'H', 'chi')) expected_output = [ + 'D %s\n' % os.path.join(wc_dir, 'A', 'B'), 'D %s\n' % os.path.join(wc_dir, 'A', 'D', 'H', 'psi'), 'D %s\n' % os.path.join(wc_dir, 'A', 'D', 'H', 'omega'), - 'D %s\n' % os.path.join(wc_dir, 'A', 'B'), ] expected_disk = svntest.main.greek_state.copy() @@ -2376,7 +2376,7 @@ test_list = [ None, patch_strip1, patch_no_index_line, patch_add_new_dir, - patch_remove_empty_dir, + patch_remove_empty_dirs, patch_reject, patch_keywords, patch_with_fuzz,