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
Received on 2015-02-21 00:47:42 CET