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;
>> }
> ...
Received on Wed May 30 23:55:43 2007