-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Paul Burba wrote:
> On Thu, Apr 10, 2008 at 10:19 AM, Kamesh Jayachandran <kamesh_at_collab.net> wrote:
>> -----BEGIN PGP SIGNED MESSAGE-----
>> Hash: SHA1
>>
>>
>> > + SVN_ERR(svn_mergeinfo_to_string(&filtered_mergeinfo_str,
>> > + filtered_mergeinfo,
>> > pool));
>> > adjusted_prop->name = SVN_PROP_MERGEINFO;
>> > - adjusted_prop->value =
>> > - svn_string_create(apr_pstrcat(pool, source_path, ":",
>> > - adjusted_rangelist_s->data,
>> > - NULL),
>> > - pool);
>> > + adjusted_prop->value = filtered_mergeinfo_str;
>> > APR_ARRAY_PUSH(adjusted_props, svn_prop_t) = *adjusted_prop;
>> > }
>> > - } /* mergeinfo hash iteration */
>>
>>
>> What about 'un-filtered' mergeinfo? Then we are not adding anything to
>> adjusted_props so may loose un-filtered mergeinfo.
>
> Kamesh,
>
> If we don't filter anything then filtered_mergeinfo is equivalent to
> the incoming svn:mergeinfo property addition. Notice that in the
> 'else /* Non-empty mergeinfo; filter self-referential mergeinfo out.
> */' block we *always* push unfiltered revisions into the
> adjusted_rangelist array.
>
Thanks for the explanation Paul.
Following snip answers my concern.
<snip>
/* PATH_at_TARGET_ENTRY->REVISION exists on the same
line of history at RANGE->START. But it might
have existed under a different name then, so
check if the URL it had then is the same as the
URL for the mergeinfo we are trying to add. If
it is the same we can filter it out. */
if (strcmp(start_url, merge_source_url) != 0)
{
APR_ARRAY_PUSH(adjusted_rangelist,
svn_merge_range_t *) = range;
}
</snip>
> Does your local test case actually show a problem? If so could you post it.
No.
>
>> The code(even before this commit) looks buggy in this aspect.
>> Following patch may fix it.
>
> Despite what I said above I think your patch is actually needed to be
> technically correct in one case:
>
> If the incoming svn:mergeinfo prop add is simply empty mergeinfo, then
> the empty mergeinfo would be filtered out from the property additions!
> This is because that is the only case (that I can see) where the
>
> for (hi = apr_hash_first(NULL, mergeinfo);
> hi; hi = apr_hash_next(hi))
>
> block is skipped entirely and we don't enter the 'if
> (filtered_mergeinfo)' block. The result being the empty mergeinfo is
> lost.
>
> Now I don't think there is any practical effect from this because we
> ultimately set mergeinfo describing the merge on PATH
> merge_props_changed() is operating on and the empty mergeinfo will
> seamlessly merge with that.
>
Nice explanation.
With regards
Kamesh Jayachandran
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.2.6 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org
iD8DBQFH/i7W3WHvyO0YTCwRAkPTAJ4nVwBdVD6R7A8piyaARVPT+C8rfQCeIZyQ
XNrlKNg1C1rdfNeYVWkOnRE=
=KB1P
-----END PGP SIGNATURE-----
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe_at_subversion.tigris.org
For additional commands, e-mail: dev-help_at_subversion.tigris.org
Received on 2008-04-10 17:15:14 CEST