You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Stefan Sperling <st...@elego.de> on 2013/06/26 14:46:29 UTC
Re: svn commit: r1496886 -
/subversion/trunk/subversion/libsvn_client/ra.c
On Wed, Jun 26, 2013 at 11:48:49AM -0000, stefan2@apache.org wrote:
> Author: stefan2
> Date: Wed Jun 26 11:48:49 2013
> New Revision: 1496886
>
> URL: http://svn.apache.org/r1496886
> Log:
> Follow-up to r1494966: When revisions are not given, set them to 0 / HEAD,
> respectively. Also, return an empty segment list for empty revision ranges.
>
> * subversion/libsvn_client/ra.c
> (svn_client__repos_location_segments): revise revision edge case handling
>
> Modified:
> subversion/trunk/subversion/libsvn_client/ra.c
>
> Modified: subversion/trunk/subversion/libsvn_client/ra.c
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_client/ra.c?rev=1496886&r1=1496885&r2=1496886&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/libsvn_client/ra.c (original)
> +++ subversion/trunk/subversion/libsvn_client/ra.c Wed Jun 26 11:48:49 2013
> @@ -615,13 +615,23 @@ svn_client__repos_location_segments(apr_
> SVN_ERR(svn_ra_get_path_relative_to_root(ra_session, &rel_path, url, pool));
> if (rel_path && rel_path[0] == 0)
> {
> - svn_location_segment_t *segment = apr_pcalloc(pool, sizeof(*segment));
> - segment->range_start
> - = end_revision <= start_revision ? end_revision : 0;
> - segment->range_end
> - = end_revision <= start_revision ? start_revision : 0;
> - segment->path = rel_path;
> - APR_ARRAY_PUSH(*segments, svn_location_segment_t *) = segment;
> + svn_location_segment_t *segment;
> +
Much better. But I suspect that you need to enhance this even futher
to retain proper behaviour in error cases.
For instance, you're ignoring peg_revision here. It could be either
SVN_INVALID_REVNUM, in which case it is considered equal to
start_revision, or it could be a valid revision number and then
it must be >= start_revision.
I think this block of code should either enforce and fulfill the
API contract in the exact same way as svn_ra_get_location_segments()
does, or it should not exist at all. I don't think we should bother
with optimizations that don't behave exactly like the non-optimized case.
> + /* complete revision parameters if not given */
> + if (start_revision == SVN_INVALID_REVNUM)
> + SVN_ERR(svn_ra_get_latest_revnum(ra_session, &start_revision, pool));
> + if (end_revision == SVN_INVALID_REVNUM)
> + end_revision = 0;
> +
> + /* root exists for any non-empty revision range */
> + if (end_revision <= start_revision)
> + {
> + segment = apr_pcalloc(pool, sizeof(*segment));
> + segment->range_start = end_revision;
> + segment->range_end = start_revision;
> + segment->path = rel_path;
> + APR_ARRAY_PUSH(*segments, svn_location_segment_t *) = segment;
> + }
> return SVN_NO_ERROR;
> }
>
Re: svn commit: r1496886 - /subversion/trunk/subversion/libsvn_client/ra.c
Posted by Stefan Fuhrmann <st...@wandisco.com>.
On Wed, Jun 26, 2013 at 2:46 PM, Stefan Sperling <st...@elego.de> wrote:
> On Wed, Jun 26, 2013 at 11:48:49AM -0000, stefan2@apache.org wrote:
> > Author: stefan2
> > Date: Wed Jun 26 11:48:49 2013
> > New Revision: 1496886
> >
> > URL: http://svn.apache.org/r1496886
> > Log:
> > Follow-up to r1494966: When revisions are not given, set them to 0 /
> HEAD,
> > respectively. Also, return an empty segment list for empty revision
> ranges.
> >
> > * subversion/libsvn_client/ra.c
> > (svn_client__repos_location_segments): revise revision edge case
> handling
> >
> > Modified:
> > subversion/trunk/subversion/libsvn_client/ra.c
> >
> > Modified: subversion/trunk/subversion/libsvn_client/ra.c
> > URL:
> http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_client/ra.c?rev=1496886&r1=1496885&r2=1496886&view=diff
> >
> ==============================================================================
> > --- subversion/trunk/subversion/libsvn_client/ra.c (original)
> > +++ subversion/trunk/subversion/libsvn_client/ra.c Wed Jun 26 11:48:49
> 2013
> > @@ -615,13 +615,23 @@ svn_client__repos_location_segments(apr_
> > SVN_ERR(svn_ra_get_path_relative_to_root(ra_session, &rel_path, url,
> pool));
> > if (rel_path && rel_path[0] == 0)
> > {
> > - svn_location_segment_t *segment = apr_pcalloc(pool,
> sizeof(*segment));
> > - segment->range_start
> > - = end_revision <= start_revision ? end_revision : 0;
> > - segment->range_end
> > - = end_revision <= start_revision ? start_revision : 0;
> > - segment->path = rel_path;
> > - APR_ARRAY_PUSH(*segments, svn_location_segment_t *) = segment;
> > + svn_location_segment_t *segment;
> > +
>
> Much better. But I suspect that you need to enhance this even futher
> to retain proper behaviour in error cases.
>
> For instance, you're ignoring peg_revision here. It could be either
> SVN_INVALID_REVNUM, in which case it is considered equal to
> start_revision, or it could be a valid revision number and then
> it must be >= start_revision.
>
> I think this block of code should either enforce and fulfill the
> API contract in the exact same way as svn_ra_get_location_segments()
> does, or it should not exist at all. I don't think we should bother
> with optimizations that don't behave exactly like the non-optimized case.
>
> > + /* complete revision parameters if not given */
> > + if (start_revision == SVN_INVALID_REVNUM)
> > + SVN_ERR(svn_ra_get_latest_revnum(ra_session, &start_revision,
> pool));
> > + if (end_revision == SVN_INVALID_REVNUM)
> > + end_revision = 0;
> > +
> > + /* root exists for any non-empty revision range */
> > + if (end_revision <= start_revision)
> > + {
> > + segment = apr_pcalloc(pool, sizeof(*segment));
> > + segment->range_start = end_revision;
> > + segment->range_end = start_revision;
> > + segment->path = rel_path;
> > + APR_ARRAY_PUSH(*segments, svn_location_segment_t *) = segment;
> > + }
> > return SVN_NO_ERROR;
> > }
> >
>
As it turns out, we cannot exactly replicate the error behavior
with client-side only information in that function.
Reverted.
-- Stefan^2.
Re: svn commit: r1496886 -
/subversion/trunk/subversion/libsvn_client/ra.c
Posted by Stefan Sperling <st...@elego.de>.
On Wed, Jun 26, 2013 at 05:50:58PM +0200, Stefan Fuhrmann wrote:
> On Wed, Jun 26, 2013 at 2:52 PM, Stefan Sperling <st...@elego.de> wrote:
>
> > On Wed, Jun 26, 2013 at 02:46:29PM +0200, Stefan Sperling wrote:
> > > I think this block of code should either enforce and fulfill the
> > > API contract in the exact same way as svn_ra_get_location_segments()
> > > does, or it should not exist at all. I don't think we should bother
> > > with optimizations that don't behave exactly like the non-optimized case.
> >
> > Another idea: what about moving this special case handling into
> > libsvn_ra itself, rather than having it in libsvn_client?
> >
>
> The server-side implementation is pretty efficient already.
libsvn_ra is client-side, so I'm not sure why you mention
the server. But anyway, it seems you've determined the RA
API is too general to allow for this optimisation.
> This has been about the extra network roundtrip (Interwebs!).
>
> r1496957 accomplishes that optimization now.
I'll take a look. Thanks!
Re: svn commit: r1496886 - /subversion/trunk/subversion/libsvn_client/ra.c
Posted by Stefan Fuhrmann <st...@wandisco.com>.
On Wed, Jun 26, 2013 at 2:52 PM, Stefan Sperling <st...@elego.de> wrote:
> On Wed, Jun 26, 2013 at 02:46:29PM +0200, Stefan Sperling wrote:
> > I think this block of code should either enforce and fulfill the
> > API contract in the exact same way as svn_ra_get_location_segments()
> > does, or it should not exist at all. I don't think we should bother
> > with optimizations that don't behave exactly like the non-optimized case.
>
> Another idea: what about moving this special case handling into
> libsvn_ra itself, rather than having it in libsvn_client?
>
The server-side implementation is pretty efficient already.
This has been about the extra network roundtrip (Interwebs!).
r1496957 accomplishes that optimization now.
-- Stefan^2.
Re: svn commit: r1496886 -
/subversion/trunk/subversion/libsvn_client/ra.c
Posted by Stefan Sperling <st...@elego.de>.
On Wed, Jun 26, 2013 at 02:46:29PM +0200, Stefan Sperling wrote:
> I think this block of code should either enforce and fulfill the
> API contract in the exact same way as svn_ra_get_location_segments()
> does, or it should not exist at all. I don't think we should bother
> with optimizations that don't behave exactly like the non-optimized case.
Another idea: what about moving this special case handling into
libsvn_ra itself, rather than having it in libsvn_client?