You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Gustavo Niemeyer <ni...@conectiva.com> on 2002/08/30 21:30:36 UTC

[PATCH] show hook errors on client side

Fix issue 443:

* hooks.c
  (run_cmd_with_output) Function renamed to run_hook_cmd, and changed
  its interface. Now command error checking is done inside the function.
  Also, the stderr from the command run is appended to the error msg,
  so that remote clients can see the error as well.
  
  (run_start_commit_hook,run_pre_commit_hook,run_post_commit_hook):
  Changed to use new run_hook_cmd interface.

  (run_start_commit_hook) Fixed behavior. Now considers hook exit code,
  as stated in the documentation.


--- subversion/subversion/libsvn_repos/hooks.c.reporterror	2002-08-26 11:16:20.000000000 -0300
+++ subversion/subversion/libsvn_repos/hooks.c	2002-08-30 18:13:20.000000000 -0300
@@ -38,29 +38,74 @@
 /*** Hook drivers. ***/
 
 static svn_error_t *
-run_cmd_with_output (const char *cmd,
-                     const char **args,
-                     int *exitcode,
-                     apr_exit_why_e *exitwhy,
-                     apr_pool_t *pool)
+run_hook_cmd (const char *name,
+              const char *cmd,
+              const char **args,
+              svn_boolean_t check_exitcode,
+              apr_pool_t *pool)
 {
-  apr_file_t *outhandle, *errhandle;
+  apr_file_t *read_outhandle, *write_outhandle;
+  apr_file_t *read_errhandle, *write_errhandle;
   apr_status_t apr_err;
+  svn_error_t *err;
+  int exitcode;
+  apr_exit_why_e exitwhy;
+
+  /* Create pipes to access stdout and stderr of the child. */
+  apr_err = apr_file_pipe_create (&read_outhandle, &write_outhandle, pool);
+  if (apr_err)
+    return svn_error_create 
+      (apr_err, 0, NULL, pool,
+       "run_hook_cmd: can't create pipe for stdout");
+  apr_err = apr_file_pipe_create(&read_errhandle, &write_errhandle, pool);
+  if (apr_err)
+    return svn_error_create 
+      (apr_err, 0, NULL, pool,
+       "run_hook_cmd: can't create pipe for stderr");
   
-  /* Get an apr_file_t representing stdout and stderr. */
-  apr_err = apr_file_open_stdout (&outhandle, pool);
+  err = svn_io_run_cmd (".", cmd, args, &exitcode, &exitwhy, FALSE,
+                            NULL, write_outhandle, write_errhandle, pool);
+
+  /* This seems to be done automatically if we pass the third parameter
+     of apr_procattr_child_in/out_set(), but svn_io_run_cmd()'s interface
+     is currently not supporting these parameters. */
+  apr_err = apr_file_close(write_outhandle);
   if (apr_err)
     return svn_error_create 
       (apr_err, 0, NULL, pool,
-       "run_cmd_with_output: can't open handle to stdout");
-  apr_err = apr_file_open_stderr (&errhandle, pool);
+       "run_hook_cmd: can't close parent side stdout pipe");
+  apr_err = apr_file_close(write_errhandle);
   if (apr_err)
     return svn_error_create 
       (apr_err, 0, NULL, pool,
-       "run_cmd_with_output: can't open handle to stderr");
+       "run_hook_cmd: can't close parent side stderr pipe");
+
+  /* Function failed. */
+  if (err)
+    {
+      return svn_error_createf 
+        (SVN_ERR_REPOS_HOOK_FAILURE, 0, err, pool,
+         "run_hook_cmd: error running cmd `%s'", cmd);
+    }
+
+  if (check_exitcode)
+    {
+      /* Command failed. */
+      if (! APR_PROC_CHECK_EXIT (exitwhy) || exitcode != 0)
+        {
+          svn_stringbuf_t *error = svn_stringbuf_create ("", pool);
+
+          /* Read the file's contents into a stringbuf, allocated in POOL. */
+          SVN_ERR (svn_string_from_aprfile (&error, read_errhandle, pool));
 
-  return svn_io_run_cmd (".", cmd, args, exitcode, exitwhy, FALSE,
-                         NULL, outhandle, errhandle, pool);
+          return svn_error_createf
+              (SVN_ERR_REPOS_HOOK_FAILURE, 0, err, pool,
+               "%s hook return non-zero status.  Aborting txn.\n"
+               "Command output:\n%s", name, error->data);
+        }
+    }
+
+  return SVN_NO_ERROR;
 }
 
 
