On Wed, 2008-08-06 at 17:53 -0400, Paul Burba wrote:
> On Wed, Aug 6, 2008 at 7:29 AM, Julian Foad <julianfoad_at_btopenworld.com> wrote:
> > Here's an improved version. It now compares the properties as well as
> > the text content. It ignores the "svn:mergeinfo" property because that
> > property does not contribute to a difference in the file's current state
> > but only tells how the file reached its current state.
>
> +1
>
> > Just one test fails: merge_tests.py 77. It looks like the reverse-merge
> > "svn merge -r9:2 A A_COPY" is omitting r8 from the revision ranges it
> > thinks it needs to handle. r8 was a text mod to A/D/H/nu. Is this
> > because Subversion thinks, "there's no point merging r8 (a text mod)
> > because I can see that I would follow that with deleting the file, so it
> > would be a waste of effort"?
>
> Your guess is pretty much spot on, r8 is being ignored, as far as 'nu'
> is concerned, because 'nu' will be deleted anyway when r7 is reverse
> merged. Well, to be accurate, it's not doing this simply because 'nu'
> will be deleted, rather it is taking advantage of that fact as part of
> the fix for issue #3067, which tries to avoid describing
> path/revisions to the editor which don't actually exist.
>
> It might help to walk through exactly what is happening in merge_tests.py 77:
Thanks for all this, Paul.
> Right before we reverse merge -r9:2 from 'A' to 'A_COPY', the tree
> rooted at 'A_COPY' has the following paths explicit mergeinfo:
>
> Path Mergeinfo
> 1) The merge target itself : A_COPY : /A:5-6
> 2) A subtree with mergeinfo : A_COPY\D\H : /A/D/H:5-7
> 3) A subtree with mergeinfo : A_COPY\D\H\nu : /A/D/H/nu:5-8
>
> (Forgive me if I over-explain but I'm not sure how much you have
> looked at these parts of the merge code)
>
> Each of these three paths are stored in the ubiquitous
> children_with_mergeinfo array in a svn_client__merge_path_t struct
> which contains a slew of merge-related info about the path, including
> the remaining_ranges to be merged.
>
> The merge looks at each path (this is all happening in
> lbisvn_client/merge.c:populate_remaining_ranges() and its many
> helpers) and determines what revisions of the requested r9:2 should be
> reverse merged into that path.
OK.
> Note, that since this *is* a reverse merge we only consider revisions
> which are present in the implicit mergeinfo (i.e. natural history) or
> its inherited/explicit mergeinfo.
I didn't quite follow this sentence at first, but I think I get it now.
Your mention of "natural history" refers to the general case where we
might be doing a reverse-merge from our branch's own history, but that's
not what we're doing in this case.
We are reverse-merging changes r9:2 from a source branch "A" to a
destination branch "A_COPY". The only revisions we could possibly
consider merging are those within 9:2 that were operative in the source
branch's natural history (the revisions in which any kind of change was
committed to this tree). In this test, "svn log" shows that those are
A/: r3,4,5,6,7,8; A/D/H: r3,6,7,8; A/D/H/nu: r7,8.
I think are saying that our merge-tracking logic deems that, of those
possible source revisions, the only ones that it makes sense to
reverse-merge are those that the mergeinfo says are present in the
target. (For A_COPY/, those are r5,6. For A_COPY/D/H/nu, the
intersection is r7,8.)
> First 'A_COPY', this is simple, it needs r5 and r6 reverse merged.
>
> Next 'A_COPY\D\H', again this is straightforward, needing r5, r6, and
> r7 reverse merged.
>
> Lastly 'A_COPY\D\H\nu', this is where things get slightly more
> complicated. On the face of it (going simply by the mergeinfo) this
> path needs r5-r8 reverse merged. But 'nu' doesn't exist in the merge
> source until r7 so we don't want to reverse merge r5 or r6 into 'nu'
> directly as this would break the editor since 'nu' doesn't exist in
> the source at those revisions.
Sure, that's clear.
> I don't want to bury you in #3067 minutiae but now might be a good
> time to read the doc string for merge.c:prepare_subtree_ranges().
Urg. :-) Comments/review, just of the doc string:
Can you add a sentence that says what the REQUESTED_RANGELIST parameter
is for and what its element type is? There are partial descriptions of
it in Note1, Note2 and some of the 8 cases, but an overall statement
about it would be helpful. Maybe something like:
Set *REQUESTED_RANGELIST to a newly allocated array of
unordered (svn_merge_range_t) elements representing the
revision ranges in which the node PRIMARY_URL_at_REVISION1
existed, plus any range in which its parent existed before
it did, between REVISION1 and REVISION2. ("Before" being
relative to the direction REVISION1 >> REVISION2.) (???)
Sorry, that attempt is surely wrong and unhelpful!
Intro: "Filter the requested ranges" -> "Filter the requested range
REVISION1:REVISION2"?
Intro: "Note in *CHILD_DELETED_OR_NONEXISTANT if the subtree doesn't
exist, is deleted, or is renamed in the requested range." Say, "or is
renamed or replaced". Should this say, "doesn't exist at all in the
requested range, or is deleted, renamed or replaced before the end of
the range"? What I'm trying to get at is it's not clear about how the
object is allowed to come into existence after the start of the range.
Case B: I'm afraid I can't follow the description. "Ranges ... at which
PRIMARY_URL exists": meaning all ranges in which any node exists at
PRIMARY_URL? If there are two or more, does it matter that these might
be history segments of different objects? Then, adding a segment in
which it does not exist: is that behaviour specific to a particular
requirement that the current caller has in this case, or something that
can be explained more generally? "PARENT->REQUESTED_RANGELIST": do you
mean PARENT->REMAINING_RANGES?
Case D says "not replaced within that range". Whereas case H is
complemented by case F which covers the broken-line-of-history variant,
case D is not complemented by such a case.
Case E: The "###" note: Urg? Sorry, overwhelmed and baffled :-0
> What we have merge_tests.py 77 is described in case 'F' in that doc
> string. prepare_subtree_ranges() figures out that 'nu' will be
> deleted by the merge, so doesn't do any filering of the requested
> ranges, rather it reports to its caller filter_merged_revisions() that
> 'nu' is going to be deleted. Now see "A little trick" in
> filter_merged_revisions() -- It sees that the subtree 'nu' will be
> deleted, so it sets its remaining_ranges to be the same as its nearest
> parent in the children_with_mergeinfo array, namely 'A_COPY/D/H', so
> r8 is never reverse merged, which obviously breaks your patch in this
> case.
>
> I'm playing around with some tweaks to filter_merged_revisions() and
> prepare_subtree_ranges()'s behaviors in case F (and maybe it's forward
> merge equivalent described in case A) to see if your patch and the
> issue #3067 fixes can coexist.
Cool. Thanks.
May I commit my patch with this one case not yet working? Or, less
onerously, do you agree that the merge_file_deleted method is being
called incorrectly and we should look to fix it as a separate exercise?
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe_at_subversion.tigris.org
For additional commands, e-mail: dev-help_at_subversion.tigris.org
Received on 2008-08-07 13:34:31 CEST