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.
Received on 2010-03-03 16:25:49 CET