You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Chia-liang Kao <cl...@clkao.org> on 2008/07/17 00:41:02 UTC
[PATCH] allow pre-lock hook to specify token for locking
(This is only a preliminary patch)
The patch enabling the pre-lock hook to decide the lock_token to be
used in the lock operation, which is usually autogenerated. The
changes are mostly in run_hook_cmd, making it to optionally take
stdout and return it as a string.
The function is basically reimplemented for AS400 though, so I'd
appreciate helps for making the relevant code changes in the other
part of the #ifdef... (I was also wondering why such abstraction
didn't happen in svn_io_start_cmd level though)
IMHO if the hook script decides to use such feature, it should
guarantee the uniqueness of the tokens. But kfogel mentioned on irc
we should do some guarding about it. I haven't checked if the fs
level does any guarding already, and it appears that no one is
actually using the svn_repos_fs_lock's token argument in the code
base, so there's some room for exploration.
the patch also includes my pre-{,un}lock changes, which should
probably be committed separately and i will update this patch
accordingly.
[[[
* 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_cmd2): renamed from run_hook_cmd, allow stdout of hook to be
captured and returned.
(run_hook_cmd): wrapper for run_hook_cmd2 to provide compatibility.
(svn_repos__hooks_pre_lock): call run_hook_cmd2 and return the token.
* subversion/libsvn_repos/repos.c: Document the pre-lock feature for
token from stdout.
]]]
Re: [PATCH] allow pre-lock hook to specify token for locking
Posted by Karl Fogel <kf...@red-bean.com>.
"Chia-liang Kao" <cl...@clkao.org> writes:
> Please ignore the previous one. There were some inconsistency in the
> updated run_hook_cmd.
*nod* Got it; I've deleted the previous and am considering only this
one. More soon...
-Karl
> [[[
>
> Allow pre-lock hook to be able to specify lock-token to be used with
> stdout output.
>
> **** 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.
>
> ]]]
>
> 2008/8/25 Chia-liang Kao <cl...@clkao.org>:
>> Karl,
>>
>> Thanks for the review. Here's the updated version of the patch that
>> should include all the issues addressed.
>>
>> Cheers,
>> CLK
>>
>> [[[
>>
>> Allow pre-lock hook to be able to specify lock-token to be used with
>> stdout output.
>>
>> **** 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.
>>
>> ]]]
>>
>> 2008/8/19 Karl Fogel <kf...@red-bean.com>:
>>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] allow pre-lock hook to specify token for locking
Posted by Karl Fogel <kf...@red-bean.com>.
"Chia-liang Kao" <cl...@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
Re: [PATCH] allow pre-lock hook to specify token for locking
Posted by Chia-liang Kao <cl...@clkao.org>.
Please ignore the previous one. There were some inconsistency in the
updated run_hook_cmd.
[[[
Allow pre-lock hook to be able to specify lock-token to be used with
stdout output.
**** 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.
]]]
2008/8/25 Chia-liang Kao <cl...@clkao.org>:
> Karl,
>
> Thanks for the review. Here's the updated version of the patch that
> should include all the issues addressed.
>
> Cheers,
> CLK
>
> [[[
>
> Allow pre-lock hook to be able to specify lock-token to be used with
> stdout output.
>
> **** 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.
>
> ]]]
>
> 2008/8/19 Karl Fogel <kf...@red-bean.com>:
>
Re: [PATCH] allow pre-lock hook to specify token for locking
Posted by Chia-liang Kao <cl...@clkao.org>.
Karl,
Thanks for the review. Here's the updated version of the patch that
should include all the issues addressed.
Cheers,
CLK
[[[
Allow pre-lock hook to be able to specify lock-token to be used with
stdout output.
**** 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.
]]]
2008/8/19 Karl Fogel <kf...@red-bean.com>:
Re: [PATCH] allow pre-lock hook to specify token for locking
Posted by Karl Fogel <kf...@red-bean.com>.
"Chia-liang Kao" <cl...@clkao.org> writes:
> [[[
> Allow pre-lock hook to be able to specify lock-token to be used with
> stdout output.
>
> **** 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.
> ]]]
Log message looks good.
> === subversion/libsvn_repos/fs-wrap.c
> ==================================================================
> --- subversion/libsvn_repos/fs-wrap.c (revision 32395)
> +++ 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;
>
> /* Setup an array of paths in anticipation of the ra layers handling
> @@ -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;
> /* Lock. */
> SVN_ERR(svn_fs_lock(lock, repos->fs, path, token, comment, is_dav_comment,
> expiration_date, current_rev, steal_lock, pool));
The code here looks fine. But shouldn't the doc string for
svn_repos_fs_lock() mention that this can happen now? Currently, that
function is documented to be "Like svn_fs_lock()..." with a few
differences. Now there's one more difference: the token that is
actually used can be different from the token the caller passed!
> === subversion/libsvn_repos/hooks.c
> ==================================================================
> --- subversion/libsvn_repos/hooks.c (revision 32395)
> +++ subversion/libsvn_repos/hooks.c (local)
> @@ -156,15 +156,20 @@
> 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, capture the stdout of the hook and return it.
> +*/
"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."
But shouldn't result be a stringbuf or some other type that can store
arbitrary binary data? In general, there's no guarantee that a hook's
output will be appropriate for a 'char *' result. Sure, that will be
the case with the particular hook involved in this change, but
run_hook_cmd() is a generic interface to all the hooks.
> static svn_error_t *
> -run_hook_cmd(const char *name,
> +run_hook_cmd(const char **result,
> + const char *name,
> const char *cmd,
> const char **args,
> apr_file_t *stdin_handle,
> apr_pool_t *pool)
> {
> apr_file_t *read_errhandle, *write_errhandle, *null_handle;
> + apr_file_t *read_outhandle, *write_outhandle;
> apr_status_t apr_err;
> svn_error_t *err;
> apr_proc_t cmd_proc;
> @@ -194,16 +199,39 @@
> (apr_err, _("Can't make pipe write handle non-inherited for hook '%s'"),
> cmd);
>
> + if (result)
> + {
> + /* Create a pipe to access stdout of the child. */
> + apr_err = apr_file_pipe_create(&read_outhandle, &write_outhandle, pool);
> + if (apr_err)
> + return svn_error_wrap_apr
> + (apr_err, _("Can't create pipe for hook '%s'"), cmd);
>
> - /* Redirect stdout to the null device */
> - apr_err = apr_file_open(&null_handle, SVN_NULL_DEVICE_NAME, APR_WRITE,
> - APR_OS_DEFAULT, pool);
> - if (apr_err)
> - return svn_error_wrap_apr
> - (apr_err, _("Can't create null stdout for hook '%s'"), cmd);
> + apr_err = apr_file_inherit_unset(read_outhandle);
> + if (apr_err)
> + return svn_error_wrap_apr
> + (apr_err,
> + _("Can't make pipe read handle non-inherited for hook '%s'"), cmd);
>
> + apr_err = apr_file_inherit_unset(write_outhandle);
> + if (apr_err)
> + return svn_error_wrap_apr
> + (apr_err,
> + _("Can't make pipe write handle non-inherited for hook '%s'"), cmd);
> + }
> + else
> + {
> + /* Redirect stdout to the null device */
> + apr_err = apr_file_open(&null_handle, SVN_NULL_DEVICE_NAME, APR_WRITE,
> + APR_OS_DEFAULT, pool);
> + if (apr_err)
> + return svn_error_wrap_apr
> + (apr_err, _("Can't create null stdout for hook '%s'"), cmd);
> + }
> +
> err = svn_io_start_cmd(&cmd_proc, ".", cmd, args, FALSE,
> - stdin_handle, null_handle, write_errhandle, pool);
> + stdin_handle, result ? write_outhandle : null_handle,
> + write_errhandle, pool);
>
> /* This seems to be done automatically if we pass the third parameter of
> apr_procattr_child_in/out_set(), but svn_io_run_cmd()'s interface does
> @@ -214,6 +242,14 @@
> return svn_error_wrap_apr
> (apr_err, _("Error closing write end of stderr pipe"));
>
> + if (result)
> + {
> + apr_err = apr_file_close(write_outhandle);
> + if (!err && apr_err)
> + return svn_error_wrap_apr
> + (apr_err, _("Error closing write end of stderr pipe"));
> + }
> +
> if (err)
> {
> err = svn_error_createf
> @@ -232,10 +268,24 @@
> return svn_error_wrap_apr
> (apr_err, _("Error closing read end of stderr pipe"));
>
> - apr_err = apr_file_close(null_handle);
> - if (!err && apr_err)
> - return svn_error_wrap_apr(apr_err, _("Error closing null file"));
> + if (result)
> + {
> + svn_stringbuf_t *native_stdout;
> + SVN_ERR(svn_stringbuf_from_aprfile(&native_stdout, read_outhandle, pool));
> + apr_err = apr_file_close(read_outhandle);
> + if (!err && apr_err)
> + return svn_error_wrap_apr
> + (apr_err, _("Error closing read end of stderr pipe"));
>
> + *result = (svn_string_create_from_buf(native_stdout, pool))->data;
> + }
> + else
> + {
> + apr_err = apr_file_close(null_handle);
> + if (!err && apr_err)
> + return svn_error_wrap_apr(apr_err, _("Error closing null file"));
> + }
> +
> return err;
> }
That all looks good to me (not very experienced with in/out redirection
via APR interfaces, but nothing jumps out at me).
> @@ -258,7 +308,6 @@
> return SVN_NO_ERROR;
> }
>
> -
> /* Check if the HOOK program exists and is a file or a symbolic link, using
> POOL for temporary allocations.
Spurious change, but hey, I can't claim that it really distracted me
that much :-)
> @@ -541,6 +589,7 @@
>
> svn_error_t *
> svn_repos__hooks_pre_lock(svn_repos_t *repos,
> + const char **token,
> const char *path,
> const char *username,
> const char *comment,
> @@ -566,7 +615,8 @@
> 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(token, SVN_REPOS__HOOK_PRE_LOCK, hook, args, NULL,
> + pool));
> }
>
> return SVN_NO_ERROR;
> === subversion/libsvn_repos/repos.c
> ==================================================================
> --- subversion/libsvn_repos/repos.c (revision 32395)
> +++ subversion/libsvn_repos/repos.c (local)
> @@ -540,6 +540,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 each." NL
> +"# time." NL
> +"#" NL
We should say unique across what space. Globally unique?
> === subversion/libsvn_repos/repos.h
> ==================================================================
> --- subversion/libsvn_repos/repos.h (revision 32395)
> +++ subversion/libsvn_repos/repos.h (local)
> @@ -223,10 +223,12 @@
> PATH is the path being locked, USERNAME is the person doing it,
> COMMENT is the comment of the lock, and is treated as an empty
> string when NULL is given. STEAL-LOCK is a flag if the user is
> - stealing the lock. */
> + stealing the lock. If TOKEN is returned by the hook, it will be
> + used as the token for this lock. */
>
> svn_error_t *
> svn_repos__hooks_pre_lock(svn_repos_t *repos,
> + const char **token,
> const char *path,
> const char *username,
> const char *comment,
This doc string doesn't say that TOKEN can be null. Also, you say that
token "... will be used as the token for this lock", but isn't that
really a question of how the caller uses *TOKEN? All that happens here
is that the hook returns a unique string. Whether or not the caller
uses that token as intended is a separate matter, beyond the control of
svn_repos__hooks_pre_lock().
Also, it might be good to be both more technically accurate and more
reference-ish (so the reader knows where to look for more information):
"If TOKEN is non-null, set *TOKEN to a new lock token generated by
the pre-lock hook if any (see the pre-lock hook template for more
information). If TOKEN is non-null but the hook does not return any
token, then set *TOKEN to <what? NULL? empty string?>"
(Looks like empty string to me, but I wasn't positive...)
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] allow pre-lock hook to specify token for locking
Posted by Chia-liang Kao <cl...@clkao.org>.
Point. Meanwhile I think I somehow lost the summary of the log
message, leaving just the per-function entries. How about:
[[[
Allow pre-lock hook to be able to specify lock-token to be used with
stdout output.
**** 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.
]]]
Cheers,
CLK
2008/8/8 C. Michael Pilato <cm...@collab.net>:
> This might be considered quite tangential, but I very much feel that the log
> message for the commit of this feature should call out clearly (perhaps with
> "***NOTE***" or something) that this changes the contract for existing
> pre-lock hooks which might be spewing stdout today. We'll want to
> release-note this as a compatibility concern. Or, we can go ahead and begin
> the svn_1.6_releasenotes.html document with at least something about this
> scribbled therein.
>
>
> Chia-liang Kao wrote:
>>
>> Alright, here's the one with all callers updated.
>>
>> [[[
>> * 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.
>> ]]]
>>
>>
>> 2008/8/7 Karl Fogel <kf...@red-bean.com>:
>>>
>>> "Chia-liang Kao" <cl...@clkao.org> writes:
>>>>
>>>> 2008/8/3 Arfrever Frehtes Taifersar Arahesis <ar...@gmail.com>:
>>>>>
>>>>> This is private function so no compatibility should be required.
>>>>
>>>> I guess it's mostly because i have another patch at the same time and
>>>> i didn't want to make them interdependent. Can I un-revision the
>>>> private function after they are both approved?
>>>
>>> I confess, I completely didn't understand this explanation... :-) Help?
>>> (Or just post a new version without the needless private API rev?)
>>>
>>> Thanks,
>>> -K
>>>
>>>
>>> ------------------------------------------------------------------------
>>>
>>> ---------------------------------------------------------------------
>>> To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
>>> For additional commands, e-mail: dev-help@subversion.tigris.org
>
>
> --
> C. Michael Pilato <cm...@collab.net>
> CollabNet <> www.collab.net <> Distributed Development On Demand
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] allow pre-lock hook to specify token for locking
Posted by "C. Michael Pilato" <cm...@collab.net>.
This might be considered quite tangential, but I very much feel that the log
message for the commit of this feature should call out clearly (perhaps with
"***NOTE***" or something) that this changes the contract for existing
pre-lock hooks which might be spewing stdout today. We'll want to
release-note this as a compatibility concern. Or, we can go ahead and begin
the svn_1.6_releasenotes.html document with at least something about this
scribbled therein.
Chia-liang Kao wrote:
> Alright, here's the one with all callers updated.
>
> [[[
> * 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.
> ]]]
>
>
> 2008/8/7 Karl Fogel <kf...@red-bean.com>:
>> "Chia-liang Kao" <cl...@clkao.org> writes:
>>> 2008/8/3 Arfrever Frehtes Taifersar Arahesis <ar...@gmail.com>:
>>>> This is private function so no compatibility should be required.
>>> I guess it's mostly because i have another patch at the same time and
>>> i didn't want to make them interdependent. Can I un-revision the
>>> private function after they are both approved?
>> I confess, I completely didn't understand this explanation... :-) Help?
>> (Or just post a new version without the needless private API rev?)
>>
>> Thanks,
>> -K
>>
>>
>> ------------------------------------------------------------------------
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
>> For additional commands, e-mail: dev-help@subversion.tigris.org
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Distributed Development On Demand
Re: [PATCH] allow pre-lock hook to specify token for locking
Posted by Chia-liang Kao <cl...@clkao.org>.
Alright, here's the one with all callers updated.
[[[
* 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.
]]]
2008/8/7 Karl Fogel <kf...@red-bean.com>:
> "Chia-liang Kao" <cl...@clkao.org> writes:
>> 2008/8/3 Arfrever Frehtes Taifersar Arahesis <ar...@gmail.com>:
>>> This is private function so no compatibility should be required.
>>
>> I guess it's mostly because i have another patch at the same time and
>> i didn't want to make them interdependent. Can I un-revision the
>> private function after they are both approved?
>
> I confess, I completely didn't understand this explanation... :-) Help?
> (Or just post a new version without the needless private API rev?)
>
> Thanks,
> -K
>
Re: [PATCH] allow pre-lock hook to specify token for locking
Posted by Karl Fogel <kf...@red-bean.com>.
"Chia-liang Kao" <cl...@clkao.org> writes:
> 2008/8/3 Arfrever Frehtes Taifersar Arahesis <ar...@gmail.com>:
>> This is private function so no compatibility should be required.
>
> I guess it's mostly because i have another patch at the same time and
> i didn't want to make them interdependent. Can I un-revision the
> private function after they are both approved?
I confess, I completely didn't understand this explanation... :-) Help?
(Or just post a new version without the needless private API rev?)
Thanks,
-K
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] allow pre-lock hook to specify token for locking
Posted by Chia-liang Kao <cl...@clkao.org>.
2008/8/3 Arfrever Frehtes Taifersar Arahesis <ar...@gmail.com>:
> This is private function so no compatibility should be required.
I guess it's mostly because i have another patch at the same time and
i didn't want to make them interdependent. Can I un-revision the
private function after they are both approved?
Cheers,
CLK
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] allow pre-lock hook to specify token for locking
Posted by Arfrever Frehtes Taifersar Arahesis <ar...@gmail.com>.
2008-07-28 14:41:59 Chia-liang Kao napisał(a):
> [[[
> * 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_cmd2): renamed from run_hook_cmd, allow stdout of hook to be
> captured and returned.
> (run_hook_cmd): wrapper for run_hook_cmd2 to provide compatibility.
This is private function so no compatibility should be required.
> (svn_repos__hooks_pre_lock): call run_hook_cmd2 and return the token.
>
> * subversion/libsvn_repos/repos.c: Document the pre-lock feature for
> token from stdout.
>
> ]]]
--
Arfrever Frehtes Taifersar Arahesis
Re: [PATCH] allow pre-lock hook to specify token for locking
Posted by Chia-liang Kao <cl...@clkao.org>.
Here's the updated version of the patch now that some of the parts are
committed. I've also added the notes about the generated locks must
be unique each time if the user chooses to use this feature, per the
discussion in this thread.
[[[
* 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_cmd2): renamed from run_hook_cmd, allow stdout of hook to be
captured and returned.
(run_hook_cmd): wrapper for run_hook_cmd2 to provide compatibility.
(svn_repos__hooks_pre_lock): call run_hook_cmd2 and return the token.
* subversion/libsvn_repos/repos.c: Document the pre-lock feature for
token from stdout.
]]]
2008/7/21 Chia-liang Kao <cl...@clkao.org>:
> 2008/7/21 Karl Fogel <kf...@red-bean.com>:
>>> This is for locking through a slave repository with pushmi, we'd use the
>>> pre-lock hook to lock the mater server and then notify svn about the token
>>> we got from the master, so they are in sync.
>>
>> In other words, the slave would be locked too, and it would be using the
>> exact same lock token as the master?
>
> Yes, because it's supposed to be in total sync that you can switch back and
> forth between master and different replicas. of course the whole lock
> database need to be sync'ed over time, but this particular hook is to make
> the lock immediately in sync when committing through the replica.
>
> Cheers,
> CLK
>
Re: [PATCH] allow pre-lock hook to specify token for locking
Posted by Chia-liang Kao <cl...@clkao.org>.
2008/7/21 Karl Fogel <kf...@red-bean.com>:
>> This is for locking through a slave repository with pushmi, we'd use the
>> pre-lock hook to lock the mater server and then notify svn about the token
>> we got from the master, so they are in sync.
>
> In other words, the slave would be locked too, and it would be using the
> exact same lock token as the master?
Yes, because it's supposed to be in total sync that you can switch back and
forth between master and different replicas. of course the whole lock
database need to be sync'ed over time, but this particular hook is to make
the lock immediately in sync when committing through the replica.
Cheers,
CLK
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] allow pre-lock hook to specify token for locking
Posted by Karl Fogel <kf...@red-bean.com>.
"Chia-liang Kao" <cl...@clkao.org> writes:
> Well from what i can see in the code is that fs layer API actually takes
> the token optionally, which might not be generated by us. It's just that
> no one is using that API.
Ah, I didn't realize that.
>> The requirement that the tokens be unique is not for Subversion, it's
>> for Subversion's consumers -- the whole point of a lock token is to
>> unambiguously, universally, uniquely identify the lock. (I can't think
>> of any more words beginning with "u" to help make the point. :-) ) It's
>> not Subversion that needs that guarantee -- it's other software that
>> might be helping users communicate, and that might depend on unique lock
>> tokens to do so.
>
> The feature I am asking and implementing is not about adding non-unique
> locks, if you read my original patch. it could be a side-effect if
> the feature is
> not used carefully, which is the same thing if the current fs layer api is not
> used carefully.
Yes, I understand that non-unique locks is not your goal; custom
lock-tokens is.
>> Can you describe why you want this feature? It might help us all think
>> better...
>
> This is for locking through a slave repository with pushmi, we'd use the
> pre-lock hook to lock the mater server and then notify svn about the token
> we got from the master, so they are in sync.
In other words, the slave would be locked too, and it would be using the
exact same lock token as the master?
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] allow pre-lock hook to specify token for locking
Posted by Chia-liang Kao <cl...@clkao.org>.
2008/7/21 Karl Fogel <kf...@red-bean.com>:
> "Chia-liang Kao" <cl...@clkao.org> writes:
>> This is mainly cleanup. token uniqueness are mentioned in fs_fs layer
>> lock unction as TODO:
>>
>> /* If the caller provided a TOKEN, we *really* need to see
>> if a lock already exists with that token, and if so, verify that
>> the lock's path matches PATH. Otherwise we run the risk of
>> breaking the 1-to-1 mapping of lock tokens to locked paths. */
>> /* ### TODO: actually do this check. This is tough, because the
>> schema doesn't supply a lookup-by-token mechanism. */
>>
>> I think it's a bit overkilling for this feature to implement the check
>> that is currently lacking underlying facilities. I am also wondering
>> why do we need to lock-token to be globally unique? not just unique
>> per-path? Given the comment stating we don't do any lookup-by-token,
>> I don't suppose lock would work across renamed nodes either? Or am i
>> being naïve and missing something obvious here?
>
> The reason the current code does not check ("guard") to make sure the
> token is unique is that the current code generates those tokens itself,
> and so it knows they're unique. If external code becomes responsible
> for generating tokens, then that guarantee goes away.
Well from what i can see in the code is that fs layer API actually takes
the token optionally, which might not be generated by us. It's just that
no one is using that API.
> The requirement that the tokens be unique is not for Subversion, it's
> for Subversion's consumers -- the whole point of a lock token is to
> unambiguously, universally, uniquely identify the lock. (I can't think
> of any more words beginning with "u" to help make the point. :-) ) It's
> not Subversion that needs that guarantee -- it's other software that
> might be helping users communicate, and that might depend on unique lock
> tokens to do so.
The feature I am asking and implementing is not about adding non-unique
locks, if you read my original patch. it could be a side-effect if
the feature is
not used carefully, which is the same thing if the current fs layer api is not
used carefully.
> Can you describe why you want this feature? It might help us all think
> better...
This is for locking through a slave repository with pushmi, we'd use the
pre-lock hook to lock the mater server and then notify svn about the token
we got from the master, so they are in sync.
Cheers,
CLK
Re: [PATCH] allow pre-lock hook to specify token for locking
Posted by Karl Fogel <kf...@red-bean.com>.
"Chia-liang Kao" <cl...@clkao.org> writes:
> This is mainly cleanup. token uniqueness are mentioned in fs_fs layer
> lock unction as TODO:
>
> /* If the caller provided a TOKEN, we *really* need to see
> if a lock already exists with that token, and if so, verify that
> the lock's path matches PATH. Otherwise we run the risk of
> breaking the 1-to-1 mapping of lock tokens to locked paths. */
> /* ### TODO: actually do this check. This is tough, because the
> schema doesn't supply a lookup-by-token mechanism. */
>
> I think it's a bit overkilling for this feature to implement the check
> that is currently lacking underlying facilities. I am also wondering
> why do we need to lock-token to be globally unique? not just unique
> per-path? Given the comment stating we don't do any lookup-by-token,
> I don't suppose lock would work across renamed nodes either? Or am i
> being naïve and missing something obvious here?
The reason the current code does not check ("guard") to make sure the
token is unique is that the current code generates those tokens itself,
and so it knows they're unique. If external code becomes responsible
for generating tokens, then that guarantee goes away.
The requirement that the tokens be unique is not for Subversion, it's
for Subversion's consumers -- the whole point of a lock token is to
unambiguously, universally, uniquely identify the lock. (I can't think
of any more words beginning with "u" to help make the point. :-) ) It's
not Subversion that needs that guarantee -- it's other software that
might be helping users communicate, and that might depend on unique lock
tokens to do so.
You're probably going to ask for for an example of some software now,
and I don't have one. I just know that we've been making a promise of
uniqueness (or at least, uniqueness-per-repository), and now we're
contemplating taking that away.
There might be no consequences, I just don't know.
Can you describe why you want this feature? It might help us all think
better...
-Karl
> 2008/7/16 Chia-liang Kao <cl...@clkao.org>:
>> IMHO if the hook script decides to use such feature, it should
>> guarantee the uniqueness of the tokens. But kfogel mentioned on irc
>> we should do some guarding about it. I haven't checked if the fs
>> level does any guarding already, and it appears that no one is
>> actually using the svn_repos_fs_lock's token argument in the code
>> base, so there's some room for exploration.
>>
>> the patch also includes my pre-{,un}lock changes, which should
>> probably be committed separately and i will update this patch
>> accordingly.
>>
>> [[[
>> * 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_cmd2): renamed from run_hook_cmd, allow stdout of hook to be
>> captured and returned.
>> (run_hook_cmd): wrapper for run_hook_cmd2 to provide compatibility.
>> (svn_repos__hooks_pre_lock): call run_hook_cmd2 and return the token.
>>
>> * subversion/libsvn_repos/repos.c: Document the pre-lock feature for
>> token from stdout.
>>
>> ]]]
>>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
> For additional commands, e-mail: dev-help@subversion.tigris.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] allow pre-lock hook to specify token for locking
Posted by "C. Michael Pilato" <cm...@collab.net>.
Chia-liang Kao wrote:
> Attached is the updated patch.
>
> This is mainly cleanup. token uniqueness are mentioned in fs_fs layer
> lock unction as TODO:
>
> /* If the caller provided a TOKEN, we *really* need to see
> if a lock already exists with that token, and if so, verify that
> the lock's path matches PATH. Otherwise we run the risk of
> breaking the 1-to-1 mapping of lock tokens to locked paths. */
> /* ### TODO: actually do this check. This is tough, because the
> schema doesn't supply a lookup-by-token mechanism. */
>
> I think it's a bit overkilling for this feature to implement the check
> that is currently lacking underlying facilities. I am also wondering
> why do we need to lock-token to be globally unique? not just unique
> per-path?
I *think* we inherited this from the WebDAV specification's requirements for
locks. See http://www.webdav.org/specs/rfc2518.html#rfc.section.6.3
> Given the comment stating we don't do any lookup-by-token,
> I don't suppose lock would work across renamed nodes either? Or am i
> being naïve and missing something obvious here?
No, locks do not follow their paths if the paths are renamed.
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Distributed Development On Demand
Re: [PATCH] allow pre-lock hook to specify token for locking
Posted by Chia-liang Kao <cl...@clkao.org>.
Attached is the updated patch.
This is mainly cleanup. token uniqueness are mentioned in fs_fs layer
lock unction as TODO:
/* If the caller provided a TOKEN, we *really* need to see
if a lock already exists with that token, and if so, verify that
the lock's path matches PATH. Otherwise we run the risk of
breaking the 1-to-1 mapping of lock tokens to locked paths. */
/* ### TODO: actually do this check. This is tough, because the
schema doesn't supply a lookup-by-token mechanism. */
I think it's a bit overkilling for this feature to implement the check
that is currently lacking underlying facilities. I am also wondering
why do we need to lock-token to be globally unique? not just unique
per-path? Given the comment stating we don't do any lookup-by-token,
I don't suppose lock would work across renamed nodes either? Or am i
being naïve and missing something obvious here?
2008/7/16 Chia-liang Kao <cl...@clkao.org>:
> IMHO if the hook script decides to use such feature, it should
> guarantee the uniqueness of the tokens. But kfogel mentioned on irc
> we should do some guarding about it. I haven't checked if the fs
> level does any guarding already, and it appears that no one is
> actually using the svn_repos_fs_lock's token argument in the code
> base, so there's some room for exploration.
>
> the patch also includes my pre-{,un}lock changes, which should
> probably be committed separately and i will update this patch
> accordingly.
>
> [[[
> * 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_cmd2): renamed from run_hook_cmd, allow stdout of hook to be
> captured and returned.
> (run_hook_cmd): wrapper for run_hook_cmd2 to provide compatibility.
> (svn_repos__hooks_pre_lock): call run_hook_cmd2 and return the token.
>
> * subversion/libsvn_repos/repos.c: Document the pre-lock feature for
> token from stdout.
>
> ]]]
>