On Wed, Sep 10, 2008 at 11:40 AM, Paul Burba <ptburba_at_gmail.com> wrote:
> On Tue, Sep 9, 2008 at 4:32 PM, Paul Burba <ptburba_at_gmail.com> wrote:
>> On Mon, Sep 1, 2008 at 11:46 AM, Arfrever Frehtes Taifersar Arahesis
>> <arfrever.fta_at_gmail.com> wrote:
>>> 2008-09-01 17:39:20 Arfrever Frehtes Taifersar Arahesis napisał(a):
>>>> I discovered 2 bugs in handling of svn:mergeinfo when I was performing some
>>>> merges on the svnpatch-diff branch.
>>>>
>>>> 1. Addition of incorrect svn:mergeinfo on some files or directories:
>>>>
>>>> Steps to reproduce:
>>>> svn co -r32624 https://svn.collab.net/repos/svn/branches/svnpatch-diff subversion-svnpatch-diff-r32624
>>>> cd subversion-svnpatch-diff-r32624
>>>> svn merge -r32500:32620 ^/trunk
>>>>
>>>> This merge adds incorrect svn:mergeinfo on:
>>>> build.conf
>>>> subversion/include
>>>> subversion/libsvn_subr
>>>> subversion/tests/libsvn_subr
>>>>
>>>> These files / directories don't have svn:mergeinfo before merging.
>>>> After merging, svn:mergeinfo on these files / directories doesn't contain:
>>>> /trunk:1-31375,31378-32620
>>>>
>>>> (Next `svn merge ^/trunk` would try to merge r25664:30782 from trunk which causes
>>>> many conflicts.)
>>>>
>>>>
>>>> 2. Incomplete reversion of changes during reverse merging:
>>>>
>>>> Steps to reproduce:
>>>> svn co -r32625 https://svn.collab.net/repos/svn/branches/svnpatch-diff subversion-svnpatch-diff-r32625
>>>> cd subversion-svnpatch-diff-r32625
>>>> svn merge -c-32625 ^/branches/svnpatch-diff
>>>>
>>>> r32625 added (incorrect) svn:mergeinfo on:
>>>> build.conf
>>>> subversion/include
>>>> subversion/libsvn_subr
>>>> subversion/tests/libsvn_subr
>>>>
>>>> Reverse merging should delete svn:mergeinfo property, but it leaves empty svn:mergeinfo.
>>>
>>>
>>> 3. Reverse merging mentioned above removes '/trunk:r1-31375' from svn:mergeinfo on branch root:
>>> $ svn pg svn:mergeinfo .
>>> /branches/1.5.x-r30215:30238
>>> /branches/bdb-reverse-deltas:31976-32455
>>> /branches/diff-callbacks3:29985-30687
>>> /branches/dont-save-plaintext-passwords-by-default:30654-31044
>>> /branches/gnome-keyring:30484-31336
>>> /branches/in-memory-cache:29755-31378
>>> /branches/issue-3000:31639,31642-31645,31647-31652,31654,31660
>>> /branches/issue-3220-dev:32136-32152
>>> /branches/kwallet:30711-31240
>>> /branches/log-g-performance:30867-30958
>>> /branches/svn-mergeinfo-enhancements:30045-30214
>>> /branches/svnserve-logging:29754-30819
>>> /trunk:31378-32500
>>
>> Arfrever,
>>
>> Yes, that is most definitely wrong, we should see mergeinfo of
>> '/trunk:1-31375,31378-32500', i.e. the mergeinfo that existed @32624.
>>
>> It is worth noting that a portion of this mergeinfo, '/trunk:1-28961',
>> is self-referential, that is to say it explicitly describes
>> paths/revisions that are part of the svn/branches/svnpatch-diff
>> branch's natural history (the branch was copied from trunk_at_28961).
>> This came about because back in January when this branch was created
>> we were still explicitly recording natural history in explicit
>> mergeinfo when doing a copy.
>>
>> Now for the specifics: What's going wrong here is that when we perform
>> the reverse merge of -c-32625,
>> libsvn_subr/merge.c:filter_self_referential_mergeinfo() is filtering
>> out /trunk:1-31375' portion of svnpatch-diff's mergeinfo. As
>> mentioned above it should only filter out '/trunk:1-28961', so that is
>> one bug. But I can't see any reason to ever do filtering of this type
>> during a *reverse* merge. Currently the svn_wc_diff_callbacks3_t
>> callback merge.c:merge_props_changed() calls
>> filter_self_referential_mergeinfo() without consideration of the
>> merge's direction.
>>
>> So we need to do the following:
>>
>> 1) Stop filtering self-referential mergeinfo during reverse merges
>> (easier said than done as we don't have enough context in the callback
>> to determine merge direction right now). If you or anything else can
>> see a reason this filtering needs to be done in reverse merges please
>> let me know!
>
> The attached patch takes care of this. I'll commit later today if
> there are no objections.
>
>> 2) Determine if the filtering bug can occur during forward merges and
>> fix it (I'm pretty certain it can, just trying to come up with a
>> test).
>
> Not too hard to do it turns out, test and fix in progress.
Added a new merge test in r33013 where valid mergeinfo (i.e. not part
of the target's natural history) is actually filtered out.
Paul
Received on 2008-09-11 15:44:41 CEST