WC merge info elision and paths with empty revision ranges

On Tue, 29 May 2007, Paul Burba wrote:
...
> > On Tue, 22 May 2007, Paul Burba wrote:
...
> > > I added a new merge test in r25101 that tests some practical
> > > applications of this fix, most importantly merging to a path
> > > then reversing that merge to a subtree of the path:
> > >
> > > svn merge -rX:Y URL/SOURCE PATH
> > > svn merge -rY:X URL/SOURCE/CHILD PATH/CHILD
> > >
> > > PATH/CHILD should end up with mergeinfo for SOURCE/CHILD
> > > with an empty rev range, overriding the merge info on PATH.
...
> The attached patch allows empty revision range merge info to be set and
> handles almost all of the elision cases for such merge info. Unlike my
> previous patch it uses the existing elision code to prevent spurious
> empty range merge info from being set on a path. There is still a
> problem when there is no WC ancestor with merge info to elide to however
> - see REMAINING PROBLEM below. Before I go further in solving that
> problem though, I want to make sure no one objects to the idea of
> 'partial' elision that this patch allows, as well as an expansion of
> what 'full' elision up till now has meant:
>
> ----------------------------------------
>
> NEW TYPE OF 'FULL' ELISION:
>
> If the merge info on PATH_CHILD consists *only* of paths that map to
> empty revision ranges, and *none* of these paths exist in PATH_PARENT's
> merge info, then PATH_CHILD's merge info elides to PATH_PARENT.

I agree. Furthermore, if PATH_PARENT has merge info which contains a
path with an empty revision range, CHILD_PATH should still elide to
PATH_PARENT. (You probably aren't considering this to be new type of
full elision.)

> e.g.
> Properties on 'A/D':
> svn:mergeinfo : /A_COPY/D:3-4
> Properties on 'A/D/H':
> svn:mergeinfo : /A_COPY_2/D/H:
> svn:mergeinfo : /A_COPY_3/D/H:
>
> The merge info on 'merge_tests-1/A/D/H' would fully elide, leaving no
> merge info on 'merge_tests-1/A/D/H'.
>
> ----------------------------------------
>
> ANOTHER NEW TYPE OF 'FULL' ELISION:
>
> If the merge info on PATH_CHILD contains *some* paths that don't exist
> in PATH_PARENT's merge info and map to empty revision ranges, then
> PATH_CHILD's merge info still elides if it is otherwise equivalent to
> PATH_PARNET's merge info.

I agree. This is the converse of what I suggested above.

> e.g.
> Properties on 'A/D':
> svn:mergeinfo : /A_COPY/D:3-4
> Properties on 'A/D/H':
> svn:mergeinfo : /A_COPY_2/D/H:
> svn:mergeinfo : /A_COPY_3/D/H:
> svn:mergeinfo : /A_COPY/D/H:3-4
>
> The merge info on 'merge_tests-1/A/D/H' would fully elide, leaving no
> merge info.
>
> ----------------------------------------
>
> 'PARTIAL' ELISION:
>
> If the merge info on PATH_CHILD contains *some* paths that don't exist
> in PATH_PARENT's merge info and map to empty revision ranges, but the
> other paths in PATH_CHILD's merge are *not* equivalent to PATH_PARENT's
> merge info, then only the empty rev range paths unique to PATH_CHILD's
> merge info elide.

Agreed!

