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

From: Julian Foad <julianfoad_at_btopenworld.com>
Date: Thu, 15 Dec 2011 12:53:23 +0000 (GMT)

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_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 13:54:01 CET

Contemporary messages sorted: [ by date ] [ by thread ] [ by subject ] [ by author ] [ by messages with attachments ]