Issue 3085 mod_dav_svn and pre-revprop-change hooks

[Philip Martin]

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