@@ -85,12 +130,7 @@
       args[2] = user;
       args[3] = NULL;
 
-      if ((err = run_cmd_with_output (hook, args, NULL, NULL, pool)))
-        {
-          return svn_error_createf 
-            (SVN_ERR_REPOS_HOOK_FAILURE, 0, err, pool,
-             "run_start_commit_hook: error running cmd `%s'", hook);
-        }
+      SVN_ERR (run_hook_cmd ("start-commit", hook, args, TRUE, pool));
     }
 
   return SVN_NO_ERROR;
@@ -110,9 +150,6 @@
   if ((! svn_io_check_path (hook, &kind, pool)) 
       && (kind == svn_node_file))
     {
-      svn_error_t *err;
-      int exitcode;
-      apr_exit_why_e exitwhy;
       const char *args[4];
 
       args[0] = hook;
@@ -120,18 +157,7 @@
       args[2] = txn_name;
       args[3] = NULL;
 
-      if ((err = run_cmd_with_output (hook, args, &exitcode, &exitwhy, pool)))
-        {
-          return svn_error_createf 
-            (SVN_ERR_REPOS_HOOK_FAILURE, 0, err, pool,
-             "run_pre_commit_hook: error running cmd `%s'", hook);
-        }
-      if (! APR_PROC_CHECK_EXIT (exitwhy) || exitcode != 0)
-        {
-          return svn_error_create
-              (SVN_ERR_REPOS_HOOK_FAILURE, 0, err, pool,
-               "pre-commit hook return non-zero status.  Aborting txn.");
-        }
+      SVN_ERR (run_hook_cmd ("pre-commit", hook, args, TRUE, pool));
     }
 
   return SVN_NO_ERROR;
@@ -151,7 +177,6 @@
   if ((! svn_io_check_path (hook, &kind, pool)) 
       && (kind == svn_node_file))
     {
-      svn_error_t *err;
       const char *args[4];
 
       args[0] = hook;
@@ -159,12 +184,7 @@
       args[2] = apr_psprintf (pool, "%" SVN_REVNUM_T_FMT, rev);
       args[3] = NULL;
 
-      if ((err = run_cmd_with_output (hook, args, NULL, NULL, pool)))
-        {
-          return svn_error_createf 
-            (SVN_ERR_REPOS_HOOK_FAILURE, 0, err, pool,
-             "run_post_commit_hook: error running cmd `%s'", hook);
-        }
+      SVN_ERR (run_hook_cmd ("post-commit", hook, args, FALSE, pool));
     }
 
   return SVN_NO_ERROR;


-- 
Gustavo Niemeyer

[ 2AAC 7928 0FBF 0299 5EB5  60E2 2253 B29A 6664 3A0C ]

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: [PATCH] show hook errors on client side

Posted by Philip Martin <ph...@codematters.co.uk>.
Gustavo Niemeyer <ni...@conectiva.com> writes:

> Fix issue 443:

Well, some of it anyway :)  I've applied a modified version of this
patch in rev 3144.  Thanks!

-- 
Philip Martin

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: [PATCH] show hook errors on client side

Posted by Philip Martin <ph...@codematters.co.uk>.
Branko Čibej <br...@xbc.nu> writes:

> Philip Martin wrote:
> 
> >Kevin Pilch-Bisson <ke...@pilch-bisson.net> writes:
> >
> >
> 
> >> Right, subversion needs to have some mechanism to output warnings
> >> basically.  A way for a lib to call back to the client with the
> >> warning, and then continue it's operation.
> 
> >>
> 
> >
> >Gustavo Niemeyer made the same point elsewhere in this thread.  Note
> >that in this case the "lib" is part of httpd, and the client can be a
> >separate process on a remote machine.  So it's not a conventional
> >callback.
> >
> I think what we need is a way to send a multistatus response. I'm sure
> mod_dav(_svn) can send those, becasue I've seen them coming from
> there; and neon can handle them.
> 
> 
> The only question, then, is where to generate such a beastie.

