You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Daniel Rall <dl...@finemaltcoding.com> on 2008/01/07 20:44:03 UTC
Re: svn commit: r28768 - in branches/issue-2897/subversion: libsvn_client tests/cmdline
Comments inline below.
On Mon, 07 Jan 2008, kameshj@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));
> }
> }
> }
...
Re: svn commit: r28768 - in branches/issue-2897/subversion: libsvn_client
tests/cmdline
Posted by Julian Foad <ju...@btopenworld.com>.
Kamesh Jayachandran wrote:
> Daniel Rall wrote:
>>--- branches/issue-2897/subversion/libsvn_client/merge.c (original)
>>+++ branches/issue-2897/subversion/libsvn_client/merge.c Mon Jan 7 07:32:32 2008
>>+summarize_reflected_ranges(svn_depth_t depth,
>>+ merge_cmd_baton_t *merge_b)
>>+{
>> This is also going to be a potentially really expensive operation, and
>> should be documented as such in the API's doc string.
>
> I agree it is expensive, we run such summaries *only* for reflective
> revisions *not always*. How come documenting it helps?
Most of Subversion is fast and efficient. Wherever we know that a Subversion
API can behave in an unexpected or undesirable way, we aim to tell the API user
so that they will not be surprised and can make an informed choice of whether
to use it, or find a different way, or try to improve the implementation,
without having to read and understand all of the code. For example, the user
might want to display a progress indicator if calling this in a loop. And it
will help the programmer who next reads this code to understand that the
expensive operation is a known disadvantage that we have accepted for the time
being, not something that we accidentally allowed to happen.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r28768 - in branches/issue-2897/subversion: libsvn_client
tests/cmdline
Posted by Kamesh Jayachandran <ka...@collab.net>.
>> apr_array_header_t *reflected_ranges;
>>
>
> I'd like to see a newline here.
>
Fixed in r28784.
>> + 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.
>
Posted to dev list seeking for how to format the longish_func_call with
very long parameter.
> This is also going to be a potentially really expensive operation, and should
> be documented as such in the API's doc string.
>
>
I agree it is expensive, we run such summaries *only* for reflective
revisions *not always*. How come documenting it helps?
>> + }
>>
>
> To avoid a memory leak, we need to destroy the loop's pool here:
>
> svn_pool_destroy(iterpool);
>
Fixed in r28786.
> Null -> NULL
> enough!!. -> sufficient,
>
Fixed in r28784.
>
>> + As *reflected* summary and reflective merge drive can not give
>>
>
> as
>
Fixed in r28784.
>> + 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?
>
Fixed in r28785.
>
>> +
>> + /* Checking for non-Null summary_kind should be enough!!.
>> + As *reflected* summary and reflective merge drive can not give
>> + two different summaries. */
>>
>
> Ditto.
>
Fixed in r28784.
Thanks Dan.
With regards
Kamesh Jayachandran
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org