On Wed, Aug 6, 2008 at 7:29 AM, Julian Foad <julianfoad_at_btopenworld.com> wrote:
> On Tue, 2008-08-05 at 15:57 +0100, Julian Foad wrote:
>> On Fri, 2008-07-25 at 12:52 +0100, Julian Foad wrote:
>> > On Thu, 2008-07-24 at 11:57 -0400, Paul Burba wrote:
>> > > Now we try the reverse merge, and as in < 1.5 the files scheduled for
>> > > addition are skipped:
>> > >
>> > > 1.5.1>svn.exe merge -r3:2 %URL%/branches/mybranch
>> > > Skipped 'newfile1'
>> > > Skipped 'newfile2'
>> > > Skipped 'newfile3'
>> >
>> > A good enhancement will be to compare the locally-added file with the
>> > one that the incoming change thinks it's deleting (i.e. the merge-left
>> > source), and only skip if they're different. That check is something
>> > we're doing for tree conflict detection anyway, but can be done
>> > independently.
>> >
>> > I'll have a go at a patch to make this happen. (It so happens that this
>> > is what I am about to work on for tree conflicts anyway.) I haven't
>> > missed any reason why we'd not want to change this, have I?
>>
>> Here we go.
>>
>> Can anyone take a look at the attached patch and comment on the places
>> I've flagged with "###" and generally on whether it's going the right
>> way?
>>
>> Much appreciated...
>
> 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:
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.
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.
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.
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().
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.
Paul
---------------------------------------------------------------------
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-06 23:54:16 CEST