On Tue, May 21, 2024 at 10:31 AM Timofey Zhakov <t...@chemodax.net> wrote: > > Hi, > > When I was fixing a bug related to the change revision argument (-c), > I noticed that the function which parses the change revision argument > is implemented implicitly in the sub_main (in file svn.c) [1], while > other revision parsers such as svn_opt_parse_revision_to_range > are implemented in the opt.c file [2]. These functions do similar > things. It follows that a function like svn_opt_parse_change_to_range > can be easily factored-out from the svn.c file into the opt.c file and > also be a part of the public API. > > [1] subversion\svn\svn.c:2331 at r1917853 > [2] subversion\libsvn_subr\opt.c:615 at r1917853 > > Pros: > - The new function can be tested by unit tests and be a part of the public > API. > - There would be less code in the svn.c file. > > Cons: > - The error messages have to be converted into one. > > I did this in my working copy and the code got a lot better. > > What do you think? > > -- > Timofei Zhakov
Hi Timofei, I haven't looked at the code yet, but I'd like to say that conceptually a refactor with improved unit testing sounds good. It's a shame that multiple error messages will have to become one, but we can word it carefully and then see if the documentation can be improved instead. If it becomes part of the public API, that likely means we could not backport this refactoring to the 1.14.x branch. (Upgrading and downgrading between patch releases within a minor release line should both always remain possible.) It could first be released with 1.15.0. However, if we find any additional bugs in unit testing, we likely can still backport those bug fixes by porting the fixes back to sub_main() on a branch. It's an extra step but not difficult. I'll try to look at the code in a little while. If you still have the changes in your working copy, you can send a patch (even if incomplete/draft) and I'll look at them together. Thanks! Nathan
