You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Kamesh Jayachandran <ka...@collab.net> on 2008/01/07 16:27:25 UTC
Re: svn commit: r28561 - in branches/issue-2897/subversion: libsvn_client
tests/cmdline
Hi David,
Responding to items that I did not in my earlier response.
>> + apr_pool_t *subpool = svn_pool_create(merge_b->pool);
>> + SVN_ERR(svn_io_temp_dir(&temp_dir, subpool));
>> +
>> + if (merge_b->merge_options)
>> + SVN_ERR(svn_diff_file_options_parse(options,
>> + merge_b->merge_options, subpool));
>> + else
>> + options = svn_diff_file_options_create(subpool);
>> + for (i = 0; i < merge_b->reflected_ranges->nelts; i++)
>>
>
> Loop without iterpool.
>
Fixed in r28711.
> Also, um. I might be paranoid, but I'm not really comfortable with
> you overwriting "older" like this. I'd rather this function just make
> a series of temporary files and return the last filename to
> reflective_merge_file_changed. There's nothing guaranteeing that
> "older" might not be something that some other part of the program
> cares about. Just figuring out what context this function can be
> called in is non-trivial; specifically, "older" gets its value from
> the diff callback file_changed call, and I notice that in
> libsvn_wc/diff.c(file_diff) it can get called with "older" equal to
> the text base path! Maybe this code path can't ever reach the merge
> code, but I'm still not comfortable with this...
>
Will fix it in subsequent commit.
>> /* A svn_wc_diff_callbacks2_t function. */
>> static svn_error_t *
>> +reflective_merge_file_added(svn_wc_adm_access_t *adm_access,
>> + svn_wc_notify_state_t *content_state,
>> + svn_wc_notify_state_t *prop_state,
>> + const char *mine,
>> + const char *older,
>> + const char *yours,
>> + svn_revnum_t rev1,
>> + svn_revnum_t rev2,
>> + const char *mimetype1,
>> + const char *mimetype2,
>> + const apr_array_header_t *prop_changes,
>> + apr_hash_t *original_props,
>> + 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));
>>
>
> I don't really get what this implementation is doing. Is it basically
> "the same as the normal merge_file_added, except if the file is
> already there then nothing happens and it's OK"? Why?
>
It was a faulty implementation, corrected at r28768.
>> +reflective_merge_file_deleted(svn_wc_adm_access_t *adm_access,
>> + svn_wc_notify_state_t *state,
>> + const char *mine,
>> + const char *older,
>> + const char *yours,
>> + const char *mimetype1,
>> + const char *mimetype2,
>> + apr_hash_t *original_props,
>> + 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_file)
>> + SVN_ERR(merge_file_deleted(adm_access, state, mine, older, yours,
>> + mimetype1, mimetype2, original_props, baton));
>>
>
> Similarly here. Maybe just a comment somewhere explaining the point
> of the reflective callbacks might help...
>
>
It is also faulty will correct in subsequent commit.
>> static svn_error_t *
>> +reflective_merge_dir_added(svn_wc_adm_access_t *adm_access,
>> + svn_wc_notify_state_t *state,
>> + const char *path,
>> + svn_revnum_t rev,
>> + 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)
>> + SVN_ERR(merge_dir_added(adm_access, state, path, rev, baton));
>>
>
> Yup, still don't get it :)
>
It was a faulty implementation, corrected at r28768.
>>
>> +/* A svn_wc_diff_callbacks2_t function. */
>> +static svn_error_t *
>> +reflective_merge_dir_deleted(svn_wc_adm_access_t *adm_access,
>> + svn_wc_notify_state_t *state,
>> + const char *path,
>> + 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_dir)
>> + SVN_ERR(merge_dir_deleted(adm_access, state, path, baton));
>>
>
> ... and still :)
>
It is also faulty, will correct in subsequent commit.
>
>> +static const svn_wc_diff_callbacks2_t
>> +reflective_merge_callbacks =
>> + {
>> + reflective_merge_file_changed,
>> + reflective_merge_file_added,
>> + reflective_merge_file_deleted,
>> + reflective_merge_dir_added,
>> + reflective_merge_dir_deleted,
>> + merge_props_changed
>>
>
> Hmm, why no reflective props changed?
>
Except 'svn:mergeinfo' props rest of the properties are treated like
binary ones and hence no-contextual merging and hence *no* 'extract and
apply non-reflective changes'.
>
>> + };
>>
>> /*-----------------------------------------------------------------------*/
>>
>> @@ -1091,12 +1308,15 @@ notification_receiver(void *baton, const
>> && notify_b->merge_b->target_has_dummy_merge_range))
>> {
>> svn_wc_notify_t *notify_merge_begin;
>> + svn_client__remaining_range_info_t *range_info;
>> notify_merge_begin =
>> svn_wc_create_notify(child->path,
>> svn_wc_notify_merge_begin, pool);
>> - notify_merge_begin->merge_range =
>> - APR_ARRAY_IDX(child->remaining_ranges, 0,
>> - svn_merge_range_t *);
>> + range_info =
>> + APR_ARRAY_IDX(child->remaining_ranges, 0,
>> + svn_client__remaining_range_info_t *);
>> + notify_merge_begin->merge_range = range_info->range;
>> +
>> if (notify_b->wrapped_func)
>> (*notify_b->wrapped_func)(notify_b->wrapped_baton,
>> notify_merge_begin, pool);
>> @@ -1170,6 +1390,12 @@ notification_receiver(void *baton, const
>>
>> NOTE: This should only be called when honoring mergeinfo.
>>
>> + *REFLECTIVE_RANGELIST will be populated with commits post merge from
>> + target on source within revision1:revision2.
>> +
>>
>
> Again, what are the types of the array elements? It's confusing to
> call an array a "rangelist" since that already means something.
>
IIUC We mean rangelist as list a.k.a apr_array and all rangelist's value
type being 'svn_merge_range_t *'.
Updated doc string to state the array value types at r28769.
>
>> + *REFLECTED_RANGES_LIST will be populated with merge ranges that are
>> + merged from target to source within revision1:revision2.
>> +
>>
>
> Ditto. And state that they match up with the previous one?
>
>
Fixed in r28770.
>> @@ -1231,13 +1457,13 @@ filter_reflected_revisions(apr_array_hea
>> drive_merge_report_editor()'s application of the editor to the WC
>> -- by subtracting revisions which have already been merged from
>> MERGEINFO_PATH into the working copy from the requested range(s)
>> - REQUESTED_MERGE, and storing what's left in REMAINING_RANGES.
>> + REQUESTED_MERGE, and storing what's left in RANGES_TO_MERGE.
>> TARGET_MERGEINFO may be NULL.
>>
>> NOTE: This should only be called when honoring mergeinfo.
>> */
>> static svn_error_t *
>> -filter_merged_revisions(apr_array_header_t **remaining_ranges,
>> +filter_merged_revisions(apr_array_header_t **ranges_to_merge,
>> const char *mergeinfo_path,
>> apr_hash_t *target_mergeinfo,
>> apr_hash_t *implicit_mergeinfo,
>> @@ -1265,20 +1491,20 @@ filter_merged_revisions(apr_array_header
>> revert to work properly. */
>> requested_merge = svn_rangelist_dup(requested_merge, pool);
>> SVN_ERR(svn_rangelist_reverse(requested_merge, pool));
>> - SVN_ERR(svn_rangelist_intersect(remaining_ranges,
>> + SVN_ERR(svn_rangelist_intersect(ranges_to_merge,
>> target_rangelist,
>> requested_merge, pool));
>> - SVN_ERR(svn_rangelist_reverse(*remaining_ranges, pool));
>> + SVN_ERR(svn_rangelist_reverse(*ranges_to_merge, pool));
>> }
>> else
>> {
>> - *remaining_ranges =
>> + *ranges_to_merge =
>> apr_array_make(pool, 1, sizeof(svn_merge_range_t *));
>> }
>> }
>> else
>> {
>> - *remaining_ranges = requested_merge;
>> + *ranges_to_merge = requested_merge;
>>
>> /* ### TODO: Which evil shall we choose?
>> ###
>> @@ -1312,7 +1538,7 @@ filter_merged_revisions(apr_array_header
>> #endif
>>
>> if (target_rangelist)
>> - SVN_ERR(svn_rangelist_remove(remaining_ranges, target_rangelist,
>> + SVN_ERR(svn_rangelist_remove(ranges_to_merge, target_rangelist,
>> requested_merge, FALSE, pool));
>> }
>> return SVN_NO_ERROR;
>> @@ -1345,14 +1571,23 @@ calculate_remaining_ranges(apr_array_hea
>> apr_pool_t *pool)
>> {
>> apr_array_header_t *requested_rangelist;
>> + apr_array_header_t *reflected_ranges_list = NULL;
>> + apr_array_header_t *reflective_rangelist = NULL;
>> + apr_array_header_t *ranges_to_merge;
>> + svn_merge_range_t *range = NULL, *reflective_range = NULL;
>> + apr_array_header_t *reflected_ranges;
>> const char *old_url;
>> const char *mergeinfo_path;
>> + int i = 0, j = 0;
>> const char *primary_url = (revision1 < revision2) ? url2 : url1;
>>
>> /* Determine which of the requested ranges to consider merging... */
>> SVN_ERR(svn_ra_get_session_url(ra_session, &old_url, pool));
>> SVN_ERR(svn_ra_reparent(ra_session, source_root_url, pool));
>> - SVN_ERR(filter_reflected_revisions(&requested_rangelist, source_root_url,
>> + SVN_ERR(filter_reflected_revisions(&requested_rangelist,
>> + &reflected_ranges_list,
>> + &reflective_rangelist,
>> + source_root_url,
>> url1, revision1, url2, revision2,
>> inheritable, entry->url,
>> ra_session, ctx, pool));
>> @@ -1363,10 +1598,64 @@ calculate_remaining_ranges(apr_array_hea
>> SVN_ERR(svn_client__path_relative_to_root(&mergeinfo_path, primary_url,
>> source_root_url, TRUE,
>> ra_session, NULL, pool));
>> - SVN_ERR(filter_merged_revisions(remaining_ranges, mergeinfo_path,
>> + SVN_ERR(filter_merged_revisions(&ranges_to_merge, mergeinfo_path,
>> target_mergeinfo, implicit_mergeinfo,
>> requested_rangelist,
>> (revision1 > revision2), entry, pool));
>> +
>> + *remaining_ranges =
>> + apr_array_make(pool, 0,
>> + sizeof(svn_client__remaining_range_info_t *));
>> + /* populate remaining_ranges list. */
>> + while (TRUE)
>> + {
>>
>
> I *think* youc an safely move the declarations of range and
> reflective_range and reflected_reanges to here, which would be more
> clear. (Having the top-level declarations include
> requested_rangelist, reflected_ranges_list, reflective_rangelist,
> ranges_to_mege, range, reflective_range, and reflected_ranges is a
> little overwhelming!)
>
>
requested_rangelist, reflected_ranges_list, reflective_rangelist,
ranges_to_merge need to be having the scope as they have now,
I will look at other variables closely and fix them in subsequent commit if needed.
>> + APR_ARRAY_PUSH(*remaining_ranges,
>> + svn_client__remaining_range_info_t *) = range_info;
>> + range = NULL;
>> + reflective_range = NULL;
>>
>
> (And remove these NULLings.)
>
They are needed!, or else we would have wrong results in subsequent
WHILE iteration.
>> + default_start = range_info->range->start;
>> + /* ### We handle reflective_merges only if target has such
>> + * reflective merges. We should do for childs in
>> + * children_with_mergeinfo too, but that would be complex
>> + * enough to handle.
>> + */
>>
>
> I don't really follow what this comment means.
>
Suppose we have a working copy of url $URL/trunk.
If some directory 'src' under '/trunk' has a reflective commit, we are
not handling that as of today in issue-2897 branch, implementing that
would be too complex I believe. In other words we honour reflections
only at url from the given working copy merge target.
>> + SVN_ERR(svn_client__get_diff_editor(target_wcpath, adm_access,
>> + callbacks, merge_b, depth,
>> + merge_b->dry_run, merge_b->ra_session2,
>> + default_start, notification_receiver,
>> + notify_b, merge_b->ctx->cancel_func,
>> + merge_b->ctx->cancel_baton, &diff_editor,
>> + &diff_edit_baton, pool));
>>
>
> Not sure why you reflowed this; it's hard to see what the change was.
>
Sorry about that.
>> Index: branches/issue-2897/subversion/libsvn_client/mergeinfo.h
>> ===================================================================
>> --- branches/issue-2897/subversion/libsvn_client/mergeinfo.h (revision 28560)
>> +++ branches/issue-2897/subversion/libsvn_client/mergeinfo.h (revision 28561)
>> @@ -23,6 +23,14 @@
>> /*** Data Structures ***/
>>
>>
>> +typedef struct svn_client__remaining_range_info_t {
>> + /* Subset of requested merge range. */
>> + svn_merge_range_t *range;
>> + /* If reflected_ranges is not NULL then above 'range' is a
>> + reflective range of it. */
>> + apr_array_header_t *reflected_ranges;
>>
>
> Document the type of the elements. And I can't really parse the
> comment there. Do you mean that these are the ranges reflected in
> 'range', or something?
>
>
Yes. Fixed the doc with array element types at r28772.
Thanks again for your kind review.
With regards
Kamesh Jayachandran
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r28561 - in branches/issue-2897/subversion: libsvn_client
tests/cmdline
Posted by Kamesh Jayachandran <ka...@collab.net>.
Hi David,
Responding to items that I promised to complete in my earlier email.
>> Also, um. I might be paranoid, but I'm not really comfortable with
>> you overwriting "older" like this. I'd rather this function just make
>> a series of temporary files and return the last filename to
>> reflective_merge_file_changed. There's nothing guaranteeing that
>> "older" might not be something that some other part of the program
>> cares about. Just figuring out what context this function can be
>> called in is non-trivial; specifically, "older" gets its value from
>> the diff callback file_changed call, and I notice that in
>> libsvn_wc/diff.c(file_diff) it can get called with "older" equal to
>> the text base path! Maybe this code path can't ever reach the merge
>> code, but I'm still not comfortable with this...
>>
> Will fix it in subsequent commit.
Fixed in r28847.
>>> +reflective_merge_file_deleted(svn_wc_adm_access_t *adm_access,
>>> + svn_wc_notify_state_t *state,
>>> + const char *mine,
>>> + const char *older,
>>> + const char *yours,
>>> + const char *mimetype1,
>>> + const char *mimetype2,
>>> + apr_hash_t *original_props,
>>> + 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_file)
>>> + SVN_ERR(merge_file_deleted(adm_access, state, mine, older, yours,
>>> + mimetype1, mimetype2,
>>> original_props, baton));
>>>
>>
>> Similarly here. Maybe just a comment somewhere explaining the point
>> of the reflective callbacks might help...
>>
>>
>
> It is also faulty will correct in subsequent commit.
Fixed in r28797.
>>>
>>> +/* A svn_wc_diff_callbacks2_t function. */
>>> +static svn_error_t *
>>> +reflective_merge_dir_deleted(svn_wc_adm_access_t *adm_access,
>>> + svn_wc_notify_state_t *state,
>>> + const char *path,
>>> + 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_dir)
>>> + SVN_ERR(merge_dir_deleted(adm_access, state, path, baton));
>>>
>>
>> ... and still :)
>>
>
> It is also faulty, will correct in subsequent commit.
Fixed in r28797.
>> I *think* youc an safely move the declarations of range and
>> reflective_range and reflected_reanges to here, which would be more
>> clear. (Having the top-level declarations include
>> requested_rangelist, reflected_ranges_list, reflective_rangelist,
>> ranges_to_mege, range, reflective_range, and reflected_ranges is a
>> little overwhelming!)
>>
>>
>
>
> requested_rangelist, reflected_ranges_list, reflective_rangelist,
> ranges_to_merge need to be having the scope as they have now, I will
> look at other variables closely and fix them in subsequent commit if
> needed.
>
>
>
>>> + APR_ARRAY_PUSH(*remaining_ranges,
>>> + svn_client__remaining_range_info_t *) = range_info;
>>> + range = NULL;
>>> + reflective_range = NULL;
>>>
>>
>> (And remove these NULLings.)
>>
>
> They are needed!, or else we would have wrong results in subsequent
> WHILE iteration.
Reduced the scope of those variables in r28854.
With regards
Kamesh Jayachandran
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org