Re: svn commit: r27710 - trunk/subversion/libsvn_client

On Mon, 12 Nov 2007, C. Michael Pilato wrote:

> Daniel Rall wrote:
> > On Thu, 08 Nov 2007, cmpilato@tigris.org wrote:
> > ...
> >> New client library helper function, svn_client__get_implicit_mergeinfo().
> >>
> >> * subversion/libsvn_client/mergeinfo.h,
> >> * subversion/libsvn_client/mergeinfo.c
> >> (svn_client__get_implicit_mergeinfo): New function.
> >>
> >> Modified:
> >> trunk/subversion/libsvn_client/mergeinfo.c
> >> trunk/subversion/libsvn_client/mergeinfo.h
> >>
> >> Modified: trunk/subversion/libsvn_client/mergeinfo.c
> >> URL: http://svn.collab.net/viewvc/svn/trunk/subversion/libsvn_client/mergeinfo.c?pathrev=27710&r1=27709&r2=27710
> >> ==============================================================================
> >> --- trunk/subversion/libsvn_client/mergeinfo.c (original)
> >> +++ trunk/subversion/libsvn_client/mergeinfo.c Thu Nov 8 07:25:48 2007
> >> @@ -400,6 +400,129 @@
> >> return SVN_NO_ERROR;
> >> }
> >>
> >> +
> >> +svn_error_t *
> >> +svn_client__get_implicit_mergeinfo(apr_hash_t **mergeinfo_p,
> >> + const char *path_or_url,
> >> + const svn_opt_revision_t *peg_revision,
> >> + svn_ra_session_t *ra_session,
> >> + svn_wc_adm_access_t *adm_access,
> >> + svn_client_ctx_t *ctx,
> >> + apr_pool_t *pool)
> >> +{
> >> + apr_array_header_t *segments;
> >> + svn_revnum_t peg_revnum = SVN_INVALID_REVNUM;
> >> + svn_ra_session_t *session = ra_session;
> >> + const char *url;
> >> + apr_hash_t *mergeinfo = apr_hash_make(pool);
> >> + apr_pool_t *sesspool = NULL; /* only used for an RA session we open */
> >> + int i;
> >> +
> >> + /* If PATH_OR_URL is a local path (not a URL), we need to transform
> >> + it into a URL, open an RA session for it, and resolve the peg
> >> + revision. Note that if the local item is scheduled for addition
> >> + as a copy of something else, we'll use its copyfrom data to query
> >> + its history. */
> >> + if (! svn_path_is_url(path_or_url))
> >> + {
> >> + const svn_wc_entry_t *entry;
> >> +
> >> + ra_session = NULL;
> >> + if (adm_access)
> >> + {
> >> + SVN_ERR(svn_wc_entry(&entry, path_or_url, adm_access, FALSE, pool));
> >> + }
> >> + else
> >> + {
> >> + SVN_ERR(svn_wc_adm_probe_open3(&adm_access, NULL, path_or_url,
> >> + FALSE, 0, ctx->cancel_func,
> >> + ctx->cancel_baton, pool));
> >> + SVN_ERR(svn_wc_entry(&entry, path_or_url, adm_access, FALSE, pool));
> >> + SVN_ERR(svn_wc_adm_close(adm_access));
> >> + }
> >> +
> >> + if (entry->copyfrom_url
> >> + && peg_revision->kind == svn_opt_revision_working)
> >> + {
> >> + url = entry->copyfrom_url;
> >> + peg_revnum = entry->copyfrom_rev;
> >> + }
> >> + else if (entry->url)
> >> + {
> >> + url = entry->url;
> >> + }
> >> + else
> >> + {
> >> + return svn_error_createf(SVN_ERR_ENTRY_MISSING_URL, NULL,
> >> + _("'%s' has no URL"),
> >> + svn_path_local_style(path_or_url, pool));
> >> + }
> >> + }
> >> + else
> >> + {
> >> + url = path_or_url;
> >> + }
> >
> > I feel like I've written the above code ten times. I can't put my
> > finger on exactly what it should look like, but I get the feeling we
> > should be using a shared routine for this...
> >
> > How about something like:
> >
> > /* Set *URL to the repository URL of PATH_OR_URL. ADM_ACCESS may be
> > NULL, if PATH_OR_URL is a URL. Use POOL for all allocations. */
> > svn_error_t *
> > svn_client__derive_url(const char **url,
> > const char *path_or_url,
> > svn_wc_adm_access_t *adm_access,
> > apr_pool_t *pool);
>
> I'm all for it. Even had a patch for it at one point (that got lost in a
> code scramble). I think that we need a flag to say how to deal with
> schedule-add-with-history items (do we want the target URL or the source
> copyfrom URL?), but other than that, +1 on the idea of such a helper function.

I'm attaching a patch-in-progress that implements this. Unfortunately
I managed to inject a few test failures while adding that optimization
we discussed on IRC (had a clean test run before that, bleh).