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. Cheers, Nathan
