On Tue, May 1, 2012 at 4:53 AM, Julian Foad <julianfoad_at_btopenworld.com> wrote:
> I'm just trying to follow some existing code. The comment that I'm adding in this pseudo-patch is a stalled attempt to describe the following code block.
>
>
> [[[
> /* Helper for do_file_merge and do_directory_merge (by way of
> populate_remaining_ranges() for the latter).
>
> Determine what portions of SOURCE have already been merged
> to CHILD->ABSPATH and populate CHILD->REMAINING_RANGES with
> the ranges that still need merging.
>
> [...] TARGET_MERGEINFO is the working mergeinfo on CHILD.
> [...]
> */
>
> static svn_error_t *
> calculate_remaining_ranges(svn_client__merge_path_t *parent,
> svn_client__merge_path_t *child,
> const merge_source_t *source,
> svn_mergeinfo_t target_mergeinfo,
> const apr_array_header_t *implicit_src_gap,
> svn_boolean_t child_inherits_implicit,
> svn_ra_session_t *ra_session,
> svn_client_ctx_t *ctx,
> apr_pool_t *result_pool,
> apr_pool_t *scratch_pool)
> {
> const char *mergeinfo_path;
> const char *primary_url = (source->loc1->rev < source->loc2->rev)
> ? source->loc2->url : source->loc1->url;
> svn_mergeinfo_t adjusted_target_mergeinfo = NULL;
> svn_revnum_t child_base_revision;
>
> /* Determine which of the requested ranges to consider merging... */
> SVN_ERR(svn_ra__get_fspath_relative_to_root(ra_session, &mergeinfo_path,
> primary_url, result_pool));
>
> + /* [Let's see if we can describe the following if-else block.]
Hi Julian,
A few things to start. If you haven't already looked at issue #3432
and its corresponding test 'merge_tests.py 100: correctly consider
natural history gaps', then do so, because that describes the bug the
first half of the if block is addressing. Also see 'merge_tests.py
97: mergeinfo aware merges ignore natural history gaps' for another
example.
Ok, I'm sure you are aware of this note at the start of merge.c
(particularly item #2):
[[[
/* MERGEINFO MERGE SOURCE NORMALIZATION
*
* Nearly any helper function herein that accepts two URL/revision
* pairs expects one of two things to be true:
*
* 1. that mergeinfo is not being recorded at all for this
* operation, or
*
* 2. that the pairs represent two locations along a single line
* of version history such that there are no copies in the
* history of the object between the locations when treating
* the oldest of the two locations as non-inclusive. In other
* words, if there is a copy at all between them, there is only
* one copy and its source was the oldest of the two locations.
*
* We use svn_ra_get_location_segments() to split a given range of
* revisions across an object's history into several which obey these
* rules. For example, a merge between r19500 and r27567 of
* Subversion's own /tags/1.4.5 directory gets split into sequential
* merges of the following location pairs:
*
* [/trunk:19549, /trunk:19523]
* (recorded in svn:mergeinfo as /trunk:19500-19523)
*
* [/trunk:19523, /branches/1.4.x:25188]
* (recorded in svn:mergeinfo as /branches/1.4.x:19524-25188)
*
* [/branches/1.4.x:25188, /tags/1.4.4_at_26345]
* (recorded in svn:mergeinfo as /tags/1.4.4:25189-26345)
*
* [/tags/1.4.4_at_26345, /branches/1.4.5_at_26350]
* (recorded in svn:mergeinfo as /branches/1.4.5:26346-26350)
*
* [/branches/1.4.5_at_26350, /tags/1.4.5_at_27567]
* (recorded in svn:mergeinfo as /tags/1.4.5:26351-27567)
*
* Our helper functions would then operate on one of these location
* pairs at a time.
*/
]]]
So the now ubiquitous merge_source_t's we pass around in merge.c may represent:
For the sake of convenience assume:
A = source->loc1->url
X = source->loc1->rev
B = source->loc2->url
Z = source->loc2->rev
1) A_at_X---------------->B_at_Z
Two locations along a single line
of history such with no copies.
2) A_at_X-------B@Y------>B_at_Z
A_at_X was copied to B_at_Y where
X < Y <= Z
When recording mergeinfo describing a merge we need to consider that,
in the second situation, the segment described by,
A_at_X------->B@(Y-1)
might not exist or might exist but be part of another line of history.
Getting back to calculate_remaining_ranges(), we need to:
[[[
...
Determine what portions of SOURCE have already
been merged to CHILD->ABSPATH and populate CHILD->REMAINING_RANGES with
the ranges that still need merging.
...
]]]
We certainly don't want to consider A_at_X------->B@(Y-1) as already
merged if that segment is *not* part of SOURCE and therefore should
not be merged or recorded as merged (or unmerged) It should be
ignored completely as it has nothing to do with the merge being done.
Considering A_at_X--->B@(Y-1) as eligible for merging can break the
merge, as described in
http://svn.haxx.se/dev/archive-2008-11/0618.shtml Considering that
segment as merged during a reverse merge "works" (i.e. the merge
doesn't fail) but can remove legitimate mergeinfo from previous merges
as described in
http://subversion.tigris.org/issues/show_bug.cgi?id=3432#desc1
In r1332798 I tweaked the comment around this block and the doc string
for. Does the above and that change help clarify this?
Paul
> + Set ADJUSTED_TARGET_MERGEINFO to ... let's see ... in some cases,
> + the portion of CHILD's pre-merge mergeinfo that represents merges
> + from the branch-segment SOURCE. If CHILD has no mergeinfo that
> + refers to SOURCE, this could leave it as NULL or set it to an
> + empty hash. In other cases, set it to TARGET_MERGEINFO -- that is,
> + CHILD's current working mergeinfo, in its entirety.
>
> +
> + [That doesn't seem too coherent. Maybe we can better describe this
> + code block in terms of preparing for the next step:]
> +
>
> + We need to pass mergeinfo in to filter_merged_revisions(), which says
> + its TARGET_MERGEINFO input should be "the CHILD->ABSPATH's explicit
> + or inherited mergeinfo [NULL if none, empty if empty]". So ...
> +
>
> + [But we're calculating something more specific here. The doc string on
> + filter_merged_revisions() doesn't say, but apparent from this code (its
> + only caller) is that it should not always be passed the whole mergeinfo
> + of CHILD.]
> + */
> /* Consider: CHILD might have explicit mergeinfo '/MERGEINFO_PATH:M-N'
> where M-N fall into the gap in SOURCE's natural
> history allowed by 'MERGEINFO MERGE SOURCE NORMALIZATION'. If this is
> the case, then '/MERGEINFO_PATH:N' actually refers to a completely
> different line of history than SOURCE and we
> *don't* want to consider those revisions merged already. */
> if (implicit_src_gap && child->pre_merge_mergeinfo)
> {
> apr_array_header_t *explicit_mergeinfo_gap_ranges =
> apr_hash_get(child->pre_merge_mergeinfo, mergeinfo_path,
> APR_HASH_KEY_STRING);
>
> if (explicit_mergeinfo_gap_ranges)
> {
> svn_mergeinfo_t gap_mergeinfo = apr_hash_make(scratch_pool);
>
> apr_hash_set(gap_mergeinfo, mergeinfo_path, APR_HASH_KEY_STRING,
> implicit_src_gap);
> SVN_ERR(svn_mergeinfo_remove2(&adjusted_target_mergeinfo,
> gap_mergeinfo, target_mergeinfo,
> FALSE, result_pool, scratch_pool));
> }
> }
> else
> {
> adjusted_target_mergeinfo = target_mergeinfo;
> }
>
> /* Initialize CHILD->REMAINING_RANGES and filter out revisions already
> merged (or, in the case of reverse merges, ranges not yet merged). */
> SVN_ERR(filter_merged_revisions(parent, child, mergeinfo_path,
> adjusted_target_mergeinfo,
> source->loc1->rev, source->loc2->rev,
> child_inherits_implicit,
> ra_session, ctx, result_pool,
> scratch_pool));
>
> [...]
> }
>
> - Julian
>
Received on 2012-05-01 21:54:09 CEST