[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: Subversion AppleDouble patch

From: Stefan Sperling <stsp_at_elego.de>
Date: Tue, 12 Aug 2008 12:33:59 +0200

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
> + /* 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

Someone will need to test it properly though before we can commit it.

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?


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

This is an archived mail posted to the Subversion Dev mailing list.

This site is subject to the Apache Privacy Policy and the Apache Public Forum Archive Policy.