You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Kamesh Jayachandran <ka...@collab.net> on 2008/04/10 14:19:03 UTC
Re: svn commit: r30467 - trunk/subversion/libsvn_client
-----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.
The code(even before this commit) looks buggy in this aspect.
Following patch may fix it.
I am testing the same with custom testcase.
Index: subversion/libsvn_client/merge.c
===================================================================
- --- subversion/libsvn_client/merge.c (revision 30479)
+++ subversion/libsvn_client/merge.c (working copy)
@@ -485,6 +485,8 @@
adjusted_prop->value = filtered_mergeinfo_str;
APR_ARRAY_PUSH(adjusted_props, svn_prop_t) = *adjusted_prop;
}
+ else
+ APR_ARRAY_PUSH(adjusted_props, svn_prop_t) = *prop;
/* If we reparented MERGE_B->RA_SESSION2 above, put it back
to the original URL. */
With regards
Kamesh Jayachandran
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.2.6 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org
iD8DBQFH/iHX3WHvyO0YTCwRAtOhAJ9/s+cbwnv74P4y63okvrMPqjaYbgCfUFA0
00XyzHjV/AzDYvqeU/9eGcs=
=hyj4
-----END PGP SIGNATURE-----
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r30467 - trunk/subversion/libsvn_client
Posted by Kamesh Jayachandran <ka...@collab.net>.
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
> Kamesh,
>
> I committed this change in r30503...
>
> ...Then I realized that not only did r30503 have a bug (if all
> mergeinfo was filtered then it put the original mergeinfo back in the
> prop array!) but that the only reason we needed it regarding empty
> mergeinfo was because of *another* bug:
No it won't if all the mergeinfo is filtered 'filtered_mergeinfo' hash
still would still be non-null and its string form is '' which is what we
wanted.
Though I like r30510 for its early handling of this problem.
With regards
Kamesh Jayachandran
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.2.6 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org
iD8DBQFH/yVg3WHvyO0YTCwRAgGIAJsGFfl0dWY3iP4vqjfcb2GbkKgPagCgrNVg
CeWbDm8IAfvJteV2Fokcby0=
=47vz
-----END PGP SIGNATURE-----
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r30467 - trunk/subversion/libsvn_client
Posted by Paul Burba <pt...@gmail.com>.
On Thu, Apr 10, 2008 at 1:48 PM, Kamesh Jayachandran <ka...@collab.net> wrote:
>
>
>
> >>
> >> Paul
>
> >Kamesh,
>
> >I'll take care of this (I forget sometimes how late it is for you!)
>
> >Paul
>
>
> Thanks Paul!
Kamesh,
I committed this change in r30503...
...Then I realized that not only did r30503 have a bug (if all
mergeinfo was filtered then it put the original mergeinfo back in the
prop array!) but that the only reason we needed it regarding empty
mergeinfo was because of *another* bug:
/* If this property isn't mergeinfo or is empty mergeinfo it
does not require any special handling. */
if ((strcmp(prop->name, SVN_PROP_MERGEINFO) != 0)
|| (! prop->value))
{
APR_ARRAY_PUSH(adjusted_props, svn_prop_t) = *prop;
}
If PROP represents empty mergeinfo there is still a value (the empty
string) this should be:
if ((strcmp(prop->name, SVN_PROP_MERGEINFO) != 0)
|| (! prop->value->len))
^^^
Fixed that in r30510. I'll remove r30503 from 1.5.x STATUS and put r30510 in.
Apologies to you and cmpilato for time wasted on this.
Paul
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
RE: svn commit: r30467 - trunk/subversion/libsvn_client
Posted by Kamesh Jayachandran <ka...@collab.net>.
>>
>> Paul
>Kamesh,
>I'll take care of this (I forget sometimes how late it is for you!)
>Paul
Thanks Paul!
With regards
Kamesh Jayachandran
Re: svn commit: r30467 - trunk/subversion/libsvn_client
Posted by Paul Burba <pt...@gmail.com>.
On Thu, Apr 10, 2008 at 11:17 AM, Paul Burba <pt...@gmail.com> wrote:
>
> On Thu, Apr 10, 2008 at 11:14 AM, Kamesh Jayachandran <ka...@collab.net> wrote:
> > -----BEGIN PGP SIGNED MESSAGE-----
> > Hash: SHA1
> >
> >
> >
> >
> > Paul Burba wrote:
> > > On Thu, Apr 10, 2008 at 10:19 AM, Kamesh Jayachandran <ka...@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@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@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r30467 - trunk/subversion/libsvn_client
Posted by Paul Burba <pt...@gmail.com>.
On Thu, Apr 10, 2008 at 11:14 AM, Kamesh Jayachandran <ka...@collab.net> wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
>
>
>
> Paul Burba wrote:
> > On Thu, Apr 10, 2008 at 10:19 AM, Kamesh Jayachandran <ka...@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@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@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r30467 - trunk/subversion/libsvn_client
Posted by Kamesh Jayachandran <ka...@collab.net>.
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Paul Burba wrote:
> On Thu, Apr 10, 2008 at 10:19 AM, Kamesh Jayachandran <ka...@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@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@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r30467 - trunk/subversion/libsvn_client
Posted by Paul Burba <pt...@gmail.com>.
On Thu, Apr 10, 2008 at 10:19 AM, Kamesh Jayachandran <ka...@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.
Does your local test case actually show a problem? If so could you post it.
> 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.
Paul
> I am testing the same with custom testcase.
>
> Index: subversion/libsvn_client/merge.c
> ===================================================================
> - --- subversion/libsvn_client/merge.c (revision 30479)
> +++ subversion/libsvn_client/merge.c (working copy)
> @@ -485,6 +485,8 @@
>
> adjusted_prop->value = filtered_mergeinfo_str;
> APR_ARRAY_PUSH(adjusted_props, svn_prop_t) = *adjusted_prop;
> }
> + else
> + APR_ARRAY_PUSH(adjusted_props, svn_prop_t) = *prop;
>
>
> /* If we reparented MERGE_B->RA_SESSION2 above, put it back
> to the original URL. */
>
>
>
> With regards
> Kamesh Jayachandran
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.2.6 (GNU/Linux)
> Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org
>
> iD8DBQFH/iHX3WHvyO0YTCwRAtOhAJ9/s+cbwnv74P4y63okvrMPqjaYbgCfUFA0
> 00XyzHjV/AzDYvqeU/9eGcs=
> =hyj4
> -----END PGP SIGNATURE-----
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
> For additional commands, e-mail: dev-help@subversion.tigris.org
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org