Yes, there are really two problems here.  First, the commit has to
report the successful repository operation, including the new revision
etc., as well as the failure of the post-commit hook.  Multistatus may
be the correct solution here, Greg also mentioned multistatus in issue
443.  Second, the client needs a way to report the hook failure while
continuing to do post-commit processing.

It occurs to me that the new warning callback could simply be
implemented as part of the existing notify callback.  If we add
another symbol to the svn_wc_notify_action_t enum, something like
svn_wc_notify_warning, and add another argument (probably an
svn_error_t*) to the notify callback to pass the warning data, then we
have all we need.  Rather than simply extending the notify callback
argument list, it may be better to introduce a union, so that only
those items relevant to a particular action need to be set.

-- 
Philip Martin

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: [PATCH] show hook errors on client side

Posted by Branko Čibej <br...@xbc.nu>.
Philip Martin wrote:

>Kevin Pilch-Bisson <ke...@pilch-bisson.net> writes:
>
>  
>
>>Right, subversion needs to have some mechanism to output warnings basically.  
>>A way for a lib to call back to the client with the warning, and then continue 
>>it's operation.
>>    
>>
>
>Gustavo Niemeyer made the same point elsewhere in this thread.  Note
>that in this case the "lib" is part of httpd, and the client can be a
>separate process on a remote machine.  So it's not a conventional
>callback.
>
I think what we need is a way to send a multistatus response. I'm sure 
mod_dav(_svn) can send those, becasue I've seen them coming from there; 
and neon can handle them.

The only question, then, is where to generate such a beastie.

-- 
Brane Čibej   <br...@xbc.nu>   http://www.xbc.nu/brane/


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: [PATCH] show hook errors on client side

Posted by Philip Martin <ph...@codematters.co.uk>.
Kevin Pilch-Bisson <ke...@pilch-bisson.net> writes:

> Right, subversion needs to have some mechanism to output warnings basically.  
> A way for a lib to call back to the client with the warning, and then continue 
> it's operation.

Gustavo Niemeyer made the same point elsewhere in this thread.  Note
that in this case the "lib" is part of httpd, and the client can be a
separate process on a remote machine.  So it's not a conventional
callback.

-- 
Philip Martin

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: [PATCH] show hook errors on client side

Posted by Kevin Pilch-Bisson <ke...@pilch-bisson.net>.
Quoting Karl Fogel <kf...@newton.ch.collab.net>:

> Philip Martin <ph...@codematters.co.uk> writes:
> > The client's post-commit processing depends on the DAV merge-response
> > message returned after a successful commit.  If the repos library's
> > svn_repos_fs_commit_txn returns the post-commit hook error then
> > ra_dav's dav_svn_merge will pass that error back to the client instead
> > of the merge-response.  Although the client can recognize the error it
> > won't have the merge-response that is required to carry on and do
> > post-commit processing.  So, it is not sufficient for the client to
> > recognize a post-commit hook error.  We need a something more, either
> > the ability to return an error independently from the merge-response,
> > or the ability to combine the error with the merge-response.
> 
> Oh, okay -- I meant "get a non-fatal error along with the merge
> response", not "get a non-fatal error instead of a merge response",
> sorry I didn't make that clearer.
> 
> That we must currently choose between the two is a bug.
> 
Right, subversion needs to have some mechanism to output warnings basically.  
A way for a lib to call back to the client with the warning, and then continue 
it's operation.


--
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Kevin Pilch-Bisson
kevin@pilch-bisson.net
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: [PATCH] show hook errors on client side

