Re: [PATCH] possible improvement to svn log with "forward" revision range (issue 3830)

Dirk Thomas <websvn_at_dirk-thomas.net> writes:

> +/* find the revision at which the node was deleted
> + and sets *REVISION_DELETED to that revision. */
> +static svn_error_t *
> +check_for_deleted_rev(svn_ra_session_t *ra_session,
> + const char *url_or_path,
> + svn_revnum_t peg_revnum,
> + svn_revnum_t end_revnum,
> + svn_revnum_t *revision_deleted,
> + svn_client_ctx_t *ctx,
> + apr_pool_t *pool)
> +{
> + const char *session_url;
> + const char *rel_path;
> +
> + SVN_ERR(svn_ra_get_session_url(ra_session,
> + &session_url,
> + pool));
> +
> + SVN_ERR(svn_client__path_relative_to_root(&rel_path,
> + ctx->wc_ctx,
> + url_or_path,
> + session_url,
> + FALSE,
> + ra_session,
> + pool,
> + pool));
> +
> + SVN_ERR(svn_ra_get_deleted_rev(ra_session,
> + rel_path,
> + peg_revnum,
> + end_revnum,
> + revision_deleted,
> + pool));
> +
> + return SVN_NO_ERROR;
> +}
> +
> 
> /*** Public Interface. ***/
>
> @@ -471,7 +508,11 @@
> }
>
>
> + iterpool = svn_pool_create(pool);
> {
> + svn_revnum_t peg_revnum, session_opt_revnum;
> + svn_error_t *err;
> +
> /* If this is a revision type that requires access to the working copy,
> * we use our initial target path to figure out where to root the RA
> * session, otherwise we use our URL. */
> @@ -481,11 +522,58 @@
> else
> ra_target = url_or_path;
>
> - SVN_ERR(svn_client__ra_session_from_path(&ra_session, &ignored_revnum,
> - &actual_url, ra_target, NULL,
> - &peg_rev, &session_opt_rev,
> - ctx, pool));
> + SVN_ERR(svn_client__get_revision_number(&peg_revnum, NULL,
> + ctx->wc_ctx, ra_target,
> + ra_session, &peg_rev,
> + pool));

You are not opening the session before passing it to
svn_client__get_revision_number, how does that work?

> + SVN_ERR(svn_client__get_revision_number(&session_opt_revnum, NULL,
> + ctx->wc_ctx, ra_target,
> + ra_session, &session_opt_rev,
> + pool));
>
> + err = svn_client__ra_session_from_path(&ra_session, &ignored_revnum,
> + &actual_url, ra_target, NULL,
> + &peg_rev, &session_opt_rev,
> + ctx, pool);
> +
> + if (err && err->apr_err == SVN_ERR_FS_NOT_FOUND
> + && session_opt_revnum > peg_revnum)
> + {
> + svn_revnum_t revision_deleted = SVN_INVALID_REVNUM;
> + svn_opt_revision_t session_mod_rev;
> +
> + svn_error_clear(err);
> +
> + svn_pool_clear(iterpool);
> +
> + SVN_ERR(svn_client__ra_session_from_path(&ra_session, &ignored_revnum,
> + &actual_url, ra_target, NULL,
> + &peg_rev, &peg_rev,
> + ctx, iterpool));
> +
> + SVN_ERR(check_for_deleted_rev(ra_session,
> + url_or_path, peg_revnum,
> + session_opt_revnum,
> + &revision_deleted,
> + ctx, iterpool));
> +
> + if (!SVN_IS_VALID_REVNUM(revision_deleted))
> + {
> + return svn_error_create
> + (SVN_ERR_CLIENT_BAD_REVISION, NULL,
> + _("Could not determine revision where target was deleted"));
> + }

So now you have opened a session that works, but below you open another
session, why? You seem to be throwing away the working session?
Opening sessions to the server is expensive.

I'm not really familiar with this code. Is it possible to simply open
one session at the peg rev? Then adjust the revisions as required?

> +
> + session_mod_rev.kind = svn_opt_revision_number;
> + session_mod_rev.value.number = revision_deleted - 1;
> +
> + err = svn_client__ra_session_from_path(&ra_session, &ignored_revnum,
> + &actual_url, ra_target, NULL,
> + &peg_rev, &session_mod_rev,
> + ctx, pool);
> + }
> + SVN_ERR(err);
> +
> SVN_ERR(svn_ra_has_capability(ra_session, &has_log_revprops,
> SVN_RA_CAPABILITY_LOG_REVPROPS, pool));
>
> @@ -546,10 +634,9 @@
> * epg wonders if the repository could send a unified stream of log
> * entries if the paths and revisions were passed down.
> */
> - iterpool = svn_pool_create(pool);
> for (i = 0; i < revision_ranges->nelts; i++)
> {
> - svn_revnum_t start_revnum, end_revnum, youngest_rev = SVN_INVALID_REVNUM;
> + svn_revnum_t start_revnum, end_revnum, peg_revnum, youngest_rev = SVN_INVALID_REVNUM, revision_deleted = SVN_INVALID_REVNUM;
> const char *path = APR_ARRAY_IDX(targets, 0, const char *);
> const char *local_abspath_or_url;
> svn_opt_revision_range_t *range;
> @@ -575,6 +662,10 @@
> ctx->wc_ctx, local_abspath_or_url,
> ra_session, &range->end,
> iterpool));
> + SVN_ERR(svn_client__get_revision_number(&peg_revnum, &youngest_rev,
> + ctx->wc_ctx, local_abspath_or_url,
> + ra_session, &peg_rev,
> + iterpool));
>
> if (has_log_revprops)
> {
> @@ -603,6 +694,20 @@
> passed_receiver_baton = &lb;
> }
>
> + if (SVN_IS_VALID_REVNUM(peg_revnum) && end_revnum > peg_revnum)
> + {
> + SVN_ERR(check_for_deleted_rev(ra_session,
> + url_or_path, peg_revnum,
> + end_revnum,
> + &revision_deleted,
> + ctx, pool));
> + }

This looks expensive as well. It's a round trip to find out if the path
has been deleted. Would it be more efficient to try the log and only do
this if the log fails?

> +
> + if (SVN_IS_VALID_REVNUM(revision_deleted))
> + {
> + end_revnum = revision_deleted - 1;
> + }
> +
> SVN_ERR(svn_ra_get_log2(ra_session,
> condensed_targets,
> start_revnum,

-- 
Philip