On Thu, Apr 10, 2008 at 11:14 AM, Kamesh Jayachandran <kamesh_at_collab.net> wrote:
> -----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.
Doh, I spoke too soon! Of course there is a practical effect! There
won't be a merge notification that the property on PATH is
Udpated/merGed. For that reason alone you should make this change,
though I would suggest this comment:
adjusted_prop->value = filtered_mergeinfo_str;
APR_ARRAY_PUSH(adjusted_props, svn_prop_t) = *adjusted_prop;
}
+ else
+ {
+ * If the incoming svn:mergeinfo property addition was simply
+ for empty mergeinfo then the filtering logic wouldn't apply
+ (there are *no* revision ranges to filter after all). In
+ that case just put empty mergeinfo prop in the array.
+ */
+ APR_ARRAY_PUSH(adjusted_props, svn_prop_t) = *prop;
+ }
Paul
---------------------------------------------------------------------
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:17:22 CEST