On 2024/05/20 23:36, Timofey Zhakov 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!
Found another similar issue in -c option. The following works correctly: $ ~/svn/1.14.3/bin/svn diff -c '0-1' file:///tmp/svnrepos svn: E205000: There is no change 0 However, the following aborts: $ ~/svn/1.14.3/bin/svn diff -c '1-0' file:///tmp/svnrepos svn: E235000: In file 'subversion/libsvn_client/ra.c' line 692: assertion failed (SVN_IS_VALID_REVNUM(start_revnum)) Aborted -- Jun Omae <jun6...@gmail.com> (大前 潤)
