[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: Julian Foad <julianfoad_at_btopenworld.com>
Date: Thu, 20 Oct 2011 13:02:18 +0100 (BST)

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.
>
> [[[
> 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 14:02:54 CEST

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