On Sun, May 26, 2024 at 2:34 PM Timofey Zhakov <[email protected]> wrote:
>
> On Wed, May 22, 2024 at 10:26 PM Timofey Zhakov <[email protected]> wrote:
> >
> > 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.
> [...]
>
> Hello!
>
> I'm finished with the implementation of this refactoring. I decided to
> factor out only the parse of a single change instead of parsing the
> entire collection of the change arguments, separated by comma or
> whitespace; this logic I left in the svn.c file.
>
> Additionally, I found that there is a similar option in the svnbench
> null-log command, so the svn_opt_parse_change_to_range() function can
> also be used there. I'm going to prepare a separate patch for this.
>
> [[[
> Factor-out parse 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 and add the unit tests
> on the function.
>
> * 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:sub_main() function.
> * subversion/svn/svn.c
> (sub_main): Use the factored-out function instead of doing the work inline.
> (includes): Remove private/svn_opt_private.h, because it is no longer
> needed.
> * subversion/tests/cmdline/diff_tests.py
> (diff_invalid_change_arg): Update the test expectations to account for the
> updated error messages.
> * subversion/tests/libsvn_subr/opt-test.c
> (includes): Include private/svn_opt_private.h to use the
> svn_opt__revision_to_string() function.
> (revision_ranges_to_string): New function.
> (test_svn_opt_parse_change_to_range): New test.
> (test_funcs): Run new test.
> ]]]
>
> --
> Timofei Zhakov
Hi Timofei,
I recommend two minor changes.
Rather than describe them inline, I'll just attach a modified patch
for convenience ("v7").
Please let me know if these are agreeable to you, and I'll go ahead
and commit...
The changes are:
(1) Adding comments: Previously, when the --change parsing was in
svn.c::sub_main(), the text of the individual error messages helped
understand the various checks for zero, negative, non-numeric, etc. As
the error texts are no longer mixed with the parsing code, the checks
are less obvious, so I added those texts as comments.
(2) Minor mistake in docstring: In svn_opt.h the docstring for
svn_opt_parse_change_to_range() referred to parameter "pool" but it is
actually called "result_pool".
Otherwise, looks good.
Thanks for helping!
Nathan
Index: subversion/include/svn_opt.h
===================================================================
--- subversion/include/svn_opt.h (revision 1918056)
+++ subversion/include/svn_opt.h (working copy)
@@ -534,6 +534,30 @@ svn_opt_parse_revision_to_range(apr_array_header_t
apr_pool_t *pool);
/**
+ * Parse @a arg, where @a arg is "N", "-N", "N-M" into a
+ * @c svn_opt_revision_range_t and push that onto @a opt_ranges.
+ *
+ * - If @a arg is "N", set the @c start field of the
+ * @c svn_opt_revision_range_t to N-1 and @c end field to N.
+ *
+ * - If @a arg is "-N", set the @c start field of the
+ * @c svn_opt_revision_range_t to N and @c end field to N-1.
+ *
+ * - If @a arg is "N-M", set the @c start field of the
+ * @c svn_opt_revision_range_t to N-1 and @c end field to M.
+ *
+ * If @a arg is invalid, return -1; else return 0.
+ *
+ * Use @a result_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 *result_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 1918056)
+++ subversion/libsvn_subr/opt.c (working copy)
@@ -629,6 +629,86 @@ 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 *result_pool)
+{
+ char *end;
+ svn_revnum_t changeno, changeno_end;
+ const char *s = arg;
+ 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 == '-')
+ {
+ /* Negative number in range not supported with -c */
+ if (changeno < 0 || is_negative)
+ return -1;
+
+ s = end + 1;
+ while (*s == 'r')
+ s++;
+ changeno_end = strtol(s, &end, 10);
+
+ /* Negative number in range not supported with -c */
+ if (changeno_end < 0)
+ return -1;
+ }
+
+ /* Non-numeric change argument given to -c? */
+ if (end == arg || *end != '\0')
+ return -1;
+
+ /* There is no change 0 */
+ if (changeno == 0 || changeno_end == 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,
+ result_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 1918056)
+++ subversion/svn/svn.c (working copy)
@@ -55,7 +55,6 @@
#include "shelf2-cmd.h"
#include "shelf-cmd.h"
-#include "private/svn_opt_private.h"
#include "private/svn_cmdline_private.h"
#include "private/svn_subr_private.h"
#include "private/svn_utf_private.h"
@@ -2344,98 +2343,18 @@ sub_main(int *exit_code, int argc, const char *arg
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 (svn_opt_parse_change_to_range(opt_state.revision_ranges,
+ change_str, pool) != 0)
{
- 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);
+ _("Syntax error in change argument "
+ "'%s'"), change_str);
}
- if (changeno == 0 || changeno_end == 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);
}
}
break;
Index: subversion/tests/cmdline/diff_tests.py
===================================================================
--- subversion/tests/cmdline/diff_tests.py (revision 1918056)
+++ subversion/tests/cmdline/diff_tests.py (working copy)
@@ -5336,33 +5336,33 @@ 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')
svntest.actions.run_and_verify_svn(
None,
- (r'.*svn: E205000: There is no change 0'),
+ (r'.*svn: E205000: Syntax error in change argument \'1-0\''),
'diff', sbox.wc_dir, '-c', '1-0')
########################################################################
Index: subversion/tests/libsvn_subr/opt-test.c
===================================================================
--- subversion/tests/libsvn_subr/opt-test.c (revision 1918056)
+++ subversion/tests/libsvn_subr/opt-test.c (working copy)
@@ -27,6 +27,7 @@
#include "../svn_test.h"
#include "svn_opt.h"
+#include "private/svn_opt_private.h"
static svn_error_t *
@@ -190,6 +191,78 @@ test_svn_opt_args_to_target_array2(apr_pool_t *poo
return SVN_NO_ERROR;
}
+static const char *
+revision_ranges_to_string(apr_array_header_t *ranges,
+ apr_pool_t *result_pool,
+ apr_pool_t *scratch_pool)
+{
+ svn_stringbuf_t *result = svn_stringbuf_create_empty(result_pool);
+ int i;
+
+ for (i = 0; i < ranges->nelts; i++)
+ {
+ svn_opt_revision_range_t *range =
+ APR_ARRAY_IDX(ranges, i, svn_opt_revision_range_t *);
+
+ if (i > 0)
+ {
+ svn_stringbuf_appendcstr(result, ",");
+ }
+
+ svn_stringbuf_appendcstr(result,
+ svn_opt__revision_to_string(&range->start,
+ scratch_pool));
+ svn_stringbuf_appendcstr(result, ":");
+
+ svn_stringbuf_appendcstr(result,
+ svn_opt__revision_to_string(&range->end,
+ scratch_pool));
+ }
+
+ return result->data;
+}
+
+static svn_error_t*
+test_svn_opt_parse_change_to_range(apr_pool_t *pool)
+{
+ int i;
+ static struct {
+ const char *input;
+ const int expected_rv;
+ const char *expected_range;
+ } const tests[] = {
+ { "123", 0, "122:123"},
+ { "r123", 0, "122:123"},
+ { "-123", 0, "123:122"},
+ { "-r123", 0, "123:122"},
+ { "r-123", 0, "123:122"},
+ { "--123", -1, ""},
+ { "-r-123", -1, ""},
+ { "123-456", 0, "122:456"},
+ { "r123-r456", 0, "122:456"},
+ { "--r123", -1, ""},
+ { "123--456", -1, ""},
+ { "1-0", -1, ""},
+ { "0-1", -1, ""},
+ };
+
+ for (i = 0; i < sizeof(tests) / sizeof(tests[0]); i++)
+ {
+ apr_array_header_t *ranges =
+ apr_array_make(pool, 0, sizeof(svn_opt_revision_range_t *));
+
+ SVN_TEST_INT_ASSERT(
+ svn_opt_parse_change_to_range(ranges, tests[i].input, pool),
+ tests[i].expected_rv);
+
+ SVN_TEST_STRING_ASSERT(
+ revision_ranges_to_string(ranges, pool, pool),
+ tests[i].expected_range);
+ }
+
+ return SVN_NO_ERROR;
+}
+
/* The test table. */
@@ -202,6 +275,8 @@ static struct svn_test_descriptor_t test_funcs[] =
"test svn_opt_parse_path"),
SVN_TEST_PASS2(test_svn_opt_args_to_target_array2,
"test svn_opt_args_to_target_array2"),
+ SVN_TEST_PASS2(test_svn_opt_parse_change_to_range,
+ "test svn_opt_parse_change_to_range"),
SVN_TEST_NULL
};
