Re: [PATCH] Make pre-commit hook have access to lock tokens

Thanks for the review again. Here's the updated log and the comments
are inline.

[[[
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
 Include private/svn_fs_private.h.
 (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/19 Karl Fogel <kfogel_at_red-bean.com>:
>> No, the codepath never actually invokes the hook, it's the MERGE
>> request that would, only the tokens are used.
>
> Okay. The last sentence of the comment is ambiguous; it should say
>
> "However, we're using only the tokens anyway (for access control)."

Fixed.

> I don't think it's a big deal, but this header addition isn't mentioned
> in the log message. Yes, I'm an evil, evil nitpicker :-).

I think it is in the original message.

> There are some extra "*"s in your doc string, I think.

Removed.
>> +svn_error_t *
>> svn_fs_access_add_lock_token(svn_fs_access_t *access_ctx,
>> const char *token);
>
> A thought: does this mean that if a caller uses the old
> svn_fs_access_add_lock_token() API, but *also* uses the new
> svn_fs__access_get_lock_tokens() API, that the values from the latter
> will be unusuable, because they'll be 1? If so, maybe the doc string
> for svn_fs__access_get_lock_tokens() should point this out, as well as
> the doc string for svn_fs_access_add_lock_token().
>
> Or maybe svn_fs__access_get_lock_tokens() should look for the special
> value 1 (yuck, sigh) and return NULL in that case? (This would need to
> be documented too, of course.)

Added docstring about using the v2 api if you intend to use get_lock_tokens.

>> * 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
>
> This change is not mentioned in the log message either. This is
> actually not nit-picking: as a reader of the change, I'm wondering what
> the motivation behind the above doc string tweak is.
>
> Actually, it would be even better to say in the doc string itself why
> the no-newline requirement is imposed: talk about the pre-commit format.

I think it is in the original message as well. But i added some more
comment about why the new restriction is imposed.
>> 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;
>> +}
>
> Why is the '(void *)' cast necessary?

I don't know, because the original one was doing (void *) cast for 1?
I was only imitating ;)

>> +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);
>> +}
>
> Yup. Should be 'svn_fs__access_add_lock_token', as we discussed.

I think the add_lock_token is public, just the get_lock_tokens is private.

>> + const char *path, *token;
>> + const void *key;
>>
>> - apr_hash_this(hi, NULL, NULL, &val);
>> + apr_hash_this(hi, &key, NULL, &val);
>> + path = svn_path_join(sess->fs_path->data, (const char *)key, pool);
>> token = val;
>> - SVN_ERR(svn_fs_access_add_lock_token(fs_access, token));
>> + SVN_ERR(svn_fs_access_add_lock_token2(fs_access, (const char *)path, token));
>> }
>> }
>> }
>
> 'path' is already a 'const char *'. Why is the second cast necessary?

Fixed.

>> +/* Turns the lock_tokens hash into the file handle in the format
>> + documented in the pre-commit template */
>>
>> +static svn_error_t *
>> +lock_token_content(apr_file_t **handle, apr_hash_t *lock_tokens, apr_pool_t *pool)
>> +{
>
> Can we have a real doc string please? It's fine to refer to the
> pre-commit template, but a doc string needs to say exactly what happens
> with all the arguments. Among other things, this should document the
> format of 'lock_tokens' (referring out to svn_fs_access_get_lock_tokens
> or some other function is fine; the main thing is that a reader of the
> code does not have to spend time wondering).

I did some tweak to conform the style of other static functions, but
it's not from what I had before. and it does refer to the pre-commit
template...

> So, hmm, should we worry about the fact that this isn't done streamily?
> That is, we put the whole list in a string and then put the string in a
> tmp file. Does this open up a D.o.S attack on the server? Is there any
> way to do it streamily?

the current run_hook_cmd interface doesn't allow it easily. I suppose
this is the same issue with prop value etc being passed in as stdin
from a tmp file as well. If it is indeed an issue, it should be fixed
along with other uses of the api.

Thanks again for the review.

Cheers,
CLK

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe_at_subversion.tigris.org
For additional commands, e-mail: dev-help_at_subversion.tigris.org