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-unsubscribesubversion.tigris.org
For additional commands, e-mail: dev-helpsubversion.tigris.org
|