Re: svn commit: r1932855 - in subversion/trunk/subversion: include libsvn_subr tests/libsvn_subr

[Branko Čibej]

On 5. 4. 26 23:55, Nathan Hartman wrote:
On Sun, Apr 5, 2026 at 3:45 PM Branko Čibej <br...@apache.org> wrote:
    On 5. 4. 26 21:37, Daniel Sahlberg wrote:
    Den sön 5 apr. 2026 kl 16:43 skrev Branko Čibej <br...@apache.org>:

        On 5. 4. 26 16:33, Timofei Zhakov wrote:
        On Sun, Apr 5, 2026 at 4:07 PM Branko Čibej<br...@apache.org> 
<mailto:br...@apache.org> wrote:
        On 5. 4. 26 11:44,rin...@apache.org 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? :-)

It's not, but it's been the policy and practice for Subversion APIs to 
not check for a null pointer value in arguments unless it's explicitly 
documented as valid. Because it's harder to ignore a crash than an 
(error) return value from a function, so failing hard and fast is better.

[...]

-- Brane