Thanks for the review. Here's the updated patch and log. Comments
regarding the review are at the end.
[[[
Make pre-commit hook take lock tokens associated with the commit from stdin.
* subversion/include/svn_fs_private.h
Include svn_fs.h, as we need svn_fs_access_t.
(svn_fs_access_get_lock_tokens): New.
* subversion/include/svn_fs.h
* subversion/libsvn_fs/access.c
Document that lock tokens cannot contain newlines
(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.
]]]
2008/8/7 Karl Fogel <kfogel_at_red-bean.com>:
>> - 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)?
updated.
>> + 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)?
That was what the uri-escaping intended to avoid.
>> + {
>> + 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?
Well, all other functions in hook.c are calling it stdin_handle, I
guess I just didn't want to be special. ;) But I am happy to change
if it feels confusing.
>> --- 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.
No, the codepath never actually invokes the hook, it's the MERGE
request that would, only the tokens are used.
Cheers,
CLK
---------------------------------------------------------------------
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-08 00:13:59 CEST