[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

RE: svn commit: r26803 - in trunk/subversion: libsvn_client tests/cmdline

From: Paul Burba <pburba_at_collab.net>
Date: 2007-10-05 18:25:17 CEST

> -----Original Message-----
> From: Kamesh Jayachandran
> Sent: Friday, October 05, 2007 6:17 AM
> To: Paul Burba
> Cc: dev@subversion.tigris.org; Lieven Govaerts
> Subject: Re: svn commit: r26803 - in trunk/subversion:
> libsvn_client tests/cmdline
>
> Paul,
>
> > But if it did not match on Linux, why has this test been passing on
> > Linux?
> >
> It passes on Linux because, verify.createExpectedOutput
> creates 'RegexOutput' object with 'match_all' being false and
> hence it tries to match atleast on line line in the output.
> In linux
>
> 'D
> svn-test-work/working_copies/merge_tests-63/A_copy/D/gamma'
> matches our regex and hence we get the PASS.
>
> In windows
>
> 'D
> svn-test-work\working_copies\merge_tests-63\A_copy\D\gamma'
> does not match our regex and hence we get the FAILURE.
>
> The cause of this difference is when we author a regex we
> need to escape '\' with '\\' when combined with our python
> escaping we need to replace '\\' with '\\\\'.
>
> I tried this concept in win32 box and it proves correct,
> Committed the same in r26945.

Thanks for the explanation Kamesh! Now I finally got my head around
that part of the problem.

I tweaked r26945 a bit further in r26949, moving the '\' escaping logic
from merge_fails_if_subtree_is_deleted_on_src() into
expected_merge_output(). No other test needs it now, but might as well
make it available for future tests.

> Currently the notification happens from do_merge based on the
> revisions we give from discover_and_merge_children. Actual
> decision of which range to merge happens inside the
> drive_merge_report_editor. If I move this logic to
> discover_and_merge_children, we can fix the notification.

Is it possible instead to move the logic determining the notification
ranges from discover_and_merge_children() into
drive_merge_report_editor()? Just thinking out loud...
 
> But I am concerned about the following case.
>
> Consider the case where the 'target' has mergeinfo
> '/src:7-10' and 'target/child' has mergeinfo '/src/child:9
> (This is possible via reverts).
> If we merge -r6:11 of /src,
> Currently it would notify
>
> --- Merging r7 through r8 into 'target':
> --- Merging r9 through r11 into 'target':

This second notification is *not* how trunk currently behaves. That is
part of problem I am pointing out, it would be this instead:

  --- Merging r8 through r11 into 'target':
            ^^
See below for an example of this.
 
> I think the current behaviour is correct "Merging r7 through r8 into
> 'target'"(no-op) as it means a possible merge on child.
>
> If we tweak this to "--- Merging r11 into 'target': ", It
> would be wrong again as it does a merge of 'r6:8' on target/child.

Agreed, but I'm not saying that, my example had no subtree so is not
analogous to yours.

Let's for a moment say that all our notifications report we are merging
into 'target' even though we may really be merging only into a subtree
of target's (I don't think this is correct, but if I'm a lone voice in
the wilderness on this I won't fight it).

>svn st merge_tests-1 -v
                1 1 jrandom merge_tests-1
                1 1 jrandom merge_tests-1\A
                1 1 jrandom merge_tests-1\A\B
                1 1 jrandom merge_tests-1\A\B\lambda
                1 1 jrandom merge_tests-1\A\B\E
                1 1 jrandom merge_tests-1\A\B\E\alpha
                5 5 jrandom merge_tests-1\A\B\E\beta
                1 1 jrandom merge_tests-1\A\B\F
                1 1 jrandom merge_tests-1\A\mu
                1 1 jrandom merge_tests-1\A\C
                1 1 jrandom merge_tests-1\A\D
                1 1 jrandom merge_tests-1\A\D\gamma
                1 1 jrandom merge_tests-1\A\D\G
                7 7 pburba merge_tests-1\A\D\G\pi
                4 4 jrandom merge_tests-1\A\D\G\rho
                8 8 pburba merge_tests-1\A\D\G\tau
                1 1 jrandom merge_tests-1\A\D\H
                9 9 pburba merge_tests-1\A\D\H\chi
                6 6 jrandom merge_tests-1\A\D\H\omega
                3 3 jrandom merge_tests-1\A\D\H\psi
               10 10 pburba merge_tests-1\A_COPY
                2 2 jrandom merge_tests-1\A_COPY\B
                2 2 jrandom merge_tests-1\A_COPY\B\lambda
                2 2 jrandom merge_tests-1\A_COPY\B\E
                2 2 jrandom merge_tests-1\A_COPY\B\E\alpha
               10 10 pburba merge_tests-1\A_COPY\B\E\beta
                2 2 jrandom merge_tests-1\A_COPY\B\F
                2 2 jrandom merge_tests-1\A_COPY\mu
                2 2 jrandom merge_tests-1\A_COPY\C
               11 11 pburba merge_tests-1\A_COPY\D
                2 2 jrandom merge_tests-1\A_COPY\D\gamma
                2 2 jrandom merge_tests-1\A_COPY\D\G
                2 2 jrandom merge_tests-1\A_COPY\D\G\pi
                2 2 jrandom merge_tests-1\A_COPY\D\G\rho
                2 2 jrandom merge_tests-1\A_COPY\D\G\tau
                2 2 jrandom merge_tests-1\A_COPY\D\H
                2 2 jrandom merge_tests-1\A_COPY\D\H\chi
               11 11 pburba merge_tests-1\A_COPY\D\H\omega
                2 2 jrandom merge_tests-1\A_COPY\D\H\psi
                1 1 jrandom merge_tests-1\iota

# I've set up a scenario similar to what you described
# above, a path 'A_COPY', with a subtree 'A_COPY\D'
# which has a subset of of 'A_COPY's mergeinfo.
>svn pl -vR merge_tests-1
Properties on 'merge_tests-1\A_COPY':
  svn:mergeinfo : /A:1,4-8
Properties on 'merge_tests-1\A_COPY\D':
  svn:mergeinfo : /A/D:1,6
 
# Now with trunk (r26949) if we do a merge of
# ranges which are a superset of the 'A_COPY's
# mergeinfo we get this (some newlines added for
# readability):
>svn merge -r2:10 %URL%/A merge_tests-1\A_COPY

# merging r3 into A_COPY *and* A_COPY\D
--- Merging r3 into 'merge_tests-1\A_COPY':
U merge_tests-1\A_COPY\D\H\psi

# merging r4-5 into A_COPY\D *only*
# It would be better if the notification reflected this
# --- Merging r4 through r5 into 'merge_tests-1\A_COPY\D':
# no?
--- Merging r4 through r5 into 'merge_tests-1\A_COPY':
U merge_tests-1\A_COPY\D\G\rho

# merging r7-10 into A_COPY and A_COPY\D
--- Merging r6 through r10 into 'merge_tests-1\A_COPY':
U merge_tests-1\A_COPY\D\G\pi
U merge_tests-1\A_COPY\D\G\tau
U merge_tests-1\A_COPY\D\H\chi

# Using my patch from
# http://svn.haxx.se/dev/archive-2007-10/0216.shtml
# The results are the same, except for the final
# notification, which correctly reports r7 rather
# than r6 as the inclusive start of the rev range.
>svn merge -r2:10 %URL%/A merge_tests-1\A_COPY
--- Merging r3 into 'merge_tests-1\A_COPY':
U merge_tests-1\A_COPY\D\H\psi
--- Merging r4 through r5 into 'merge_tests-1\A_COPY':
U merge_tests-1\A_COPY\D\G\rho
--- Merging r7 through r10 into 'merge_tests-1\A_COPY':
U merge_tests-1\A_COPY\D\G\pi
U merge_tests-1\A_COPY\D\G\tau
U merge_tests-1\A_COPY\D\H\chi

> Should we notify on each 'set_path'(non-noop) call in
> drive_merge_report_editor?

I hesitate to answer that without looking at the code more.
 
> > >svn pl -vR merge_tests-1
> > Properties on 'merge_tests-1\A_COPY':
> > svn:mergeinfo : /A:1
> >
> > ### WE EXPECT AND SEE NOTIFICATION FOR r4-5 INCLUSIVE...
> > >svn merge -r3:5 %URL%/A/D merge_tests-1\A_COPY\D
> > --- Merging r4 through r5 into 'merge_tests-1\A_COPY\D':
> > U merge_tests-1\A_COPY\D\G\rho
> >
> > >svn pl -vR merge_tests-1
> > Properties on 'merge_tests-1\A_COPY':
> > svn:mergeinfo : /A:1
> > Properties on 'merge_tests-1\A_COPY\D':
> > svn:mergeinfo : /A/D:1,4-5
> >
> > ### ...NOW REPEAT A SUPERSET OF THE PREVIOUS MERGE RANGE
> > >svn merge -r2:6 %URL%/A/D merge_tests-1\A_COPY\D
> > ### THIS LOOKS FINE, JUST r2
> > --- Merging r3 into 'merge_tests-1\A_COPY\D':
> > U merge_tests-1\A_COPY\D\H\psi
> > ### BUT SHOULDN'T THIS BE *JUST* r6 IN THE NOTIFICATION?
> > --- Merging r4 through r6 into 'merge_tests-1\A_COPY\D':
> >
>
> Hope the explanation above answers this.

Somewhat, but again in my example above there is no subtree under the
merge target with it's own mergeinfo, so we are talking about two
different cases. And regardless, I have trouble convincing myself that
the notification in my example above is anything but wrong, or at best
misleading -- particularly considering that
discover_and_merge_children() *does* adjust the notification end
revisions by calling get_nearest_end_rev() or get_farthest_end_rev().
This can be seen in my example below, there the start and end
notifications are accurate with the actual merges being performed.

> > U merge_tests-1\A_COPY\D\H\omega
> >
> > ### AT LEAST THE MERGEINFO IS RIGHT :-)
> > >svn pl -vR merge_tests-1
> > Properties on 'merge_tests-1\A_COPY':
> > svn:mergeinfo : /A:1
> > Properties on 'merge_tests-1\A_COPY\D':
> > svn:mergeinfo : /A/D:1,3-6
> >
> > Kamesh is looking into this problem...and speaking of, I
> did notice that
> > in the case where the second merge range isn't a superset,
> but rather an
> > intersection with the existing mergeinfo *and* the ranges
> still to be
> > merged are greater than the intersecting ranges, then the
> notification
> > works:
> >
> > >svn merge -r4:6 %URL%/A/D merge_tests-1\A_COPY\D
> > --- Merging r5 through r6 into 'merge_tests-1\A_COPY\D':
> > U merge_tests-1\A_COPY\D\H\omega
> >
> > >svn pl -vR merge_tests-1
> > Properties on 'merge_tests-1\A_COPY':
> > svn:mergeinfo : /A:1
> > Properties on 'merge_tests-1\A_COPY\D':
> > svn:mergeinfo : /A/D:1,5-6
> >
> > >svn merge -r2:6 %URL%/A/D merge_tests-1\A_COPY\D
> > ### r5-6 ALREADY PRESENT, SO THE
> > ### NOTIFICATION ONLY REPORTS r3-4
> > --- Merging r3 through r4 into 'merge_tests-1\A_COPY\D':
> > U merge_tests-1\A_COPY\D\G\rho
> > U merge_tests-1\A_COPY\D\H\psi
> >
> > >svn pl -vR merge_tests-1
> > Properties on 'merge_tests-1\A_COPY':
> > svn:mergeinfo : /A:1
> > Properties on 'merge_tests-1\A_COPY\D':
> > svn:mergeinfo : /A/D:1,3-6
> >
> > We still see the notification problem when the ranges
> intersect but the
> > remaining ranges are less than the intersecting ranges:
> >
> > >svn merge -r2:4 %URL%/A/D merge_tests-1\A_COPY\D
> > --- Merging r3 through r4 into 'merge_tests-1\A_COPY\D':
> > U merge_tests-1\A_COPY\D\G\rho
> > U merge_tests-1\A_COPY\D\H\psi
> >
> > >svn pl -vR merge_tests-1
> > Properties on 'merge_tests-1\A_COPY':
> > svn:mergeinfo : /A:1
> > Properties on 'merge_tests-1\A_COPY\D':
> > svn:mergeinfo : /A/D:1,3-4
> >
> > >svn merge -r2:6 %URL%/A/D merge_tests-1\A_COPY\D
> > ### SHOULD BE 'r5 through r6'
> > --- Merging r3 through r6 into 'merge_tests-1\A_COPY\D':

Summing up, at present I see two problems:

1) We have inconsistent start range notifications, sometimes the
notification range represents the acutal merge being performed,
sometimes it does not. Not sure how to spin that as other than a flaw
:-(

2) Our notifications always show the merge target as the destination,
even if the range in question is only being merged into one of the
target's subtrees with differing mergeinfo. IMHO this is less serious,
but does make for somewhat confusing outout.

Paul

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Fri Oct 5 18:29:26 2007

This is an archived mail posted to the Subversion Dev mailing list.

This site is subject to the Apache Privacy Policy and the Apache Public Forum Archive Policy.