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

Re: [PATCH] Merge source and target must be related but different branches, v1

From: Paul Burba <ptburba_at_gmail.com>
Date: Thu, 15 Dec 2011 10:51:04 -0500

On Thu, Dec 15, 2011 at 10:15 AM, Paul Burba <ptburba_at_gmail.com> wrote:
> On Thu, Dec 15, 2011 at 7:53 AM, Julian Foad <julianfoad_at_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_at_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_at_1
DBG: util.c:1470: url1:
file:///C:/SVN/src-trunk/Debug/subversion/tests/cmdline/svn-test-work/repositories/merge_tests-6/A/mu_at_1
DBG: util.c:1471: url2:
file:///C:/SVN/src-trunk/Debug/subversion/tests/cmdline/svn-test-work/repositories/merge_tests-6/A/mu_at_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_at_1'
and 'file:///C:/SVN/src-trunk/Debug/subversion/tests/cmdline/svn-test-work/repositories/merge_tests-6/A/mu_at_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_at_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_at_1
DBG: util.c:1470: url1:
file:///C:/SVN/src-trunk/Debug/subversion/tests/cmdline/svn-test-work/repositories/merge_tests-6/A/mu_at_1
DBG: util.c:1471: url2:
file:///C:/SVN/src-trunk/Debug/subversion/tests/cmdline/svn-test-work/repositories/merge_tests-6/A/mu_at_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_at_1'
and 'file:///C:/SVN/src-trunk/Debug/subversion/tests/cmdline/svn-test-work/repositories/merge_tests-6/A/mu_at_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
Received on 2011-12-15 16:51:38 CET

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