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?

> + 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_at_subversion.tigris.org
For additional commands, e-mail: dev-help_at_subversion.tigris.org