RE: Re: svn commit: r28624 - branches/issue-2897/subversion/libsvn_fs_util

Hi,

>> if (inherit == svn_mergeinfo_inherited
>> || inherit == svn_mergeinfo_nearest_ancestor)
>> - SVN_ERR(get_parent_target_path_having_mergeinfo(&real_mergeinfo_target,
>> - db, merge_target,
>> - min_commit_rev,
>> - max_commit_rev, pool));
>> + get_elided_parent_mergeinfo = TRUE;

>Should this variable be called get_inherited_mergeinfo instead?

Fixed in r28638.

>(Also, I don't think you'll be handling the nearest_ancestor case
>correctly.)

Yes, Will correct it as soon as I complete the 'peg-awareness work'.

>> + "mergedrevend, inheritable, mergedfrom, "
>> + "mergedto FROM mergeinfo_changed "
>> + "WHERE revision between ? AND ? "
>> + "ORDER BY revision ASC, mergedto ASC; ",
>> + pool));

>This will iterate over *every* mergeinfo change *everywhere* in the
>repository between the two revisions. I can't imagine that this is
>efficient for large multi-project multi-branch repositories.

Yes I am aware of it, there should be better way to achieve the same, but I don't have one now.

>> + apr_hash_set(rev_target_hash, commit_rev_ptr,
>> + sizeof(*commit_rev_ptr),
>> + apr_pstrdup(pool, mergedto));
>> + /* merge target changed, so discard the earlier merge ranges.
>> + TODO we should clear of the array. */

>I'd also comment that the *only* reason this is safe is that you have
>an ORDER BY mergedto in effect.

Yes.

>> + merge_rangelist = apr_array_make(pool, 0,
>> + sizeof(svn_merge_range_t *));
>> + }
>> + }
>> + else
>> + apr_hash_set(rev_target_hash, commit_rev_ptr, sizeof(*commit_rev_ptr),
>> + apr_pstrdup(pool, mergedto));
>> if ((last_commit_rev != commit_rev)
>> && (last_commit_rev != SVN_INVALID_REVNUM))
>> {

>Of course, now that you're effectively doing a "loop over every change
>in every path and every revision, doing filtering in C in svn instead
>of in Sqlite in some index-improvable way", it's quite possible that a
>pure-FS solution will be able to implement this API as well...

Yes.

Thanks for your review.
With regards
Kamesh Jayachandran
Received on Sat Dec 22 19:41:50 2007