Thanks for the patch! Agree about the problems with the way main.c is
currently set up to handle implicit ".". I think Brian Fitzpatrick is
making changes related to this issue, among others, so will defer
handling of this patch to him. (Is that okay, Fitz?)
-K
Mo DeJong <mdejong@cygnus.com> writes:
> Hi all.
>
> In looking at the way the command line client parses
> arguments passed in from the user, I found a block of
> code in main.c that seems rather error prone:
>
> if ((targets->nelts == 0)
> && ( (subcommand->cmd_code == svn_cl__commit_command)
> || (subcommand->cmd_code == svn_cl__proplist_command)
> || (subcommand->cmd_code == svn_cl__propget_command)
> || (subcommand->cmd_code == svn_cl__status_command)
> || (subcommand->cmd_code == svn_cl__update_command)))
> {
> (*((svn_string_t **) apr_array_push (targets)))
> = svn_string_create (".", pool);
> }
>
> It is adding a "." argument if the command type if the
> command type is one of the commands that takes an
> implicit dot. This seems to be a bad idea since the
> main() function needs to know about every possible
> command and it is easy to miss one. For example,
> the diff command is not included in this list.
> It really should be, `svn diff` should really
> work the same as `cvs diff`. It seems like a
> much better solution would be to do this
> implicit dot check inside the functions that
> actually need to do it. Here is my patch to
> implement this change. Note that I did not
> reformat spaces and added a FIXME not for the
> for loops that were in an if block before. This
> is so that the patch is more clear, I can
> send another patch to fix the spacing if/when
> this one gets added.
>
> Mo DeJong
> Red Hat Inc
>
>
>
> 2001-03-05 Mo DeJong <mdejong@redhat.com>
>
> * subversion/client/cl.h (push_implicit_dot_target):
> Add header for new method defined in main.c.
> * subversion/client/commit-cmd.c (svn_cl__commit):
> Call push_implicit_dot_target.
> * subversion/client/diff-cmd.c (svn_cl__diff):
> Call push_implicit_dot_target.
> * subversion/client/main.c: #include <assert.h>,
> define push_implicit_dot_target. Remove error
> prone block of code to push implicit dot target
> by comparing to known commands.
> * subversion/client/propget-cmd.c (svn_cl__propget):
> * subversion/client/proplist-cmd.c (svn_cl__proplist):
> * subversion/client/status-cmd.c (svn_cl__status):
> * subversion/client/update-cmd.c (svn_cl__update):
> Call push_implicit_dot_target.
>
>
>
> Index: subversion/client/cl.h
> ===================================================================
> RCS file: /cvs/subversion/subversion/client/cl.h,v
> retrieving revision 1.34
> diff -u -r1.34 cl.h
> --- subversion/client/cl.h 2001/03/04 18:36:00 1.34
> +++ subversion/client/cl.h 2001/03/06 05:55:53
> @@ -172,6 +172,8 @@
> const svn_cl__cmd_desc_t *
> svn_cl__get_canonical_command (const char *cmd);
>
> +void push_implicit_dot_target(apr_array_header_t *targets, apr_pool_t *pool);
> +
>
>
> #endif /* SVN_CL_H */
> Index: subversion/client/commit-cmd.c
> ===================================================================
> RCS file: /cvs/subversion/subversion/client/commit-cmd.c,v
> retrieving revision 1.11
> diff -u -r1.11 commit-cmd.c
> --- subversion/client/commit-cmd.c 2001/02/06 00:45:51 1.11
> +++ subversion/client/commit-cmd.c 2001/03/06 05:55:53
> @@ -37,7 +37,11 @@
> svn_error_t *err;
> int i;
>
> - if (targets->nelts)
> + /* Add "." if user passed 0 arguments */
> + push_implicit_dot_target(targets, pool);
> +
> + /* FIXME: reformat block to remove extra spaces */
> +
> for (i = 0; i < targets->nelts; i++)
> {
> svn_string_t *target = ((svn_string_t **) (targets->elts))[i];
> @@ -61,14 +65,6 @@
> if (err)
> return err;
> }
> - else
> - {
> - fprintf (stderr, "svn commit: arguments required\n");
> - err = svn_cl__help (opt_state, targets, pool);
> - if (err)
> - return err;
> - }
> -
>
> return SVN_NO_ERROR;
> }
> Index: subversion/client/diff-cmd.c
> ===================================================================
> RCS file: /cvs/subversion/subversion/client/diff-cmd.c,v
> retrieving revision 1.1
> diff -u -r1.1 diff-cmd.c
> --- subversion/client/diff-cmd.c 2001/01/26 14:21:06 1.1
> +++ subversion/client/diff-cmd.c 2001/03/06 05:55:53
> @@ -37,7 +37,11 @@
> svn_error_t *err;
> int i;
>
> - if (targets->nelts)
> + /* Add "." if user passed 0 arguments */
> + push_implicit_dot_target(targets, pool);
> +
> + /* FIXME: reformat block to remove extra spaces */
> +
> for (i = 0; i < targets->nelts; i++)
> {
> svn_string_t *target = ((svn_string_t **) (targets->elts))[i];
> @@ -45,14 +49,6 @@
> err = svn_cl__print_file_diff (target, pool);
> if (err) return err;
> }
> - else
> - {
> - fprintf (stderr, "svn diff: arguments required\n");
> - err = svn_cl__help (opt_state, targets, pool);
> - if (err)
> - return err;
> - }
> -
>
> return SVN_NO_ERROR;
> }
> Index: subversion/client/main.c
> ===================================================================
> RCS file: /cvs/subversion/subversion/client/main.c,v
> retrieving revision 1.62
> diff -u -r1.62 main.c
> --- subversion/client/main.c 2001/03/04 18:23:38 1.62
> +++ subversion/client/main.c 2001/03/06 05:55:54
> @@ -19,6 +19,7 @@
> /*** Includes. ***/
>
> #include <string.h>
> +#include <assert.h>
>
> #include <apr_strings.h>
> #include <apr_getopt.h>
> @@ -121,6 +122,18 @@
> };
>
>
> +/* Some commands take an implicit "." string argument when invoked
> + * with no arguments. Those commands make use of this function to
> + * add "." to the target array if the user passes no args */
> +void push_implicit_dot_target(apr_array_header_t *targets, apr_pool_t *pool)
> +{
> + if (targets->nelts == 0) {
> + (*((svn_string_t **) apr_array_push (targets)))
> + = svn_string_create (".", pool);
> + }
> + assert(targets->nelts);
> +}
> +
> /* Return the entry in svn_cl__cmd_table whose name matches CMD_NAME,
> * or null if none. CMD_NAME may be an alias, in which case the alias
> * entry will be returned (so caller may need to canonicalize result). */
> @@ -396,25 +409,10 @@
> const char *this_arg = os->argv[os->ind];
> (*((svn_string_t **) apr_array_push (targets)))
> = svn_string_create (this_arg, pool);
> - }
> -
> - /* Certain commands have an implied `.' as argument, if nothing else
> - is specified. */
> - if ((targets->nelts == 0)
> - && ( (subcommand->cmd_code == svn_cl__commit_command)
> - || (subcommand->cmd_code == svn_cl__proplist_command)
> - || (subcommand->cmd_code == svn_cl__propget_command)
> - || (subcommand->cmd_code == svn_cl__status_command)
> - || (subcommand->cmd_code == svn_cl__update_command)))
> - {
> - (*((svn_string_t **) apr_array_push (targets)))
> - = svn_string_create (".", pool);
> - }
> - else
> - {
> - /* kff todo: need to remove redundancies from targets before
> - passing it to the cmd_func. */
> }
> +
> + /* kff todo: need to remove redundancies from targets before
> + passing it to the cmd_func. */
>
> /* Run the subcommand. */
> err = (*subcommand->cmd_func) (&opt_state, targets, pool);
> Index: subversion/client/propget-cmd.c
> ===================================================================
> RCS file: /cvs/subversion/subversion/client/propget-cmd.c,v
> retrieving revision 1.8
> diff -u -r1.8 propget-cmd.c
> --- subversion/client/propget-cmd.c 2001/03/04 18:07:45 1.8
> +++ subversion/client/propget-cmd.c 2001/03/06 05:55:54
> @@ -39,7 +39,11 @@
> svn_error_t *err;
> int i;
>
> - if (targets->nelts)
> + /* Add "." if user passed 0 file arguments */
> + push_implicit_dot_target(targets, pool);
> +
> + /* FIXME: reformat block to remove extra spaces */
> +
> for (i = 0; i < targets->nelts; i++)
> {
> svn_string_t *value;
> @@ -54,13 +58,6 @@
> value);
> svn_cl__print_prop_hash (prop_hash, pool);
> }
> - else
> - {
> - fprintf (stderr, "svn propget: arguments required\n");
> - err = svn_cl__help (opt_state, targets, pool);
> - if (err)
> - return err;
> - }
>
> return SVN_NO_ERROR;
> }
> Index: subversion/client/proplist-cmd.c
> ===================================================================
> RCS file: /cvs/subversion/subversion/client/proplist-cmd.c,v
> retrieving revision 1.9
> diff -u -r1.9 proplist-cmd.c
> --- subversion/client/proplist-cmd.c 2001/02/08 12:32:41 1.9
> +++ subversion/client/proplist-cmd.c 2001/03/06 05:55:54
> @@ -37,7 +37,11 @@
> svn_error_t *err;
> int i;
>
> - if (targets->nelts)
> + /* Add "." if user passed 0 arguments */
> + push_implicit_dot_target(targets, pool);
> +
> + /* FIXME: reformat block to remove extra spaces */
> +
> for (i = 0; i < targets->nelts; i++)
> {
> svn_string_t *target = ((svn_string_t **) (targets->elts))[i];
> @@ -49,13 +53,6 @@
>
> svn_cl__print_prop_hash (prop_hash, pool);
> }
> - else
> - {
> - fprintf (stderr, "svn proplist: arguments required\n");
> - err = svn_cl__help (opt_state, targets, pool);
> - if (err)
> - return err;
> - }
>
> return SVN_NO_ERROR;
> }
> Index: subversion/client/status-cmd.c
> ===================================================================
> RCS file: /cvs/subversion/subversion/client/status-cmd.c,v
> retrieving revision 1.9
> diff -u -r1.9 status-cmd.c
> --- subversion/client/status-cmd.c 2001/01/08 18:55:39 1.9
> +++ subversion/client/status-cmd.c 2001/03/06 05:55:54
> @@ -38,7 +38,11 @@
> apr_hash_t *statushash;
> int i;
>
> - if (targets->nelts)
> + /* Add "." if user passed 0 arguments */
> + push_implicit_dot_target(targets, pool);
> +
> + /* FIXME: reformat block to remove extra spaces */
> +
> for (i = 0; i < targets->nelts; i++)
> {
> svn_string_t *target = ((svn_string_t **) (targets->elts))[i];
> @@ -54,14 +58,6 @@
>
> svn_cl__print_status_list (statushash, pool);
> }
> - else
> - {
> - fprintf (stderr, "svn status: arguments required\n");
> - err = svn_cl__help (opt_state, targets, pool);
> - if (err)
> - return err;
> - }
> -
>
> return SVN_NO_ERROR;
> }
> Index: subversion/client/update-cmd.c
> ===================================================================
> RCS file: /cvs/subversion/subversion/client/update-cmd.c,v
> retrieving revision 1.11
> diff -u -r1.11 update-cmd.c
> --- subversion/client/update-cmd.c 2001/02/06 00:45:51 1.11
> +++ subversion/client/update-cmd.c 2001/03/06 05:55:54
> @@ -37,7 +37,11 @@
> svn_error_t *err;
> int i;
>
> - if (targets->nelts)
> + /* Add "." if user passed 0 arguments */
> + push_implicit_dot_target(targets, pool);
> +
> + /* FIXME: reformat block to remove extra spaces */
> +
> for (i = 0; i < targets->nelts; i++)
> {
> svn_string_t *target = ((svn_string_t **) (targets->elts))[i];
> @@ -59,13 +63,6 @@
> if (err)
> return err;
> }
> - else
> - {
> - fprintf (stderr, "svn update: arguments required\n");
> - err = svn_cl__help (opt_state, targets, pool);
> - if (err)
> - return err;
> - }
>
> return SVN_NO_ERROR;
> }
Received on Sat Oct 21 14:36:25 2006