Re: svn commit: r1656893 - in /subversion/trunk/subversion: svnsync/sync.c tests/cmdline/svnsync_tests_data/mergeinfo-contains-r0.dump tests/cmdline/svnsync_tests_data/mergeinfo-contains-r0.expected.dump

[Julian Foad <ju...@btopenworld.com>]

Hi Philip,

Thanks for fixing my embarrassing bug.

I've just finished fixing this too, and expanding the test, and then found you already committed this last night. We should have used more words to communicate. The fix was easy (not too different from yours) but I spent some time on the tests.

As I think I have made a considerably more thorough set of test cases for this issue, I'm taking the liberty of committing that part. r1657182.

Apparently we had no test that synced a repository containing merginfo at all. That's another deficiency we could address with the approach of running additional (svnsync in this case) tests at the end of existing tests. I'll look into doing that after reintegrating the 'dump-load-cross-check' branch.

- Julian



> Author: philip
> Date: Tue Feb  3 19:07:09 2015
> New Revision: 1656893
> 
> URL: http://svn.apache.org/r1656893
> Log:
> Fix svnsync r0 filtering to handle svn:mergeinfo where not all
> lines contain r0.
> 
> * subversion/svnsync/sync.c
>   (remove_r0_mergeinfo): Build up new string when r0 not present.
> 
> [In subversion/tests/cmdline/svnsync_tests_data/]
> 
> * mergeinfo-contains-r0.dump: Extend with svn:mergeinfo that has
>    only some, or no, lines with r0.
> 
> * mergeinfo-contains-r0.expected.dump: Adjust.
> 
> Modified:
>     subversion/trunk/subversion/svnsync/sync.c
>     
> subversion/trunk/subversion/tests/cmdline/svnsync_tests_data/mergeinfo-contains-r0.dump
>     
> subversion/trunk/subversion/tests/cmdline/svnsync_tests_data/mergeinfo-contains-r0.expected.dump

Re: svn commit: r1656893 - in /subversion/trunk/subversion: svnsync/sync.c tests/cmdline/svnsync_tests_data/mergeinfo-contains-r0.dump tests/cmdline/svnsync_tests_data/mergeinfo-contains-r0.expected.dump

[Ivan Zhakov <iv...@visualsvn.com>]

On 5 February 2015 at 16:05, Julian Foad <ju...@btopenworld.com> wrote:
> Ivan Zhakov wrote:
>
>> On 4 February 2015 at 20:45, Julian Foad wrote:
>>>  So we're on the third version of the code and third version of the test, for a
>>>  tiny edge-case feature. Clearly it's fragile. I had a bad feeling about writing it
>>>  this way in the first place.
>>
>> I had the same feelings when were reviewing backport nomination in 1.8.x.
>>
>> I wonder why you duplicated code that parses svn:mergeinfo instead of
>> using svn_mergeinfo_parse() ? I mean:
>> 1. Parse svn:mergeinfo using svn_mergeinfo_parse()
>
> The parser contains checks including a check that the start rev is not zero.
>
> I would have to split the low-level parsing from the validation feature of the current parser.
>
> I started doing this... but didn't know when to stop. (There are lots of things the current
> parser currently checks for, and canonicalizes, and so lots of possibilities for separating
> low-level parsing from higher-level processing.)
>
Now I understand the situation. Check for zero revision introduced in
r925290 and released in 1.7.0. But didn't not this change break
backward compatibility?

Making svnsync rewrite svn:mergeinfo is also questionable approach
taking into account write-through proxy configurations etc.

I'm not familiar with merge-tracking code, but I'm not sure that
svn_mergeinfo_parse() is proper layer for validating for zero revision
reference.

-- 
Ivan Zhakov

Re: svn commit: r1656893 - in /subversion/trunk/subversion: svnsync/sync.c tests/cmdline/svnsync_tests_data/mergeinfo-contains-r0.dump tests/cmdline/svnsync_tests_data/mergeinfo-contains-r0.expected.dump

[Julian Foad <ju...@btopenworld.com>]

Ivan Zhakov wrote:

