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);
> }
> }
