Re: r1438683 - issue #4306 'multiple editor drive file merges record wrong mergeinfo during conflicts'

--
Certified & Supported Apache Subversion Downloads: http://www.wandisco.com/subversion/download
----- Original Message -----
> From: Paul Burba <ptburba_at_gmail.com>
> To: Julian Foad <julianfoad_at_btopenworld.com>
> Cc: "dev_at_subversion.apache.org" <dev_at_subversion.apache.org>
> Sent: Tuesday, 12 February 2013, 21:03
> Subject: Re: r1438683 - issue #4306 'multiple editor drive file merges record wrong mergeinfo during conflicts'
> 
> On Mon, Feb 4, 2013 at 5:08 PM, Julian Foad <julianfoad_at_btopenworld.com> 
> wrote:
>>  Paul Burba wrote:
>> 
>>>  Julian Foad wrote:
>>>>  I (Julian Foad) wrote on 2013-01-31:
>>>> 
>>>>>  Hi Paul.  Not sure about this...
>>>> 
>>>>  1441810 fixes this and extends the test.
>>> 
>>>  Thanks Julian.
>>> 
>>>  I added r1441810 to the issue #4306 group on 1.7.x, with the caveat
>>>  that property conflicts are not handled properly in 1.7, rendering
>>>  your latest version of the test problematic on that branch -- see
>>> 
> http://svn.apache.org/viewvc/subversion/branches/1.7.x/STATUS?r1=1442368&r2=1442367&pathrev=1442368
>> 
>>  OK, but something's lost in the translation.  You say you 
> "reworked the earlier version of test to demonstrate the problem r1441810 
> fixes", but it fixes two problems (quoting from the log msg):
>> 
>>  (1) It didn't abort if there were conflicts on the last sub-range of a 
> non-last
>>  requested range.  (2) When aborting with conflicts it recorded mergeinfo
>>  describing only the current sub-range, not the sub-ranges merged before the
>>  conflict.
>> 
>>  Your test only specifies a single range ("all") to merge, so it 
> can't test (1).
>> 
>> 
>>  Something looks wrong in this hunk of your change, at least with the 
> comment:
>> 
>>     # Previously this test failed because the merge failed after merging
>>  -  # only r2 (as it should) but mergeinfo for r5-6 was recorded, preventing
>>  +  # only r5 (as it should) but only mergeinfo for r5 was recorded, even
>>  +  # though preventing
>>     # subsequent repeat merges from applying the operative r5.
>>     svntest.actions.run_and_verify_svn(
>>       "Incorrect mergeinfo set during conflict aborted merge",
>>  -    ['/iota:2-4\n'], [], 'pg', SVN_PROP_MERGEINFO, 
> iota_copy_path)
>>  +    ['/iota:2-6\n'], [], 'pg', SVN_PROP_MERGEINFO, 
> iota_copy_path)
>> 
>>  On trunk, there were two previous buggy behaviours: most recently, 
> mergeinfo for only r5 would have been recorded here, but before your initial fix 
> (and, I guess, in 1.7.x) mergeinfo for the whole requested range (including in 
> this case r3, r5, r7) was recorded; the latter is therefore what we want to 
> mention here.
>> 
>>  - Julian
> 
> In r1445454 I did what I probably should have done from the start:
> Take your version test (rather than my earlier version) and use it as
> a template for a 1.7 test that simple replaces the prop edits on
> 'A/mu' with text edits.  The test coverage should now be as identical
> as possible on the 1.7.x-issue4306 branch and trunk.
Cool, thanks Paul.  (The annoying thing is I originally started writing the test with text edits, and then I deleted that and switched to prop edits in anticipation of extending it to test a directory merge as well (which I have since done).  Of course a directory merge *can* be tested with text edits (in children) but it was easier to write the test code with prop edits.)
- Julian