Posted by Karl Fogel <kf...@newton.ch.collab.net>.
Philip Martin <ph...@codematters.co.uk> writes:
> The client's post-commit processing depends on the DAV merge-response
> message returned after a successful commit.  If the repos library's
> svn_repos_fs_commit_txn returns the post-commit hook error then
> ra_dav's dav_svn_merge will pass that error back to the client instead
> of the merge-response.  Although the client can recognize the error it
> won't have the merge-response that is required to carry on and do
> post-commit processing.  So, it is not sufficient for the client to
> recognize a post-commit hook error.  We need a something more, either
> the ability to return an error independently from the merge-response,
> or the ability to combine the error with the merge-response.

Oh, okay -- I meant "get a non-fatal error along with the merge
response", not "get a non-fatal error instead of a merge response",
sorry I didn't make that clearer.

That we must currently choose between the two is a bug.

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: [PATCH] show hook errors on client side

Posted by Philip Martin <ph...@codematters.co.uk>.
Karl Fogel <kf...@newton.ch.collab.net> writes:

> Philip Martin <ph...@codematters.co.uk> writes:
> > I tried passing SVN_ERR_REPOS_POST_COMMIT_FAILURE and the error
> > passing works.  However, that doesn't really solve the problem, if the
> > client is going to carry out its post-commit proccessing it needs to
> > get the normal successful commit response, with the new revision
> > number, etc.  Simply having the client recognize a post-commit hook
> > error, print it and continue, is not sufficient.
> 
> I didn't understand why your last sentence above is true (?).
> 
> Granted that non-fatal errors are difficult to implement in the
> current scheme of things, but let's assume for the moment that that
> bit of architectural lossage were solved: in that case, it should be
> sufficient to have the client recognize a post-commit hook error,
> print the error as an informational warning, and continue with the
> normal post-commit processing.  What would be bad about that?

Nothing is bad, it's what I suggested.  However it is not sufficient.

The client's post-commit processing depends on the DAV merge-response
message returned after a successful commit.  If the repos library's
svn_repos_fs_commit_txn returns the post-commit hook error then
ra_dav's dav_svn_merge will pass that error back to the client instead
of the merge-response.  Although the client can recognize the error it
won't have the merge-response that is required to carry on and do
post-commit processing.  So, it is not sufficient for the client to
recognize a post-commit hook error.  We need a something more, either
the ability to return an error independently from the merge-response,
or the ability to combine the error with the merge-response.

-- 
Philip Martin

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: [PATCH] show hook errors on client side

Posted by Karl Fogel <kf...@newton.ch.collab.net>.
Philip Martin <ph...@codematters.co.uk> writes:
> I tried passing SVN_ERR_REPOS_POST_COMMIT_FAILURE and the error
> passing works.  However, that doesn't really solve the problem, if the
> client is going to carry out its post-commit proccessing it needs to
> get the normal successful commit response, with the new revision
> number, etc.  Simply having the client recognize a post-commit hook
> error, print it and continue, is not sufficient.

I didn't understand why your last sentence above is true (?).

Granted that non-fatal errors are difficult to implement in the
current scheme of things, but let's assume for the moment that that
bit of architectural lossage were solved: in that case, it should be
sufficient to have the client recognize a post-commit hook error,
print the error as an informational warning, and continue with the
normal post-commit processing.  What would be bad about that?

-K



---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: [PATCH] show hook errors on client side

Posted by Philip Martin <ph...@codematters.co.uk>.
Gustavo Niemeyer <ni...@conectiva.com> writes:

[returning post-commit hook error to the client]

> > > I must confess I'm not sure how to do that. Is it possible to use
> > 
> > I'm hoping it's quite simple :)
> 
> :-)

It's not :-(

> > Does dav_svn_convert_err get in the way?  If we have the post-commit
> > hook failure generate a specific SVN_ERR_REPOS_POST_COMMIT_FAILURE
> > error, say in svn_repos_fs_commit_txn, will ra_dav pass this error
> > back to the client?
> 
> Well, I think that the main intention behind issuing the error is to
> let the user know what's going on, so it'd be nice to pass it across
> mod_dav.
> 
> > If it does, then when the SVN_ERR_REPOS_POST_COMMIT_FAILURE gets to
> > svn_client_commit it will be returned by svn_client__do_commit. At
> > this stage the client can call svn_handle_error and then continue.
> 
> I think that the best way to do that would be to use a more general
> approach, not requiring errors to be passed around. For now, it's ok,
> since those messages are really bounded to a fatal error.

