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_at_subversion.tigris.org
For additional commands, e-mail: dev-help_at_subversion.tigris.org
Received on 2008-01-11 07:11:58 CET