Den mån 6 apr. 2026 kl 13:17 skrev Timofei Zhakov <[email protected]>:
> On Sun, Apr 5, 2026 at 11:55 PM Nathan Hartman <[email protected]> > wrote: > >> On Sun, Apr 5, 2026 at 3:45 PM Branko Čibej <[email protected]> wrote: >> >>> On 5. 4. 26 21:37, Daniel Sahlberg wrote: >>> >>> Den sön 5 apr. 2026 kl 16:43 skrev Branko Čibej <[email protected]>: >>> >>>> On 5. 4. 26 16:33, Timofei Zhakov wrote: >>>> >>>> On Sun, Apr 5, 2026 at 4:07 PM Branko Čibej <[email protected]> >>>> <[email protected]> wrote: >>>> >>>> On 5. 4. 26 11:44, [email protected] wrote: >>>> >>>> Author: rinrab >>>> Date: Sun Apr 5 09:44:54 2026 >>>> New Revision: 1932855 >>>> >>>> Log: >>>> Export a function for parsing a single opt_revision into the public API. >>>> We'll >>>> probably use it in svnbrowse. Neither of other tools need it because they >>>> primarly operate over a revision range for which there is a separate >>>> function >>>> in the API. >>>> >>>> >>>> Is making this public really necessary? A wrapper for >>>> svn_opt_parse_revision() that checks that there was only one revision in >>>> the argument is literally two lines of code that can be local in svnbrowse. >>>> >>>> Not exactly. It wraps parse_one_rev(). >>>> >>>> >>>> I know this implementation does, but parse_one_rev() is static. >>>> >>>> >>>> svn_opt_parse_revision() has extra code for revision ranges. It >>>> technically is possible to use it and just check that the end is >>>> missing (which svn does if revision ranges are not supported for the >>>> specific subcommand), but I believe it's easier to have more specific >>>> APIs. >>>> >>>> >>>> Every new public API is another that needs to be maintained forever, >>>> that's all I'm trying to say. I really don't think it's worth it for one >>>> usage in our code. >>>> >>>> -- Brane >>>> >>>> >>> I believe both Timofei and Brane have valid points here. Furthermore, >>> they both know much more C than I do so maybe my suggestion is not correct, >>> but: >>> >>> Would it be possible to re-write svn_opt_parse_revision so one could say >>> "Please extract exactly ONE revision and not a range"? What if you could >>> send 0 for end_revision? >>> >>> [[[ >>> Index: libsvn_subr/opt_revision.c >>> =================================================================== >>> --- libsvn_subr/opt_revision.c (revision 1932860) >>> +++ libsvn_subr/opt_revision.c (working copy) >>> @@ -187,7 +187,7 @@ >>> left_rev = apr_pstrdup(pool, arg); >>> >>> right_rev = parse_one_rev(start_revision, left_rev, pool); >>> - if (right_rev && *right_rev == ':') >>> + if (end_revision && right_rev && *right_rev == ':') >>> { >>> right_rev++; >>> end = parse_one_rev(end_revision, right_rev, pool); >>> ]]] >>> >>> Shouldn't this be functionally equivalent to svn_opt_parse_one_revision >>> when passing 0 for end_revision? >>> >>> >>> I thought of that of course. It seamlessly extends the semantics of the >>> function, but it also creates a non-obvious ABI incompatibility: >>> previously, sending a NULL revision parameter would cause a segfault. >>> >> >> >> Is a segfault ever a desired outcome? :-) >> >> >> On the other hand, expanding the domain of a parameter should be a valid >>> change for a minor revision (not a patch). So if you decide to do this, the >>> change should be merged into 1.5.0. >>> >> >> >> I think you meant 1.15.0. >> >> svnbrowse would need to be introduced no earlier than 1.16.0, so the API >> semantics could be merged into 1.15.0 or just wait until 1.16.0, I think. >> >> >> >>> Except, of course, returning the error as an int instead of as an >>> svn_error_t*. The latter may be a nice feature but it is orthogonal to the >>> the question of parsing one or more revisions and could be an argument for >>> svn_opt_parse_revision2(). >>> >>> >>> ... which amounts to adding another public API. :) >>> >> >> >> Given that public APIs need to be maintained forever I agree with Brane >> that we should scrutinize any potential new APIs carefully. I'm not against >> adding them. I'm just saying we should think carefully first. >> > > I would argue. We shouldn't be afraid of adding new public APIs. > > In this case there is svn_opt_revision_t which is a structure. It holds > information and other components can accept it as parameters. This means > methods to parse, serialise, and maybe even compare, construct and more may > be needed. For one reason or another. > I think we all agree that we should add a new API when we need functionality that doesn't yet exists. If we have any of the use cases above and neither of the existing APIs fullfill the requirements - no problem. I also think that returning svn_error_t* instead of int may worthwile of a new api, for example if we want to return more useful error messages. > > svn_opt_parse_one_revision() does just that. svn_opt_parse_revision() > parses a range. > > I think this is what justifies this API. > Den mån 6 apr. 2026 kl 13:12 skrev Timofei Zhakov <[email protected]>: > On Sun, Apr 5, 2026 at 9:37 PM Daniel Sahlberg < > [email protected]> wrote: > ... > Would it be possible to re-write svn_opt_parse_revision so one could say >> "Please extract exactly ONE revision and not a range"? What if you could >> send 0 for end_revision? >> > > I think this behaviour would be too complicated and implicit. It is also > bad for backward compatibility. > We have existing functions that accept NULL meaning "I don't care", for example passing NULL as fetched_rev to svn_ra_get_dir2[1]. There should be 0 existing implementations that pass NULL as end_revison to svn_opt_parse_revision() because, as Brane and Nathan points out, that would lead to a segfault. (I'm sure someone will now show how to catch that segfault as a kind of C++ style throw/catch in plain C but I wouldn't like to do that in production code). I don't see backwards compatibility to be an issue here. I think that the learning curve is steeper with two separate functions where the naming of the second (svn_opt_parse_revision) doesn't immediately tell what sets it apart from svn_opt_parse_one_revision. Kind regards, Daniel [1] https://subversion.apache.org/docs/api/1.14/svn__ra_8h.html#ac1fb454bedcb76ac56ce74606b77c53a
