You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Paul Burba <pb...@collab.net> on 2008/01/03 23:50:38 UTC

[PATCH] Fix cause of merge_tests.py 61 failure

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@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).

[[[
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

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

Posted by Paul Burba <pb...@collab.net>.
> -----Original Message-----
> From: Paul Burba [mailto:pburba@collab.net] 
> Sent: Tuesday, January 08, 2008 12:28 PM
> To: David Glasser
> Cc: dev
> Subject: RE: [PATCH] Fix cause of merge_tests.py 61 failure
> 
> > -----Original Message-----
> > From: Paul Burba [mailto:pburba@collab.net]
> > Sent: Monday, January 07, 2008 11:49 AM
> > To: David Glasser
> > Cc: dev
> > Subject: 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@gmail.com [mailto:dglasser@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 <pb...@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@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@REVISION1
> > > > +     might have been deleted and URL2@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@REVISION2 might have been added and not 
> > exist at URL1@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.:
> 
> 'I think yes' is now definitely yes, see below.
> 
> > >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. 
> 
> It takes a relatively contrived setup to expose this error.  
> Added issue
> #3067 to track it and merge_test.py 77
> 'new_subtrees_should_not_break_merge' on trunk to test for 
> it.  This test, when applied to the reintegrate branch does 
> double duty, exposing the need for the similar change to the 
> start_mergeinfo query.
> 
> Shall I commit this (the filter_reflected_revisions changes 
> that is) on the reintegrate branch or did you have any other 
> questions or concerns?

No one has raised any objections to this fix and with the reintegrate
branch merged to trunk I didn't see any reason to delay further.
Committed the fix for merge_tests.py 61 and 77 in r28988.  Any problems,
questions, etc., please let me know!

Thanks,

Paul

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org


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

Posted by Paul Burba <pb...@collab.net>.
> -----Original Message-----
> From: Paul Burba [mailto:pburba@collab.net] 
> Sent: Monday, January 07, 2008 11:49 AM
> To: David Glasser
> Cc: dev
> Subject: 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@gmail.com [mailto:dglasser@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 <pb...@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@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@REVISION1
> > > +     might have been deleted and URL2@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@REVISION2 might have been added and 
> not exist at URL1@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.:

'I think yes' is now definitely yes, see below.

> >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. 

It takes a relatively contrived setup to expose this error.  Added issue
#3067 to track it and merge_test.py 77
'new_subtrees_should_not_break_merge' on trunk to test for it.  This
test, when applied to the reintegrate branch does double duty, exposing
the need for the similar change to the start_mergeinfo query.

Shall I commit this (the filter_reflected_revisions changes that is) on
the reintegrate branch or did you have any other questions or concerns?

Paul

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org


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

Posted by Paul Burba <pb...@collab.net>.
Sorry for the delay, for reasons unknown this ended up in my junk e-mail
folder... 

> -----Original Message-----
> From: dglasser@gmail.com [mailto:dglasser@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 <pb...@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@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@REVISION1
> > +     might have been deleted and URL2@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@REVISION2 might have been added and not exist
at URL1@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

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

Posted by David Glasser <gl...@davidglasser.net>.
On Jan 3, 2008 6:50 PM, Paul Burba <pb...@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@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@REVISION1
> +     might have been deleted and URL2@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?

> +  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.

> +      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.

--dave


-- 
David Glasser | glasser@davidglasser.net | http://www.davidglasser.net/

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org