> On 4 February 2015 at 20:45, Julian Foad wrote:
>>  So we're on the third version of the code and third version of the test, for a
>>  tiny edge-case feature. Clearly it's fragile. I had a bad feeling about writing it
>>  this way in the first place.
> 
> I had the same feelings when were reviewing backport nomination in 1.8.x.
> 
> I wonder why you duplicated code that parses svn:mergeinfo instead of
> using svn_mergeinfo_parse() ? I mean:
> 1. Parse svn:mergeinfo using svn_mergeinfo_parse()

The parser contains checks including a check that the start rev is not zero.

I would have to split the low-level parsing from the validation feature of the current parser.

I started doing this... but didn't know when to stop. (There are lots of things the current parser currently checks for, and canonicalizes, and so lots of possibilities for separating low-level parsing from higher-level processing.)

I then thought writing the stripping-r0 code as a text manipulation would be "simpler" in the sense of a smaller change.

> 2. For every path remove zero revision using svn_rangelist_remove()
> 3. Build new svn:mergeinfo using svn_mergeinfo_to_string()

So, yes, this is how it *should* be done, I agree. Maybe I will do it.

- Julian

Re: svn commit: r1656893 - in /subversion/trunk/subversion: svnsync/sync.c tests/cmdline/svnsync_tests_data/mergeinfo-contains-r0.dump tests/cmdline/svnsync_tests_data/mergeinfo-contains-r0.expected.dump

[Ivan Zhakov <iv...@visualsvn.com>]

On 4 February 2015 at 20:45, Julian Foad <ju...@btopenworld.com> wrote:
> So we're on the third version of the code and third version of the test, for a
> tiny edge-case feature. Clearly it's fragile. I had a bad feeling about writing it
> this way in the first place.
I had the same feelings when were reviewing backport nomination in 1.8.x.

I wonder why you duplicated code that parses svn:mergeinfo instead of
using svn_mergeinfo_parse() ? I mean:
1. Parse svn:mergeinfo using svn_mergeinfo_parse()
2. For every path remove zero revision using svn_rangelist_remove()
3. Build new svn:mergeinfo using svn_mergeinfo_to_string()


-- 
Ivan Zhakov

Re: svn commit: r1656893 - in /subversion/trunk/subversion: svnsync/sync.c tests/cmdline/svnsync_tests_data/mergeinfo-contains-r0.dump tests/cmdline/svnsync_tests_data/mergeinfo-contains-r0.expected.dump

[Julian Foad <ju...@btopenworld.com>]

I (Julian Foad) wrote:
> I have proceeded to implement 'svnsync' cross-checking in the test 

> suite: running every repository produced by a test through 'svnsync' and 
> checking there is no change.
> 
> This turns up at least one further bug: assertion failure on deleting a 
> mergeinfo property.
> 
> I'll get to fixing that and committing the extra testing tomorrow.

For the record, I fixed this in r1657401 but didn't add a specific test for it.

- Julian

Re: svn commit: r1656893 - in /subversion/trunk/subversion: svnsync/sync.c tests/cmdline/svnsync_tests_data/mergeinfo-contains-r0.dump tests/cmdline/svnsync_tests_data/mergeinfo-contains-r0.expected.dump

[Julian Foad <ju...@btopenworld.com>]

So we're on the third version of the code and third version of the test, for a tiny edge-case feature. Clearly it's fragile. I had a bad feeling about writing it this way in the first place.

I have proceeded to implement 'svnsync' cross-checking in the test suite: running every repository produced by a test through 'svnsync' and checking there is no change.

This turns up at least one further bug: assertion failure on deleting a mergeinfo property.

I'll get to fixing that and committing the extra testing tomorrow.

- Julian

Re: svn commit: r1656893 - in /subversion/trunk/subversion: svnsync/sync.c tests/cmdline/svnsync_tests_data/mergeinfo-contains-r0.dump tests/cmdline/svnsync_tests_data/mergeinfo-contains-r0.expected.dump

[Julian Foad <ju...@btopenworld.com>]

I (Julian Foad) wrote:
> I've just finished fixing this too, and expanding the test, and then found 

> you already committed this last night. We should have used more words to 
> communicate. The fix was easy (not too different from yours) but I spent some 
> time on the tests.

It turns out your fix was not quite right -- see r1657195. Before committing r1657182, I briefly inspected it and thought it looked OK and re-ran my tests on it but I must have failed to re-build. The buildbots caught it.

- Julian