Noorul Islam K M <noorul_at_collab.net> writes:
> Julian Foad <julian.foad_at_wandisco.com> writes:
>
>> On Sat, 2010-12-04, Noorul Islam K M wrote:
>>
>>> Julian Foad <julian.foad_at_wandisco.com> writes:
>>> > On Fri, 2010-12-03, Noorul Islam K M wrote:
>>> >> Julian Foad <julian.foad_at_wandisco.com> writes:
>>> >> > I think we should change this behaviour and make "svn update" throw an
>>> >> > error if any target is a URL.
>>> >>
>>> >> Attached is the patch for same.
>>> > [...]
>>> >> Make 'svn update' verify that URLs are not passed as targets.
>>> >>
>>> >> * subversion/svn/update-cmd.c,
>>> >> subversion/libsvn_client/update.c:
>>> >> (svn_cl__update, svn_client_update4): Raise an error if a URL was
>>> >> passed. Remove code that notifies 'Skipped' message for URL targets.
>>> > [...]
>>
>>> Please find attached the updated patch.
>>
>> Hi Noorul.
>>
>> Several tests fail with this patch.
>>
>> FAIL: basic_tests.py 4: basic update command
>> FAIL: commit_tests.py 56: 'svn commit --changelist=foo' above a
>> conflict
>> FAIL: externals_tests.py 15: should not be able to mv or rm a file
>> external
>> FAIL: externals_tests.py 16: place a file external into a directory
>> external
>> [...]
>>
>> Before you send a patch, always run the test suite ("make check" or
>> similar) and check that all tests pass. If you have done no testing or
>> different testing or you found failures but want to send the patch
>> anyway, then simply say so in the email, so we know. If you don't say
>> anything, we will assume you ran the test suite and all tests pass.
>>
>
> I will keep this in mind. I actually added a test case for this as part
> of the patch. My mistake to not run the entire test suite. I will send
> follow-up patch with modified test cases.
>
Ran test suite and found some failures and fixed them. All of them were
passing URL to 'svn up' which is not at all required. Now all tests are
passing with the below attached patch.
Log
[[[
Make 'svn update' verify that URLs are not passed as targets.
* subversion/svn/update-cmd.c,
subversion/libsvn_client/update.c:
(svn_cl__update, svn_client_update4): Raise an error if a URL was
passed. Remove code that notifies 'Skipped' message for URL targets.
* subversion/tests/cmdline/input_validation_tests.py
(invalid_update_targets, test_list): New test
* subversion/tests/cmdline/externals_tests.py,
subversion/tests/cmdline/svntest/actions.py,
(cannot_move_or_remove_file_externals,
can_place_file_external_into_dir_external,
external_into_path_with_spaces,
inject_conflict_into_wc): Remove URL target from 'svn up' command.
* subversion/tests/cmdline/basic_tests.py
(basic_update): URLs are no longer accepted as targets. Remove test
case for 'Skipped' message.
Patch by: Noorul Islam K M <noorul{_AT_}collab.net>
]]]
Thanks and Regards
Noorul
Index: subversion/tests/cmdline/externals_tests.py
===================================================================
--- subversion/tests/cmdline/externals_tests.py (revision 1042948)
+++ subversion/tests/cmdline/externals_tests.py (working copy)
@@ -1007,8 +1007,7 @@
# Bring the working copy up to date and check that the file the file
# external is switched to still exists.
svntest.actions.run_and_verify_svn(None, None, [],
- 'up',
- repo_url, wc_dir)
+ 'up', wc_dir)
open(os.path.join(wc_dir, 'A', 'D', 'gamma')).close()
@@ -1036,8 +1035,7 @@
# Bring the working copy up to date and check that the file the file
# external is switched to still exists.
svntest.actions.run_and_verify_svn(None, None, [],
- 'up',
- repo_url, wc_dir)
+ 'up', wc_dir)
beta1_path = os.path.join(wc_dir, 'A', 'B', 'E', 'beta')
beta1_contents = open(beta1_path).read()
@@ -1064,8 +1062,7 @@
None,
expected_error,
1,
- 'up',
- repo_url, wc_dir)
+ 'up', wc_dir)
#----------------------------------------------------------------------
@@ -1082,8 +1079,7 @@
change_external(wc_dir, ext)
svntest.actions.run_and_verify_svn(None, None, [],
- 'up',
- repo_url, wc_dir)
+ 'up', wc_dir)
probe_paths_exist([
os.path.join(wc_dir, 'A', 'copy of D'),
os.path.join(wc_dir, 'A', 'another copy of D'),
@@ -1334,8 +1330,7 @@
externals_desc = '^/A/B/E external'
change_external(A_path, externals_desc)
svntest.actions.run_and_verify_svn(None, None, [],
- 'up',
- repo_url, wc_dir)
+ 'up', wc_dir)
# create another repository
other_repo_dir, other_repo_url = sbox.add_repo_path('other')
Index: subversion/tests/cmdline/input_validation_tests.py
===================================================================
--- subversion/tests/cmdline/input_validation_tests.py (revision 1042948)
+++ subversion/tests/cmdline/input_validation_tests.py (working copy)
@@ -242,6 +242,12 @@
run_and_verify_svn_in_wc(sbox, "svn: Cannot mix repository and working "
"copy targets", 'mkdir', "folder", "^/folder")
+def invalid_update_targets(sbox):
+ "non-working copy paths for 'update'"
+ sbox.build(read_only=True)
+ run_and_verify_svn_in_wc(sbox, "svn:.*is not a local path", 'update',
+ "^/")
+
########################################################################
# Run the tests
@@ -270,6 +276,7 @@
invalid_switch_targets,
invalid_relocate_targets,
invalid_mkdir_targets,
+ invalid_update_targets,
]
if __name__ == '__main__':
Index: subversion/tests/cmdline/svntest/actions.py
===================================================================
--- subversion/tests/cmdline/svntest/actions.py (revision 1042948)
+++ subversion/tests/cmdline/svntest/actions.py (working copy)
@@ -1852,7 +1852,6 @@
conflicting_contents, contents,
merged_rev)
exit_code, output, errput = main.run_svn(None, "up", "-r", str(merged_rev),
- sbox.repo_url + "/" + state_path,
file_path)
if expected_status:
expected_status.tweak(state_path, wc_rev=merged_rev)
Index: subversion/tests/cmdline/basic_tests.py
===================================================================
--- subversion/tests/cmdline/basic_tests.py (revision 1042948)
+++ subversion/tests/cmdline/basic_tests.py (working copy)
@@ -211,16 +211,6 @@
"update xx/xx", [], [],
'update', '--quiet', xx_path)
- # URL's are also skipped.
- urls = ('http://localhost/a/b/c', 'http://localhost', 'svn://localhost')
- for url in urls:
- exit_code, out, err = svntest.actions.run_and_verify_svn(
- "update " + url,
- ["Skipped '"+url+"'\n",
- "Summary of conflicts:\n",
- " Skipped paths: 1\n"], [],
- 'update', url)
-
#----------------------------------------------------------------------
def basic_mkdir_url(sbox):
"basic mkdir URL"
Index: subversion/svn/update-cmd.c
===================================================================
--- subversion/svn/update-cmd.c (revision 1042948)
+++ subversion/svn/update-cmd.c (working copy)
@@ -110,6 +110,7 @@
svn_boolean_t depth_is_sticky;
struct svn_cl__check_externals_failed_notify_baton nwb;
apr_array_header_t *result_revs;
+ int i;
SVN_ERR(svn_cl__args_to_target_array_print_reserved(&targets, os,
opt_state->targets,
@@ -120,30 +121,16 @@
SVN_ERR(svn_cl__eat_peg_revisions(&targets, targets, scratch_pool));
- /* If any targets are URLs, notify that we're skipping them and remove
- them from TARGETS. Although svn_client_update4() would skip them
- anyway, we don't want to pass invalid targets to it, and especially
- not to print_update_summary(). */
- {
- apr_array_header_t *new_targets
- = apr_array_make(scratch_pool, targets->nelts, sizeof(const char *));
- int i;
+ /* If any targets are URLs, display error message and exit. */
+ for (i = 0; i < targets->nelts; i++)
+ {
+ const char *target = APR_ARRAY_IDX(targets, i, const char *);
+
+ if (svn_path_is_url(target))
+ return svn_error_createf(SVN_ERR_CL_ARG_PARSING_ERROR, NULL,
+ _("'%s' is not a local path"), target);
+ }
- for (i = 0; i < targets->nelts; i++)
- {
- const char *target = APR_ARRAY_IDX(targets, i, const char *);
-
- if (svn_path_is_url(target))
- ctx->notify_func2(ctx->notify_baton2,
- svn_wc_create_notify_url(
- target, svn_wc_notify_skip, scratch_pool),
- scratch_pool);
- else
- APR_ARRAY_PUSH(new_targets, const char *) = target;
- }
- targets = new_targets;
- }
-
/* If using changelists, convert targets into a set of paths that
match the specified changelist(s). */
if (opt_state->changelists)
Index: subversion/libsvn_client/update.c
===================================================================
--- subversion/libsvn_client/update.c (revision 1042948)
+++ subversion/libsvn_client/update.c (working copy)
@@ -397,68 +397,50 @@
for (i = 0; i < paths->nelts; ++i)
{
+ path = APR_ARRAY_IDX(paths, i, const char *);
+
+ if (svn_path_is_url(path))
+ return svn_error_createf(SVN_ERR_ILLEGAL_TARGET, NULL,
+ _("'%s' is not a local path"), path);
+ }
+
+ for (i = 0; i < paths->nelts; ++i)
+ {
svn_boolean_t sleep;
- svn_boolean_t skipped = FALSE;
svn_error_t *err = SVN_NO_ERROR;
svn_revnum_t result_rev;
+ const char *local_abspath;
path = APR_ARRAY_IDX(paths, i, const char *);
svn_pool_clear(subpool);
if (ctx->cancel_func && (err = ctx->cancel_func(ctx->cancel_baton)))
break;
-
- if (svn_path_is_url(path))
+
+ SVN_ERR(svn_dirent_get_absolute(&local_abspath, path, subpool));
+ err = svn_client__update_internal(&result_rev, local_abspath,
+ revision, depth, depth_is_sticky,
+ ignore_externals,
+ allow_unver_obstructions,
+ &sleep, FALSE, make_parents,
+ ctx, subpool);
+
+ if (err && err->apr_err != SVN_ERR_WC_NOT_WORKING_COPY)
{
- skipped = TRUE;
+ return svn_error_return(err);
}
- else
+
+ if (err)
{
- const char *local_abspath;
-
- SVN_ERR(svn_dirent_get_absolute(&local_abspath, path, subpool));
- err = svn_client__update_internal(&result_rev, local_abspath,
- revision, depth, depth_is_sticky,
- ignore_externals,
- allow_unver_obstructions,
- &sleep, FALSE, make_parents,
- ctx, subpool);
-
- if (err && err->apr_err != SVN_ERR_WC_NOT_WORKING_COPY)
- {
- return svn_error_return(err);
- }
-
- if (err)
- {
- /* SVN_ERR_WC_NOT_WORKING_COPY: it's not versioned */
- svn_error_clear(err);
- skipped = TRUE;
- }
- }
-
- if (skipped)
- {
+ /* SVN_ERR_WC_NOT_WORKING_COPY: it's not versioned */
+ svn_error_clear(err);
result_rev = SVN_INVALID_REVNUM;
if (ctx->notify_func2)
{
svn_wc_notify_t *notify;
-
- if (svn_path_is_url(path))
- {
- /* For some historic reason this user error is supported,
- and must provide correct notifications. */
- notify = svn_wc_create_notify_url(path,
- svn_wc_notify_skip,
- subpool);
- }
- else
- {
- notify = svn_wc_create_notify(path,
- svn_wc_notify_skip,
- subpool);
- }
-
+ notify = svn_wc_create_notify(path,
+ svn_wc_notify_skip,
+ subpool);
(*ctx->notify_func2)(ctx->notify_baton2, notify, subpool);
}
}
Received on 2010-12-08 08:06:52 CET