Re: [PATCH] automatic properties

Martin Furter <mf@rola.ch> writes:

> Index: subversion/libsvn_client/client.h
> ===================================================================
> --- subversion/libsvn_client/client.h (revision 7208)
> +++ subversion/libsvn_client/client.h (working copy)
> @@ -237,6 +237,38 @@
> 
> /*** Add/delete ***/
>
> +/* Method which automatically adds configured properties.
> +
> + path, ctx and pool must always be valid. When adding adm_access must
> + be valid and editor must be NULL, when importing editor, file_baton,
> + mimetype and executable must be valid.

This sounds a little bit like two separate functions, not sure though.

> +
> + First the function checks that auto-props is enabled for add or import.
> + Then i enumerates the 'auto-props' section and tries to find fnmatches.

"i" should be "it".

> + All matches found are parsed and the properties added to given path.
> +
> + The format of the value is [propname[=value][;name[=val]]...]
> +
> + @param path a filename
> + @param ctx the client context
> + @param pool apr memory pool
> + @param adm_access access baton, used when adding
> + @param editor editor, used when importing, must be NULL when adding
> + @param file_baton file baton for editor, used when importing
> + @param mimetype pointer for returning mimetype, used when importing
> + @param executable pointer for returning executable, used when importing
> + @return an svn_error_t or SVN_NO_ERROR when everything went OK
> +*/

The format of this comment is different from that of the others in the
file. The others don't generally end with a list of parameters. The
others use "@a name" when refering to parameters in the body of the
comment.

