subversion-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Noorul Islam K M <noo...@collab.net>
Subject Re: Input validation observations
Date Sat, 04 Dec 2010 07:27:18 GMT
Julian Foad <julian.foad@wandisco.com> writes:

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

Please find attached the updated patch.

Thanks and Regards
Noorul


Mime
View raw message