Re: Thinko in libsvn_client/merge.c?

Roderich Schupp wrote:
> Hi,
>
> while looking at calls of svn_client__open_ra_session_internal (to check for
> unnecessarily opening a session in the repository root) I stumbled about
> the following (first few lines of function merge_reintegrate_locked
> in libsvn_client/merge.c):
>
> static svn_error_t *
> merge_reintegrate_locked(const char *source,
> const svn_opt_revision_t *peg_revision,
> const char *target_abspath,
> svn_boolean_t dry_run,
> const apr_array_header_t *merge_options,
> svn_client_ctx_t *ctx,
> apr_pool_t *scratch_pool)
> {
> ...
> /* Make sure we're dealing with a real URL. */
> SVN_ERR(svn_client_url_from_path2(&url2, source, ctx,
> scratch_pool, scratch_pool));
> if (! url2)
> return svn_error_createf(SVN_ERR_ENTRY_MISSING_URL, NULL,
> _("'%s' has no URL"),
> svn_dirent_local_style(source, scratch_pool));
>
> /* Determine the working copy target's repository root URL. */
> working_revision.kind = svn_opt_revision_working;
> SVN_ERR(svn_client__get_repos_root(&wc_repos_root, target_abspath,
> &working_revision, ctx,
> scratch_pool, scratch_pool));
>
> /* Open an RA session to our source URL, and determine its root URL. */
> SVN_ERR(svn_client__open_ra_session_internal(&ra_session,
> wc_repos_root, <========
> NULL, NULL,
> FALSE, FALSE, ctx,
> scratch_pool));
> SVN_ERR(svn_ra_get_repos_root2(ra_session, &source_repos_root, scratch_pool));
>
> /* source_repos_root and wc_repos_root are required to be the same,
> as mergeinfo doesn't come into play for cross-repository merging. */
> if (strcmp(source_repos_root, wc_repos_root) != 0)
> return svn_error_createf(SVN_ERR_CLIENT_UNRELATED_RESOURCES, NULL,
> _("'%s' must be from the same repository as "
> ...
>
> My gripe is with the line marked <===== and the check at the end of the snippet:
> If we go from working copy (target_abspath) to its repos_root (wc_repos_root),
> then open a session there and then ask that for the repos_root
> (source_repos_root),
> the two *repos_roots should be the same by construction, hence the
> check is bogus.
>
> However, it would make sense if "wc_repos_root" in <==== was a thinko and
> "url2" was actually intended. Then also the comment above <===== would make
> sense, because url2 is the URL form of "source".
>
> Cheers, Roderich

Roderich, huge thanks for looking more at this collection of issues lovingly
(*cough*) known as "issue 3242".

I agree -- something is weird about this function, and I think you've
identified exactly the weirdness and what the right fix might be. Have you
tried the suggested fix to see how it affects our test suite runs? Would
you be willing/able to generate, test, and transmit a patch to that affect?

-- 
C. Michael Pilato <cmpilato_at_collab.net>
CollabNet   <>   www.collab.net   <>   Distributed Development On Demand