You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Julian Foad <ju...@btopenworld.com> on 2011/12/14 14:18:34 UTC
[PATCH] Merge source and target must be related but different branches, v1
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.
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.
Comments?
- Julian
Re: [PATCH] Merge source and target must be related but different
branches, v1
Posted by "C. Michael Pilato" <cm...@collab.net>.
On 12/15/2011 07:53 AM, Julian Foad wrote:
> This "sync merge from my own history" operation seems bogus. I notice
> that (without my patch) it merges changes from the future *and* records mergeinfo
> for them. Surely we didn't ever intend that?
Subversion has supported "sync merge from my own history" since before merge
was written. It's called "svn update". :-)
Seriously, I'm having trouble coming up a really solid use-case for
supporting this. You could never commit the results of that merge because
any changed files would necessarily be out of date. *Maybe* if you were
trying to build a custom branch with select changes, you might 'svn update
-rOLD_REV', selective merge changes from the future of the branch, and then
copy the whole thing elsewhere. But let's nip that in the bud, and suggest
instead that folks trying to do this update to HEAD, and selective
reverse-merge the revisions they *don't* want. At least that's a scenario
that's supportable and committable while still logically equivalent to the
former.
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Distributed Development On Demand
Re: [PATCH] Merge source and target must be related but different
branches, v1
Posted by Paul Burba <pt...@gmail.com>.
On Thu, Dec 15, 2011 at 10:15 AM, Paul Burba <pt...@gmail.com> wrote:
> On Thu, Dec 15, 2011 at 7:53 AM, Julian Foad <ju...@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
Re: [PATCH] Merge source and target must be related but different
branches, v1
Posted by Paul Burba <pt...@gmail.com>.
On Thu, Dec 15, 2011 at 7:53 AM, Julian Foad <ju...@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.
>
> Also merge_reintegrate_tests.py 5 and 9 fail. That's because the specified source or target location is also the branching point, so one of the locations is equal to the common ancestor. My patch currently rejects that, but I'll simply remove that part of the check for now and then consider later whether it can be tightened a bit. In other words I'm happy about dealing with this part.
>
>> A bit bikesheddy, bit I wonder if it might make for a cleaner error
>> messages if we use the repo root shorthand notation: For example,
>> this:
>>
>> svn: E205000: Source and target are the same branch:
>> '^/subversion/trunk@1145992' and
>
> Sure, that would be nice throughout 'svn', but I won't aim to do that within this patch.
>
> - Julian
Re: [PATCH] Merge source and target must be related but different branches, v1
Posted by Julian Foad <ju...@btopenworld.com>.
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. I notice that (without my patch) it merges changes from the future *and* records mergeinfo for them. Surely we didn't ever intend that?
$ 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? I'm assuming that's bogus and the merge should really be rejected or at least the mergeinfo should be elided.
>> 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.
Also merge_reintegrate_tests.py 5 and 9 fail. That's because the specified source or target location is also the branching point, so one of the locations is equal to the common ancestor. My patch currently rejects that, but I'll simply remove that part of the check for now and then consider later whether it can be tightened a bit. In other words I'm happy about dealing with this part.
> A bit bikesheddy, bit I wonder if it might make for a cleaner error
> messages if we use the repo root shorthand notation: For example,
> this:
>
> svn: E205000: Source and target are the same branch:
> '^/subversion/trunk@1145992' and
Sure, that would be nice throughout 'svn', but I won't aim to do that within this patch.
- Julian
Re: [PATCH] Merge source and target must be related but different
branches, v1
Posted by Paul Burba <pt...@gmail.com>.
On Wed, Dec 14, 2011 at 8:18 AM, Julian Foad <ju...@btopenworld.com> 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.
Hi Julian,
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.:
C:\SVN\src-trunk-4>svn up
Updating '.':
At revision 1214356.
C:\SVN\src-trunk-4>svn log -q -l1
------------------------------------------------------------------------
r1214216 | stsp | 2011-12-14 07:38:48 -0500 (Wed, 14 Dec 2011)
------------------------------------------------------------------------
C:\SVN\src-trunk-4>svn up -r1214215
Updating '.':
U subversion\libsvn_repos\load-fs-vtable.c
Updated to revision 1214215.
C:\SVN\src-trunk-4>svn merge ^^/subversion/trunk .
DBG: util.c:1462: ancestor:
https://svn.apache.org/repos/asf/subversion/trunk@1214215
DBG: util.c:1470: url1:
https://svn.apache.org/repos/asf/subversion/trunk@1214358
DBG: util.c:1471: url2:
https://svn.apache.org/repos/asf/subversion/trunk@1214215
..\..\..\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:
'https://svn.apache.org/repos/asf/subversion/trunk@1214358' and
https://svn.apache.org/repos/asf/subversion/trunk@1214215'
But was this ever useful? I don't see how. We can still do this *if*
we specify an actual revision/revision range.
> 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.
> Comments?
A bit bikesheddy, bit I wonder if it might make for a cleaner error
messages if we use the repo root shorthand notation: For example,
this:
svn: E205000: Source and target are the same branch:
'^/subversion/trunk@1145992' and '^/subversion/branches/1.7.x@1213741'
Rather than this:
svn: E205000: Source and target are the same branch:
'https://svn.apache.org/repos/asf/subversion/trunk@1145992' and
'https://svn.apache.org/repos/asf/subversion/branches/1.7.x@1213741'
Paul