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? 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(). Cheers, Daniel
