Daniel Rall wrote:
> 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. :)
Fixed it as 'Make subtree merge smarter enough by not to do series of
merge on subtree that is eventually getting deleted at a later stage of
merge.' in IZ.
>
>> * 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'...
This is what Peter Lundblad as suggested via
http://subversion.tigris.org/servlets/BrowseList?list=dev&by=thread&from=565086.
I could not think any other better way to catch the 'Deletions'.
>
>> (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?
When the 'subtree with differing mergeinfo' is getting deleted by the
merge(as hinted by 'diff summarize'), we just make the corresponding
list entry null.
>
> ...
>> +/* 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.
This is for permanent allocation of 'keys' in baton->deleted_paths's keys.
For temporary allocations we need not pass 'pool' at all, as callback
caller himself passes one. We pass 'pool' in the baton only for
'permanent 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.
Fixed in r26284.
>
>> +
>> + 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?
May be we can, But I could see no use. My purpose is to fast lookup the
existence of a path from the hash not its 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?
Yes. Fixed in r26284.
>
>> + 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?
I could not think one.
>
>> 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 +
> ...
>
>
Thanks for your review.
With regards
Kamesh Jayachandran
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Fri Aug 24 16:24:56 2007