[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: svn commit r1156750 - More fixes for issue #3986 'svn_client_mergeinfo_log API is broken'.

From: Paul Burba <ptburba_at_gmail.com>
Date: Thu, 20 Oct 2011 13:13:09 -0400

On Thu, Oct 20, 2011 at 8:02 AM, Julian Foad <julianfoad_at_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.foad_at_wandisco.com> wrote:
>
>> From: Julian Foad <julian.foad_at_wandisco.com>
>> Subject: Re: svn commit r1156750 - More fixes for issue #3986 'svn_client_mergeinfo_log API is broken'.
>> To: dev_at_subversion.apache.org, "Paul Burba" <ptburba_at_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
>>
>
Received on 2011-10-20 19:13:42 CEST

This is an archived mail posted to the Subversion Dev mailing list.