>> 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_at_subversion.tigris.org
For additional commands, e-mail: dev-help_at_subversion.tigris.org
Received on 2008-01-08 17:13:27 CET