You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by ju...@apache.org on 2010/11/22 16:08:26 UTC
svn commit: r1037748 - in /subversion/trunk/subversion:
include/private/svn_mergeinfo_private.h libsvn_subr/mergeinfo.c
Author: julianfoad
Date: Mon Nov 22 15:08:25 2010
New Revision: 1037748
URL: http://svn.apache.org/viewvc?rev=1037748&view=rev
Log:
Simplify and remove ambiguity in a mergeinfo function.
* subversion/include/private/svn_mergeinfo_private.h,
subversion/libsvn_subr/mergeinfo.c
(svn_mergeinfo__add_suffix_to_mergeinfo): Require that the suffix is a
canonical relpath.
Modified:
subversion/trunk/subversion/include/private/svn_mergeinfo_private.h
subversion/trunk/subversion/libsvn_subr/mergeinfo.c
Modified: subversion/trunk/subversion/include/private/svn_mergeinfo_private.h
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/include/private/svn_mergeinfo_private.h?rev=1037748&r1=1037747&r2=1037748&view=diff
==============================================================================
--- subversion/trunk/subversion/include/private/svn_mergeinfo_private.h (original)
+++ subversion/trunk/subversion/include/private/svn_mergeinfo_private.h Mon Nov 22 15:08:25 2010
@@ -112,14 +112,15 @@ svn_mergeinfo__add_prefix_to_catalog(svn
apr_pool_t *result_pool,
apr_pool_t *scratch_pool);
-/* Makes a deep copy of MERGEINFO in *OUT_MERGEINFO. If SUFFIX_REL_PATH is
- a valid relative path then add it to the end of each key path in
- *OUT_MERGEINFO. *OUT_MERGEINFO is allocated in RESULT_POOL. SCRATCH_POOL
- is used for any temporary allocations. */
+/* Set *OUT_MERGEINFO to a deep copy of MERGEINFO with the relpath
+ SUFFIX_RELPATH added to the end of each key path.
+
+ Allocate *OUT_MERGEINFO in RESULT_POOL. Use SCRATCH_POOL for any
+ temporary allocations. */
svn_error_t *
svn_mergeinfo__add_suffix_to_mergeinfo(svn_mergeinfo_t *out_mergeinfo,
svn_mergeinfo_t mergeinfo,
- const char *suffix,
+ const char *suffix_relpath,
apr_pool_t *result_pool,
apr_pool_t *scratch_pool);
Modified: subversion/trunk/subversion/libsvn_subr/mergeinfo.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_subr/mergeinfo.c?rev=1037748&r1=1037747&r2=1037748&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_subr/mergeinfo.c (original)
+++ subversion/trunk/subversion/libsvn_subr/mergeinfo.c Mon Nov 22 15:08:25 2010
@@ -1809,33 +1809,28 @@ svn_mergeinfo__add_prefix_to_catalog(svn
svn_error_t *
svn_mergeinfo__add_suffix_to_mergeinfo(svn_mergeinfo_t *out_mergeinfo,
svn_mergeinfo_t mergeinfo,
- const char *suffix,
+ const char *suffix_relpath,
apr_pool_t *result_pool,
apr_pool_t *scratch_pool)
{
- if (!suffix || svn_dirent_is_absolute(suffix))
- {
- *out_mergeinfo = svn_mergeinfo_dup(mergeinfo, result_pool);
- }
- else
- {
- apr_hash_index_t *hi;
- const char *canonical_suffix = svn_uri_canonicalize(suffix,
- scratch_pool);
- *out_mergeinfo = apr_hash_make(result_pool);
+ apr_hash_index_t *hi;
- for (hi = apr_hash_first(scratch_pool, mergeinfo);
- hi;
- hi = apr_hash_next(hi))
- {
- const char *path = svn__apr_hash_index_key(hi);
- apr_array_header_t *rangelist = svn__apr_hash_index_val(hi);
+ SVN_ERR_ASSERT(suffix_relpath && svn_relpath_is_canonical(suffix_relpath,
+ scratch_pool));
- apr_hash_set(*out_mergeinfo,
- svn_dirent_join(path, canonical_suffix, result_pool),
- APR_HASH_KEY_STRING,
- svn_rangelist_dup(rangelist, result_pool));
- }
+ *out_mergeinfo = apr_hash_make(result_pool);
+
+ for (hi = apr_hash_first(scratch_pool, mergeinfo);
+ hi;
+ hi = apr_hash_next(hi))
+ {
+ const char *path = svn__apr_hash_index_key(hi);
+ apr_array_header_t *rangelist = svn__apr_hash_index_val(hi);
+
+ apr_hash_set(*out_mergeinfo,
+ svn_dirent_join(path, suffix_relpath, result_pool),
+ APR_HASH_KEY_STRING,
+ svn_rangelist_dup(rangelist, result_pool));
}
return SVN_NO_ERROR;
RE: svn commit: r1037748 - in /subversion/trunk/subversion:
include/private/svn_mergeinfo_private.h libsvn_subr/mergeinfo.c
Posted by Julian Foad <ju...@wandisco.com>.
On Mon, 2010-11-22, Julian Foad wrote:
> On Mon, 2010-11-22, Bert Huijben wrote:
> > > svn_mergeinfo__add_suffix_to_mergeinfo(svn_mergeinfo_t *out_mergeinfo,
> > > svn_mergeinfo_t mergeinfo,
> > > - const char *suffix,
> > > + const char *suffix_relpath,
> > > apr_pool_t *result_pool,
> > > apr_pool_t *scratch_pool)
> > > {
> [...]
> > > - apr_hash_set(*out_mergeinfo,
> > > - svn_dirent_join(path, canonical_suffix,
> > > result_pool),
> > > - APR_HASH_KEY_STRING,
> > > - svn_rangelist_dup(rangelist, result_pool));
> > > - }
> [...]
> > > + apr_hash_set(*out_mergeinfo,
> > > + svn_dirent_join(path, suffix_relpath, result_pool),
> > > + APR_HASH_KEY_STRING,
> > > + svn_rangelist_dup(rangelist, result_pool));
> >
> > Path is not a dirent (a local disk path), so this should not use the dirent API.
> >
> > I think this should use the new svn_fspath__ join. (Merge info contains a '/')
>
> Yes - that's why I'm looking at this function! (Note: I didn't touch
> that part of it in this commit, just the indentation changed.)
Thinking back, that was a lie. I was looking at the function because it
contained "svn_uri_canonicalize"; I only noticed the two uses of
dirent_* when I went to fix that. And I might not have remembered to
revisit the remaining dirent_join if you hadn't said this, so thank you.
After seeing this, I might go and search for other bad uses of
svn_dirent_* as well, at some point.
- Julian
RE: svn commit: r1037748 - in /subversion/trunk/subversion:
include/private/svn_mergeinfo_private.h libsvn_subr/mergeinfo.c
Posted by Julian Foad <ju...@wandisco.com>.
On Mon, 2010-11-22, Bert Huijben wrote:
> > svn_mergeinfo__add_suffix_to_mergeinfo(svn_mergeinfo_t *out_mergeinfo,
> > svn_mergeinfo_t mergeinfo,
> > - const char *suffix,
> > + const char *suffix_relpath,
> > apr_pool_t *result_pool,
> > apr_pool_t *scratch_pool)
> > {
[...]
> > - apr_hash_set(*out_mergeinfo,
> > - svn_dirent_join(path, canonical_suffix,
> > result_pool),
> > - APR_HASH_KEY_STRING,
> > - svn_rangelist_dup(rangelist, result_pool));
> > - }
[...]
> > + apr_hash_set(*out_mergeinfo,
> > + svn_dirent_join(path, suffix_relpath, result_pool),
> > + APR_HASH_KEY_STRING,
> > + svn_rangelist_dup(rangelist, result_pool));
>
> Path is not a dirent (a local disk path), so this should not use the dirent API.
>
> I think this should use the new svn_fspath__ join. (Merge info contains a '/')
Yes - that's why I'm looking at this function! (Note: I didn't touch
that part of it in this commit, just the indentation changed.)
- Julian
RE: svn commit: r1037748 - in /subversion/trunk/subversion: include/private/svn_mergeinfo_private.h libsvn_subr/mergeinfo.c
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: julianfoad@apache.org [mailto:julianfoad@apache.org]
> Sent: maandag 22 november 2010 16:08
> To: commits@subversion.apache.org
> Subject: svn commit: r1037748 - in /subversion/trunk/subversion:
> include/private/svn_mergeinfo_private.h libsvn_subr/mergeinfo.c
>
> Author: julianfoad
> Date: Mon Nov 22 15:08:25 2010
> New Revision: 1037748
>
> URL: http://svn.apache.org/viewvc?rev=1037748&view=rev
> Log:
> Simplify and remove ambiguity in a mergeinfo function.
>
> * subversion/include/private/svn_mergeinfo_private.h,
> subversion/libsvn_subr/mergeinfo.c
> (svn_mergeinfo__add_suffix_to_mergeinfo): Require that the suffix is
> a
> canonical relpath.
>
> Modified:
> subversion/trunk/subversion/include/private/svn_mergeinfo_private.h
> subversion/trunk/subversion/libsvn_subr/mergeinfo.c
>
<snip>
> Modified: subversion/trunk/subversion/libsvn_subr/mergeinfo.c
> URL:
> http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_subr/me
> rgeinfo.c?rev=1037748&r1=1037747&r2=1037748&view=diff
> =======================================================================
> =======
> --- subversion/trunk/subversion/libsvn_subr/mergeinfo.c (original)
> +++ subversion/trunk/subversion/libsvn_subr/mergeinfo.c Mon Nov 22
> 15:08:25 2010
> @@ -1809,33 +1809,28 @@ svn_mergeinfo__add_prefix_to_catalog(svn
> svn_error_t *
> svn_mergeinfo__add_suffix_to_mergeinfo(svn_mergeinfo_t *out_mergeinfo,
> svn_mergeinfo_t mergeinfo,
> - const char *suffix,
> + const char *suffix_relpath,
> apr_pool_t *result_pool,
> apr_pool_t *scratch_pool)
> {
> - if (!suffix || svn_dirent_is_absolute(suffix))
> - {
> - *out_mergeinfo = svn_mergeinfo_dup(mergeinfo, result_pool);
> - }
> - else
> - {
> - apr_hash_index_t *hi;
> - const char *canonical_suffix = svn_uri_canonicalize(suffix,
> -
> scratch_pool);
> - *out_mergeinfo = apr_hash_make(result_pool);
> + apr_hash_index_t *hi;
>
> - for (hi = apr_hash_first(scratch_pool, mergeinfo);
> - hi;
> - hi = apr_hash_next(hi))
> - {
> - const char *path = svn__apr_hash_index_key(hi);
> - apr_array_header_t *rangelist = svn__apr_hash_index_val(hi);
> + SVN_ERR_ASSERT(suffix_relpath &&
> svn_relpath_is_canonical(suffix_relpath,
> +
> scratch_pool));
>
> - apr_hash_set(*out_mergeinfo,
> - svn_dirent_join(path, canonical_suffix,
> result_pool),
> - APR_HASH_KEY_STRING,
> - svn_rangelist_dup(rangelist, result_pool));
> - }
> + *out_mergeinfo = apr_hash_make(result_pool);
> +
> + for (hi = apr_hash_first(scratch_pool, mergeinfo);
> + hi;
> + hi = apr_hash_next(hi))
> + {
> + const char *path = svn__apr_hash_index_key(hi);
> + apr_array_header_t *rangelist = svn__apr_hash_index_val(hi);
> +
> + apr_hash_set(*out_mergeinfo,
> + svn_dirent_join(path, suffix_relpath, result_pool),
> + APR_HASH_KEY_STRING,
> + svn_rangelist_dup(rangelist, result_pool));
Path is not a dirent (a local disk path), so this should not use the dirent API.
I think this should use the new svn_fspath__ join. (Merge info contains a '/')
Bert