Re: [PATCH] automatic properties

Martin Furter wrote:
>
> On Mon, 29 Sep 2003, Julian Foad wrote:
>
>>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?
>
> If i understood it correctly the current behaviour is the following when
> importing:
> 1. check if the file contains a '\0', if yes set the property
> svn:mime-type to application/octet-stream
> 2. check if the file has the executable bit set, if yes add the
> property svn:executable
> These properties are applied in subversion/libsvn_wc/adm_ops.c in
> import_file() and subversion/libsvn_client/commit.c in svn_wc_add().

(The other way round - in adm_ops.c n svn_wc_add and commit.c in import_file.)

> When adding a file svn_wc_add() sets these properties and
> svn_client__add_auto_props() overwrites them.
> When importing svn_client__add_auto_props() adds the properties and
> import_file() adds the dfault if they aren't allready set.
> So both commands have the same result.

Ah, yes. I missed that. I am glad that I was wrong. I was misled because there is no call to "svn_io_detect_mimetype" in subversion/libsvn_client/add.c. The call is at a lower level, in svn_wc_add.

The implementations of "svn_io_detect_mimetype" and "svn_io_is_file_executable" are in libsvn_wc because they ar WC-only operations. It seems to me that your new property-configuration function is very similar: it is also a WC-only operation, and so belongs with its two friends in libsvn_wc. I would make it like this:

/* Return a hash of configured AND detected properties together */
svn_wc_get_auto_props(...)
{
  /* Look up auto-props in the config file (the first half of
     your svn_client__add_auto_props). */

  /* Detect mime-type and executable if they were not configured. */

  /* Return all of these properties in a hash table. */
}

and then the two existing callers of "svn_io_is_file_executable"/"svn_io_detect_mime_type" would call the single new function instead of those two old functions:

import_file(...)
{
  ...
  properties = svn_wc_get_auto_props(...);
  for (hi in properties)
    SVN_ERR (editor->change_file_prop (file_baton, hi->name, hi->value, pool));
  ...
}

svn_wc_add(...)
{
  ...
  if (! copyfrom_url)
    {
      properties = svn_wc_get_auto_props(...);
      for (hi in properties)
        SVN_ERR (svn_wc_prop_set (hi->name, hi->value, path, parent_access, pool));
    }
  ...
}

This way, the logic of reading the configured properties and detecting two properties and having the former override the latter is all in one place, and there is only one call to each of editor->change_file_prop and svn_wc_prop_set rather than three of each.

The only remaining wrinkle is that the choice between "add" and "import" still has to be passed to svn_wc_get_auto_props(...). Now, you might not like this, but I'm going to suggest that the ability to enable auto-props for "add" but not "import" or vice-versa is not necessary. Not only is it not necessary, but it is not sufficient, because as soon as someone sees that they can enable one property during adds only, they will want to enable another property during commits ... without turning off the one for adds. I.e. I think that the ability to selectively _enable_ auto-props is not of very much use without being able to selectively _specify_ them. Of course it is of _some_ use; I don't dispute that. But enough use to justify the extra complication (small though it is)?

I hope you will agree that making this simplification to the configurability would actually improve the overall scheme.

>>>+/* 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.
>
> I could duplicate the function to get two smaller functions which do
> almost the same: check that autoprops are enabled, enumerate the config
> and finally set the properties.
> And i think adding a comment that both functions do the same thing and
> should be checked when something changes would be good too.

Oh, it would only be good to split it into two functions if duplication of code can be avoided. In other words, there would have to be a third function which contains the code that would be common to both.

But, as I say, I think we can make this distinction go away
- Julian

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Tue Sep 30 02:39:06 2003