On Tue, Jun 25, 2013 at 10:34 AM, Stefan Sperling <s...@elego.de> wrote:
> On Thu, Jun 20, 2013 at 12:56:05PM -0000, stef...@apache.org wrote: > > Author: stefan2 > > Date: Thu Jun 20 12:56:05 2013 > > New Revision: 1494966 > > > > URL: http://svn.apache.org/r1494966 > > Log: > > Optimize 'svn log' and related operations: Teach the client to skip > > the RA call when querying locations segments for repository roots. > > We already know the answer. > > > > * subversion/libsvn_client/ra.c > > (svn_client__repos_location_segments): special case repo roots > > > > 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=1494966&r1=1494965&r2=1494966&view=diff > > > ============================================================================== > > --- subversion/trunk/subversion/libsvn_client/ra.c (original) > > +++ subversion/trunk/subversion/libsvn_client/ra.c Thu Jun 20 12:56:05 > 2013 > > @@ -601,8 +601,32 @@ svn_client__repos_location_segments(apr_ > > struct gls_receiver_baton_t gls_receiver_baton; > > const char *old_session_url; > > svn_error_t *err; > > + const char *rel_path; > > > > *segments = apr_array_make(pool, 8, sizeof(svn_location_segment_t *)); > > + > > + /* Save us an RA layer round trip if we are on the repository root and > > + know the result in advance. It's fair to assume that the repo root > > + has already been cached in ra_session. > > + > > + We also assume that all parameters are valid and reivisons properly > > + ordered. Otherwise, the error behavior might differ. > > + */ > > + 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; > > The above logic looks strange. > > If I understand correctly, we return the following location segment > for the root node, if end_revision < start_revision: > > [end revision, start revision] > > and otherwise we return the following location segment for the > root node: > > [0, 0] > > I would have expected the following instead: > > [0, start_revision] (if start_revision is the younger rev) > > or: > > [0, end_revision] (if end_revision is the younger rev) > > Because the root node exists in all revisions starting at zero and > ending at max(start_revision, end_revision). > > > Also, svn_ra_get_location_segments(), which this function is a wrapper > of, is supposed to treat start and end revisions of SVN_INVALID_REVNUM > in a special way: > > * @a end_rev may be @c SVN_INVALID_REVNUM to indicate that you want > * to trace the history of the object to its origin. > * > * @a start_rev may be @c SVN_INVALID_REVNUM to indicate "the HEAD > * revision". Otherwise, @a start_rev must be younger than @a end_rev > * (unless @a end_rev is @c SVN_INVALID_REVNUM). > > Your client-side optimization doesn't seem to account for these cases. > Why not? > > > + segment->path = rel_path; > > + APR_ARRAY_PUSH(*segments, svn_location_segment_t *) = segment; > > + > > + return SVN_NO_ERROR; > > + } > > + > > + /* Do it the hard way and ask the repository layer. */ > > gls_receiver_baton.segments = *segments; > > gls_receiver_baton.ctx = ctx; > > gls_receiver_baton.pool = pool; > > > Due to the heatwave, I obviously wrote that code using half a braincell at best. r1496886 should be much better. Thanks for the review! -- Stefan^2.
