On Sun, Apr 5, 2026 at 9:37 PM Daniel Sahlberg <[email protected]> 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? > I think this behaviour would be too complicated and implicit. It is also bad for backward compatibility. > > [[[ > 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 > > -- Timofei Zhakov
