While unlocking a file forcibly using subversion client, and if the unlock is blocked by pre-unlock hook script, the client is getting "500 Internal Server Error" instead of the error message sent by the subversion server. Simple reproduction recipe: 1. Lock a file. "svn lock testfile.txt --username user1" 2. Make pre-unlock hook script to fail. 3. Try to unlock the file forcibly as a different user. "svn unlock testfile.txt --force --username user2" step 3 fails with "500 Internal Server Error" message. Whenever the pre-unlock hook fails, Subversion server sends the error message "Unlock blocked by pre-unlock hook (exit code 1)...." to mod_dav. But mod_dav is not handling the error and it is not sending any error messages to client side, instead it sends only HTTP_STATUS code alone(confirmed it with the following codepath). From dav/main/mod_dav.c: dav_method_unlock(), <snip> if ((result = dav_unlock(r, resource, locktoken)) != OK) { return result; } </snip> From dav/main/util_lock.c: dav_unlock(), <snip> err = (*repos_hooks->walk)(&ctx.w, DAV_INFINITY, &multi_status); /* ### fix this! */ /* ### do something with multi_status */ result = err == NULL ? OK : err->status; (*hooks->close_lockdb)(lockdb); return result; </snip> This walk() method finds out the resource to unlock and walker callback functin dav_unlock_walker() removes lock from that resource. From dav/main/util_lock.c: dav_unlock_walker(), <snip> if ((err = (*ctx->w.lockdb->hooks->remove_lock)(ctx->w.lockdb, wres->resource, ctx->locktoken)) != NULL) { /* ### should we stop or return a multistatus? looks like STOP */ /* ### add a higher-level description? */ return err; </snip> In this case, unlock is blocked by pre-unlock hook, subvesion server marshalls the error back to mod_dav, mod_dav's dav_unlock_walker() returns the error sent by svn server, but dav_unlock() returns only the HTTP_STATUS code(err->status) and the error message is *lost* there.
Created attachment 27095 [details] patch that improves error handling during "dav unlock" Attaching the patch that fixes this issue. This patch does the following things to maintain backward compatibility. 1. Copied dav/main/util_lock.c: dav_unlock() to a static function do_dav_unlock() which accepts "dav_error **err" as an outparam and preserves any error in case of failures. 2. dav_unlock() will call do_dav_unlock() and return HTTP_STATUS code in case of any failures; Otherwise return OK.(Old callers can still use this method) 3.dav_unlock2() will also call do_dav_unlock() and return dav_error type in case of any failures; Otherwise return NULL. 4.dav/main/mod_dav.c: dav_method_unlock() will call dav_unlock2() and *handles* the error in case of any failures.
Updating Keyword as "PatchAvailable".