Well, a post-commit hook failure is not really fatal, the commit has
succeeded!

I tried passing SVN_ERR_REPOS_POST_COMMIT_FAILURE and the error
passing works.  However, that doesn't really solve the problem, if the
client is going to carry out its post-commit proccessing it needs to
get the normal successful commit response, with the new revision
number, etc.  Simply having the client recognize a post-commit hook
error, print it and continue, is not sufficient.

I wonder if post-commit hook errors should be logged by Apache rather
than sent back to the client?  That's not a solution for ra_local.

-- 
Philip Martin

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: [PATCH] show hook errors on client side

Posted by Gustavo Niemeyer <ni...@conectiva.com>.
> > I must confess I'm not sure how to do that. Is it possible to use
> 
> I'm hoping it's quite simple :)

:-)

> > dav_svn_convert_err() in a non-fatal way?
> 
> Does dav_svn_convert_err get in the way?  If we have the post-commit
> hook failure generate a specific SVN_ERR_REPOS_POST_COMMIT_FAILURE
> error, say in svn_repos_fs_commit_txn, will ra_dav pass this error
> back to the client?

Well, I think that the main intention behind issuing the error is to
let the user know what's going on, so it'd be nice to pass it across
mod_dav.

> If it does, then when the SVN_ERR_REPOS_POST_COMMIT_FAILURE gets to
> svn_client_commit it will be returned by svn_client__do_commit. At
> this stage the client can call svn_handle_error and then continue.

I think that the best way to do that would be to use a more general
approach, not requiring errors to be passed around. For now, it's ok,
since those messages are really bounded to a fatal error. But in the
future, I'd like to see a different scheme which allowed messages to
be sent to the client even when no errors happened at all, allowing
hooks to print messages like "Hey, warn here! You've just committed
something strange, but I'll go on nevertheless.". I don't know
subversion internals well enough to tell which would be the right
way to pass such hook output to the client.

> I *think* that might work, but I haven't tried it.  Care to give it a
> go?

Unfortunately, I must finish some stuff here and don't have time to work
on it right now. :-( OTOH, I really appreciate the work you're doing in
subversion, and would be a pleasure to contribute with this or other
issues once I have time.

> Even without such an enhancement, I think your patch is an improvement.

Thank you!

We're using it here in a repository system I'm working on. I wouldn't be
able to use the beautiful access control offered by the hook scheme if
the committing user didn't know what was going on.

-- 
Gustavo Niemeyer

[ 2AAC 7928 0FBF 0299 5EB5  60E2 2253 B29A 6664 3A0C ]

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: [PATCH] show hook errors on client side

Posted by Philip Martin <ph...@codematters.co.uk>.
Gustavo Niemeyer <ni...@conectiva.com> writes:

> > > +      SVN_ERR (run_hook_cmd ("post-commit", hook, args, FALSE, pool));
> > 
> > Why did you choose to ignore post-commit errors?  I suppose it's so
> > that the client carries out the post-commit processing and doesn't
> > abort.  I think it would be better to define a specific error for this
> 
> Indeed. It wasn't checking errors before, so I've just used the same
> principles.
> 
> > case, return that instead of SVN_ERR_REPOS_HOOK_FAILURE, and get the
> > client to detect it, print the error message (svn_handle_error with
> > FATAL set FALSE probably) and continue.
> 
> I must confess I'm not sure how to do that. Is it possible to use

I'm hoping it's quite simple :)

> dav_svn_convert_err() in a non-fatal way?

Does dav_svn_convert_err get in the way?  If we have the post-commit
hook failure generate a specific SVN_ERR_REPOS_POST_COMMIT_FAILURE
error, say in svn_repos_fs_commit_txn, will ra_dav pass this error
back to the client?

If it does, then when the SVN_ERR_REPOS_POST_COMMIT_FAILURE gets to
svn_client_commit it will be returned by svn_client__do_commit. At
this stage the client can call svn_handle_error and then continue.

I *think* that might work, but I haven't tried it.  Care to give it a
go?

