On Tue, Aug 12, 2008 at 02:24:42AM -0400, Matt Slot wrote:
> I've enclosed the latest diff that adds AppleDouble support to 1.5.1,
> applying many of the suggestions posted earlier.
In general, patches against trunk are preferred, because all
changes enter our code base through trunk.
> Some notes and questions regarding the patch:
>
> -- The patch now responds to SVN_HAVE_APPLEDOUBLE instead of DARWIN,
> but I'm leery of touching the configure script to make it recognize
> the platform and enable the feature.
>
> -- I've replaced the controversial "applefile.h" with a rewritten-
> from-scratch interface file "svn_applefile.h" that uses standard APR
> types. This should avoid any legal issues with the header from the RFC.
>
> -- It was necessary to add a parameter to install_committed_file()
> in libsvn_wc/log.c, but I'm not sure that conditionally changing a
> function declaration is necessarily the best way to go. Is there a
> preferred way to conditionalize this?
Just pass the parameter on all platforms. It is OK if it is ignored
on platforms which don't support the feature.
> -- I based my patch on the implementation of svn:executable auto-
> property, which does not recognize when the executable bit is changed
> in the working copy. I worked around this in my original patch by
> posting a script to "bang" the property flag when the user modifies
> the resource file, but I'd prefer that svn recognize these
> modifications and flag the property accordingly.
Are you saying that you've already implemented such "on-demand"
recognition you're describing, or not?
>
> Thanks!
Thank you for the patch.
Two minor stylistic nits:
> +/** Portable declaration of MacOS QuickDraw Point. */
> +typedef struct Point
> +{
Since you are defining these in a public header file,
all these structs need an svn_appledouble_ prefix.
I wonder if it is needed to expose these structs in public API.
Couldn't this header be moved to subversion/include/private/ ?
In any case, the naming conventions for the structs should
follow the ones of other headers. "struct Point" should probably
be "struct svn_appledouble_point", etc.
> + apr_int16_t h;
> + apr_int16_t v;
> +} Point;
> +
[ ...]
> @@ -202,6 +205,14 @@
> }
> #endif
>
> +#ifdef SVN_HAVE_APPLEDOUBLE
> + /* load any appledouble data for the indicated file */
> + SVN_ERR(svn_io_get_appledouble_data(&appledouble, path, pool));
> + if (appledouble)
> + apr_hash_set(autoprops.properties, SVN_PROP_APPLEDOUBLE,
> + strlen (SVN_PROP_APPLEDOUBLE), appledouble);
No space after function names, i.e. 'strlen(' instead of 'strlen ('.
Glancing over the patch I could not see much else wrong with it
stylistically.
Someone will need to test it properly though before we can commit it.
@everyone:
I might not have clearly stated this before, but I am +1 on the idea
of adding this functionality now. But only on the condition that we decide
to move out all platform-specific property handling into its own layer
in the future, as proposed by Julian. This code, and all the other special
props managing platform-specific features shall go into that layer
once it's ready. The client and working copy libraries should not get
littered with additional platform-specific code in the future.
It really does not belong there.
I think we should open a "bite-sized" issue for factoring out
platform-specific property handling into a layer of its own.
Shall I do that?
Stefan
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe_at_subversion.tigris.org
For additional commands, e-mail: dev-help_at_subversion.tigris.org
Received on 2008-08-12 12:33:48 CEST