[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: svn commit: r10148 - in trunk/subversion: libsvn_repos tests/clients/cmdline

From: <kfogel_at_collab.net>
Date: 2004-07-06 14:17:02 CEST

jpieper@tigris.org writes:
> Log:
> Issue #1904: Crash when deleting a revprop. This patch writes an
> empty stdin to the pre-revprop change hook when the revprop has been
> deleted. While we're at it, also implement passing the old revprop
> value to the post-revprop change hook and handle creating a revprop in
> the same way.
>
> * subversion/tests/clients/cmdline/prop_tests.py
> (revprop_change): Add a test for deleting a revprop.
>
> * subversion/libsvn_reops/hooks.c
> (svn_repos__hooks_pre_revprop_change): If a NULL previous value is
> passed, write an empty string to the pre-revprop change hook.
> (svn_repos__hooks_post_revprop_change): Write the old value of the
> revprop to the hook on stdin in the same fashion as for the
> pre-revprop change hook.
>
> * subversion/libsvn_repos/repos.h
> (svn_repos__hooks_pre_revprop_change,
> svn_repos__hooks_post_revprop_change): Clarify the docstrings to
> clearly show which hook gets the old value and which gets the new
> value and what happens in the case of a revprop deletion or
> creation.

Whew, thanks for fixing that pre- and post-revprop change mess, Josh!

A small implementation nit: When the value is NULL, perhaps we
shouldn't bother to create a tempfile containing only the empty
string? Instead, we should just pass NULL, and not create a tempfile
at all.

Of course, the possibility of doing it this way was obscured by
run_hook_cmd() not explicitly documenting that 'stdin_handle' can be
NULL. However, many callers were already passing NULL, and one can
trace it through to svn_io_run_cmd() to see that it's okay.

I'd like to commit the patch below, please tell me what you think. It
passes prop_tests 11, and some other basic tests. I haven't run the
full test suite on it yet, but will before committing.

--------------------8-<-------cut-here---------8-<-----------------------

Follow up to r10148: Don't bother to create tempfiles for empty stdin,
just pass null stdin instead.

* subversion/libsvn_repos/hooks.c
  (run_hook_cmd): Document that 'stdin_handle' parameter can be null.
  (svn_repos__hooks_pre_revprop_change,
   svn_repos__hooks_post_revprop_change): Adjust to use null
    stdin_handle if new_value / old_value is null.

Index: subversion/libsvn_repos/hooks.c
===================================================================
--- subversion/libsvn_repos/hooks.c (revision 10148)
+++ subversion/libsvn_repos/hooks.c (working copy)
@@ -41,8 +41,12 @@
 /* NAME, CMD and ARGS are the name, path to and arguments for the hook
    program that is to be run. If CHECK_EXITCODE is TRUE then the hook's
    exit status will be checked, and if an error occurred the hook's stderr
- output will be added to the returned error. If CHECK_EXITCODE is FALSE
- the hook's exit status will be ignored. */
+ output will be added to the returned error.
+
+ If CHECK_EXITCODE is FALSE the hook's exit status will be ignored.
+
+ If STDIN_HANDLE is non-null, pass it as the hook's stdin, else pass
+ no stdin to the hook. */
 static svn_error_t *
 run_hook_cmd (const char *name,
               const char *cmd,
@@ -258,14 +262,11 @@
   if ((hook = check_hook_cmd (hook, pool)))
     {
       const char *args[6];
- apr_file_t *stdin_handle;
+ apr_file_t *stdin_handle = NULL;
 
       /* Pass the new value as stdin to hook */
       if (new_value)
         SVN_ERR (create_temp_file (&stdin_handle, new_value, pool));
- else
- SVN_ERR (create_temp_file (&stdin_handle, svn_string_create ("", pool),
- pool));
 
       args[0] = hook;
       args[1] = svn_repos_path (repos, pool);
@@ -277,7 +278,8 @@
       SVN_ERR (run_hook_cmd ("pre-revprop-change", hook, args, TRUE,
                              stdin_handle, pool));
 
- SVN_ERR (svn_io_file_close (stdin_handle, pool));
+ if (stdin_handle)
+ SVN_ERR (svn_io_file_close (stdin_handle, pool));
     }
   else
     {
@@ -309,14 +311,11 @@
   if ((hook = check_hook_cmd (hook, pool)))
     {
       const char *args[6];
- apr_file_t *stdin_handle;
+ apr_file_t *stdin_handle = NULL;
 
       /* Pass the new value as stdin to hook */
       if (old_value)
         SVN_ERR (create_temp_file (&stdin_handle, old_value, pool));
- else
- SVN_ERR (create_temp_file (&stdin_handle, svn_string_create ("", pool),
- pool));
 
       args[0] = hook;
       args[1] = svn_repos_path (repos, pool);
@@ -328,7 +327,8 @@
       SVN_ERR (run_hook_cmd ("post-revprop-change", hook, args, FALSE,
                              stdin_handle, pool));
       
- SVN_ERR (svn_io_file_close (stdin_handle, pool));
+ if (stdin_handle)
+ SVN_ERR (svn_io_file_close (stdin_handle, pool));
     }
 
   return SVN_NO_ERROR;

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Tue Jul 6 17:40:31 2004

This is an archived mail posted to the Subversion Dev mailing list.