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
Received on 2015-02-21 10:18:25 CET