RE: "Unable to parse reversed revision range" when merging from trunk to branch

[Bert Huijben]

> -Original Message-
> From: Jens Christian Restemeier [mailto:j...@playtonicgames.com]
> Sent: maandag 3 juli 2017 16:31
> To: 'Johan Corveleyn' 
> Cc: 'Stefan Sperling' ; users@subversion.apache.org
> Subject: RE: "Unable to parse reversed revision range" when merging from
> trunk to branch
> 
> I narrowed it down to the code in subversion/libsvn_subr/mergeinfo.c line
> 892-898 in adjust_remaining_ranges. At that point next_range actually starts
> before modified_range, so my guess is svn_sort__array_insert has some
> unexpected behaviour.
> 
>x892   svn_merge_range_t *new_modified_range =
> x
>x893 apr_palloc(result_pool, 
> sizeof(*new_modified_range));
> x
>x894   new_modified_range->start = next_range->end;
> x
>x895   new_modified_range->end = modified_range->end;
> x
>x896   new_modified_range->inheritable = FALSE;
> x
>x897   modified_range->end = next_range->start;
> x
>x898   (*range_index)+=2;
> x
>   >x899   svn_sort__array_insert(rangelist, 
> &new_modified_range,
> x
>   *range_index);
> x
>x901   /* Recurse with the new range. */
> x
>x902   adjust_remaining_ranges(rangelist, range_index,
> result_pool); 
> x
> 
> Intuitively it seems to be awfully complicated to expand a range to the end of
> a change, and then cut it down recursively with adjust_remaining_ranges.
> My first thought would be to step through "rangelist" and "changes" side by
> side in svn_rangelist_merge2, and to modify, insert or skip a range in either
> list until you're at the end. Though obviously I have no idea about all the 
> edge
> cases the current code most likely fixes...
> 
> So how should I proceed from here? Should I open a bug with my findings
> and the test case?

I see you already opened an issue.

To me it looks like the issue is related to mixing recursive and non-recursive 
mergeinfo inside the same range... (The ranges with and without a '*' in the 
string). I see that your example case in the issue has quite a bit of overlap 
with both these kinds of ranges.

It looks like your case triggers a very interesting edge case.

Bert 

RE: "Unable to parse reversed revision range" when merging from trunk to branch

[Jens Christian Restemeier]

You are correct, it is a sparse workspace directory, so I assume any merge 
touching the excluded directories is marked as non-recursive. 
The merge is a plain merge from trunk, so the "changes" range starts at the 
same revision as the rangelist (15014) and goes up to the then-current revision 
on trunk (20515). There are no gaps in rangelist and the last range has the 
same inheritance, so all it should do is extend the last range to 20515.

-Original Message-
From: Bert Huijben [mailto:b...@qqmail.nl] 
Sent: 04 July 2017 10:41
To: 'Jens Christian Restemeier' ; 'Johan Corveleyn' 

Cc: 'Stefan Sperling' ; users@subversion.apache.org
Subject: RE: "Unable to parse reversed revision range" when merging from trunk 
to branch



> -Original Message-
> From: Jens Christian Restemeier [mailto:j...@playtonicgames.com]
> Sent: maandag 3 juli 2017 16:31
> To: 'Johan Corveleyn' 
> Cc: 'Stefan Sperling' ; users@subversion.apache.org
> Subject: RE: "Unable to parse reversed revision range" when merging 
> from trunk to branch
> 
> I narrowed it down to the code in subversion/libsvn_subr/mergeinfo.c 
> line
> 892-898 in adjust_remaining_ranges. At that point next_range actually 
> starts before modified_range, so my guess is svn_sort__array_insert 
> has some unexpected behaviour.
> 
>x892   svn_merge_range_t *new_modified_range =
> x
>x893 apr_palloc(result_pool, 
> sizeof(*new_modified_range));
> x
>x894   new_modified_range->start = next_range->end;
> x
>x895   new_modified_range->end = modified_range->end;
> x
>x896   new_modified_range->inheritable = FALSE;
> x
>x897   modified_range->end = next_range->start;
> x
>x898   (*range_index)+=2;
> x
>   >x899   svn_sort__array_insert(rangelist, 
> &new_modified_range,
> x
>   *range_index); x
>x901   /* Recurse with the new range. */
> x
>x902   adjust_remaining_ranges(rangelist, range_index,
> result_pool); 
> x
> 
> Intuitively it seems to be awfully complicated to expand a range to 
> the end of a change, and then cut it down recursively with 
> adjust_remaining_ranges.
> My first thought would be to step through "rangelist" and "changes" 
> side by side in svn_rangelist_merge2, and to modify, insert or skip a 
> range in either list until you're at the end. Though obviously I have 
> no idea about all the edge cases the current code most likely fixes...
> 
> So how should I proceed from here? Should I open a bug with my 
> findings and the test case?