> e.g.
> Properties on 'A/D':
> svn:mergeinfo : /A_COPY/D:3-4
> Properties on 'A/D/H':
> svn:mergeinfo : /A_COPY_2/D/H:
> svn:mergeinfo : /A_COPY_3/D/H:
> svn:mergeinfo : /A_COPY/D/H:3-6
>
> Only the merge info for the empty revision ranges on
> 'merge_tests-1/A/D/H' elides, leaving:
>
> Properties on 'A/D':
> svn:mergeinfo : /A_COPY/D:3-4
> Properties on 'A/D/H':
> svn:mergeinfo : /A_COPY/D/H:3-6
>
> ----------------------------------------
>
> REMAINING PROBLEM:
>
> Currently the elision code only tries to elide merge info the nearest
> *working copy* ancestor with merge info. This causes a problem with
> this patch when there is no WC ancestor with merge info.
>
> e.g. A greek WC with the following changes:
> r2 - Copy A to A_COPY
> r3 - Text mod to A/D/H/psi
>
> >svn st merge_tests-1
>
> >svn pl -vR merge_tests-1
> Properties on 'merge_tests-1\A_COPY':
> svn:mergeinfo : /A:1
>
> # Merge to a target with no WC
> # ancestors with merge info
> >svn merge -c3 %URL%/A_COPY/D/H merge_tests-1\A\D\H
> U merge_tests-1\A\D\H\psi
>
> >svn pl -vR merge_tests-1
> Properties on 'merge_tests-1\A\D\H':
> svn:mergeinfo : /A_COPY/D/H:3
> Properties on 'merge_tests-1\A_COPY':
> svn:mergeinfo : /A:1
>
> # Reverse the previous merge...
> >svn merge -c-3 %URL%/A_COPY/D/H merge_tests-1\A\D\H
> G merge_tests-1\A\D\H\psi
>
> # ...which leaves spurious empty range
> # merge info on the target.
> >svn pl -vR merge_tests-1
> Properties on 'merge_tests-1\A\D\H':
> svn:mergeinfo : /A_COPY/D/H:
> Properties on 'merge_tests-1\A_COPY':
> svn:mergeinfo : /A:1
>
> >svn st merge_tests-1
> M merge_tests-1\A\D\H
>
> We can't simply say, "don't set empty range merge info on a target if no
> WC ancestor with merge info is found". In this example it would be ok,
> but if /A/D/H was a disconnected working copy then we wouldn't know if
> we were overriding the merge info of some ancestor path in the
> repository. This example could also be set up as a partial elision
> sceario.
>
> The solution to this problem seems straightforward: check the repository
> for the nearest ancestor with merge info if one in the WC cannot be
> found. svn_ra_get_mergeinfo() would need to be changed to indicate if
> the merge info it obtained was set explicitly on a path or was
> inherited, analogous to what merge.c:get_wc_mergeinfo() does.
> svn_client__get_repos_mergeinfo() would be similarly changed and then
> the elision code could use it to decide if elision occurs or not.
> Question is, do we want to elide merge info to the *repos*? I don't see
> any problems with this. Sound ok to you?

Your suggested solution sounds reasonable. For operations which
affect only the WC, I certainly don't want to elide merge info IN the
repository, but considering the repository's merge info when
performing eliding does sound like the way to go.

> [[[
> Support setting and elision of empty rev range merge info.
>
> * subversion/libsvn_client/merge.c
> (separate_child_empty_revs): New helper for mergeinfo_elides(), finds
> merge
> info for paths in a child hash that map to empty revision ranges and
> don't
> exist and a parent hash.
> (elision_type): New enum describing the three possible states of
> elision:
> 'none', 'partial', or 'full'.
> (mergeinfo_elides): Implement determination of new 'partial' elision
> state.
> (elide_children, svn_client__elide_mergeinfo): Upate callers of
> mergeinfo_elides(), implment partial elision of merge info.
> (update_wc_mergeinfo): Permit setting of empty revision range merge
> info.
>
> * subversion/libsvn_client/mergeinfo.h
> (svn_client__elide_mergeinfo): Expand docstring to describe partial
> elision.
> ]]]

> Index: subversion/libsvn_client/merge.c
> ===================================================================
> --- subversion/libsvn_client/merge.c (revision 25175)
> +++ subversion/libsvn_client/merge.c (working copy)
> @@ -1135,32 +1135,96 @@
> 
> /*** Eliding merge info. ***/
>
> -/* Helper for svn_client__elide_mergeinfo and elide_children.
> +/* Helper for mergeinfo_elides().
>
> - Compare PARENT_MERGEINFO and CHILD_MERGEINFO to see if they are
> - identical (they may be NULL). If PATH_SUFFIX is not NULL append
> - PATH_SUFFIX to each path in PARENT_MERGEINFO before performing the
> - comparison.
> + Find all paths in CHILD_MERGEINFO which map to empty revision ranges
> + and move these from CHILD_MERGEINFO to *EMPTY_RANGE_MERGEINFO iff
> + PARENT_MERGEINFO is NULL or does not have merge info for the path in
> + question. If no merge info is moved, *EMPTY_RANGE_MERGEINFO is set to
> + an empty hash. */
> +static svn_error_t *
> +get_child_only_empty_revs(apr_hash_t **empty_range_mergeinfo,
> + apr_hash_t *child_mergeinfo,
> + apr_hash_t *parent_mergeinfo,
> + apr_pool_t *pool)
> +{
> + *empty_range_mergeinfo = apr_hash_make(pool);
>
> + if (child_mergeinfo)
> + {
> + apr_hash_index_t *hi;
> + void *child_val;
> + const void *child_key;
> +
> + /* Iterate through CHILD_MERGEINFO looking for merge info with empty
> + revision ranges. */
> + for (hi = apr_hash_first(pool, child_mergeinfo); hi;
> + hi = apr_hash_next(hi))
> + {
> + apr_hash_this(hi, &child_key, NULL, &child_val);
> +
> + if (((apr_array_header_t *)child_val)->nelts == 0)
                                        ^
Missing whitespace.

> + {
> + /* "Move" paths with empty revision ranges which don't
> + exist in PARENT_MERGEINFO from CHILD_MERGEINFO to
> + *EMPTY_RANGE_MERGEINFO. */
> + if (parent_mergeinfo == NULL
> + || !apr_hash_get(parent_mergeinfo, child_key,
> + APR_HASH_KEY_STRING))
                                                          ^
"== NULL" would be more explicit.

> + {
> + apr_hash_set(*empty_range_mergeinfo, child_key,
> + APR_HASH_KEY_STRING, child_val);

I guess we're not worried about APR pool lifetime issues? If we were,
we'd duplicate "child_key" and "child_val" for *empty_range_mergeinfo
using its pool. If we're not, we should indicate as much in the
function's doc string.

> + apr_hash_set(child_mergeinfo, child_key,
> + APR_HASH_KEY_STRING, NULL);
> + }
> + }
> + }
> + }
> + return SVN_NO_ERROR;
> +}
> +
> +/* A tri-state value returned by mergeinfo_elides(). */

