Hi Paul.
I have only had a chance to skim through your reply so far. I'm glad that you
find it to be an explicable and harmless difference.
I have assigned Issue #4217 to track this, as I want to be able to refer to it
before having a chance to study your reply properly.
- Julian
Paul Burba wrote:
> On Tue, Jul 31, 2012 at 1:50 PM, Julian Foad wrote:
>> I'm investigating a discrepancy that shows up, when 'symmetric
>> merge' is enabled [1], in the notifications printed by some merges.
>> Merge_tests 78 fails because the expected notification for the last
>> merge is:
>>
>> --- Merging r6 through r9 into 'H_COPY':
>> U H_COPY/psi
>> D H_COPY/nu
>> --- Recording mergeinfo for merge of r6 through r9 into 'H_COPY':
>> U H_COPY
>>
>> but the first line of that changes to:
>>
>> --- Merging r7 through r9 into 'H_COPY':
>>
>> while the rest stays the same. In this instance the difference is
>> probably harmless since r6 is a no-op in the merge source, but we
>> need to check whether this is a symptom of a wider problem.
>>
>>
>> NEW TEST FOR THIS ISSUE
>>
>> I have simplified the recipe into a new test, merge_tests-130 in
>> the attached patch [2]. I removed the '--force' option, and this
>> changes things such that the merge prints a separate notification
>> for the subtree. Also
>> in that patch are some debug prints in the merge code which help to
>> trace the source of the problem. A diagram of the test scenario:
>>
>> # merge -c4 A/D/H/nu@4 H_COPY/nu
>> # | merge A/D/H H_COPY
>> # | |
>> # A/D/H A---------------
>> # +-psi +---------M-----
>> # +-nu A---M-D
>> # H_COPY C----------G
>> # +-psi +----------+
>> # +-nu +-------G--+
>> # 1 2 3 4 5 6 wc wc
>> #
>> # Key: Add Mod Del Copy merGe
>>
>> The difference in outputs ('-': existing 1.7 merge, '+':
>> symmetric merge):
>>
>> I: CMD: svn merge ^/A/D/H H_COPY
>> I: DBG: merge.c:8568: 1: ...130/H_COPY: 3-6
>> I: DBG: merge.c:8568: 1: .../H_COPY/nu: 3,5-6
>> I: DBG: merge.c:8568: 2: ...130/H_COPY: 3-6
>> I: DBG: merge.c:8568: 2: .../H_COPY/nu: 3,5-6
>> I: DBG: merge.c:8568: 3: ...130/H_COPY: 3-6
>> -I: DBG: merge.c:8568: 3: .../H_COPY/nu: 3-6
>> +I: DBG: merge.c:8568: 3: .../H_COPY/nu: 3,5-6
>> -I: --- Merging r3 through r6 into 'H_COPY':
>> +I: --- Merging r4 through r6 into 'H_COPY':
>> I: U H_COPY/psi
>> -I: --- Merging r3 through r6 into 'H_COPY/nu':
>> +I: --- Merging r5 through r6 into 'H_COPY/nu':
>> I: C H_COPY/nu
>> I: --- Recording mergeinfo for merge of r3 through r6 into
>> 'H_COPY':
>> I: U H_COPY
>> I: --- Recording mergeinfo for merge of r3 through r6 into
>> 'H_COPY/nu':
>> I: G H_COPY/nu
>> I: --- Eliding mergeinfo from 'H_COPY/nu':
>> I: U H_COPY/nu
>> I: Summary of conflicts:
>> I: Tree conflicts: 1
>>
>> The immediate cause of the discrepancy is that the symmetric merge code
>> passes the YCA revision as the beginning of the range to be merged,
>> whereas the existing merge code passes revision '1' as the
>> beginning of
>> the range to be merged. At the beginning of the main loop in do_merge():
>>
>> - source = {loc1 = "/A/D/H@1", loc2 = "/A/D/H@6",
>> ancestral = TRUE}
>> + source = {loc1 = "/A/D/H@2", loc2 = "/A/D/H@6",
>> ancestral = TRUE}
>>
>> The underlying problem -- the reason why the merge code doesn't resolve
>> these two requests to the same result -- is deeper, involving the
>> function fix_deleted_subtree_ranges(). That function yields
>>
>> (child 'H_COPY/nu')->remaining_ranges = 3-6
>>
>> for the 1.7 merge and
>>
>> (child 'H_COPY/nu')->remaining_ranges = 3,5-6
>>
>> for the symmetric merge.
>>
>> Revision 4 has already been merged to 'H_COPY/nu', so it should not
>> be merged again; but that doesn't mean 'remaining_ranges = 3-6' is
>> wrong. fix_deleted_subtree_ranges() is designed to set a subtree's
>> remaining range to match its parent's remaining range if the subtree
>> doesn't need a separate editor drive. The function appears to be
>> concluding in the first case that the subtree doesn't need a separate
>> editor drive (since, over the range 3-6, the net result is simply to
>> delete the subtree), but in the second case that it does and should
>> be split into two sub-ranges.
>
> Hi Julian,
>
> I don't think there is a problem here, rather, as you point out above,
> it is a fundamental difference between 1.7 sync merges, which assume a
> starting rev of 1, and symmetric merges, which calculates the YCA and
> passes that as the starting rev to do_merge(). Different inputs to a
> function produce different outputs...nothing terribly shocking about
> that ;-)
>
> Specifically:
>
> When we reach fix_deleted_subtree_ranges we are asking different
> questions during a 1.7 merge vs. a symmetric merge.
>
> First let's look at a couple key parts of the doc string for
> adjust_deleted_subtree_ranges (which does the heavy lifting for
> fix_deleted_subtree_ranges), first:
>
> Since this function is only invoked for subtrees of the merge target, the
> guarantees afforded by normalize_merge_sources() don't apply - see the
> 'MERGEINFO MERGE SOURCE NORMALIZATION' comment at the top of this
> file. Therefore it is possible that PRIMARY_URL@REVISION1 and
> PRIMARY_URL@REVISION2 don't describe the endpoints of an unbroken line of
> history. The purpose of this helper is to identify these cases of broken
> history and adjust CHILD->REMAINING_RANGES in such a way we don't later
> try to describe nonexistent path/revisions to the merge report editor --
> see drive_merge_report_editor().
>
> So, we are trying to not break the editor drive. In the 1.7 case we
> ask adjust_deleted_subtree_ranges about this child:
>
> adjust_deleted_subtree_ranges(svn_client__merge_path_t *child =
> "H_COPY/nu"...,
> svn_client__merge_path_t *parent =
> "H_COPY"...,
> svn_revnum_t revision1 = 1,
> svn_revnum_t revision2 = 6,
> const char *primary_url = "^/A/D/H/nu",
>
> *but* neither ^/A/D/H/nu@1 or ^/A/D/H/nu@6 exist, so CHILD needs no
> special consideration during the editor drive, so we set the CHILD's
> remaining ranges as identical to it's parents. As the doc string for
> drive_merge_report_editor points out, this means we effectively ignore
> these children during the editor drive:
>
> ...
> Note: If the first svn_merge_range_t * element of some subtree
> child's remaining_ranges array is the same as the first range of
> that child's nearest path-wise ancestor, then the subtree child
> *will not* be described to the reporter.
> ...
>
> The symmetric merge however, is asking about a CHILD that exists at
> the start of the requested range, but not at the end (it is deleted):
>
> adjust_deleted_subtree_ranges(svn_client__merge_path_t *child =
> "H_COPY/nu"...,
> svn_client__merge_path_t *parent =
> "H_COPY"...,
> svn_revnum_t revision1 = 2,
> svn_revnum_t revision2 = 6,
> const char *primary_url = "^/A/D/H/nu",
>
> So per the guarantees of adjust_deleted_subtree_ranges we end up with
> 2 discrete remaining ranges for H_COPY/nu:
>
> If PRIMARY_URL@REVISION1 exists but PRIMARY_URL@REVISION2 doesn't,
> then find the revision 'N' in which PRIMARY_URL@REVISION1 was
> deleted. Leave the subset of CHILD->REMAINING_RANGES that
> intersects with REVISION1:(N - 1) as-is and set the subset of
> CHILD->REMAINING_RANGES that intersects with (N - 1):REVISION2 equal
> to PARENT->REMAINING_RANGES' intersection with (N - 1):REVISION2.
>
>> I'm not sure whether one of those conclusions is right and the other
>> wrong; maybe both are OK.
>
> I think everything is ok here, at least as far as the resulting merge,
> we should always get the same WC state AFAICT with either the
> symmetric merge or the 1.7 merge.
>
> The only drawback to the symmetric approach is that it performs two
> editor drives:
>
> Editor Drive #1: ^/merge_tests-130 rev=2 : ^/merge_tests-130 rev=3
> Editor Drive #2: ^/merge_tests-130 rev=3 : ^/merge_tests-130 rev=6
>
> The first is inoperative obviously, so there is no user visible output.
>
> While 1.7 performs only one:
>
> Editor Drive #1: ^/merge_tests-130 rev=2 : ^/merge_tests-130 rev=6
>
> Not sure how to avoid this extra editor drive. Possibly postponing
> the call to remove_noop_subtree_ranges() in
> do_mergeinfo_aware_dir_merge until *after* the call to
> fix_deleted_subtree_ranges() is all that is needed, but I haven't
> checked this out.
>
>> I'm not sure why there are two different conclusions.
>
> Again, different conclusions based on different inputs.
>
> --
> Paul T. Burba
