On Wed, Jun 26, 2013 at 2:46 PM, Stefan Sperling <[email protected]> wrote:
> On Wed, Jun 26, 2013 at 11:48:49AM -0000, [email protected] 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.
