You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Daniel Rall <dl...@collab.net> on 2007/05/26 01:34:54 UTC
Re: svn commit: r25140 - branches/merge-sensitive-log/subversion/libsvn_repos
On Thu, 24 May 2007, hwright@tigris.org wrote:
...
> Include merged revisions in the list of log messages returned by the
> repository, if requested.
>
> This patch, in my mind, is the crux of the merge-sensitive-log issue. It works
> for simple cases, and I'll be updating the command-line client shortly to
> use the new information returned from the repository.
...
> * subversion/libsvn_repos/log.c
> (get_combined_rangelist, get_merge_changed_rangelist, send_child_revs):
> New helper functions.
> (send_change_rev): If requested, check for merged revisions, and send them
> after sending the original revision.
> (svn_repos_get_logs4): Pass extra parameters fo send_change_rev().
...
> --- branches/merge-sensitive-log/subversion/libsvn_repos/log.c (original)
> +++ branches/merge-sensitive-log/subversion/libsvn_repos/log.c Thu May 24 22:19:34 2007
...
Should the doc string go here, with the function decl?
> +static svn_error_t *
> +send_change_rev(const apr_array_header_t *paths,
> + svn_revnum_t rev,
> + svn_fs_t *fs,
> + svn_boolean_t discover_changed_paths,
> + svn_boolean_t include_merged_revisions,
> + svn_repos_authz_func_t authz_read_func,
> + void *authz_read_baton,
> + svn_log_message_receiver2_t receiver,
> + void *receiver_baton,
> + apr_pool_t *pool);
...
> +/* Get a RANGELIST of the combined mergeinfo for PATHS at REV. */
Better as something like:
/* Return the combined rangelists for everyone merge info for the
PATHS tree at REV in *RANGELIST. Perform all allocations in
POOL. */
> +static svn_error_t *
> +get_combined_rangelist(apr_array_header_t **rangelist,
> + svn_fs_t *fs,
> + svn_revnum_t rev,
> + const apr_array_header_t *paths,
> + apr_pool_t *pool)
> +{
> + svn_fs_root_t *root;
> + apr_hash_index_t *hi;
> + apr_hash_t *mergeinfo;
> +
> + *rangelist = apr_array_make(pool, 1, sizeof(svn_merge_range_t *));
Might want to hold off on this alloc until after the following two
statements. Also, I use nelts of 0; even though APR will use 1 under
the covers, we're not leading the reader on with any indication that
we might have a clue as to how many there'll be.
> +
> + SVN_ERR(svn_fs_revision_root(&root, fs, rev, pool));
> + SVN_ERR(svn_fs_get_children_mergeinfo(&mergeinfo, root, paths, pool));
It might make sense to use a subpool for the above two statements,
since we eventually discard all the memory they alloc.
*rangelist = apr_array_make(pool, 0, sizeof(svn_merge_range_t *));
> +
> + for (hi = apr_hash_first(pool, mergeinfo); hi; hi = apr_hash_next(hi))
> + {
> + apr_hash_t *path_mergeinfo;
> + apr_hash_index_t *mhi;
Might as well still call this local var "hi". Seems to be the
convention in the Subversion code base for this type of loop variable.
> +
> + apr_hash_this(hi, NULL, NULL, (void *)&path_mergeinfo);
^
We typically put a space between cast and variable name.
apr_hash_this(hi, NULL, NULL, (void *) &path_mergeinfo);
> +
> + for (mhi = apr_hash_first(pool, path_mergeinfo); mhi;
> + mhi = apr_hash_next(mhi))
> + {
> + apr_array_header_t *path_rangelist;
> +
> + apr_hash_this(mhi, NULL, NULL, (void *)&path_rangelist);
> + SVN_ERR(svn_rangelist_merge(rangelist, path_rangelist, pool));
> + }
> + }
...and destroy the subpool here.
> +
> + return SVN_NO_ERROR;
> +}
> +
> +/* Determine all the revisions which were merged into PATHS in REV. Return
> + them as a new RANGELIST. */
in *RANGELIST, allocated from POOL. */
This API name sets off my spidey sense...no good alternate suggestion,
tho.
> +static svn_error_t *
> +get_merge_changed_rangelist(apr_array_header_t **rangelist,
> + svn_fs_t *fs,
> + const apr_array_header_t *paths,
> + svn_revnum_t rev,
> + apr_pool_t *pool)
> +{
> + apr_array_header_t *curr_rangelist, *prev_rangelist;
> + apr_array_header_t *deleted, *changed;
> + apr_pool_t *subpool;
> + int i;
> +
> + if (rev == 0)
> + {
> + *rangelist = apr_array_make(pool, 1, sizeof(svn_merge_range_t *));
Again, I suggest using 0 rather than 1.
> + return SVN_NO_ERROR;
> + }
> +
> + subpool = svn_pool_create(pool);
> +
> + SVN_ERR(get_combined_rangelist(&curr_rangelist, fs, rev, paths, subpool));
> + SVN_ERR(get_combined_rangelist(&prev_rangelist, fs, rev - 1, paths, subpool));
> +
> + SVN_ERR(svn_rangelist_diff(&deleted, &changed, prev_rangelist, curr_rangelist,
> + subpool));
What about rollbacks? Don't we need to do something meaningful with
"deleted"? (I think this means an API change.) That still needs to
go into the spec(s), too.
> + SVN_ERR(svn_rangelist_merge(&changed, deleted, subpool));
> +
> + *rangelist = svn_rangelist_dup(changed, pool);
> + svn_pool_destroy(subpool);
Nice subpool usage! "changed" could be potentially very large, so I'm
suspicious of having to deep-copy it, but I don't see a better way
than how you did it.
> +
> + return SVN_NO_ERROR;
> +}
> +
> +/* Same as send_change_rev(), only send all the revisions in RANGELIST. Also,
> + INCLUDE_MERGED_REVISIONS is assumed to be TRUE. */
"but" might be a better word choice than "only" -- I misread this doc
string until I read the implementation. Then again, that might just
be me...
> +static svn_error_t *
> +send_child_revs(const apr_array_header_t *paths,
> + const apr_array_header_t *rangelist,
> + svn_fs_t *fs,
> + svn_boolean_t discover_changed_paths,
> + svn_repos_authz_func_t authz_read_func,
> + void *authz_read_baton,
> + svn_log_message_receiver2_t receiver,
> + void *receiver_baton,
> + apr_pool_t *pool)
> +{
> + apr_array_header_t *revs;
> + int i;
> +
I would definitely use a subpool here, for both the subsequent call,
and the loop. "revs" could potentially be a very large list.
> + SVN_ERR(svn_rangelist_to_revs(&revs, rangelist, pool));
> +
> + for (i = 0; i < revs->nelts; i++)
> + {
> + svn_revnum_t rev = APR_ARRAY_IDX(revs, i, svn_revnum_t);
> +
> + SVN_ERR(send_change_rev(paths, rev, fs, discover_changed_paths, TRUE,
> + authz_read_func, authz_read_baton,
> + receiver, receiver_baton, pool));
> + }
> +
> + return SVN_NO_ERROR;
> +}
...
> @@ -506,11 +620,16 @@
> *
> * The detect_changed function is used if either AUTHZ_READ_FUNC is
> * not NULL, or if DISCOVER_CHANGED_PATHS is TRUE. See it for details.
> + *
> + * If INCLUDE_MERGED_REVISIONS is TRUE, also pass history information to
> + * RECEIVER for any revisions which were merged in a result of REV.
> */
> static svn_error_t *
> -send_change_rev(svn_revnum_t rev,
> +send_change_rev(const apr_array_header_t *paths,
> + svn_revnum_t rev,
> svn_fs_t *fs,
> svn_boolean_t discover_changed_paths,
> + svn_boolean_t include_merged_revisions,
> svn_repos_authz_func_t authz_read_func,
> void *authz_read_baton,
> svn_log_message_receiver2_t receiver,
> @@ -520,6 +639,8 @@
> svn_string_t *author, *date, *message;
> apr_hash_t *r_props, *changed_paths = NULL;
> svn_log_entry_t *log_entry;
> + apr_uint64_t nbr_children = 0;
> + apr_array_header_t *rangelist;
...
> + /* Check to see if we need to include any extra merged revisions. */
> + if (include_merged_revisions)
> + {
> + SVN_ERR(get_merge_changed_rangelist(&rangelist, fs, paths, rev, pool));
> +
> + nbr_children = svn_rangelist_count_revs(rangelist);
> + }
> +
> log_entry = svn_log_entry_create(pool);
>
> log_entry->changed_paths = changed_paths;
> @@ -578,11 +707,17 @@
> log_entry->author = author ? author->data : NULL;
> log_entry->date = date ? date->data : NULL;
> log_entry->message = message ? message->data : NULL;
> + log_entry->nbr_children = nbr_children;
>
> SVN_ERR((*receiver)(receiver_baton,
> log_entry,
> pool));
>
> + if (nbr_children > 0)
> + SVN_ERR(send_child_revs(paths, rangelist, fs, discover_changed_paths,
> + authz_read_func, authz_read_baton, receiver,
> + receiver_baton, pool));
> +
> return SVN_NO_ERROR;
> }
...
Re: svn commit: r25140 - branches/merge-sensitive-log/subversion/libsvn_repos
Posted by Daniel Rall <dl...@collab.net>.
On Wed, 30 May 2007, Hyrum K. Wright wrote:
> Dan,
> Thanks for the review! I implemented most of your suggestions in
> r25220. A couple of comments below.
...
> Daniel Rall wrote:
...
> >> + for (hi = apr_hash_first(pool, mergeinfo); hi; hi = apr_hash_next(hi))
> >> + {
> >> + apr_hash_t *path_mergeinfo;
> >> + apr_hash_index_t *mhi;
> >
> > Might as well still call this local var "hi". Seems to be the
> > convention in the Subversion code base for this type of loop variable.
>
> We're already using the local variable "hi" for the outer loop, and I
> don't think that it'd be wise to shadow it. There may be a better name
> than "mhi", though.
Didn't notice that. "mhi" sounds fine to me for a loop variable.
...
> >> +/* Determine all the revisions which were merged into PATHS in REV. Return
> >> + them as a new RANGELIST. */
> >
> > in *RANGELIST, allocated from POOL. */
> >
> > This API name sets off my spidey sense...no good alternate suggestion,
> > tho.
>
> What about get_merged_rev_ranglist() ?
Yeah, I like that suggestion!
> >> +static svn_error_t *
> >> +get_merge_changed_rangelist(apr_array_header_t **rangelist,
> >> + svn_fs_t *fs,
> >> + const apr_array_header_t *paths,
> >> + svn_revnum_t rev,
> >> + apr_pool_t *pool)
...
> >> + SVN_ERR(get_combined_rangelist(&curr_rangelist, fs, rev, paths, subpool));
> >> + SVN_ERR(get_combined_rangelist(&prev_rangelist, fs, rev - 1, paths, subpool));
> >> +
> >> + SVN_ERR(svn_rangelist_diff(&deleted, &changed, prev_rangelist, curr_rangelist,
> >> + subpool));
> >
> > What about rollbacks? Don't we need to do something meaningful with
> > "deleted"? (I think this means an API change.) That still needs to
> > go into the spec(s), too.
>
> The next line merges the "deleted" and "changed" rangelists, so both
> should be included in the final list of revisions.
>
> >> + SVN_ERR(svn_rangelist_merge(&changed, deleted, subpool));
Ah, duh...'k.
Re: svn commit: r25140 - branches/merge-sensitive-log/subversion/libsvn_repos
Posted by "Hyrum K. Wright" <hy...@mail.utexas.edu>.
Dan,
Thanks for the review! I implemented most of your suggestions in
r25220. A couple of comments below.
Daniel Rall wrote:
> On Thu, 24 May 2007, hwright@tigris.org wrote:
> ...
>> Include merged revisions in the list of log messages returned by the
>> repository, if requested.
>>
>> This patch, in my mind, is the crux of the merge-sensitive-log issue. It works
>> for simple cases, and I'll be updating the command-line client shortly to
>> use the new information returned from the repository.
> ...
>> * subversion/libsvn_repos/log.c
>> (get_combined_rangelist, get_merge_changed_rangelist, send_child_revs):
>> New helper functions.
>> (send_change_rev): If requested, check for merged revisions, and send them
>> after sending the original revision.
>> (svn_repos_get_logs4): Pass extra parameters fo send_change_rev().
> ...
>> --- branches/merge-sensitive-log/subversion/libsvn_repos/log.c (original)
>> +++ branches/merge-sensitive-log/subversion/libsvn_repos/log.c Thu May 24 22:19:34 2007
> ...
>
> Should the doc string go here, with the function decl?
>
>> +static svn_error_t *
>> +send_change_rev(const apr_array_header_t *paths,
>> + svn_revnum_t rev,
>> + svn_fs_t *fs,
>> + svn_boolean_t discover_changed_paths,
>> + svn_boolean_t include_merged_revisions,
>> + svn_repos_authz_func_t authz_read_func,
>> + void *authz_read_baton,
>> + svn_log_message_receiver2_t receiver,
>> + void *receiver_baton,
>> + apr_pool_t *pool);
> ...
>> +/* Get a RANGELIST of the combined mergeinfo for PATHS at REV. */
>
> Better as something like:
>
> /* Return the combined rangelists for everyone merge info for the
> PATHS tree at REV in *RANGELIST. Perform all allocations in
> POOL. */
>
>> +static svn_error_t *
>> +get_combined_rangelist(apr_array_header_t **rangelist,
>> + svn_fs_t *fs,
>> + svn_revnum_t rev,
>> + const apr_array_header_t *paths,
>> + apr_pool_t *pool)
>> +{
>> + svn_fs_root_t *root;
>> + apr_hash_index_t *hi;
>> + apr_hash_t *mergeinfo;
>> +
>> + *rangelist = apr_array_make(pool, 1, sizeof(svn_merge_range_t *));
>
> Might want to hold off on this alloc until after the following two
> statements. Also, I use nelts of 0; even though APR will use 1 under
> the covers, we're not leading the reader on with any indication that
> we might have a clue as to how many there'll be.
>
>> +
>> + SVN_ERR(svn_fs_revision_root(&root, fs, rev, pool));
>> + SVN_ERR(svn_fs_get_children_mergeinfo(&mergeinfo, root, paths, pool));
>
> It might make sense to use a subpool for the above two statements,
> since we eventually discard all the memory they alloc.
>
> *rangelist = apr_array_make(pool, 0, sizeof(svn_merge_range_t *));
>
>> +
>> + for (hi = apr_hash_first(pool, mergeinfo); hi; hi = apr_hash_next(hi))
>> + {
>> + apr_hash_t *path_mergeinfo;
>> + apr_hash_index_t *mhi;
>
> Might as well still call this local var "hi". Seems to be the
> convention in the Subversion code base for this type of loop variable.
We're already using the local variable "hi" for the outer loop, and I
don't think that it'd be wise to shadow it. There may be a better name
than "mhi", though.
>> +
>> + apr_hash_this(hi, NULL, NULL, (void *)&path_mergeinfo);
> ^
> We typically put a space between cast and variable name.
>
> apr_hash_this(hi, NULL, NULL, (void *) &path_mergeinfo);
>
>> +
>> + for (mhi = apr_hash_first(pool, path_mergeinfo); mhi;
>> + mhi = apr_hash_next(mhi))
>> + {
>> + apr_array_header_t *path_rangelist;
>> +
>> + apr_hash_this(mhi, NULL, NULL, (void *)&path_rangelist);
>> + SVN_ERR(svn_rangelist_merge(rangelist, path_rangelist, pool));
>> + }
>> + }
>
> ...and destroy the subpool here.
>
>> +
>> + return SVN_NO_ERROR;
>> +}
>> +
>> +/* Determine all the revisions which were merged into PATHS in REV. Return
>> + them as a new RANGELIST. */
>
> in *RANGELIST, allocated from POOL. */
>
> This API name sets off my spidey sense...no good alternate suggestion,
> tho.
What about get_merged_rev_ranglist() ?
>> +static svn_error_t *
>> +get_merge_changed_rangelist(apr_array_header_t **rangelist,
>> + svn_fs_t *fs,
>> + const apr_array_header_t *paths,
>> + svn_revnum_t rev,
>> + apr_pool_t *pool)
>> +{
>> + apr_array_header_t *curr_rangelist, *prev_rangelist;
>> + apr_array_header_t *deleted, *changed;
>> + apr_pool_t *subpool;
>> + int i;
>> +
>> + if (rev == 0)
>> + {
>> + *rangelist = apr_array_make(pool, 1, sizeof(svn_merge_range_t *));
>
> Again, I suggest using 0 rather than 1.
>
>> + return SVN_NO_ERROR;
>> + }
>> +
>> + subpool = svn_pool_create(pool);
>> +
>> + SVN_ERR(get_combined_rangelist(&curr_rangelist, fs, rev, paths, subpool));
>> + SVN_ERR(get_combined_rangelist(&prev_rangelist, fs, rev - 1, paths, subpool));
>> +
>> + SVN_ERR(svn_rangelist_diff(&deleted, &changed, prev_rangelist, curr_rangelist,
>> + subpool));
>
> What about rollbacks? Don't we need to do something meaningful with
> "deleted"? (I think this means an API change.) That still needs to
> go into the spec(s), too.
The next line merges the "deleted" and "changed" rangelists, so both
should be included in the final list of revisions.
>> + SVN_ERR(svn_rangelist_merge(&changed, deleted, subpool));
>> +
>> + *rangelist = svn_rangelist_dup(changed, pool);
>> + svn_pool_destroy(subpool);
>
> Nice subpool usage! "changed" could be potentially very large, so I'm
> suspicious of having to deep-copy it, but I don't see a better way
> than how you did it.
>
>> +
>> + return SVN_NO_ERROR;
>> +}
>> +
>> +/* Same as send_change_rev(), only send all the revisions in RANGELIST. Also,
>> + INCLUDE_MERGED_REVISIONS is assumed to be TRUE. */
>
> "but" might be a better word choice than "only" -- I misread this doc
> string until I read the implementation. Then again, that might just
> be me...
>
>> +static svn_error_t *
>> +send_child_revs(const apr_array_header_t *paths,
>> + const apr_array_header_t *rangelist,
>> + svn_fs_t *fs,
>> + svn_boolean_t discover_changed_paths,
>> + svn_repos_authz_func_t authz_read_func,
>> + void *authz_read_baton,
>> + svn_log_message_receiver2_t receiver,
>> + void *receiver_baton,
>> + apr_pool_t *pool)
>> +{
>> + apr_array_header_t *revs;
>> + int i;
>> +
>
> I would definitely use a subpool here, for both the subsequent call,
> and the loop. "revs" could potentially be a very large list.
>
>> + SVN_ERR(svn_rangelist_to_revs(&revs, rangelist, pool));
>> +
>> + for (i = 0; i < revs->nelts; i++)
>> + {
>> + svn_revnum_t rev = APR_ARRAY_IDX(revs, i, svn_revnum_t);
>> +
>> + SVN_ERR(send_change_rev(paths, rev, fs, discover_changed_paths, TRUE,
>> + authz_read_func, authz_read_baton,
>> + receiver, receiver_baton, pool));
>> + }
>> +
>> + return SVN_NO_ERROR;
>> +}
> ...
>> @@ -506,11 +620,16 @@
>> *
>> * The detect_changed function is used if either AUTHZ_READ_FUNC is
>> * not NULL, or if DISCOVER_CHANGED_PATHS is TRUE. See it for details.
>> + *
>> + * If INCLUDE_MERGED_REVISIONS is TRUE, also pass history information to
>> + * RECEIVER for any revisions which were merged in a result of REV.
>> */
>> static svn_error_t *
>> -send_change_rev(svn_revnum_t rev,
>> +send_change_rev(const apr_array_header_t *paths,
>> + svn_revnum_t rev,
>> svn_fs_t *fs,
>> svn_boolean_t discover_changed_paths,
>> + svn_boolean_t include_merged_revisions,
>> svn_repos_authz_func_t authz_read_func,
>> void *authz_read_baton,
>> svn_log_message_receiver2_t receiver,
>> @@ -520,6 +639,8 @@
>> svn_string_t *author, *date, *message;
>> apr_hash_t *r_props, *changed_paths = NULL;
>> svn_log_entry_t *log_entry;
>> + apr_uint64_t nbr_children = 0;
>> + apr_array_header_t *rangelist;
> ...
>> + /* Check to see if we need to include any extra merged revisions. */
>> + if (include_merged_revisions)
>> + {
>> + SVN_ERR(get_merge_changed_rangelist(&rangelist, fs, paths, rev, pool));
>> +
>> + nbr_children = svn_rangelist_count_revs(rangelist);
>> + }
>> +
>> log_entry = svn_log_entry_create(pool);
>>
>> log_entry->changed_paths = changed_paths;
>> @@ -578,11 +707,17 @@
>> log_entry->author = author ? author->data : NULL;
>> log_entry->date = date ? date->data : NULL;
>> log_entry->message = message ? message->data : NULL;
>> + log_entry->nbr_children = nbr_children;
>>
>> SVN_ERR((*receiver)(receiver_baton,
>> log_entry,
>> pool));
>>
>> + if (nbr_children > 0)
>> + SVN_ERR(send_child_revs(paths, rangelist, fs, discover_changed_paths,
>> + authz_read_func, authz_read_baton, receiver,
>> + receiver_baton, pool));
>> +
>> return SVN_NO_ERROR;
>> }
> ...