On Thu, Apr 10, 2008 at 11:17 AM, Paul Burba <ptburba_at_gmail.com> wrote:
>
> 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
Kamesh,
I'll take care of this (I forget sometimes how late it is for you!)
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:34:22 CEST