RE: [PATCH] Fix cause of merge_tests.py 61 failure

Sorry for the delay, for reasons unknown this ended up in my junk e-mail
folder...

> -----Original Message-----
> From: dglasser_at_gmail.com [mailto:dglasser_at_gmail.com] On
> Behalf Of David Glasser
> Sent: Friday, January 04, 2008 11:34 AM
> To: Paul Burba
> Cc: dev
> Subject: Re: [PATCH] Fix cause of merge_tests.py 61 failure
>
> On Jan 3, 2008 6:50 PM, Paul Burba <pburba_at_collab.net> wrote:
> > On IRC today...
> >
> > <glasser> pburba: so basically I'm having some angst with
> > merge_fails_if_subtree_is_deleted_on_src test <glasser> it
> first does
> > a merge of some changes into A_copy/D/gamma, so that file
> ends up with
> > mergeinfo <glasser> and then it does a merge of merge changes into
> > A_copy/ which includes a delete of A/D/gamma <glasser> and
> deep inside
> > calculate_remaining_ranges -> filter_reflected_revisions it ends up
> > called svn_ra_get_mergeinfo on A/D/gamma <glasser> with the
> revision
> > after it is deleted (which is the last revision specified on the
> > commandline) as the rev <glasser> so on the reintegrate
> branch, this
> > is a file-not-found error <glasser> what should the fix be?
> this code
> > is pretty complex <pburba> glasser: Let me take a look
> >
> > The test fails because of the call to
> > svn_client__get_repos_mergeinfo() in filter_reflected_revisions()
> > returns an error when we ask for the mergeinfo for a non-existant
> > path. On trunk no error is returned, instead
> *TARGET_MERGEINFO is set
> > to an empty hash. I recall you discussing the fact that on
> trunk our
> > queries of the SQLite db didn't return an error when a requested
> > path_at_rev didn't exist. I assume the changes you made on
> the integrate
> > branch resulted in an error now being returned by
> svn_client__get_repos_mergeinfo(), does that sound right?
> > If so, then the fix looks as simple as clearing the error. See
> > attached patch (which passes all merge tests).
>
> Thanks! Yes, that is the issue I was discussing. Some
> further questions:
>
> >
> > [[[
> > On the reintegrate branch, fix cause of merge_tests.py 61 failure.
> >
> > * subversion/libsvn_client/merge.c
> > (filter_reflected_revisions): Handle the fact that the second
> > URL/revision
> > pair might not exist causing svn_client__get_repos_mergeinfo() to
> > return an
> > error.
> > ]]]
> >
> > Paul
> >
>
> > Index: subversion/libsvn_client/merge.c
> > ===================================================================
> > --- subversion/libsvn_client/merge.c (revision 28740)
> > +++ subversion/libsvn_client/merge.c (working copy)
> > @@ -1219,6 +1219,7 @@
> > const char *min_url = (revision1 < revision2) ? url1 : url2;
> > const char *max_url = (revision1 < revision2) ? url2 : url1;
> > const char *min_rel_path, *max_rel_path;
> > + svn_error_t *err;
> >
> > SVN_ERR(svn_client__path_relative_to_root(&min_rel_path, min_url,
> >
> source_root_url, FALSE,
> > ra_session, @@ -1233,25 +1234,38 @@
> > min_rel_path, min_rev,
> >
> svn_mergeinfo_inherited, TRUE,
> > pool));
> > - SVN_ERR(svn_client__get_repos_mergeinfo(ra_session,
> &end_mergeinfo,
> > - max_rel_path, max_rev,
> > -
> svn_mergeinfo_inherited, TRUE,
> > - pool));
> > -
> > - SVN_ERR(svn_mergeinfo_diff(&deleted_mergeinfo, &added_mergeinfo,
> > - start_mergeinfo, end_mergeinfo,
> > - FALSE, pool));
> > -
> > - if (added_mergeinfo)
> > + err = svn_client__get_repos_mergeinfo(ra_session, &end_mergeinfo,
> > + max_rel_path, max_rev,
> > +
> svn_mergeinfo_inherited, TRUE,
> > + pool);
> > + /* When operating on subtrees with intersecting
> mergeinfo, URL1_at_REVISION1
> > + might have been deleted and URL2_at_REVISION2 won't
> exist. Don't return an
> > + error in this case, just proceed without bothering to look for
> > + ADDED_MERGEINFO. */
>
> Would there be a reason to make a similar change to the
> start_mergeinfo query?

I think yes, because URL2_at_REVISION2 might have been added and not exist
at URL1_at_REVISION1. Something along the lines of the new attached patch
(which passes all merge tests on the reintegrate branch). But I'm
seeing an unrelated problem while trying to test this out (a problem
which exist on trunk). Specifically, if we perform a merge which adds a
path, then, without committing, perform a second merge with a range that
is a superset of the first we get an error, e.g.:

>svn merge %url%/A merge_tests-61\A_copy -r4:6
--- Merging r5 through r6 into 'merge_tests-61\A_copy':
A merge_tests-61\A_copy\D\newfile
D merge_tests-61\A_copy\D\gamma

>svn merge %url%/A merge_tests-61\A_copy -r1:6
..\..\..\subversion\libsvn_repos\reporter.c:778: (apr_err=160013)
svn: Working copy path 'D/newfile' does not exist in repository

Looking at this now.
 
> > + if (err)
> > {
> > - const char *mergeinfo_path;
> > -
> SVN_ERR(svn_client__path_relative_to_root(&mergeinfo_path, target_url,
> > -
> source_root_url, TRUE,
> > -
> ra_session, NULL, pool));
> > - src_rangelist_for_tgt =
> apr_hash_get(added_mergeinfo, mergeinfo_path,
> > - APR_HASH_KEY_STRING);
> > + if (err->apr_err == SVN_ERR_FS_NOT_FOUND
> > + || err->apr_err == SVN_ERR_RA_DAV_REQUEST_FAILED)
> > + svn_error_clear(err);
>
> I'm a big fan of also setting cleared errors to NULL, to
> avoid accidentally returning them in a later codepath.

Agreed, that's a good practice.
 
> > + else
> > + return err;
> > }
> > + else
> > + {
> > + SVN_ERR(svn_mergeinfo_diff(&deleted_mergeinfo,
> &added_mergeinfo,
> > + start_mergeinfo, end_mergeinfo,
> > + FALSE, pool));
> >
> > + if (added_mergeinfo)
> > + {
> > + const char *mergeinfo_path;
> > +
> SVN_ERR(svn_client__path_relative_to_root(&mergeinfo_path, target_url,
> > +
> source_root_url, TRUE,
> > +
> ra_session, NULL, pool));
> > + src_rangelist_for_tgt =
> apr_hash_get(added_mergeinfo, mergeinfo_path,
> > +
> APR_HASH_KEY_STRING);
> > + }
> > + }
> > /* Create a single-item list of ranges with our one
> requested range
> > in it, and then remove overlapping revision ranges
> from that range. */
> > *requested_rangelist = apr_array_make(pool, 1,
> sizeof(range)); @@
> > -5215,6 +5229,7 @@
> > "it has descendents with
> different revisions "
> > "merged", source_repos_rel_path);
> > }
> > + return SVN_NO_ERROR;
> > }
>
> This is an unrelated (reintegrate-branch-only) change. Did
> you need it to avoid warnings? All the branches of the
> if/else structure return except one which aborts.

Oops, I was getting a warning and meant to just fix that separately but
forgot about it and accidentally included it in the patch. Any
objections to that particular change?

Paul

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe_at_subversion.tigris.org
For additional commands, e-mail: dev-help_at_subversion.tigris.org