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.
>
> 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_at_1145992' and
>
> Sure, that would be nice throughout 'svn', but I won't aim to do that within this patch.
>
> - Julian
Received on 2011-12-15 16:15:47 CET