[prev in list] [next in list] [prev in thread] [next in thread] 

List:       subversion-dev
Subject:    Re: [PATCH] allow pre-lock hook to specify token for locking
From:       Karl Fogel <kfogel () red-bean ! com>
Date:       2008-08-28 5:10:15
Message-ID: 871w094te0.fsf () red-bean ! com
[Download RAW message or body]

"Chia-liang Kao" <clkao@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@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

[prev in list] [next in list] [prev in thread] [next in thread]