Mike, some questions inline below...
On Aug 27, 2007, at 9:46 AM, cmpilato@tigris.org wrote:
...
> Finish issue #2890: Expose svn_client__suggest_merge_sources(); make
> svn_client_merge_peg3() require real input again.
>
> Rather than have svn_client_merge_peg3() do a bunch of super-secret
> voodoo calculations to determine the merge source when none is
> provided, we now expose the voodoo-calculating function through the
> public API. This will allow alternative clients to tell users exactly
> what they would use as part of a merge *before* actually using it, and
> overall makes for a tighter API.
>
> * subversion/include/private/svn_client_private.h,
> * subversion/include/svn_client.h
> (svn_client_suggest_merge_sources): Moved from svn_client_private.h
> to svn_client.h. Move 'suggestions' parameter to the head of the
> list. Lose the unnecessary 'revision' parameter.
Should we allow querying of merge sources by URL (doc string seems to
indicate that we don't currently allow it)? If so, how are you
assuming that the revision parameter will be passed now (e.g. in peg
rev-style)? I think this needs a doc tweak either way to clarify
things.
> * subversion/libsvn_client/log.c
> (svn_client_suggest_merge_sources): Was
> svn_client__suggest_merge_sources.
> Move 'suggestions' parameter to the head of the list. Lose the
> unnecessary 'revision' parameter, and just create the needed
> revision on-the-fly. Make returned paths be full URLs. Finally,
> do a little whitespace management.
The doc string on svn_client_suggest_merge_sources() (in
svn_client.h) doesn't seem to reflect the fact that the suggestions
are full URLs. Question: Are these URLs also peg rev-style?
> * subversion/libsvn_client/merge.c
> (svn_client__get_repos_root): Was get_repos_root.
> (svn_client_merge3): Update calls to svn_client__get_repos_root().
> (svn_client_merge_peg3): Disallow missing source paths, and leave
> merge source suggestion-taking to our caller. Update calls to
> svn_client__get_repos_root().
Are any doc string changes to svn_client_merge_peg3() necessary as a
result of this change in behavior?
...
> --- trunk/subversion/include/svn_client.h (original)
> +++ trunk/subversion/include/svn_client.h Mon Aug 27 09:46:02 2007
> @@ -2335,6 +2335,22 @@
> * @{
> */
>
> +/** Set @a suggestions to an ordered array of @c const char *
> + * potential merge sources (expressed as URLs relative to the
> + * repository root URL for the repository with which @a path is
> + * associated) for @a path at @a revision. @path a working copy
> path.
> + * @a ctx is a context used for authentication in the repository
> case.
> + * Use @a pool for all allocations.
> + *
> + * @since New in 1.5.
> + */
> +svn_error_t *
> +svn_client_suggest_merge_sources(apr_array_header_t **suggestions,
> + const char *path,
> + svn_client_ctx_t *ctx,
> + apr_pool_t *pool);
To me, it's not entirely clear that the doc string for SUGGESTIONS
meshes with its description in the log message quoted above.
...
> --- trunk/subversion/libsvn_client/log.c (original)
> +++ trunk/subversion/libsvn_client/log.c Mon Aug 27 09:46:02 2007
> @@ -206,56 +206,60 @@
> }
>
> svn_error_t *
> -svn_client__suggest_merge_sources(const char *path_or_url,
> - const svn_opt_revision_t *revision,
> - apr_array_header_t **suggestions,
> - svn_client_ctx_t *ctx,
> - apr_pool_t *pool)
> +svn_client_suggest_merge_sources(apr_array_header_t **suggestions,
> + const char *path,
> + svn_client_ctx_t *ctx,
> + apr_pool_t *pool)
> {
> + const char *repos_root;
> const char *copyfrom_path;
> + apr_array_header_t *list;
> svn_revnum_t copyfrom_rev;
> apr_hash_t *mergeinfo;
> apr_hash_index_t *hi;
> + svn_opt_revision_t revision;
> + revision.kind = svn_opt_revision_working;
>
> - *suggestions = apr_array_make(pool, 1, sizeof(const char *));
> + list = apr_array_make(pool, 1, sizeof(const char *));
>
> /* In our ideal algorithm, the list of recommendations should be
> ordered by:
>
> - 1) The most recent existing merge source.
> - 2) The copyfrom source (which will also be listed as a merge
> - source if the copy was made with a 1.5+ client and server).
> - 3) All other merge sources, most recent to least recent.
> + 1. The most recent existing merge source.
> + 2. The copyfrom source (which will also be listed as a merge
> + source if the copy was made with a 1.5+ client and
> server).
> + 3. All other merge sources, most recent to least recent.
>
> However, determining the order of application of merge sources
> requires a new RA API. Until such an API is available, our
> algorithm will be:
>
> - 1) The copyfrom source.
> - 2) All remaining merge sources (unordered). */
> -
> - /* ### TODO: Use RA APIs directly to improve efficiency. */
> - SVN_ERR(svn_client__get_copy_source(path_or_url, revision,
> ©from_path,
> + 1. The copyfrom source.
> + 2. All remaining merge sources (unordered).
> + */
> +
> + /* ### TODO: Share ra_session batons to improve efficiency? */
> + SVN_ERR(svn_client__get_repos_root(&repos_root, path, ctx, pool));
> + SVN_ERR(svn_client__get_copy_source(path, &revision,
> ©from_path,
> ©from_rev, ctx, pool));
> if (copyfrom_path)
> - APR_ARRAY_PUSH(*suggestions, const char *) = copyfrom_path;
> -
> - SVN_ERR(svn_client_get_mergeinfo(&mergeinfo, path_or_url, revision,
> - ctx, pool));
> -
> - if (!mergeinfo)
> - return SVN_NO_ERROR;
> + APR_ARRAY_PUSH(list, const char *) =
> + svn_path_url_add_component(repos_root, copyfrom_path + 1,
> pool);
>
> - for (hi = apr_hash_first(NULL, mergeinfo); hi; hi = apr_hash_next
> (hi))
> + SVN_ERR(svn_client_get_mergeinfo(&mergeinfo, path, &revision,
> ctx, pool));
> + if (mergeinfo)
> {
> - const char *path;
> - apr_hash_this(hi, (void *) &path, NULL, NULL);
> - if (copyfrom_path == NULL || strcmp(path, copyfrom_path) != 0)
> + for (hi = apr_hash_first(NULL, mergeinfo); hi; hi =
> apr_hash_next(hi))
> {
> - APR_ARRAY_PUSH(*suggestions, const char *) = apr_pstrdup
> (pool, path);
> + const char *merge_path;
> + apr_hash_this(hi, (void *)(&merge_path), NULL, NULL);
> + if (copyfrom_path == NULL || strcmp(merge_path,
> copyfrom_path) != 0)
> + APR_ARRAY_PUSH(list, const char *) =
> + svn_path_url_add_component(repos_root, merge_path +
> 1, pool);
> }
> }
>
> + *suggestions = list;
> return SVN_NO_ERROR;
> }
...
> --- trunk/subversion/libsvn_client/merge.c (original)
> +++ trunk/subversion/libsvn_client/merge.c Mon Aug 27 09:46:02 2007
> @@ -3503,19 +3503,20 @@
> return SVN_NO_ERROR;
> }
>
> -/* Get repository root of PATH_OR_URL as *REPOS_ROOT.
> - * Cascade CTX.
> - * All allocations occur in POOL.
> - */
> -static svn_error_t *
> -get_repos_root(const char **repos_root, const char *path_or_url,
> - svn_client_ctx_t *ctx, apr_pool_t *pool)
> +svn_error_t *
> +svn_client__get_repos_root(const char **repos_root,
> + const char *path_or_url,
> + svn_client_ctx_t *ctx,
> + apr_pool_t *pool)
This routine should find a new home, ether in util.c or ra.c.
> {
> svn_revnum_t rev;
> svn_opt_revision_t target_revision;
> svn_ra_session_t *ra_session;
> const char *target_url;
> target_revision.kind = svn_opt_revision_working;
> +
> + /* ### FIXME: If PATH_OR_URL is a local path, we should first see
> + ### if its entry already has the repository root URL. */
Something like:
if (!svn_path_is_url(path_or_url))
{
const svn_wc_entry_t *entry;
// get adm_access ...
SVN_ERR(svn_wc__entry_version(&entry, ...));
}
I think we have some very similar code in
svn_client__path_relative_to_root()...
> SVN_ERR(svn_client__ra_session_from_path(&ra_session,
> &rev,
> &target_url,
> @@ -3623,7 +3624,8 @@
> }
> else
> {
> - SVN_ERR(get_repos_root(&wc_repos_root, target_wcpath, ctx,
> pool));
> + SVN_ERR(svn_client__get_repos_root(&wc_repos_root,
> target_wcpath,
> + ctx, pool));
> }
>
> if (depth == svn_depth_unknown)
> @@ -3805,48 +3807,24 @@
> }
> else
> {
> - SVN_ERR(get_repos_root(&wc_repos_root, target_wcpath, ctx,
> pool));
> + SVN_ERR(svn_client__get_repos_root(&wc_repos_root,
> target_wcpath,
> + ctx, pool));
> }
>
> -
> - if (source)
> - {
> - /* If source is a path, we need to get the underlying URL from
> - the wc and save the initial path we were passed so we can
> use
> - it as a path parameter (either in the baton or not).
> - otherwise, the path will just be NULL, which means we won't
> - be able to figure out some kind of revision specifications,
> - but in that case it won't matter, because those ways of
> - specifying a revision are meaningless for a URL. */
> - SVN_ERR(svn_client_url_from_path(&URL, source, pool));
> - if (! URL)
> - return svn_error_createf(SVN_ERR_ENTRY_MISSING_URL, NULL,
> - _("'%s' has no URL"),
> - svn_path_local_style(source, pool));
> - if (URL != source)
> - path = source;
> - }
> - else
> - {
> - /* If a merge source was not specified, try to derive it. */
> - apr_array_header_t *suggested_sources;
> - svn_opt_revision_t target_revision;
> - target_revision.kind = svn_opt_revision_working;
> - SVN_ERR(svn_client__suggest_merge_sources(target_wcpath,
> - &target_revision,
> - &suggested_sources,
> - ctx, pool));
> - if (! suggested_sources->nelts)
> - return svn_error_createf(SVN_ERR_INCORRECT_PARAMS, NULL,
> - _("Unable to determine merge
> source for "
> - "'%s', please provide an
> explicit source"),
> - svn_path_local_style
> (target_wcpath, pool));
> -
> - /* Prepend the repository root path to the copy source path. */
> - URL = apr_pstrcat(pool, wc_repos_root,
> - APR_ARRAY_IDX(suggested_sources, 0, char *),
> - NULL);
> - }
> + /* If source is a path, we need to get the underlying URL from
> + the wc and save the initial path we were passed so we can use
> + it as a path parameter (either in the baton or not).
> + otherwise, the path will just be NULL, which means we won't
> + be able to figure out some kind of revision specifications,
> + but in that case it won't matter, because those ways of
> + specifying a revision are meaningless for a URL. */
> + SVN_ERR(svn_client_url_from_path(&URL, source, pool));
> + if (! URL)
> + return svn_error_createf(SVN_ERR_ENTRY_MISSING_URL, NULL,
> + _("'%s' has no URL"),
> + svn_path_local_style(source, pool));
> + if (URL != source)
> + path = source;
>
...
> --- trunk/subversion/svn/merge-cmd.c (original)
> +++ trunk/subversion/svn/merge-cmd.c Mon Aug 27 09:46:02 2007
> @@ -195,6 +195,24 @@
>
> if (using_rev_range_syntax)
> {
> + if (! sourcepath1)
> + {
> + /* If a merge source was not specified, try to derive
> it. */
> + apr_array_header_t *suggested_sources;
> + svn_opt_revision_t working_revision;
> + working_revision.kind = svn_opt_revision_working;
working_revision has been pushed into svn_client_suggest_merge_sources
(). I removed it in r26343.
> +
> + SVN_ERR(svn_client_suggest_merge_sources
> (&suggested_sources,
> + targetpath,
> ctx, pool));
> + if (! suggested_sources->nelts)
> + return svn_error_createf(SVN_ERR_INCORRECT_PARAMS, NULL,
> + _("Unable to determine merge
> source for "
> + "'%s'. Please provide an
> explicit "
> + "source"),
This error message uses a period in the middle of it, but lacks a
trailing period. I'd rather see the period used in the middle of it
replaced with "--" or some such.
> + svn_path_local_style
> (targetpath, pool));
> + sourcepath1 = APR_ARRAY_IDX(suggested_sources, 0, const
> char *);
> + }
...
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Tue Aug 28 05:23:58 2007