You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Daniel Rall <dl...@collab.net> on 2007/05/31 22:20:19 UTC
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?
Re: WC merge info elision and paths with empty revision ranges
Posted by Daniel Rall <dl...@collab.net>.
On Mon, 11 Jun 2007, Paul Burba wrote:
...
> When I posted the previous patch I hadn't run the full 6-way tests to
> completion. When they finished there were two failures over http:
>
> FAIL: update_tests.py 22: update a schedule-add directory
> FAIL: copy_tests.py 21: copy a deleted directory back from the repos
>
> These were attributable to merge.c:get_wc_or_repos_mergeinfo() trying to
> get reposisory mergeinfo for WC scheduled add paths. Fixed that in
> attached patch, everything else is the same.
>
> Paul
> Support setting and elision of empty rev range mergeinfo and elision to repos.
>
> Prior to this patch we supported empty revision range merge info,
> e.g. svn:mergeinfo '/A/B:', in the sqlite db but we didn't allow it to be set
> on a working copy path. This patch allows that and also makes the mergeinfo
> elision code DTRT with empty revision range merge info, basically this means
^^^
I'd start a new sentence here.
> that if empty revision range merge info has no meaning it elides. This patch
> also introduces the new concept of 'partial' elision, where only paths mapped
> to empty rev ranges elide, leaving other path mappings behind. Again, this is
> only done where the elision of the empty ranges has no semantic impact on the
> total merg einfo.
^^
Typo.
>
> Lastly this patch allows elision not only to a path's nearest WC ancestor but
> also it's nearest REPOS ancestor if a WC ancestor doesn't exist.
^
its
>
> * subversion/include/private/svn_fs_mergeinfo.h
> (svn_fs_mergeinfo__get_mergeinfo):
> * subversion/include/svn_fs.h
> (svn_fs_get_mergeinfo):
> * subversion/include/svn_ra.h
> (svn_ra_get_mergeinfo):
> * subversion/include/svn_repos.h
> (svn_repos_fs_get_mergeinfo):
> Add argument specifying retrieval of inherited merge info only.
You are going to hate -- well, perhaps only be irrated with -- me for
suggesting this, but I think the include_parents and parents_only
arguments should be collapsed into an enumeration (marshalled over the
wire as an integer). parents_only is only going to matter when
include_parents is true (looking below, the code ignores
include_parents when parents_only is set, but not a big difference
there), but even then has slightly different semantics which are best
expressed by a single parameter.
> * subversion/libsvn_client/copy.c
> (calculate_target_mergeinfo): Update call to
> svn_client__get_repos_mergeinfo().
>
> * subversion/libsvn_client/merge.c
> (get_wc_or_repos_mergeinfo): Add two new boolean arguments, one signaling
> retrieval of inherited merge info only, one signaling retrieval of merge
> info only from the repository. Update call to
> svn_client__get_repos_mergeinfo().
> (get_child_only_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 in the parent hash.
> (mergeinfo_elides): Renamed to elide_mergeinfo()
> (elide_mergeinfo): New, replacement for mergeinfo_elides, now not only
> determines if elision occurs, but also performs the elision.
> (elide_children): Replace call to mergeinfo_elides() with elide_mergeinfo().
> (svn_client__elide_mergeinfo): Let helper elide_mergeinfo() test for *and*
> perform elision. If no working copy ancestor with mergeinfo is found permit
> possible check of the repository for ancestor mergeinfo. Rename
> elision_limit_path argument to more accurate wc_elision_limit_path and use
> it as a flag indicating whether to check the repos for ancestor merge info.
> (update_wc_mergeinfo): Allow empty revision range merge info to be set.
> (do_merge, do_single_file_merge): Update callers of
> get_wc_or_repos_mergeinfo().
> (svn_client_get_mergeinfo): Update calls to
> svn_client__get_repos_mergeinfo() and get_wc_or_repos_mergeinfo().
>
> * subversion/libsvn_client/mergeinfo.h
> (svn_client__get_repos_mergeinfo): Add boolean flag indicating only
> inherited mergeinfo should be accquired.
> (svn_client__elide_mergeinfo): Rename elision_limit_path argument to more
> accurate wc_elision_limit_path. Tweak docstring to reflect new empty rev
> range elision and repos elision behaviors.
>
> * subversion/libsvn_client/mergeinfo.c
> (svn_client__get_repos_mergeinfo): Add inherited_only arg, update call to
> svn_ra_get_mergeinfo().
>
> * subversion/libsvn_fs/fs-loader.h
> (struct root_vtable_t): Add parents_only arg to 'get_mergeinfo' hook.
>
> * subversion/libsvn_fs/fs-loader.c
> (svn_fs_get_mergeinfo): Add parents_only argument, update call to
> get_mergeinfo().
>
> * subversion/libsvn_fs_util/mergeinfo-sqlite-index.c
> (get_mergeinfo_for_path): Add parents_only argument, update recursive call.
> (index_path_mergeinfo): Update call to get_mergeinfo_for_path().
> (get_mergeinfo): Add parents_only arg, update call to
> get_mergeinfo_for_path().
> (svn_fs_mergeinfo__get_mergeinfo): Add parents_only argument, update call
> to get_mergeinfo().
> (svn_fs_mergeinfo__get_mergeinfo_for_tree): Update call to get_mergeinfo().
>
> * subversion/libsvn_ra/ra_loader.h
> (struct svn_ra__vtable_t): Add parents_only arg to 'get_mergeinfo' hook.
>
> * subversion/libsvn_ra/ra_loader.c (svn_ra_dav__get_mergeinfo):
> * subversion/libsvn_ra_dav/mergeinfo.c (svn_ra_dav__get_mergeinfo):
> * subversion/libsvn_ra_dav/ra_dav.h (svn_ra_dav__get_mergeinfo):
> * subversion/libsvn_ra_serf/mergeinfo.c (svn_ra_serf__get_mergeinfo):
> * subversion/libsvn_ra_svn/client.c (ra_svn_get_merge_info):
> Add parents_only argument.
You missed update of the subversion/libsvn_ra_svn/protocol document.
It needs to be updated whenever we change the "svn" protocol.
There's a similar document at notes/webdav-protocol, but I'm not sure
it has the same level of detail. I don't recall ever having updated
it for a protocol tweak -- oops! In fact, it looks like it should
list the "merge-info-report" REPORT.
On a related note, this new REPORT also appears to be missing (?) from
dav_svn__reports_list in subversion/mod_dav_svn/dav_svn.h -- I'm
working up a patch to fix that.
...
> --- subversion/libsvn_client/merge.c (revision 25357)
> +++ subversion/libsvn_client/merge.c (working copy)
...
> + If PARENT_ONLY is TRUE only return inherited merge info.
We call this parents_only in a lot of other places.
> +
> + If TARGET_WCPATH inherited its merge info from a working copy ancestor
> + or if it was obtained from the repository, set *INDIRECT to TRUE, set it
> + to FALSE *otherwise. */
> static svn_error_t *
> get_wc_or_repos_mergeinfo(apr_hash_t **target_mergeinfo,
> const svn_wc_entry_t **entry,
> svn_boolean_t *indirect,
> + svn_boolean_t repos_only,
> + svn_boolean_t parent_only,
> svn_ra_session_t *ra_session,
> const char *target_wcpath,
> svn_wc_adm_access_t *adm_access,
> @@ -1103,27 +1110,34 @@
> ### differs from that of TARGET_WCPATH, and if those paths are
> ### directories, their children as well. */
>
> - SVN_ERR(get_wc_mergeinfo(target_mergeinfo, indirect, FALSE, *entry,
> - target_wcpath, NULL, NULL, adm_access, ctx,
> - pool));
> + if (repos_only)
> + *target_mergeinfo = NULL;
> + else
> + SVN_ERR(get_wc_mergeinfo(target_mergeinfo, indirect, parent_only, *entry,
> + target_wcpath, NULL, NULL, adm_access, ctx,
> + pool));
>
> /* If there in no WC merge info check the repository. */
> if (*target_mergeinfo == NULL)
> {
> apr_hash_t *repos_mergeinfo;
...
> + /* No need to check the repos is this is a local addition. */
> + if ((*entry)->schedule != svn_wc_schedule_add)
Great! This was a pre-existing problem then, yes?
> {
...
> + if (ra_session == NULL)
> + SVN_ERR(svn_client__open_ra_session_internal(&ra_session, url,
> + NULL, NULL, NULL, FALSE,
> + TRUE, ctx, pool));
> + SVN_ERR(svn_client__get_repos_mergeinfo(ra_session,
> + &repos_mergeinfo,
> + repos_rel_path, target_rev,
> + parent_only, pool));
> + if (repos_mergeinfo)
> + {
> + *target_mergeinfo = repos_mergeinfo;
> + *indirect = TRUE;
> + }
> }
> }
> return SVN_NO_ERROR;
...
> +/* Helper for elide_mergeinfo().
...
> + Find all paths in CHILD_MERGEINFO which map to empty revision ranges
> + and copy these from CHILD_MERGEINFO to *EMPTY_RANGE_MERGEINFO iff
> + PARENT_MERGEINFO is NULL or does not have merge info for the path in
> + question.
"parent_" prefix again. I rather prefer "parent_" to "parents_", come
to think of it.
> +
> + All mergeinfo in CHILD_MERGEINFO not copied to *EMPTY_RANGE_MERGEINFO
> + is copied to *NONEMPTY_RANGE_MERGEINFO.
> +
> + *EMPTY_RANGE_MERGEINFO and *NONEMPTY_RANGE_MERGEINFO are set to empty
> + hashes if nothing is copied into them.
> +
> + All copied hashes are deep copies allocated in POOL. */
> static svn_error_t *
...
> +get_child_only_empty_revs(apr_hash_t **empty_range_mergeinfo,
Function name is somewhat awkward. Maybe get_empty_rangelists_for_child()
or something similar?
...
> +/* Helper for svn_client__elide_mergeinfo() and elide_children().
> +
> + Given a working copy PATH, its mergeinfo hash CHILD_MERGEINFO, and the
> + mergeinfo of PATH's nearest ancestor PARENT_MERGEINFO, compare
> + CHILD_MERGEINFO to PARENT_MERGEINFO to see if the former elides to
> + the latter, following the elision rules described in
> + svn_client__elide_mergeinfo()'s docstring. If elision (full or partial)
> + does occur, then update PATH's mergeinfo appropriately. If CHILD_MERGEINFO
> + is NULL, do nothing.
> +
> + If PATH_SUFFIX and PARENT_MERGEINFO are not NULL append PATH_SUFFIX to each
> + path in PARENT_MERGEINFO before performing the comparison. */
> +static svn_error_t *
> +elide_mergeinfo(apr_hash_t *parent_mergeinfo,
...
RE: WC merge info elision and paths with empty revision ranges
Posted by Paul Burba <pb...@collab.net>.
> -----Original Message-----
> From: Paul Burba [mailto:pburba@collab.net]
> Sent: Thursday, June 07, 2007 4:23 PM
> To: Daniel Rall
> Cc: dev@subversion.tigris.org
> Subject: RE: WC merge info elision and paths with empty
> revision ranges
<snip>
> Ok, new patch and log message attached.
>
> Thanks for taking the time to review the first patch, if you
> have the time to take another look at this before I commit it
> would be much appreciated.
When I posted the previous patch I hadn't run the full 6-way tests to
completion. When they finished there were two failures over http:
FAIL: update_tests.py 22: update a schedule-add directory
FAIL: copy_tests.py 21: copy a deleted directory back from the repos
These were attributable to merge.c:get_wc_or_repos_mergeinfo() trying to
get reposisory mergeinfo for WC scheduled add paths. Fixed that in
attached patch, everything else is the same.
Paul
Re: WC merge info elision and paths with empty revision ranges
Posted by Daniel Rall <dl...@collab.net>.
On Thu, 07 Jun 2007, Paul Burba wrote:
> > -----Original Message-----
> > From: Daniel Rall [mailto:dlr@collab.net]
...
> > > 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.)
>
> Agreed, well if what you are saying is the following:
>
> If the merge info on PATH_CHILD is equivalent to the merge info on
> PATH_PARENT, *except* for paths which exist *only* in PATH_PARENT and
> map to empty rev ranges, then PATH_CHILD's merge info elides fully.
>
> And I'm pretty sure that is what you are saying!
>
> Though to be honest I can't quite see how we would ever end up with such
> a situation...regardless I added a test of this (and every other case
> mentioned here) in r25318.
Users will screw up the merge info in their WC. Best to be
accomodating, when reasonably possible.
...
> > > 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.
>
> What I ended up doing instead was adding a boolean arg to
> svn_ra_get_mergeinfo() allowing us to specifically request inherited
> mergeinfo only. This actually seemed a lot simpler. Simpler to use
> anyhow, never having added a arg to svn_ra_* function I had no idea the
> horrific amount of trickle down it causes :-\
Oh yes! I commented on the revised patch later in this email thread.
RE: WC merge info elision and paths with empty revision ranges
Posted by Paul Burba <pb...@collab.net>.
> -----Original Message-----
> From: Daniel Rall [mailto:dlr@collab.net]
> Sent: Thursday, May 31, 2007 6:20 PM
> To: Paul Burba
> Cc: dev@subversion.tigris.org
> Subject: 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:
> ...
<snip>
> > 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.)
Agreed, well if what you are saying is the following:
If the merge info on PATH_CHILD is equivalent to the merge info on
PATH_PARENT, *except* for paths which exist *only* in PATH_PARENT and
map to empty rev ranges, then PATH_CHILD's merge info elides fully.
And I'm pretty sure that is what you are saying!
Though to be honest I can't quite see how we would ever end up with such
a situation...regardless I added a test of this (and every other case
mentioned here) in r25318.
> > 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.
<snip>
> > ----------------------------------------
> >
> > '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!
<snip>
> > ----------------------------------------
> >
> > 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.
What I ended up doing instead was adding a boolean arg to
svn_ra_get_mergeinfo() allowing us to specifically request inherited
mergeinfo only. This actually seemed a lot simpler. Simpler to use
anyhow, never having added a arg to svn_ra_* function I had no idea the
horrific amount of trickle down it causes :-\
> > 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.
Agreed, never "IN" the repos, only TO it. The new patch attached
supports elision to the repos.
> > [[[
<snip>
> > + {
> > + /* "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.
Made that change. You've mentioned this before, I'll try to be more
diligent about it in the future.
> > + {
> > + 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.
I hadn't thought about this. Looking at similar functions like
svn_mergeinfo_diff(), which do copy the key/val pairs, I changed this to
follow suit.
> > + 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. ;-)
elision_type_unknown wasn't ultimately needed so this is back to three
states now and completely within elide_mergeinfo().
> I would also indicate in the doc string and/or enum name that
> this has to do with eliding of WC merge info.
Done.
> > +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.
N/A now that mergeinfo_elides() is now elide_mergeinfo().
> > +
> > + 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.
Moved to appropriate block.
> > +
> > + /* 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.
I see how that could be a bad idea in light of the fact that we don't
typically do it in our code so its not expected, even when it is
documented. I was getting a bit greedy and tried to make a function
that tells us something about mergeinfo, mergeinfo_elides(), try to *do*
something about it too.
I was going to change mergeinfo_elides()/get_child_only_empty_revs() to
stop modifying their input hashes and instead return another hash with
the non-empty range merge info -- similar to what svn_mergeinfo_diff()
does -- but the simplicity of making mergeinfo_elides() actually *do*
the elision struck me and I changed its name to elide_mergeinfo(), added
wcpath and adm_access args, and now everything is a lot cleaner.
> > +
> > + *elision_type = elision_type_unknown;
>
> This can be set in an "else" block just below...
N/A now.
> >
> > + /* 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.
^^^^^
lose I assume.
> > +
> > + 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.
Fixed.
> > + 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.
Things have changed bit since this first patch, but I moved it to an
appropriate block.
> > + 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.
Cleaned this up.
> > + }
> > +
> > +/* DELETE BEFORE COMMIT !!!!!!!!!!!! */ assert(*elision_type !=
> > +elision_type_unknown); /* DELETE BEFORE COMMIT !!!!!!!!!!!! */
> > +
>
> Hehe! <voice personality="borat">very nice</voice>
Nothing to see here anymore...move along :-)
> > 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.
This logic is now in elide_mergeinfo() where it does use a switch.
> ...
> > @@ -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?
Yes it would, consolidated in elide_mergeinfo()
-----
Ok, new patch and log message attached.
Thanks for taking the time to review the first patch, if you have the
time to take another look at this before I commit it would be much
appreciated.
Paul