I see you already opened an issue.

To me it looks like the issue is related to mixing recursive and non-recursive 
mergeinfo inside the same range... (The ranges with and without a '*' in the 
string). I see that your example case in the issue has quite a bit of overlap 
with both these kinds of ranges.

It looks like your case triggers a very interesting edge case.

Bert 

RE: "Unable to parse reversed revision range" when merging from trunk to branch

[Jens Christian Restemeier]

Just a quick update: I specified the range to merge from trunk manually and the 
merge completed without error. Looking at the mergeinfo property svn correctly 
extended the last revision range.

-Original Message-
From: Jens Christian Restemeier [mailto:j...@playtonicgames.com] 
Sent: 04 July 2017 11:01
To: 'Bert Huijben' ; 'Johan Corveleyn' 
Cc: 'Stefan Sperling' ; 'users@subversion.apache.org' 

Subject: RE: "Unable to parse reversed revision range" when merging from trunk 
to branch

You are correct, it is a sparse workspace directory, so I assume any merge 
touching the excluded directories is marked as non-recursive. 
The merge is a plain merge from trunk, so the "changes" range starts at the 
same revision as the rangelist (15014) and goes up to the then-current revision 
on trunk (20515). There are no gaps in rangelist and the last range has the 
same inheritance, so all it should do is extend the last range to 20515.

-Original Message-
From: Bert Huijben [mailto:b...@qqmail.nl] 
Sent: 04 July 2017 10:41
To: 'Jens Christian Restemeier' ; 'Johan Corveleyn' 

Cc: 'Stefan Sperling' ; users@subversion.apache.org
Subject: RE: "Unable to parse reversed revision range" when merging from trunk 
to branch



> -Original Message-
> From: Jens Christian Restemeier [mailto:j...@playtonicgames.com]
> Sent: maandag 3 juli 2017 16:31
> To: 'Johan Corveleyn' 
> Cc: 'Stefan Sperling' ; users@subversion.apache.org
> Subject: RE: "Unable to parse reversed revision range" when merging 
> from trunk to branch
> 
> I narrowed it down to the code in subversion/libsvn_subr/mergeinfo.c 
> line
> 892-898 in adjust_remaining_ranges. At that point next_range actually 
> starts before modified_range, so my guess is svn_sort__array_insert 
> has some unexpected behaviour.
> 
>x892   svn_merge_range_t *new_modified_range =
> x
>x893 apr_palloc(result_pool, 
> sizeof(*new_modified_range));
> x
>x894   new_modified_range->start = next_range->end;
> x
>x895   new_modified_range->end = modified_range->end;
> x
>x896   new_modified_range->inheritable = FALSE;
> x
>x897   modified_range->end = next_range->start;
> x
>x898   (*range_index)+=2;
> x
>   >x899   svn_sort__array_insert(rangelist, 
> &new_modified_range,
> x
>   *range_index); x
>x901   /* Recurse with the new range. */
> x
>x902   adjust_remaining_ranges(rangelist, range_index,
> result_pool); 
> x
> 
> Intuitively it seems to be awfully complicated to expand a range to 
> the end of a change, and then cut it down recursively with 
> adjust_remaining_ranges.
> My first thought would be to step through "rangelist" and "changes" 
> side by side in svn_rangelist_merge2, and to modify, insert or skip a 
> range in either list until you're at the end. Though obviously I have 
> no idea about all the edge cases the current code most likely fixes...
> 
> So how should I proceed from here? Should I open a bug with my 
> findings and the test case?

I see you already opened an issue.

To me it looks like the issue is related to mixing recursive and non-recursive 
mergeinfo inside the same range... (The ranges with and without a '*' in the 
string). I see that your example case in the issue has quite a bit of overlap 
with both these kinds of ranges.

It looks like your case triggers a very interesting edge case.

Bert 

RE: "Unable to parse reversed revision range" when merging from trunk to branch

[Jens Christian Restemeier]

I attached an patch with a test to the bug. I can't get gmock to work at the 
moment, so I have no idea if this test works...
It looks like the URL for gmock has changed since get-deps.sh was written, and 
it is not distributed as fused files.

