[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: svn commit: r26243 - in trunk/subversion: libsvn_client tests/cmdline

From: Daniel Rall <dlr_at_finemaltcoding.com>
Date: 2007-08-24 02:47:56 CEST

On Aug 22, 2007, at 2:21 AM, kameshj@tigris.org wrote:
...
> Fix issue 2876 - "Merge on parent having subtree with differing
> mergeinfo
> which is getting deleted by the current merge fails".

Karl and I were looking at the description of this issue in Mission
Creek Cafe yesterday, and were both quite puzzled by it. I think it
really needs to be re-worded into something a bit more digestible. :)

> * subversion/libsvn_client/merge.c
> (elide_children, do_merge):
> No need to do anything on a children_with_mergeinfo's item that
> is set to NULL.
> (struct get_diff_summary_baton):
> New baton passed to 'svn_client_diff_summarize_func_t' callback.
> (get_diff_summary_func_cb):
> New diff summarize callback interface.

I'm quite concerned about the use of 'diff --summarize' as part of
'merge'...

> (discover_and_merge_children):
> Summarize the merge. If any path is found to be deleted remove
> it from
> '*children_with_mergeinfo' and continue with next subtree
> without doing
> any merge on this to be deleted subtree.
>
> * subversion/tests/cmdline/merge_tests.py
> (merge_fails_if_subtree_is_deleted_on_src): Fix the testcase.
> Force the merge so that 'locally modified file' won't get
> skipped upon
> deletion by merge.
> (test_list): remove XFail marker on
> 'merge_fails_if_subtree_is_deleted_on_src'
...
> --- trunk/subversion/libsvn_client/merge.c (original)
> +++ trunk/subversion/libsvn_client/merge.c Wed Aug 22 02:21:51 2007
> @@ -1444,6 +1444,10 @@
> merge_path_t *child = APR_ARRAY_IDX
> (children_with_mergeinfo, i,
> merge_path_t *);
> svn_pool_clear(iterpool);
> +
> + if (!child)
> + continue;
> +
> if (i == 0)
> {
> /* children_with_mergeinfo is sorted depth
> @@ -2271,6 +2275,9 @@
> merge_path_t *child =
> APR_ARRAY_IDX(children_with_mergeinfo, j,
> merge_path_t *);
>
> + if (!child)
> + continue;

In what cases are NULL merge_path_t *'s added to
children_with_mergeinfo?

...
> +/* A baton for get_diff_summary_func_cb. */
> +struct get_diff_summary_baton
> +{
> + /* Target path. */
> + const char* target_path;
> + /* Hash of Deleted paths. */
> + apr_hash_t* deleted_paths;
> + /* Pool to use for allocations. */

A pool is always used for allocations. :-p
The interesting distinction is between "temporary" or "all" allocations.

> + apr_pool_t *pool;
> +};
> +
> +/* Records the path getting deleted in BATON->deleted_paths,
> implements
> + * svn_client_diff_summarize_func_t interface. */
> +static svn_error_t *
> +get_diff_summary_func_cb(const svn_client_diff_summarize_t *summary,
> + void *baton,
> + apr_pool_t *pool)
> +{
> + struct get_diff_summary_baton *sb = baton;
> + const char* path;
> +
> + path = svn_path_join(sb->target_path, summary->path, sb->pool);

How about:

     const char *path = svn_path_join(sb->target_path, summary->path,
sb->pool);

It's all in one-line, and puts the * in the typical spot.

> +
> + if (summary->summarize_kind ==
> svn_client_diff_summarize_kind_deleted)
> + apr_hash_set(sb->deleted_paths, path, APR_HASH_KEY_STRING, "");

Since we're using this apr_hash_t as a set, how about using path as
both the key and value?

> +
> + return SVN_NO_ERROR;
> +}
> +
> +
> /* Fill *CHILDREN_WITH_MERGEINFO with child paths (const char *)
> which
> might have intersecting merges because they have explicit working
> svn:mergeinfo and/or are switched. Here the paths are arranged
> in a depth
> @@ -3304,9 +3342,17 @@
> const svn_wc_entry_t *child_entry;
> int merge_target_len = strlen(merge_cmd_baton->target);
> int i;
> + apr_hash_t *deleted_subtrees;

deleted_subtrees seems extraneous -- can't we use sb.deleted_paths in
its place?

> + struct get_diff_summary_baton sb;
> + svn_opt_revision_t peg_revision;
> const char* merge_src_canon_path = apr_pstrdup(pool,
>
> parent_merge_source_url +
> strlen
> (wc_root_url));
> + deleted_subtrees = apr_hash_make(pool);
> + sb.target_path = merge_cmd_baton->target;
> + sb.deleted_paths = deleted_subtrees;
> + sb.pool = pool;
> + peg_revision.kind = svn_opt_revision_head;
>
> *children_with_mergeinfo = apr_array_make(pool, 0, sizeof
> (merge_path_t *));
> SVN_ERR(get_mergeinfo_paths(*children_with_mergeinfo,
> @@ -3315,6 +3361,17 @@
> parent_entry, adm_access,
> merge_cmd_baton->ctx, pool));
>
> + SVN_ERR(svn_client_diff_summarize_peg2(parent_merge_source_url,
> + &peg_revision,
> + revision1,
> + revision2,
> + depth,
> + ignore_ancestry,
> + get_diff_summary_func_cb,
> + &sb,
> + merge_cmd_baton->ctx,
> + pool));
> +

Invoking diff-summarize here significantly increases the cost of
performing a merge (almost as expensive as the merge itself).

Is there another way to do this?

> for (i = 0; i < (*children_with_mergeinfo)->nelts; i++)
> {
> const char *child_repos_path;
> @@ -3328,6 +3385,17 @@
> merge_cmd_baton->existing_mergeinfo = TRUE;
> continue;
> }
> +
> + /* If the path is getting deleted don't bother doing subtree
> merge.
> + * Just remove it from children_sw_or_with_mergeinfo, so
> that merge
> + * on a parent can handle it in a usual way.
> + */
> + if (apr_hash_get(deleted_subtrees, child->path,
> APR_HASH_KEY_STRING))
> + {
> + APR_ARRAY_IDX(*children_with_mergeinfo, i, merge_path_t
> *) = NULL;
> + continue;
> + }
> +
> SVN_ERR(svn_wc__entry_versioned(&child_entry, child->path,
> adm_access,
> FALSE, pool));
> child_repos_path = child->path +
...

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Fri Aug 24 02:38:29 2007

This is an archived mail posted to the Subversion Dev mailing list.