On Thu, Dec 15, 2011 at 10:15 AM, Paul Burba <ptbu...@gmail.com> wrote:
> On Thu, Dec 15, 2011 at 7:53 AM, Julian Foad <julianf...@btopenworld.com>
> wrote:
>> Thanks Paul.
>>
>> Paul Burba wrote:
>>
>>> Julian Foad wrote:
>>>> Here's a patch to reject silly merge attempts, which up to now give
>>>> silly results.
>>>>
>>>> This does not apply to all merges (general 2-URL and cherry-pick
>>>> merges), but the commonly used 'sync' and 'reintegrate' forms of
>>>> merge only make sense when the source and target 'branches' are
>>>> related (have a common ancestor) and are not the same.
>>>>
>>>> This patch applies such checks to the 'reintegrate' merge, simple
>>>> 'sync' merge, and the 'svn mergeinfo' command.
>>>
>>> This premise of this patch seems reasonable to me. I don't see that
>>> it thwarts any legitimate use cases. We'll longer be able to do
>>> things like update a WC target to a rev N<HEAD then 'sync' all the
>>> changes from N+1:HEAD, e.g.:
>> [...]
>>> But was this ever useful? I don't see how. We can still do this *if*
>>> we specify an actual revision/revision range.
>>
>> This "sync merge from my own history" operation seems bogus.
>
> Ok, you call it "bogus", I question if it has any practical use -- I
> think we essentially agree :-)
>
>>I notice that (without my patch) it merges changes from the future *and*
>>records mergeinfo for them. Surely we didn't ever intend that?
>
> I don't recall that we ever *planned* to record mergeinfo in these
> cases, it is simply a side-effect of how we record mergeinfo to
> describe any merge.
>
>> $ svn up -r1214700
>> Updating '.':
>> At revision 1214700.
>>
>> $ svn merge ^/subversion/trunk
>> --- Merging r1214701 through r1214711 into '.':
>> U subversion/libsvn_fs_fs/fs_fs.c
>> --- Recording mergeinfo for merge of r1214701 through r1214711 into '.':
>> U .
>>
>> $ svn diff
>> [...]
>> Modified: svn:mergeinfo
>> Merged /subversion/trunk:r1214701-1214711
>>
>> We don't ever intend to record self-referential mergeinfo, do we?
>
> No, but this isn't self-referential mergeinfo. Just in case we have
> different definitions, here's is how I've always defined
> self-referential mergeinfo:
>
> Implicit Mergeinfo: A path's *past* history described as mergeinfo.
> Self-Referential Mergeinfo: Explicit mergeinfo that is redundant with
> implicit mergeinfo
>
> In your example the implicit mergeinfo for the path (i.e. a WC for
> ^/subversion/trunk@1214700) in question is
> '/subversion/trunk:836420-1214700' (r836420 being the rev
> ^/subversion/trunk was created). The new mergeinfo recorded to
> describe the merge isn't from the merge target's past history, so it's
> not self-referential.
>
> Let's divorce this question from the sync merge use case and show
> where it is correct. Assume we have a ^/branchs/projX where
> HEAD=1000.
>
> We update a WC for projX back to an older revision:
>
> \SVN\projX-WC>svn up -r900 -q
>
> We merge some of the target's *future* history to it, using explicit
> revisions (no sync):
>
> \SVN\projX-WC>svn merge ^/branches -c 907,912 -q
>
> Then we copy the result:
>
> \SVN\projX-WC>svn copy . ..\projX.2-WC
>
> Obviously the mergeinfo on the copy destination is correct and not
> self-referential:
>
> \SVN\projX-WC>svn pg svn:mergeinfo ..\projX.2-WC
> Properties on '\SVN\projX.2-WC':
> svn:mergeinfo
> /branches/projX:907,912
>
> I admit this example is a bit contrived, but I hope it demonstrates
> that what you are calling self-referential is not. Of course if in my
> above example we updated the WC, rather than copying it, then yes, the
> mergeinfo is self-referential. All we need to do if read the user's
> mind :-)
>
>> I'm assuming that's bogus and the merge should really be rejected or at
>> least the mergeinfo should be elided.
>
> Elision, at least how we've historically used that term as relates to
> mergeinfo, has little to do with this example. Elision is the removal
> of explicit mergeinfo on a subtree when that mergeinfo is equivalent
> to what that subtree would inherit from its nearest parent with
> explicit mergeinfo.
>
>>>> A few tests currently fail with this patch -- tests that use the
>>>> special "svn merge FILE[@REV]" syntax. I'm currently investigating
>>>> this and learning what it's supposed to be doing.
>>>
>>> I assume you mean these two merge tests?
>>>
>>> 6 merging a file w/no explicit target path or revs [#785]
>>> 12 merge one file without explicit revisions
>>>
>>> Those fail because of the new limitation I described above. Again, I
>>> don't think this is necessarily a bad thing.
>>
>> I'm struggling to see what we expected this kind of syntax to do and why.
>> It seems to have been committed mainly in r864853 and I'm seeing a long
>> email thread <http://svn.haxx.se/dev/archive-2007-04/0957.shtml> about it.
>> I'm not happy about simply removing those test cases until I understand a
>> little better what's wanted.
Avoiding reading five year old threads ;-) and looking at what
merge_tests.py does today:
### The repos has two revs:
C:\SVN\src-trunk\Debug\subversion\tests\cmdline\svn-test-work\working_copies\merge_tests-6.other\A>svn
st -v
1 1 jrandom .
1 1 jrandom mu
1 1 jrandom B
1 1 jrandom B\lambda
1 1 jrandom B\E
1 1 jrandom B\E\alpha
1 1 jrandom B\E\beta
1 1 jrandom B\F
1 1 jrandom C
1 1 jrandom D
1 1 jrandom D\gamma
1 1 jrandom D\G
1 1 jrandom D\G\rho
1 1 jrandom D\G\pi
1 1 jrandom D\G\tau
1 1 jrandom D\H
1 1 jrandom D\H\chi
1 1 jrandom D\H\omega
1 1 jrandom D\H\psi
### Our WC target is at r1:
C:\SVN\src-trunk\Debug\subversion\tests\cmdline\svn-test-work\working_copies\merge_tests-6.other\A>svn
log -v -r1:HEAD ^^/
------------------------------------------------------------------------
r1 | jrandom | 2011-12-15 10:20:49 -0500 (Thu, 15 Dec 2011) | 1 line
Changed paths:
A /A
A /A/B
A /A/B/E
A /A/B/E/alpha
A /A/B/E/beta
A /A/B/F
A /A/B/lambda
A /A/C
A /A/D
A /A/D/G
A /A/D/G/pi
A /A/D/G/rho
A /A/D/G/tau
A /A/D/H
A /A/D/H/chi
A /A/D/H/omega
A /A/D/H/psi
A /A/D/gamma
A /A/mu
A /iota
Log message for revision 1.
------------------------------------------------------------------------
r2 | jrandom | 2011-12-15 10:20:49 -0500 (Thu, 15 Dec 2011) | 1 line
Changed paths:
M /A/mu
log msg
------------------------------------------------------------------------
### We try to sync merge future changes to a file
### 'mu' is interpreted as the merge source and because it is a
### file we assume the target is a file of the same name -- the Subversion book
### describes the expected behavior this way:
###
### "svn merge requires a working copy path as a target, that is, a
place where it
### should apply the generated patch. If the target isn't specified,
it assumes you
### are trying to perform one of the following common operations:
### * You want to merge directory changes into your current working directory.
### * You want to merge the changes in a specific file into a file by
the same name that exists in your current working directory."
###
### And as we have discussed, your patch prevents this 'future sync
with our own history':
C:\SVN\src-trunk\Debug\subversion\tests\cmdline\svn-test-work\working_copies\merge_tests-6.other\A>svn
merge mu
DBG: util.c:1462: ancestor:
file:///C:/SVN/src-trunk/Debug/subversion/tests/cmdline/svn-test-work/repositories/merge_tests-6/A/mu@1
DBG: util.c:1470: url1:
file:///C:/SVN/src-trunk/Debug/subversion/tests/cmdline/svn-test-work/repositories/merge_tests-6/A/mu@1
DBG: util.c:1471: url2:
file:///C:/SVN/src-trunk/Debug/subversion/tests/cmdline/svn-test-work/repositories/merge_tests-6/A/mu@1
..\..\..\subversion\svn\main.c:2684: (apr_err=205000)
svn: E205000: Try 'svn help' for more info
..\..\..\subversion\svn\merge-cmd.c:370: (apr_err=205000)
..\..\..\subversion\svn\util.c:1487: (apr_err=205000)
svn: E205000: This merge requires two different but related branches
..\..\..\subversion\svn\util.c:1473: (apr_err=205000)
svn: E205000: Source and target are the same branch:
'file:///C:/SVN/src-trunk/Debug/subversion/tests/cmdline/svn-test-work/repositories/merge_tests-6/A/mu@1'
and
'file:///C:/SVN/src-trunk/Debug/subversion/tests/cmdline/svn-test-work/repositories/merge_tests-6/A/mu@1'
### It's the same result as if we spelled out both the source and the target:
C:\SVN\src-trunk\Debug\subversion\tests\cmdline\svn-test-work\working_copies\merge_tests-6.other\A>svn
merge ^^/A/mu@1 mu
DBG: util.c:1462: ancestor:
file:///C:/SVN/src-trunk/Debug/subversion/tests/cmdline/svn-test-work/repositories/merge_tests-6/A/mu@1
DBG: util.c:1470: url1:
file:///C:/SVN/src-trunk/Debug/subversion/tests/cmdline/svn-test-work/repositories/merge_tests-6/A/mu@1
DBG: util.c:1471: url2:
file:///C:/SVN/src-trunk/Debug/subversion/tests/cmdline/svn-test-work/repositories/merge_tests-6/A/mu@1
..\..\..\subversion\svn\main.c:2684: (apr_err=205000)
svn: E205000: Try 'svn help' for more info
..\..\..\subversion\svn\merge-cmd.c:370: (apr_err=205000)
..\..\..\subversion\svn\util.c:1487: (apr_err=205000)
svn: E205000: This merge requires two different but related branches
..\..\..\subversion\svn\util.c:1473: (apr_err=205000)
svn: E205000: Source and target are the same branch:
'file:///C:/SVN/src-trunk/Debug/subversion/tests/cmdline/svn-test-work/repositories/merge_tests-6/A/mu@1'
and
'file:///C:/SVN/src-trunk/Debug/subversion/tests/cmdline/svn-test-work/repositories/merge_tests-6/A/mu@1'
So...if we all agree that your patch is good (which I think we do)
then this test can be adjusted to expect the new error or deleted
altogether (I prefer the former with a pointer to this discussion).
Paul
