svn commit: r1079508 - in /subversion/trunk/subversion: include/svn_client.h include/svn_wc.h libsvn_client/commit_util.c libsvn_client/deprecated.c svn/notify.c

[Daniel Shahaf]

Author: cmpilato
Date: Tue Mar 8 20:05:50 2011
New Revision: 1079508
URL: http://svn.apache.org/viewvc?rev=1079508&view=rev
For issue #3752 ("Warn about committing copied directory at depth
'empty'"), introduce new notification types for overwriting and
non-overwriting copies. Current, the command-line client treats these
just as it does their non-copy-ful equivalents.
* subversion/include/svn_wc.h
(svn_wc_notify_action_t): Add new 'svn_wc_notify_commit_copied' and
'svn_wc_notify_commit_copied_replaced' notification types.
* subversion/include/svn_client.h
(svn_client_commit5): Update docstring to indicate that this
function will transmit the new notification types, too.
(svn_client_commit4): Explicitly note that this interface does *not*
use the new notification types.
* subversion/libsvn_client/commit_util.c
(do_item_commit): Check for a non-NULL copyfrom_url on the commit
item and, if found, use the new notification types rather than
mere "added" and "replaced" notifications.
* subversion/libsvn_client/deprecated.c
(struct downgrade_commit_copied_notify_baton,
downgrade_commit_copied_notify_func): New notification wrapper
function and baton.
(svn_client_commit4): Use downgrade_commit_copied_notify_func()
wrapper as necessary to avoid sending unrecognized notification
types to the caller.
* subversion/svn/notify.c
(notify): Handle svn_wc_notify_commit_copied as
svn_wc_notify_commit_added; svn_wc_notify_commit_copied_replaced
as svn_wc_notify_commit_replaced.
subversion/trunk/subversion/include/svn_client.h
subversion/trunk/subversion/include/svn_wc.h
subversion/trunk/subversion/libsvn_client/commit_util.c
subversion/trunk/subversion/libsvn_client/deprecated.c
subversion/trunk/subversion/svn/notify.c
Modified: subversion/trunk/subversion/include/svn_client.h
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/include/svn_client.h?rev=1079508&r1=1079507&r2=1079508&view=diff
==============================================================================
--- subversion/trunk/subversion/include/svn_client.h (original)
+++ subversion/trunk/subversion/include/svn_client.h Tue Mar 8 20:05:50 2011
@@ -1850,6 +1850,7 @@ svn_client_import(svn_client_commit_info
* actions: #svn_wc_notify_commit_modified, #svn_wc_notify_commit_added,
* #svn_wc_notify_commit_deleted, #svn_wc_notify_commit_replaced,
+ * #svn_wc_notify_commit_copied, #svn_wc_notify_commit_copied_replaced,
* #svn_wc_notify_commit_postfix_txdelta.
*
@@ -1895,7 +1896,10 @@ svn_client_commit5(const apr_array_heade
/**
+ * #svn_wc_notify_commit_copied or #svn_wc_notify_commit_copied_replaced
+ * (preferring #svn_wc_notify_commit_added and
+ * #svn_wc_notify_commit_replaced, respectively, instead).
*
* #SVN_INVALID_REVNUM, then the commit was a no-op; nothing needed to
Modified: subversion/trunk/subversion/include/svn_wc.h
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/include/svn_wc.h?rev=1079508&r1=1079507&r2=1079508&view=diff
==============================================================================
--- subversion/trunk/subversion/include/svn_wc.h (original)
+++ subversion/trunk/subversion/include/svn_wc.h Tue Mar 8 20:05:50 2011
@@ -1144,6 +1144,16 @@ typedef enum svn_wc_notify_action_t
svn_wc_notify_patch_hunk_already_applied,
+ /** Committing a non-overwriting copy (path is the target of the
+ * copy, not the source).
+ svn_wc_notify_commit_copied,
+
+ /** Committing an overwriting (replace) copy (path is the target of
+ * the copy, not the source).
+ svn_wc_notify_commit_copied_replaced,
+
/** The server has instructed the client to follow a URL
* redirection.
Modified: subversion/trunk/subversion/libsvn_client/commit_util.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_client/commit_util.c?rev=1079508&r1=1079507&r2=1079508&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_client/commit_util.c (original)
+++ subversion/trunk/subversion/libsvn_client/commit_util.c Tue Mar 8 20:05:50 2011
@@ -1391,8 +1391,14 @@ do_item_commit(void **dir_baton,
/* We don't print the "(bin)" notice for binary files when
replacing, only when adding. So we don't bother to get
the mime-type here. */
- notify = svn_wc_create_notify(npath, svn_wc_notify_commit_replaced,
- pool);
+ if (item->copyfrom_url)
+ notify = svn_wc_create_notify(npath,
+ svn_wc_notify_commit_copied_replaced,
+ pool);
+ else
+ notify = svn_wc_create_notify(npath, svn_wc_notify_commit_replaced,
+ pool);
+
}
else if (item->state_flags & SVN_CLIENT_COMMIT_ITEM_DELETE)
{
@@ -1401,8 +1407,13 @@ do_item_commit(void **dir_baton,
}
else if (item->state_flags & SVN_CLIENT_COMMIT_ITEM_ADD)
{
- notify = svn_wc_create_notify(npath, svn_wc_notify_commit_added,
- pool);
+ if (item->copyfrom_url)
+ notify = svn_wc_create_notify(npath, svn_wc_notify_commit_copied,
+ pool);
+ else
+ notify = svn_wc_create_notify(npath, svn_wc_notify_commit_added,
+ pool);
+
if (item->kind == svn_node_file)
{
const svn_string_t *propval;
Modified: subversion/trunk/subversion/libsvn_client/deprecated.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_client/deprecated.c?rev=1079508&r1=1079507&r2=1079508&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_client/deprecated.c (original)
+++ subversion/trunk/subversion/libsvn_client/deprecated.c Tue Mar 8 20:05:50 2011
@@ -425,6 +425,43 @@ svn_client_import(svn_client_commit_info
return svn_error_return(err);
}
+
+/* Wrapper notify_func2 function and baton for downgrading
+ svn_wc_notify_commit_copied and svn_wc_notify_commit_copied_replaced
+ to svn_wc_notify_commit_added and svn_wc_notify_commit_replaced,
+ respectively. */
+struct downgrade_commit_copied_notify_baton
+{
+ svn_wc_notify_func2_t orig_notify_func2;
+ void *orig_notify_baton2;
+};
+
+static void
+downgrade_commit_copied_notify_func(void *baton,
+ const svn_wc_notify_t *notify,
+ apr_pool_t *pool)
+{
+ svn_wc_notify_t *my_notify = (svn_wc_notify_t *)notify;
+ struct downgrade_commit_copied_notify_baton *b = baton;
+
+ if (notify->action == svn_wc_notify_commit_copied)
+ {
+ my_notify = svn_wc_dup_notify(notify, pool);
+ my_notify->action = svn_wc_notify_commit_added;
+ }
+ else if (notify->action == svn_wc_notify_commit_copied)
+ {
+ my_notify = svn_wc_dup_notify(notify, pool);
+ my_notify->action = svn_wc_notify_commit_added;
+ }
+
+ /* Call the wrapped notification system with MY_NOTIFY, which is
+ either the original NOTIFY object, or a tweaked deep copy
+ thereof. */
+ b->orig_notify_func2(b->orig_notify_baton2, my_notify, pool);
+}
+
+
svn_error_t *
svn_client_commit4(svn_commit_info_t **commit_info_p,
const apr_array_header_t *targets,
@@ -437,14 +474,33 @@ svn_client_commit4(svn_commit_info_t **c
apr_pool_t *pool)
{
struct capture_baton_t cb;
+ struct downgrade_commit_copied_notify_baton notify_baton;
+ svn_error_t *err;
+
+ notify_baton.orig_notify_func2 = ctx->notify_func2;
+ notify_baton.orig_notify_baton2 = ctx->notify_baton2;
*commit_info_p = NULL;
cb.info = commit_info_p;
cb.pool = pool;
- SVN_ERR(svn_client_commit5(targets, depth, keep_locks, keep_changelists,
- changelists, revprop_table,
- capture_commit_info, &cb, ctx, pool));
+ /* Swap out the notification system (if any) with a thin filtering
+ wrapper. */
+ if (ctx->notify_func2)
+ {
+ ctx->notify_func2 = downgrade_commit_copied_notify_func;
+ ctx->notify_baton2 = &notify_baton;
+ }
+
+ err = svn_client_commit5(targets, depth, keep_locks, keep_changelists,
+ changelists, revprop_table,
+ capture_commit_info, &cb, ctx, pool);
+
+ /* Ensure that the original notification system is in place. */
+ ctx->notify_func2 = notify_baton.orig_notify_func2;
+ ctx->notify_baton2 = notify_baton.orig_notify_baton2;
+

This is actually a race condition, isn't it? (for clients that call the
deprecated API while using CTX->notify_func2 in another thread (in the
same or another API))

(Okay, so maybe we'll just let it live on. Presumably N other
deprecated wrappers do this too.)

+ SVN_ERR(err);
if (! *commit_info_p)
*commit_info_p = svn_create_commit_info(pool);
Modified: subversion/trunk/subversion/svn/notify.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/svn/notify.c?rev=1079508&r1=1079507&r2=1079508&view=diff
==============================================================================
--- subversion/trunk/subversion/svn/notify.c (original)
+++ subversion/trunk/subversion/svn/notify.c Tue Mar 8 20:05:50 2011
@@ -668,6 +668,7 @@ notify(void *baton, const svn_wc_notify_
break;
if (n->mime_type && svn_mime_type_is_binary(n->mime_type))
{
if ((err = svn_cmdline_printf(pool,
@@ -691,6 +692,7 @@ notify(void *baton, const svn_wc_notify_
break;
if ((err = svn_cmdline_printf(pool,
_("Replacing %s\n"),
path_local)))

[C. Michael Pilato]

Post by Daniel Shahaf

Author: cmpilato
Date: Tue Mar 8 20:05:50 2011
New Revision: 1079508

[...]

Post by Daniel Shahaf

svn_client_commit4(svn_commit_info_t **commit_info_p,
const apr_array_header_t *targets,

[...]

Post by Daniel Shahaf

+ /* Ensure that the original notification system is in place. */
+ ctx->notify_func2 = notify_baton.orig_notify_func2;
+ ctx->notify_baton2 = notify_baton.orig_notify_baton2;
+

This is actually a race condition, isn't it? (for clients that call the
deprecated API while using CTX->notify_func2 in another thread (in the
same or another API))
(Okay, so maybe we'll just let it live on. Presumably N other
deprecated wrappers do this too.)

I hadn't considered that. We do alot of pointer swaps like this up in the
command-line client code itself, but that's a bit different than doing so
down in the libsvn_client library as in this case. There is one prior
instance of us doing this kind of swap inside the libsvn_client library: in
libsvn_client/copy.c:repos_to_wc_copy_single(). But I'd prefer mere
precedent not to be our reason for allowing badness to persist in the codebase.

Got suggestions?

[Daniel Shahaf]

Post by C. Michael Pilato

Post by Daniel Shahaf

Author: cmpilato
Date: Tue Mar 8 20:05:50 2011
New Revision: 1079508

[...]

Post by Daniel Shahaf

svn_client_commit4(svn_commit_info_t **commit_info_p,
const apr_array_header_t *targets,

[...]

Post by Daniel Shahaf

+ /* Ensure that the original notification system is in place. */
+ ctx->notify_func2 = notify_baton.orig_notify_func2;
+ ctx->notify_baton2 = notify_baton.orig_notify_baton2;
+

This is actually a race condition, isn't it? (for clients that call the
deprecated API while using CTX->notify_func2 in another thread (in the
same or another API))
(Okay, so maybe we'll just let it live on. Presumably N other
deprecated wrappers do this too.)

I hadn't considered that. We do alot of pointer swaps like this up in the
command-line client code itself, but that's a bit different than doing so
down in the libsvn_client library as in this case. There is one prior
instance of us doing this kind of swap inside the libsvn_client library: in
libsvn_client/copy.c:repos_to_wc_copy_single(). But I'd prefer mere
precedent not to be our reason for allowing badness to persist in the codebase.
Got suggestions?

Would this work? ---

svn_client_ctx_t ctx2 = *ctx;
ctx2.notify_func2 = notify_baton.orig_notify_func2;
ctx2.notify_baton2 = notify_baton.orig_notify_baton2;
svn_client_commit5(ctx=&ctx2);

[C. Michael Pilato]

[Tweaking Subject: for (hopefully) additional visibility.]

Post by Daniel Shahaf

Post by C. Michael Pilato

Post by Daniel Shahaf

Author: cmpilato
Date: Tue Mar 8 20:05:50 2011
New Revision: 1079508

[...]

Post by Daniel Shahaf

svn_client_commit4(svn_commit_info_t **commit_info_p,
const apr_array_header_t *targets,

[...]

Post by Daniel Shahaf

+ /* Ensure that the original notification system is in place. */
+ ctx->notify_func2 = notify_baton.orig_notify_func2;
+ ctx->notify_baton2 = notify_baton.orig_notify_baton2;
+

This is actually a race condition, isn't it? (for clients that call the
deprecated API while using CTX->notify_func2 in another thread (in the
same or another API))
(Okay, so maybe we'll just let it live on. Presumably N other
deprecated wrappers do this too.)

I hadn't considered that. We do alot of pointer swaps like this up in the
command-line client code itself, but that's a bit different than doing so
down in the libsvn_client library as in this case. There is one prior
instance of us doing this kind of swap inside the libsvn_client library: in
libsvn_client/copy.c:repos_to_wc_copy_single(). But I'd prefer mere
precedent not to be our reason for allowing badness to persist in the codebase.
Got suggestions?

Would this work? ---
svn_client_ctx_t ctx2 = *ctx;
ctx2.notify_func2 = notify_baton.orig_notify_func2;
ctx2.notify_baton2 = notify_baton.orig_notify_baton2;
svn_client_commit5(ctx=&ctx2);