On Thu, Oct 20, 2011 at 8:02 AM, Julian Foad <julianf...@btopenworld.com> wrote:
> And in one more place in the same file (this is untested and my previouos
> patch was not fully tested):
> [[[
> @@ -1598,31 +1586,15 @@ logs_for_mergeinfo_rangelist(const char
>
> if (! target_mergeinfo_catalog)
> target_mergeinfo_catalog = apr_hash_make(scratch_pool);
>
> /* FILTER_LOG_ENTRY_BATON_T->TARGET_MERGEINFO_CATALOG's keys are required
> to be repository-absolute. */
> - if (apr_hash_count(target_mergeinfo_catalog))
> - {
> - apr_hash_index_t *hi;
> - svn_mergeinfo_catalog_t rekeyed_catalog = apr_hash_make(scratch_pool);
> -
> - for (hi = apr_hash_first(scratch_pool, target_mergeinfo_catalog);
> - hi;
> - hi = apr_hash_next(hi))
> - {
> - const char *path = svn__apr_hash_index_key(hi);
> -
> - if (!svn_dirent_is_absolute(path))
> - apr_hash_set(rekeyed_catalog,
> - svn_dirent_join("/", path, scratch_pool),
> - APR_HASH_KEY_STRING,
> - svn__apr_hash_index_val(hi));
> - }
> - target_mergeinfo_catalog = rekeyed_catalog;
> - }
> + SVN_ERR(svn_mergeinfo__add_prefix_to_catalog(&target_mergeinfo_catalog,
> + target_mergeinfo_catalog, "/",
> + scratch_pool, scratch_pool));
>
> /* Build the log filtering callback baton. */
> fleb.filtering_merged = filtering_merged;
> ]]]
>
> - Julian
>
>
> --- On Thu, 20/10/11, Julian Foad <julian.f...@wandisco.com> wrote:
>
>> From: Julian Foad <julian.f...@wandisco.com>
>> Subject: Re: svn commit r1156750 - More fixes for issue #3986
>> 'svn_client_mergeinfo_log API is broken'.
>> To: dev@subversion.apache.org, "Paul Burba" <ptbu...@gmail.com>
>> Date: Thursday, 20 October, 2011, 12:46
>> Hi Paul.
>>
>> r1156750 added a chunk of code in
>> svn_client__get_repos_mergeinfo_catalog() that appears to
>> be adding a
>> prefix to a mergeinfo catalog. There is already a
>> function
>> "svn_mergeinfo__add_prefix_to_catalog()" for doing that;
>> could you use
>> it? Suggested patch below.
>>
>> Also r1156750 changed the line "repos_mergeinfo = NULL;"
>> to
>> "*mergeinfo_cat = NULL;" which I think is wrong because in
>> that case
>> the following "if (repos_mergeinfo_cat == NULL)" may be
>> testing an
>> uninitialized value (depending on the implementation of
>> svn_ra_get_mergeinfo() in the case when it returns an
>> error). See in
>> the patch below.
Hi Julian,
You are quite correct, the above was relying on the current
implementation of svn_ra_get_mergeinfo. Made that change by itself in
r1186928. I will nominate for backport in a moment.
The rest of your patch looks good and behaves as expected. I tested
it without any problems. Committed that too (r1186931) since it was
in front of me (crediting you of course).
Paul
>> [[[
>> Index: subversion/libsvn_client/mergeinfo.c
>> ===================================================================
>> --- subversion/libsvn_client/mergeinfo.c
>> (revision 1186733)
>> +++ subversion/libsvn_client/mergeinfo.c
>> (working copy)
>> @@ -487,54 +487,42 @@
>> svn_client__get_repos_mergeinfo_catalog(
>> if (err)
>> {
>> if (squelch_incapable
>> && err->apr_err == SVN_ERR_UNSUPPORTED_FEATURE)
>> {
>>
>> svn_error_clear(err);
>>
>> *mergeinfo_cat = NULL;
>> + /* ### should say
>> "repos_mergeinfo_cat = NULL" or add a
>> "return" here? */
>> }
>> else
>> return
>> svn_error_trace(err);
>> }
>>
>> if (repos_mergeinfo_cat == NULL)
>> {
>> *mergeinfo_cat = NULL;
>> }
>> else
>> {
>> - const char *repos_root;
>> const char *session_url;
>> + const char *session_relpath;
>>
>> -
>> SVN_ERR(svn_ra_get_repos_root2(ra_session, &repos_root,
>> scratch_pool));
>>
>> SVN_ERR(svn_ra_get_session_url(ra_session,
>> &session_url, scratch_pool));
>> +
>> SVN_ERR(svn_ra_get_path_relative_to_root(ra_session,
>> &session_relpath,
>> +
>>
>>
>> session_url, scratch_pool));
>>
>> - if (strcmp(repos_root, session_url)
>> == 0)
>> + if (session_relpath[0] == '\0')
>> {
>>
>> *mergeinfo_cat = repos_mergeinfo_cat;
>> }
>> else
>> {
>> - apr_hash_index_t *hi;
>> - svn_mergeinfo_catalog_t
>> rekeyed_mergeinfo_cat =
>> -
>> apr_hash_make(result_pool);
>> -
>> - for (hi =
>> apr_hash_first(scratch_pool, repos_mergeinfo_cat);
>> -
>> hi;
>> -
>> hi = apr_hash_next(hi))
>> - {
>> - const
>> char *path =
>> -
>> svn_path_url_add_component2(session_url,
>> -
>>
>>
>> svn__apr_hash_index_key(hi),
>> -
>>
>> scratch_pool);
>> -
>> SVN_ERR(svn_ra_get_path_relative_to_root(ra_session,
>> &path,
>> -
>>
>>
>> path,
>> -
>>
>>
>> result_pool));
>> -
>> apr_hash_set(rekeyed_mergeinfo_cat, path,
>> APR_HASH_KEY_STRING,
>> -
>>
>> svn__apr_hash_index_val(hi));
>> - }
>> - *mergeinfo_cat =
>> rekeyed_mergeinfo_cat;
>> +
>> SVN_ERR(svn_mergeinfo__add_prefix_to_catalog(mergeinfo_cat,
>> +
>>
>>
>> repos_mergeinfo_cat,
>> +
>>
>>
>> session_relpath,
>> +
>>
>>
>> result_pool,
>> +
>>
>>
>> scratch_pool));
>> }
>> }
>> return SVN_NO_ERROR;
>> }
>>
>>
>> ]]]
>>
>> - Julian
>>
>
