2008/10/5 Lieven Govaerts <svnlgo_at_mobsol.be>:
> I'd like to get this patch reviewed by someone familiar with the merge
> code before I commit.
Looks good to me. Few nits below.
> Index: subversion/libsvn_client/merge.c
> ===================================================================
> --- subversion/libsvn_client/merge.c (revision 33458)
> +++ subversion/libsvn_client/merge.c (working copy)
> @@ -2969,7 +2969,8 @@ populate_remaining_ranges(apr_array_header_t *chil
> SVN_ERR(get_full_mergeinfo(&(child->pre_merge_mergeinfo),
> &(child->implicit_mergeinfo), child_entry,
> &(child->indirect_mergeinfo),
> - svn_mergeinfo_inherited, NULL, child->path,
> + svn_mergeinfo_inherited, ra_session,
> + child->path,
> MAX(revision1, revision2),
> MIN(revision1, revision2),
> adm_access, merge_b->ctx, pool));
One other optimization point is that if ra_session *were* NULL at this
point (not quite sure it *could* be, but if it were...), then the new
session value wouldn't be reused across invocations of this call. So,
perhaps get_full_mergeinfo should be taking a double pointer to
ra_session here instead. Not sure - but with it down to 3 sessions at
this point, it's probably far far better than it was. =)
> @@ -6085,7 +6086,38 @@ do_directory_merge(const char *url1,
> return err;
> }
>
> +/** Ensure the caller receives a RA_SESSION object to URL. This function will
> + * reuse RA_SESSION if it is not NULL and is opened to the same repository as
> + * URL is pointing to. Otherwise a new session object will be created.
> + */
> +static svn_error_t *
> +ensure_ra_session_url(svn_ra_session_t **ra_session,
> + const char *url,
> + svn_client_ctx_t *ctx,
> + apr_pool_t *pool)
> +{
> + svn_error_t *err = SVN_NO_ERROR;
>
> + if (*ra_session)
> + {
> + const char *old_session_url;
> + err = svn_client__ensure_ra_session_url(&old_session_url,
> + *ra_session,
> + url,
> + pool);
> + }
> +
> + if ((err && err->apr_err == SVN_ERR_RA_ILLEGAL_URL) || ! *ra_session)
For readability purposes, I'd flip the order of the conditional here. That is:
> + if (! *ra_session || (err && err->apr_err == SVN_ERR_RA_ILLEGAL_URL))
Looks good otherwise. Thanks. -- justin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe_at_subversion.tigris.org
For additional commands, e-mail: dev-help_at_subversion.tigris.org
Received on 2008-10-05 18:13:43 CEST