Re: [PATCH] Implement --ignore and associated config option

mark benedetto king <bking@inquira.com> writes:

> Index: subversion/include/svn_wc.h
> ===================================================================
> --- subversion/include/svn_wc.h
> +++ subversion/include/svn_wc.h Mon Aug 12 21:11:46 2002
> @@ -674,6 +674,7 @@
> svn_boolean_t get_all,
> svn_boolean_t strict,
> svn_boolean_t no_ignore,
> + const char *ignore,

This is subversion's public interface, so you need to document the
parameters.

> apr_pool_t *pool);
>
>
> Index: subversion/include/svn_client.h
> ===================================================================
> --- subversion/include/svn_client.h
> +++ subversion/include/svn_client.h Mon Aug 12 21:11:46 2002
> @@ -533,6 +533,7 @@
> svn_boolean_t get_all,
> svn_boolean_t update,
> svn_boolean_t no_ignore,
> + const char *ignore,

Document.

> apr_pool_t *pool);
>
>
> Index: subversion/libsvn_subr/config_file.c
> ===================================================================
> --- subversion/libsvn_subr/config_file.c
> +++ subversion/libsvn_subr/config_file.c Mon Aug 12 22:15:59 2002
> @@ -453,6 +453,53 @@
> return err;
> }
>
> +static svn_error_t *
> +svn_config__write_file (const char *name, apr_pool_t *pool,
> + const char *contents, svn_boolean_t *give_up)

Could do with a brief documentation string. I'm unsure of the
rationale for give_up, why not return an error?

A minor niggle, most subversion functions put pool as the last
argument.

> +{
> + enum svn_node_kind kind;
> + apr_status_t apr_err;
> + svn_error_t *err;
> + const char *path;
> +
> + *give_up = TRUE;
> +
> + SVN_ERR (svn_config__user_config_path
> + (&path, name, pool));
> +
> + if (! path) /* highly unlikely, since a previous call succeeded */
> + return SVN_NO_ERROR;
> +
> + err = svn_io_check_path (path, &kind, pool);
> + if (err)

You need an svn_error_clear_all here.

> + return SVN_NO_ERROR;
> +
> + if (kind == svn_node_none)
> + {
> + apr_file_t *f;
> +
> + apr_err = apr_file_open (&f, path,
> + (APR_WRITE | APR_CREATE | APR_EXCL),
> + APR_OS_DEFAULT,
> + pool);
> +
> + if (! apr_err)
> + {
> + apr_err = apr_file_write_full (f, contents, strlen (contents), NULL);
> + if (apr_err)
> + return svn_error_createf (apr_err, 0, NULL, pool,
> + "writing config file `%s'", path);
> +
> + apr_err = apr_file_close (f);
> + if (apr_err)
> + return svn_error_createf (apr_err, 0, NULL, pool,
> + "closing config file `%s'", path);
> + }
> + }
> +
> + *give_up = FALSE;
> + return SVN_NO_ERROR;
> +}
>
[snip]
> Index: subversion/clients/cmdline/main.c
> ===================================================================
> --- subversion/clients/cmdline/main.c
> +++ subversion/clients/cmdline/main.c Tue Aug 13 13:49:33 2002
> @@ -51,6 +51,7 @@
> {
> {"force", svn_cl__force_opt, 0, "force operation to run"},
> {"help", 'h', 0, "show help on a subcommand"},
> + {"ignore", svn_cl__ignore_opt, 1, "set of wildcards to ignore"},

I think "additional" is better that "set of".

> {NULL, '?', 0, "show help on a subcommand"},
> {"message", 'm', 1, "specify commit message \"ARG\""},
> {"quiet", 'q', 0, "print as little as possible"},
> @@ -380,7 +381,7 @@
> " M 965 687 joe ./buildcheck.sh\n",
> { 'u', 'v', 'N', 'q',
> svn_cl__auth_username_opt, svn_cl__auth_password_opt,
> - svn_cl__no_ignore_opt } },
> + svn_cl__no_ignore_opt, svn_cl__ignore_opt } },
>
> { "switch", svn_cl__switch, {"sw"},
> "Update working copy to mirror a new URL\n"
> @@ -1049,6 +1050,16 @@
> TRUE, pool);
> }
> break;
> + case svn_cl__ignore_opt:
> + err = svn_utf_cstring_to_utf8 (&opt_state.ignore, opt_arg,
> + NULL, pool);
> + if (err)
> + {
> + svn_handle_error (err, stdout, FALSE);
> + svn_pool_destroy (pool);
> + return EXIT_FAILURE;
> + }
> + break;
> case svn_cl__force_opt:
> opt_state.force = TRUE;
> break;

Other than that, it's good and should be committed.

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