On Mon, 29 Sep 2003, Julian Foad wrote:
> 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 try to keep it as small and simple as possible.
> 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().
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.
> 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
I'd prefer that when i chmod 755 a file and then add or import it, that
the svn:executable property is set even when i didn't configure autoprops
that way.
I started this WE learning python and hacked that test script. I don't
have the .py extension in my autoprops and for testing the script i had to
make it executable. So when i add it to the repos it would be nice when
svn:executable is added even when i not explicitely requested that.
> and similarly for mime-type NONE, <SPECIFIED>, AUTO. As I say, a
> lesser issue, but I think it ought to be possible.
For svn:mime-type i think it would be dangerous to remove the property
when it isn't explicitely set to a value. The SVN developers know better
why they added this detection of binary files.
A config entry like '*.doc = svn:mime-type' would result in the property
svn:mime-type set to an empty value. I don't know if that case should be
catched somehow and the property removed.
The behaviour of autoprops is that it only adds properties but never
removes (overwriting of svn:mimetype looks like adding a property for the
user). Adding these ON/OFF/AUTO features would not only make the visible
behaviour more complex, it would also need changes in libsvn_wc and
perhaps some duplicated code. At the moment the important code is in one
file (add.c) at one place (only 2 functions and a struct).
> ...
> > Index: subversion/libsvn_subr/config_file.c
> > 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.
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.
> > ----- and of patch -----
>
> "and" -> "end" ? :-)
Hmm, your eyes see every typo ;-)
Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Tue Sep 30 00:05:53 2003