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
