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;
> }
>
