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;
> }
...