Hmmm. While we don't really use elision_type_unknown, I wouldn't call
this a tri-state enum. ;-)

I would also indicate in the doc string and/or enum name that this has
to do with eliding of WC merge info.

> +enum elision_type
> +{
> + elision_type_unknown, /* Elision status not determined - never returned by
> + mergeinfo_elides(), used internally only. */
> + elision_type_none, /* No elision occurs. */
> + elision_type_partial, /* Paths that exist only in CHILD_MERGEINFO and
> + map to empty revision ranges elide. */
> + elision_type_full /* All merge info in CHILD_MERGEINFO elides. */
> +};
> +
> +/* Helper for svn_client__elide_mergeinfo() and elide_children().
> +
> + Compare PARENT_MERGEINFO and CHILD_MERGEINFO to see if they are identical.
> + If CHILD_MERGEINFO is NULL, ELISION_TYPE is set to ELISION_TYPE_NONE. If
> + PATH_SUFFIX and PARENT_MERGEINFO are not NULL append PATH_SUFFIX to each
> + path in PARENT_MERGEINFO before performing the comparison.
> +
> Set *ELIDES to whether PARENT_MERGEINFO and CHILD_MERGEINFO are
> - identical (TRUE or FALSE). */
> + identical (TRUE or FALSE).

We no longer set *ELIDES -- it's been replaced by ELISION_TYPE.

