On Feb 1, 2008 5:37 PM, David Glasser <glasser_at_davidglasser.net> wrote:
Hi David, appreciate you taking a look at this, comments below.
> On Jan 30, 2008 11:34 AM, <pburba_at_tigris.org> wrote:
> > Author: pburba
> > Date: Wed Jan 30 11:34:41 2008
> > New Revision: 29085
> >
> > Log:
> > Stop merge from adding mergeinfo from a target's own history.
> >
> > As discussed here,
> > http://subversion.tigris.org/servlets/ReadMsg?listName=dev&msgNo=134491, the
> > new merge test 'dont_add_mergeinfo_from_own_history' added in this commit
> > fails on Win32 over ra_svn. But since that appears to be an existing problem
> > (uncovered by this new test) I'm committing this and looking into a separate
> > fix.
> >
> > * subversion/libsvn_client/merge.c
> > (mergeinfo_behavior): Move ahead of first call.
> > (filter_self_referential_mergeinfo): New.
> > (merge_props_changed): Call filter_self_referential_mergeinfo() before
> > merging incoming prop changes with original props.
> >
> > * subversion/tests/cmdline/merge_tests.py
> > (dont_add_mergeinfo_from_own_history): New.
> > (test_list): Add dont_add_mergeinfo_from_own_history.
> >
> > Modified:
> > trunk/subversion/libsvn_client/merge.c
> > trunk/subversion/tests/cmdline/merge_tests.py
> >
> > Modified: trunk/subversion/libsvn_client/merge.c
> > URL: http://svn.collab.net/viewvc/svn/trunk/subversion/libsvn_client/merge.c?pathrev=29085&r1=29084&r2=29085
> > ==============================================================================
> > --- trunk/subversion/libsvn_client/merge.c (original)
> > +++ trunk/subversion/libsvn_client/merge.c Wed Jan 30 11:34:41 2008
> > @@ -299,6 +299,218 @@
> > apr_hash_count(merge_b->conflicted_paths) > 0);
> > }
> >
> > +/* Set *HONOR_MERGEINFO and *RECORD_MERGEINFO (if non-NULL)
> > + appropriately for MERGE_B. */
> > +static APR_INLINE void
> > +mergeinfo_behavior(svn_boolean_t *honor_mergeinfo,
> > + svn_boolean_t *record_mergeinfo,
> > + merge_cmd_baton_t *merge_b)
> > +{
> > + if (honor_mergeinfo)
> > + *honor_mergeinfo = (merge_b->mergeinfo_capable
> > + && merge_b->sources_ancestral
> > + && merge_b->same_repos
> > + && (! merge_b->ignore_ancestry));
> > +
> > + if (record_mergeinfo)
> > + *record_mergeinfo = (merge_b->mergeinfo_capable
> > + && merge_b->sources_ancestral
> > + && merge_b->same_repos
> > + && (! merge_b->dry_run));
> > +}
> > +
> > +/* Helper for merge_props_changed(). Filter out mergeinfo property additions
> > + to PATH when those additions refer to the same line of history.
> > +
> > + *PROPS is an array of svn_prop_t structures representing regular properties
> > + to be added to the working copy PATH. ADM_ACCESS and MERGE_B are cascaded
> > + from merge_props_changed().
> > +
> > + If mergeinfo is not being honored, do nothing. Otherwise examine the added
> > + mergeinfo, looking at each range (or single rev) of each source path. If a
> > + source_path/range refers to the same line of history as PATH (pegged at its
> > + base revision), then filter out that range. If the entire rangelist for a
> > + given path is filtered then filter out the path as well. Set outgoing
> > + *PROPS to a shallow copy (allocated in POOL) of incoming *PROPS minus the
> > + filtered self-referential mergeinfo. */
> > +static svn_error_t*
> > +filter_self_referential_mergeinfo(apr_array_header_t **props,
> > + const char *path,
> > + merge_cmd_baton_t *merge_b,
> > + svn_wc_adm_access_t *adm_access,
> > + apr_pool_t *pool)
> > +{
> > + svn_boolean_t honor_mergeinfo, record_mergeinfo;
> > +
> > + mergeinfo_behavior(&honor_mergeinfo, &record_mergeinfo, merge_b);
>
> If record_mergeinfo is unneeded then just pass in NULL.
Fixed.
> > + if (honor_mergeinfo)
> > + {
> > + int i;
> > + apr_array_header_t *adjusted_props =
> > + apr_array_make(pool, 1, sizeof(svn_prop_t));
> > +
> > + for (i = 0; i < (*props)->nelts; ++i)
> > + {
> > + svn_prop_t *prop = &APR_ARRAY_IDX((*props), i, svn_prop_t);
> > +
> > + /* If this property isn't mergeinfo or is empty mergeinfo it
> > + does not require any special handling. */
> > + if (strcmp(prop->name, SVN_PROP_MERGEINFO) != 0
> > + || !prop->value)
> > + {
> > + APR_ARRAY_PUSH(adjusted_props, svn_prop_t) = *prop;
> > + }
> > + else /* Property is non-empty mergeinfo, check if any self-
> > + referential mergeinfo should be filtered out. */
> > + {
> > + apr_hash_t *mergeinfo_catalog;
> > + apr_hash_index_t *hi;
> > + const char *target_url, *merge_source_root_url;
> > + const svn_wc_entry_t *target_entry;
> > + const char *original_ra_url;
> > +
> > + SVN_ERR(svn_ra_get_repos_root(merge_b->ra_session1,
> > + &merge_source_root_url,
> > + pool));
> > +
> > + /* Get an entry for PATH so we can find it's base revision. */
> > + SVN_ERR(svn_wc__entry_versioned(&target_entry, path,
> > + adm_access, FALSE,
> > + pool));
> > +
> > + /* Temporarily reparent our RA session to the merge
> > + target's URL. */
> > + SVN_ERR(svn_client_url_from_path(&target_url, path,
> > + pool));
> > + SVN_ERR(svn_ra_get_session_url(merge_b->ra_session1,
> > + &original_ra_url, pool));
> > + SVN_ERR(svn_ra_reparent(merge_b->ra_session1,
> > + target_url, pool));
> > +
> > + /* Parse the incoming mergeinfo to allow easier meddling. */
> > + SVN_ERR(svn_mergeinfo_parse(&mergeinfo_catalog,
> > + prop->value->data, pool));
>
> Admittedly we haven't documented the proposed term "catalog" yet, but
> in my definition this is a "mergeinfo", not a "mergeinfo catalog". A
> "mergeinfo catalog" is a hash mapping targets to mergeinfos;
> mergeinfos are hashes mapping sources to rangelists.
My mistake, I had the terms confused at one point and forgot to fix. Fixed now.
> > +
> > + for (hi = apr_hash_first(NULL, mergeinfo_catalog);
> > + hi; hi = apr_hash_next(hi))
> > + {
> > + int j;
> > + const void *key;
> > + void *value;
> > + const char *source_path;
> > + apr_array_header_t *rangelist;
> > + apr_array_header_t *adjusted_rangelist =
> > + apr_array_make(pool, 0,
> > + sizeof(svn_merge_range_t *));
> > +
> > + apr_hash_this(hi, &key, NULL, &value);
> > + source_path = key;
> > + rangelist = value;
> > +
> > + for (j = 0; j < rangelist->nelts; j++)
> > + {
> > + svn_error_t *err;
> > + svn_merge_range_t *range =
> > + APR_ARRAY_IDX(rangelist, j, svn_merge_range_t *);
> > +
> > + svn_opt_revision_t *start_revision, *end_revision;
> > + const char *start_url, *end_url;
> > + const char *merge_source_url =
> > + svn_path_join(merge_source_root_url,
> > + source_path + 1, pool);
> > + svn_opt_revision_t peg_rev, rev1_opt, rev2_opt;
> > +
> > + peg_rev.kind = svn_opt_revision_number;
> > + peg_rev.value.number = target_entry->revision;
> > + rev1_opt.kind = svn_opt_revision_number;
> > + rev1_opt.value.number = range->start;
> > +
> > + /* Because the merge source normalization code
> > + ensures mergeinfo refers to real locations on
> > + the same line of history, there's no need to
> > + look at the whole range, just the start. */
> > + rev2_opt.kind = svn_opt_revision_unspecified;
> > +
> > + /* Check if PATH_at_TARGET_ENTRY->REVISION exists at
> > + RANGE->START on the same line of history. */
> > + err = svn_client__repos_locations(&start_url,
> > + &start_revision,
> > + &end_url,
> > + &end_revision,
> > + merge_b->ra_session1,
> > + target_url,
> > + &peg_rev,
> > + &rev1_opt,
> > + &rev2_opt,
> > + merge_b->ctx,
> > + pool);
>
> Whoa. We're doing a full RA roundtrip for every range in every source
> in every mergeinfo prop? That seems terribly suboptimal. We should
> go the other way round: use svn_client__get_history_as_mergeinfo to
> find the history of the path just once, and then use something like
> svn_mergeinfo_remove. You should be able to replace the whole
> function with that sort of thing.
The problem is that svn_client__get_history_as_mergeinfo() can only
look backwards (I know, hence the name!). This isn't a problem if our
merge targets working revision is greater than any of the added
mergeinfo's revisions. Just peg the target to the working rev and do
as you suggest (I haven't tried this but it certainly seems like it
should work).
But what if our WC merge target is at working revision *less* than any
of the added ranges? What can we use for our peg revision to
svn_client__get_history_as_mergeinfo()? Consider this simple example:
Starting with a standard greek tree we make a branch:
>svn copy %url85%/A/D/H merge_tests-85\A\D\H_BRANCH
A merge_tests-85\A\D\H_BRANCH\chi
A merge_tests-85\A\D\H_BRANCH\omega
A merge_tests-85\A\D\H_BRANCH\psi
Checked out revision 1.
A merge_tests-85\A\D\H_BRANCH
>svn ci -m "" merge_tests-85
Adding merge_tests-85\A\D\H_BRANCH
Committed revision 2.
In r3 make a change on the branch source:
>echo text change >> merge_tests-85\A\D\H\chi
>svn ci -m "" merge_tests-85
Sending merge_tests-85\A\D\H\chi
Transmitting file data .
Committed revision 3.
In r4 merge r3 to the branch:
>svn merge %url85%/A/D/H merge_tests-85\A\D\H_BRANCH -c3
--- Merging r3 into 'merge_tests-85\A\D\H_BRANCH':
U merge_tests-85\A\D\H_BRANCH\chi
>svn ci -m "" merge_tests-85
Sending merge_tests-85\A\D\H_BRANCH
Sending merge_tests-85\A\D\H_BRANCH\chi
Transmitting file data .
Committed revision 4.
We see the expected mergeinfo on the branch:
>svn pl -vR merge_tests-85
Properties on 'merge_tests-85\A\D\H_BRANCH':
svn:mergeinfo : /A/D/H:3
Now we cherry pick a revision (r4) from the branch to merge back to
the merge source:
>svn merge %url85%/A/D/H_BRANCH merge_tests-85\A\D\H -c4
...This would attempt to add the mergeinfo '/A/D/H:3' to 'A\D\H'. In
IRC you suggested "get my history as mergeinfo. subtract it from the
mergeinfo prop we're handed. done." But 'A/D/H' is at working
revision 1...
>svn st merge_tests-85 -v
1 1 jrandom merge_tests-85
1 1 jrandom merge_tests-85\A
1 1 jrandom merge_tests-85\A\B
1 1 jrandom merge_tests-85\A\B\lambda
1 1 jrandom merge_tests-85\A\B\E
1 1 jrandom merge_tests-85\A\B\E\alpha
1 1 jrandom merge_tests-85\A\B\E\beta
1 1 jrandom merge_tests-85\A\B\F
1 1 jrandom merge_tests-85\A\mu
1 1 jrandom merge_tests-85\A\C
1 1 jrandom merge_tests-85\A\D
1 1 jrandom merge_tests-85\A\D\gamma
4 4 pburba merge_tests-85\A\D\H_BRANCH
4 4 pburba merge_tests-85\A\D\H_BRANCH\chi
2 2 pburba merge_tests-85\A\D\H_BRANCH\omega
2 2 pburba merge_tests-85\A\D\H_BRANCH\psi
1 1 jrandom merge_tests-85\A\D\G
1 1 jrandom merge_tests-85\A\D\G\pi
1 1 jrandom merge_tests-85\A\D\G\rho
1 1 jrandom merge_tests-85\A\D\G\tau
1 1 jrandom merge_tests-85\A\D\H
3 3 pburba merge_tests-85\A\D\H\chi
1 1 jrandom merge_tests-85\A\D\H\omega
1 1 jrandom merge_tests-85\A\D\H\psi
1 1 jrandom merge_tests-85\iota
...so we can't ask anything about it's history that is useful in
figuring out if '/A/D/H:3' should be added. We can't just ask about
'/A/D/H:'@HEAD either as it could be a completely different line of
history (though of course in this example it isn't). The current
implementation using svn_client__repos_locations() works in this
situation since it allows us to ask about 'A/D/H'@1 at revision 3.
Does that make sense at all?
> > + if (err)
> > + {
> > + if (err->apr_err == SVN_ERR_FS_NOT_FOUND
> > + || err->apr_err == SVN_ERR_RA_DAV_PATH_NOT_FOUND
> > + || err->apr_err ==
> > + SVN_ERR_CLIENT_UNRELATED_RESOURCES)
> > + {
> > + /* PATH_at_TARGET_ENTRY->REVISION didn't exist at
> > + RANGE->START or is unrelated to the resource
> > + PATH_at_RANGE->START. Either way we don't
> > + filter. */
> > + svn_error_clear(err);
> > + err = NULL;
> > + APR_ARRAY_PUSH(adjusted_rangelist,
> > + svn_merge_range_t *) = range;
> > + }
> > + else
> > + {
> > + return err;
> > + }
> > + }
> > + else
> > + {
> > + /* PATH_at_TARGET_ENTRY->REVISION exists on the same
> > + line of history at RANGE->START. But it might
> > + have existed under a different name then, so
> > + check if the URL it had then is the same as the
> > + URL for the mergeinfo we are trying to add. If
> > + it is the same we can filter it out. */
> > + if (strcmp(start_url, merge_source_url) != 0)
> > + {
> > + APR_ARRAY_PUSH(adjusted_rangelist,
> > + svn_merge_range_t *) = range;
> > + }
> > + }
> > + } /* for (j = 0; j < rangelist->nelts; j++) */
> > +
> > + /* If only some of the ranges mapped from SOURCE_PATH were
> > + filtered then create a new svn_prop_t to represent
> > + this. */
> > + if (adjusted_rangelist->nelts)
> > + {
> > + svn_stringbuf_t *adjusted_rangelist_sb;
> > + svn_prop_t *adjusted_prop =
> > + apr_pcalloc(pool, sizeof(*adjusted_prop));
> > +
> > + SVN_ERR(svn_rangelist_to_stringbuf(
> > + &adjusted_rangelist_sb, adjusted_rangelist,
> > + pool));
> > + adjusted_prop->name = SVN_PROP_MERGEINFO;
> > + adjusted_prop->value = svn_string_create(
> > + apr_pstrcat(pool, source_path, ":",
> > + adjusted_rangelist_sb->data, NULL),
> > + pool);
> > + APR_ARRAY_PUSH(adjusted_props, svn_prop_t) =
> > + *adjusted_prop;
>
> OK, now I feel really confused. If the mergeinfo you're looking at
> has a bunch of lines, this is going to split it into a bunch of
> different svn_prop_t's, one per line?
Yes...
> I must be missing something
> because I can't see how this can possibly work at all.
...and at first I thought you were right, this must be broken, but I
tested it out with multi-line mergeinfo and it works. Why, because
filter_self_referential_mergeinfo() returns an array of svn_prop_t's
to merge_props_changed() which calls svn_wc_merge_props2() to merge
the prop adds to the original props. Thing is, svn_wc_merge_props2()
works perfectly well even if multiple elements in the incoming prop
changes array refer to the same propname, e.g.
If we have this single elelment propchanges array:
Name Value
svn:mergeinfo '/A:6\n/A_COPY_2:9\n/A_COPY_3:10'
Or this semantically equivalent 3 element array:
Name Value
svn:mergeinfo '/A:6'
svn:mergeinfo '/A_COPY_2:9'
svn:mergeinfo '/A_COPY_3:10'
Then svn_wc_merge_props2() happily treats these the same way.
Now I admit this needs to be documented better or perhaps
reimplemented in a clearer way, but I'll hold off on that for now
until the whole question of using
svn_client__get_history_as_mergeinfo() is resolved, since a lot of
this code may go away.
> (Also,
> shouldn't there be a space after the ":" in the property value?)
No, our created mergeinfo has no whitespace. Although it appears our
parser allows whitespace when using svn propset, but then it doesn't
get interpreted correctly. I'll look into this some more and start a
separate thread if necessary.
> > + }
> > + } /* mergeinfo_catalog hash iteration */
> > + /* Reparent the WB->RA_SESSION1 to its original URL. */
> > + SVN_ERR(svn_ra_reparent(merge_b->ra_session1,
> > + original_ra_url, pool));
> > + } /* Property is non-empty mergeinfo. */
> > + } /* (i = 0; i < (*props)->nelts; ++i) */
> > + *props = adjusted_props;
> > + }
> > + return SVN_NO_ERROR;
> > +}
> > /* A svn_wc_diff_callbacks2_t function. Used for both file and directory
> > property merges. */
> > static svn_error_t *
>
>
> --
> David Glasser | glasser@davidglasser.net | http://www.davidglasser.net/
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: svn-unsubscribe_at_subversion.tigris.org
> For additional commands, e-mail: svn-help_at_subversion.tigris.org
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe_at_subversion.tigris.org
For additional commands, e-mail: dev-help_at_subversion.tigris.org
Received on 2008-02-04 18:41:32 CET