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 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
Received on 2015-02-05 14:06:15 CET