Alexander Thomas <alexander@collab.net> writes:
> [[[
> Fix issue #2317: Handles filenames containing '@' for svn commands
> supporting peg revision.
>
> * subversion/libsvn_subr/opt.c
> (svn_opt_parse_path): Translates empty suffix '@' sign in
> filename to '@HEAD' or '@BASE' depending on URL or PATH respectively.
>
> * subversion/tests/clients/cmdline/basic_tests.py
> (basic_peg_revision): New test.
> (test_list): Added basic_peg_revision to list.
> ]]]
> Fix issue #2317: Handles filenames containing '@' for svn commands
> supporting peg revision.
>
> * subversion/libsvn_subr/opt.c
> (svn_opt_parse_path): Translates empty suffix '@' sign in
> filename to '@HEAD' or '@BASE' depending on URL or PATH respectively.
>
> * subversion/tests/clients/cmdline/basic_tests.py
> (basic_peg_revision): New test.
> (test_list): Added basic_peg_revision to list.
Please include the log message exactly once. It can be in the body of
the message (using "[[[ ... ]]]"), or it can be an attachment, or
wherever -- just as long as it's not duplicated.
> Index: subversion/libsvn_subr/opt.c
> ===================================================================
> --- subversion/libsvn_subr/opt.c (revision 15206)
> +++ subversion/libsvn_subr/opt.c (working copy)
> @@ -498,13 +498,37 @@
>
> if (path[i] == '@')
> {
> + svn_boolean_t is_url;
> + int ret;
> svn_opt_revision_t start_revision, end_revision;
>
> end_revision.kind = svn_opt_revision_unspecified;
> - if (svn_opt_parse_revision (&start_revision,
> - &end_revision,
> - path + i + 1, pool)
> - || end_revision.kind != svn_opt_revision_unspecified)
> +
> + /* URLs and wc-paths get treated differently. */
> + is_url = svn_path_is_url (path);
Say instead "URLs get treated different from wc paths" to avoid
ambiguity. (Otherwise, it might mean "URLs and wc paths get treated
different from some other thing").
> +
> + /* If empty peg-rev was attached to URL target, then assume HEAD. */
> + if (path [i + 1] == '\0' && is_url)
> + {
> + ret = svn_opt_parse_revision (&start_revision,
> + &end_revision,
> + "head", pool);
> + }
> + /* If empty peg-rev was attached to PATH target, then assume BASE. */
> + else if (path [i + 1] == '\0' && !is_url)
Say "path[i + 1]"; we don't put a space before an array bracket.
> + {
> + ret = svn_opt_parse_revision (&start_revision,
> + &end_revision,
> + "base", pool);
> + }
> + else
> + {
> + ret = svn_opt_parse_revision (&start_revision,
> + &end_revision,
> + path + i + 1, pool);
> + }
To me it felt weird to test path[i + 1] twice. I'd write instead
if (path[i + 1] == '\0')
{
if (is_url)
...;
else /* wc path */
...;
}
else
{
...;
}
It's not a big deal -- it won't make any noticeable difference at
run-time, it's just a question of reading flow, really.
> +
> + if (ret || end_revision.kind != svn_opt_revision_unspecified)
> return svn_error_createf (SVN_ERR_CL_ARG_PARSING_ERROR, NULL,
> _("Syntax error parsing revision '%s'"),
> path + i + 1);
> Index: subversion/tests/clients/cmdline/basic_tests.py
> ===================================================================
> --- subversion/tests/clients/cmdline/basic_tests.py (revision 15206)
> +++ subversion/tests/clients/cmdline/basic_tests.py (working copy)
> @@ -1584,6 +1584,30 @@
> "lambda"))
> check_repos_root(output)
>
> +def basic_peg_revision(sbox):
> + "checks peg revision on filename with @ sign"
> +
> + sbox.build()
> + wc_dir = sbox.wc_dir
> + repos_dir = sbox.repo_url
> + filename = 'abc@abc'
> +
> + wc_file = wc_dir + '/' + filename
> + repos_file = repos_dir + '/' + filename
Typically we call it 'repos_url' or 'url' or something, instead of
'repos_file'. No big deal, just alerting you to the convention.
> +
> + svntest.main.file_append(wc_file, 'A probe into the secrets about the Solar System')
Please keep lines under 80 columns, as per HACKING.
> + svntest.main.run_svn(None, 'add', wc_file)
> + svntest.main.run_svn(None, 'ci', '-m', 'secret log msg', wc_file)
> +
> + output, errlines = svntest.main.run_svn(0, 'cat', wc_file+'@')
> + if len(output) < 1:
> + raise svntest.Failure
> +
> + output, errlines = svntest.main.run_svn(0, 'cat', repos_file+'@')
> + if len(output) < 1:
> + raise svntest.Failure
Better to use svntest.actions.run_and_verify_svn() -- this sort of
thing is exactly what it's for.
Also, it would be good to test the failure case:
$ svn cat abc@abc
subversion/libsvn_subr/opt.c:508: (apr_err=205000)
svn: Syntax error parsing revision 'abc'
$
By the way, I think you didn't run the entire 'make check' on this
change :-). It causes subversion/tests/libsvn_subr/opt-test to fail:
START: opt-test
subversion/tests/libsvn_subr/opt-test.c:83: (apr_err=200006)
svn_tests: svn_opt_parse_path ('foo/bar@') returned 'foo/bar' \
instead of 'NULL'
FAIL: lt-opt-test 1: test svn_opt_parse_path
END: opt-test
Obviously your code is correct; it's the test that needs to be adjusted.
> #----------------------------------------------------------------------
> ########################################################################
> # Run the tests
> @@ -1617,6 +1641,7 @@
> basic_add_local_ignores,
> basic_add_no_ignores,
> repos_root,
> + basic_peg_revision,
> ### todo: more tests needed:
> ### test "svn rm http://some_url"
> ### not sure this file is the right place, though.
I've made all the above changes, and committed in r15289. Thanks!
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Thu Jul 7 21:37:33 2005