Re: svn commit: r29127 - in trunk/subversion: bindings/swig include libsvn_subr libsvn_wc svn tests/cmdline

On Jan 31, 2008 8:15 PM, <kfogel_at_tigris.org> wrote:
> Author: kfogel
> Date: Thu Jan 31 20:15:40 2008
> New Revision: 29127
>
> Log:
> Fix issue #3026: _svn and .svn are silently ignored as file arguments.
>
> Patch by: Augie Fackler: <durin42_at_gmail.com>
> (Test by me.)
> Modified: trunk/subversion/include/svn_error_codes.h
> URL: http://svn.collab.net/viewvc/svn/trunk/subversion/include/svn_error_codes.h?pathrev=29127&r1=29126&r2=29127
> ==============================================================================
> --- trunk/subversion/include/svn_error_codes.h (original)
> +++ trunk/subversion/include/svn_error_codes.h Thu Jan 31 20:15:40 2008
> @@ -1124,6 +1124,10 @@
> SVN_ERRDEF(SVN_ERR_UNKNOWN_CHANGELIST,
> SVN_ERR_MISC_CATEGORY_START + 24,
> "Unknown changelist")
> +
> + SVN_ERRDEF(SVN_ERR_RESERVED_FILENAME_SPECIFIED,
> + SVN_ERR_MISC_CATEGORY_START + 25,
> + "Reserved directory name in command line arguments")

Indentation is weird.

>
> /* command-line client errors */
>
>
> Modified: trunk/subversion/include/svn_opt.h
> URL: http://svn.collab.net/viewvc/svn/trunk/subversion/include/svn_opt.h?pathrev=29127&r1=29126&r2=29127
> ==============================================================================
> --- trunk/subversion/include/svn_opt.h (original)
> +++ trunk/subversion/include/svn_opt.h Thu Jan 31 20:15:40 2008
> @@ -490,6 +490,18 @@
> apr_array_header_t *known_targets,
> apr_pool_t *pool);
>
> +/**
> + * This is the same as svn_opt_args_to_target_array2() except that it returns
> + * errors when a path has the same name as a working copy directory.
> + *
> + * @since New in 1.5.
> + */

We should always document the newest version, and change the old
version to say how it differs. I also find this to be confusing: I'd
say "a working copy administrative directory". Also, it should be
explicit about the fact that while it returns an error (and probably
should say which one), it still actually completes the operation.
(Please do not backport this revision without fixing this, and the
related issue mentioned below!)

> +svn_error_t *
> +svn_opt_args_to_target_array3(apr_array_header_t **targets_p,
> + apr_getopt_t *os,
> + apr_array_header_t *known_targets,
> + apr_pool_t *pool);
> +
>
> /**
> * The same as svn_opt_args_to_target_array2() except that, in
>
> Modified: trunk/subversion/libsvn_subr/opt.c
> URL: http://svn.collab.net/viewvc/svn/trunk/subversion/libsvn_subr/opt.c?pathrev=29127&r1=29126&r2=29127
> ==============================================================================
> --- trunk/subversion/libsvn_subr/opt.c (original)
> +++ trunk/subversion/libsvn_subr/opt.c Thu Jan 31 20:15:40 2008
> @@ -872,7 +872,30 @@
> apr_array_header_t *known_targets,
> apr_pool_t *pool)
> {
> + svn_error_t *error = svn_opt_args_to_target_array3(targets_p,
> + os,
> + known_targets,
> + pool);
> + if (error)
> + {
> + if (error->apr_err == SVN_ERR_RESERVED_FILENAME_SPECIFIED)
> + svn_error_clear(error);
> + else
> + return error;
> + }
> + return SVN_NO_ERROR;
> +
> +}
> +
> +
> +svn_error_t *
> +svn_opt_args_to_target_array3(apr_array_header_t **targets_p,
> + apr_getopt_t *os,
> + apr_array_header_t *known_targets,
> + apr_pool_t *pool)
> +{
> int i;
> + svn_error_t *error = SVN_NO_ERROR;
> apr_array_header_t *input_targets =
> apr_array_make(pool, DEFAULT_ARRAY_SIZE, sizeof(const char *));
> apr_array_header_t *output_targets =
> @@ -1010,7 +1033,14 @@
> synchronized! */
> if (0 == strcmp(base_name, ".svn")
> || 0 == strcmp(base_name, "_svn"))
> - continue;
> + {
> + char *error_str = apr_psprintf(pool, _("'%s' ends in a "
> + "reserved name"), target);
> + error = svn_error_create(SVN_ERR_RESERVED_FILENAME_SPECIFIED,
> + error,
> + error_str);
> + continue;

Hmm, why not use svn_error_createf?

> + }
> }
>
> /* Append the peg revision back to the canonicalized target if
> @@ -1026,6 +1056,8 @@
> passing it to the cmd_func. */
>
> *targets_p = output_targets;
> + if (error)
> + return error;
> return SVN_NO_ERROR;
> }
>
>

> Modified: trunk/subversion/svn/util.c
> URL: http://svn.collab.net/viewvc/svn/trunk/subversion/svn/util.c?pathrev=29127&r1=29126&r2=29127
> ==============================================================================
> --- trunk/subversion/svn/util.c (original)
> +++ trunk/subversion/svn/util.c Thu Jan 31 20:15:40 2008
> @@ -1010,6 +1010,28 @@
> }
>
>
> +svn_error_t *
> +svn_cl__args_to_target_array_print_reserved(apr_array_header_t **targets,
> + apr_getopt_t *os,
> + apr_array_header_t *known_targets,
> + apr_pool_t *pool)
> +{
> + svn_error_t *error = svn_opt_args_to_target_array3(targets, os,
> + known_targets, pool);
> + if (error)
> + {
> + if (error->apr_err == SVN_ERR_RESERVED_FILENAME_SPECIFIED)
> + {
> + svn_handle_error2(error, stderr, FALSE, "svn: Skipping argument: ");
> + svn_error_clear(error);

This is making the interesting assumption that any chain containing
this sort of error *only* contains this sort of error. This
assumption should be documented in the svn_opt_args_to_target_array3 docstring.

> + }
> + else
> + return error;
> + }
> + return SVN_NO_ERROR;
> +}
> +
> +
> /* Helper for svn_cl__get_changelist(); implements
> svn_changelist_receiver_t. */
> static svn_error_t *
>

-- 
David Glasser | glasser@davidglasser.net | http://www.davidglasser.net/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe_at_subversion.tigris.org
For additional commands, e-mail: dev-help_at_subversion.tigris.org