[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

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

From: Philip Martin <philip.martin_at_wandisco.com>
Date: Mon, 13 Jun 2011 11:53:21 +0100

Dirk Thomas <websvn_at_dirk-thomas.net> writes:

A short comment along the lines 'Sets *REVISION_DELETED ...'

> +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
> +)

No whitespace before ')'.

> +{
> + 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. ***/
>
> @@ -472,6 +508,9 @@
>
>
> {
> + 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 +520,54 @@
> 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));
> + 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)

Split long line before '&& session_opt_revnum'

> + {
> + svn_revnum_t revision_deleted = SVN_INVALID_REVNUM;
> + apr_pool_t *sesspool = svn_pool_create(pool);
> +

You don't clear err, so if the following two calls return an error then
err leaks.

> + SVN_ERR(svn_client__ra_session_from_path(&ra_session, &ignored_revnum,
> + &actual_url, ra_target, NULL,
> + &peg_rev, &peg_rev,
> + ctx, sesspool));
> +
> + SVN_ERR(check_for_deleted_rev(ra_session,
> + url_or_path, peg_revnum,
> + session_opt_revnum,
> + &revision_deleted,
> + ctx, sesspool));
> +
> + svn_pool_destroy(sesspool);
> +
> + if (SVN_IS_VALID_REVNUM(revision_deleted))
> + {
> + svn_opt_revision_t session_mod_rev;
> + session_mod_rev.kind = svn_opt_revision_number;
> + session_mod_rev.value.number = revision_deleted - 1;
> +
> + svn_error_clear(err);
> +
> + err = svn_client__ra_session_from_path(&ra_session, &ignored_revnum,
> + &actual_url, ra_target, NULL,
> + &peg_rev, &session_mod_rev,
> + ctx, pool);
> + }

A temporary session is used to identify the deleted revision, and then
another session is created to run log. Since the temporary session
"works" could it be permanent and used for log?

> + }
> + SVN_ERR(err);
> +
> SVN_ERR(svn_ra_has_capability(ra_session, &has_log_revprops,
> SVN_RA_CAPABILITY_LOG_REVPROPS, pool));
>
> @@ -549,7 +631,7 @@
> 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 +657,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 +689,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));
> + }
> +
> + 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
Received on 2011-06-13 12:54:00 CEST

This is an archived mail posted to the Subversion Dev mailing list.

This site is subject to the Apache Privacy Policy and the Apache Public Forum Archive Policy.