You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Bert Huijben <be...@qqmail.nl> on 2010/11/22 15:16:47 UTC
RE: svn commit: r1037748 - in /subversion/trunk/subversion: include/private/svn_mergeinfo_private.h libsvn_subr/mergeinfo.c
> -----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
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