Martin Furter wrote:
>
> Here's the next version of the patch.
I think this is a very useful feature, but I am a little concerned at adding this fairly substantial amount of code and behaviour while trying to get a stable version 1.0 ready. I would definitely like to see it go in soon though, even if it is after 1.0. The imlementation looks pretty good to me; a few comments follow.
I am wondering about the relationship between these new configurable auto-properties and the existing non-configurable automatic properties "svn:executable" and "svn:mimetype". The exising non-configurable auto-props seem to be applied only on commit, and not on add. It would be silly for the two types of auto-props not to work in the same situations. Is there a good reason why they don't?
A lesser issue is that the configurable auto-props do not appear to cleanly override the existing non-configurable ones, especially svn:executable, as I have noted in the patch below. I am not quite sure of this, but I can't see how to choose among the three options:
svn:executable OFF
svn:executable ON
svn:executable AUTO
and similarly for mime-type NONE, <SPECIFIED>, AUTO. As I say, a lesser issue, but I think it ought to be possible.
> ----- log message -----
> Automatic properties for 'svn add' and 'svn import' added (issue #1502).
That is probably enough information for the log message header. It says what the purpose of this commit is. The description of how the new feature works, and the syntax of the config file, belongs in the book and the config file and/or elsewhere.
...
> ----- end of log message -----
>
> ----- patch -----
...
> Index: subversion/libsvn_subr/config_file.c
> ===================================================================
> --- subversion/libsvn_subr/config_file.c (revision 7208)
> +++ subversion/libsvn_subr/config_file.c (working copy)
> @@ -978,7 +978,30 @@
> "### Set use-commit-times to make checkout/update/switch/revert\n"
> "### put last-committed timestamps on every file touched.\n"
> "# use-commit-times = yes\n"
> + "### Enable automatic properties for 'svn add' (true | false).\n"
> + "### Automatic properties are defined in the section 'auto-props'.\n"
> + "# enable-auto-props-add = true\n"
> + "### Enable automatic properties for 'svn import' (true | false).\n"
> + "# enable-auto-props-imp = true\n"
> "\n"
> + "### Section for configuring automatic properties.\n"
> + "### The format of the entries is:\n"
> + "### expression = [propname[=value][;name[=val]]...]\n"
How about:
expression = propname[=value][;propname[=value]...]
to make clear that the things after the semicolon are the same type of things as before the semicolon. Better still,
file-name-pattern = propname[=value][;propname[=value]...]
> + "### The expression can contain * as wildcard. All entries which\n"
> + "### match will be applied to the file.\n"
Indentation: tabs have appeared here and in some other parts of this patch.
> + "# [auto-props]\n"
> + "# *.c = svn:eol-style=native\n"
> + "# *.cpp = svn:eol-style=native\n"
> + "# *.h = svn:eol-style=native\n"
> + "# *.dsp = svn:eol-style=CRLF\n"
> + "# *.dsw = svn:eol-style=CRLF\n"
> + "# *.sh = svn:eol-style=native;svn:executable\n"
> + "# *.txt = svn:eol-style=native\n"
> + "# *.png = svn:mimetype=image/png\n"
> + "# *.jpg = svn:mimetype=image/jpeg\n"
> + "# Makefile = svn:eol-style=native\n"
> + "\n"
> + "\n"
> "### See http://subversion.tigris.org/issues/show_bug.cgi?id=668\n"
> "### for what else will soon be customized in this file.\n";
>
> Index: subversion/libsvn_client/client.h
> ===================================================================
> --- subversion/libsvn_client/client.h (revision 7208)
> +++ subversion/libsvn_client/client.h (working copy)
> @@ -237,6 +237,32 @@
>
> /*** Add/delete ***/
>
> +/* Function which automatically adds configured properties.
> +
> + PATH, CTX and POOL must always be valid.
> +
> + When adding files or directories ADDING must be TRUE and ADM_ACCESS
> + must be valid.
> + When importing files or directories ADDING must be TRUE and EDITOR,
> + FILE_BATON, MIMETYPE and EXECUTABLE must be valid.
> +
> + The caller of this method is responsible to create a subpool if needed.
> +
> + First the function checks that auto-props is enabled for add or import.
> + Then it enumerates the 'auto-props' section and tries to find fnmatches.
> + All matches found are parsed and the properties added to given PATH.
> +*/
> +svn_error_t *
> +svn_client__add_auto_props (const char **mimetype,
> + int *executable,
> + const char *path,
> + svn_client_ctx_t *ctx,
> + const svn_boolean_t adding,
> + svn_wc_adm_access_t *adm_access,
> + const svn_delta_editor_t *editor,
> + void *file_baton,
> + apr_pool_t *pool);
> +
I mentioned in a separate mail about that being two functions - but only if Brane agrees. Or, ideally, the disparity between the existing auto-props acting only on commit versus these acting on both add and commit would go away, thus making this distinction redundant.
> /* 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)
> @@ -114,21 +114,28 @@
> 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 (&mimetype, &executable, path, ctx,
> + FALSE, NULL, editor, file_baton,
> + pool));
> +
> /* 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));
So the configured auto-props can force a particular MIME type to be set, but can they force no MIME type to be set? Perhaps they can do this by specifying an empty value: "svn:mime-type=".
> 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)
This is a bit odd. The configured auto-props can override the built-in detection in one direction (forcing "svn:executable" to be set), but not in the other direction (forcing "svn:executable" to be not set).
> + 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,159 @@
> #include "svn_error.h"
> #include "svn_path.h"
> #include "svn_io.h"
> +#include "svn_config.h"
> #include "client.h"
>
>
>
> /*** Code. ***/
>
> +/* This structure is used as baton for enumerating the config entries
> + in the auto-props section.
> +*/
> +typedef struct
> +{
> + /* the file name for which properties are searched */
> + char *filename;
> +
> + /* the hash table for storing the property name/value pairs */
> + apr_hash_t *properties;
> +
> + /* a pool used for allocationg memory */
"allocationg"?
> + apr_pool_t *pool;
> +} auto_props_baton_t;
> +
> +/* Enumerate through auto-props entries and store value if name matches.
> + NAME is the config entry name, VALUE its value and BATON must be an
> + auto_props_baton_t.
> +*/
How about:
/* For one auto-props config entry (NAME, VALUE), if the filename pattern
NAME matches BATON->filename then add the properties listed in VALUE
into BATON->properties. BATON must point to an auto_props_baton_t.
*/
> +static svn_boolean_t
> +auto_props_enumerator (const char *name,
> + const char *value,
> + void *baton)
> +{
> + auto_props_baton_t *autoprops = baton;
> + char *property;
> + char *last_token;
> + int len;
> +
> + /* nothing to do here without a value */
> + if (strlen (value) == 0)
> + return TRUE;
> +
> + /* 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)
> + {
> + apr_hash_set (autoprops->properties, property, len, value );
> + }
> + property = apr_strtok (NULL, ";", &last_token);
> + }
> + return TRUE;
> +}
> +
> +/* adds automatic properties, for a description look into client.h */
No need for that comment. It is standard practice (in this project) for the description of a function to be found at its declaration.
> +svn_error_t *
> +svn_client__add_auto_props (const char **mimetype,
> + int *executable,
> + const char *path,
> + svn_client_ctx_t *ctx,
> + const svn_boolean_t adding,
> + svn_wc_adm_access_t *adm_access,
> + const svn_delta_editor_t *editor,
> + void *file_baton,
> + apr_pool_t *pool)
> +{
> + svn_config_t *cfg;
> + auto_props_baton_t autoprops;
> + const char *cfgvalue;
> + 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 (adding)
> + svn_config_get (cfg, &cfgvalue, SVN_CONFIG_SECTION_MISCELLANY,
> + SVN_CONFIG_OPTION_ENABLE_AUTO_PROPS_ADD, "false");
> + else
> + {
> + svn_config_get (cfg, &cfgvalue, SVN_CONFIG_SECTION_MISCELLANY,
> + SVN_CONFIG_OPTION_ENABLE_AUTO_PROPS_IMP, "false");
> + /* initialize output parameters */
> + *mimetype = NULL;
> + *executable = FALSE;
> + }
> + if (strcmp (cfgvalue, "true") != 0)
> + 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 */
> + err = SVN_NO_ERROR;
> + for (hi = apr_hash_first (pool, autoprops.properties);
> + hi != NULL && err == SVN_NO_ERROR;
> + hi = apr_hash_next (hi))
> + {
> + char *propname;
> + svn_string_t propvalue;
> +
> + apr_hash_this (hi, (const void **)&propname, NULL,
> + (void **)&propvalue.data);
> + propvalue.len = strlen (propvalue.data);
> + if (adding)
> + /* we're adding, so just set the property */
> + err = svn_wc_prop_set (propname, &propvalue, path,
> + adm_access, pool);
> + else
> + {
> + /* SVN_PROP_MIME_TYPE and SVN_PROP_EXECUTABLE
> + * are returned to the caller when importing */
> + if (strcmp (propname, SVN_PROP_MIME_TYPE) == 0)
> + *mimetype = propvalue.data;
> + else if (strcmp (propname, SVN_PROP_EXECUTABLE) == 0)
> + *executable = TRUE;
> + else
> + err = editor->change_file_prop (file_baton, propname,
> + &propvalue, pool);
> + }
> + }
> +
> + return err;
> +}
> +
> +
> static svn_error_t *
> add_dir_recursive (const char *dirname,
> svn_wc_adm_access_t *adm_access,
> @@ -97,9 +245,14 @@
> 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 (NULL, NULL, fullpath, ctx,
> + TRUE, adm_access, NULL, NULL,
> + subpool));
> + }
>
> /* Clean out the per-iteration pool. */
> svn_pool_clear (subpool);
> @@ -147,9 +300,15 @@
> 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 (NULL, NULL, path, ctx,
> + TRUE, adm_access, NULL, NULL,
> + pool);
> + }
>
> return err;
> }
...
> 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)
> @@ -0,0 +1,509 @@
...
> +def autopros_test(sbox, cmd, cfgtype, paramtype, subdir):
"autopros_test" -> "autoprops_test" ?
(I didn't review the tests at all really, but just noticed that.)
...
> ----- and of patch -----
"and" -> "end" ? :-)
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Mon Sep 29 22:43:35 2003