On Sat, Sep 10, 2011 at 5:43 AM, Stefan Sperling <stsp_at_elego.de> wrote:
> On Sat, Sep 10, 2011 at 03:43:36AM +0200, Stefan Sperling wrote:
>> It seems that the elision algorithm is very basic at the moment,
>> which prevents this from being really useful. Subversion only elides
>> mergeinfo which exactly matches the mergeinfo of a parent.
>> It doesn't perform elision if mergeinfo on the parent covers a
>> greater range of revisions than the mergeinfo on the child.
>>
>> Is this deliberate?
Hi Stefan,
Yes it is, see below...
>> For instance:
>>
>> r3 changed '/branch/gamma'.
>> This is merged into '/trunk', as follows:
>>
>> $ svn merge --reintegrate ^/branch/gamma gamma
>> --- Merging differences between repository URLs into 'gamma':
>> U gamma/delta
>> --- Recording mergeinfo for merge between repository URLs into 'gamma':
>> U gamma
>> $
>>
>> Nothing further happens on the branch.
>> Later, we merge the entire branch (a no-op merge):
>>
>> $ svn merge --reintegrate ^/branch
>> --- Recording mergeinfo for merge between repository URLs into '.':
>> U .
>> $
>>
>> Note that no elision took place.
>> Now we have the following mergeinfo which will not elide:
>>
>> Properties on '.':
>> svn:mergeinfo
>> /branch:2-4
>> Properties on 'gamma':
>> svn:mergeinfo
>> /branch:2-3
>>
>> The following mergeinfo will elide:
>>
>> Properties on '.':
>> svn:mergeinfo
>> /branch:2-4
>> Properties on 'gamma':
>> svn:mergeinfo
>> /branch:2-4
>>
>> So the user is required to run no-op merges on subtrees in order
>> to elide mergeinfo from subtrees. This can be quite tedious.
>
> The updated patch below allows mergeinfo to elide if the child
> mergeinfo is a subset of the mergeinfo inherited from a parent.
>
> This seems to work fine in ad-hoc testing.
> There are some test failures with this patch, but before looking
> into fixing those I'd like to ask merge-tracking gurus if I am being
> naive and overlooking some obvious problem?
You are quite right that elision needs to be smarter, particularly in
the 1.7 world where we no longer set mergeinfo unconditionally on all
subtrees. As it stands now elision will only work in the simplest of
cases; we need a more aggressive approach. Unfortunately yours is a
bit too simplistic. Simply because a subtree's explicit mergeinfo is
a subset of its inherited[1] mergeinfo doesn't allow us to elide the
subtree mergeinfo.
One way your approach is broken is with reverse subtree merges. A
quick demonstration of what I mean, starting with one of our vanilla
Greek trees:
### Make a simple branch, then make a couple of edits on the "trunk"
### in r3 and r4:
trunk_at_1167503>svn copy ^^/A ^^/branch -m "Make a branch"
Committed revision 2.
trunk_at_1167503>echo trunk edit> A\D\gamma
trunk_at_1167503>svn ci -m "trunk edit"
Sending A\D\gamma
Transmitting file data .
Committed revision 3.
trunk_at_1167503>echo trunk edit> A\B\lambda
trunk_at_1167503>svn ci -m "trunk edit"
Sending A\B\lambda
Transmitting file data .
Committed revision 4.
trunk_at_1167503>svn up -q
### Sync merge the root of the branch with "trunk".
### This applies r3 and r4 to the branch:
trunk_at_1167503>svn merge ^^/A branch
--- Merging r2 through r4 into 'branch':
U branch\B\lambda
U branch\D\gamma
--- Recording mergeinfo for merge of r2 through r4 into 'branch':
U branch
trunk_at_1167503>svn pl -vR
Properties on 'branch':
svn:mergeinfo
/A:2-4
### Now reverse merge r4 from a subtree:
trunk_at_1167503>svn merge ^^/A/B branch\B -c-4
--- Reverse-merging r4 into 'branch\B':
G branch\B\lambda
--- Recording mergeinfo for reverse merge of r4 into 'branch\B':
G branch\B
### This leaves us with a situation where r4 hasn't been merged
### to the branch:
trunk_at_1167503>svn st
M branch
M branch\B
M branch\D\gamma
trunk_at_1167503>svn pl -vR
Properties on 'branch':
svn:mergeinfo
/A:2-4
Properties on 'branch\B':
svn:mergeinfo
/A/B:2-3
trunk_at_1167503>svn ci -m "partial sync merge"
Sending branch
Sending branch\B
Sending branch\D\gamma
Transmitting file data .
Committed revision 5.
But your patch would elide[2] the mergeinfo on ^/branch/B and make it
appear r4 was fully merged to ^/branch, which is clearly not the case.
Paul
[1] Of course a path with explicit mergeinfo never inherits mergeinfo,
what I mean here is simply the mergeinfo it would inherit *if* it had
no explicit mergeinfo.
[2] IIUC, I cannot apply your patch to my windows WC with either TSVN
or svn patch...some line ending problems.
> I would commit this change in two steps. One that elides mergeinfo
> more aggressively, and one for the mergeinfo --elide parts.
> However, this combined patch is more useful for testing and review.
>
> [[[
> Elide mergeinfo if mergeinfo inherited from a parent is a superset
> of existing mergeinfo.
>
> In some situations it can be useful to remove redundant mergeinfo
> without actually running a merge. For this purpose, add a new option
> --elide to 'svn mergeinfo' which causes it to elide mergeinfo instead
> of displaying it. The new --elide option is mutually exclusive with
> the --show-revs option.
>
> * subversion/include/svn_client.h
> (svn_client_elide_subtree_mergeinfo): New.
>
> * subversion/svn/cl.h
> (svn_cl__opt_state_t): New option ELIDE_MERGEINFO.
>
> * subversion/svn/mergeinfo-cmd.c
> (svn_cl__mergeinfo): If the --elide option was given, elide
> mergeinfo within the specified working copy path instead
> of displaying mergeinfo, and default to depth 'infinity'.
> If the working copy path was not given, default to the
> current directory.
>
> * subversion/svn/main.c
> (svn_cl__longopt_t, svn_cl__options): New option --elide.
> (svn_cl__cmd_table): Update help for 'svn mergeinfo'.
> (main): Handle new --elide option. Disallow --elide and
> --show-revs being used together.
>
> * subversion/libsvn_client/mergeinfo.c
> (should_elide_mergeinfo): Instead of checking the path-adjusted
> parent mergeinfo for equality with the child's mergeinfo, remove
> inheritable, and path-adjusted, parent mergeinfo from the child,
> and allow elision if the resulting child mergeinfo is empty.
> (elide_subtree_mergeinfo_baton, elide_subtree_mergeinfo_cb,
> elide_subtree_mergeinfo_locked, svn_client_elide_subtree_mergeinfo): New.
>
> * subversion/libsvn_client/mergeinfo.h
> (svn_client__elide_mergeinfo): Update documentation of elision rules.
> ]]]
>
> Index: subversion/include/svn_client.h
> ===================================================================
> --- subversion/include/svn_client.h (revision 1167379)
> +++ subversion/include/svn_client.h (working copy)
> @@ -3587,6 +3587,17 @@ svn_client_mergeinfo_log_eligible(const char *path
> svn_client_ctx_t *ctx,
> apr_pool_t *pool);
>
> +/* Try to elide mergeinfo on working copy node @a local_abspath
> + * and children within the specified @a depth.
> + * Use @a scratch_pool for any temporary allocations.
> + * @see svn_client__elide_mergeinfo() for elision rules
> + * @since New in 1.8. */
> +svn_error_t *
> +svn_client_elide_subtree_mergeinfo(const char *local_abspath,
> + svn_depth_t depth,
> + svn_client_ctx_t *ctx,
> + apr_pool_t *scratch_pool);
> +
> /** @} */
>
> /**
> Index: subversion/svn/cl.h
> ===================================================================
> --- subversion/svn/cl.h (revision 1167379)
> +++ subversion/svn/cl.h (working copy)
> @@ -230,6 +230,7 @@ typedef struct svn_cl__opt_state_t
> svn_boolean_t internal_diff; /* override diff_cmd in config file */
> svn_boolean_t use_git_diff_format; /* Use git's extended diff format */
> svn_boolean_t allow_mixed_rev; /* Allow operation on mixed-revision WC */
> + svn_boolean_t elide_mergeinfo; /* Elide mergeinfo. */
> } svn_cl__opt_state_t;
>
>
> Index: subversion/svn/mergeinfo-cmd.c
> ===================================================================
> --- subversion/svn/mergeinfo-cmd.c (revision 1167379)
> +++ subversion/svn/mergeinfo-cmd.c (working copy)
> @@ -30,6 +30,7 @@
> #include "svn_pools.h"
> #include "svn_client.h"
> #include "svn_cmdline.h"
> +#include "svn_dirent_uri.h"
> #include "svn_path.h"
> #include "svn_error.h"
> #include "svn_error_codes.h"
> @@ -66,14 +67,40 @@ svn_cl__mergeinfo(apr_getopt_t *os,
> apr_array_header_t *targets;
> const char *source, *target;
> svn_opt_revision_t src_peg_revision, tgt_peg_revision;
> - /* Default to depth empty. */
> - svn_depth_t depth = opt_state->depth == svn_depth_unknown
> - ? svn_depth_empty : opt_state->depth;
> -
> + svn_depth_t depth;
> +
> SVN_ERR(svn_cl__args_to_target_array_print_reserved(&targets, os,
> opt_state->targets,
> ctx, FALSE, pool));
>
> + /* If we're eliding mergeinfo we don't display any. */
> + if (opt_state->elide_mergeinfo)
> + {
> + if (targets->nelts > 1)
> + return svn_error_create(SVN_ERR_CL_ARG_PARSING_ERROR, NULL,
> + _("Too many arguments given"));
> +
> + if (targets->nelts == 1)
> + target = APR_ARRAY_IDX(targets, 0, const char *);
> + else
> + target = "";
> +
> + SVN_ERR(svn_dirent_get_absolute(&target, target, pool));
> +
> + /* Default to depth infinity. */
> + depth = opt_state->depth == svn_depth_unknown
> + ? svn_depth_infinity : opt_state->depth;
> +
> + SVN_ERR(svn_client_elide_subtree_mergeinfo(target, depth, ctx, pool));
> + return SVN_NO_ERROR;
> + }
> +
> + /* Not eliding mergeinfo, so display mergeinfo. */
> +
> + /* Default to depth empty. */
> + depth = opt_state->depth == svn_depth_unknown
> + ? svn_depth_empty : opt_state->depth;
> +
> /* We expect a single source URL followed by a single target --
> nothing more, nothing less. */
> if (targets->nelts < 1)
> Index: subversion/svn/main.c
> ===================================================================
> --- subversion/svn/main.c (revision 1167379)
> +++ subversion/svn/main.c (working copy)
> @@ -125,6 +125,7 @@ typedef enum svn_cl__longopt_t {
> opt_internal_diff,
> opt_use_git_diff_format,
> opt_allow_mixed_revisions,
> + opt_elide,
> } svn_cl__longopt_t;
>
>
> @@ -342,6 +343,7 @@ const apr_getopt_option_t svn_cl__options[] =
> "Use of this option is not recommended!\n"
> " "
> "Please run 'svn update' instead.")},
> + {"elide", opt_elide, 0, N_("elide mergeinfo")},
>
> /* Long-opt Aliases
> *
> @@ -926,16 +928,21 @@ const svn_opt_subcommand_desc2_t svn_cl__cmd_table
> opt_allow_mixed_revisions} },
>
> { "mergeinfo", svn_cl__mergeinfo, {0}, N_
> - ("Display merge-related information.\n"
> - "usage: mergeinfo SOURCE[@REV] [TARGET[@REV]]\n"
> + ("Display or eliminate redundancy in merge-related information.\n"
> + "usage: 1. mergeinfo SOURCE[@REV] [TARGET[@REV]]\n"
> + " 2. mergeinfo --elide [PATH]\n"
> "\n"
> - " Display information related to merges (or potential merges) between\n"
> - " SOURCE and TARGET (default: '.'). Display the type of information\n"
> - " specified by the --show-revs option. If --show-revs isn't passed,\n"
> - " it defaults to --show-revs='merged'.\n"
> + " 1. Display information related to merges (or potential merges) between\n"
> + " SOURCE and TARGET (default: '.'). Display the type of information\n"
> + " specified by the --show-revs option. If --show-revs isn't passed,\n"
> + " it defaults to --show-revs='merged'.\n"
> + " The depth can be 'empty' or 'infinity'; the default is 'empty'.\n"
> "\n"
> - " The depth can be 'empty' or 'infinity'; the default is 'empty'.\n"),
> - {'r', 'R', opt_depth, opt_show_revs} },
> + " 2. Attempt to elide merge information within the subtree rooted at PATH\n"
> + " (default '.'). This removes redundant svn:mergeinfo properties from\n"
> + " children whose parents record a superset of the same merge information.\n"
> + " The depth can be any valid depth value; the default is 'infinity'.\n"),
> + {'r', 'R', opt_depth, opt_show_revs, opt_elide} },
>
> { "mkdir", svn_cl__mkdir, {0}, N_
> ("Create a new directory under version control.\n"
> @@ -1525,7 +1532,7 @@ main(int argc, const char *argv[])
> opt_state.depth = svn_depth_unknown;
> opt_state.set_depth = svn_depth_unknown;
> opt_state.accept_which = svn_cl__accept_unspecified;
> - opt_state.show_revs = svn_cl__show_revs_merged;
> + opt_state.show_revs = svn_cl__show_revs_invalid;
>
> /* No args? Show usage. */
> if (argc <= 1)
> @@ -2030,6 +2037,9 @@ main(int argc, const char *argv[])
> case opt_allow_mixed_revisions:
> opt_state.allow_mixed_rev = TRUE;
> break;
> + case opt_elide:
> + opt_state.elide_mergeinfo = TRUE;
> + break;
> default:
> /* Hmmm. Perhaps this would be a good place to squirrel away
> opts that commands like svn diff might need. Hmmm indeed. */
> @@ -2222,6 +2232,21 @@ main(int argc, const char *argv[])
> return svn_cmdline_handle_exit_error(err, pool, "svn: ");
> }
>
> + /* Disallow simultaneous use of both --show-revs and --elide. */
> + if (opt_state.elide_mergeinfo &&
> + opt_state.show_revs != svn_cl__show_revs_invalid)
> + {
> + err = svn_error_create(SVN_ERR_INCORRECT_PARAMS, NULL,
> + _("--show-revs and --elide are mutually "
> + "exclusive"));
> + return svn_cmdline_handle_exit_error(err, pool, "svn: ");
> + }
> + else if (opt_state.show_revs == svn_cl__show_revs_invalid)
> + {
> + /* Set the default value of --show-revs. */
> + opt_state.show_revs = svn_cl__show_revs_merged;
> + }
> +
> /* Ensure that 'revision_ranges' has at least one item, and make
> 'start_revision' and 'end_revision' match that item. */
> if (opt_state.revision_ranges->nelts == 0)
> Index: subversion/libsvn_client/mergeinfo.c
> ===================================================================
> --- subversion/libsvn_client/mergeinfo.c (revision 1167379)
> +++ subversion/libsvn_client/mergeinfo.c (working copy)
> @@ -840,9 +840,9 @@ should_elide_mergeinfo(svn_boolean_t *elides,
> else
> path_tweaked_parent_mergeinfo = parent_mergeinfo;
>
> - SVN_ERR(svn_mergeinfo__equals(elides,
> - path_tweaked_parent_mergeinfo,
> - child_mergeinfo, TRUE, subpool));
> + SVN_ERR(svn_mergeinfo_remove2(&child_mergeinfo, parent_mergeinfo,
> + child_mergeinfo, TRUE, subpool, subpool));
> + *elides = (apr_hash_count(child_mergeinfo) == 0);
> svn_pool_destroy(subpool);
> }
>
> @@ -2255,3 +2255,64 @@ svn_client__mergeinfo_status(svn_boolean_t *mergei
>
> return SVN_NO_ERROR;
> }
> +
> +/* Baton for elide_subtree_mergeinfo_cb() and
> + * elide_subtree_mergeinfo_locked(). */
> +struct elide_subtree_mergeinfo_baton {
> + const char *local_abspath;
> + svn_client_ctx_t *ctx;
> + svn_depth_t depth;
> +};
> +
> +/* Implements svn_wc__proplist_receiver_t. */
> +static svn_error_t *
> +elide_subtree_mergeinfo_cb(void *baton,
> + const char *local_abspath,
> + apr_hash_t *props,
> + apr_pool_t *scratch_pool)
> +{
> + struct elide_subtree_mergeinfo_baton *b = baton;
> +
> + SVN_ERR(svn_client__elide_mergeinfo(local_abspath,
> + b->local_abspath,
> + b->ctx, scratch_pool));
> + return SVN_NO_ERROR;
> +}
> +
> +static svn_error_t *
> +elide_subtree_mergeinfo_locked(void *baton,
> + apr_pool_t *result_pool,
> + apr_pool_t *scratch_pool)
> +{
> + struct elide_subtree_mergeinfo_baton *b = baton;
> +
> + /* Use proplist to find nodes with mergeinfo, and try to elide
> + * mergeinfo on each. */
> + SVN_ERR(svn_wc__prop_list_recursive(b->ctx->wc_ctx, b->local_abspath,
> + SVN_PROP_MERGEINFO,
> + b->depth, FALSE, FALSE, NULL,
> + elide_subtree_mergeinfo_cb, baton,
> + b->ctx->cancel_func,
> + b->ctx->cancel_baton,
> + scratch_pool));
> + return SVN_NO_ERROR;
> +}
> +
> +svn_error_t *
> +svn_client_elide_subtree_mergeinfo(const char *local_abspath,
> + svn_depth_t depth,
> + svn_client_ctx_t *ctx,
> + apr_pool_t *scratch_pool)
> +{
> + struct elide_subtree_mergeinfo_baton baton;
> +
> + baton.local_abspath = local_abspath;
> + baton.depth = depth;
> + baton.ctx = ctx;
> +
> + SVN_ERR(svn_wc__call_with_write_lock(elide_subtree_mergeinfo_locked,
> + &baton, ctx->wc_ctx,
> + local_abspath, FALSE,
> + scratch_pool, scratch_pool));
> + return SVN_NO_ERROR;
> +}
> Index: subversion/libsvn_client/mergeinfo.h
> ===================================================================
> --- subversion/libsvn_client/mergeinfo.h (revision 1167379)
> +++ subversion/libsvn_client/mergeinfo.h (working copy)
> @@ -347,7 +347,7 @@ svn_client__record_wc_mergeinfo(const char *local_
> B) TARGET_WCPATH has empty mergeinfo and its nearest parent also
> has empty mergeinfo.
>
> - C) TARGET_WCPATH has the same mergeinfo as its nearest parent
> + C) TARGET_WCPATH has a subset of the mergeinfo as its nearest parent
> when that parent's mergeinfo is adjusted for the path
> difference between the two, e.g.:
>
> @@ -355,9 +355,14 @@ svn_client__record_wc_mergeinfo(const char *local_
> TARGET_WCPATH's mergeinfo = '/A/D/H:3'
> TARGET_WCPATH nearest parent = A_COPY
> Parent's mergeinfo = '/A:3'
> - Path differece = 'D/H'
> + Path difference = 'D/H'
> Parent's adjusted mergeinfo = '/A/D/H:3'
>
> + TARGET_WCPATH's mergeinfo is a subset of the nearest parent
> + mergeinfo if removing mergeinfo inherited from the parent,
> + with adjusted paths, from TARGET_WCPATH's mergeinfo results
> + in empty mergeinfo. See svn_mergeinfo_remove2().
> +
> If Elision occurs remove the svn:mergeinfo property from
> TARGET_WCPATH. */
> svn_error_t *
>
Received on 2011-09-10 18:46:24 CEST