> +svn_error_t *
> +svn_client__add_auto_props (const char *path,
> + svn_client_ctx_t *ctx,
> + apr_pool_t *pool,
> + svn_wc_adm_access_t *adm_access,
> + const svn_delta_editor_t *editor,
> + void *file_baton,
> + const char **mimetype,
> + int *executable);
> +
> /* The main logic for client deletion from a working copy. Deletes PATH
> from ADM_ACCESS. If PATH (or any item below a directory PATH) is
> modified the delete will fail and return an error unless FORCE is TRUE.
> Index: subversion/libsvn_client/commit.c
> ===================================================================
> --- subversion/libsvn_client/commit.c (revision 7208)
> +++ subversion/libsvn_client/commit.c (working copy)
> @@ -105,8 +105,8 @@
> apr_pool_t *pool)
> {
> void *file_baton;
> - const char *mimetype;
> - svn_boolean_t executable;
> + const char *mimetype = NULL;
> + svn_boolean_t executable = FALSE;

Yikes! Is this initialisation necessary? I cannot tell from the
documentation of svn_client__add_auto_props. I'd happier if it wasn't
necessary.

> unsigned char digest[MD5_DIGESTSIZE];
> const char *text_checksum;
>
> @@ -114,21 +114,27 @@
> SVN_ERR (editor->add_file (edit_path, dir_baton, NULL, SVN_INVALID_REVNUM,
> pool, &file_baton));
>
> + /* add automatic properties */
> + SVN_ERR (svn_client__add_auto_props (path, ctx, pool, NULL, editor,
> + file_baton, &mimetype, &executable));
> +
> /* If the file has a discernable mimetype, add that as a property to
> the file. */
> - SVN_ERR (svn_io_detect_mimetype (&mimetype, path, pool));
> + if (!mimetype)
> + SVN_ERR (svn_io_detect_mimetype (&mimetype, path, pool));
> if (mimetype)
> SVN_ERR (editor->change_file_prop (file_baton, SVN_PROP_MIME_TYPE,
> svn_string_create (mimetype, pool),
> pool));
>
> /* If the file is executable, add that as a property to the file. */
> - SVN_ERR (svn_io_is_file_executable (&executable, path, pool));
> + if (!executable)
> + SVN_ERR (svn_io_is_file_executable (&executable, path, pool));
> if (executable)
> SVN_ERR (editor->change_file_prop (file_baton, SVN_PROP_EXECUTABLE,
> svn_string_create ("", pool),
> pool));
> -
> +
> if (ctx->notify_func)
> (*ctx->notify_func) (ctx->notify_baton,
> path,
> Index: subversion/libsvn_client/add.c
> ===================================================================
> --- subversion/libsvn_client/add.c (revision 7208)
> +++ subversion/libsvn_client/add.c (working copy)
> @@ -23,6 +23,7 @@
> /*** Includes. ***/
>
> #include <string.h>
> +#include <apr_fnmatch.h>
> #include "svn_wc.h"
> #include "svn_client.h"
> #include "svn_string.h"
> @@ -30,12 +31,151 @@
> #include "svn_error.h"
> #include "svn_path.h"
> #include "svn_io.h"
> +#include "svn_config.h"
> #include "client.h"
>
>
> 
> /*** Code. ***/
>
> +typedef struct {
> + char *filename;
> + apr_pool_t *pool;
> + apr_hash_t *properties;

Some documentation would be good. What goes in this hash?

> +} auto_props_baton_t;
> +
> +/**
> + * enumerate through auto-props entries and store value if name matches
> + *
> + * @param name a property name
> + * @param value value of the property
> + * @param baton a baton of type auto_props_baton_t
> + * @return FALSE to stop the enumeration, TRUE to continue
> + */

Is this a doxygen comment? We don't usually do that for "internal"
functions.

> +static svn_boolean_t
> +auto_props_enumerator (const char *name,
> + const char *value,
> + void *baton)
> +{
> + auto_props_baton_t *autoprops = (auto_props_baton_t*)baton;

The cast is not needed.

> + char *property;
> + char *last_token;
> + int len;
> +
> + /* we need a baton, a name and a value */
> + if (!baton)
> + return FALSE;
> + if (!name || strlen (name) == 0)
> + return TRUE;
> + if (!value || strlen (value) == 0)
> + return TRUE;

Possibly a little paranoid :)

Although if we get passed NULL parameters doesn't that indicate that
something has gone wrong? Should we really carry on? Perhaps asserts
would be better.

> +
> + /* check if filename matches and return if it doesn't */
> + if (apr_fnmatch (name, autoprops->filename, 0) == APR_FNM_NOMATCH)
> + return TRUE;
> +
> + /* parse the value */
> + property = apr_pstrdup (autoprops->pool, value);
> + property = apr_strtok (property, ";", &last_token);
> + while (property)
> + {
> + char *value;
> +
> + value = strchr (property, '=');
> + if (value)
> + {
> + *value = 0;
> + value++;
> + apr_collapse_spaces (value, value);
> + }
> + else
> + value = "";
> + apr_collapse_spaces (property, property);
> + len = strlen (property);
> + if (len > 0)
> + {
> + property = apr_pstrdup (autoprops->pool, property);
> + value = apr_pstrdup (autoprops->pool, value);

Is it really necessary to dup these again?

> + apr_hash_set (autoprops->properties, property, len+1, value );
> + }
> + property = apr_strtok (NULL, ";", &last_token);
> + }
> + return TRUE;
> +}
> +
> +/* adds automatic properties, for a description look into client.h */
> +svn_error_t *
> +svn_client__add_auto_props (const char *path,
> + svn_client_ctx_t *ctx,
> + apr_pool_t *pool,
> + svn_wc_adm_access_t *adm_access,
> + const svn_delta_editor_t *editor,
> + void *file_baton,
> + const char **mimetype,
> + int *executable)
> +{
> + svn_config_t *cfg = NULL;

Is this initialisation needed?

> + auto_props_baton_t autoprops;
> + const char *cfgvalue = 0;

ditto.

> + apr_hash_index_t *hi;
> + svn_error_t *err;
> +
> + /* check that we have config */
> + if (!ctx->config)
> + return SVN_NO_ERROR;
> + cfg = apr_hash_get (ctx->config, SVN_CONFIG_CATEGORY_CONFIG,
> + APR_HASH_KEY_STRING);
> + if (!cfg)
> + return SVN_NO_ERROR;
> +
> + /* check that auto props is enabled */
> + if (editor)
> + svn_config_get (cfg, &cfgvalue, SVN_CONFIG_SECTION_MISCELLANY,
> + SVN_CONFIG_OPTION_ENABLE_AUTO_PROPS_IMP, "false");
> + else
> + svn_config_get (cfg, &cfgvalue, SVN_CONFIG_SECTION_MISCELLANY,
> + SVN_CONFIG_OPTION_ENABLE_AUTO_PROPS_ADD, "false");
> + if (!cfgvalue || strcmp(cfgvalue, "true"))
> + return SVN_NO_ERROR;
> +
> + /* search for auto props */
> + autoprops.filename = svn_path_basename (path, pool);
> + autoprops.pool = pool;
> + autoprops.properties = apr_hash_make (pool);
> + svn_config_enumerate (cfg, SVN_CONFIG_SECTION_AUTO_PROPS,
> + auto_props_enumerator, &autoprops);
> +
> + /* loop through the hashtable and add the properties */
> + for (hi = apr_hash_first (pool, autoprops.properties); hi;
> + hi = apr_hash_next (hi))
> + {
> + char *propname;
> + svn_string_t propvalue;
> +
> + apr_hash_this (hi, (const void**)&propname, 0, (void**)&propvalue.data);

Look at other uses of apr_hash_this, we don't use casts, or "0".

> + propvalue.len = strlen (propvalue.data);
> + if (editor)
> + {
> + /* SVN_PROP_MIME_TYPE and SVN_PROP_EXECUTABLE
> + * are returned to the caller when importing */
> + if (!strcmp(propname, SVN_PROP_MIME_TYPE))
> + *mimetype = propvalue.data;
> + else if (!strcmp (propname, SVN_PROP_EXECUTABLE))
> + *executable = TRUE;
> + else
> + err = editor->change_file_prop (file_baton, propname,
> + &propvalue, pool);
> + }
> + else
> + /* we're adding, so just set the property */
> + err = svn_wc_prop_set (propname, &propvalue, path,
> + adm_access, pool);
> + }
> +
> + return SVN_NO_ERROR;

Whoops! Looks like err should be returned.

> +}
> +
> +
> static svn_error_t *
> add_dir_recursive (const char *dirname,
> svn_wc_adm_access_t *adm_access,
> @@ -97,9 +237,13 @@
> SVN_ERR (add_dir_recursive (fullpath, dir_access, ctx, subpool));
>
> else if (this_entry.filetype == APR_REG)
> - SVN_ERR (svn_wc_add (fullpath, dir_access, NULL, SVN_INVALID_REVNUM,
> - ctx->cancel_func, ctx->cancel_baton,
> - ctx->notify_func, ctx->notify_baton, subpool));
> + {
> + SVN_ERR (svn_wc_add (fullpath, dir_access, NULL, SVN_INVALID_REVNUM,
> + ctx->cancel_func, ctx->cancel_baton,
> + ctx->notify_func, ctx->notify_baton, subpool));
> + SVN_ERR (svn_client__add_auto_props (fullpath, ctx, subpool,
> + adm_access, NULL, NULL, NULL, NULL));
> + }

Are we going to add auto-properties on directories as well? Users are
going to want to set svn:ignore. Even though the current patch does
not do it, it is important that will be possible to do it in the
future. Will there be a problem implementing directory properties?
Will the config file format have to change?

>
> /* Clean out the per-iteration pool. */
> svn_pool_clear (subpool);
> @@ -147,9 +291,14 @@
> if ((kind == svn_node_dir) && recursive)
> err = add_dir_recursive (path, adm_access, ctx, pool);
> else
> - err = svn_wc_add (path, adm_access, NULL, SVN_INVALID_REVNUM,
> - ctx->cancel_func, ctx->cancel_baton,
> - ctx->notify_func, ctx->notify_baton, pool);
> + {
> + err = svn_wc_add (path, adm_access, NULL, SVN_INVALID_REVNUM,
> + ctx->cancel_func, ctx->cancel_baton,
> + ctx->notify_func, ctx->notify_baton, pool);
> + if (!err && kind == svn_node_file)
> + err = svn_client__add_auto_props (path, ctx, pool, adm_access,
> + NULL, NULL, NULL, NULL);
> + }
>
> return err;
> }
> Index: subversion/clients/cmdline/main.c
> ===================================================================
> --- subversion/clients/cmdline/main.c (revision 7208)
> +++ subversion/clients/cmdline/main.c (working copy)
> @@ -124,6 +124,10 @@
> "relocate via URL-rewriting"},
> {"config-dir", svn_cl__config_dir_opt, 1,
> "read user configuration files from directory ARG"},
> + {"auto-props", svn_cl__enable_autoprops_opt, 0,
> + "enable automatic properties"},
> + {"no-auto-props", svn_cl__disable_autoprops_opt, 0,
> + "disable automatic properties"},
> {0, 0, 0, 0}
> };
>
> @@ -151,7 +155,8 @@
> "Put files and directories under revision control, scheduling\n"
> "them for addition to repository. They will be added in next commit.\n"
> "usage: add PATH...\n",
> - {svn_cl__targets_opt, 'N', 'q', svn_cl__config_dir_opt} },
> + {svn_cl__targets_opt, 'N', 'q', svn_cl__config_dir_opt,
> + svn_cl__enable_autoprops_opt, svn_cl__disable_autoprops_opt} },

