Issue #4467 - blame youngest to oldest needs to handle SVN_INVALID_REVNUM - 1.9 blocker?

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

This issue looks like one that we should fix for 1.9.

[[[
Several of the blame functions don't work properly when start=HEAD and end=N
because they compare start and end to work out whether start is less than or
greater than end.  At least these functions need attention:

  svn_client_blame5 gets younger_rev wrong
  svn_ra_get_file_revs2 gets the capability check wrong
  svn_ra_serf__get_file_revs gets the peg_rev wrong

See also r1568872 which fixed a similar problem with log.
]]]

Philip wrote on IRC just now,

[[[

$ svn blame -rHEAD:1000000 ../src/README
svn: E235000: In file '../src/subversion/libsvn_client/blame.c' line 497: assertion failed ((frb->last_filename == NULL) || frb->include_merged_revisions)

[...] I think it is a consequence of things like blame.c:659 which does younger_end.value.number = MAX(start_revnum, end_revnum);
]]]

Anyone want to take a look at it?

- Julian

Re: Issue #4467 - blame youngest to oldest needs to handle SVN_INVALID_REVNUM - 1.9 blocker?

[Branko Čibej <br...@wandisco.com>]

On 17.02.2015 17:56, Philip Martin wrote:
> Philip Martin <ph...@wandisco.com> writes:
>
>> Essentially every time we compare start and end we need to make sure
>> that we handle SVN_INVALID_REVNUM properly.  I think the functions
>> listed in the issue are a few locations I identified that need to be
>> checked.
> Some of the APIs in svn_ra.h document that SVN_INVALID_REVNUM can be
> passed to mean HEAD, other functions in the file are silent on the
> matter.  Is passing SVN_INVALID_REVNUM to mean HEAD supported by
> svn_ra_get_file_revs2, svn_ra_replay_range, etc.

Sadly, the RA layer is not consistent there ... I wouldn't be surprised
if it was inconsistent amongst different RA implementations, too. Needs
code review, IMO.

-- Brane

Re: Issue #4467 - blame youngest to oldest needs to handle SVN_INVALID_REVNUM - 1.9 blocker?

[Philip Martin <ph...@wandisco.com>]

Philip Martin <ph...@wandisco.com> writes:

> Essentially every time we compare start and end we need to make sure
> that we handle SVN_INVALID_REVNUM properly.  I think the functions
> listed in the issue are a few locations I identified that need to be
> checked.

Some of the APIs in svn_ra.h document that SVN_INVALID_REVNUM can be
passed to mean HEAD, other functions in the file are silent on the
matter.  Is passing SVN_INVALID_REVNUM to mean HEAD supported by
svn_ra_get_file_revs2, svn_ra_replay_range, etc.

-- 
Philip Martin | Subversion Committer
WANdisco // *Non-Stop Data*

Re: Issue #4467 - blame youngest to oldest needs to handle SVN_INVALID_REVNUM - 1.9 blocker?

[Philip Martin <ph...@wandisco.com>]

Julian Foad <ju...@btopenworld.com> writes:

> This issue looks like one that we should fix for 1.9.
>
> [[[
> Several of the blame functions don't work properly when start=HEAD and end=N
> because they compare start and end to work out whether start is less than or
> greater than end.  At least these functions need attention:
>
>   svn_client_blame5 gets younger_rev wrong
>   svn_ra_get_file_revs2 gets the capability check wrong
>   svn_ra_serf__get_file_revs gets the peg_rev wrong
>
> See also r1568872 which fixed a similar problem with log.
> ]]]
>
> Philip wrote on IRC just now,
>
> [[[
>
> $ svn blame -rHEAD:1000000 ../src/README
> svn: E235000: In file '../src/subversion/libsvn_client/blame.c' line 497: assertion failed ((frb->last_filename == NULL) || frb->include_merged_revisions)
>
> [...] I think it is a consequence of things like blame.c:659 which does younger_end.value.number = MAX(start_revnum, end_revnum);
> ]]]
>
> Anyone want to take a look at it?

r1568872 was

Index: ../src/subversion/libsvn_ra_serf/log.c
===================================================================
--- ../src/subversion/libsvn_ra_serf/log.c	(revision 1568871)
+++ ../src/subversion/libsvn_ra_serf/log.c	(revision 1568872)
@@ -602,7 +602,7 @@
   /* At this point, we may have a deleted file.  So, we'll match ra_neon's
    * behavior and use the larger of start or end as our 'peg' rev.
    */
