[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: Daniel Shahaf <danielsh_at_elego.de>
Date: Wed, 7 Aug 2013 17:54:29 +0300

A review mostly of the code formatting (whitespace etc). In particular
I'm not reviewing the feature being added:

> Index: subversion/include/svn_props.h
> ===================================================================
> --- subversion/include/svn_props.h (revision 1509957)
> +++ subversion/include/svn_props.h (working copy)
> @@ -412,6 +412,14 @@ svn_prop_name_is_valid(const char *prop_name);
> */
> #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.

That's still wrong. It does not make sense to describe this symbol as
"for compatibility with 1.4" since /tags/1.4.0/subversion/include/
doesn't have it.

Do you understand what @deprecated means? It's used to mark API
entities that were added in a prior minor (1.x.0) release that are
scheduled to be removed in the next major (x.0.0) release, so it doesn't
make sense to create an API deprecated (only to deprecate an API that
has appeared in a minor release).

> + * @since New in 1.9.
> + */
> +#define SVN_PROP_USE_COMMIT_TIMES_VALUE SVN_PROP_BOOLEAN_TRUE

> @@ -449,6 +457,12 @@ svn_prop_name_is_valid(const char *prop_name);
> +/**
> + * Property used to control whether timestamp of a file is modified to the commit time * @since New in 1.9.

Fix line breaks, wrap to 80 columns.

> + * @since New in 1.9.
> + */
> +#define SVN_PROP_USE_COMMIT_TIMES SVN_PROP_PREFIX "use-commit-times"

> +++ subversion/libsvn_wc/update_editor.c (working copy)
> @@ -751,6 +751,12 @@ struct file_baton
>
> /* 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;

Please make the comments say more than the struct member name does.

> @@ -3830,6 +3836,17 @@ change_file_prop(void *file_baton,
> fb->already_notified = TRUE;
> }
> }
> + if (strcmp(name, SVN_PROP_USE_COMMIT_TIMES) == 0)
> + {
> + if (propchange->value)

Wrong indentation. The 'if' (including its block) should be two spaces
fewer indented than it is.

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

"mutually exclusive"

> + SVN_ERR_ASSERT( (fb->use_commit_times & fb->remove_use_commit_times) == 0 );

Use boolean-and, not bitwise.

> + if ( !fb->adding_file )

No whitespace between parentheses and condition.

> " before it is modified. Makes the working copy file read-only¥n"
> " when it is not locked. Use 'svn propdel svn:needs-lock PATH...'¥n"
> " to clear.¥n"
> + " svn:use-commit-times - If present, make the timestamp of the file¥n"
> + " to commit time. Use 'svn propdel svn:use-commit-times PATH...' to clear.¥n"
> "¥n"
> " Subversion recognizes the following special versioned properties on a¥n"
> " directory:¥n"

Encoding problem? I see a Yen symbol where a backslash should be.

> + 'svn:date', '2001-01-01T00:00:00.000000Z',
> + secs_svn_date = time.mktime( (2001, 1, 1, 0, 0, 0, 0, 0, 0) )

Personally I wouldn't use all-zeroes as the test value, that's too
likely to false negative the test (ie, pass when there's a problem).

> - checkout_wc_from_drive
> + checkout_wc_from_drive,
> + checkout_with_use_commit_times

Please add a trailing comma.

> ]
Received on 2013-08-07 16:55:12 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.