On Jan 7, 2008 7:32 AM, <kameshj_at_tigris.org> wrote:
> Author: kameshj
> Date: Mon Jan 7 07:32:32 2008
> New Revision: 28768
>
> Log:
> On the issue-2897 branch:
>
> Handle reflective file/directory additions sensibly.
>
> * subversion/libsvn_client/merge.c
> (merge_cmd_baton_t): Add new member 'reflective_rev_affected_paths'.
> (get_diff_summary_func_cb, summarize_reflected_ranges): New functions.
> (reflective_merge_file_added): Refer 'cumulative reflected merges for this
> reflective merge' and decide whether to add this file or not.
> (reflective_merge_dir_added): Refer 'cumulative reflected merges for this
> reflective merge' and decide whether to add this directory or not.
> (drive_merge_report_editor): Summarize the reflected merges of reflective
> revision prior to merging the reflective revision.
> (do_merge): Initialize 'merge_cmd_baton_t.reflective_rev_affected_paths'.
>
> * subversion/libsvn_client/repos_diff.c
> (close_directory): If there is no regular prop change don't even call
> props_changed call back.
>
> * subversion/tests/cmdline/merge_tests.py
> (test_list): Remove XFail marker from
> 'merge_non_reflective_changes_from_reflective_rev'.
> Add XFail marker on 'reflective_merge_on_reincarnated_target' as this
> commit breaks it.
>
>
> Modified:
> branches/issue-2897/subversion/libsvn_client/merge.c
> branches/issue-2897/subversion/libsvn_client/repos_diff.c
> branches/issue-2897/subversion/tests/cmdline/merge_tests.py
>
> Modified: branches/issue-2897/subversion/libsvn_client/merge.c
> URL: http://svn.collab.net/viewvc/svn/branches/issue-2897/subversion/libsvn_client/merge.c?pathrev=28768&r1=28767&r2=28768
> ==============================================================================
> --- branches/issue-2897/subversion/libsvn_client/merge.c (original)
> +++ branches/issue-2897/subversion/libsvn_client/merge.c Mon Jan 7 07:32:32 2008
> @@ -229,6 +229,11 @@
> * It contains individual elements of type ' svn_merge_range_t *'.
> */
> apr_array_header_t *reflected_ranges;
> + /* Before running the actual reflective rev merge we do get
> + * a summary of merge and store the to be affected paths as keys in
> + * this hash with value being of type svn_client_diff_summarize_kind_t *.
> + */
I'd rephrase this as something more like "A hash mapping paths to
(svn_client_diff_summarize_kind_t *) values describing how they were
affected by the reflective range under consideration."
> + apr_hash_t *reflective_rev_affected_paths;
> apr_pool_t *pool;
> } merge_cmd_baton_t;
>
> @@ -371,6 +376,83 @@
> return err;
> }
>
> +
> +/* Records the affected path in BATON hash,
> + * implements svn_client_diff_summarize_func_t interface. All persistent
> + * allocations occur from BATON hash's pool. */
> +static svn_error_t *
> +get_diff_summary_func_cb(const svn_client_diff_summarize_t *summary,
> + void *baton,
> + apr_pool_t *pool)
> +{
> + apr_hash_t *reflective_rev_affected_paths = baton;
> + const char *path;
> + svn_client_diff_summarize_kind_t *summary_kind, *orig_summary_kind;
> + apr_pool_t *persist_pool = apr_hash_pool_get(reflective_rev_affected_paths);
> + summary_kind = apr_pcalloc(persist_pool, sizeof(*summary_kind));
> +
> + path = apr_pstrdup(persist_pool, summary->path);
> + *summary_kind = summary->summarize_kind;
> + orig_summary_kind = apr_hash_get(reflective_rev_affected_paths, path,
> + APR_HASH_KEY_STRING);
> + if (orig_summary_kind)
> + {
> + if (((*orig_summary_kind == svn_client_diff_summarize_kind_added)
> + && (*summary_kind == svn_client_diff_summarize_kind_deleted))
> + ||
> + ((*orig_summary_kind == svn_client_diff_summarize_kind_deleted)
> + && (*summary_kind == svn_client_diff_summarize_kind_added)))
> + apr_hash_set(reflective_rev_affected_paths, path, APR_HASH_KEY_STRING,
> + NULL);
> + }
> + else
> + apr_hash_set(reflective_rev_affected_paths, path, APR_HASH_KEY_STRING,
> + summary_kind);
> +
> + return SVN_NO_ERROR;
> +}
> +
> +
> +/* Summarize MERGE_B->reflected_ranges from MERGE_B->target url to
> + MERGE_B->reflective_rev_affected_paths. */
> +static svn_error_t *
> +summarize_reflected_ranges(svn_depth_t depth,
> + merge_cmd_baton_t *merge_b)
> +{
> + int i;
> + const char *target_url;
> + apr_pool_t *iterpool = svn_pool_create(merge_b->pool);
> +
> + SVN_ERR(svn_ra_get_session_url(merge_b->target_ra_session,
> + &target_url,
> + merge_b->pool));
> + svn_hash__clear(merge_b->reflective_rev_affected_paths);
How often does this function get called? Since the keys and values of
this hash are allocated in the top-level pool passed to do_merge, it
seems like this might lead to inappropriate unbounded memory usage.
> + for (i = 0; i < merge_b->reflected_ranges->nelts; i++)
> + {
> + svn_merge_range_t *range;
> + svn_opt_revision_t opt_revision1;
> + svn_opt_revision_t opt_revision2;
> + svn_opt_revision_t peg_revision;
> + svn_pool_clear(iterpool);
> + range = APR_ARRAY_IDX(merge_b->reflected_ranges, i, svn_merge_range_t *);
> + peg_revision.kind = svn_opt_revision_number;
> + peg_revision.value.number = range->start;
> + opt_revision1.kind = svn_opt_revision_number;
> + opt_revision1.value.number = range->start;
> + opt_revision2.kind = svn_opt_revision_number;
> + opt_revision2.value.number = range->end;
> + SVN_ERR(svn_client_diff_summarize_peg2(target_url, &peg_revision,
> + &opt_revision1, &opt_revision2,
> + depth,
> + merge_b->ignore_ancestry,
> + get_diff_summary_func_cb,
> + merge_b->reflective_rev_affected_paths,
> + merge_b->ctx,
> + iterpool));
> + }
Others have pointed out that this should be documented as an expensive
operation. I'd take it a step farther: it's worth considering whether
a many-range-diff-summarize RA API would be helpful here.
--dave
--
David Glasser | glasser@davidglasser.net | http://www.davidglasser.net/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe_at_subversion.tigris.org
For additional commands, e-mail: dev-help_at_subversion.tigris.org
Received on 2008-02-01 01:26:52 CET