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

Re: [PATCH] add 'svn:use-commit-times' property

From: Philip Martin <philip.martin_at_wandisco.com>
Date: Wed, 31 Jul 2013 15:09:10 +0100

Masaru Tsuchiyama <m.tmatma_at_gmail.com> writes:

> Index: subversion/include/svn_props.h
> ===================================================================
> --- subversion/include/svn_props.h (revision 1507762)
> +++ subversion/include/svn_props.h (working copy)
> @@ -412,6 +412,14 @@
> */
> #define SVN_PROP_SPECIAL_VALUE SVN_PROP_BOOLEAN_TRUE
>
> +/** The value to force the executable property to when set.
> + *
> + * @deprecated Provided for backward compatibility with the 1.4 API.
> + * Use @c SVN_PROP_BOOLEAN_TRUE instead.
> + * @since New in 1.9.
> + */
> +#define SVN_PROP_USE_COMMIT_TIMES_VALUE SVN_PROP_BOOLEAN_TRUE
> +

We don't want that, no existing code can be using it.

> /** Describes external items to check out into this directory.
> *
> * The format is a series of lines, each in the following format:
> @@ -449,6 +457,12 @@
> /** Property used to record inheritable configuration ignores. */
> #define SVN_PROP_INHERITABLE_IGNORES SVN_PROP_PREFIX "global-ignores"
>
> +/**
> + * Property used to control whether timestamp of a file is modified to the commit time * @since New in 1.9.
> + * @since New in 1.9.
> + */
> +#define SVN_PROP_USE_COMMIT_TIMES SVN_PROP_PREFIX "use-commit-times"
> +
> /** Meta-data properties.
> *
> * The following properties are used for storing meta-data about
> @@ -509,7 +523,8 @@
> SVN_PROP_TEXT_TIME, \
> SVN_PROP_OWNER, \
> SVN_PROP_GROUP, \
> - SVN_PROP_UNIX_MODE,
> + SVN_PROP_UNIX_MODE, \
> + SVN_PROP_USE_COMMIT_TIMES,
>
> /** @} */
>
> Index: subversion/libsvn_subr/properties.c
> ===================================================================
> --- subversion/libsvn_subr/properties.c (revision 1507762)
> +++ subversion/libsvn_subr/properties.c (working copy)
> @@ -444,7 +444,8 @@
> make any speed difference. */
> if (strcmp(prop_name, SVN_PROP_EXECUTABLE) == 0
> || strcmp(prop_name, SVN_PROP_NEEDS_LOCK) == 0
> - || strcmp(prop_name, SVN_PROP_SPECIAL) == 0)
> + || strcmp(prop_name, SVN_PROP_SPECIAL) == 0
> + || strcmp(prop_name, SVN_PROP_USE_COMMIT_TIMES) == 0)
> return TRUE;
> return FALSE;
> }
> Index: subversion/libsvn_wc/props.c
> ===================================================================
> --- subversion/libsvn_wc/props.c (revision 1507762)
> +++ subversion/libsvn_wc/props.c (working copy)
> @@ -2153,7 +2153,7 @@
> }
> else if (svn_prop_is_boolean(propname))
> {
> - /* SVN_PROP_EXECUTABLE, SVN_PROP_NEEDS_LOCK, SVN_PROP_SPECIAL */
> + /* SVN_PROP_EXECUTABLE, SVN_PROP_NEEDS_LOCK, SVN_PROP_SPECIAL, SVN_PROP_USE_COMMIT_TIMES */
> propval = &boolean_value;
> }
> else if (strcmp(propname, SVN_PROP_MERGEINFO) == 0)
> Index: subversion/libsvn_wc/update_editor.c
> ===================================================================
> --- subversion/libsvn_wc/update_editor.c (revision 1507762)
> +++ subversion/libsvn_wc/update_editor.c (working copy)
> @@ -23,6 +23,7 @@
>
>
>
> +#include <assert.h>

Avoid assert if possible.

> #include <stdlib.h>
> #include <string.h>
>
> @@ -751,6 +752,12 @@
>
> /* The tree conflict to install once the node is really edited */
> svn_skel_t *edit_conflict;
> +
> + /* use-commit-times */
> + svn_boolean_t use_commit_times;
> +
> + /* remove use-commit-times */
> + svn_boolean_t remove_use_commit_times;
> };
>
>
> @@ -3830,6 +3837,17 @@
> fb->already_notified = TRUE;
> }
> }
> + if ( strcmp(name, SVN_PROP_USE_COMMIT_TIMES) == 0)

The whitespace is wrong, use

     if (foo)

not

     if ( foo )

> + {
> + if( propchange->value )

and "if (" not "if(".

> + {
> + fb->use_commit_times = TRUE;
> + }
> + else
> + {
> + fb->remove_use_commit_times = TRUE;
> + }
> + }
>
> return SVN_NO_ERROR;
> }
> @@ -4636,6 +4654,24 @@
> eb->notify_func(eb->notify_baton, notify, scratch_pool);
> }
>
> + /* use_commit_times and remove_use_commit_times are mutably exclusive */
> + assert( (fb->use_commit_times & fb->remove_use_commit_times) == 0 );

Use SVN_ERR_ASSERT instead.

> + if ( !fb->adding_file )
> + {
> + if ( fb->use_commit_times && fb->changed_date )
> + {
> + SVN_ERR(svn_io_set_file_affected_time(fb->changed_date,
> + fb->local_abspath,
> + scratch_pool));
> + }
> + else if ( fb->remove_use_commit_times )
> + {
> + SVN_ERR(svn_io_set_file_affected_time(apr_time_now(),
> + fb->local_abspath,
> + scratch_pool));
> + }

The apr_time_now call looks odd. Do we need to change the file's
timestamp in this case?

> + }
> +
> svn_pool_destroy(fb->pool); /* Destroy scratch_pool */
>
> /* We have one less referrer to the directory */

You also need to update the help text for 'svn ps' to describe the new
property, see subversion/svn/svn.c.

Perhaps this property should be inheritable? Then it could be set on a
directory and apply to the whole tree? I imagine users will want that
but it raises a new problem: how would a node override the inherited
property? It can't delete a property that isn't set, and it can't set
it as that enables use-commit-times. How would we make inheriting work?
Perhaps another property svn:no-commit-times?

-- 
Philip Martin | Subversion Committer
WANdisco | Non-Stop Data
Received on 2013-07-31 16:09:53 CEST

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