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.
- application/pgp-signature attachment: stored
Received on Thu May 31 20:19:42 2007