Re: svn commit: r28768 - in branches/issue-2897/subversion: libsvn_client tests/cmdline

Comments inline below.

On Mon, 07 Jan 2008, kameshj_at_tigris.org wrote:
...
> 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.
...
> --- 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;

I'd like to see a newline here.

> + /* 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 *.
> + */
> + apr_hash_t *reflective_rev_affected_paths;
> apr_pool_t *pool;
> } merge_cmd_baton_t;
...
> +/* 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);
> + 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));

Level of indentation looks off here.

This is also going to be a potentially really expensive operation, and should
be documented as such in the API's doc string.

> + }

To avoid a memory leak, we need to destroy the loop's pool here:

     svn_pool_destroy(iterpool);

> + return SVN_NO_ERROR;
> +}
...
> @@ -844,10 +926,18 @@
> void *baton)
> {
> merge_cmd_baton_t *merge_b = baton;
> - svn_node_kind_t kind;
> -
> - SVN_ERR(svn_io_check_path(mine, &kind, merge_b->pool));
> - if (kind == svn_node_none)
> + int merge_target_len = strlen(merge_b->target);
> + const char *file_path_relative_to_target =
> + mine + (merge_target_len ? merge_target_len + 1 : 0);
> + svn_client_diff_summarize_kind_t *summary_kind =
> + apr_hash_get(merge_b->reflective_rev_affected_paths,
> + file_path_relative_to_target,
> + APR_HASH_KEY_STRING);
> +
> + /* Checking for non-Null summary_kind should be enough!!.

Null -> NULL
enough!!. -> sufficient,

> + As *reflected* summary and reflective merge drive can not give

as

> + two different summaries. */
> + if (!summary_kind)
> SVN_ERR(merge_file_added(adm_access, content_state, prop_state, mine,
> older, yours, rev1, rev2, mimetype1, mimetype2,
> prop_changes, original_props, baton));
> @@ -1083,9 +1173,18 @@
> void *baton)
> {
> merge_cmd_baton_t *merge_b = baton;
> - svn_node_kind_t kind;
> - SVN_ERR(svn_io_check_path(path, &kind, merge_b->pool));
> - if (kind == svn_node_none)
> + int merge_target_len = strlen(merge_b->target);
> + const char *file_path_relative_to_target =
> + path + (merge_target_len ? merge_target_len + 1 : 0);
> + svn_client_diff_summarize_kind_t *summary_kind =
> + apr_hash_get(merge_b->reflective_rev_affected_paths,
> + file_path_relative_to_target,
> + APR_HASH_KEY_STRING);

This code looks redundant with what's above. Use a helper function?

> +
> + /* Checking for non-Null summary_kind should be enough!!.
> + As *reflected* summary and reflective merge drive can not give
> + two different summaries. */

Ditto.

> + if (!summary_kind)
> SVN_ERR(merge_dir_added(adm_access, state, path, rev, baton));
> else
> *state = svn_wc_notify_state_unchanged;
> @@ -2537,6 +2636,7 @@
> {
> merge_b->reflected_ranges = range_info->reflected_ranges;
> callbacks = &reflective_merge_callbacks;
> + SVN_ERR(summarize_reflected_ranges(depth, merge_b));
> }
> }
> }
...