You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Julian Foad <ju...@btopenworld.com> on 2006/03/29 23:51:53 UTC
Specification of hook script interface
The specification of how Subversion handles a hook's output streams and error
status is in several places wrong or insufficient. The book says:
> start-commit
[...]
> If the program returns a non-zero exit value, the commit is stopped
> before the transaction is even created. If the hook program writes
> data to stderr, it will be marshalled back to the client.
That is misleading (as stderr will only be marshalled if the return code was
non-zero) and there are other errors and omissions in the descriptions of the
other hooks. A patch to fix these would be welcome. The writer of such a
patch should check what the current behaviour is in each case, and decide
whether the current behaviour is correct and should be documented or is wrong
in which case a correction could be discussed. Also check whether the
specification in the book agrees with that in the hook script templates that
are created by "svnadmin" when a new repository is created.
In the case of hooks that determine whether an operation may proceed
(start-commit, pre-commit, pre-revprop-change, pre-lock, pre-unlock), I will
state that the correct (desired) interface, as far as the hook script is
concerned, is this:
* (The working directory, environment variables and command-line arguments
should be specified but I'm not dealing with that here.)
* The state of stdin is undefined; the hook script must not try to read from
it. (I'm not sure that's true for all hooks: don't some of them supply
arguments via stdin?)
* Anything sent to stdout is discarded.
* If it returns exit code 0, the operation proceeds and anything sent to
stderr is discarded.
* If it returns a non-zero exit value, the operation is aborted and anything
sent to stderr is marshalled back to the client.
The interfaces for post-* hooks are similar but a bit different, with the
primary difference that stderr and the exit code are ignored.
(Software engineering technical note: The specification for implementing the
Subversion core code that calls the hooks could be at a slightly higher level
of promise, e.g. instead of merely satisfying the above promises that include
"stdin is undefined", we would do well to choose a particular well-defined
behaviour such as stdin being connected to /dev/null. This creates a tolerance
gap between what is required by one side of the interface and what is provided
by the other side, which is a recognised principle for increasing robustness
when two sides are developed independently. To do the equivalent for stdout
would require that we ask hook scripts not to write to it; unfortunately it is
too late to demand that though we could recommend it.)
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Specification of hook script interface
Posted by "Peter N. Lundblad" <pe...@famlundblad.se>.
Philip Martin writes:
> Index: subversion/libsvn_repos/hooks.c
> ===================================================================
> --- subversion/libsvn_repos/hooks.c (revision 19083)
> +++ subversion/libsvn_repos/hooks.c (working copy)
> @@ -690,7 +690,7 @@
> args[5] = action_string;
> args[6] = NULL;
>
> - SVN_ERR(run_hook_cmd("post-revprop-change", hook, args, FALSE,
> + SVN_ERR(run_hook_cmd("post-revprop-change", hook, args, TRUE,
> stdin_handle, pool));
>
> SVN_ERR(svn_io_file_close(stdin_handle, pool));
[...]
I'd say that's just an oversight. +1 to changing it.
I don't know how to propagate the error back through DAV, though...
//Peter
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Specification of hook script interface
Posted by Philip Martin <ph...@codematters.co.uk>.
Julian Foad <ju...@btopenworld.com> writes:
> The interfaces for post-* hooks are similar but a bit different, with
> the primary difference that stderr and the exit code are ignored.
That's no longer true for 1.4, it returns post-commit errors back to
the client.
Since I didn't know what the other post-* hooks did I decided to
investigate. The first thing I see is that libsvn_repos/hooks.c
throws away stderr for these hooks, so the default hook error message
"hook failed; no error output available" is used.
First ra_svn:
$ svn lock svn://localhost/repo/foo
../svn/subversion/libsvn_repos/fs-wrap.c:453: (apr_err=165008)
svn: Lock succeeded, but post-lock hook failed
../svn/subversion/libsvn_repos/hooks.c:144: (apr_err=165001)
svn: 'post-lock' hook failed; no error output available
$ svn unlock svn://localhost/repo/foo
../svn/subversion/libsvn_repos/fs-wrap.c:496: (apr_err=165009)
svn: Unlock succeeded, but post-unlock hook failed
../svn/subversion/libsvn_repos/hooks.c:144: (apr_err=165001)
svn: 'post-unlock' hook failed; no error output available
$ svn ps --revprop -r1 xxx yyy svn://localhost/repo/foo
../svn/subversion/libsvn_repos/hooks.c:144: (apr_err=165001)
svn: 'post-revprop-change' hook failed; no error output available
That's not so bad, the failure is reported but with the default hook
error message, it looks as if a minor tweak to hooks.c would cause the
hook's stderr to be returned.
Next ra_dav:
$ svn lock http://localhost/repo
../svn/subversion/libsvn_ra_dav/util.c:389: (apr_err=165008)
svn: Lock succeeded, but post-lock hook failed
$ svn lock http://localhost/repo
../svn/subversion/libsvn_ra_dav/util.c:296: (apr_err=175002)
svn: Unlock request failed: 500 Internal Server Error (http://localhost:8888)
$ svn ps --revprop -r1 xxx yyy http://localhost/repo
../svn/subversion/libsvn_ra_dav/fetch.c:1906: (apr_err=175002)
svn: DAV request failed; it's possible that the repository's pre-revprop-change hook either failed or is non-existent
../svn/subversion/libsvn_ra_dav/props.c:1104: (apr_err=175008)
svn: At least one property change failed; repository is unchanged
That's not so good. Lock gives an error, but it's not the default
hook error message. Unlock looks to have a real bug. The propset
gives a very misleading error message considering it's a
post-revprop-change failure after the change has been made.
Looking in the error log I see this for lock:
[Thu Mar 30 19:40:31 2006] [error] [client 127.0.0.1] Failed to create new lock. [500, #165008]
[Thu Mar 30 19:40:31 2006] [error] [client 127.0.0.1] Lock succeeded, but post-lock hook failed [500, #165008]
[Thu Mar 30 19:40:31 2006] [error] [client 127.0.0.1] 'post-lock' hook failed; no error output available [500, #165001]
nothing for unlock, and this for propset:
[Thu Mar 30 19:41:58 2006] [error] [client 127.0.0.1] 'post-revprop-change' hook failed; no error output available [500, #165001]
[Thu Mar 30 19:41:58 2006] [error] [client 127.0.0.1] Could not execute PROPPATCH. [500, #206]
[Thu Mar 30 19:41:58 2006] [error] [client 127.0.0.1] 'post-revprop-change' hook failed; no error output available [500, #165001]
There's the default hook error message, so once again a small tweak to
hooks.c should cause the hook's stderr to get reported. Does anyone
know why we are throwing away the hook's stderr? This is what I mean:
Index: subversion/libsvn_repos/hooks.c
===================================================================
--- subversion/libsvn_repos/hooks.c (revision 19083)
+++ subversion/libsvn_repos/hooks.c (working copy)
@@ -690,7 +690,7 @@
args[5] = action_string;
args[6] = NULL;
- SVN_ERR(run_hook_cmd("post-revprop-change", hook, args, FALSE,
+ SVN_ERR(run_hook_cmd("post-revprop-change", hook, args, TRUE,
stdin_handle, pool));
SVN_ERR(svn_io_file_close(stdin_handle, pool));
@@ -760,7 +760,7 @@
args[3] = NULL;
args[4] = NULL;
- SVN_ERR(run_hook_cmd("post-lock", hook, args, FALSE,
+ SVN_ERR(run_hook_cmd("post-lock", hook, args, TRUE,
stdin_handle, pool));
SVN_ERR(svn_io_file_close(stdin_handle, pool));
@@ -829,7 +829,7 @@
args[3] = NULL;
args[4] = NULL;
- SVN_ERR(run_hook_cmd("post-unlock", hook, args, FALSE,
+ SVN_ERR(run_hook_cmd("post-unlock", hook, args, TRUE,
stdin_handle, pool));
SVN_ERR(svn_io_file_close(stdin_handle, pool));
--
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org