Julian Foad <julian.f...@wandisco.com> writes:
> On Fri, 2010-12-03, Noorul Islam K M wrote:
>
>> Julian Foad <julian.f...@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.
> [...]
>> Index: subversion/libsvn_client/update.c
>> ===================================================================
>> --- subversion/libsvn_client/update.c (revision 1041293)
>> +++ subversion/libsvn_client/update.c (working copy)
>> @@ -397,44 +397,45 @@
>>
>> 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;
>
> Hi Noorul.
>
> Having removed this "skipped = TRUE" statement, the only remaining use
> of the "skipped" variable is ...
>
>> + 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;
>> - }
>> + /* SVN_ERR_WC_NOT_WORKING_COPY: it's not versioned */
>> + svn_error_clear(err);
>> + skipped = TRUE;
>
> ... here ...
>
>> }
>>
>> if (skipped)
>
> ... and here. So you can eliminate the variable and join the two blocks
> together. That would be simpler.
>
> The patch looks functionally correct.
>
> Thanks.
>
> - Julian
>
>
>> @@ -443,22 +444,9 @@
>> 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);
>> }
>> }
Please find attached the updated patch.
Thanks and Regards
Noorul
Index: subversion/tests/cmdline/input_validation_tests.py
===================================================================
--- subversion/tests/cmdline/input_validation_tests.py (revision 1041944)
+++ 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/svn/update-cmd.c
===================================================================
--- subversion/svn/update-cmd.c (revision 1041944)
+++ 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 1041944)
+++ 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);
}
}
