David Glasser wrote:
> The big comment on this revision: I'm not positive if this reflective
> merge callback implementation is the right idea or not, but it
> absolutely needs a nice big comment block somewhere explaining what
> it's for, when it can be called, why it works the way it does, etc.
>
> More comments follow.
>
>
Added a doc near reflective_merge_callbacks struct at r28691. Let me
know if it needs correction.
>> + /* reflected_ranges is set for each merge range if the merge is reflective.*/
>> + apr_array_header_t *reflected_ranges;
>>
>
> You should document what type the elements of this array are (and,
> more fundamentally, what they represent --- your comment only says
> when it exists).
>
> In fact, the whole reflective/reflected terminology is somewhat
> complicated; I'd recommend a big comment block somewhere, maybe the
> top of the file, explaining it.
>
>
Done in r28691.
>> apr_pool_t *pool;
>> } merge_cmd_baton_t;
>>
>> @@ -366,6 +369,89 @@ conflict_resolver(svn_wc_conflict_result
>> return err;
>> }
>>
>> +/* Retrieves the file at PATH relative to RA_SESSION at revision REVISION
>> + and stores it in *FILENAME. All allocation occurs in POOL. */
>> +static svn_error_t *
>> +get_file_from_ra(const char **filename, const char *path,
>> + svn_revnum_t revision, svn_ra_session_t *ra_session,
>> + apr_pool_t *pool)
>> +{
>> + apr_file_t *file;
>> + svn_stream_t *fstream;
>> + const char *temp_dir;
>> +
>> + SVN_ERR(svn_io_temp_dir(&temp_dir, pool));
>> + SVN_ERR(svn_io_open_unique_file2(&file, filename,
>> + svn_path_join(temp_dir, "tmp", pool),
>> + "", svn_io_file_del_on_pool_cleanup, pool));
>> +
>> + fstream = svn_stream_from_aprfile(file, pool);
>> + SVN_ERR(svn_ra_get_file(ra_session, path, revision, fstream,
>> + NULL, NULL, pool));
>> + SVN_ERR(svn_io_file_close(file, pool));
>> +
>> + return SVN_NO_ERROR;
>> +}
>>
>
> Isn't this basically the same as a function in repos_diff.c?
>
Yes, except for the signature and some dependency on 'private batons'.
>
>> +/* merges MERGE_B->reflected_ranges from MERGE_B->target url to OLDER. */
>> +static svn_error_t *
>> +merge_reflected_ranges_b4_reflecting(const char *older,
>>
>
> Spell out words: before, not b4. I also don't get why this function
> does the right thing. Like, if these revisions are reflected, then
> wouldn't they already be in "older"? Or do we somehow know that not
> only are they reflected, but they're newer than "older"?
>
Done in r28691.
If you merge r100 from /feature_branch/test.c and 100 is a reflective
revision of r20, r25, r45, r86 from /trunk/test.c
older is /feature_branch/test.c@99
yours(a.k.a newer) is /feature_branch/test.c@100
mine is your working copy.
diff(older, yours) = has changes from r20, r25, r45, r86 + some local
changes call it a RAW_CHANGE.
As this RAW_CHANGE has r20, r25, r45, r86 a.k.a REPEAT_CHANGE can cause
a conflict.
We should eradicate the repeat merge as much as possible.
This reflective_merge_file_changed upgrades OLDER as much as it can by
applying r20, r25, r45, r86, thus reducing the RAW_CHANGE.
Assuming merge of r20, r25, r45, r86 did not give a conflict, we could
get only 'some local changes' from r100.
>
>> + const char *file_path_relative_to_target,
>> + merge_cmd_baton_t *merge_b)
>> +{
>> + int i;
>> + svn_diff_file_options_t *options;
>> + const char *target_marker = "<<<<<<< .working";
>> + const char *left_marker = "||||||| .old";
>> + const char *right_marker = ">>>>>>> .new";
>> + const char *temp_dir;
>> + 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.
>
Will fix it.
> + {
>
>> + svn_diff_t *diff;
>> + const char *left, *right, *result_target;
>> + svn_merge_range_t *range;
>> + svn_stream_t *ostream;
>> + apr_file_t *result_f;
>> + SVN_ERR(svn_io_open_unique_file2(&result_f, &result_target,
>> + svn_path_join(temp_dir, "tmp", subpool),
>> + "", svn_io_file_del_on_pool_cleanup,
>> + subpool));
>> + ostream = svn_stream_from_aprfile(result_f, subpool);
>> + range = APR_ARRAY_IDX(merge_b->reflected_ranges, i, svn_merge_range_t *);
>> + SVN_ERR(get_file_from_ra(&left, file_path_relative_to_target,
>> + range->start, merge_b->target_ra_session,
>> + subpool));
>> + SVN_ERR(get_file_from_ra(&right, file_path_relative_to_target,
>> + range->end, merge_b->target_ra_session,
>> + subpool));
>> + SVN_ERR(svn_diff_file_diff3_2(&diff, left, older, right,
>> + options, subpool));
>> + SVN_ERR(svn_diff_file_output_merge(ostream, diff,
>> + left, older, right,
>> + left_marker,
>> + target_marker,
>> + right_marker,
>> + "=======", /* seperator */
>> + FALSE, /* display original */
>> + FALSE, /* resolve conflicts */
>> + subpool));
>> + SVN_ERR(svn_stream_close(ostream));
>> + SVN_ERR(svn_io_file_flush_to_disk(result_f, subpool));
>> +
>> + SVN_ERR(svn_io_copy_file(result_target, older, TRUE, subpool));
>>
>
> So, how much have you tested this? It seems to me that it's really
> likely that you'll end up getting conflicts on top of conflicts on top
> of conflicts, in a file that the user has never even touched. Is this
> just me being paranoid, or is this a real possibility?
>
Yes it is possible, I had one todo item and fixed it via a testcase and
a code change at r28649 and r28650 respectively.
Will respond to your other review comments later.
Thanks for your review.
With regards
Kamesh Jayachandran
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Fri Dec 28 17:41:40 2007