Issue 3085 mod_dav_svn and pre-revprop-change hooks

http://subversion.tigris.org/issues/show_bug.cgi?id=3085

The issue is that when a pre-revprop-change hook fails mod_dav_svn
runs the hook a second time but with a different property change.
This means that the message returned to the user doesn't contain the
correct hook error output.

The reason for this is that mod_dav has a rollback mechanism. When a
reprop is changed mod_dav first calls mod_dav_svn to setup for
rollback, then it calls mod_dav_svn a second time to make the change,
and if the change fails it calls mod_dav_svn a third time to rollback
the change.

If I patch mod_dav_svn to make the rollback into a no-op then I get
the expected behaviour, the hook runs once and the change succeeds or
fails. It also fixes the problem that when a post-revprop-change
hook fails it reverts the successful change.

There is however one problem: the message returned to the client no
longer contains the hook output. This is because when mod_dav_svn
returns an the hook error to mod_dav then mod_dav creates it's own
generic PROPPATCH failed error and returns that to the user. Before I
made rollback a no-op the rollback could return an error that was
generated after mod_dav's generic PROPPATCH error and that gets
returned to thwe client.

I can be sneaky and work around this. When the hook fails mod_dav_svn
can cache the error, and then when rollback gets invoked the cached
error can be returned even though rollback is otherwise a no-op.

What do people think? I suppose the proper solution is to change
mod_dav to return Subversion's error but then users would need to
coordinate upgrades to both mod_dav_svn and mod_dav.

Proof of principle patch:

Index: subversion/mod_dav_svn/deadprops.c
===================================================================
--- subversion/mod_dav_svn/deadprops.c (revision 947740)
+++ subversion/mod_dav_svn/deadprops.c (working copy)
@@ -218,6 +218,8 @@
                                                db->authz_read_func,
                                                db->authz_read_baton,
                                                resource->pool);
+ if (serr)
+ resource->info->revprop_error = svn_error_dup(serr);
 
           /* Tell the logging subsystem about the revprop change. */
           dav_svn__operational_log(resource->info,
@@ -681,12 +683,20 @@
 static dav_error *
 db_apply_rollback(dav_db *db, dav_deadprop_rollback *rollback)
 {
+#if 0
   if (rollback->value.data == NULL)
     {
       return db_remove(db, &rollback->name);
     }
 
   return save_value(db, &rollback->name, &rollback->value);
+#endif
+ db->props = NULL;
+ if (db->resource->info->revprop_error)
+ return dav_svn__convert_err(db->resource->info->revprop_error,
+ HTTP_INTERNAL_SERVER_ERROR, NULL,
+ db->resource->pool);
+ return NULL;
 }
 
 
Index: subversion/mod_dav_svn/dav_svn.h
===================================================================
--- subversion/mod_dav_svn/dav_svn.h (revision 947740)
+++ subversion/mod_dav_svn/dav_svn.h (working copy)
@@ -271,6 +271,8 @@
      interface (ie: /path/to/item?p=PEGREV]? */
   svn_boolean_t pegged;
 
+ svn_error_t *revprop_error;
+
   /* Pool to allocate temporary data from */
   apr_pool_t *pool;
 };
 

-- 
Philip