On Wed, Jun 26, 2013 at 2:46 PM, Stefan Sperling <stsp_at_elego.de> wrote:
> On Wed, Jun 26, 2013 at 11:48:49AM -0000, stefan2_at_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.
Received on 2013-06-26 17:49:00 CEST