You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Daniel Näslund <da...@longitudo.com> on 2010/03/03 16:25:07 UTC
[PATCH v2] 'svn patch' should remove empty dir
Hi!
Here's my revised patch. I've commented on the changes inline.
One strange thing: If you compile this you'll get a compiler warning
about this variable not beeing used:
1354 apr_array_header_t *deleted_targets;
But if I remove it, no targets will be condensed. It's really silly. Can
someone explain why a variable that isn't used must be declared in order
for my code to work. What is the simple logical explanation?
On Mon, Mar 01, 2010 at 04:10:09PM +0100, Stefan Sperling wrote:
> On Sat, Feb 27, 2010 at 09:09:52PM +0100, Daniel N�slund wrote:
> > Index: subversion/libsvn_client/patch.c
> > ===================================================================
> > --- subversion/libsvn_client/patch.c (revision 916918)
> > +++ subversion/libsvn_client/patch.c (arbetskopia)
[...]
> > +/* 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_client_ctx_t *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_client_status5(NULL, parent_dir_abspath, &revision,
> > + find_existing_children, &btn,
> > + svn_depth_immediates, TRUE, FALSE, TRUE,
> > + FALSE, NULL, ctx, scratch_pool));
>
> Not sure if svn_client_status5() is really what we want here.
> Is there a public or semi-private libsvn_wc function we could
> use instead? One that doesn't need a client context?
I used svn_wc_walk_status() instead. It does not handle externals but
treats them as unversioned. No problemos for us, we just want to find
out if anything exists in a dir.
> > + /* 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;
>
> Are you missing a 'break' here?
Added it. Saves us some iterations.
> > + }
> > + 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. */
>
> The comma should either be removed, or moved: "... wc, but we know ...".
Fixed.
> > + 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)
> > + {
> > + APR_ARRAY_PUSH(deleted_targets_in_dir, patch_target_t *) = target;
> > +
> > + apr_hash_set(targets_to_be_deleted, dirname,
> > + APR_HASH_KEY_STRING, deleted_targets_in_dir);
> > + }
> > + else
> > + {
> > + apr_array_header_t *new_array;
> > +
> > + new_array = apr_array_make(result_pool, 0, sizeof(patch_target_t *));
> > + APR_ARRAY_PUSH(new_array, patch_target_t *) = target;
> > + apr_hash_set(targets_to_be_deleted,
> > + dirname, APR_HASH_KEY_STRING, new_array);
> > + }
> > + 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. */
> > +static int
> > +sort_compare_nr_of_path_elements(const svn_sort__item_t *a,
> > + const svn_sort__item_t *b)
>
> Indentation is off above.
Fixed.
> > + const char *astr, *bstr;
> > + int n_a, n_b, i;
> > +
> > + astr = a->key;
> > + bstr = b->key;
> > +
> > + for (i = 0; i < a->klen; i++)
> > + if (astr[i] == '/')
> > + n_a++;
> > +
> > + for (i = 0; i < b->klen; i++)
> > + if (bstr[i] == '/')
> > + n_b++;
> > +
> > + if (n_a > n_b)
> > + return -1;
> > + else if (n_a == n_b)
> > + return 0;
> > + else
> > + return 1;
> > +}
> > +
> > +/* If all TARGETS in a dir is to be deleted, we condense those targets into
> > + * one, representing their parent dirs. A new array is allocated from
> > + * RESULT_POOL and returned in TARGETS. */
>
> Maybe phrase this as:
>
> /* Condense the list of TARGETS to be deleted such that there is
> * only a single entry single entry for each common parent directory
> * of deleted targets. Re-allocate TARGETS in RESULT_POOL.
> * Do temporary allocations in SCRATCH_POOL. */
>
> I'm not documenting CTX cause I hope we can get rid of it, see above :)
I took your suggestion straight off.
> Also, is there a way we can get around always allocating a new
> target arrray?
In my new patch I loop over the targets in the beginning and check for
targets to be deleted. If none, we're done and can return. Takes more
iterations but saves some memory.
> > +static svn_error_t *
> > +maybe_condense_deleted_targets(apr_array_header_t **targets,
> > + svn_client_ctx_t *ctx, apr_pool_t *result_pool,
> > + apr_pool_t *scratch_pool)
> > +{
> > + int i;
> > + apr_array_header_t *new_targets;
> > + apr_array_header_t *deleted_targets;
> > + apr_array_header_t *sorted_keys;
> > + apr_hash_t *targets_to_be_deleted;
> > +
> > + 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_nr_of_path_elements,
> > + scratch_pool);
> > +
> > + for (i = 0; i < sorted_keys->nelts ; i++)
> > + {
> > + svn_sort__item_t *item;
> > + svn_boolean_t empty;
> > + char *key;
> > + apr_array_header_t *targets_array;
> > + int j;
> > +
> > + item = &APR_ARRAY_IDX(sorted_keys, i, svn_sort__item_t);
> > + key = (char *) item->key;
> > + targets_array = (apr_array_header_t *) item->value;
> > +
> > + SVN_ERR(is_dir_empty(&empty, key, ctx, targets_array,
> > + result_pool, scratch_pool));
> > + if (empty)
> > + {
> > + patch_target_t *target = apr_palloc(result_pool, sizeof(*target));
> > + target->deleted = TRUE;
> > + target->kind = svn_node_dir;
> > + target->abs_path = apr_pstrdup(result_pool, key);
> > +
> > + /* 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));
> > + }
> > +
> > + SVN_ERR(add_target_to_hash_keyed_by_parent_dir(targets_to_be_deleted,
> > + 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_nr_of_path_elements,
> > + 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.
> > @@ -1413,6 +1652,9 @@
> > }
> > while (patch);
> >
> > + SVN_ERR(maybe_condense_deleted_targets(&targets, btn->ctx, scratch_pool,
> > + result_pool));
>
> Indentation off above.
Fixed
> > +
> > /* Install patched targets into the working copy. */
> > for (i = 0; i < targets->nelts; i++)
> > {
> > @@ -1428,7 +1670,8 @@
> > 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));
> > - SVN_ERR(svn_diff__close_patch(target->patch));
> > + if (target->patch)
> > + SVN_ERR(svn_diff__close_patch(target->patch));
>
> Hmm... this bit looks like it could be committed separately?
Nope, it's needed. If we have a parent dir target, then it does not
contain any patch. When svn_diff__close_patch() tries to close the
streams we get a segfault.
Re: [PATCH v2] 'svn patch' should remove empty dir
Posted by Daniel Näslund <da...@longitudo.com>.
Committed in r930662 on my branch svn-patch-improvements.
On Wed, Mar 03, 2010 at 04:25:07PM +0100, Daniel Näslund wrote:
> Hi!
>
> Here's my revised patch. I've commented on the changes inline.
>
> One strange thing: If you compile this you'll get a compiler warning
> about this variable not beeing used:
>
> 1354 apr_array_header_t *deleted_targets;
>
> But if I remove it, no targets will be condensed. It's really silly. Can
> someone explain why a variable that isn't used must be declared in order
> for my code to work. What is the simple logical explanation?
>
> On Mon, Mar 01, 2010 at 04:10:09PM +0100, Stefan Sperling wrote:
> > On Sat, Feb 27, 2010 at 09:09:52PM +0100, Daniel Näslund wrote:
> > > Index: subversion/libsvn_client/patch.c
> > > ===================================================================
> > > --- subversion/libsvn_client/patch.c (revision 916918)
> > > +++ subversion/libsvn_client/patch.c (arbetskopia)
>
> [...]
>
> > > +/* 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_client_ctx_t *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_client_status5(NULL, parent_dir_abspath, &revision,
> > > + find_existing_children, &btn,
> > > + svn_depth_immediates, TRUE, FALSE, TRUE,
> > > + FALSE, NULL, ctx, scratch_pool));
> >
> > Not sure if svn_client_status5() is really what we want here.
> > Is there a public or semi-private libsvn_wc function we could
> > use instead? One that doesn't need a client context?
>
> I used svn_wc_walk_status() instead. It does not handle externals but
> treats them as unversioned. No problemos for us, we just want to find
> out if anything exists in a dir.
>
> > > + /* 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;
> >
> > Are you missing a 'break' here?
>
> Added it. Saves us some iterations.
>
> > > + }
> > > + 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. */
> >
> > The comma should either be removed, or moved: "... wc, but we know ...".
>
> Fixed.
>
> > > + 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)
> > > + {
> > > + APR_ARRAY_PUSH(deleted_targets_in_dir, patch_target_t *) = target;
> > > +
> > > + apr_hash_set(targets_to_be_deleted, dirname,
> > > + APR_HASH_KEY_STRING, deleted_targets_in_dir);
> > > + }
> > > + else
> > > + {
> > > + apr_array_header_t *new_array;
> > > +
> > > + new_array = apr_array_make(result_pool, 0, sizeof(patch_target_t *));
> > > + APR_ARRAY_PUSH(new_array, patch_target_t *) = target;
> > > + apr_hash_set(targets_to_be_deleted,
> > > + dirname, APR_HASH_KEY_STRING, new_array);
> > > + }
> > > + 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. */
> > > +static int
> > > +sort_compare_nr_of_path_elements(const svn_sort__item_t *a,
> > > + const svn_sort__item_t *b)
> >
> > Indentation is off above.
>
> Fixed.
>
> > > + const char *astr, *bstr;
> > > + int n_a, n_b, i;
> > > +
> > > + astr = a->key;
> > > + bstr = b->key;
> > > +
> > > + for (i = 0; i < a->klen; i++)
> > > + if (astr[i] == '/')
> > > + n_a++;
> > > +
> > > + for (i = 0; i < b->klen; i++)
> > > + if (bstr[i] == '/')
> > > + n_b++;
> > > +
> > > + if (n_a > n_b)
> > > + return -1;
> > > + else if (n_a == n_b)
> > > + return 0;
> > > + else
> > > + return 1;
> > > +}
> > > +
> > > +/* If all TARGETS in a dir is to be deleted, we condense those targets into
> > > + * one, representing their parent dirs. A new array is allocated from
> > > + * RESULT_POOL and returned in TARGETS. */
> >
> > Maybe phrase this as:
> >
> > /* Condense the list of TARGETS to be deleted such that there is
> > * only a single entry single entry for each common parent directory
> > * of deleted targets. Re-allocate TARGETS in RESULT_POOL.
> > * Do temporary allocations in SCRATCH_POOL. */
> >
> > I'm not documenting CTX cause I hope we can get rid of it, see above :)
>
> I took your suggestion straight off.
>
> > Also, is there a way we can get around always allocating a new
> > target arrray?
>
> In my new patch I loop over the targets in the beginning and check for
> targets to be deleted. If none, we're done and can return. Takes more
> iterations but saves some memory.
>
> > > +static svn_error_t *
> > > +maybe_condense_deleted_targets(apr_array_header_t **targets,
> > > + svn_client_ctx_t *ctx, apr_pool_t *result_pool,
> > > + apr_pool_t *scratch_pool)
> > > +{
> > > + int i;
> > > + apr_array_header_t *new_targets;
> > > + apr_array_header_t *deleted_targets;
> > > + apr_array_header_t *sorted_keys;
> > > + apr_hash_t *targets_to_be_deleted;
> > > +
> > > + 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_nr_of_path_elements,
> > > + scratch_pool);
> > > +
> > > + for (i = 0; i < sorted_keys->nelts ; i++)
> > > + {
> > > + svn_sort__item_t *item;
> > > + svn_boolean_t empty;
> > > + char *key;
> > > + apr_array_header_t *targets_array;
> > > + int j;
> > > +
> > > + item = &APR_ARRAY_IDX(sorted_keys, i, svn_sort__item_t);
> > > + key = (char *) item->key;
> > > + targets_array = (apr_array_header_t *) item->value;
> > > +
> > > + SVN_ERR(is_dir_empty(&empty, key, ctx, targets_array,
> > > + result_pool, scratch_pool));
> > > + if (empty)
> > > + {
> > > + patch_target_t *target = apr_palloc(result_pool, sizeof(*target));
> > > + target->deleted = TRUE;
> > > + target->kind = svn_node_dir;
> > > + target->abs_path = apr_pstrdup(result_pool, key);
> > > +
> > > + /* 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));
> > > + }
> > > +
> > > + SVN_ERR(add_target_to_hash_keyed_by_parent_dir(targets_to_be_deleted,
> > > + 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_nr_of_path_elements,
> > > + 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.
> > > @@ -1413,6 +1652,9 @@
> > > }
> > > while (patch);
> > >
> > > + SVN_ERR(maybe_condense_deleted_targets(&targets, btn->ctx, scratch_pool,
> > > + result_pool));
> >
> > Indentation off above.
>
> Fixed
> > > +
> > > /* Install patched targets into the working copy. */
> > > for (i = 0; i < targets->nelts; i++)
> > > {
> > > @@ -1428,7 +1670,8 @@
> > > 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));
> > > - SVN_ERR(svn_diff__close_patch(target->patch));
> > > + if (target->patch)
> > > + SVN_ERR(svn_diff__close_patch(target->patch));
> >
> > Hmm... this bit looks like it could be committed separately?
>
> Nope, it's needed. If we have a parent dir target, then it does not
> contain any patch. When svn_diff__close_patch() tries to close the
> streams we get a segfault.
>
> Index: subversion/tests/cmdline/patch_tests.py
> ===================================================================
> --- subversion/tests/cmdline/patch_tests.py (revision 918486)
> +++ subversion/tests/cmdline/patch_tests.py (arbetskopia)
> @@ -911,7 +911,108 @@
> None, # expected err
> 1, # check-props
> 1) # dry-run
> +def patch_remove_empty_dir(sbox):
> + "delete the dir if all children is deleted"
>
> + sbox.build()
> + wc_dir = sbox.wc_dir
> +
> + patch_file_path = tempfile.mkstemp(dir=os.path.abspath(svntest.main.temp_dir))[1]
> +
> + # Contents of B:
> + # A/B/lamba
> + # A/B/F
> + # A/B/E/{alpha,beta}
> + # Before patching we've deleted F, which means that B is empty after patching and
> + # should be removed.
> + #
> + # Contents of H:
> + # A/D/H/{chi,psi,omega}
> + # Before patching, chi has been removed by a non-svn operation which means it has
> + # status missing. The patch deletes the other two files but should not delete H.
> +
> + unidiff_patch = [
> + "Index: psi\n",
> + "===================================================================\n",
> + "--- A/D/H/psi\t(revision 0)\n",
> + "+++ A/D/H/psi\t(revision 0)\n",
> + "@@ -1 +0,0 @@\n",
> + "-This is the file 'psi'.\n",
> + "Index: omega\n",
> + "===================================================================\n",
> + "--- A/D/H/omega\t(revision 0)\n",
> + "+++ A/D/H/omega\t(revision 0)\n",
> + "@@ -1 +0,0 @@\n",
> + "-This is the file 'omega'.\n",
> + "Index: lambda\n",
> + "===================================================================\n",
> + "--- A/B/lambda\t(revision 0)\n",
> + "+++ A/B/lambda\t(revision 0)\n",
> + "@@ -1 +0,0 @@\n",
> + "-This is the file 'lambda'.\n",
> + "Index: alpha\n",
> + "===================================================================\n",
> + "--- A/B/E/alpha\t(revision 0)\n",
> + "+++ A/B/E/alpha\t(revision 0)\n",
> + "@@ -1 +0,0 @@\n",
> + "-This is the file 'alpha'.\n",
> + "Index: beta\n",
> + "===================================================================\n",
> + "--- A/B/E/beta\t(revision 0)\n",
> + "+++ A/B/E/beta\t(revision 0)\n",
> + "@@ -1 +0,0 @@\n",
> + "-This is the file 'beta'.\n",
> + ]
> +
> + svntest.main.file_write(patch_file_path, ''.join(unidiff_patch))
> +
> + F_path = os.path.join(wc_dir, 'A', 'B', 'F')
> + svntest.actions.run_and_verify_svn("Deleting F failed", None, [],
> + 'rm', F_path)
> + svntest.actions.run_and_verify_svn("Update failed", None, [],
> + 'up', wc_dir)
> +
> + # We should be able to handle one path beeing missing.
> + 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'),
> + ]
> +
> + expected_disk = svntest.main.greek_state.copy()
> + expected_disk.remove('A/D/H/chi')
> + expected_disk.remove('A/D/H/psi')
> + expected_disk.remove('A/D/H/omega')
> + expected_disk.remove('A/B/lambda')
> + expected_disk.remove('A/B/E/alpha')
> + expected_disk.remove('A/B/E/beta')
> +
> + expected_status = svntest.actions.get_virginal_state(wc_dir, 1)
> + expected_status.add({'A/D/H/chi' : Item(status='! ', wc_rev=1)})
> + expected_status.add({'A/D/H/omega' : Item(status='D ', wc_rev=1)})
> + expected_status.add({'A/D/H/psi' : Item(status='D ', wc_rev=1)})
> + expected_status.add({'A/B' : Item(status='D ', wc_rev=1)})
> + expected_status.add({'A/B/E' : Item(status='D ', wc_rev=1)})
> + expected_status.add({'A/B/E/beta' : Item(status='D ', wc_rev=1)})
> + expected_status.add({'A/B/E/alpha' : Item(status='D ', wc_rev=1)})
> + expected_status.add({'A/B/lambda' : Item(status='D ', wc_rev=1)})
> + expected_status.add({'A/B/F' : Item(status='D ', wc_rev=1)})
> +
> + expected_skip = wc.State('', { })
> +
> + svntest.actions.run_and_verify_patch(wc_dir,
> + os.path.abspath(patch_file_path),
> + expected_output,
> + expected_disk,
> + expected_status,
> + expected_skip,
> + None, # expected err
> + 1, # check-props
> + 1) # dry-run
> +
> +
> def patch_reject(sbox):
> "apply a patch which is rejected"
>
> @@ -1363,6 +1464,7 @@
> patch_chopped_leading_spaces,
> patch_strip1,
> patch_add_new_dir,
> + patch_remove_empty_dir,
> patch_reject,
> patch_keywords,
> patch_with_fuzz,
> Index: subversion/libsvn_client/patch.c
> ===================================================================
> --- subversion/libsvn_client/patch.c (revision 918486)
> +++ subversion/libsvn_client/patch.c (arbetskopia)
> @@ -34,6 +34,7 @@
> #include "svn_path.h"
> #include "svn_pools.h"
> #include "svn_props.h"
> +#include "svn_sorts.h"
> #include "svn_subst.h"
> #include "svn_wc.h"
> #include "client.h"
> @@ -1184,6 +1185,268 @@
> 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;
> +
> +#if 0
> + SVN_ERR(svn_client_status5(NULL, parent_dir_abspath, &revision,
> + find_existing_children, &btn,
> + svn_depth_immediates, TRUE, FALSE, TRUE,
> + FALSE, NULL, ctx, scratch_pool));
> +#endif
> +
> + 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));
> +
> + /* 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)
> + {
> + APR_ARRAY_PUSH(deleted_targets_in_dir, patch_target_t *) = target;
> +
> + apr_hash_set(targets_to_be_deleted, dirname,
> + APR_HASH_KEY_STRING, deleted_targets_in_dir);
> + }
> + else
> + {
> + apr_array_header_t *new_array;
> +
> + new_array = apr_array_make(result_pool, 0, sizeof(patch_target_t *));
> + APR_ARRAY_PUSH(new_array, patch_target_t *) = target;
> + apr_hash_set(targets_to_be_deleted,
> + dirname, APR_HASH_KEY_STRING, new_array);
> + }
> + 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. */
> +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, n_b, i;
> +
> + astr = a->key;
> + bstr = b->key;
> +
> + for (i = 0; i < a->klen; i++)
> + if (astr[i] == '/')
> + n_a++;
> +
> + for (i = 0; i < b->klen; i++)
> + if (bstr[i] == '/')
> + n_b++;
> +
> + if (n_a > n_b)
> + return -1;
> + else if (n_a == n_b)
> + return 0;
> + else
> + return 1;
> +}
> +
> +/* Condense the list of TARGETS to be deleted 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 *
> +maybe_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 *deleted_targets;
> + apr_array_header_t *sorted_keys;
> + apr_hash_t *targets_to_be_deleted;
> + svn_boolean_t deleted_targets_exists = FALSE;
> +
> + for (i = 0; i < (*targets)->nelts; i++)
> + {
> + patch_target_t *target = APR_ARRAY_IDX(*targets, i, patch_target_t *);
> + if (target->deleted)
> + deleted_targets_exists = TRUE;
> + }
> +
> + if (! deleted_targets_exists)
> + return SVN_NO_ERROR;
> +
> + /* 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_nr_of_path_elements,
> + scratch_pool);
> +
> + for (i = 0; i < sorted_keys->nelts ; i++)
> + {
> + svn_sort__item_t *item;
> + svn_boolean_t empty;
> + char *key;
> + apr_array_header_t *targets_array;
> + int j;
> +
> + item = &APR_ARRAY_IDX(sorted_keys, i, svn_sort__item_t);
> + key = (char *) item->key;
> + targets_array = (apr_array_header_t *) item->value;
> +
> + SVN_ERR(is_dir_empty(&empty, key, wc_ctx, targets_array,
> + result_pool, scratch_pool));
> + if (empty)
> + {
> + patch_target_t *target = apr_palloc(result_pool, sizeof(*target));
> + target->deleted = TRUE;
> + target->kind = svn_node_dir;
> + target->abs_path = apr_pstrdup(result_pool, key);
> +
> + /* 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));
> + }
> +
> + SVN_ERR(add_target_to_hash_keyed_by_parent_dir(targets_to_be_deleted,
> + 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_nr_of_path_elements,
> + 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.
> @@ -1438,6 +1701,9 @@
> }
> while (patch);
>
> + SVN_ERR(maybe_condense_deleted_targets(&targets, btn->ctx->wc_ctx,
> + scratch_pool, result_pool));
> +
> /* Install patched targets into the working copy. */
> for (i = 0; i < targets->nelts; i++)
> {
> @@ -1453,7 +1719,8 @@
> 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));
> - SVN_ERR(svn_diff__close_patch(target->patch));
> + if (target->patch)
> + SVN_ERR(svn_diff__close_patch(target->patch));
> }
>
> svn_pool_destroy(iterpool);
Re: [PATCH v2] 'svn patch' should remove empty dir
Posted by Daniel Näslund <da...@longitudo.com>.
Committed in r930662 on my branch svn-patch-improvements.
On Wed, Mar 03, 2010 at 04:25:07PM +0100, Daniel Näslund wrote:
> Hi!
>
> Here's my revised patch. I've commented on the changes inline.
>
> One strange thing: If you compile this you'll get a compiler warning
> about this variable not beeing used:
>
> 1354 apr_array_header_t *deleted_targets;
>
> But if I remove it, no targets will be condensed. It's really silly. Can
> someone explain why a variable that isn't used must be declared in order
> for my code to work. What is the simple logical explanation?
>
> On Mon, Mar 01, 2010 at 04:10:09PM +0100, Stefan Sperling wrote:
> > On Sat, Feb 27, 2010 at 09:09:52PM +0100, Daniel Näslund wrote:
> > > Index: subversion/libsvn_client/patch.c
> > > ===================================================================
> > > --- subversion/libsvn_client/patch.c (revision 916918)
> > > +++ subversion/libsvn_client/patch.c (arbetskopia)
>
> [...]
>
> > > +/* 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_client_ctx_t *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_client_status5(NULL, parent_dir_abspath, &revision,
> > > + find_existing_children, &btn,
> > > + svn_depth_immediates, TRUE, FALSE, TRUE,
> > > + FALSE, NULL, ctx, scratch_pool));
> >
> > Not sure if svn_client_status5() is really what we want here.
> > Is there a public or semi-private libsvn_wc function we could
> > use instead? One that doesn't need a client context?
>
> I used svn_wc_walk_status() instead. It does not handle externals but
> treats them as unversioned. No problemos for us, we just want to find
> out if anything exists in a dir.
>
> > > + /* 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;
> >
> > Are you missing a 'break' here?
>
> Added it. Saves us some iterations.
>
> > > + }
> > > + 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. */
> >
> > The comma should either be removed, or moved: "... wc, but we know ...".
>
> Fixed.
>
> > > + 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)
> > > + {
> > > + APR_ARRAY_PUSH(deleted_targets_in_dir, patch_target_t *) = target;
> > > +
> > > + apr_hash_set(targets_to_be_deleted, dirname,
> > > + APR_HASH_KEY_STRING, deleted_targets_in_dir);
> > > + }
> > > + else
> > > + {
> > > + apr_array_header_t *new_array;
> > > +
> > > + new_array = apr_array_make(result_pool, 0, sizeof(patch_target_t *));
> > > + APR_ARRAY_PUSH(new_array, patch_target_t *) = target;
> > > + apr_hash_set(targets_to_be_deleted,
> > > + dirname, APR_HASH_KEY_STRING, new_array);
> > > + }
> > > + 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. */
> > > +static int
> > > +sort_compare_nr_of_path_elements(const svn_sort__item_t *a,
> > > + const svn_sort__item_t *b)
> >
> > Indentation is off above.
>
> Fixed.
>
> > > + const char *astr, *bstr;
> > > + int n_a, n_b, i;
> > > +
> > > + astr = a->key;
> > > + bstr = b->key;
> > > +
> > > + for (i = 0; i < a->klen; i++)
> > > + if (astr[i] == '/')
> > > + n_a++;
> > > +
> > > + for (i = 0; i < b->klen; i++)
> > > + if (bstr[i] == '/')
> > > + n_b++;
> > > +
> > > + if (n_a > n_b)
> > > + return -1;
> > > + else if (n_a == n_b)
> > > + return 0;
> > > + else
> > > + return 1;
> > > +}
> > > +
> > > +/* If all TARGETS in a dir is to be deleted, we condense those targets into
> > > + * one, representing their parent dirs. A new array is allocated from
> > > + * RESULT_POOL and returned in TARGETS. */
> >
> > Maybe phrase this as:
> >
> > /* Condense the list of TARGETS to be deleted such that there is
> > * only a single entry single entry for each common parent directory
> > * of deleted targets. Re-allocate TARGETS in RESULT_POOL.
> > * Do temporary allocations in SCRATCH_POOL. */
> >
> > I'm not documenting CTX cause I hope we can get rid of it, see above :)
>
> I took your suggestion straight off.
>
> > Also, is there a way we can get around always allocating a new
> > target arrray?
>
> In my new patch I loop over the targets in the beginning and check for
> targets to be deleted. If none, we're done and can return. Takes more
> iterations but saves some memory.
>
> > > +static svn_error_t *
> > > +maybe_condense_deleted_targets(apr_array_header_t **targets,
> > > + svn_client_ctx_t *ctx, apr_pool_t *result_pool,
> > > + apr_pool_t *scratch_pool)
> > > +{
> > > + int i;
> > > + apr_array_header_t *new_targets;
> > > + apr_array_header_t *deleted_targets;
> > > + apr_array_header_t *sorted_keys;
> > > + apr_hash_t *targets_to_be_deleted;
> > > +
> > > + 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_nr_of_path_elements,
> > > + scratch_pool);
> > > +
> > > + for (i = 0; i < sorted_keys->nelts ; i++)
> > > + {
> > > + svn_sort__item_t *item;
> > > + svn_boolean_t empty;
> > > + char *key;
> > > + apr_array_header_t *targets_array;
> > > + int j;
> > > +
> > > + item = &APR_ARRAY_IDX(sorted_keys, i, svn_sort__item_t);
> > > + key = (char *) item->key;
> > > + targets_array = (apr_array_header_t *) item->value;
> > > +
> > > + SVN_ERR(is_dir_empty(&empty, key, ctx, targets_array,
> > > + result_pool, scratch_pool));
> > > + if (empty)
> > > + {
> > > + patch_target_t *target = apr_palloc(result_pool, sizeof(*target));
> > > + target->deleted = TRUE;
> > > + target->kind = svn_node_dir;
> > > + target->abs_path = apr_pstrdup(result_pool, key);
> > > +
> > > + /* 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));
> > > + }
> > > +
> > > + SVN_ERR(add_target_to_hash_keyed_by_parent_dir(targets_to_be_deleted,
> > > + 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_nr_of_path_elements,
> > > + 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.
> > > @@ -1413,6 +1652,9 @@
> > > }
> > > while (patch);
> > >
> > > + SVN_ERR(maybe_condense_deleted_targets(&targets, btn->ctx, scratch_pool,
> > > + result_pool));
> >
> > Indentation off above.
>
> Fixed
> > > +
> > > /* Install patched targets into the working copy. */
> > > for (i = 0; i < targets->nelts; i++)
> > > {
> > > @@ -1428,7 +1670,8 @@
> > > 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));
> > > - SVN_ERR(svn_diff__close_patch(target->patch));
> > > + if (target->patch)
> > > + SVN_ERR(svn_diff__close_patch(target->patch));
> >
> > Hmm... this bit looks like it could be committed separately?
>
> Nope, it's needed. If we have a parent dir target, then it does not
> contain any patch. When svn_diff__close_patch() tries to close the
> streams we get a segfault.
>
> Index: subversion/tests/cmdline/patch_tests.py
> ===================================================================
> --- subversion/tests/cmdline/patch_tests.py (revision 918486)
> +++ subversion/tests/cmdline/patch_tests.py (arbetskopia)
> @@ -911,7 +911,108 @@
> None, # expected err
> 1, # check-props
> 1) # dry-run
> +def patch_remove_empty_dir(sbox):
> + "delete the dir if all children is deleted"
>
> + sbox.build()
> + wc_dir = sbox.wc_dir
> +
> + patch_file_path = tempfile.mkstemp(dir=os.path.abspath(svntest.main.temp_dir))[1]
> +
> + # Contents of B:
> + # A/B/lamba
> + # A/B/F
> + # A/B/E/{alpha,beta}
> + # Before patching we've deleted F, which means that B is empty after patching and
> + # should be removed.
> + #
> + # Contents of H:
> + # A/D/H/{chi,psi,omega}
> + # Before patching, chi has been removed by a non-svn operation which means it has
> + # status missing. The patch deletes the other two files but should not delete H.
> +
> + unidiff_patch = [
> + "Index: psi\n",
> + "===================================================================\n",
> + "--- A/D/H/psi\t(revision 0)\n",
> + "+++ A/D/H/psi\t(revision 0)\n",
> + "@@ -1 +0,0 @@\n",
> + "-This is the file 'psi'.\n",
> + "Index: omega\n",
> + "===================================================================\n",
> + "--- A/D/H/omega\t(revision 0)\n",
> + "+++ A/D/H/omega\t(revision 0)\n",
> + "@@ -1 +0,0 @@\n",
> + "-This is the file 'omega'.\n",
> + "Index: lambda\n",
> + "===================================================================\n",
> + "--- A/B/lambda\t(revision 0)\n",
> + "+++ A/B/lambda\t(revision 0)\n",
> + "@@ -1 +0,0 @@\n",
> + "-This is the file 'lambda'.\n",
> + "Index: alpha\n",
> + "===================================================================\n",
> + "--- A/B/E/alpha\t(revision 0)\n",
> + "+++ A/B/E/alpha\t(revision 0)\n",
> + "@@ -1 +0,0 @@\n",
> + "-This is the file 'alpha'.\n",
> + "Index: beta\n",
> + "===================================================================\n",
> + "--- A/B/E/beta\t(revision 0)\n",
> + "+++ A/B/E/beta\t(revision 0)\n",
> + "@@ -1 +0,0 @@\n",
> + "-This is the file 'beta'.\n",
> + ]
> +
> + svntest.main.file_write(patch_file_path, ''.join(unidiff_patch))
> +
> + F_path = os.path.join(wc_dir, 'A', 'B', 'F')
> + svntest.actions.run_and_verify_svn("Deleting F failed", None, [],
> + 'rm', F_path)
> + svntest.actions.run_and_verify_svn("Update failed", None, [],
> + 'up', wc_dir)
> +
> + # We should be able to handle one path beeing missing.
> + 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'),
> + ]
> +
> + expected_disk = svntest.main.greek_state.copy()
> + expected_disk.remove('A/D/H/chi')
> + expected_disk.remove('A/D/H/psi')
> + expected_disk.remove('A/D/H/omega')
> + expected_disk.remove('A/B/lambda')
> + expected_disk.remove('A/B/E/alpha')
> + expected_disk.remove('A/B/E/beta')
> +
> + expected_status = svntest.actions.get_virginal_state(wc_dir, 1)
> + expected_status.add({'A/D/H/chi' : Item(status='! ', wc_rev=1)})
> + expected_status.add({'A/D/H/omega' : Item(status='D ', wc_rev=1)})
> + expected_status.add({'A/D/H/psi' : Item(status='D ', wc_rev=1)})
> + expected_status.add({'A/B' : Item(status='D ', wc_rev=1)})
> + expected_status.add({'A/B/E' : Item(status='D ', wc_rev=1)})
> + expected_status.add({'A/B/E/beta' : Item(status='D ', wc_rev=1)})
> + expected_status.add({'A/B/E/alpha' : Item(status='D ', wc_rev=1)})
> + expected_status.add({'A/B/lambda' : Item(status='D ', wc_rev=1)})
> + expected_status.add({'A/B/F' : Item(status='D ', wc_rev=1)})
> +
> + expected_skip = wc.State('', { })
> +
> + svntest.actions.run_and_verify_patch(wc_dir,
> + os.path.abspath(patch_file_path),
> + expected_output,
> + expected_disk,
> + expected_status,
> + expected_skip,
> + None, # expected err
> + 1, # check-props
> + 1) # dry-run
> +
> +
> def patch_reject(sbox):
> "apply a patch which is rejected"
>
> @@ -1363,6 +1464,7 @@
> patch_chopped_leading_spaces,
> patch_strip1,
> patch_add_new_dir,
> + patch_remove_empty_dir,
> patch_reject,
> patch_keywords,
> patch_with_fuzz,
> Index: subversion/libsvn_client/patch.c
> ===================================================================
> --- subversion/libsvn_client/patch.c (revision 918486)
> +++ subversion/libsvn_client/patch.c (arbetskopia)
> @@ -34,6 +34,7 @@
> #include "svn_path.h"
> #include "svn_pools.h"
> #include "svn_props.h"
> +#include "svn_sorts.h"
> #include "svn_subst.h"
> #include "svn_wc.h"
> #include "client.h"
> @@ -1184,6 +1185,268 @@
> 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;
> +
> +#if 0
> + SVN_ERR(svn_client_status5(NULL, parent_dir_abspath, &revision,
> + find_existing_children, &btn,
> + svn_depth_immediates, TRUE, FALSE, TRUE,
> + FALSE, NULL, ctx, scratch_pool));
> +#endif
> +
> + 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));
> +
> + /* 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)
> + {
> + APR_ARRAY_PUSH(deleted_targets_in_dir, patch_target_t *) = target;
> +
> + apr_hash_set(targets_to_be_deleted, dirname,
> + APR_HASH_KEY_STRING, deleted_targets_in_dir);
> + }
> + else
> + {
> + apr_array_header_t *new_array;
> +
> + new_array = apr_array_make(result_pool, 0, sizeof(patch_target_t *));
> + APR_ARRAY_PUSH(new_array, patch_target_t *) = target;
> + apr_hash_set(targets_to_be_deleted,
> + dirname, APR_HASH_KEY_STRING, new_array);
> + }
> + 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. */
> +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, n_b, i;
> +
> + astr = a->key;
> + bstr = b->key;
> +
> + for (i = 0; i < a->klen; i++)
> + if (astr[i] == '/')
> + n_a++;
> +
> + for (i = 0; i < b->klen; i++)
> + if (bstr[i] == '/')
> + n_b++;
> +
> + if (n_a > n_b)
> + return -1;
> + else if (n_a == n_b)
> + return 0;
> + else
> + return 1;
> +}
> +
> +/* Condense the list of TARGETS to be deleted 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 *
> +maybe_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 *deleted_targets;
> + apr_array_header_t *sorted_keys;
> + apr_hash_t *targets_to_be_deleted;
> + svn_boolean_t deleted_targets_exists = FALSE;
> +
> + for (i = 0; i < (*targets)->nelts; i++)
> + {
> + patch_target_t *target = APR_ARRAY_IDX(*targets, i, patch_target_t *);
> + if (target->deleted)
> + deleted_targets_exists = TRUE;
> + }
> +
> + if (! deleted_targets_exists)
> + return SVN_NO_ERROR;
> +
> + /* 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_nr_of_path_elements,
> + scratch_pool);
> +
> + for (i = 0; i < sorted_keys->nelts ; i++)
> + {
> + svn_sort__item_t *item;
> + svn_boolean_t empty;
> + char *key;
> + apr_array_header_t *targets_array;
> + int j;
> +
> + item = &APR_ARRAY_IDX(sorted_keys, i, svn_sort__item_t);
> + key = (char *) item->key;
> + targets_array = (apr_array_header_t *) item->value;
> +
> + SVN_ERR(is_dir_empty(&empty, key, wc_ctx, targets_array,
> + result_pool, scratch_pool));
> + if (empty)
> + {
> + patch_target_t *target = apr_palloc(result_pool, sizeof(*target));
> + target->deleted = TRUE;
> + target->kind = svn_node_dir;
> + target->abs_path = apr_pstrdup(result_pool, key);
> +
> + /* 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));
> + }
> +
> + SVN_ERR(add_target_to_hash_keyed_by_parent_dir(targets_to_be_deleted,
> + 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_nr_of_path_elements,
> + 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.
> @@ -1438,6 +1701,9 @@
> }
> while (patch);
>
> + SVN_ERR(maybe_condense_deleted_targets(&targets, btn->ctx->wc_ctx,
> + scratch_pool, result_pool));
> +
> /* Install patched targets into the working copy. */
> for (i = 0; i < targets->nelts; i++)
> {
> @@ -1453,7 +1719,8 @@
> 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));
> - SVN_ERR(svn_diff__close_patch(target->patch));
> + if (target->patch)
> + SVN_ERR(svn_diff__close_patch(target->patch));
> }
>
> svn_pool_destroy(iterpool);
Re: [PATCH v2] 'svn patch' should remove empty dir
Posted by Julian Foad <ju...@wandisco.com>.
Daniel Näslund wrote:
> Index: subversion/libsvn_client/patch.c
> ===================================================================
> --- subversion/libsvn_client/patch.c (revision 918486)
> +++ subversion/libsvn_client/patch.c (arbetskopia)
[...]
> @@ -1184,6 +1185,268 @@
> 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;
> +
> +#if 0
> + SVN_ERR(svn_client_status5(NULL, parent_dir_abspath, &revision,
> + find_existing_children, &btn,
> + svn_depth_immediates, TRUE, FALSE, TRUE,
> + FALSE, NULL, ctx, scratch_pool));
> +#endif
> +
> + 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));
> +
> + /* 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)
> + {
> + APR_ARRAY_PUSH(deleted_targets_in_dir, patch_target_t *) = target;
> +
> + apr_hash_set(targets_to_be_deleted, dirname,
> + APR_HASH_KEY_STRING, deleted_targets_in_dir);
'deleted_targets_in_dir' is only a pointer to the array header, so it
hasn't changed, and apr_hash_get() doesn't remove it from the hash, so
there is no need to update the hash entry here.
> + }
> + else
> + {
> + apr_array_header_t *new_array;
I'm a fan of tightly scoped local variables, but it would be logical to
use the variable 'deleted_targets_in_dir' (which already exists) here...
> + new_array = apr_array_make(result_pool, 0, sizeof(patch_target_t *));
> + APR_ARRAY_PUSH(new_array, patch_target_t *) = target;
... and then this PUSH line can go after the whole if/else block, as it
will be common to both branches. (That is valid because putting a
pointer to the array into the hash doesn't give the hash *ownership* of
the array or the ability to move it: you still have control of it.)
> + apr_hash_set(targets_to_be_deleted,
> + dirname, APR_HASH_KEY_STRING, new_array);
> + }
> + 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. */
> +static int
> +sort_compare_nr_of_path_elements(const svn_sort__item_t *a,
> + const svn_sort__item_t *b)
(I wonder if you can do what you need by using the normal path sort
order given by svn_sort_compare_items_as_paths(), instead of this
function. I haven't examined this function yet.)
> +{
> + const char *astr, *bstr;
> + int n_a, n_b, i;
> +
> + astr = a->key;
> + bstr = b->key;
> +
> + for (i = 0; i < a->klen; i++)
> + if (astr[i] == '/')
> + n_a++;
> +
> + for (i = 0; i < b->klen; i++)
> + if (bstr[i] == '/')
> + n_b++;
> +
> + if (n_a > n_b)
> + return -1;
> + else if (n_a == n_b)
> + return 0;
> + else
> + return 1;
> +}
> +
> +/* Condense the list of TARGETS to be deleted such that there is only a
> + * single entry for each common parent directory of deleted targets. Use
The array is "*TARGETS" rather than "TARGETS", and *TARGETS is an array
of (patch_target_t *).
I found this doc string hard to understand until I worked it out from
the implementation. The wording implies TARGETS is a list of targets to
be deleted, but really it is a list of targets, some of which may be
marked "to be deleted". Could you use another sentence or two to explain
it?
> + * 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 *
> +maybe_condense_deleted_targets(apr_array_header_t **targets,
Why "maybe"? Doc string indicates it always does something. If it just
means "if there are no targets to be deleted, then obviously there is
nothing to do" I would drop the "maybe".
> + 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 *deleted_targets;
> + apr_array_header_t *sorted_keys;
> + apr_hash_t *targets_to_be_deleted;
> + svn_boolean_t deleted_targets_exists = FALSE;
> +
> + for (i = 0; i < (*targets)->nelts; i++)
> + {
> + patch_target_t *target = APR_ARRAY_IDX(*targets, i, patch_target_t *);
> + if (target->deleted)
> + deleted_targets_exists = TRUE;
> + }
> +
> + if (! deleted_targets_exists)
> + return SVN_NO_ERROR;
> +
> + /* We have deleted targets, time to do some allocations. */
Is it really worth adding the above chunk of code to "optimise" these
simple array and hash allocations? Allocations are worth worrying about
if they're done frequently, but these are just once per whole patch
file.
I would just delete the above code, let the loop below run through its
business ...
(The array and hash operations in this loop are very fast and take very
little space when you're just storing pointers in the hash.)
> + 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));
> + }
... and then maybe you could write an "easy exit":
if (apr_hash_is_empty(targets_to_be_deleted))
return SVN_NO_ERROR;
(If I'm on a code-deletion spree, I would even say that this is of
marginal value, and is more to make the reader feel good than to make
the software run faster. The "sort" and the loop below will both be
instantaneous if there are no targets to be deleted. But I'm not really
on a spree.)
> + /* ... 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_nr_of_path_elements,
> + scratch_pool);
This loop could do with an explanatory comment:
/* For each directory that contains targets to be deleted... if the entire
* directory is to be deleted, then
* ...?
* 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 *key;
Please call the key something more specific: "path"?
> + apr_array_header_t *targets_array;
> + int j;
> +
> + item = &APR_ARRAY_IDX(sorted_keys, i, svn_sort__item_t);
> + key = (char *) item->key;
> + targets_array = (apr_array_header_t *) item->value;
> +
> + SVN_ERR(is_dir_empty(&empty, key, wc_ctx, targets_array,
> + result_pool, scratch_pool));
> + if (empty)
> + {
> + patch_target_t *target = apr_palloc(result_pool, sizeof(*target));
> + target->deleted = TRUE;
> + target->kind = svn_node_dir;
> + target->abs_path = apr_pstrdup(result_pool, key);
> +
> + /* 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));
> + }
> +
> + SVN_ERR(add_target_to_hash_keyed_by_parent_dir(targets_to_be_deleted,
> + 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. */
This depends on your sort order being "stable". Is it stable? I didn't
check.
That's all I have time for, today. I have no idea why it was misbehaving
w.r.t. removing the unused variable, unless something to do with this
re-sorting here.
- Julian
> + sorted_keys = svn_sort__hash(targets_to_be_deleted,
> + sort_compare_nr_of_path_elements,
> + 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.
> @@ -1438,6 +1701,9 @@
> }
> while (patch);
>
> + SVN_ERR(maybe_condense_deleted_targets(&targets, btn->ctx->wc_ctx,
> + scratch_pool, result_pool));
> +
> /* Install patched targets into the working copy. */
> for (i = 0; i < targets->nelts; i++)
> {
> @@ -1453,7 +1719,8 @@
> 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));
> - SVN_ERR(svn_diff__close_patch(target->patch));
> + if (target->patch)
> + SVN_ERR(svn_diff__close_patch(target->patch));
> }
>
> svn_pool_destroy(iterpool);