You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by kf...@collab.net on 2004/07/06 12:17:02 UTC
Re: svn commit: r10148 - in trunk/subversion: libsvn_repos tests/clients/cmdline
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
Re: svn commit: r10148 - in trunk/subversion: libsvn_repos tests/clients/cmdline
Posted by Josh Pieper <jj...@pobox.com>.
kfogel@collab.net wrote:
> 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.
Sounds good to me, I just looked at the docstring for run_hook_cmd and
didn't actually look to see how it was implemented.
> 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.
No objections here.
-Josh
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org