"Chia-liang Kao" <clkao_at_clkao.org> writes:
> Apparently this patch somehow contains parts of my other patch. sorry
> for sending from patch hell. here's the cleaned up version.
>
> [[[
> Make pre-commit hook take lock tokens associated with the commit from stdin.
>
> * subversion/include/svn_fs.h
> * subversion/libsvn_fs/access.c
> Impose lock token restriction on newline characters.
> (svn_fs_access_add_lock_token2): New. Like svn_fs_access_add_lock_token but
> takes the path as well.
> (svn_fs_access_get_lock_tokens): New.
>
> * subversion/libsvn_ra_local/ra_plugin.c
> (svn_ra_local__get_commit_editor): Call svn_fs_access_add_lock_token2 instead,
> with the full paths in repository.
>
> * subversion/libsvn_repos/hooks.c
> (_lock_token_content): New, for turning path -> lock token hashs into
> serialized format used in pre-commit hook.
> (svn_repos__hooks_pre_commit): If there are lock tokens, make the stdin of
> the hook script take a series of lock tokens associated with this commit.
>
> * subversion/libsvn_repos/repos.c: Document the new pre-commit feature that
> lock tokens are passed in from stdin.
>
> * subversion/mod_dav_svn/repos.c
> (get_resource): Call svn_fs_access_add_lock_token2 intead.
>
> * subversion/mod_dav_svn/version.c
> (dav_svn__push_locks): Call svn_fs_access_add_lock_token2 instead.
>
> * subversion/svnserve/serve.c
> (add_lock_tokens): Call svn_fs_access_add_lock_token2 instead.
>
> ]]]
>
> === subversion/include/svn_fs.h
> ==================================================================
> --- subversion/include/svn_fs.h (revision 32317)
> +++ subversion/include/svn_fs.h (local)
> @@ -475,11 +475,27 @@
> * context remembers all tokens it receives, and makes them available
> * to fs functions. The token is not duplicated into @a access_ctx's
> * pool; make sure the token's lifetime is at least as long as @a
> - * access_ctx. */
> + * access_ctx.
> + *
> + * @since New in 1.6. */
> +
> svn_error_t *
> +svn_fs_access_add_lock_token2(svn_fs_access_t *access_ctx,
> + const char *path,
> + const char *token);
> +/**
> + * Same as svn_fs_access_add_lock_token2(), but with @a path set to value 1.
> + *
> + * @deprecated Provided for backward compatibility with the 1.1 API.
> + */
> +
> +svn_error_t *
> svn_fs_access_add_lock_token(svn_fs_access_t *access_ctx,
> const char *token);
You've added a new parameter, 'path', but not documented it. What does
it mean? What are its lifetime requirements?
> +apr_hash_t *
> +svn_fs_access_get_lock_tokens(svn_fs_access_t *access_ctx);
> +
> /** @} */
Doc string for new public function svn_fs_access_get_lock_tokens()?
>
> @@ -1794,7 +1810,7 @@
> * string by the client), and is required to make further use of the
> * lock (including removal of the lock.) A lock-token can also be
> * queried to return a svn_lock_t structure that describes the details
> - * of the lock.
> + * of the lock. lock-tokens must not contain any newline character.
> *
> * Locks are not secret; anyone can view existing locks in a
> * filesystem. Locks are not omnipotent: they can broken and stolen
Ah. The log message said "Impose lock token restriction on newline
characters." Might be clearer if it said "Document that lock tokens
cannot contain newlines".
> === subversion/libsvn_fs/access.c
> ==================================================================
> --- subversion/libsvn_fs/access.c (revision 32317)
> +++ subversion/libsvn_fs/access.c (local)
> @@ -77,11 +77,23 @@
>
>
> svn_error_t *
> +svn_fs_access_add_lock_token2(svn_fs_access_t *access_ctx,
> + const char *path,
> + const char *token)
> +{
> + apr_hash_set(access_ctx->lock_tokens,
> + token, APR_HASH_KEY_STRING, (void *) path);
> + return SVN_NO_ERROR;
> +}
See question about lifetime concerns mentioned earlier.
> +svn_error_t *
> svn_fs_access_add_lock_token(svn_fs_access_t *access_ctx,
> const char *token)
> {
> - apr_hash_set(access_ctx->lock_tokens,
> - token, APR_HASH_KEY_STRING, (void *) 1);
> + return svn_fs_access_add_lock_token2(access_ctx, (const char *) 1, token);
> +}
>
> - return SVN_NO_ERROR;
> +apr_hash_t *
> +svn_fs_access_get_lock_tokens(svn_fs_access_t *access_ctx) {
> + return access_ctx->lock_tokens;
> }
This new function is used in only one place in libsvn_repos so
far... Should we mark it private (i.e., call it
svn_fs__access_get_lock_tokens() and declare it in
include/private/svn_fs_private.h instead of here)?
> === subversion/libsvn_repos/hooks.c
> ==================================================================
> --- subversion/libsvn_repos/hooks.c (revision 32317)
> +++ subversion/libsvn_repos/hooks.c (local)
> @@ -367,7 +423,36 @@
> return SVN_NO_ERROR;
> }
>
> +static svn_error_t *
> +_lock_token_content(apr_file_t **handle, apr_hash_t *lock_tokens, apr_pool_t *pool)
> +{
This needs to be documented (yes, local internal functions too. All
functions. Every one of them. Always. :-) )
We don't usually use the leading "_", by the way; the fact that it's
local-static is enough.
> + svn_stringbuf_t *lock_str = svn_stringbuf_create("LOCKS-TOKENS:\n", pool);
"LOCK-TOKENS" I think you want, not "LOCKS-TOKENS".
> + apr_hash_index_t *hi;
>
> + for (hi = apr_hash_first(pool, lock_tokens); hi;
> + hi = apr_hash_next(hi))
> + {
> + void *val;
> + const char *path, *token;
> +
> + apr_hash_this(hi, (void *)&token, NULL, &val);
> + path = val;
> + svn_stringbuf_appendstr(lock_str,
> + svn_stringbuf_createf(pool, "%s|%s\n",
> + svn_path_uri_autoescape(path, pool),
> + token));
> + }
What happens with a path that contains a "|" (which is perfectly legal
in Subversion)?
Maybe "//" would be a better separator? Or, better yet, put the lock
token first, followed by "|" or some other character documented to be
impossible in a lock token, then put the path second.
> @@ -383,14 +468,29 @@
> else if (hook)
> {
> const char *args[4];
> + svn_fs_access_t *access_ctx;
> + apr_file_t *stdin_handle = NULL;
>
> args[0] = hook;
> args[1] = svn_path_local_style(svn_repos_path(repos, pool), pool);
> args[2] = txn_name;
> args[3] = NULL;
>
> - SVN_ERR(run_hook_cmd(SVN_REPOS__HOOK_PRE_COMMIT, hook, args, NULL,
> - pool));
> + SVN_ERR(svn_fs_get_access(&access_ctx, repos->fs));
> + if (access_ctx)
> + {
> + apr_hash_t *lock_tokens = svn_fs_access_get_lock_tokens(access_ctx);
> + if (apr_hash_count(lock_tokens)) {
> + SVN_ERR(_lock_token_content(&stdin_handle, lock_tokens, pool));
> + }
> + }
You call the handle 'stdin_handle', but what it will actually get filled
with is some tmp file handle. That could confuse the reader. Maybe
just call it hook_input, or something else that won't make the reader
think it's a handle to stdin itself?
> --- subversion/libsvn_repos/repos.c (revision 32317)
> +++ subversion/libsvn_repos/repos.c (local)
> @@ -381,6 +381,15 @@
> "# [1] REPOS-PATH (the path to this repository)" NL
> "# [2] TXN-NAME (the name of the txn about to be committed)" NL
> "#" NL
> +"# [STDIN] LOCK-TOKENS ** the lock tokens are passed via STDIN." NL
> +"#" NL
> +"# If the STDIN contains a line matching LOCK-TOKENS and a newline," NL
> +"# the lines following it are used as the lock tokens for this commit," NL
> +"# until it reaches a line containing only a newline character." NL
> +"#" NL
> +"# Each line of lock token is of the format: URI-escaped path, a '|'" NL
> +"# character, the lock token string, and a new line character." NL
> +"#" NL
Guess I was right, it's "LOCK-TOKENS" :-).
> --- subversion/mod_dav_svn/repos.c (revision 32317)
> +++ subversion/mod_dav_svn/repos.c (local)
> @@ -1783,8 +1783,13 @@
> }
>
> do {
> - serr = svn_fs_access_add_lock_token(access_ctx,
> - list->locktoken->uuid_str);
> + /* Note the path/lock pairs are only for lock token checking in access,
> + and the relative path is not actually accurate as it contains the !svn bits.
> + However we are only using the tokens for the access control. */
> +
> + serr = svn_fs_access_add_lock_token2(access_ctx, relative,
> + list->locktoken->uuid_str);
> +
Does this mean that the paths seen by the hook will be different
depending on the RA layer? That the "!svn" bits will still be in the
paths here? That would be bad; a hook is an interface, and the data we
feed that interface has to be consistent across all RA layers.
Okay, awaiting next iteration. I know that you just sent an interim
version, due to our IRC conversation just now, but that IRC conversation
didn't contain all the feedback above. So I'll just ignore the
interim version, unless you repost confirming that it addresses
everything above, not just the stuff we were talking about in IRC.
Best,
-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-07 22:23:12 CEST