> +
> + If CHILD_MERGEINFO consists only of paths mapped to empty revision ranges,
> + and these paths do not exist in PARENT_MERGEINFO ????. */
> static svn_error_t *
> -mergeinfo_elides(svn_boolean_t *elides,
> +mergeinfo_elides(enum elision_type *elision_type,
> apr_hash_t *parent_mergeinfo,
> apr_hash_t *child_mergeinfo,
> const char *path_suffix,
> apr_pool_t *pool)
> {
> - apr_pool_t *subpool = svn_pool_create(pool);
> - apr_hash_t *mergeinfo;
> -
> - if (parent_mergeinfo == NULL || child_mergeinfo == NULL ||
> - apr_hash_count(parent_mergeinfo) != apr_hash_count(child_mergeinfo))
> + apr_pool_t *subpool;
> + apr_hash_t *mergeinfo, *child_empty_mergeinfo;
> + svn_boolean_t equal_mergeinfo;

We can defer declaration of "equal_mergeinfo" until later on.

> +
> + /* Easy out: No child merge info to elide or no parent to elide to. */
> + if (parent_mergeinfo == NULL || child_mergeinfo == NULL)
> {
> - *elides = FALSE;
> + *elision_type = elision_type_none;
> return SVN_NO_ERROR;
> }
>
> + subpool = svn_pool_create(pool);
> if (path_suffix)
> {
> apr_hash_index_t *hi;
> @@ -1186,9 +1250,50 @@
> mergeinfo = parent_mergeinfo;
> }
>
> - SVN_ERR(svn_mergeinfo__equals(elides, child_mergeinfo, mergeinfo,
> - subpool));
> + /* Separate any merge info with empty rev ranges for paths that exist only
> + in CHILD_MERGEINFO and store these in CHILD_EMPTY_MERGEINFO. */
> + SVN_ERR(get_child_only_empty_revs(&child_empty_mergeinfo, child_mergeinfo,
> + mergeinfo, pool));

IMO, it's not typically great practice to modify your inputs (as
get_child_only_empty_revs() does). While get_child_only_empty_revs()
documents that it's modifying its input, mergeinfo_elides() is not,
but should if it's going to continue to do so.

> +
> + *elision_type = elision_type_unknown;

This can be set in an "else" block just below...

>
> + /* If *all* paths in CHILD_MERGEINFO map to empty revision ranges and none
> + of these paths exist in PARENT_MERGEINFO full elision occurs; if only
> + *some* of the paths in CHILD_MERGEINFO meet this criteria we know, at a
> + minimum, partial elision will occur. */
> + if (apr_hash_count(child_empty_mergeinfo) > 0)
> + {
> + *elision_type = apr_hash_count(child_mergeinfo) == 0
> + ? elision_type_full : elision_type_partial;
> + }

     else
       *elision_type = elision_type_unknown;

...and we could close the curlies from the "if" block, too.

> +
> + if (*elision_type == elision_type_partial
> + || *elision_type == elision_type_unknown)
> + {
> + /*If no determination of elision status has been made yet or we know
           ^
Missing space.

> + only that partial elision occurs, compare CHILD_MERGEINFO with the
> + PATH_SUFFIX tweaked version of PARENT_MERGEINFO for equality.
> +
> + If we determined that at least partial elision occurs, full elision
> + may still yet occur if CHILD_MERGEINFO, which no longer contains any
> + paths unique to it that map to empty revision ranges, is equivalent to
> + PARENT_MERGEINFO. */

Declare "equal_mergeinfo" here.

> + SVN_ERR(svn_mergeinfo__equals(&equal_mergeinfo, child_mergeinfo,
> + mergeinfo, subpool));
> + if (*elision_type == elision_type_partial)
> + {
> + if (equal_mergeinfo)
> + *elision_type = elision_type_full;
> + }
> + else
> + {
> + *elision_type = equal_mergeinfo ? elision_type_full
> + : elision_type_none;
> + }

This might be more clear if the "if (equal_mergeinfo)" check was the
outermost.

> + }
> +
> +/* DELETE BEFORE COMMIT !!!!!!!!!!!! */ assert(*elision_type != elision_type_unknown); /* DELETE BEFORE COMMIT !!!!!!!!!!!! */
> +

Hehe! <voice personality="borat">very nice</voice>

> svn_pool_destroy(subpool);
> return SVN_NO_ERROR;
> }
> @@ -1228,7 +1333,8 @@
> {
> apr_hash_t *child_mergeinfo;
> const char *child_wcpath;
> - svn_boolean_t elides, switched;
> + svn_boolean_t switched;
> + enum elision_type elision_type;
> const svn_wc_entry_t *child_entry;
> svn_sort__item_t *item = &APR_ARRAY_IDX(children_with_mergeinfo, i,
> svn_sort__item_t);
> @@ -1285,13 +1391,18 @@
> path_prefix = svn_path_dirname(path_prefix, iterpool);
> }
>
> - SVN_ERR(mergeinfo_elides(&elides, target_mergeinfo,
> + SVN_ERR(mergeinfo_elides(&elision_type, target_mergeinfo,
> child_mergeinfo, path_suffix,
> iterpool));
> - if (elides)
> + if (elision_type == elision_type_full)
> SVN_ERR(svn_wc_prop_set2(SVN_PROP_MERGE_INFO, NULL,
> child_wcpath, adm_access, TRUE,
> iterpool));
> + else if (elision_type == elision_type_partial)
> + SVN_ERR(svn_client__record_wc_mergeinfo(child_wcpath,
> + child_mergeinfo,
> + adm_access,
> + iterpool));

I would definitely use a "switch" statement here.

...
> @@ -1344,11 +1456,15 @@
> if (mergeinfo == NULL)
> return SVN_NO_ERROR;
>
> - SVN_ERR(mergeinfo_elides(&elides, mergeinfo, target_mergeinfo,
> + SVN_ERR(mergeinfo_elides(&elision_type, mergeinfo, target_mergeinfo,
> NULL, pool));
> - if (elides)
> + if (elision_type == elision_type_full)
> SVN_ERR(svn_wc_prop_set2(SVN_PROP_MERGE_INFO, NULL,
> target_wcpath, adm_access, TRUE, pool));
> + else if (elision_type == elision_type_partial)
> + SVN_ERR(svn_client__record_wc_mergeinfo(target_wcpath,
> + target_mergeinfo,
> + adm_access, pool));
...

Ditto. These above two blocks look quite similar -- would refactoring
into another small help function make sense?