You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Madan U Sreenivasan <ma...@collab.net> on 2006/10/09 10:00:59 UTC
[PATCH][MERGE-TRACKING] New function to create a mergeinfo hash out of a single path and single merge-range
Hi,
As per http://svn.haxx.se/dev/archive-2006-09/0975.shtml, please find
below an internal function that makes a mergeinfo hash out of a single
path and a single merge-range list.
I have gone the svn_sort__hash() way of adding a '__' function in a
header file. The other option would have been to create a libsvn_subr
local header file (ala utf_impl.h), but I feel this is an overkill for a
single function.
Comments suggestions welcome.
Thanks to Kamesh for pointing out the two different ways to make
internal APIs.
[[[
Abstract construction of a mergeinfo of a single path with single
merge-range
into a separate function.
On the merge-tracking branch:
Patch By: dlr
madanus
* subversion/include/svn_mergeinfo.h
(svn_mergeinfo__make): New.
* subversion/libsvn_subr/mergeinfo.c
(svn_mergeinfo__make): New function to create a mergeinfo, given a single
path and a single merge-range.
]]]
Regards,
Madan.
Re: [PATCH][MERGE-TRACKING] New function to create a mergeinfo hash
out of a single path and single merge-range
Posted by Kamesh Jayachandran <ka...@collab.net>.
> Current usage -- at least in libsvn_client's merging code, and some of
> mergeinfo.c -- has primarily been to either stack-allocate the
> svn_merge_range_t (using syntax like "r = {3, 7};"), or call
> svn_merge_range_dup().
>
>
Have seen it.
Atlast could find heap allocated 'svn_merge_range_t' in
subversion/libsvn_fs_util/merge_info_index.c :parse_mergeinfo_from_db.
With regards
Kamesh Jayachandran
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH][MERGE-TRACKING] New function to create a mergeinfo hash out of a single path and single merge-range
Posted by Daniel Rall <dl...@collab.net>.
On Wed, 11 Oct 2006, Kamesh Jayachandran wrote:
> Daniel Rall wrote:
> >On Tue, 10 Oct 2006, Kamesh Jayachandran wrote:
> >
> >
> >>I would see it would be more useful to have the following,
> >>svn_range_create(start, end) which would have more use than this combo
> >>(svn_mergeinfo__make).
> >>
> >
> >An svn_merge_range_t only contains two field, start and end (see
> >svn_types.h). Is there really enough value in adding a constructor
> >function for such a simple data type? (We already have an
> >svn_range_dup() API).
>
> Sorry I was under the opinion each place we create 'svn_mergerange_t *'
> we have some sequence of alloc mem, set start, set end. This I thought
> could be replaced with one utility function. Unfortunately I could not
> find one :).
Current usage -- at least in libsvn_client's merging code, and some of
mergeinfo.c -- has primarily been to either stack-allocate the
svn_merge_range_t (using syntax like "r = {3, 7};"), or call
svn_merge_range_dup().
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH][MERGE-TRACKING] New function to create a mergeinfo hash
out of a single path and single merge-range
Posted by Kamesh Jayachandran <ka...@collab.net>.
Daniel Rall wrote:
> On Tue, 10 Oct 2006, Kamesh Jayachandran wrote:
>
>
>> I would see it would be more useful to have the following,
>> svn_range_create(start, end) which would have more use than this combo
>> (svn_mergeinfo__make).
>>
>
> An svn_merge_range_t only contains two field, start and end (see
> svn_types.h). Is there really enough value in adding a constructor
> function for such a simple data type? (We already have an
> svn_range_dup() API).
>
Sorry I was under the opinion each place we create 'svn_mergerange_t *'
we have some sequence of alloc mem, set start, set end. This I thought
could be replaced with one utility function. Unfortunately I could not
find one :).
With regards
Kamesh Jayachandran
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH][MERGE-TRACKING] New function to create a mergeinfo hash out of a single path and single merge-range
Posted by Daniel Rall <dl...@collab.net>.
On Tue, 10 Oct 2006, Kamesh Jayachandran wrote:
> I would see it would be more useful to have the following,
> svn_range_create(start, end) which would have more use than this combo
> (svn_mergeinfo__make).
An svn_merge_range_t only contains two field, start and end (see
svn_types.h). Is there really enough value in adding a constructor
function for such a simple data type? (We already have an
svn_range_dup() API).
I was thinking of adding a svn_rangelist_create(start, end, pool) API,
which would add one range to a list, providing part of the
implementation for svn_mergeinfo__make(start, end, pool). However,
I'd be most influenced by a patch which shows where we'd actually use
these APIs.
Re: [PATCH][MERGE-TRACKING] New function to create a mergeinfo hash
out of a single path and single merge-range
Posted by Kamesh Jayachandran <ka...@collab.net>.
I would see it would be more useful to have the following,
svn_range_create(start, end) which would have more use than this combo
(svn_mergeinfo__make).
With regards
Kamesh Jayachandran
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH][MERGE-TRACKING] New function to create a mergeinfo hash out of a single path and single merge-range
Posted by Daniel Rall <dl...@collab.net>.
On Tue, 24 Oct 2006, Madan S. wrote:
> On Mon, 09 Oct 2006 15:30:59 +0530, Madan U Sreenivasan <ma...@collab.net>
> wrote:
>
> >Hi,
> >
> > As per http://svn.haxx.se/dev/archive-2006-09/0975.shtml, please find
> >below an internal function that makes a mergeinfo hash out of a single
> >path and a single merge-range list.
> >
> > I have gone the svn_sort__hash() way of adding a '__' function in a
> >header file. The other option would have been to create a libsvn_subr
> >local header file (ala utf_impl.h), but I feel this is an overkill for a
> >single function.
>
> I haven't heard any serious reservations against this patch. But this
> hasnt been committed yet. So, this mail is just a reminder. I need this to
> complete my repos_to_repos_copy() stuff on the merge_tracking branch
Before committing a change like this, I'd like to see a patch putting
it to use everywhere possible to make sure it's worth adding to our
internal API.
Re: [PATCH][MERGE-TRACKING] New function to create a mergeinfo hash out of a single path and single merge-range
Posted by Madan U Sreenivasan <ma...@collab.net>.
On Mon, 09 Oct 2006 15:30:59 +0530, Madan U Sreenivasan <ma...@collab.net>
wrote:
> Hi,
>
> As per http://svn.haxx.se/dev/archive-2006-09/0975.shtml, please find
> below an internal function that makes a mergeinfo hash out of a single
> path and a single merge-range list.
>
> I have gone the svn_sort__hash() way of adding a '__' function in a
> header file. The other option would have been to create a libsvn_subr
> local header file (ala utf_impl.h), but I feel this is an overkill for a
> single function.
I haven't heard any serious reservations against this patch. But this
hasnt been committed yet. So, this mail is just a reminder. I need this to
complete my repos_to_repos_copy() stuff on the merge_tracking branch
Regards,
Madan.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH][MERGE-TRACKING] New function to create a mergeinfo hash
out of a single path and single merge-range
Posted by Kamesh Jayachandran <ka...@collab.net>.
Malcolm Rowe wrote:
> On Tue, Oct 10, 2006 at 02:48:47PM +0530, Kamesh Jayachandran wrote:
>
>>> + svn_merge_range_t *range = apr_palloc(pool, sizeof(*range));
>>>
>>>
>> I feel
>>
>> - svn_merge_range_t *range = apr_palloc(pool, sizeof(*range));
>> + svn_merge_range_t *range = apr_palloc(pool, sizeof(svn_merge_range_t));
>>
>> more clearer.
>>
>>
>
> We already use the former pattern pretty-much exclusively. It has the
> advantage that you only need to give the type once, so that the
> allocation can never get out of step with the definition. For example,
> if we were to create an svn_merge_range2_t type, we might change the
> definition of 'range' but forget to change the argument to sizeof().
>
> Regards,
> Malcolm
>
Makes sense, When I looked at it I was wondering from where the 'range'
comes from,(I first looked at args, I could not find one, then happened
to find it at left hand side.)
With regards
Kamesh Jayachandran
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH][MERGE-TRACKING] New function to create a mergeinfo hash out of a single path and single merge-range
Posted by Malcolm Rowe <ma...@farside.org.uk>.
On Tue, Oct 10, 2006 at 02:48:47PM +0530, Kamesh Jayachandran wrote:
> >+ svn_merge_range_t *range = apr_palloc(pool, sizeof(*range));
> >
> I feel
>
> - svn_merge_range_t *range = apr_palloc(pool, sizeof(*range));
> + svn_merge_range_t *range = apr_palloc(pool, sizeof(svn_merge_range_t));
>
> more clearer.
>
We already use the former pattern pretty-much exclusively. It has the
advantage that you only need to give the type once, so that the
allocation can never get out of step with the definition. For example,
if we were to create an svn_merge_range2_t type, we might change the
definition of 'range' but forget to change the argument to sizeof().
Regards,
Malcolm
Re: [PATCH][MERGE-TRACKING] New function to create a mergeinfo hash out of a single path and single merge-range
Posted by Daniel Rall <dl...@collab.net>.
On Tue, 10 Oct 2006, Kamesh Jayachandran wrote:
>
> >[[[
> >Abstract construction of a mergeinfo of a single path with single
> >merge-range
> >into a separate function.
> >
> >On the merge-tracking branch:
> >Patch By: dlr
> > madanus
> >
> >* subversion/include/svn_mergeinfo.h
> > (svn_mergeinfo__make): New.
> Somehow I feel not having '__', if it is '__' the function should be on
> a private header file.
> May be 'svn_mergeinfo__make' -> 'svn_mergeinfo_make'?
We already have double-underscore (private) functions in our private
header files. If there were an appropriate private header, I would
prefer to add private APIs there. Without an appropriate spot, I'm
okay adding them to the usual headers. *shrug*
Re: [PATCH][MERGE-TRACKING] New function to create a mergeinfo hash
out of a single path and single merge-range
Posted by Kamesh Jayachandran <ka...@collab.net>.
> [[[
> Abstract construction of a mergeinfo of a single path with single
> merge-range
> into a separate function.
>
> On the merge-tracking branch:
> Patch By: dlr
> madanus
>
> * subversion/include/svn_mergeinfo.h
> (svn_mergeinfo__make): New.
Somehow I feel not having '__', if it is '__' the function should be on
a private header file.
May be 'svn_mergeinfo__make' -> 'svn_mergeinfo_make'?
> ------------------------------------------------------------------------
>
> Index: subversion/include/svn_mergeinfo.h
> ===================================================================
> --- subversion/include/svn_mergeinfo.h (revision 21841)
> +++ subversion/include/svn_mergeinfo.h (working copy)
> @@ -47,6 +47,13 @@
> svn_mergeinfo_parse(const char *input, apr_hash_t **mergehash,
> apr_pool_t *pool);
>
> +/** Make a mergeinfo hash for the given @a path, @a start and @a end
> + * revisions.
> + */
> +apr_hash_t *
> +svn_mergeinfo__make(const char *path, svn_revnum_t start, svn_revnum_t end,
> + apr_pool_t *pool);
> +
> /** Calculate the delta between two hashes of merge info, @a mergefrom
> * and @a mergeto, and place the result in @a deleted and @a added
> * (neither output argument will ever be @c NULL), stored as the usual
> Index: subversion/libsvn_subr/mergeinfo.c
> ===================================================================
> --- subversion/libsvn_subr/mergeinfo.c (revision 21841)
> +++ subversion/libsvn_subr/mergeinfo.c (working copy)
> @@ -224,7 +224,24 @@
> return parse_top(&input, input + strlen(input), *hash, pool);
> }
>
> +apr_hash_t *
> +svn_mergeinfo__make(const char *path, svn_revnum_t start, svn_revnum_t end,
> + apr_pool_t *pool)
> +{
> + apr_hash_t *mergeinfo;
> + svn_merge_range_t *range = apr_palloc(pool, sizeof(*range));
>
I feel
- svn_merge_range_t *range = apr_palloc(pool, sizeof(*range));
+svn_merge_range_t *range = apr_palloc(pool, sizeof(svn_merge_range_t));
more clearer.
> + apr_array_header_t *rangelist = apr_array_make(pool, 1, sizeof(range));
>
> + range->start = start;
> + range->end = end;
> +
> + APR_ARRAY_PUSH(rangelist, svn_merge_range_t *) = range;
> + mergeinfo = apr_hash_make(pool);
> + apr_hash_set(mergeinfo, path, sizeof(rangelist), rangelist);
>
Again!
- apr_hash_set(mergeinfo, path, sizeof(rangelist), rangelist);
+ apr_hash_set(mergeinfo, path, APR_HASH_KEY_STRING, rangelist);
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org