On Tue, May 21, 2024 at 9:23 PM Nathan Hartman <hartman.nat...@gmail.com> wrote: > > On Mon, May 20, 2024 at 10:37 AM Timofey Zhakov <t...@chemodax.net> wrote: > > > > On Mon, May 20, 2024 at 7:03 AM Nathan Hartman <hartman.nat...@gmail.com> > > wrote: > > > > > > On Sun, May 19, 2024 at 4:09 PM Timofey Zhakov <t...@chemodax.net> wrote: > > > > > > > > Hi, > > > > > > > > I found a little bug in parsing a change revision: If the number, > > > > given to the --change argument, starts with a double minus or with > > > > `-r-`, the command aborts. This patch fixes this bug. > > > > > > > > Steps to reproduce: > > > > > > > > $ svn diff https://svn.apache.org/repos/asf -c --123 > > > > svn: E235000: In file '..\..\..\subversion\libsvn_client\ra.c' line > > > > 692: assertion failed (SVN_IS_VALID_REVNUM(start_revnum)) > > > > > > > > Or... > > > > > > > > $ svn diff https://svn.apache.org/repos/asf -c -r-123 > > > > svn: E235000: In file '..\..\..\subversion\libsvn_client\ra.c' line > > > > 692: assertion failed (SVN_IS_VALID_REVNUM(start_revnum)) > > > > > > > > The same would happen if the svn diff command is invoked from a > > > > working copy, without URL. > > > > > > > > [[[ > > > > Fix bug: check the change argument for a double minus at the start. > > > > > > > > If changeno is negative and is_negative is TRUE, raise > > > > SVN_ERR_CL_ARG_PARSING_ERROR, because string with a double minus is > > > > not a valid number. > > > > > > > > * subversion/svn/svn.c > > > > (sub_main): If changeno is negative and is_negative is TRUE, raise > > > > SVN_ERR_CL_ARG_PARSING_ERROR. > > > > * subversion/tests/cmdline/diff_tests.py > > > > (diff_invalid_change_arg): New test. > > > > (test_list): Run new test. > > > > ]]] > > > > > > > > Best regards, > > > > Timofei Zhakov > > > > > > > > > Good catch! I agree we should issue a proper error and not assert. > > > > > > While studying this change, I noticed a second similar bug if we are > > > given a range and the second value is negative. > > > > > > In other words, this works correctly: > > > > > > $ svn diff -c r1917671-1917672 > > > http://svn.apache.org/repos/asf/subversion/trunk > > > > > > But this doesn't; notice the double minus signs: > > > > > > $ svn diff -c r1917671--1917672 > > > http://svn.apache.org/repos/asf/subversion/trunk > > > svn: E235000: In file 'subversion/libsvn_client/ra.c' line 682: > > > assertion failed (SVN_IS_VALID_REVNUM(start_revnum)) > > > Abort trap: 6 > > > > > > The first minus sign is correctly interpreted to indicate a range, but > > > the second minus sign is read by strtol(). That's several lines above > > > your change in svn.c (line 2379 on trunk@1917826). We are doing: > > > > > > changeno_end = strtol(s, &end, 10); > > > > > > but we are not checking if 'changeno_end' is negative (which it isn't > > > allowed to be). > > > > > > I'm okay to commit this patch now and handle the second bug in a > > > follow-up, or handle both bugs together. Let me know what you prefer. > > > > > > Cheers, > > > Nathan > > > > Thank you for your response! > > > > I didn't notice the second bug. I think it would be better to commit > > them together, so I fixed and added it to the new patch. The patch is > > in the attachments. > > > > Here is an updated log message: > > > > [[[ > > Fix bug: check the change argument for a double minus. > > > > If the changeno is negative and is_negative is TRUE, raise > > SVN_ERR_CL_ARG_PARSING_ERROR, because string with a double minus is > > not a valid number. Do the same if the changeno_end is negative. > > > > * subversion/svn/svn.c > > (sub_main): If the changeno is negative and is_negative is TRUE, raise > > SVN_ERR_CL_ARG_PARSING_ERROR. Do the same if the changeno_end is > > negative. > > * subversion/tests/cmdline/diff_tests.py > > (diff_invalid_change_arg): New test. > > (test_list): Run new test. > > ]]] > > > > Additionally, I am thinking about moving the code, which parses a > > change revision, into a function in the opt.c file, like > > svn_opt_parse_revision_to_range. > > > > Thanks! > > > Patch v2 (svn-check-change-for-double-minus-v2.patch.txt) committed in > r1917864 and nominated for backport to 1.14.x in r1917865. > > Thanks for finding and fixing this bug! > > Cheers, > Nathan
Thank you! -- Timofei Zhakov
