Martin Furter wrote:
>
> On 1 Oct 2003, C. Michael Pilato wrote:
>
>>Martin Furter <mf@rola.ch> writes:
>>
>>>Yes, that works very well, only one issue remaining: i don't have the
>>>mimetype anymore which is used in the notify_func (f.ex. at the end of
>>>svn_wc_add):
>>>
>>> /* Report the addition to the caller. */
>>> if (notify_func != NULL)
>>> (*notify_func) (notify_baton, path, svn_wc_notify_add,
>>> kind,
>>> NULL, <--- this was mimetype
>>> svn_wc_notify_state_unknown,
>>> svn_wc_notify_state_unknown,
>>> SVN_INVALID_REVNUM);
>>>
>>>In import_file i can get it back because i have the hash containing the
>>>properties, but in svn_wc_add i can't.
>>>I could make a notify_func which stores the data in a baton and call the
>>>real notify_func after getting the auto-props.
>>>
>>>Or is it ok to use always NULL for mimetype ?
>>
>>If I'm thinking of the right spot, using NULL is the difference
>>between seeing:
>>
>> A /some/path
>>
>>and:
>>
>> A (bin) /some/path
>>
>>In other words, we do use the mimetype to point out places where the
>>code has made an automatic decision about binariness (which effects
>>lots of things -- the ability to do contextual merging, ability to set
>>EOL properties, etc.)
>>
>>Is this is a necessary feature? I have no strong opinions.
>
> While implementing the mimetype again i saw that svn_wc_add uses either
> constants or parameter known to libsvn_client in the call to the notify
> function.
> So i can call notify_func from the new add_file() function and give NULL
> as notify_func to svn_wc_add().
Thank you for your patience, Martin. This patch is looking much better now. I have only a few comments (below).
I have one question (for C-Mike etc.) about calling the notification function:
Given that svn_wc_add just adds a single item, non-recursively (in terms of notification, at least), why does it do notification? All of the parameters to the notification callback, except for node kind, are already known to the caller of svn_wc_add, and the node kind is easy to determine.
It seems to me that a non-recursive function has no business doing notification unless it needs to pass on lots of special knowledge about what it is doing. I think I'd like to see it disappear from there, but perhaps that could be in a separate, later patch. I'd like to hear someone else's opinion.
> I hope this patch is now acceptable.
Apart from the simple matter of the example text in the config file (noted below), there are just these two tricky questions:
- Should the notification handling be changed as part of this patch? If not, we should make sure to do it soon afterwards. (Two parts: it should take a properties hash instead of just a mime-type, and it should not be called from svn_wc_add.)
- Should this patch go in to the trunk before release 1.0?
I need somebody else to help answer those questions.
- Julian
> ----- log message -----
> Automatic properties for 'svn add' and 'svn import' added (issue #1502).
...
> ----- auto-props patch -----
> Index: build.conf
> Index: subversion/include/svn_config.h
Fine.
> Index: subversion/libsvn_wc/adm_ops.c
> ===================================================================
...
> @@ -1112,7 +1080,7 @@
> if (notify_func != NULL)
> (*notify_func) (notify_baton, path, svn_wc_notify_add,
> kind,
> - mimetype,
> + NULL,
> svn_wc_notify_state_unknown,
> svn_wc_notify_state_unknown,
> SVN_INVALID_REVNUM);
Probably OK for now, as noted above.
> Index: subversion/libsvn_subr/config_file.c
> ===================================================================
...
> + "### 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"
This still refers to separate switches for 'add' and 'import'.
> Index: subversion/libsvn_client/client.h
> ===================================================================
...
> +/* Read automatic properties matching PATH from CTX->config.
> + A hash is returned in *PROPERTIES containing propname/value pairs or
> + when auto-props are disabled *PROPERTIES is set to NULL.
> + *MIMETYPE is set to to the mimetype or to NULL.
> + This function does not create a subpool, the caller is responsible to
> + create one if necessary.
> +*/
> +svn_error_t *
> +svn_client__get_auto_props (apr_hash_t **properties,
> + const char **mimetype,
> + const char *path,
> + svn_client_ctx_t *ctx,
> + apr_pool_t *pool);
> +
The extra "mimetype" parameter ... yuck. Instead, the whole hash of properties should be passed to the notification callback so that it can print out all of the information if it wants to. But I think we could leave that as a separate change to be made after this patch has been committed. What you have done here is OK temporarily if we agree to fix it up properly soon afterwards.
> Index: subversion/libsvn_client/commit.c
> ===================================================================
> --- subversion/libsvn_client/commit.c (revision 7267)
> +++ subversion/libsvn_client/commit.c (working copy)
> @@ -106,29 +106,34 @@
> {
> void *file_baton;
> const char *mimetype;
> - svn_boolean_t executable;
> unsigned char digest[MD5_DIGESTSIZE];
> const char *text_checksum;
> + apr_hash_t* properties;
> + apr_hash_index_t *hi;
>
> /* Add the file, using the pool from the FILES hash. */
> SVN_ERR (editor->add_file (edit_path, dir_baton, NULL, SVN_INVALID_REVNUM,
> pool, &file_baton));
>
> - /* 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 (editor->change_file_prop (file_baton, SVN_PROP_MIME_TYPE,
> - svn_string_create (mimetype, pool),
> + /* add automatic properties */
> + SVN_ERR (svn_client__get_auto_props (&properties, &mimetype, path, ctx,
> pool));
> + if (properties)
> + {
> + for (hi = apr_hash_first (pool, properties);
> + hi != NULL; hi = apr_hash_next (hi))
> + {
> + char *propname;
> + svn_string_t propvalue;
>
> - /* 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 (editor->change_file_prop (file_baton, SVN_PROP_EXECUTABLE,
> - svn_string_create ("", pool),
> - pool));
> -
> + apr_hash_this (hi, (const void **)&propname, NULL,
> + (void **)&propvalue.data);
> + propvalue.len = strlen (propvalue.data);
I'd like someone else to check this method of creating a string.
> + SVN_ERR (editor->change_file_prop (file_baton, propname,
> + &propvalue, pool));
> + }
> + }
> +
> Index: subversion/libsvn_client/add.c
> ===================================================================
> static svn_error_t *
> +add_file (const char *path,
...
> + SVN_ERR (svn_wc_add (path, adm_access, NULL, SVN_INVALID_REVNUM,
...
> + SVN_ERR (svn_client__get_auto_props (&properties, &mimetype, path, ctx,
...
> + if (ctx->notify_func != NULL)
> + (*ctx->notify_func) (ctx->notify_baton, path, svn_wc_notify_add,
> + svn_node_file,
> + mimetype,
> + svn_wc_notify_state_unknown,
> + svn_wc_notify_state_unknown,
> + SVN_INVALID_REVNUM);
...
> @@ -97,9 +270,7 @@
> 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 (add_file (fullpath, ctx, dir_access, subpool));
>
> /* Clean out the per-iteration pool. */
> svn_pool_clear (subpool);
> @@ -146,6 +317,8 @@
> SVN_ERR (svn_io_check_path (path, &kind, pool));
> if ((kind == svn_node_dir) && recursive)
> err = add_dir_recursive (path, adm_access, ctx, pool);
> + else if (kind == svn_node_file)
> + err = add_file (path, ctx, adm_access, pool);
> else
> err = svn_wc_add (path, adm_access, NULL, SVN_INVALID_REVNUM,
> ctx->cancel_func, ctx->cancel_baton,
Probably OK for now, as above.
> Index: subversion/clients/cmdline/cl.h
> Index: subversion/clients/cmdline/main.c
Fine.
> Index: subversion/tests/clients/cmdline/autoprop_tests.py
> Index: subversion/tests/clients/cmdline/svntest/main.py
I don't see any problems but haven't looked closely at the tests.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Thu Oct 2 14:32:19 2003