Dan,
Thanks for the review.
My comments are inline.
Daniel Rall wrote:
> Kamesh, here's a brief review of your approach for dealing with
> children of merge targets which have merge info which differs from
> that of the parent.
>
> Index: subversion/libsvn_client/diff.c
> ===================================================================
> --- subversion/libsvn_client/diff.c (revision 22779)
> +++ subversion/libsvn_client/diff.c (working copy)
> ...
> + SUBTREES_HAVING_MERGEINFO might have the subpaths which might have
>
> Avoid repeating words like "might" twice in the same sentence.
>
Sure will correct it.
> + intersecting merges, so we run the diff editor in such a way that
> + it leaves off those subpaths.
> + i.e reporter->set_path(report_baton, subtree_repospath,
> + same_rev_given_to_svn_ra_do_diff2, FALSE,
> + NULL, pool));
> + SUBTREES_HAVING_MERGEINFO is assumed to be non-NULL.
>
> We don't only want to consider sub-trees here; we want to consider any
> child of the merge target which has merge info. Files can have their
> own merge info, which overrides any merge info inherited from their
> (grand-)parent directory.
>
> Let's name this parameter CHILDREN_WITH_MERGEINFO instead. I've tried
> to make the doc string more clear, too.
>
Fine. I was under the thought that 'SUBTREE' includes both sub-sub
directories and files(tree with no child at all). CHILDREN looks clear
though.
> */
> static svn_error_t *
> do_merge(const char *initial_URL1,
> ...
> @@ -2400,7 +2410,41 @@
> SVN_ERR(reporter->set_path(report_baton, "",
> is_revert ? r->start : r->start - 1,
> FALSE, NULL, pool));
> + if (notify_b.same_urls)
> + {
> + apr_ssize_t target_wcpath_len = strlen(target_wcpath);
>
> Why use apr_ssize_t instead of apr_size_t?
>
Copy Paste mistake :).
>
> + for (hi = apr_hash_first(pool, subtrees_having_mergeinfo);
>
> The iterator over this hash doesn't need to be thread-safe, so we
> could avoid passing in pool to apr_hash_first().
>
What if in a hypothetical case where two simultaneous merges from the
same application context on two different threads exercising the same
portion of code?(A GUI built on top of JAVAHL?)
> + hi;
> + hi = apr_hash_next(hi))
> + {
> + const void *key;
> + apr_ssize_t klen;
> + const char *subtree_repospath;
> + const char *subtree_wc_target;
> +
> + apr_hash_this(hi, &key, &klen, NULL);
> +
> + subtree_wc_target = key;
> + //set no-op set-path only on subset.
> + if ((klen < target_wcpath_len) ||
> + (strcmp(subtree_wc_target,
> + target_wcpath) == 0) ||
> + (strncmp(subtree_wc_target,
> + target_wcpath,
> + target_wcpath_len) != 0))
>
> Can't we use something from svn_path.h for this complex test?
> svn_path_is_child() looks like it might work.
>
It might work. But I don't favour it here. Because it does extra things
which I am not interested in (Giving a tail portions since match.).
Given that I know about my input here('subtree_wc_target',
'target_wcpath'), I don't want it to undergo the rigorous ':' checks for
windows.
You seemed to have removed this comment prior to this check '
+ //set no-op set-path only on subset.'
May be it could be made as
+ //set no-op set-path only on child of target_wc_path not on itself and on non-childs.'
> + {
> + continue;
> + }
> +
> + subtree_repospath = key + target_wcpath_len + 1;
> +
> + SVN_ERR(reporter->set_path(report_baton, subtree_repospath,
> + is_revert ? r->end - 1 : r->end,
> + FALSE, NULL, pool));
> + }
> + }
>
> This entire block could use an inline comment indicating what it's
> intended to do.
>
>
Yes would do.
> ...
> +/*
> + Fill SUBTREES_HAVING_MERGEINFO with subpaths which might have
> + intersecting merges.
> + SUBTREES_HAVING_MERGEINFO is assumed to be non-NULL.
> + For each such subtree calls do_merge or do_single_file_merge
> + based on the kind of subtree with the appropriate arguments.
>
> This API is slightly incohesive -- it serves two purposes:
>
> 1) Gets the list of children with merge info.
>
> 2) Performs the appropriate merges for each child.
>
> Typically, this isn't a good way to go. However, we can consider #1
> to be a return value listing "what was changed", so I think we can let
> this one slide. We might want to be a little more explicit about this
> in the doc string, though, or name the API to reflect this
> (e.g. discover_and_do_child_merges?).
>
Sounds good.
> + Uses PARENT_WC_ENTRY and ADM_ACCESS to fill the
> + 'SUBTREES_HAVING_MERGEINFO'.
> + Uses the same MERGE_CMD_BATON as the caller(svn_client_merge3
> + and svn_client_merge_peg3).
> + Receives PARENT_WC_URL, INITIAL_PATH1, REVISION1, INITIAL_PATH2,
> + REVISION2, PEG_REVISION, RECURSE, IGNORE_ANCESTRY, ADM_ACCESS,
> + MERGE_CMD_BATON to cascade it to do_merge and do_single_file_merge.
> + All allocation occurs in POOL.
> +*/
> +static svn_error_t *
> +do_subtree_merges(apr_hash_t *subtrees_having_mergeinfo,
> + const svn_wc_entry_t *parent_wc_entry,
> + const char *parent_wc_url,
> + const char *initial_path1,
> + const svn_opt_revision_t *revision1,
> + const char *initial_path2,
> + const svn_opt_revision_t *revision2,
> + const svn_opt_revision_t *peg_revision,
> + svn_boolean_t recurse,
> + svn_boolean_t ignore_ancestry,
> + svn_wc_adm_access_t *adm_access,
> + struct merge_cmd_baton *merge_cmd_baton,
> + apr_pool_t *pool)
> +{
> + apr_hash_index_t *hi;
> + const svn_wc_entry_t *subtree_entry;
> +
> + SVN_ERR(svn_client__get_prop_from_wc(subtrees_having_mergeinfo,
> + SVN_PROP_MERGE_INFO,
> + merge_cmd_baton->target,
> + FALSE,
> + parent_wc_entry,
> + adm_access,
> + TRUE,
> + merge_cmd_baton->ctx,
> + pool));
> + for (hi = apr_hash_first(pool, subtrees_having_mergeinfo);
> + hi;
> + hi = apr_hash_next(hi))
> + {
> + const void *key;
> + const char *subtree_repospath;
> + const char *subtree_wc_target;
> + const char *subtree_URL;
> +
> + apr_hash_this(hi, &key, NULL, NULL);
> +
> + subtree_wc_target = key;
> + if (strcmp(subtree_wc_target, merge_cmd_baton->target) == 0)
> + {
> + continue;
> + }
> + SVN_ERR(svn_wc_entry(&subtree_entry, subtree_wc_target,
> + adm_access, FALSE, pool));
> + if (subtree_entry == NULL)
> + return svn_error_createf(SVN_ERR_UNVERSIONED_RESOURCE, NULL,
> + _("'%s' is not under version control"),
> + svn_path_local_style(subtree_wc_target, pool));
> +
> + subtree_repospath = key + strlen(merge_cmd_baton->target) + 1;
> + subtree_URL = svn_path_join(parent_wc_url, subtree_repospath, pool);
> + if (subtree_entry->kind == svn_node_file)
> + {
> + SVN_ERR(do_single_file_merge(subtree_URL, initial_path1, revision1,
> + subtree_URL, initial_path2, revision2,
> + peg_revision,
> + subtree_wc_target,
> + adm_access,
> + merge_cmd_baton,
> + pool));
> + }
> + else if (subtree_entry->kind == svn_node_dir)
> + {
> + SVN_ERR(do_merge(subtree_URL,
> + initial_path1,
> + revision1,
> + subtree_URL,
> + initial_path2,
> + revision2,
> + peg_revision,
> + subtree_wc_target,
> + adm_access,
> + recurse,
> + ignore_ancestry,
> + &merge_callbacks,
> + merge_cmd_baton,
> + subtrees_having_mergeinfo,
> + pool));
> + }
> + }
> + return SVN_NO_ERROR;
> +}
> +
> svn_error_t *
> svn_client_merge3(const char *source1,
> const svn_opt_revision_t *revision1,
> @@ -3461,6 +3605,7 @@
> const char *URL1, *URL2;
> const char *path1, *path2;
> svn_opt_revision_t peg_revision;
> + apr_hash_t *subtrees_having_mergeinfo = apr_hash_make(pool);
>
> /* This is not a pegged merge. */
> peg_revision.kind = svn_opt_revision_unspecified;
> @@ -3547,6 +3692,25 @@
> recursive diff-editor thing. */
> else if (entry->kind == svn_node_dir)
> {
> + if (strcmp(URL1, URL2) == 0)
>
> Inside of do_merge(), we perform some calculations on the two URLs
> before using them to set the merge_baton.same_urls field. The
> same_urls field is effectively what you're trying to use here to get
> do_subtree_merges(). Why the difference? Is it important?
>
It is just to do away with Three merge scenarios where these 'Subtree
exclusion' does not make sense atleast currently.
> + {
> + SVN_ERR(do_subtree_merges(subtrees_having_mergeinfo,
> ...
> + }
> +
> + /* Merge of a actual target.*/
> +
> SVN_ERR(do_merge(URL1,
> ...
> @@ -3696,6 +3862,22 @@
> recursive diff-editor thing. */
> else if (entry->kind == svn_node_dir)
> {
> + SVN_ERR(do_subtree_merges(subtrees_having_mergeinfo,
> + entry,
> + URL,
> + path,
> + revision1,
> + path,
> + revision2,
> + peg_revision,
> + recurse,
> + ignore_ancestry,
> + adm_access,
> + &merge_cmd_baton,
> + pool));
>
> And why does svn_client_merge_peg3() not gate do_subtree_merges() in
> the same style of "if (same_urls)" conditional?
>
We have only one URL in case of svn_client_merge_peg3() (Not sure why,
but that is the way it is).
> + /* Merge of a actual target.*/
> +
> SVN_ERR(do_merge(URL,
> ...
>
>
>
> In do_merge() and do_single_file_merge(), we also have:
>
> /* When only recording merge info, we don't perform an actual
> merge for the specified range. */
> if (merge_b->record_only)
> {
> /* ### TODO: Support sub-tree merge info. */
> /* ### Handle WC-local reverts which have modified our merge
> ### info. */
> apr_hash_t *merges;
> SVN_ERR(determine_merges_performed(&merges, target_wcpath, &range,
> ¬ify_b, pool));
> return update_wc_merge_info(target_wcpath, entry, rel_path,
> merges, is_revert, ra_session,
> adm_access, ctx, pool);
> }
>
> Is this TODO comment about sub-tree merge info still relevant? If so,
> what needs to be changed?
>
Yes it can be removed.
I have a less clue about what that second comment line means.
Thanks
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 Jan 19 16:43:24 2007