"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.
Okay, applied in r32778, with some tweaks. Below is discussion of the
tweaks.
> **** 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.
The documentation change to
* subversion/include/svn_repos.h (svn_repos_fs_lock)
wasn't mentioned in the log message; I inserted it.
> === subversion/include/svn_repos.h
> ==================================================================
> --- subversion/include/svn_repos.h (revision 32695)
> +++ subversion/include/svn_repos.h (local)
> @@ -1618,6 +1618,9 @@
> * hook, return the original error wrapped with
> * SVN_ERR_REPOS_POST_LOCK_HOOK_FAILED. If the caller sees this
> * error, it knows that the lock succeeded anyway.
> + *
> + * The pre-lock hook can also return a different token for the lock to
> + * be used, instead of @a token.
> */
> svn_error_t *
> svn_repos_fs_lock(svn_lock_t **lock,
Tweaked wording here a bit, just for clarity. Nothing major.
> --- subversion/libsvn_repos/fs-wrap.c (revision 32695)
> +++ 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;
Removed the initialization here. The only use of new_token comes after
it has been passed (by reference) to a caller whose doc string promises
to set it. Therefore, we should not initialize it in its declaration;
doing so could mask bugs (as you'll see later :-) ).
> @@ -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;
I changed this to:
if (*new_token)
token = new_token;
> --- subversion/libsvn_repos/hooks.c (revision 32695)
> +++ subversion/libsvn_repos/hooks.c (local)
> @@ -157,15 +157,21 @@
> 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, set *RESULT to the stdout of the hook or to
> + the empty string if the hook generates no output on stdout.
> +*/
> static svn_error_t *
> -run_hook_cmd(const char *name,
> +run_hook_cmd(svn_string_t **result,
> + const char *name,
> const char *cmd,
> const char **args,
> apr_file_t *stdin_handle,
> apr_pool_t *pool)
Very minor doc tweak here, just a matter of taste; I do not claim it is
any more correct than what you wrote :-). I said "zero-length string"
instead of "empty string", because the latter makes me think of 'char *'
strings, and this is an 'svn_string_t *'.
> @@ -620,7 +673,10 @@
> 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(&buf, SVN_REPOS__HOOK_PRE_LOCK, hook, args, NULL,
> + pool));
> + if (token)
> + *token = buf->data;
> }
Here there was a serious problem: *token would not be set if there was
no hook, despite the doc string's promise. See the r32778 diff for what
I did instead.
(The result of this was that when 'svn lock' was run with no pre-lock
hook, Subversion would simply segfault. Did you test that case?)
> --- subversion/libsvn_repos/repos.c (revision 32695)
> +++ subversion/libsvn_repos/repos.c (local)
> @@ -549,6 +549,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 across" NL
> +"# the repository each time" NL
> +"#" NL
I added the missing period at the end here, and changed "in stdout" to
"on stdout" (I think the latter is more common usage).
-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-28 07:10:32 CEST