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]>
<mailto:[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.
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.
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. :)
-- Brane
