"Chia-liang Kao" <clkao_at_clkao.org> writes:
> [[[
> Allow pre-lock hook to be able to specify lock-token to be used with
> stdout output.
>
> **** NOTE ****
>
> This will cause existing pre-lock hook that prints to stdout to be
> incompatible with this change, where the output were previously
> discarded.
>
> This should be mentioned in the release notes.
>
> * subversion/libsvn_repos/fs-wrap.c
> (svn_repos_fs_lock): allow returned new_token from pre-lock hook.
>
> * subversion/libsvn_repos/repos.h
> (svn_repos__hooks_pre_lock): now has additional returned value for token.
>
> * subversion/libsvn_repos/hooks.c
> (run_hook_cmd): allow stdout of hook to be captured and returned.
> Update all callers.
> (svn_repos__hooks_pre_lock): get token from run_hook_cmd.
>
> * subversion/libsvn_repos/repos.c: Document the pre-lock feature for
> token from stdout.
> ]]]
Log message looks good.
> === subversion/libsvn_repos/fs-wrap.c
> ==================================================================
> --- subversion/libsvn_repos/fs-wrap.c (revision 32395)
> +++ subversion/libsvn_repos/fs-wrap.c (local)
> @@ -447,7 +447,7 @@
> {
> svn_error_t *err;
> svn_fs_access_t *access_ctx = NULL;
> - const char *username = NULL;
> + const char *username = NULL, *new_token = NULL;
> apr_array_header_t *paths;
>
> /* Setup an array of paths in anticipation of the ra layers handling
> @@ -467,9 +467,9 @@
>
> /* Run pre-lock hook. This could throw error, preventing
> svn_fs_lock() from happening. */
> - SVN_ERR(svn_repos__hooks_pre_lock(repos, path, username, comment,
> + SVN_ERR(svn_repos__hooks_pre_lock(repos, &new_token, path, username, comment,
> steal_lock, pool));
> -
> + token = (new_token && *new_token) ? new_token : token;
> /* Lock. */
> SVN_ERR(svn_fs_lock(lock, repos->fs, path, token, comment, is_dav_comment,
> expiration_date, current_rev, steal_lock, pool));
The code here looks fine. But shouldn't the doc string for
svn_repos_fs_lock() mention that this can happen now? Currently, that
function is documented to be "Like svn_fs_lock()..." with a few
differences. Now there's one more difference: the token that is
actually used can be different from the token the caller passed!
> === subversion/libsvn_repos/hooks.c
> ==================================================================
> --- subversion/libsvn_repos/hooks.c (revision 32395)
> +++ subversion/libsvn_repos/hooks.c (local)
> @@ -156,15 +156,20 @@
> the returned error.
>
> If STDIN_HANDLE is non-null, pass it as the hook's stdin, else pass
> - no stdin to the hook. */
> + no stdin to the hook.
> +
> + If RESULT is non-null, capture the stdout of the hook and return it.
> +*/
"If RESULT is non-null, set *RESULT to the stdout of the hook or to
the empty string if the hook generates no output on stdout."
But shouldn't result be a stringbuf or some other type that can store
arbitrary binary data? In general, there's no guarantee that a hook's
output will be appropriate for a 'char *' result. Sure, that will be
the case with the particular hook involved in this change, but
run_hook_cmd() is a generic interface to all the hooks.
> static svn_error_t *
> -run_hook_cmd(const char *name,
> +run_hook_cmd(const char **result,
> + const char *name,
> const char *cmd,
> const char **args,
> apr_file_t *stdin_handle,
> apr_pool_t *pool)
> {
> apr_file_t *read_errhandle, *write_errhandle, *null_handle;
> + apr_file_t *read_outhandle, *write_outhandle;
> apr_status_t apr_err;
> svn_error_t *err;
> apr_proc_t cmd_proc;
> @@ -194,16 +199,39 @@
> (apr_err, _("Can't make pipe write handle non-inherited for hook '%s'"),
> cmd);
>
> + if (result)
> + {
> + /* Create a pipe to access stdout of the child. */
> + apr_err = apr_file_pipe_create(&read_outhandle, &write_outhandle, pool);
> + if (apr_err)
> + return svn_error_wrap_apr
> + (apr_err, _("Can't create pipe for hook '%s'"), cmd);
>
> - /* Redirect stdout to the null device */
> - apr_err = apr_file_open(&null_handle, SVN_NULL_DEVICE_NAME, APR_WRITE,
> - APR_OS_DEFAULT, pool);
> - if (apr_err)
> - return svn_error_wrap_apr
> - (apr_err, _("Can't create null stdout for hook '%s'"), cmd);
> + apr_err = apr_file_inherit_unset(read_outhandle);
> + if (apr_err)
> + return svn_error_wrap_apr
> + (apr_err,
> + _("Can't make pipe read handle non-inherited for hook '%s'"), cmd);
>
> + apr_err = apr_file_inherit_unset(write_outhandle);
> + if (apr_err)
> + return svn_error_wrap_apr
> + (apr_err,
> + _("Can't make pipe write handle non-inherited for hook '%s'"), cmd);
> + }
> + else
> + {
> + /* Redirect stdout to the null device */
> + apr_err = apr_file_open(&null_handle, SVN_NULL_DEVICE_NAME, APR_WRITE,
> + APR_OS_DEFAULT, pool);
> + if (apr_err)
> + return svn_error_wrap_apr
> + (apr_err, _("Can't create null stdout for hook '%s'"), cmd);
> + }
> +
> err = svn_io_start_cmd(&cmd_proc, ".", cmd, args, FALSE,
> - stdin_handle, null_handle, write_errhandle, pool);
> + stdin_handle, result ? write_outhandle : null_handle,
> + 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 does
> @@ -214,6 +242,14 @@
> return svn_error_wrap_apr
> (apr_err, _("Error closing write end of stderr pipe"));
>
> + if (result)
> + {
> + apr_err = apr_file_close(write_outhandle);
> + if (!err && apr_err)
> + return svn_error_wrap_apr
> + (apr_err, _("Error closing write end of stderr pipe"));
> + }
> +
> if (err)
> {
> err = svn_error_createf
> @@ -232,10 +268,24 @@
> return svn_error_wrap_apr
> (apr_err, _("Error closing read end of stderr pipe"));
>
> - apr_err = apr_file_close(null_handle);
> - if (!err && apr_err)
> - return svn_error_wrap_apr(apr_err, _("Error closing null file"));
> + if (result)
> + {
> + svn_stringbuf_t *native_stdout;
> + SVN_ERR(svn_stringbuf_from_aprfile(&native_stdout, read_outhandle, pool));
> + apr_err = apr_file_close(read_outhandle);
> + if (!err && apr_err)
> + return svn_error_wrap_apr
> + (apr_err, _("Error closing read end of stderr pipe"));
>
> + *result = (svn_string_create_from_buf(native_stdout, pool))->data;
> + }
> + else
> + {
> + apr_err = apr_file_close(null_handle);
> + if (!err && apr_err)
> + return svn_error_wrap_apr(apr_err, _("Error closing null file"));
> + }
> +
> return err;
> }
That all looks good to me (not very experienced with in/out redirection
via APR interfaces, but nothing jumps out at me).
> @@ -258,7 +308,6 @@
> return SVN_NO_ERROR;
> }
>
> -
> /* Check if the HOOK program exists and is a file or a symbolic link, using
> POOL for temporary allocations.
Spurious change, but hey, I can't claim that it really distracted me
that much :-)
> @@ -541,6 +589,7 @@
>
> svn_error_t *
> svn_repos__hooks_pre_lock(svn_repos_t *repos,
> + const char **token,
> const char *path,
> const char *username,
> const char *comment,
> @@ -566,7 +615,8 @@
> args[5] = steal_lock ? "1" : "0";
> args[6] = NULL;
>
> - SVN_ERR(run_hook_cmd(SVN_REPOS__HOOK_PRE_LOCK, hook, args, NULL, pool));
> + SVN_ERR(run_hook_cmd(token, SVN_REPOS__HOOK_PRE_LOCK, hook, args, NULL,
> + pool));
> }
>
> return SVN_NO_ERROR;
> === subversion/libsvn_repos/repos.c
> ==================================================================
> --- subversion/libsvn_repos/repos.c (revision 32395)
> +++ subversion/libsvn_repos/repos.c (local)
> @@ -540,6 +540,11 @@
> "# [4] COMMENT (the comment of the lock)" NL
> "# [5] STEAL-LOCK (1 if the user is trying to steal the lock, else 0)" NL
> "#" NL
> +"# If the hook program outputs anything in stdout, the output string will" NL
> +"# be used as the lock token for this lock operation. If you choose to use" NL
> +"# this feature, you must guarantee the tokens generated are unique each." NL
> +"# time." NL
> +"#" NL
We should say unique across what space. Globally unique?
> === subversion/libsvn_repos/repos.h
> ==================================================================
> --- subversion/libsvn_repos/repos.h (revision 32395)
> +++ subversion/libsvn_repos/repos.h (local)
> @@ -223,10 +223,12 @@
> PATH is the path being locked, USERNAME is the person doing it,
> COMMENT is the comment of the lock, and is treated as an empty
> string when NULL is given. STEAL-LOCK is a flag if the user is
> - stealing the lock. */
> + stealing the lock. If TOKEN is returned by the hook, it will be
> + used as the token for this lock. */
>
> svn_error_t *
> svn_repos__hooks_pre_lock(svn_repos_t *repos,
> + const char **token,
> const char *path,
> const char *username,
> const char *comment,
This doc string doesn't say that TOKEN can be null. Also, you say that
token "... will be used as the token for this lock", but isn't that
really a question of how the caller uses *TOKEN? All that happens here
is that the hook returns a unique string. Whether or not the caller
uses that token as intended is a separate matter, beyond the control of
svn_repos__hooks_pre_lock().
Also, it might be good to be both more technically accurate and more
reference-ish (so the reader knows where to look for more information):
"If TOKEN is non-null, set *TOKEN to a new lock token generated by
the pre-lock hook if any (see the pre-lock hook template for more
information). If TOKEN is non-null but the hook does not return any
token, then set *TOKEN to <what? NULL? empty string?>"
(Looks like empty string to me, but I wasn't positive...)
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe_at_subversion.tigris.org
For additional commands, e-mail: dev-help_at_subversion.tigris.org
Received on 2008-08-19 19:41:29 CEST