Even without such an enhancement, I think your patch is an improvement.


-- 
Philip Martin

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: [PATCH] show hook errors on client side

Posted by Gustavo Niemeyer <ni...@conectiva.com>.
Hi Philip!

> > +      SVN_ERR (run_hook_cmd ("post-commit", hook, args, FALSE, pool));
> 
> Why did you choose to ignore post-commit errors?  I suppose it's so
> that the client carries out the post-commit processing and doesn't
> abort.  I think it would be better to define a specific error for this

Indeed. It wasn't checking errors before, so I've just used the same
principles.

> case, return that instead of SVN_ERR_REPOS_HOOK_FAILURE, and get the
> client to detect it, print the error message (svn_handle_error with
> FATAL set FALSE probably) and continue.

I must confess I'm not sure how to do that. Is it possible to use
dav_svn_convert_err() in a non-fatal way?

-- 
Gustavo Niemeyer

[ 2AAC 7928 0FBF 0299 5EB5  60E2 2253 B29A 6664 3A0C ]

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: [PATCH] show hook errors on client side

Posted by Philip Martin <ph...@codematters.co.uk>.
Gustavo Niemeyer <ni...@conectiva.com> writes:

> Fix issue 443:

Great!

> 
> * hooks.c

* subversion/libsvn_repos/hooks.c

>   (run_cmd_with_output) Function renamed to run_hook_cmd, and changed
>   its interface. Now command error checking is done inside the function.
>   Also, the stderr from the command run is appended to the error msg,
>   so that remote clients can see the error as well.
>   
>   (run_start_commit_hook,run_pre_commit_hook,run_post_commit_hook):
>   Changed to use new run_hook_cmd interface.
> 
>   (run_start_commit_hook) Fixed behavior. Now considers hook exit code,
>   as stated in the documentation.
> 
> 
> --- subversion/subversion/libsvn_repos/hooks.c.reporterror	2002-08-26 11:16:20.000000000 -0300
> +++ subversion/subversion/libsvn_repos/hooks.c	2002-08-30 18:13:20.000000000 -0300
> @@ -38,29 +38,74 @@
>  /*** Hook drivers. ***/
>  
>  static svn_error_t *
> -run_cmd_with_output (const char *cmd,
> -                     const char **args,
> -                     int *exitcode,
> -                     apr_exit_why_e *exitwhy,
> -                     apr_pool_t *pool)
> +run_hook_cmd (const char *name,
> +              const char *cmd,
> +              const char **args,
> +              svn_boolean_t check_exitcode,
> +              apr_pool_t *pool)
>  {

[snip]

> +  if (check_exitcode)
> +    {
> +      /* Command failed. */
> +      if (! APR_PROC_CHECK_EXIT (exitwhy) || exitcode != 0)
> +        {
> +          svn_stringbuf_t *error = svn_stringbuf_create ("", pool);
> +
> +          /* Read the file's contents into a stringbuf, allocated in POOL. */
> +          SVN_ERR (svn_string_from_aprfile (&error, read_errhandle, pool));

[snip]

> @@ -159,12 +184,7 @@
>        args[2] = apr_psprintf (pool, "%" SVN_REVNUM_T_FMT, rev);
>        args[3] = NULL;
>  
> -      if ((err = run_cmd_with_output (hook, args, NULL, NULL, pool)))
> -        {
> -          return svn_error_createf 
> -            (SVN_ERR_REPOS_HOOK_FAILURE, 0, err, pool,
> -             "run_post_commit_hook: error running cmd `%s'", hook);
> -        }
> +      SVN_ERR (run_hook_cmd ("post-commit", hook, args, FALSE, pool));

Why did you choose to ignore post-commit errors?  I suppose it's so
that the client carries out the post-commit processing and doesn't
abort.  I think it would be better to define a specific error for this
case, return that instead of SVN_ERR_REPOS_HOOK_FAILURE, and get the
client to detect it, print the error message (svn_handle_error with
FATAL set FALSE probably) and continue.

>      }
>  
>    return SVN_NO_ERROR;

-- 
Philip Martin

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org