You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Roderich Schupp <ro...@googlemail.com> on 2010/04/13 09:14:59 UTC
Thinko in libsvn_client/merge.c?
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
Re: [PATCH] Re: Thinko in libsvn_client/merge.c?
Posted by "C. Michael Pilato" <cm...@collab.net>.
Roderich Schupp wrote:
> [[[
> Fix check in "merge --reintegrate" whether merge source and
> working copy target belong to the same repository.
>
> * subversion/libsvn_client/merge.c
> (merge_reintegrate_locked) Correctly compute the repos root URL
> for the merge source.
> ]]]
>
>
> Cheers, Roderich
Looks great. Committed (with the aforesuggested FIXME comment):
Sending trunk/subversion/libsvn_client/merge.c
Transmitting file data .
Committed revision 935631.
Will propose for backport to 1.6.x, too.
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Distributed Development On Demand
[PATCH] Re: Thinko in libsvn_client/merge.c?
Posted by Roderich Schupp <ro...@googlemail.com>.
On Thu, Apr 15, 2010 at 2:25 PM, C. Michael Pilato <cm...@collab.net> wrote:
> Roderich Schupp wrote:
>> On Tue, Apr 13, 2010 at 4:37 PM, C. Michael Pilato <cm...@collab.net> wrote:
> Cool. I'll watch for your followup posts, and be glad to try to answer any
> questions you might have about the code.
OK, here's my patch for merge_reintegrate_locked.
Passes all regression tests (over local and HTTP transport).
So if we try to
svn merge --reintegrate SOME-URL ANOTHER-WC
where ANOTHER-WC is from a different repository than SOME-URL
if would previously fail with
svn: URL 'SOME-URL' is not a child of repository root URL 'ANOTHER-URL'
(where ANOTHER-URL is the URL of the repos root of ANOTHER-WC)
and now it fails with the much clearer
svn: 'SOME-URL' must be from the same repository as 'ANOTHER-WC'
However, that doesn't address the fact that this is probably another case
where a session is indiscriminantly opened at the repository root
(the root cause of issue 3242). So perhaps a "fixme" comment
should be added as well.
[[[
Fix check in "merge --reintegrate" whether merge source and
working copy target belong to the same repository.
* subversion/libsvn_client/merge.c
(merge_reintegrate_locked) Correctly compute the repos root URL
for the merge source.
]]]
Cheers, Roderich
Re: Thinko in libsvn_client/merge.c?
Posted by "C. Michael Pilato" <cm...@collab.net>.
Roderich Schupp wrote:
> On Tue, Apr 13, 2010 at 4:37 PM, C. Michael Pilato <cm...@collab.net> wrote:
>> ... 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?
>
> Will do, it will take a day or so.
>
> Cheers, Roderich
Cool. I'll watch for your followup posts, and be glad to try to answer any
questions you might have about the code.
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Distributed Development On Demand
Re: Thinko in libsvn_client/merge.c?
Posted by Roderich Schupp <ro...@googlemail.com>.
On Tue, Apr 13, 2010 at 4:37 PM, C. Michael Pilato <cm...@collab.net> wrote:
> ... 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?
Will do, it will take a day or so.
Cheers, Roderich
Re: Thinko in libsvn_client/merge.c?
Posted by "C. Michael Pilato" <cm...@collab.net>.
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 <cm...@collab.net>
CollabNet <> www.collab.net <> Distributed Development On Demand