Hi Nathan,
On Wed, May 22, 2024 at 4:06 PM Nathan Hartman <[email protected]> wrote:
>
> On Tue, May 21, 2024 at 10:31 AM Timofey Zhakov <[email protected]> wrote:
[...]
> 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.
I agree that the error message would become worse, but I think this
would be acceptable, because the same error messages are used in the
parser of the --revision argument.
[...]
> 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.
Sure, see my draft patch in the attachments. I haven't started with
the unit tests yet.
[[[
Factor-out parsing of the --change argument from command line to libsvn_subr.
These changes factor-out the function, which parses a change revision, from
the command line interface to the libsvn_subr library. The function will be
called as svn_opt_parse_revision_to_range and declared in the svn_opt.h header
file.
Breaking changes:
All the error messages have to be converted into one, so the behavior of the
command line interface is changed.
* subversion\include\svn_opt.h
(svn_opt_parse_revision_to_range): Declare new function.
* subversion\libsvn_subr\opt.c
(svn_opt_parse_revision_to_range): Factor-out the function from the svn.c
file.
* subversion\svn\svn.c
(sub_main): Use the factored-out function instead of doing the work inline.
* subversion\tests\cmdline\diff_tests.py
(diff_invalid_change_arg): Update the test expectations to follow new
changes.
TODO:
* Write the function description and examples.
* Word-out the error message.
* Add unit tests for the svn_opt_parse_revision_to_range function.
]]]
--
Timofei Zhakov
Index: subversion/include/svn_opt.h
===================================================================
--- subversion/include/svn_opt.h (revision 1917907)
+++ subversion/include/svn_opt.h (working copy)
@@ -534,6 +534,22 @@ svn_opt_parse_revision_to_range(apr_array_header_t
apr_pool_t *pool);
/**
+ * Parse @a arg, into a @c svn_opt_revision_range_t and push that onto
+ * @a opt_ranges.
+ *
+ * TODO: Write the function description and examples.
+ *
+ * If @a arg is invalid, return -1; else return 0.
+ *
+ * Use @a pool to allocate @c svn_opt_revision_range_t pushed to the array.
+ *
+ * @since New in 1.15.
+ */
+int svn_opt_parse_change_to_range(apr_array_header_t *opt_ranges,
+ const char *arg,
+ apr_pool_t *pool);
+
+/**
* Resolve peg revisions and operational revisions in the following way:
*
* - If @a is_url is set and @a peg_rev->kind is
Index: subversion/libsvn_subr/opt.c
===================================================================
--- subversion/libsvn_subr/opt.c (revision 1917907)
+++ subversion/libsvn_subr/opt.c (working copy)
@@ -629,6 +629,101 @@ svn_opt_parse_revision_to_range(apr_array_header_t
return 0;
}
+int svn_opt_parse_change_to_range(apr_array_header_t *opt_ranges,
+ const char *arg,
+ apr_pool_t *pool)
+{
+ int i;
+ apr_array_header_t *change_revs;
+
+ change_revs = svn_cstring_split(arg, ", \n\r\t\v", TRUE,
+ pool);
+
+ for (i = 0; i < change_revs->nelts; i++)
+ {
+ char *end;
+ svn_revnum_t changeno, changeno_end;
+ const char *change_str =
+ APR_ARRAY_IDX(change_revs, i, const char *);
+ const char *s = change_str;
+ svn_boolean_t is_negative;
+
+ /* Check for a leading minus to allow "-c -r42".
+ * The is_negative flag is used to handle "-c -42" and "-c -r42".
+ * The "-c r-42" case is handled by strtol() returning a
+ * negative number. */
+ is_negative = (*s == '-');
+ if (is_negative)
+ s++;
+
+ /* Allow any number of 'r's to prefix a revision number. */
+ while (*s == 'r')
+ s++;
+ changeno = changeno_end = strtol(s, &end, 10);
+ if (end != s && *end == '-')
+ {
+ if (changeno < 0 || is_negative)
+ {
+ return -1;
+ }
+ s = end + 1;
+ while (*s == 'r')
+ s++;
+ changeno_end = strtol(s, &end, 10);
+
+ if (changeno_end < 0)
+ {
+ return -1;
+ }
+ }
+ if (end == change_str || *end != '\0')
+ {
+ return -1;
+ }
+
+ if (changeno == 0)
+ {
+ return -1;
+ }
+
+ /* The revision number cannot contain a double minus */
+ if (changeno < 0 && is_negative)
+ {
+ return -1;
+ }
+
+ if (is_negative)
+ changeno = -changeno;
+
+ /* Figure out the range:
+ -c N -> -r N-1:N
+ -c -N -> -r N:N-1
+ -c M-N -> -r M-1:N for M < N
+ -c M-N -> -r M:N-1 for M > N
+ -c -M-N -> error (too confusing/no valid use case)
+ */
+ if (changeno > 0)
+ {
+ if (changeno <= changeno_end)
+ changeno--;
+ else
+ changeno_end--;
+ }
+ else
+ {
+ changeno = -changeno;
+ changeno_end = changeno - 1;
+ }
+
+ APR_ARRAY_PUSH(opt_ranges,
+ svn_opt_revision_range_t *)
+ = svn_opt__revision_range_from_revnums(changeno, changeno_end,
+ pool);
+ }
+
+ return 0;
+}
+
svn_error_t *
svn_opt_resolve_revisions(svn_opt_revision_t *peg_rev,
svn_opt_revision_t *op_rev,
Index: subversion/svn/svn.c
===================================================================
--- subversion/svn/svn.c (revision 1917907)
+++ subversion/svn/svn.c (working copy)
@@ -2330,112 +2330,19 @@ sub_main(int *exit_code, int argc, const char *arg
break;
case 'c':
{
- apr_array_header_t *change_revs;
-
+ opt_state.used_change_arg = TRUE;
SVN_ERR(svn_utf_cstring_to_utf8(&utf8_opt_arg, opt_arg, pool));
- change_revs = svn_cstring_split(utf8_opt_arg, ", \n\r\t\v", TRUE,
- pool);
-
if (opt_state.old_target)
{
return svn_error_create(SVN_ERR_CL_ARG_PARSING_ERROR, NULL,
_("Can't specify -c with --old"));
}
-
- for (i = 0; i < change_revs->nelts; i++)
+ if (svn_opt_parse_change_to_range(opt_state.revision_ranges,
+ utf8_opt_arg, pool) != 0)
{
- char *end;
- svn_revnum_t changeno, changeno_end;
- const char *change_str =
- APR_ARRAY_IDX(change_revs, i, const char *);
- const char *s = change_str;
- svn_boolean_t is_negative;
-
- /* Check for a leading minus to allow "-c -r42".
- * The is_negative flag is used to handle "-c -42" and "-c -r42".
- * The "-c r-42" case is handled by strtol() returning a
- * negative number. */
- is_negative = (*s == '-');
- if (is_negative)
- s++;
-
- /* Allow any number of 'r's to prefix a revision number. */
- while (*s == 'r')
- s++;
- changeno = changeno_end = strtol(s, &end, 10);
- if (end != s && *end == '-')
- {
- if (changeno < 0 || is_negative)
- {
- return svn_error_createf(SVN_ERR_CL_ARG_PARSING_ERROR,
- NULL,
- _("Negative number in range
(%s)"
- " not supported with -c"),
- change_str);
- }
- s = end + 1;
- while (*s == 'r')
- s++;
- changeno_end = strtol(s, &end, 10);
-
- if (changeno_end < 0)
- {
- return svn_error_createf(
- SVN_ERR_CL_ARG_PARSING_ERROR, NULL,
- _("Negative number in range (%s)"
- " not supported with -c"),
- change_str);
- }
- }
- if (end == change_str || *end != '\0')
- {
- return svn_error_createf(SVN_ERR_CL_ARG_PARSING_ERROR, NULL,
- _("Non-numeric change argument (%s)
"
- "given to -c"), change_str);
- }
-
- if (changeno == 0)
- {
- return svn_error_create(SVN_ERR_CL_ARG_PARSING_ERROR, NULL,
- _("There is no change 0"));
- }
-
- /* The revision number cannot contain a double minus */
- if (changeno < 0 && is_negative)
- {
- return svn_error_createf(SVN_ERR_CL_ARG_PARSING_ERROR, NULL,
- _("Non-numeric change argument "
- "(%s) given to -c"), change_str);
- }
-
- if (is_negative)
- changeno = -changeno;
-
- /* Figure out the range:
- -c N -> -r N-1:N
- -c -N -> -r N:N-1
- -c M-N -> -r M-1:N for M < N
- -c M-N -> -r M:N-1 for M > N
- -c -M-N -> error (too confusing/no valid use case)
- */
- if (changeno > 0)
- {
- if (changeno <= changeno_end)
- changeno--;
- else
- changeno_end--;
- }
- else
- {
- changeno = -changeno;
- changeno_end = changeno - 1;
- }
-
- opt_state.used_change_arg = TRUE;
- APR_ARRAY_PUSH(opt_state.revision_ranges,
- svn_opt_revision_range_t *)
- = svn_opt__revision_range_from_revnums(changeno, changeno_end,
- pool);
+ return svn_error_createf(SVN_ERR_CL_ARG_PARSING_ERROR, NULL,
+ _("Syntax error in change argument "
+ "'%s'"), utf8_opt_arg);
}
}
break;
Index: subversion/tests/cmdline/diff_tests.py
===================================================================
--- subversion/tests/cmdline/diff_tests.py (revision 1917907)
+++ subversion/tests/cmdline/diff_tests.py (working copy)
@@ -5336,28 +5336,28 @@ def diff_invalid_change_arg(sbox):
svntest.actions.run_and_verify_svn(
None,
- (r'.*svn: E205000: Non-numeric change argument \(--1\) given to -c'),
+ (r'.*svn: E205000: Syntax error in change argument \'--1\''),
'diff', sbox.wc_dir, '-c', '--1')
svntest.actions.run_and_verify_svn(
None,
- (r'.*svn: E205000: Non-numeric change argument \(-r-1\) given to -c'),
+ (r'.*svn: E205000: Syntax error in change argument \'-r-1\''),
'diff', sbox.wc_dir, '-c', '-r-1')
svntest.actions.run_and_verify_svn(
None,
- (r'.*svn: E205000: Negative number in range \(1--3\) not supported with
-c'),
+ (r'.*svn: E205000: Syntax error in change argument \'1--3\''),
'diff', sbox.wc_dir, '-c', '1--3')
# 'r' is not a number
svntest.actions.run_and_verify_svn(
None,
- (r'.*svn: E205000: Non-numeric change argument \(r1--r3\) given to -c'),
+ (r'.*svn: E205000: Syntax error in change argument \'r1--r3\''),
'diff', sbox.wc_dir, '-c', 'r1--r3')
svntest.actions.run_and_verify_svn(
None,
- (r'.*svn: E205000: Negative number in range \(r1-r-3\) not supported with
-c'),
+ (r'.*svn: E205000: Syntax error in change argument \'r1-r-3\''),
'diff', sbox.wc_dir, '-c', 'r1-r-3')
########################################################################