Instead of building the rangelists it uses svn_rangelist__parse, which makes 
the test a bit more compact.

-Original Message-
From: Daniel Shahaf [mailto:d...@daniel.shahaf.name] 
Sent: 04 July 2017 06:09
To: Jens Restemeier 
Cc: Johan Corveleyn ; users@subversion.apache.org; Stefan 
Sperling 
Subject: Re: "Unable to parse reversed revision range" when merging from trunk 
to branch

Jens Restemeier wrote on Mon, 03 Jul 2017 20:01 +0100:
> > Am 03.07.2017 um 19:17 schrieb Stefan Sperling :
> > 
> > On Mon, Jul 03, 2017 at 03:31:00PM +0100, Jens Christian Restemeier wrote:
> >> Should I open a bug with my findings and the test case?
> > 
> > Yes, please open a bug in the issue tracker.
> > 
> Done: https://issues.apache.org/jira/browse/SVN-4686 
> 

Thanks!

> > A regression test for the subversion/tests tree would be essential.
> > Since you already wrote a test program in C, this file would be a 
> > good location for such a test: subversion/libsvn_subr/mergeinfo.c

Stefan meant subversion/tests/libsvn_subr/mergeinfo-test.c .

Re: "Unable to parse reversed revision range" when merging from trunk to branch

[Daniel Shahaf]

Jens Christian Restemeier wrote on Tue, 04 Jul 2017 14:43 +0100:
> I attached an patch with a test to the bug. I can't get gmock to work at the 
> moment, so I have no idea if this test works...

gmock is not required for running the tests; you only need python 2.7.
To run a single test, the invocation is

make mergeinfo-test && ./subversion/tests/libsvn_subr/mergeinfo-test
or
make check TESTS=subversion/tests/libsvn_subr/mergeinfo-test

What made you think gmock was required?

For future reference, when sending patches use either 'svn diff' or 'diff -u' 
to produce unified diff formatted output.

Note that Bert committed a test based on your test program in r1800754.  (I 
haven't checked how your new patch relates to that revision)

Feel free to join #svn-dev on freenode IRC as well.

Cheers,

Daniel

> It looks like the URL for gmock has changed since get-deps.sh was written, 
> and it is not distributed as fused files.
> 
> Instead of building the rangelists it uses svn_rangelist__parse, which makes 
> the test a bit more compact.

RE: "Unable to parse reversed revision range" when merging from trunk to branch

[Jens Christian Restemeier]

I ran "make tests", which fails for undefined references to gmock... I'll try 
your instructions.
I didn’t think you would look at this right away... :)

-Original Message-
From: Daniel Shahaf [mailto:d...@daniel.shahaf.name] 
Sent: 04 July 2017 15:01
To: Jens Christian Restemeier 
Cc: Johan Corveleyn ; users@subversion.apache.org; Stefan 
Sperling 
Subject: Re: "Unable to parse reversed revision range" when merging from trunk 
to branch

Jens Christian Restemeier wrote on Tue, 04 Jul 2017 14:43 +0100:
> I attached an patch with a test to the bug. I can't get gmock to work at the 
> moment, so I have no idea if this test works...

gmock is not required for running the tests; you only need python 2.7.
To run a single test, the invocation is

make mergeinfo-test && ./subversion/tests/libsvn_subr/mergeinfo-test
or
make check TESTS=subversion/tests/libsvn_subr/mergeinfo-test

What made you think gmock was required?

For future reference, when sending patches use either 'svn diff' or 'diff -u' 
to produce unified diff formatted output.

Note that Bert committed a test based on your test program in r1800754.  (I 
haven't checked how your new patch relates to that revision)

Feel free to join #svn-dev on freenode IRC as well.

Cheers,

Daniel

> It looks like the URL for gmock has changed since get-deps.sh was written, 
> and it is not distributed as fused files.
> 
> Instead of building the rangelists it uses svn_rangelist__parse, which makes 
> the test a bit more compact.

Re: "Unable to parse reversed revision range" when merging from trunk to branch

[Daniel Shahaf]

Jens Christian Restemeier wrote on Tue, 04 Jul 2017 15:10 +0100:
> I ran "make tests", which fails for undefined references to gmock...

The build.conf stanzas for the cxxhl bindings declare them as «install =
tests», which causes a «make tests» target to be created for compiling
and running the cxxhl bindings tests.

I'm not sure whether that was intentional.