The command takes both options? Isn't one the default? We don't
generally have both enable and disable options on the same command.

>
> { "cat", svn_cl__cat, {0},
> "Output the content of specified files or URLs.\n"
> @@ -261,7 +266,8 @@
> " If PATH is omitted '.' is assumed. Parent directories are created\n"
> " as necessary in the repository.\n",
> {'m', 'F', 'q', 'N', SVN_CL__AUTH_OPTIONS, svn_cl__force_log_opt,
> - svn_cl__editor_cmd_opt, svn_cl__encoding_opt, svn_cl__config_dir_opt} },
> + svn_cl__editor_cmd_opt, svn_cl__encoding_opt, svn_cl__config_dir_opt,
> + svn_cl__enable_autoprops_opt, svn_cl__disable_autoprops_opt} },

ditto.

>
> { "info", svn_cl__info, {0},
> "Display info about a resource.\n"
> @@ -841,6 +847,30 @@
> svn_path_canonicalize (opt_arg,
> pool));
> break;
> + case svn_cl__enable_autoprops_opt:
> + if (opt_state.disable_autoprops)
> + {
> + err = svn_error_create (SVN_ERR_CL_MUTUALLY_EXCLUSIVE_ARGS, NULL,
> + "--auto-props and --no-autoprops are "
> + "mutually exclusive.");
> + svn_handle_error (err, stderr, FALSE);
> + svn_pool_destroy (pool);
> + return EXIT_FAILURE;
> + }
> + opt_state.enable_autoprops = TRUE;
> + break;
> + case svn_cl__disable_autoprops_opt:
> + if (opt_state.enable_autoprops)
> + {
> + err = svn_error_create (SVN_ERR_CL_MUTUALLY_EXCLUSIVE_ARGS, NULL,
> + "--auto-props and --no-autoprops are "
> + "mutually exclusive.");
> + svn_handle_error (err, stderr, FALSE);
> + svn_pool_destroy (pool);
> + return EXIT_FAILURE;
> + }
> + opt_state.disable_autoprops = TRUE;
> + break;
> default:
> /* Hmmm. Perhaps this would be a good place to squirrel away
> opts that commands like svn diff might need. Hmmm indeed. */
> @@ -993,6 +1023,23 @@
> svn_config_set (cfg, SVN_CONFIG_SECTION_HELPERS,
> SVN_CONFIG_OPTION_DIFF3_CMD, opt_state.merge_cmd);
>
> + /* Update auto-props-enable option for add/import commands */
> + if (subcommand->cmd_func == svn_cl__add
> + || subcommand->cmd_func == svn_cl__import)
> + {
> + char *cfgopt = (subcommand->cmd_func == svn_cl__add)
> + ? SVN_CONFIG_OPTION_ENABLE_AUTO_PROPS_ADD
> + : SVN_CONFIG_OPTION_ENABLE_AUTO_PROPS_IMP;

should this be "const char *"?

> + if (opt_state.enable_autoprops)
> + {
> + svn_config_set (cfg, SVN_CONFIG_SECTION_MISCELLANY, cfgopt, "true");
> + }
> + if (opt_state.disable_autoprops)
> + {
> + svn_config_set (cfg, SVN_CONFIG_SECTION_MISCELLANY, cfgopt, "false");
> + }
> + }
> +
> /* Set the log message callback function. Note that individual
> subcommands will populate the ctx.log_msg_baton */
> ctx.log_msg_func = svn_cl__get_log_message;
> Index: subversion/tests/clients/cmdline/autoprop_tests.py
> ===================================================================
> --- subversion/tests/clients/cmdline/autoprop_tests.py (revision 0)
> +++ subversion/tests/clients/cmdline/autoprop_tests.py (revision 0)

Yay! Tests! Looks good!

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