Re: [PATCH] svn cat

Vladimir Prus <ghost@cs.msu.su> writes:

This looks pretty good, a few minor comments.

> Patch:
> Index: subversion/include/svn_error_codes.h
> ===================================================================
> --- subversion/include/svn_error_codes.h (revision 3772)
> +++ subversion/include/svn_error_codes.h (working copy)
> @@ -50,16 +50,16 @@
> #if defined(SVN_ERROR_BUILD_ARRAY)
>
> #define SVN_ERROR_START \
> - static const err_defn error_table[] = { \
> - { SVN_WARNING, "Warning" },
> + static const err_defn error_table[] = { \
> + { SVN_WARNING, "Warning" },

Please don't make unnecessary white space changes.

> #define SVN_ERRDEF(num, offset, str) { num, str },
> #define SVN_ERROR_END { 0, NULL } };
>
> #elif !defined(SVN_ERROR_ENUM_DEFINED)
>
> #define SVN_ERROR_START \
> - typedef enum svn_errno_t { \
> - SVN_WARNING = APR_OS_START_USERERR + 1,
> + typedef enum svn_errno_t { \
> + SVN_WARNING = APR_OS_START_USERERR + 1,
> #define SVN_ERRDEF(num, offset, str) num = offset,
> #define SVN_ERROR_END SVN_ERR_LAST } svn_errno_t;
>
> @@ -600,6 +600,11 @@
> SVN_ERRDEF (SVN_ERR_CLIENT_MODIFIED,
> SVN_ERR_CLIENT_CATEGORY_START + 6,
> "Attempting restricted operation for modified resource")
> +
> + SVN_ERRDEF (SVN_ERR_CLIENT_CAT_OF_DIRECTORY,
> + SVN_ERR_CLIENT_CATEGORY_START + 7,
> + "Invoked 'cat' operation on a directory")

Do we need such a specific error? The error message you have chosen
probably won't make sense for a GUI client. Perhaps a more general
SVN_ERR_CLIENT_IS_DIRECTORY or just reword the message.

> /* See definition in main.c for documentation. */
> Index: subversion/clients/cmdline/cat-cmd.c
> ===================================================================
> --- subversion/clients/cmdline/cat-cmd.c (working copy)
> +++ subversion/clients/cmdline/cat-cmd.c (working copy)
> @@ -0,0 +1,74 @@
> +/*
> + * cat-cmd.c -- Print the content of a file or URL.
> + *
> + * ====================================================================
> + * Copyright (c) 2000-2002 CollabNet. All rights reserved.
> + *
> + * This software is licensed as described in the file COPYING, which
> + * you should have received as part of this distribution. The terms
> + * are also available at http://subversion.tigris.org/license-1.html.
> + * If newer versions of this license are posted there, you may use a
> + * newer version instead, at your option.
> + *
> + * This software consists of voluntary contributions made by many
> + * individuals. For exact contribution history, see the revision
> + * history and logs, available at http://subversion.tigris.org/.
> + * ====================================================================
> + */
> +
> +/* ==================================================================== */
> +
> +
> +
> +/*** Includes. ***/
> +
> +#include "svn_wc.h"
> +#include "svn_client.h"
> +#include "svn_string.h"
> +#include "svn_path.h"
> +#include "svn_delta.h"
> +#include "svn_error.h"
> +#include "cl.h"
> +
> +
> +/*** Code. ***/
> +
> +/* This implements the `svn_opt_subcommand_t' interface. */
> +svn_error_t *
> +svn_cl__cat (apr_getopt_t *os,
> + void *baton,
> + apr_pool_t *pool)
> +{
> + svn_cl__opt_state_t *opt_state = baton;
> + apr_array_header_t *targets;
> + int i;
> + svn_client_auth_baton_t *auth_baton;
> +
> + SVN_ERR (svn_opt_args_to_target_array (&targets, os,
> + opt_state->targets,
> + &(opt_state->start_revision),
> + &(opt_state->end_revision),
> + FALSE, pool));
> +
> + /* Build an authentication baton to give to libsvn_client. */
> + auth_baton = svn_cl__make_auth_baton (opt_state, pool);

We probably ought to do something if the user specifes two revisions
with -rX:Y. Either generate a warning/error, or perhaps print all the
revisions?

> + for (i = 0; i < targets->nelts; i++)
> + {
> + const char *target = ((const char **) (targets->elts))[i];
> + svn_stream_t *out = svn_stream_from_stdio (stdout, pool);
> + const char *URL;
> +
> + SVN_ERR (svn_cl__get_url_from_target (&URL, target, pool));
> + if (! URL)
> + return svn_error_createf (SVN_ERR_ENTRY_MISSING_URL, 0, NULL,
> + "'%s' has no URL", target);
> +
> + SVN_ERR (svn_client_cat
> + (out,
> + URL, &(opt_state->start_revision),
> + auth_baton, pool));
> + }
> +
> + return SVN_NO_ERROR;
> +}
> Index: subversion/clients/cmdline/main.c
> ===================================================================
> --- subversion/clients/cmdline/main.c (revision 3772)
> +++ subversion/clients/cmdline/main.c (working copy)
> @@ -117,6 +117,13 @@
> "usage: svn add TARGETS\n",
> {svn_cl__targets_opt, 'R', 'q'} },
>
> + { "cat", svn_cl__cat, {0},
> + "Output the content of specified files or URLS.\n"
> + "usage: svn add TARGETS\n\n"

Oops :)

> + "When working copy file is specified, the repository version of\n"
> + "that file is printed\n",
> + {'r'} },
> +
> { "checkout", svn_cl__checkout, {"co"},
> "Check out a working copy from a repository.\n"
> "usage: svn checkout REPOS_URL [REPOS_URL...] [DESTINATION]\n"
> @@ -626,8 +633,8 @@
> }
> break;
> case svn_cl__targets_opt:
> - {
> - svn_stringbuf_t *buffer, *buffer_utf8;
> + {
> + svn_stringbuf_t *buffer, *buffer_utf8;

More whitespace changes.

-- 
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org