You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by pb...@apache.org on 2011/09/26 17:15:21 UTC
svn commit: r1175903 - in /subversion/trunk/subversion:
include/svn_mergeinfo.h libsvn_client/merge.c libsvn_repos/log.c
libsvn_subr/deprecated.c libsvn_subr/mergeinfo.c
Author: pburba
Date: Mon Sep 26 15:15:21 2011
New Revision: 1175903
URL: http://svn.apache.org/viewvc?rev=1175903&view=rev
Log:
Rev svn_mergeinfo_merge API to use the two-pool paradigm.
* subversion/include/svn_mergeinfo.h
(svn_mergeinfo_merge2): New.
(svn_mergeinfo_merge): Deprecated.
* subversion/libsvn_subr/mergeinfo.c
(svn_mergeinfo_merge2): New.
(svn_mergeinfo_merge): Move to deprecated.c.
(svn_mergeinfo_catalog_merge): Use the new API.
* subversion/libsvn_subr/deprecated.c
(svn_mergeinfo_merge): Move svn_mergeinfo_merge implementation to here.
* subversion/libsvn_client/merge.c
(update_wc_mergeinfo,
log_find_operative_revs):
* subversion/libsvn_repos/log.c
(get_combined_mergeinfo_changes,
store_search,
get_paths_history_as_mergeinfo):
Update all callers of svn_mergeinfo_merge, with access to a scratch pool,
to the new API.
Modified:
subversion/trunk/subversion/include/svn_mergeinfo.h
subversion/trunk/subversion/libsvn_client/merge.c
subversion/trunk/subversion/libsvn_repos/log.c
subversion/trunk/subversion/libsvn_subr/deprecated.c
subversion/trunk/subversion/libsvn_subr/mergeinfo.c
Modified: subversion/trunk/subversion/include/svn_mergeinfo.h
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/include/svn_mergeinfo.h?rev=1175903&r1=1175902&r2=1175903&view=diff
==============================================================================
--- subversion/trunk/subversion/include/svn_mergeinfo.h (original)
+++ subversion/trunk/subversion/include/svn_mergeinfo.h Mon Sep 26 15:15:21 2011
@@ -195,6 +195,9 @@ svn_mergeinfo_diff(svn_mergeinfo_t *dele
/** Merge a shallow copy of one mergeinfo, @a changes, into another mergeinfo
* @a mergeinfo.
*
+ * Additions to @a mergeinfo are allocated in @a result_pool. Temporary
+ * allocations are made in @a scratch_pool.
+ *
* When intersecting rangelists for a path are merged, the inheritability of
* the resulting svn_merge_range_t depends on the inheritability of the
* operands. If two non-inheritable ranges are merged the result is always
@@ -203,10 +206,22 @@ svn_mergeinfo_diff(svn_mergeinfo_t *dele
* e.g. '/A: 1,3-4' merged with '/A: 1,3,4*,5' --> '/A: 1,3-5'
* '/A: 1,3-4*' merged with '/A: 1,3,4*,5' --> '/A: 1,3,4*,5'
*
- * @since New in 1.5.
+ * @since New in 1.8.
+ */
+svn_error_t *
+svn_mergeinfo_merge2(svn_mergeinfo_t mergeinfo,
+ svn_mergeinfo_t changes,
+ apr_pool_t *result_pool,
+ apr_pool_t *scratch_pool);
+
+/** Like svn_mergeinfo_merge2, but uses only one pool.
+ *
+ * @deprecated Provided for backward compatibility with the 1.5 API.
*/
+SVN_DEPRECATED
svn_error_t *
-svn_mergeinfo_merge(svn_mergeinfo_t mergeinfo, svn_mergeinfo_t changes,
+svn_mergeinfo_merge(svn_mergeinfo_t mergeinfo,
+ svn_mergeinfo_t changes,
apr_pool_t *pool);
/** Combine one mergeinfo catalog, @a changes_catalog, into another mergeinfo
Modified: subversion/trunk/subversion/libsvn_client/merge.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_client/merge.c?rev=1175903&r1=1175902&r2=1175903&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_client/merge.c (original)
+++ subversion/trunk/subversion/libsvn_client/merge.c Mon Sep 26 15:15:21 2011
@@ -4638,8 +4638,8 @@ update_wc_mergeinfo(svn_mergeinfo_catalo
apr_pool_t *result_catalog_pool = apr_hash_pool_get(result_catalog);
if (existing_mergeinfo)
- SVN_ERR(svn_mergeinfo_merge(mergeinfo, existing_mergeinfo,
- result_catalog_pool));
+ SVN_ERR(svn_mergeinfo_merge2(mergeinfo, existing_mergeinfo,
+ result_catalog_pool, scratch_pool));
apr_hash_set(result_catalog,
apr_pstrdup(result_catalog_pool, local_abspath),
APR_HASH_KEY_STRING,
@@ -9919,9 +9919,10 @@ log_find_operative_revs(void *baton,
if (unmerged_for_key)
{
- SVN_ERR(svn_mergeinfo_merge(unmerged_for_key,
- log_entry_as_mergeinfo,
- log_baton->result_pool));
+ SVN_ERR(svn_mergeinfo_merge2(unmerged_for_key,
+ log_entry_as_mergeinfo,
+ log_baton->result_pool,
+ pool));
}
else
{
Modified: subversion/trunk/subversion/libsvn_repos/log.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_repos/log.c?rev=1175903&r1=1175902&r2=1175903&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_repos/log.c (original)
+++ subversion/trunk/subversion/libsvn_repos/log.c Mon Sep 26 15:15:21 2011
@@ -861,14 +861,13 @@ get_combined_mergeinfo_changes(svn_merge
/* Compare, constrast, and combine the results. */
SVN_ERR(svn_mergeinfo_diff(&deleted, &added, prev_mergeinfo,
mergeinfo, FALSE, iterpool));
- SVN_ERR(svn_mergeinfo_merge(*deleted_mergeinfo,
- svn_mergeinfo_dup(deleted, result_pool),
- result_pool));
- SVN_ERR(svn_mergeinfo_merge(*added_mergeinfo,
- svn_mergeinfo_dup(added, result_pool),
- result_pool));
+ SVN_ERR(svn_mergeinfo_merge2(*deleted_mergeinfo,
+ svn_mergeinfo_dup(deleted, result_pool),
+ result_pool, iterpool));
+ SVN_ERR(svn_mergeinfo_merge2(*added_mergeinfo,
+ svn_mergeinfo_dup(added, result_pool),
+ result_pool, iterpool));
}
- svn_pool_destroy(iterpool);
/* Merge all the mergeinfos which are, or are children of, one of
our paths of interest into one giant delta mergeinfo. */
@@ -891,18 +890,20 @@ get_combined_mergeinfo_changes(svn_merge
const char *path = APR_ARRAY_IDX(paths, i, const char *);
if (! svn_dirent_is_ancestor(path, changed_path))
continue;
+ svn_pool_clear(iterpool);
deleted = apr_hash_get(deleted_mergeinfo_catalog, key, klen);
- SVN_ERR(svn_mergeinfo_merge(*deleted_mergeinfo,
- svn_mergeinfo_dup(deleted, result_pool),
- result_pool));
- SVN_ERR(svn_mergeinfo_merge(*added_mergeinfo,
- svn_mergeinfo_dup(added, result_pool),
- result_pool));
+ SVN_ERR(svn_mergeinfo_merge2(*deleted_mergeinfo,
+ svn_mergeinfo_dup(deleted, result_pool),
+ result_pool, iterpool));
+ SVN_ERR(svn_mergeinfo_merge2(*added_mergeinfo,
+ svn_mergeinfo_dup(added, result_pool),
+ result_pool, iterpool));
break;
}
}
+ svn_pool_destroy(iterpool);
return SVN_NO_ERROR;
}
@@ -1739,8 +1740,8 @@ store_search(svn_mergeinfo_t processed,
apr_hash_set(mergeinfo, apr_pstrdup(processed_pool, path),
APR_HASH_KEY_STRING, ranges);
}
- SVN_ERR(svn_mergeinfo_merge(processed, mergeinfo,
- apr_hash_pool_get(processed)));
+ SVN_ERR(svn_mergeinfo_merge2(processed, mergeinfo,
+ apr_hash_pool_get(processed), scratch_pool));
return SVN_NO_ERROR;
}
@@ -2111,10 +2112,10 @@ get_paths_history_as_mergeinfo(svn_merge
SVN_ERR(svn_mergeinfo__mergeinfo_from_segments(
&path_history_mergeinfo, loc_seg_baton.history_segments, iterpool));
- SVN_ERR(svn_mergeinfo_merge(*paths_history_mergeinfo,
- svn_mergeinfo_dup(path_history_mergeinfo,
- result_pool),
- result_pool));
+ SVN_ERR(svn_mergeinfo_merge2(*paths_history_mergeinfo,
+ svn_mergeinfo_dup(path_history_mergeinfo,
+ result_pool),
+ result_pool, iterpool));
}
svn_pool_destroy(iterpool);
return SVN_NO_ERROR;
Modified: subversion/trunk/subversion/libsvn_subr/deprecated.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_subr/deprecated.c?rev=1175903&r1=1175902&r2=1175903&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_subr/deprecated.c (original)
+++ subversion/trunk/subversion/libsvn_subr/deprecated.c Mon Sep 26 15:15:21 2011
@@ -1135,3 +1135,12 @@ svn_rangelist_merge(apr_array_header_t *
return svn_error_trace(svn_rangelist_merge2(*rangelist, changes,
pool, pool));
}
+
+svn_error_t *
+svn_mergeinfo_merge(svn_mergeinfo_t mergeinfo,
+ svn_mergeinfo_t changes,
+ apr_pool_t *pool)
+{
+ return svn_error_trace(svn_mergeinfo_merge2(mergeinfo, changes, pool,
+ pool));
+}
Modified: subversion/trunk/subversion/libsvn_subr/mergeinfo.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_subr/mergeinfo.c?rev=1175903&r1=1175902&r2=1175903&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_subr/mergeinfo.c (original)
+++ subversion/trunk/subversion/libsvn_subr/mergeinfo.c Mon Sep 26 15:15:21 2011
@@ -1320,8 +1320,10 @@ svn_mergeinfo__equals(svn_boolean_t *is_
}
svn_error_t *
-svn_mergeinfo_merge(svn_mergeinfo_t mergeinfo, svn_mergeinfo_t changes,
- apr_pool_t *pool)
+svn_mergeinfo_merge2(svn_mergeinfo_t mergeinfo,
+ svn_mergeinfo_t changes,
+ apr_pool_t *result_pool,
+ apr_pool_t *scratch_pool)
{
apr_array_header_t *sorted1, *sorted2;
int i, j;
@@ -1330,12 +1332,14 @@ svn_mergeinfo_merge(svn_mergeinfo_t merg
if (!apr_hash_count(changes))
return SVN_NO_ERROR;
- sorted1 = svn_sort__hash(mergeinfo, svn_sort_compare_items_as_paths, pool);
- sorted2 = svn_sort__hash(changes, svn_sort_compare_items_as_paths, pool);
+ sorted1 = svn_sort__hash(mergeinfo, svn_sort_compare_items_as_paths,
+ scratch_pool);
+ sorted2 = svn_sort__hash(changes, svn_sort_compare_items_as_paths,
+ scratch_pool);
i = 0;
j = 0;
- iterpool = svn_pool_create(pool);
+ iterpool = svn_pool_create(scratch_pool);
while (i < sorted1->nelts && j < sorted2->nelts)
{
svn_sort__item_t elt1, elt2;
@@ -1354,7 +1358,7 @@ svn_mergeinfo_merge(svn_mergeinfo_t merg
rl1 = elt1.value;
rl2 = elt2.value;
- SVN_ERR(svn_rangelist_merge2(rl1, rl2, pool, iterpool));
+ SVN_ERR(svn_rangelist_merge2(rl1, rl2, result_pool, iterpool));
apr_hash_set(mergeinfo, elt1.key, elt1.klen, rl1);
i++;
j++;
@@ -1410,8 +1414,8 @@ svn_mergeinfo_catalog_merge(svn_mergeinf
svn_mergeinfo_t mergeinfo = cat_elt.value;
svn_mergeinfo_t changes_mergeinfo = change_elt.value;
- SVN_ERR(svn_mergeinfo_merge(mergeinfo, changes_mergeinfo,
- result_pool));
+ SVN_ERR(svn_mergeinfo_merge2(mergeinfo, changes_mergeinfo,
+ result_pool, scratch_pool));
apr_hash_set(mergeinfo_cat, cat_elt.key, cat_elt.klen, mergeinfo);
i++;
j++;
Re: svn commit: r1175903 - in /subversion/trunk/subversion:
include/svn_mergeinfo.h libsvn_client/merge.c libsvn_repos/log.c
libsvn_subr/deprecated.c libsvn_subr/mergeinfo.c
Posted by Stefan Sperling <st...@elego.de>.
On Wed, Sep 28, 2011 at 06:16:18PM +0200, Stefan Sperling wrote:
> On Mon, Sep 26, 2011 at 03:15:21PM -0000, pburba@apache.org wrote:
> > Author: pburba
> > Date: Mon Sep 26 15:15:21 2011
> > New Revision: 1175903
> > URL: http://svn.apache.org/viewvc?rev=1175903&view=rev
> > Log:
> > Rev svn_mergeinfo_merge API to use the two-pool paradigm.
>
> > * subversion/libsvn_subr/mergeinfo.c
> >
> > (svn_mergeinfo_merge2): New.
>
> Hi Paul,
>
> I think this commit introduced a pool lifetime issue. See below.
Oh, nevermind. Looking closer at the implementation of svn_sort__hash(),
the key pointer in the temporary array has the same value as the key
pointer in the hash table. So this should be fine.
Re: svn commit: r1175903 - in /subversion/trunk/subversion:
include/svn_mergeinfo.h libsvn_client/merge.c libsvn_repos/log.c
libsvn_subr/deprecated.c libsvn_subr/mergeinfo.c
Posted by Stefan Sperling <st...@elego.de>.
On Mon, Sep 26, 2011 at 03:15:21PM -0000, pburba@apache.org wrote:
> Author: pburba
> Date: Mon Sep 26 15:15:21 2011
> New Revision: 1175903
> URL: http://svn.apache.org/viewvc?rev=1175903&view=rev
> Log:
> Rev svn_mergeinfo_merge API to use the two-pool paradigm.
> * subversion/libsvn_subr/mergeinfo.c
>
> (svn_mergeinfo_merge2): New.
Hi Paul,
I think this commit introduced a pool lifetime issue. See below.
> ==============================================================================
> --- subversion/trunk/subversion/libsvn_subr/mergeinfo.c (original)
> +++ subversion/trunk/subversion/libsvn_subr/mergeinfo.c Mon Sep 26 15:15:21 2011
> @@ -1320,8 +1320,10 @@ svn_mergeinfo__equals(svn_boolean_t *is_
> }
>
> svn_error_t *
> -svn_mergeinfo_merge(svn_mergeinfo_t mergeinfo, svn_mergeinfo_t changes,
> - apr_pool_t *pool)
> +svn_mergeinfo_merge2(svn_mergeinfo_t mergeinfo,
> + svn_mergeinfo_t changes,
> + apr_pool_t *result_pool,
> + apr_pool_t *scratch_pool)
> {
> apr_array_header_t *sorted1, *sorted2;
> int i, j;
> @@ -1330,12 +1332,14 @@ svn_mergeinfo_merge(svn_mergeinfo_t merg
> if (!apr_hash_count(changes))
> return SVN_NO_ERROR;
>
> - sorted1 = svn_sort__hash(mergeinfo, svn_sort_compare_items_as_paths, pool);
> - sorted2 = svn_sort__hash(changes, svn_sort_compare_items_as_paths, pool);
> + sorted1 = svn_sort__hash(mergeinfo, svn_sort_compare_items_as_paths,
> + scratch_pool);
> + sorted2 = svn_sort__hash(changes, svn_sort_compare_items_as_paths,
> + scratch_pool);
Here, a temporary array of sorted keys is allocated in the scratch pool.
>
> i = 0;
> j = 0;
> - iterpool = svn_pool_create(pool);
> + iterpool = svn_pool_create(scratch_pool);
> while (i < sorted1->nelts && j < sorted2->nelts)
> {
> svn_sort__item_t elt1, elt2;
> @@ -1354,7 +1358,7 @@ svn_mergeinfo_merge(svn_mergeinfo_t merg
> rl1 = elt1.value;
> rl2 = elt2.value;
elt1 and elt2 are elements in the temporary array.
>
> - SVN_ERR(svn_rangelist_merge2(rl1, rl2, pool, iterpool));
> + SVN_ERR(svn_rangelist_merge2(rl1, rl2, result_pool, iterpool));
> apr_hash_set(mergeinfo, elt1.key, elt1.klen, rl1);
This sets the key pointer in the mergeinfo hash to data from elt1.
But the key has less lifetime than the hash itself, so this isn't safe.
Instead, the keys should be allocated in the same pool the mergeinfo
hash was allocated in.
Before your commit, they keys were stored in 'pool' which was treated
as a result pool.
The temporary keys are also used elsewhere in this function in the same
unsafe way.
I would suggest to aprpstr_dup() keys into the mergeinfo hash's pool.