-  peg_rev = (start > end) ? start : end;
+  peg_rev = (start == SVN_INVALID_REVNUM || start > end) ? start : end;
 
   SVN_ERR(svn_ra_serf__get_stable_url(&req_url, NULL /* latest_revnum */,
                                       session, NULL /* conn */,

Essentially every time we compare start and end we need to make sure
that we handle SVN_INVALID_REVNUM properly.  I think the functions
listed in the issue are a few locations I identified that need to be
checked.

The issue identifies svn_client_blame5 but looking at the code it
converts start and end to numerical revisions early on so probably does
not have the bug.  The SEGV above may be some other problem.

-- 
Philip Martin | Subversion Committer
WANdisco // *Non-Stop Data*

Re: Issue #4467 - blame youngest to oldest needs to handle SVN_INVALID_REVNUM - 1.9 blocker?

[Branko Čibej <br...@wandisco.com>]

On 21.02.2015 00:42, Daniel Shahaf wrote:
> Branko Čibej wrote on Wed, Feb 18, 2015 at 01:57:03 +0100:
>> On 18.02.2015 01:10, Daniel Shahaf wrote:
>>> Julian Foad wrote on Tue, Feb 17, 2015 at 16:26:54 +0000:
>>>> This issue looks like one that we should fix for 1.9.
>>>>
>>>> [[[
>>>> Several of the blame functions don't work properly when start=HEAD and end=N
>>>> because they compare start and end to work out whether start is less than or
>>>> greater than end.  At least these functions need attention:
>>>>
>>>>   svn_client_blame5 gets younger_rev wrong
>>>>   svn_ra_get_file_revs2 gets the capability check wrong
>>>>   svn_ra_serf__get_file_revs gets the peg_rev wrong
>>>>
>>>> See also r1568872 which fixed a similar problem with log.
>>>> ]]]
>>>>
>>> Stupid question: can't we just redefine svn_revnum_t as unsigned long,
>>> so that SVN_INVALID_REVNUM becomes ULONG_MAX, and all these comparisons
>>> — in all codepaths, not just reverse blame — start DTRTing?
>> svn_revnum_t is part of our public API. No, we can't change it.
> I realize that, of course, but I was hoping for some other creative idea
> to reduce the chance of similar problems occurring in the future.
>
> How about deprecating the convention of using SVN_INVALID_REVNUM to
> represent HEAD.  From now on, whenever an API wants to support 'The
> value of the parameter may be either a proper revision number or HEAD',
> we write that API using svn_opt_revision_t (and specify that the input
> must be either svn_opt_revision_head or svn_opt_revision_number).
>
> It doesn't have to be svn_opt_revision_t specifically; any union type
> will do.  The point is to prevent recurrence of this bug by avoiding
> using -1 as the semantic maximum value of svn_revnum_t.
>
> We don't need to mass-deprecate existing APIs; we can convert them to
> svn_opt_revision_t when they are next revved anyway for other reasons.

This convention is only used at the RA layer, where all the functions
currently accept svn_revnum_t and none accept svn_opt_revision_t. You're
effectively asking to either deprecate the whole RA API, or break that
consistency.

I frankly don't know who invented the idea that -1 means HEAD in RA;
it's not consistent about that. I can guess that the reason for that was
to avoid a roundtrip to the server just to get the youngest revision
from the repo.

Personally I'd just leave it the way it is; possibly working towards
making all of RA accept -1 for HEAD (where that makes any kind of
sense). The problems we're seeing in the implementation (with missing
cases in revision comparisons) can be quite easily and consistently
fixed by inventing proper comparison macros that take account of the
specialness of SVN_INVALID_REVNUM. That would certainly make the code
easier to understand and concentrate thinkos in one place.

-- Brane

Re: Issue #4467 - blame youngest to oldest needs to handle SVN_INVALID_REVNUM - 1.9 blocker?

[Daniel Shahaf <d....@daniel.shahaf.name>]

Branko Čibej wrote on Wed, Feb 18, 2015 at 01:57:03 +0100:
> On 18.02.2015 01:10, Daniel Shahaf wrote:
> > Julian Foad wrote on Tue, Feb 17, 2015 at 16:26:54 +0000:
> >> This issue looks like one that we should fix for 1.9.
> >>
> >> [[[
> >> Several of the blame functions don't work properly when start=HEAD and end=N
> >> because they compare start and end to work out whether start is less than or
> >> greater than end.  At least these functions need attention:
> >>
> >>   svn_client_blame5 gets younger_rev wrong
> >>   svn_ra_get_file_revs2 gets the capability check wrong
> >>   svn_ra_serf__get_file_revs gets the peg_rev wrong
> >>
> >> See also r1568872 which fixed a similar problem with log.
> >> ]]]
> >>
> > Stupid question: can't we just redefine svn_revnum_t as unsigned long,
> > so that SVN_INVALID_REVNUM becomes ULONG_MAX, and all these comparisons
> > — in all codepaths, not just reverse blame — start DTRTing?
> 
> svn_revnum_t is part of our public API. No, we can't change it.

I realize that, of course, but I was hoping for some other creative idea
to reduce the chance of similar problems occurring in the future.

How about deprecating the convention of using SVN_INVALID_REVNUM to
represent HEAD.  From now on, whenever an API wants to support 'The
value of the parameter may be either a proper revision number or HEAD',
we write that API using svn_opt_revision_t (and specify that the input
must be either svn_opt_revision_head or svn_opt_revision_number).

It doesn't have to be svn_opt_revision_t specifically; any union type
will do.  The point is to prevent recurrence of this bug by avoiding
using -1 as the semantic maximum value of svn_revnum_t.

We don't need to mass-deprecate existing APIs; we can convert them to
svn_opt_revision_t when they are next revved anyway for other reasons.

Cheers,

Daniel

Re: Issue #4467 - blame youngest to oldest needs to handle SVN_INVALID_REVNUM - 1.9 blocker?

[Branko Čibej <br...@wandisco.com>]

On 18.02.2015 01:10, Daniel Shahaf wrote:
> Julian Foad wrote on Tue, Feb 17, 2015 at 16:26:54 +0000:
>> This issue looks like one that we should fix for 1.9.
>>
>> [[[
>> Several of the blame functions don't work properly when start=HEAD and end=N
>> because they compare start and end to work out whether start is less than or
>> greater than end.  At least these functions need attention:
>>
>>   svn_client_blame5 gets younger_rev wrong
>>   svn_ra_get_file_revs2 gets the capability check wrong
>>   svn_ra_serf__get_file_revs gets the peg_rev wrong
>>
>> See also r1568872 which fixed a similar problem with log.
>> ]]]
>>
> Stupid question: can't we just redefine svn_revnum_t as unsigned long,
> so that SVN_INVALID_REVNUM becomes ULONG_MAX, and all these comparisons
> — in all codepaths, not just reverse blame — start DTRTing?

svn_revnum_t is part of our public API. No, we can't change it.

-- Brane

Re: Issue #4467 - blame youngest to oldest needs to handle SVN_INVALID_REVNUM - 1.9 blocker?

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

Daniel Shahaf wrote:
> Julian Foad wrote on Tue, Feb 17, 2015 at 16:26:54 +0000:
>>  Philip wrote on IRC just now,
>>  $ svn blame -rHEAD:1000000 ../src/README
>>  svn: E235000: In file '../src/subversion/libsvn_client/blame.c'  line 497:
>> assertion failed ((frb->last_filename == NULL) || frb->include_merged_revisions)

This failure happened to me just now so I added it to the issue, and Bert is working on a fix as I speak: r1660969, r1660975.

> I am behind the libsvn_client part of this.  Am I expected to take
> ownership of these bugs?
> [...]  (I'll try to at least review the backports if I can, but if I'm
> expected to do more, just say so.)

Thank you for offering to assist, but no, I don't think you need to.

- Julian

Re: Issue #4467 - blame youngest to oldest needs to handle SVN_INVALID_REVNUM - 1.9 blocker?

[Daniel Shahaf <d....@daniel.shahaf.name>]

Julian Foad wrote on Tue, Feb 17, 2015 at 16:26:54 +0000:
> This issue looks like one that we should fix for 1.9.
> 
> [[[
> Several of the blame functions don't work properly when start=HEAD and end=N
> because they compare start and end to work out whether start is less than or
> greater than end.  At least these functions need attention:
> 
>   svn_client_blame5 gets younger_rev wrong
>   svn_ra_get_file_revs2 gets the capability check wrong
>   svn_ra_serf__get_file_revs gets the peg_rev wrong
> 
> See also r1568872 which fixed a similar problem with log.
> ]]]
> 

Stupid question: can't we just redefine svn_revnum_t as unsigned long,
so that SVN_INVALID_REVNUM becomes ULONG_MAX, and all these comparisons
— in all codepaths, not just reverse blame — start DTRTing?

It doesn't break big repositories, since it doesn't decrease the domain
of valid non-negative revision numbers.  But it breaks some uses of
SVN_INVALID_REVNUM (passing it to a domain that represents -1 and
ULONG_MAX distinctly, and then reading it back), so I assume we can't do
it until 2.0 ☹

> Philip wrote on IRC just now,
> 
> [[[
> 
> $ svn blame -rHEAD:1000000 ../src/README
> svn: E235000: In file '../src/subversion/libsvn_client/blame.c' line 497: assertion failed ((frb->last_filename == NULL) || frb->include_merged_revisions)
> 
> [...] I think it is a consequence of things like blame.c:659 which does younger_end.value.number = MAX(start_revnum, end_revnum);
> ]]]
> 
> Anyone want to take a look at it?

I am behind the libsvn_client part of this.  Am I expected to take
ownership of these bugs?

Asking because my circumstances have changed in the 18 months since
I wrote this code: my svn time budget is different today than it was
then.  (I'll try to at least review the backports if I can, but if I'm
expected to do more, just say so.)

Cheers,

Daniel