You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Mo DeJong <md...@cygnus.com> on 2001/03/06 06:17:46 UTC
Implicit "." patch for subversion/client.
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 <md...@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;
}
Re: Implicit "." patch for subversion/client.
Posted by Karl Fogel <kf...@galois.collab.net>.
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 <md...@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 <md...@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;
> }