Re: [PATCH][MERGE-TRACKING] Step 2 of repos to repos copyfrom info recording

On Wed, 27 Sep 2006, Madan S. wrote:
...
> Further steps include obtaining the existing mergeinfo and merging that
> with the now available copyfrom mergeinfo.
>
> I'm not sure if I have to hand-build the mergeinfo hash, as I have done
> in the attached path.
...

This is the appropriate way to build merge info for your use case.
You absolutely need a parsed version of the "copy from" merge info, so
that you can merge it with the path's existing merge info.

It might be worth investing some time in simplifying the case where we
want to build a hash consisting of one path -> range list mapping,
which in turn consistes of only one merge range (as I think we may be
doing this in a few places now). Maybe an API like:

--- subversion/libsvn_subr/mergeinfo.c (revision 21688)
+++ 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));
+ 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);
+
+ return mergeinfo;
+}
+
 /* Merge two revision lists IN1 and IN2 and place the result in
    OUTPUT. We do some trivial attempts to combine ranges as we go. */
 svn_error_t *

In addition to Kamesh's comments, some more review is inline below:

> Index: subversion/libsvn_client/copy.c
> ===================================================================
> --- subversion/libsvn_client/copy.c (revision 21626)
> +++ subversion/libsvn_client/copy.c (working copy)
> @@ -185,8 +185,92 @@
> svn_string_t *mergeinfo;
> };
>
> +/* Log callback for obtaining the copyfrom revision of
> + a given path. */

How about something like:

/* A log callback conforming to the svn_log_message_receiver_t
   interface for obtaining the copyfrom revision of a given path and
   storing it in *BATON (an svn_revnum_t). */

> +static svn_error_t *
> +copyfrom_receiver(void *baton,
> + apr_hash_t *changed_paths,
> + svn_revnum_t revision,
> + const char *author,
> + const char *date,
> + const char *message,
> + apr_pool_t *pool)
> +{
> + svn_revnum_t *svn_revnum = baton;
>

I'd skip this extraneous newline for such a short function.

> + *svn_revnum = revision;
> + return SVN_NO_ERROR;
> +}
> +
> +/* Obtain the copyfrom mergeinfo of the given path. */
> static svn_error_t *

This doc string should indicate that its function can return NULL in
*COPYFROM_MERGEINFO.

> +get_copyfrom_mergeinfo(svn_ra_session_t *ra_session,
> + apr_hash_t **copyfrom_mergeinfo,
> + const char *rel_path,
> + const char *copyfrom_path,
> + svn_revnum_t src_revnum,
> + apr_pool_t *pool)

I think more of our APIs use an extra underscore in the
xxx_merge_info() function names:

  get_copyfrom_merge_info()

The SRC_REVNUM parameter could use a better name, like COPYFROM_REV or
just REV.

> +{
> + svn_revnum_t copyfrom_rev;

Looking at how "copyfrom_rev" is used below, I think it should be
named something like "oldest_copyfrom_rev" or "oldest_rev".

> + svn_merge_range_t *copyfrom_mergerange;
> + apr_array_header_t *copyfrom_mergerange_list;

Let's shorten these variable names to copyfrom_range and
copyfrom_rangelist (respectively); in addition to being more brief,
they're more in line with the naming style used elsewhere on this
branch.

> + apr_array_header_t *rel_paths = apr_array_make(pool, 1,
> + sizeof(const char *));

Should this be sizeof(rel_path) instead (the element we're adding to
this list)? We use that style down below with "copyfrom_rangelist".

> +
> + *copyfrom_mergeinfo = NULL;
> +
> + APR_ARRAY_PUSH(rel_paths, const char *) = rel_path;
> +
> + /* Extract loginfo for finding the copyfrom rev */

This comment could be more clear (or removed entirely). How about
explaining that we're tracing back through history, looking for any
*previous* copy of this node?

> + SVN_ERR(svn_ra_get_log(ra_session, rel_paths, 1, src_revnum,
> + 1, FALSE, TRUE, copyfrom_receiver,
> + &copyfrom_rev, pool));
> +
> + copyfrom_mergerange = apr_palloc(pool, sizeof(*copyfrom_mergerange));
> + copyfrom_mergerange->start = copyfrom_rev;
> + copyfrom_mergerange->end = src_revnum;
> + copyfrom_mergerange_list = apr_array_make(pool, 1,
> + sizeof(copyfrom_mergerange));
...
> +/* Obtain the copyfrom mergeinfo and the existing mergeinfo of

There should be a space between the words "merge" and "info" in the
doc string.

> + the source path, merge them and set as a svn_string_t in
> + TARGET_MERGEINFO. */
> +static svn_error_t *
> +calculate_target_mergeinfo(svn_ra_session_t *ra_session,
> + svn_string_t **target_mergeinfo,
> + const char *src_url,

Isn't this COPYFROM_URL?

> + const char *src_rel,

SRC_REL_PATH, perhaps?

> + svn_revnum_t src_revnum,
> + apr_pool_t *pool)
> +{
> + const char *repos_root;
> + const char *copyfrom_path = src_url;
> + apr_hash_t *copyfrom_mergeinfo;
> +
> + /* Find src path relative to the repos root */

Do we have code to do this anywhere else?

> + SVN_ERR(svn_ra_get_repos_root(ra_session, &repos_root, pool));
> + while (*copyfrom_path++ == *repos_root++ );
                                             ^
Extra whitespace. ^

> + copyfrom_path--;

Is this kosher if the "while" loop never executes? (Which might not be
able to happen...)

> + SVN_ERR(get_copyfrom_mergeinfo(ra_session, &copyfrom_mergeinfo, src_rel,
> + copyfrom_path, src_revnum, pool));
> +
> + /* TODO: Obtain existing mergeinfo */

...via svn_ra_get_merge_info().

> + /* TODO: Merge copyfrom and existing mergeinfo to fill target_mergeinfo */

...via svn_mergeinfo_merge().

> + *target_mergeinfo = NULL;

Alternately, you could set this to copyfrom_mergeinfo for now.

> + return SVN_NO_ERROR;
> +}
...