You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Paul Burba <pb...@collab.net> on 2007/05/22 18:38:41 UTC
RE: svn commit: r25068 - in trunk/subversion: libsvn_client libsvn_fs_util libsvn_subr tests/libsvn_subr
Dan,
As mentioned in IRC 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.
Trunk currently sets no mergeinfo on PATH/CHILD. Attached is a patch
which fixes this.
[[[
Reversing the subtree of a merge puts empty rev range mergeinfo on the
subtree.
* subversion/libsvn_client/merge.c
(svn_client__elide_mergeinfo): Add optional predicate to tell caller
if
elision was performed.
(update_wc_merge_info): Allow rangelists with no ranges.
(do_merge, do_single_file_merge): Update calls to
svn_client__elide_mergeinfo().
(clear_empty_range_mergeinfo): New file private function that clears
merge info on a WC path if consists only of empty ranges and doesn't
override
any ancestor path.
(svn_client_merge3, svn_client_merge_peg3): Use new
clear_empty_range_mergeinfo() as post-elision step on merge target.
* subversion/libsvn_client/mergeinfo.h
(svn_client__elide_mergeinfo): Update doc string and arg list for new
predicate.
* subversion/libsvn_client/switch.c (svn_client__switch_internal):
* subversion/libsvn_client/update.c (svn_client__update_internal):
Update calls to svn_client__elide_mergeinfo().
* subversion/tests/cmdline/merge_tests.py
(test_list): Remove Xfail from empty_rev_range_mergeinfo.
]]]
There's a problem with this patch however: If a path has mergeinfo from
multiple sources, removing all the ranges only for one source path
leaves spurious empty range mergeinfo for that path. For example, we
have a greek tree where:
A is copied to A_COPY in r1
A_COPY/D/G/rho is modified in r2:
A_COPY/D/H/psi is modified in r3:
>svn pl -vR merge_tests-1
Properties on 'merge_tests-1\A_COPY':
svn:mergeinfo : /A:1
# Set some fake mergeinfo on A/D and commit at r5
>svn ps svn:mergeinfo "/A_COPY/B:2" merge_tests-1\A\D
property 'svn:mergeinfo' set on 'merge_tests-1\A\D'
>svn ci -m "" merge_tests-1
Sending merge_tests-1\A\D
Committed revision 5.
# Merge the two text changes into A/D/H
>svn merge %URL%/A_COPY/D/H merge_tests-1\A\D\H -r2:4
U merge_tests-1\A\D\H\psi
>svn pl -vR merge_tests-1
Properties on 'merge_tests-1\A\D':
svn:mergeinfo : /A_COPY/B:2
Properties on 'merge_tests-1\A\D\H':
svn:mergeinfo : /A_COPY/B/H:2
/A_COPY/D/H:3-4
Properties on 'merge_tests-1\A_COPY':
svn:mergeinfo : /A:1
# Reverse the two test changes out of a
# child of the previous merge target
>svn merge %URL%/A_COPY/D/H merge_tests-1\A\D\H -r4:2
G merge_tests-1\A\D\H\psi
# My patch still leaves us with spurious mergeinfo for /A_COPY/D/H on
A/D/H:
>svn pl -vR merge_tests-1
Properties on 'merge_tests-1\A\D':
svn:mergeinfo : /A_COPY/B:2
Properties on 'merge_tests-1\A\D\H':
svn:mergeinfo : /A_COPY/B/H:2
/A_COPY/D/H:
Properties on 'merge_tests-1\A_COPY':
svn:mergeinfo : /A:1
This isn't a really serious problem in this simple example, but
eventually these meaningless empty rev ranges would litter the prop
space.
Any thoughts appreciated.
Paul
> -----Original Message-----
> From: dlr@tigris.org [mailto:dlr@tigris.org]
> Sent: Saturday, May 19, 2007 3:12 PM
> To: svn@subversion.tigris.org
> Subject: svn commit: r25068 - in trunk/subversion:
> libsvn_client libsvn_fs_util libsvn_subr tests/libsvn_subr
>
> Author: dlr
> Date: Sat May 19 12:11:55 2007
> New Revision: 25068
>
> Log:
> More changes necessary to fix issue #2769, to allow for removal of "N"
> or "all" merge sources for a path.
>
> Note: Users of (currently unreleased) APR 1.3.0 will require at least
> r538391 from the ASF's Subversion repository to make
> apr_array_clear() available.
>
> * subversion/libsvn_fs_util/mergeinfo-sqlite-index.c
> Include svn_compat.h for the APR_VERSION_AT_LEAST() macro.
> (get_merge_info_for_path): Add declaration to allow for forward
> reference.
> (no_mergeinfo): New constant representing "no merge info".
> (index_path_merge_info): Support removal of all merge sources for a
> path.
> (parse_mergeinfo_from_db): Filter out invalid revision numbers,
> which are assumed to represent dummy records indicating that a
> merge source has no merge info.
>
> * subversion/libsvn_subr/mergeinfo.c
> Include ctype.h.
> (parse_revision, parse_revision_line): Include range list
> in error text.
> (parse_revlist): Don't return an error when an empty revision list
> is encountered for a merge source. This is expected, in the case
> where other merge sources still have revision lists, but the merge
> source in question does not. Include range list in error text.
>
> * subversion/libsvn_client/merge.c
> Include apr_tables.h and apr_hash.h.
> (adjust_mergeinfo_source_paths): New function that adjusts merge
> source paths. Factored out of get_wc_merge_info().
> (get_wc_merge_info, get_wc_or_repos_merge_info, mergeinfo_elides,
> svn_client__elide_mergeinfo, calculate_merge_ranges,
> update_wc_merge_info): Account for potentially NULL merge info.
> Adjust doc string accordingly.
>
> * subversion/libsvn_client/copy.c
> (extend_wc_merge_info, wc_to_repos_copy): Account for potentially
> NULL merge info.
>
> * subversion/libsvn_client/mergeinfo.h
> * subversion/libsvn_client/mergeinfo.c
> (svn_client__parse_merge_info): Return NULL merge info if there is
> no merge info set on WCPATH. Adjust doc string accordingly.
>
> * subversion/tests/libsvn_subr/mergeinfo-test.c
> (NBR_BROKEN_MERGEINFO_VALS): Decrement to 4.
> (broken_mergeinfo_vals): Remove "/missing-revs-with-colon:
> " test case.
> (mergeinfo7): Add a multi-line merge info with empty range lists.
> (test_parse_multi_line_mergeinfo): Use mergeinfo7.
>
> Review by: kameshj
>
>
> Modified:
> trunk/subversion/libsvn_client/copy.c
> trunk/subversion/libsvn_client/merge.c
> trunk/subversion/libsvn_client/mergeinfo.c
> trunk/subversion/libsvn_client/mergeinfo.h
> trunk/subversion/libsvn_fs_util/mergeinfo-sqlite-index.c
> trunk/subversion/libsvn_subr/mergeinfo.c
> trunk/subversion/tests/libsvn_subr/mergeinfo-test.c
>
> Modified: trunk/subversion/libsvn_client/copy.c
> URL:
> http://svn.collab.net/viewvc/svn/trunk/subversion/libsvn_clien
> t/copy.c?pathrev=25068&r1=25067&r2=25068
> ==============================================================
> ================
> --- trunk/subversion/libsvn_client/copy.c (original)
> +++ trunk/subversion/libsvn_client/copy.c Sat May 19 12:11:55 2007
> @@ -447,8 +447,11 @@
> adm_access, ctx, pool));
>
> /* Combine the provided merge info with any merge info
> from the WC. */
> - SVN_ERR(svn_mergeinfo_merge(&wc_mergeinfo, mergeinfo,
> - pool));
> + if (wc_mergeinfo)
> + SVN_ERR(svn_mergeinfo_merge(&wc_mergeinfo, mergeinfo,
> + pool)); else
> + wc_mergeinfo = mergeinfo;
>
> return svn_client__record_wc_merge_info(target_wcpath,
> wc_mergeinfo,
> adm_access, pool);
> @@ -1241,7 +1244,8 @@
> SVN_ERR(svn_client__parse_merge_info(&wc_mergeinfo, entry,
> pair->src,
> adm_access, ctx,
> pool));
> - SVN_ERR(svn_mergeinfo_merge(&mergeinfo, wc_mergeinfo, pool));
> + if (wc_mergeinfo)
> + SVN_ERR(svn_mergeinfo_merge(&mergeinfo, wc_mergeinfo, pool));
> SVN_ERR(svn_mergeinfo__to_string((svn_string_t **)
> &mergeinfo_prop->value,
> mergeinfo, pool));
>
> Modified: trunk/subversion/libsvn_client/merge.c
> URL:
> http://svn.collab.net/viewvc/svn/trunk/subversion/libsvn_clien
> t/merge.c?pathrev=25068&r1=25067&r2=25068
> ==============================================================
> ================
> --- trunk/subversion/libsvn_client/merge.c (original)
> +++ trunk/subversion/libsvn_client/merge.c Sat May 19 12:11:55 2007
> @@ -23,6 +23,8 @@
> /*** Includes. ***/
>
> #include <apr_strings.h>
> +#include <apr_tables.h>
> +#include <apr_hash.h>
> #include "svn_types.h"
> #include "svn_hash.h"
> #include "svn_wc.h"
> @@ -852,9 +854,32 @@
> >
> /*** Retrieving merge info. ***/
>
> +/* Adjust merge sources in MERGEINFO (which is assumed to be
> non-NULL).
> +*/ static APR_INLINE void adjust_mergeinfo_source_paths(apr_hash_t
> +*mergeinfo, const char *walk_path,
> + apr_hash_t *wc_mergeinfo, apr_pool_t
> +*pool) {
> + apr_hash_index_t *hi;
> + const void *merge_source;
> + void *rangelist;
> + const char *path;
> +
> + for (hi = apr_hash_first(NULL, wc_mergeinfo); hi; hi =
> apr_hash_next(hi))
> + {
> + /* Copy inherited merge info into our output hash,
> adjusting the
> + merge source as appropriate. */
> + apr_hash_this(hi, &merge_source, NULL, &rangelist);
> + path = svn_path_join((const char *) merge_source, walk_path,
> + apr_hash_pool_get(mergeinfo));
> + /* ### If pool has a different lifetime than mergeinfo->pool,
> + ### this use of "rangelist" will be a problem... */
> + apr_hash_set(mergeinfo, path, APR_HASH_KEY_STRING, rangelist);
> + }
> +}
> +
> /* Find explicit or inherited WC merge info for WCPATH, and return it
> - in *MERGEINFO. Set *INHERITED to whether the merge info
> was inherited
> - (TRUE or FALSE).
> + in *MERGEINFO (NULL if no merge info is set). Set *INHERITED to
> + whether the merge info was inherited (TRUE or FALSE).
>
> If INHERITED_ONLY is TRUE look only for inherited merge
> info and ignore
> explicit merge info on WCPATH.
> @@ -889,7 +914,7 @@
> interested in inherited mergeinfo. */
> if (inherited_only)
> {
> - wc_mergeinfo = apr_hash_make(pool);
> + wc_mergeinfo = NULL;
> inherited_only = FALSE;
> }
> else
> @@ -929,7 +954,7 @@
> SVN_ERR(svn_path_get_absolute(&wcpath, wcpath, pool));
> }
>
> - if (apr_hash_count(wc_mergeinfo) == 0 &&
> + if (wc_mergeinfo == NULL &&
> !svn_dirent_is_root(wcpath, strlen(wcpath)))
> {
> svn_error_t *err;
> @@ -976,24 +1001,17 @@
> else
> {
> /* Merge info may be inherited. */
> - apr_hash_index_t *hi;
> - void *val;
> - const void *key;
> - const char *path;
> - apr_array_header_t *rangelist;
> -
> - *inherited = apr_hash_count(wc_mergeinfo) > 0;
> - *mergeinfo = apr_hash_make(pool);
> -
> - for (hi = apr_hash_first(pool, wc_mergeinfo); hi; hi =
> apr_hash_next(hi))
> + if (wc_mergeinfo)
> {
> - /* Copy inherited merge info into our output hash,
> adjusting
> - the merge source as appropriate. */
> - apr_hash_this(hi, &key, NULL, &val);
> - path = svn_path_join((const char *) key, walk_path, pool);
> - rangelist = val;
> -
> - apr_hash_set(*mergeinfo, path,
> APR_HASH_KEY_STRING, rangelist);
> + *inherited = (wc_mergeinfo &&
> apr_hash_count(wc_mergeinfo) > 0);
> + *mergeinfo = apr_hash_make(pool);
> + adjust_mergeinfo_source_paths(*mergeinfo,
> walk_path, wc_mergeinfo,
> + pool);
> + }
> + else
> + {
> + *inherited = FALSE;
> + *mergeinfo = NULL;
> }
> }
>
> @@ -1010,9 +1028,9 @@
> RA_SESSION is NULL). Store any merge info obtained for the target
> (reflected by *ENTRY, which is also acquired and returned by this
> function) in *TARGET_MERGEINFO, if no merge info is found
> - *TARGET_MERGEINFO is an empty hash. If the target inherited any
> - merge info from a WC ancestor set *INHERITED to TRUE, set it to
> - FALSE otherwise. */
> + *TARGET_MERGEINFO is NULL. If the target inherited any merge info
> + *from a WC ancestor set *INHERITED to TRUE, set it to FALSE
> + *otherwise. */
> static svn_error_t *
> get_wc_or_repos_merge_info(apr_hash_t **target_mergeinfo,
> const svn_wc_entry_t **entry, @@
> -1091,7 +1109,7 @@
> pool));
>
> /* If there in no WC merge info check the repository. */
> - if (!apr_hash_count(*target_mergeinfo))
> + if (*target_mergeinfo == NULL)
> {
> apr_hash_t *repos_mergeinfo;
> if (ra_session == NULL)
> @@ -1116,8 +1134,9 @@
> /* Helper for svn_client__elide_mergeinfo and elide_children.
>
> Compare PARENT_MERGEINFO and CHILD_MERGEINFO to see if they are
> - identical. If PATH_SUFFIX is not NULL append PATH_SUFFIX to each
> - path in PARENT_MERGEINFO before performing the comparison.
> + identical (they may be NULL). If PATH_SUFFIX is 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). */
> @@ -1131,7 +1150,8 @@
> apr_pool_t *subpool = svn_pool_create(pool);
> apr_hash_t *mergeinfo;
>
> - if (apr_hash_count(parent_mergeinfo) !=
> apr_hash_count(child_mergeinfo))
> + if (parent_mergeinfo == NULL || child_mergeinfo == NULL ||
> + apr_hash_count(parent_mergeinfo) !=
> + apr_hash_count(child_mergeinfo))
> {
> *elides = FALSE;
> return SVN_NO_ERROR;
> @@ -1305,7 +1325,7 @@
>
> /* If TARGET_WCPATH has no explicit mergeinfo,
> there's nothing to
> elide, we're done. */
> - if (inherited || apr_hash_count(target_mergeinfo) == 0)
> + if (inherited || target_mergeinfo == NULL)
> return SVN_NO_ERROR;
>
> /* Get TARGET_WCPATH's inherited mergeinfo. */ @@
> -1317,7 +1337,7 @@
>
> /* If TARGET_WCPATH has no inherited mergeinfo, there's
> nowhere to elide to, we're done. */
> - if (apr_hash_count(mergeinfo) == 0)
> + if (mergeinfo == NULL)
> return SVN_NO_ERROR;
>
> SVN_ERR(mergeinfo_elides(&elides, mergeinfo,
> target_mergeinfo, @@ -1364,8 +1384,12 @@
> /* Subtract the revision ranges which have already been merged into
> the WC (if any) from the range requested for merging (to avoid
> repeated merging). */
> - target_rangelist = apr_hash_get(target_mergeinfo, rel_path,
> - APR_HASH_KEY_STRING);
> + if (target_mergeinfo)
> + target_rangelist = apr_hash_get(target_mergeinfo, rel_path,
> + APR_HASH_KEY_STRING); else
> + target_rangelist = NULL;
> +
> if (target_rangelist)
> {
> if (is_revert)
> @@ -1539,6 +1563,8 @@
> a fresh copy before using it to update the WC's
> merge info. */
> SVN_ERR(svn_client__parse_merge_info(&mergeinfo,
> entry, path, adm_access,
> ctx, subpool));
> + if (mergeinfo == NULL)
> + mergeinfo = apr_hash_make(subpool);
>
> /* ASSUMPTION: "target_wcpath" is always both a parent and
> prefix of "path". */
>
> Modified: trunk/subversion/libsvn_client/mergeinfo.c
> URL:
> http://svn.collab.net/viewvc/svn/trunk/subversion/libsvn_clien
> t/mergeinfo.c?pathrev=25068&r1=25067&r2=25068
> ==============================================================
> ================
> --- trunk/subversion/libsvn_client/mergeinfo.c (original)
> +++ trunk/subversion/libsvn_client/mergeinfo.c Sat May
> 19 12:11:55 2007
> @@ -75,7 +75,7 @@
> if (propval)
> SVN_ERR(svn_mergeinfo_parse(mergeinfo, propval->data, pool));
> else
> - *mergeinfo = apr_hash_make(pool);
> + *mergeinfo = NULL;
>
> return SVN_NO_ERROR;
> }
>
> Modified: trunk/subversion/libsvn_client/mergeinfo.h
> URL:
> http://svn.collab.net/viewvc/svn/trunk/subversion/libsvn_clien
> t/mergeinfo.h?pathrev=25068&r1=25067&r2=25068
> ==============================================================
> ================
> --- trunk/subversion/libsvn_client/mergeinfo.h (original)
> +++ trunk/subversion/libsvn_client/mergeinfo.h Sat May
> 19 12:11:55 2007
> @@ -30,8 +30,9 @@
> svn_revnum_t rev,
> apr_pool_t *pool);
>
> -/* Parse any merge info from WCPATH's ENTRY and store it in
> MERGEINFO.
> - If no merge info is available, set MERGEINFO to an empty hash. */
> +/* Parse any merge info from the WCPATH's ENTRY and store it in
> + MERGEINFO. If no record of any merge info exists, set
> MERGEINFO to
> + NULL. Does not acount for inherited merge info. */
> svn_error_t *
> svn_client__parse_merge_info(apr_hash_t **mergeinfo,
> const svn_wc_entry_t *entry,
>
> Modified: trunk/subversion/libsvn_fs_util/mergeinfo-sqlite-index.c
> URL:
> http://svn.collab.net/viewvc/svn/trunk/subversion/libsvn_fs_ut
> il/mergeinfo-sqlite-index.c?pathrev=25068&r1=25067&r2=25068
> ==============================================================
> ================
> --- trunk/subversion/libsvn_fs_util/mergeinfo-sqlite-index.c
> (original)
> +++ trunk/subversion/libsvn_fs_util/mergeinfo-sqlite-index.c
> Sat May 19 12:11:55 2007
> @@ -30,6 +30,7 @@
> #include "svn_path.h"
> #include "svn_mergeinfo.h"
>
> +#include "private/svn_compat.h"
> #include "private/svn_fs_mergeinfo.h"
> #include "../libsvn_fs/fs-loader.h"
> #include "svn_private_config.h"
> @@ -181,6 +182,19 @@
> return SVN_NO_ERROR;
> }
>
> +static svn_error_t *
> +get_merge_info_for_path(sqlite3 *db,
> + const char *path,
> + svn_revnum_t rev,
> + apr_hash_t *result,
> + apr_hash_t *cache,
> + svn_boolean_t include_parents,
> + apr_pool_t *pool);
> +
> +/* Represents "no merge info". */
> +static svn_merge_range_t no_mergeinfo = { SVN_INVALID_REVNUM,
> + SVN_INVALID_REVNUM };;
Extra ';'
> +
> /* Insert the necessary indexing data into the DB for all the merges
> on PATH as of NEW_REV, which is provided (unparsed) in
> MERGEINFO_STR. Use POOL for temporary allocations.*/ @@
> -191,10 +205,29 @@
> apr_hash_t *mergeinfo;
> apr_hash_index_t *hi;
> sqlite3_stmt *stmt;
> + svn_boolean_t remove_mergeinfo = FALSE;
>
> SVN_ERR(svn_mergeinfo_parse(&mergeinfo,
> mergeinfo_str->data, pool));
>
> - for (hi = apr_hash_first(pool, mergeinfo);
> + if (apr_hash_count(mergeinfo) == 0)
> + {
> + /* All merge info has been removed from PATH (or explicitly set
> + to "none", if there previously was no merge info). Find all
> + previous merge info, and (further below) insert
> dummy records
> + representing "no merge info" for all its previous merge
> + sources of PATH. */
> + apr_hash_t *cache = apr_hash_make(pool);
> + remove_mergeinfo = TRUE;
> + SVN_ERR(get_merge_info_for_path(db, path, new_rev,
> mergeinfo, cache,
> + TRUE, pool));
> + mergeinfo = apr_hash_get(mergeinfo, path, APR_HASH_KEY_STRING);
> + if (mergeinfo == NULL)
> + /* There was previously no merge info, inherited or explicit,
> + for PATH. */
> + return SVN_NO_ERROR;
> + }
> +
> + for (hi = apr_hash_first(NULL, mergeinfo);
> hi != NULL;
> hi = apr_hash_next(hi))
> {
> @@ -221,11 +254,23 @@
> db);
> SQLITE_ERR(sqlite3_bind_text(stmt, 3, path, -1,
> SQLITE_TRANSIENT),
> db);
> - for (i = 0; i < rangelist->nelts; i++)
> +
> + if (remove_mergeinfo)
> {
> - svn_merge_range_t *range;
> + /* Explicitly set "no merge info" for PATH,
> which may've
> + previously had only inherited merge info. */ #if
> +APR_VERSION_AT_LEAST(1, 3, 0)
> + apr_array_clear(rangelist); #else
> + rangelist = apr_array_make(pool, 1,
> +sizeof(&no_mergeinfo)); #endif
> + APR_ARRAY_PUSH(rangelist, svn_merge_range_t *)
> = &no_mergeinfo;
> + }
>
> - range = APR_ARRAY_IDX(rangelist, i,
> svn_merge_range_t *);
> + for (i = 0; i < rangelist->nelts; i++)
> + {
> + const svn_merge_range_t *range =
> + APR_ARRAY_IDX(rangelist, i, svn_merge_range_t *);
> SQLITE_ERR(sqlite3_bind_int64(stmt, 4, range->start),
> db);
> SQLITE_ERR(sqlite3_bind_int64(stmt, 5,
> range->end), @@ -365,8 +410,6 @@
>
> do
> {
> - svn_merge_range_t *temprange;
> -
> mergedfrom = (char *) sqlite3_column_text(stmt, 0);
> startrev = sqlite3_column_int64(stmt, 1);
> endrev = sqlite3_column_int64(stmt, 2); @@ -381,10
> +424,17 @@
> pathranges = apr_array_make(pool, 1,
>
> sizeof(svn_merge_range_t *));
> }
> - temprange = apr_pcalloc(pool, sizeof(*temprange));
> - temprange->start = startrev;
> - temprange->end = endrev;
> - APR_ARRAY_PUSH(pathranges, svn_merge_range_t *) =
> temprange;
> +
> + /* Filter out invalid revision numbers, which are
> assumed to
> + represent dummy records indicating that a merge source
> + has no merge info for PATH. */
> + if (SVN_IS_VALID_REVNUM(startrev) &&
> SVN_IS_VALID_REVNUM(endrev))
> + {
> + svn_merge_range_t *range = apr_pcalloc(pool,
> sizeof(*range));
> + range->start = startrev;
> + range->end = endrev;
> + APR_ARRAY_PUSH(pathranges, svn_merge_range_t
> *) = range;
> + }
>
> sqlite_result = sqlite3_step(stmt);
> lastmergedfrom = mergedfrom;
>
> Modified: trunk/subversion/libsvn_subr/mergeinfo.c
> URL:
> http://svn.collab.net/viewvc/svn/trunk/subversion/libsvn_subr/
> mergeinfo.c?pathrev=25068&r1=25067&r2=25068
> ==============================================================
> ================
> --- trunk/subversion/libsvn_subr/mergeinfo.c (original)
> +++ trunk/subversion/libsvn_subr/mergeinfo.c Sat May 19 12:11:55 2007
> @@ -16,6 +16,7 @@
> *
> ====================================================================
> */
> #include <assert.h>
> +#include <ctype.h>
>
> #include "svn_types.h"
> #include "svn_ctype.h"
> @@ -64,8 +65,9 @@
> svn_revnum_t result = strtol(curr, &endptr, 10);
>
> if (curr == endptr)
> - return svn_error_create(SVN_ERR_MERGE_INFO_PARSE_ERROR, NULL,
> - _("Invalid revision number"));
> + return svn_error_createf(SVN_ERR_MERGE_INFO_PARSE_ERROR, NULL,
> + _("Invalid revision number
> found parsing '%s'"),
> + curr);
>
> *revision = result;
>
> @@ -121,19 +123,28 @@
> const char *curr = *input;
> svn_merge_range_t *lastrange = NULL;
>
> - if (curr == end)
> - return svn_error_create(SVN_ERR_MERGE_INFO_PARSE_ERROR, NULL,
> - _("No revision list found"));
> + /* Eat any leading horizontal white-space before the
> rangelist. */
> + while (curr < end && *curr != '\n' && isspace(*curr))
> + curr++;
> +
> + if (*curr == '\n' || curr == end)
> + {
> + /* Empty range list. */
> + *input = curr;
> + return SVN_NO_ERROR;
> + }
>
> - while (curr < end)
> + while (curr < end && *curr != '\n')
> {
> + /* Parse individual revisions or revision ranges. */
> svn_merge_range_t *mrange = apr_pcalloc(pool, sizeof(*mrange));
> svn_revnum_t firstrev;
>
> SVN_ERR(parse_revision(&curr, end, &firstrev));
> if (*curr != '-' && *curr != '\n' && *curr != ',' &&
> curr != end)
> - return svn_error_create(SVN_ERR_MERGE_INFO_PARSE_ERROR, NULL,
> - _("Invalid character found
> in revision list"));
> + return
> svn_error_createf(SVN_ERR_MERGE_INFO_PARSE_ERROR, NULL,
> + _("Invalid character '%c'
> found in revision "
> + "list"), *curr);
> mrange->start = firstrev;
> mrange->end = firstrev;
>
> @@ -146,7 +157,6 @@
> mrange->end = secondrev;
> }
>
> - /* XXX: Watch empty revision list problem */
> if (*curr == '\n' || curr == end)
> {
> combine_with_lastrange(&lastrange, mrange, FALSE,
> revlist, pool); @@ -160,14 +170,16 @@
> }
> else
> {
> - return
> svn_error_create(SVN_ERR_MERGE_INFO_PARSE_ERROR, NULL,
> - _("Invalid character found
> in revision list"));
> + return
> svn_error_createf(SVN_ERR_MERGE_INFO_PARSE_ERROR, NULL,
> + _("Invalid character '%c'
> found in "
> + "range list"), *curr);
> }
>
> }
> if (*curr != '\n')
> return svn_error_create(SVN_ERR_MERGE_INFO_PARSE_ERROR, NULL,
> - _("Revision list parsing ended
> before hitting newline"));
> + _("Range list parsing ended
> before hitting "
> + "newline"));
> *input = curr;
> return SVN_NO_ERROR;
> }
> @@ -192,8 +204,9 @@
> SVN_ERR(parse_revlist(input, end, revlist, pool));
>
> if (*input != end && *(*input) != '\n')
> - return svn_error_create(SVN_ERR_MERGE_INFO_PARSE_ERROR, NULL,
> - _("Could not find end of line in
> revision line"));
> + return svn_error_createf(SVN_ERR_MERGE_INFO_PARSE_ERROR, NULL,
> + _("Could not find end of line
> in range list line "
> + "in '%s'"), *input);
>
> if (*input != end)
> *input = *input + 1;
>
> Modified: trunk/subversion/tests/libsvn_subr/mergeinfo-test.c
> URL:
> http://svn.collab.net/viewvc/svn/trunk/subversion/tests/libsvn
> _subr/mergeinfo-test.c?pathrev=25068&r1=25067&r2=25068
> ==============================================================
> ================
> --- trunk/subversion/tests/libsvn_subr/mergeinfo-test.c
> (original)
> +++ trunk/subversion/tests/libsvn_subr/mergeinfo-test.c
> Sat May 19 12:11:55 2007
> @@ -181,12 +181,11 @@
> }
>
>
> -#define NBR_BROKEN_MERGEINFO_VALS 5
> +#define NBR_BROKEN_MERGEINFO_VALS 4
> /* Invalid merge info values. */
> static const char * const
> broken_mergeinfo_vals[NBR_BROKEN_MERGEINFO_VALS] =
> {
> "/missing-revs",
> - "/missing-revs-with-colon: ",
> "/trunk: 5,7-9,10,11,13,14,",
> "/trunk 5,7-9,10,11,13,14",
> "/trunk:5 7--9 10 11 13 14"
> @@ -226,6 +225,7 @@
> static const char *mergeinfo4 = "/trunk: 15-25, 35-45";
> static const char *mergeinfo5 = "/trunk: 10-30, 35-45,
> 55-65"; static const char *mergeinfo6 = "/trunk: 15-25";
> +static const char *mergeinfo7 =
> +"/empty-rangelist:\n/with-trailing-space: ";
>
> static svn_error_t *
> test_parse_multi_line_mergeinfo(const char **msg, @@ -239,6 +239,9 @@
> return SVN_NO_ERROR;
>
> SVN_ERR(svn_mergeinfo_parse(&info1, mergeinfo1, pool));
> +
> + SVN_ERR(svn_mergeinfo_parse(&info1, mergeinfo7, pool));
> +
> return SVN_NO_ERROR;
> }
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
WC merge info elision and paths with empty revision ranges
Posted by Daniel Rall <dl...@collab.net>.
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: Re: svn commit: r25068 - in trunk/subversion: libsvn_client libsvn_fs_util libsvn_subr tests/libsvn_subr
Posted by Paul Burba <pb...@collab.net>.
> -----Original Message-----
> From: Daniel Rall [mailto:dlr@collab.net]
> Sent: Wednesday, May 23, 2007 1:03 PM
> To: dev@subversion.tigris.org
> Subject: Re: svn commit: r25068 - in trunk/subversion:
> libsvn_client libsvn_fs_util libsvn_subr tests/libsvn_subr
>
> On Tue, 22 May 2007, Paul Burba wrote:
>
> > Dan,
> >
> > As mentioned in IRC 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.
> >
> > Trunk currently sets no mergeinfo on PATH/CHILD. Attached
> is a patch
> > which fixes this.
> ...
>
> Paul mentioned on IRC that he found a better approach to
> this, and is working on another patch.
Dan (and other interested merge tracking followers),
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.
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.
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.
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?
[[[
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.
]]]
Paul
Re: svn commit: r25068 - in trunk/subversion: libsvn_client libsvn_fs_util libsvn_subr tests/libsvn_subr
Posted by Daniel Rall <dl...@collab.net>.
On Tue, 22 May 2007, Paul Burba wrote:
> Dan,
>
> As mentioned in IRC 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.
>
> Trunk currently sets no mergeinfo on PATH/CHILD. Attached is a patch
> which fixes this.
...
Paul mentioned on IRC that he found a better approach to this, and is
working on another patch.