subversion-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Julian Foad <julian.f...@wandisco.com>
Subject Re: Input validation observations
Date Fri, 03 Dec 2010 16:15:51 GMT
On Fri, 2010-12-03, Noorul Islam K M wrote:
> Julian Foad <julian.foad@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);
>              }
>          }



Mime
View raw message