You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by cm...@apache.org on 2012/08/21 19:29:40 UTC
svn commit: r1375675 - in /subversion/trunk/subversion: include/svn_repos.h
libsvn_repos/fs-wrap.c libsvn_repos/hooks.c libsvn_repos/repos.c
libsvn_repos/repos.h tests/cmdline/commit_tests.py
tests/cmdline/svntest/actions.py
Author: cmpilato
Date: Tue Aug 21 17:29:40 2012
New Revision: 1375675
URL: http://svn.apache.org/viewvc?rev=1375675&view=rev
Log:
Introduce a new 'init-commit' hook script which runs immediately after
the commit txn is created and populated with initial txnprops.
* subversion/libsvn_repos/repos.h
(SVN_REPOS__HOOK_INIT_COMMIT): New #define.
(svn_repos__hook_init_commit): New function.
* subversion/libsvn_repos/hooks.c
(check_hook_result): Log errors for init-commit hook, too.
(svn_repos__hooks_init_commit): New function.
* subversion/include/svn_repos.h
(svn_repos_init_commit_hook): New function.
* subversion/libsvn_repos/repos.c
(svn_repos_init_commit_hook): New function.
(create_hooks): Also create new init-commit hook template.
* subversion/libsvn_repos/fs-wrap.c
(svn_repos_fs_begin_txn_for_commit2): Now also run the init-commit hook.
* subversion/tests/cmdline/svntest/actions.py
(hook_failure_message): Add "init-commit" to the list of "Commit
blocked..." expected failure messages.
* subversion/tests/cmdline/commit_tests.py
(init_commit_hook_test): New test.
(test_list): Add reference to new test.
Modified:
subversion/trunk/subversion/include/svn_repos.h
subversion/trunk/subversion/libsvn_repos/fs-wrap.c
subversion/trunk/subversion/libsvn_repos/hooks.c
subversion/trunk/subversion/libsvn_repos/repos.c
subversion/trunk/subversion/libsvn_repos/repos.h
subversion/trunk/subversion/tests/cmdline/commit_tests.py
subversion/trunk/subversion/tests/cmdline/svntest/actions.py
Modified: subversion/trunk/subversion/include/svn_repos.h
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/include/svn_repos.h?rev=1375675&r1=1375674&r2=1375675&view=diff
==============================================================================
--- subversion/trunk/subversion/include/svn_repos.h (original)
+++ subversion/trunk/subversion/include/svn_repos.h Tue Aug 21 17:29:40 2012
@@ -715,6 +715,12 @@ const char *
svn_repos_start_commit_hook(svn_repos_t *repos,
apr_pool_t *pool);
+/** Return the path to @a repos's init-commit hook, allocated in @a pool.
+ * @since New in 1.8 */
+const char *
+svn_repos_init_commit_hook(svn_repos_t *repos,
+ apr_pool_t *pool);
+
/** Return the path to @a repos's pre-commit hook, allocated in @a pool. */
const char *
svn_repos_pre_commit_hook(svn_repos_t *repos,
Modified: subversion/trunk/subversion/libsvn_repos/fs-wrap.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_repos/fs-wrap.c?rev=1375675&r1=1375674&r2=1375675&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_repos/fs-wrap.c (original)
+++ subversion/trunk/subversion/libsvn_repos/fs-wrap.c Tue Aug 21 17:29:40 2012
@@ -83,9 +83,11 @@ svn_repos_fs_begin_txn_for_commit2(svn_f
apr_hash_t *revprop_table,
apr_pool_t *pool)
{
- svn_string_t *author = apr_hash_get(revprop_table, SVN_PROP_REVISION_AUTHOR,
- APR_HASH_KEY_STRING);
apr_array_header_t *revprops;
+ const char *txn_name;
+ svn_string_t *author = apr_hash_get(revprop_table,
+ SVN_PROP_REVISION_AUTHOR,
+ APR_HASH_KEY_STRING);
/* Run start-commit hooks. */
SVN_ERR(svn_repos__hooks_start_commit(repos, author ? author->data : NULL,
@@ -94,12 +96,15 @@ svn_repos_fs_begin_txn_for_commit2(svn_f
/* Begin the transaction, ask for the fs to do on-the-fly lock checks. */
SVN_ERR(svn_fs_begin_txn2(txn_p, repos->fs, rev,
SVN_FS_TXN_CHECK_LOCKS, pool));
+ SVN_ERR(svn_fs_txn_name(&txn_name, *txn_p, pool));
/* We pass the revision properties to the filesystem by adding them
as properties on the txn. Later, when we commit the txn, these
properties will be copied into the newly created revision. */
revprops = svn_prop_hash_to_array(revprop_table, pool);
- return svn_repos_fs_change_txn_props(*txn_p, revprops, pool);
+ SVN_ERR(svn_repos_fs_change_txn_props(*txn_p, revprops, pool));
+
+ return svn_error_trace(svn_repos__hooks_init_commit(repos, txn_name, pool));
}
Modified: subversion/trunk/subversion/libsvn_repos/hooks.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_repos/hooks.c?rev=1375675&r1=1375674&r2=1375675&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_repos/hooks.c (original)
+++ subversion/trunk/subversion/libsvn_repos/hooks.c Tue Aug 21 17:29:40 2012
@@ -124,6 +124,7 @@ check_hook_result(const char *name, cons
{
const char *action;
if (strcmp(name, "start-commit") == 0
+ || strcmp(name, "init-commit") == 0
|| strcmp(name, "pre-commit") == 0)
action = _("Commit");
else if (strcmp(name, "pre-revprop-change") == 0)
@@ -397,6 +398,34 @@ svn_repos__hooks_start_commit(svn_repos_
return SVN_NO_ERROR;
}
+svn_error_t *
+svn_repos__hooks_init_commit(svn_repos_t *repos,
+ const char *txn_name,
+ apr_pool_t *pool)
+{
+ const char *hook = svn_repos_init_commit_hook(repos, pool);
+ svn_boolean_t broken_link;
+
+ if ((hook = check_hook_cmd(hook, &broken_link, pool)) && broken_link)
+ {
+ return hook_symlink_error(hook);
+ }
+ else if (hook)
+ {
+ const char *args[4];
+
+ args[0] = hook;
+ args[1] = svn_dirent_local_style(svn_repos_path(repos, pool), pool);
+ args[2] = txn_name;
+ args[3] = NULL;
+
+ SVN_ERR(run_hook_cmd(NULL, SVN_REPOS__HOOK_INIT_COMMIT, hook, args,
+ repos->hooks_env, NULL, pool));
+ }
+
+ return SVN_NO_ERROR;
+}
+
/* Set *HANDLE to an open filehandle for a temporary file (i.e.,
automatically deleted when closed), into which the LOCK_TOKENS have
been written out in the format described in the pre-commit hook
Modified: subversion/trunk/subversion/libsvn_repos/repos.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_repos/repos.c?rev=1375675&r1=1375674&r2=1375675&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_repos/repos.c (original)
+++ subversion/trunk/subversion/libsvn_repos/repos.c Tue Aug 21 17:29:40 2012
@@ -111,6 +111,13 @@ svn_repos_start_commit_hook(svn_repos_t
const char *
+svn_repos_init_commit_hook(svn_repos_t *repos, apr_pool_t *pool)
+{
+ return svn_dirent_join(repos->hook_path, SVN_REPOS__HOOK_INIT_COMMIT, pool);
+}
+
+
+const char *
svn_repos_pre_commit_hook(svn_repos_t *repos, apr_pool_t *pool)
{
return svn_dirent_join(repos->hook_path, SVN_REPOS__HOOK_PRE_COMMIT, pool);
@@ -375,6 +382,91 @@ PREWRITTEN_HOOKS_TEXT
SVN_ERR(svn_io_set_file_executable(this_path, TRUE, FALSE, pool));
} /* end start-commit hook */
+ /* Init-commit hook. */
+ {
+ this_path = apr_psprintf(pool, "%s%s",
+ svn_repos_init_commit_hook(repos, pool),
+ SVN_REPOS__HOOK_DESC_EXT);
+
+#define SCRIPT_NAME SVN_REPOS__HOOK_INIT_COMMIT
+
+ contents =
+"#!/bin/sh" NL
+"" NL
+"# INIT-COMMIT HOOK" NL
+"#" NL
+"# The init-commit hook is invoked immediate after a Subversion commit txn" NL
+"# is created and initialized. Subversion runs this hook by invoking a" NL
+"# program (script, executable, binary, etc.) named '"SCRIPT_NAME"' (for" NL
+"# which this file is a template), with the following ordered arguments:" NL
+"#" NL
+"# [1] REPOS-PATH (the path to this repository)" NL
+"# [2] TXN-NAME (the name of the commit txn just created)" NL
+"#" NL
+"# The default working directory for the invocation is undefined, so" NL
+"# the program should set one explicitly if it cares." NL
+"#" NL
+"# If the hook program exits with success, the commit txn remains active;" NL
+"# but if it exits with failure (non-zero), the txn is aborted, no commit" NL
+"# takes place, and STDERR is returned to the client. The hook" NL
+"# program can use the 'svnlook' utility to help it examine the txn." NL
+"#" NL
+"# On a Unix system, the normal procedure is to have '"SCRIPT_NAME"'" NL
+"# invoke other programs to do the real work, though it may do the" NL
+"# work itself too." NL
+"#" NL
+"# *** NOTE: THE HOOK PROGRAM MUST NOT MODIFY THE TXN, EXCEPT ***" NL
+"# *** FOR REVISION PROPERTIES (like svn:log or svn:author). ***" NL
+"#" NL
+"# This is why we recommend using the read-only 'svnlook' utility." NL
+"# In the future, Subversion may enforce the rule that pre-commit" NL
+"# hooks should not modify the versioned data in txns, or else come" NL
+"# up with a mechanism to make it safe to do so (by informing the" NL
+"# committing client of the changes). However, right now neither" NL
+"# mechanism is implemented, so hook writers just have to be careful." NL
+"#" NL
+"# Note that '"SCRIPT_NAME"' must be executable by the user(s) who will" NL
+"# invoke it (typically the user httpd runs as), and that user must" NL
+"# have filesystem-level permission to access the repository." NL
+"#" NL
+"# On a Windows system, you should name the hook program" NL
+"# '"SCRIPT_NAME".bat' or '"SCRIPT_NAME".exe'," NL
+"# but the basic idea is the same." NL
+"#" NL
+"# WARNING: The degree of txn \"initialization\" may differ depending on" NL
+"# how the commit process is being driven. For example, some older" NL
+"# Subversion clients (notably pre-1.7 clients committing over HTTP) will" NL
+"# not have yet attached some metadata -- such as the commit log message --" NL
+"# to the txn by the time this hook runs. Hook authors should therefore" NL
+"# not assume that a txn which lacks a log message at this stage of the " NL
+"# commit will necessarily still lack a log message by the time the commit" NL
+"# completes." NL
+"#" NL
+HOOKS_ENVIRONMENT_TEXT
+"# " NL
+"# Here is an example hook script, for a Unix /bin/sh interpreter." NL
+PREWRITTEN_HOOKS_TEXT
+"" NL
+"" NL
+"REPOS=\"$1\"" NL
+"TXN=\"$2\"" NL
+"" NL
+"# If a log message is present, make sure it mentions an issue tracker id." NL
+"SVNLOOK=" SVN_BINDIR "/svnlook" NL
+"$SVNLOOK log -t \"$TXN\" \"$REPOS\" | \\" NL
+" grep \"[a-zA-Z0-9]\" > /dev/null || exit 1" NL
+"" NL
+"# All checks passed, so allow the commit to proceed." NL
+"exit 0" NL;
+
+#undef SCRIPT_NAME
+ SVN_ERR_W(svn_io_file_create(this_path, contents, pool),
+ _("Creating init-commit hook"));
+
+ SVN_ERR(svn_io_set_file_executable(this_path, TRUE, FALSE, pool));
+ } /* end init-commit hook */
+
+
/* Pre-commit hook. */
{
this_path = apr_psprintf(pool, "%s%s",
Modified: subversion/trunk/subversion/libsvn_repos/repos.h
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_repos/repos.h?rev=1375675&r1=1375674&r2=1375675&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_repos/repos.h (original)
+++ subversion/trunk/subversion/libsvn_repos/repos.h Tue Aug 21 17:29:40 2012
@@ -70,6 +70,7 @@ extern "C" {
/* In the repository hooks directory, look for these files. */
#define SVN_REPOS__HOOK_START_COMMIT "start-commit"
+#define SVN_REPOS__HOOK_INIT_COMMIT "init-commit"
#define SVN_REPOS__HOOK_PRE_COMMIT "pre-commit"
#define SVN_REPOS__HOOK_POST_COMMIT "post-commit"
#define SVN_REPOS__HOOK_READ_SENTINEL "read-sentinels"
@@ -163,6 +164,15 @@ svn_repos__hooks_start_commit(svn_repos_
const apr_array_header_t *capabilities,
apr_pool_t *pool);
+/* Run the init-commit hook for REPOS. Use POOL for any temporary
+ allocations. If the hook fails, return SVN_ERR_REPOS_HOOK_FAILURE.
+
+ TXN_NAME is the name of the transaction that is being committed. */
+svn_error_t *
+svn_repos__hooks_init_commit(svn_repos_t *repos,
+ const char *txn_name,
+ apr_pool_t *pool);
+
/* Run the pre-commit hook for REPOS. Use POOL for any temporary
allocations. If the hook fails, return SVN_ERR_REPOS_HOOK_FAILURE.
Modified: subversion/trunk/subversion/tests/cmdline/commit_tests.py
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/tests/cmdline/commit_tests.py?rev=1375675&r1=1375674&r2=1375675&view=diff
==============================================================================
--- subversion/trunk/subversion/tests/cmdline/commit_tests.py (original)
+++ subversion/trunk/subversion/tests/cmdline/commit_tests.py Tue Aug 21 17:29:40 2012
@@ -2940,7 +2940,47 @@ def commit_moved_dir_with_nested_mod_in_
'Last Changed Rev' : '2',
}
svntest.actions.run_and_verify_info([expected], E_copied)
-
+
+#----------------------------------------------------------------------
+def init_commit_hook_test(sbox):
+ "init-commit hook failure case testing"
+
+ sbox.build()
+
+ # Get paths to the working copy and repository
+ wc_dir = sbox.wc_dir
+ repo_dir = sbox.repo_dir
+
+ # Create a hook that outputs a message to stderr and returns exit code 1
+ # Include a non-XML-safe message as history shows there've been
+ # problems with such in hooks (see issue #3553).
+ error_msg = "Text with <angle brackets> & ampersand"
+ svntest.actions.create_failing_hook(repo_dir, "init-commit", error_msg)
+
+ # Modify iota just so there is something to commit.
+ iota_path = sbox.ospath('iota')
+ svntest.main.file_append(iota_path, "More stuff in iota")
+
+ # Commit, expect error code 1
+ exit_code, actual_stdout, actual_stderr = svntest.main.run_svn(
+ 1, 'ci', '--quiet', '-m', 'log msg', wc_dir)
+
+ # No stdout expected
+ svntest.verify.compare_and_display_lines('Init-commit hook test',
+ 'STDOUT', [], actual_stdout)
+
+ # Compare only the last two lines of stderr since the preceding ones
+ # contain source code file and line numbers.
+ if len(actual_stderr) > 2:
+ actual_stderr = actual_stderr[-2:]
+ expected_stderr = [ "svn: E165001: " +
+ svntest.actions.hook_failure_message('init-commit'),
+ error_msg + "\n",
+ ]
+ svntest.verify.compare_and_display_lines('Init-commit hook test',
+ 'STDERR',
+ expected_stderr, actual_stderr)
+
########################################################################
# Run the tests
@@ -3013,6 +3053,7 @@ test_list = [ None,
commit_add_subadd,
commit_danglers,
commit_moved_dir_with_nested_mod_in_subdir,
+ init_commit_hook_test,
]
if __name__ == '__main__':
Modified: subversion/trunk/subversion/tests/cmdline/svntest/actions.py
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/tests/cmdline/svntest/actions.py?rev=1375675&r1=1375674&r2=1375675&view=diff
==============================================================================
--- subversion/trunk/subversion/tests/cmdline/svntest/actions.py (original)
+++ subversion/trunk/subversion/tests/cmdline/svntest/actions.py Tue Aug 21 17:29:40 2012
@@ -1837,7 +1837,7 @@ def hook_failure_message(hook_name):
if svntest.main.options.server_minor_version < 5:
return "'%s' hook failed with error output:\n" % hook_name
else:
- if hook_name in ["start-commit", "pre-commit"]:
+ if hook_name in ["start-commit", "pre-commit", "init-commit"]:
action = "Commit"
elif hook_name == "pre-revprop-change":
action = "Revprop change"
Re: svn commit: r1375675 - in /subversion/trunk/subversion: include/svn_repos.h
libsvn_repos/fs-wrap.c libsvn_repos/hooks.c libsvn_repos/repos.c libsvn_repos/repos.h
tests/cmdline/commit_tests.py tests/cmdline/svntest/actions.py
Posted by "C. Michael Pilato" <cm...@collab.net>.
On 08/21/2012 02:02 PM, Bert Huijben wrote:
> When is this script run when committing with a non http-v2 dav client?
>
> Are the txnprops available then? Or just with the other ra layers and
> new dav clients?
The script is still run after the txn is created and the initial txnprops
set. So, you'll get svn:date and svn:author, but not svn:log or the
user-provided custom revprops. (If this is a point of concern, the hook
author can simply bounce commits on the grounds that the client isn't new
enough for its standards.)
>> +svn_error_t *
>> +svn_repos__hooks_init_commit(svn_repos_t *repos,
>> + const char *txn_name,
>> + apr_pool_t *pool)
[...]
> Should we also pass the user here?
>
> We pass it for at least some of the other hooks and I'm not sure if it is
> guaranteed always to be the same as the one stored in the transaction
> properties.
It would have *just* been set as the svn:author txnprop, but I strictly
speaking there is a race condition between the time the property is set and
the time the hook executes. If a program was able to guess the name of the
newly created transaction and change svn:author before the hook examined it,
that might be a point of concern?
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Enterprise Cloud Development
RE: svn commit: r1375675 - in /subversion/trunk/subversion: include/svn_repos.h libsvn_repos/fs-wrap.c libsvn_repos/hooks.c libsvn_repos/repos.c libsvn_repos/repos.h tests/cmdline/commit_tests.py tests/cmdline/svntest/actions.py
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: cmpilato@apache.org [mailto:cmpilato@apache.org]
> Sent: dinsdag 21 augustus 2012 19:30
> To: commits@subversion.apache.org
> Subject: svn commit: r1375675 - in /subversion/trunk/subversion:
> include/svn_repos.h libsvn_repos/fs-wrap.c libsvn_repos/hooks.c
> libsvn_repos/repos.c libsvn_repos/repos.h tests/cmdline/commit_tests.py
> tests/cmdline/svntest/actions.py
>
> Author: cmpilato
> Date: Tue Aug 21 17:29:40 2012
> New Revision: 1375675
>
> URL: http://svn.apache.org/viewvc?rev=1375675&view=rev
> Log:
> Introduce a new 'init-commit' hook script which runs immediately after
> the commit txn is created and populated with initial txnprops.
This is more about your previous patches:
When is this script run when committing with a non http-v2 dav client?
Are the txnprops available then? Or just with the other ra layers and new dav clients?
<snip>
> +svn_error_t *
> +svn_repos__hooks_init_commit(svn_repos_t *repos,
> + const char *txn_name,
> + apr_pool_t *pool)
> +{
> + const char *hook = svn_repos_init_commit_hook(repos, pool);
> + svn_boolean_t broken_link;
> +
> + if ((hook = check_hook_cmd(hook, &broken_link, pool)) && broken_link)
> + {
> + return hook_symlink_error(hook);
> + }
> + else if (hook)
> + {
> + const char *args[4];
> +
> + args[0] = hook;
> + args[1] = svn_dirent_local_style(svn_repos_path(repos, pool), pool);
> + args[2] = txn_name;
> + args[3] = NULL;
Should we also pass the user here?
We pass it for at least some of the other hooks and I'm not sure if it is guaranteed always to be the same as the one stored in the transaction properties.
Bert
RE: svn commit: r1375675 - in /subversion/trunk/subversion: include/svn_repos.h libsvn_repos/fs-wrap.c libsvn_repos/hooks.c libsvn_repos/repos.c libsvn_repos/repos.h tests/cmdline/commit_tests.py tests/cmdline/svntest/actions.py
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: cmpilato@apache.org [mailto:cmpilato@apache.org]
> Sent: dinsdag 21 augustus 2012 19:30
> To: commits@subversion.apache.org
> Subject: svn commit: r1375675 - in /subversion/trunk/subversion:
> include/svn_repos.h libsvn_repos/fs-wrap.c libsvn_repos/hooks.c
> libsvn_repos/repos.c libsvn_repos/repos.h tests/cmdline/commit_tests.py
> tests/cmdline/svntest/actions.py
>
> Author: cmpilato
> Date: Tue Aug 21 17:29:40 2012
> New Revision: 1375675
>
> URL: http://svn.apache.org/viewvc?rev=1375675&view=rev
> Log:
> Introduce a new 'init-commit' hook script which runs immediately after
> the commit txn is created and populated with initial txnprops.
This is more about your previous patches:
When is this script run when committing with a non http-v2 dav client?
Are the txnprops available then? Or just with the other ra layers and new dav clients?
<snip>
> +svn_error_t *
> +svn_repos__hooks_init_commit(svn_repos_t *repos,
> + const char *txn_name,
> + apr_pool_t *pool)
> +{
> + const char *hook = svn_repos_init_commit_hook(repos, pool);
> + svn_boolean_t broken_link;
> +
> + if ((hook = check_hook_cmd(hook, &broken_link, pool)) && broken_link)
> + {
> + return hook_symlink_error(hook);
> + }
> + else if (hook)
> + {
> + const char *args[4];
> +
> + args[0] = hook;
> + args[1] = svn_dirent_local_style(svn_repos_path(repos, pool), pool);
> + args[2] = txn_name;
> + args[3] = NULL;
Should we also pass the user here?
We pass it for at least some of the other hooks and I'm not sure if it is guaranteed always to be the same as the one stored in the transaction properties.
Bert
Re: svn commit: r1375675 - in /subversion/trunk/subversion: include/svn_repos.h
libsvn_repos/fs-wrap.c libsvn_repos/hooks.c libsvn_repos/repos.c libsvn_repos/repos.h
tests/cmdline/commit_tests.py tests/cmdline/svntest/actions.py
Posted by Branko Čibej <br...@wandisco.com>.
On 21.08.2012 20:45, Philip Martin wrote:
> Blair Zajac <bl...@orcaware.com> writes:
>
>> On 08/21/2012 11:09 AM, C. Michael Pilato wrote:
>>> I actually considered using "post-create-txn" and renaming "start-commit" to
>>> "pre-create-txn" (with code to run "start-commit" iff not "pre-create-txn"
>>> hook exists, for compat purposes).
>> +1. I always have to remember which comes first, start-commit or
>> pre-commit, so this renaming helps.
> Suppose both pre-create-txn and start-commit exist. Is it an error?
> If not which one is run?
>
> We have already bumped the FSFS format in 1.8 but we have not yet bumped
> the repos format. Perhaps we could bump and have an upgrade that
> renames the hook?
How will that conform to our compatibility guarantees? The hooks are
part of the Subversion repository API. You can't rename them.
-- Brane
--
Certified & Supported Apache Subversion Downloads:
http://www.wandisco.com/subversion/download
Re: svn commit: r1375675 - in /subversion/trunk/subversion: include/svn_repos.h
libsvn_repos/fs-wrap.c libsvn_repos/hooks.c libsvn_repos/repos.c libsvn_repos/repos.h
tests/cmdline/commit_tests.py tests/cmdline/svntest/actions.py
Posted by Blair Zajac <bl...@orcaware.com>.
On 08/21/2012 11:56 AM, C. Michael Pilato wrote:
> On 08/21/2012 02:45 PM, Philip Martin wrote:
>> Blair Zajac <bl...@orcaware.com> writes:
>>
>>> On 08/21/2012 11:09 AM, C. Michael Pilato wrote:
>>>>
>>>> I actually considered using "post-create-txn" and renaming "start-commit" to
>>>> "pre-create-txn" (with code to run "start-commit" iff not "pre-create-txn"
>>>> hook exists, for compat purposes).
>>>
>>> +1. I always have to remember which comes first, start-commit or
>>> pre-commit, so this renaming helps.
>>
>> Suppose both pre-create-txn and start-commit exist. Is it an error?
>> If not which one is run?
>>
>> We have already bumped the FSFS format in 1.8 but we have not yet bumped
>> the repos format. Perhaps we could bump and have an upgrade that
>> renames the hook?
>
> Are we comfortable with renaming the hook, which -- strictly speaking -- is
> user-managed data, not Subversion managed data? What if the hook itself is
> kept under version control (which is pretty common)? I lean against doing
> so. And because no change of administrative behavior is required (we'll
> still happily run "start-commit" if there's no "pre-txn-create"), I see no
> need for the format bump.
True, good point. Moving anything there isn't save. In my setup, we
have symlinks to a common area, which wouldn't break with a rename, but
I would be surprised.
> As to whether to flag an error if both "start-commit" and "pre-txn-create"
> exist: this makes sense. I see value in warning *someone* that the
> repository configuration is non-ideal, similarly to the error we return from
> a missing pre-revprop-change hook script ("ask the administrator to [fix
> this problem]").
A missing pre-revprop-change script will warn to users doing a propedit,
which is infrequent. But for a non-ideal hooks, do we warn all
committers? That would be annoying, but probably result in a prompt fix
by the admin ;)
Blair
Re: svn commit: r1375675 - in /subversion/trunk/subversion: include/svn_repos.h
libsvn_repos/fs-wrap.c libsvn_repos/hooks.c libsvn_repos/repos.c libsvn_repos/repos.h
tests/cmdline/commit_tests.py tests/cmdline/svntest/actions.py
Posted by "C. Michael Pilato" <cm...@collab.net>.
On 08/21/2012 02:45 PM, Philip Martin wrote:
> Blair Zajac <bl...@orcaware.com> writes:
>
>> On 08/21/2012 11:09 AM, C. Michael Pilato wrote:
>>>
>>> I actually considered using "post-create-txn" and renaming "start-commit" to
>>> "pre-create-txn" (with code to run "start-commit" iff not "pre-create-txn"
>>> hook exists, for compat purposes).
>>
>> +1. I always have to remember which comes first, start-commit or
>> pre-commit, so this renaming helps.
>
> Suppose both pre-create-txn and start-commit exist. Is it an error?
> If not which one is run?
>
> We have already bumped the FSFS format in 1.8 but we have not yet bumped
> the repos format. Perhaps we could bump and have an upgrade that
> renames the hook?
Are we comfortable with renaming the hook, which -- strictly speaking -- is
user-managed data, not Subversion managed data? What if the hook itself is
kept under version control (which is pretty common)? I lean against doing
so. And because no change of administrative behavior is required (we'll
still happily run "start-commit" if there's no "pre-txn-create"), I see no
need for the format bump.
As to whether to flag an error if both "start-commit" and "pre-txn-create"
exist: this makes sense. I see value in warning *someone* that the
repository configuration is non-ideal, similarly to the error we return from
a missing pre-revprop-change hook script ("ask the administrator to [fix
this problem]").
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Enterprise Cloud Development
Re: svn commit: r1375675 - in /subversion/trunk/subversion: include/svn_repos.h libsvn_repos/fs-wrap.c libsvn_repos/hooks.c libsvn_repos/repos.c libsvn_repos/repos.h tests/cmdline/commit_tests.py tests/cmdline/svntest/actions.py
Posted by Philip Martin <ph...@wandisco.com>.
Blair Zajac <bl...@orcaware.com> writes:
> On 08/21/2012 11:45 AM, Philip Martin wrote:
>> Blair Zajac <bl...@orcaware.com> writes:
>
>> We have already bumped the FSFS format in 1.8 but we have not yet bumped
>> the repos format. Perhaps we could bump and have an upgrade that
>> renames the hook?
>
> That would be make repos maintenance easier in the long run.
It also means that the pre-create-txn cannot be bypassed by using older
Subversion.
--
Certified & Supported Apache Subversion Downloads:
http://www.wandisco.com/subversion/download
Re: svn commit: r1375675 - in /subversion/trunk/subversion: include/svn_repos.h
libsvn_repos/fs-wrap.c libsvn_repos/hooks.c libsvn_repos/repos.c libsvn_repos/repos.h
tests/cmdline/commit_tests.py tests/cmdline/svntest/actions.py
Posted by Blair Zajac <bl...@orcaware.com>.
On 08/21/2012 11:45 AM, Philip Martin wrote:
> Blair Zajac <bl...@orcaware.com> writes:
>
>> On 08/21/2012 11:09 AM, C. Michael Pilato wrote:
>>>
>>> I actually considered using "post-create-txn" and renaming "start-commit" to
>>> "pre-create-txn" (with code to run "start-commit" iff not "pre-create-txn"
>>> hook exists, for compat purposes).
>>
>> +1. I always have to remember which comes first, start-commit or
>> pre-commit, so this renaming helps.
>
> Suppose both pre-create-txn and start-commit exist. Is it an error?
> If not which one is run?
I don't think it's an error, run both of them. I don't have any
thoughts on the ordering, perhaps start-commit first, since it's always
been there?
> We have already bumped the FSFS format in 1.8 but we have not yet bumped
> the repos format. Perhaps we could bump and have an upgrade that
> renames the hook?
That would be make repos maintenance easier in the long run.
Blair
Re: svn commit: r1375675 - in /subversion/trunk/subversion: include/svn_repos.h libsvn_repos/fs-wrap.c libsvn_repos/hooks.c libsvn_repos/repos.c libsvn_repos/repos.h tests/cmdline/commit_tests.py tests/cmdline/svntest/actions.py
Posted by Philip Martin <ph...@wandisco.com>.
Blair Zajac <bl...@orcaware.com> writes:
> On 08/21/2012 11:09 AM, C. Michael Pilato wrote:
>>
>> I actually considered using "post-create-txn" and renaming "start-commit" to
>> "pre-create-txn" (with code to run "start-commit" iff not "pre-create-txn"
>> hook exists, for compat purposes).
>
> +1. I always have to remember which comes first, start-commit or
> pre-commit, so this renaming helps.
Suppose both pre-create-txn and start-commit exist. Is it an error?
If not which one is run?
We have already bumped the FSFS format in 1.8 but we have not yet bumped
the repos format. Perhaps we could bump and have an upgrade that
renames the hook?
--
Certified & Supported Apache Subversion Downloads:
http://www.wandisco.com/subversion/download
Re: svn commit: r1375675 - in /subversion/trunk/subversion: include/svn_repos.h
libsvn_repos/fs-wrap.c libsvn_repos/hooks.c libsvn_repos/repos.c libsvn_repos/repos.h
tests/cmdline/commit_tests.py tests/cmdline/svntest/actions.py
Posted by "C. Michael Pilato" <cm...@collab.net>.
On 08/21/2012 03:41 PM, Branko Čibej wrote:
> Seriously though: How will the presence of revprops affect the decision
> to reject a commit? If based on svn:author, we already have a better way
> in place. What kind of other questions are relevant?
The one that seems most common is to reject commits whose log messages don't
conform to some ruleset. Since we lack a log message template feature, we
advise folks to simply bounce non-conforming commits with the log template
embedded in the bounce message. Subversion is also integrated into workflow
systems that may demand that an issue tracker ID be associated with every
commit -- again, something that can't be checked today until the whole
commit crosses the wire. Some releases ago (1.5?) we made extra effort to
allow folks to attach custom revprops to their commits as part of the
process (rather than as a follow-up, post-commit thing): any validation of
those things must necessarily wait until pre-commit today as well.
We have all kinds of just-in-time checks that are part of our commit
process: just-in-time out-of-dateness checks (this is why we do postfix
text deltas), just-in-time lock token checks, etc. It seems like a
deficiency that we have no just-in-time metadata checks, too. And even
though this is isn't part of the codified roadmap yet, it seems reasonable
to look to the future where we've discussed having clients pre-announce the
to-be-modified files before sending even the first bit of real commit data
across the wire and realize that we'd want a hook in place that could do
something with that information to, again, prevent unnecessary additional
wire transfer.
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Enterprise Cloud Development
Re: svn commit: r1375675 - in /subversion/trunk/subversion:
include/svn_repos.h libsvn_repos/fs-wrap.c libsvn_repos/hooks.c
libsvn_repos/repos.c libsvn_repos/repos.h tests/cmdline/commit_tests.py tests/cmdline/svntest/actions.py
Posted by Mark Phippard <ma...@gmail.com>.
On Tue, Aug 21, 2012 at 3:41 PM, Branko Čibej <br...@wandisco.com> wrote:
> Seriously though: How will the presence of revprops affect the decision
> to reject a commit? If based on svn:author, we already have a better way
> in place. What kind of other questions are relevant?
My understanding is that Mike proposed properties because he could not
figure out a good way to jam the client version into the capabilities
strings. I think this led him to remember that people have often
wished they could reject a commit based on svn:log BEFORE the client
wasted time sending all the content to the server.
FWIW, I basically agree with Mike that these are both high-value,
low-cost features that users have consistently asked for.
--
Thanks
Mark Phippard
http://markphip.blogspot.com/
Re: svn commit: r1375675 - in /subversion/trunk/subversion: include/svn_repos.h
libsvn_repos/fs-wrap.c libsvn_repos/hooks.c libsvn_repos/repos.c libsvn_repos/repos.h
tests/cmdline/commit_tests.py tests/cmdline/svntest/actions.py
Posted by Branko Čibej <br...@wandisco.com>.
On 21.08.2012 21:29, C. Michael Pilato wrote:
> On 08/21/2012 03:14 PM, Branko Čibej wrote:
>> On 21.08.2012 21:07, Mark Phippard wrote:
>>> I am also not crazy about introducing a bunch of new hooks that are
>>> all more or less what the original start-commit hook was doing.
>> Have to agree here. I'm a bit worried that we're trying to introduce
>> features too specific to what some vendor's client requests. This
>> started off with finding ways to communicate the client version to the
>> server (doubtful, but marginally acceptable) and suddenly we're talking
>> about a new hook which, among other things, would set in stone the
>> process that the server must follow during a commit.
>>
>> I'm not at all sure that enough thought has been put into the idea.
> While it is the case that client-version stuff stirred my creativity towards
> this change, I want to be clear that I didn't actually decide to make the
> change for that reason. I made the change to introduce the hook because
> when I mentioned the possibility of blocking a commit destined to fail
> before a gig of data was transmitted across the wire, I got feedback from
> several folks who were singing the praises of the idea.
Ah well, people are going to sing praises to all sorts of things.
> for the record, I
> still am not sure that the client-version stuff is best approached using
> txnprops. (But thanks anyway for the public implication that I'm merely a
> hive-mind hacker.)
Heh. I know I've been guilty of that kind of thinking every so often, so
welcome to the club. :)
Seriously though: How will the presence of revprops affect the decision
to reject a commit? If based on svn:author, we already have a better way
in place. What kind of other questions are relevant?
-- Brane
--
Certified & Supported Apache Subversion Downloads:
http://www.wandisco.com/subversion/download
Re: svn commit: r1375675 - in /subversion/trunk/subversion:
include/svn_repos.h libsvn_repos/fs-wrap.c libsvn_repos/hooks.c
libsvn_repos/repos.c libsvn_repos/repos.h tests/cmdline/commit_tests.py tests/cmdline/svntest/actions.py
Posted by Johan Corveleyn <jc...@gmail.com>.
On Tue, Aug 21, 2012 at 11:22 PM, Branko Čibej <br...@wandisco.com> wrote:
> On 21.08.2012 22:28, Johan Corveleyn wrote:
>> On Tue, Aug 21, 2012 at 9:52 PM, Mark Phippard <ma...@gmail.com> wrote:
>>> On Tue, Aug 21, 2012 at 3:46 PM, Branko Čibej <br...@wandisco.com> wrote:
>>>> On 21.08.2012 21:37, Mark Phippard wrote:
>> ...
>>>>> People could do a lot with that, and if
>>>>> it is easier to attach the client version to the txn properties, then
>>>>> that is another good reason.
>>>>>
>>>>> IMO, we have had a LOT of users@ requests for the client version in
>>>>> hook scripts.
>>>> Yeah, well, if these requests can also define what the client version
>>>> actually is, we can start thinking about a solution. In the meantime,
>>>> I'd prefer this information to not be mixed with revision properties;
>>>> not least because I can't think of a way of preventing older servers
>>>> from just writing such props into the repository.
>>> Mike would have to answer that, but ISTR that the client/server
>>> negotiation kept the client from sending this information. So older
>>> servers would not get the props.
>> Actually, as an svn admin, I wouldn't mind having the version revprops
>> written to the repository. That can be very interesting information,
>> especially if you're diagnosing some kind of problem, trying to find a
>> pattern, pinpointing it to particular clients, ...
>>
>> Last year I had a couple of situations where I really wanted to find
>> out what client (and client-version) had performed a particular
>> commit, or trying to find a common pattern between various occurrences
>> of strangeness (mostly related to issue #4065 [1], which was first
>> "broken" by some git-svn clients, and then later on by some versions
>> of svnkit).
>>
>> But that might be a rather exceptional use case, so not sure if it's
>> worth it ...
>
> That sort of thing belongs in server logs, not in the repository.
Ah, true, that would be even better.
--
Johan
Re: svn commit: r1375675 - in /subversion/trunk/subversion: include/svn_repos.h
libsvn_repos/fs-wrap.c libsvn_repos/hooks.c libsvn_repos/repos.c libsvn_repos/repos.h
tests/cmdline/commit_tests.py tests/cmdline/svntest/actions.py
Posted by Branko Čibej <br...@wandisco.com>.
On 21.08.2012 22:28, Johan Corveleyn wrote:
> On Tue, Aug 21, 2012 at 9:52 PM, Mark Phippard <ma...@gmail.com> wrote:
>> On Tue, Aug 21, 2012 at 3:46 PM, Branko Čibej <br...@wandisco.com> wrote:
>>> On 21.08.2012 21:37, Mark Phippard wrote:
> ...
>>>> People could do a lot with that, and if
>>>> it is easier to attach the client version to the txn properties, then
>>>> that is another good reason.
>>>>
>>>> IMO, we have had a LOT of users@ requests for the client version in
>>>> hook scripts.
>>> Yeah, well, if these requests can also define what the client version
>>> actually is, we can start thinking about a solution. In the meantime,
>>> I'd prefer this information to not be mixed with revision properties;
>>> not least because I can't think of a way of preventing older servers
>>> from just writing such props into the repository.
>> Mike would have to answer that, but ISTR that the client/server
>> negotiation kept the client from sending this information. So older
>> servers would not get the props.
> Actually, as an svn admin, I wouldn't mind having the version revprops
> written to the repository. That can be very interesting information,
> especially if you're diagnosing some kind of problem, trying to find a
> pattern, pinpointing it to particular clients, ...
>
> Last year I had a couple of situations where I really wanted to find
> out what client (and client-version) had performed a particular
> commit, or trying to find a common pattern between various occurrences
> of strangeness (mostly related to issue #4065 [1], which was first
> "broken" by some git-svn clients, and then later on by some versions
> of svnkit).
>
> But that might be a rather exceptional use case, so not sure if it's
> worth it ...
That sort of thing belongs in server logs, not in the repository.
-- Brane
--
Certified & Supported Apache Subversion Downloads:
http://www.wandisco.com/subversion/download
Re: svn commit: r1375675 - in /subversion/trunk/subversion:
include/svn_repos.h libsvn_repos/fs-wrap.c libsvn_repos/hooks.c
libsvn_repos/repos.c libsvn_repos/repos.h tests/cmdline/commit_tests.py tests/cmdline/svntest/actions.py
Posted by Johan Corveleyn <jc...@gmail.com>.
On Tue, Aug 21, 2012 at 9:52 PM, Mark Phippard <ma...@gmail.com> wrote:
> On Tue, Aug 21, 2012 at 3:46 PM, Branko Čibej <br...@wandisco.com> wrote:
>> On 21.08.2012 21:37, Mark Phippard wrote:
...
>>> People could do a lot with that, and if
>>> it is easier to attach the client version to the txn properties, then
>>> that is another good reason.
>>>
>>> IMO, we have had a LOT of users@ requests for the client version in
>>> hook scripts.
>>
>> Yeah, well, if these requests can also define what the client version
>> actually is, we can start thinking about a solution. In the meantime,
>> I'd prefer this information to not be mixed with revision properties;
>> not least because I can't think of a way of preventing older servers
>> from just writing such props into the repository.
>
> Mike would have to answer that, but ISTR that the client/server
> negotiation kept the client from sending this information. So older
> servers would not get the props.
Actually, as an svn admin, I wouldn't mind having the version revprops
written to the repository. That can be very interesting information,
especially if you're diagnosing some kind of problem, trying to find a
pattern, pinpointing it to particular clients, ...
Last year I had a couple of situations where I really wanted to find
out what client (and client-version) had performed a particular
commit, or trying to find a common pattern between various occurrences
of strangeness (mostly related to issue #4065 [1], which was first
"broken" by some git-svn clients, and then later on by some versions
of svnkit).
But that might be a rather exceptional use case, so not sure if it's
worth it ...
[1] http://subversion.tigris.org/issues/show_bug.cgi?id=4065 (server
should enforce LF normalization for svn:eol-style=native files)
--
Johan
Re: svn commit: r1375675 - in /subversion/trunk/subversion:
include/svn_repos.h libsvn_repos/fs-wrap.c libsvn_repos/hooks.c
libsvn_repos/repos.c libsvn_repos/repos.h tests/cmdline/commit_tests.py tests/cmdline/svntest/actions.py
Posted by Mark Phippard <ma...@gmail.com>.
On Tue, Aug 21, 2012 at 3:46 PM, Branko Čibej <br...@wandisco.com> wrote:
> On 21.08.2012 21:37, Mark Phippard wrote:
>> On Tue, Aug 21, 2012 at 3:29 PM, C. Michael Pilato <cm...@collab.net> wrote:
>> FWIW, I agree it would be nice to have access to svn:log before the
>> data is sent from the client.
>
> I expect this would mean checking for log-file format conformance?
Yes. Users use the log file for lots of things that they might
validate in the pre-commit hook today. Could be conforming to some
kind of template or format, but it often is also something like
requiring a link to a bug ID, or some kind of "signed-off" by line
etc.
>> People could do a lot with that, and if
>> it is easier to attach the client version to the txn properties, then
>> that is another good reason.
>>
>> IMO, we have had a LOT of users@ requests for the client version in
>> hook scripts.
>
> Yeah, well, if these requests can also define what the client version
> actually is, we can start thinking about a solution. In the meantime,
> I'd prefer this information to not be mixed with revision properties;
> not least because I can't think of a way of preventing older servers
> from just writing such props into the repository.
Mike would have to answer that, but ISTR that the client/server
negotiation kept the client from sending this information. So older
servers would not get the props.
--
Thanks
Mark Phippard
http://markphip.blogspot.com/
Re: svn commit: r1375675 - in /subversion/trunk/subversion: include/svn_repos.h
libsvn_repos/fs-wrap.c libsvn_repos/hooks.c libsvn_repos/repos.c libsvn_repos/repos.h
tests/cmdline/commit_tests.py tests/cmdline/svntest/actions.py
Posted by Branko Čibej <br...@wandisco.com>.
On 21.08.2012 21:37, Mark Phippard wrote:
> On Tue, Aug 21, 2012 at 3:29 PM, C. Michael Pilato <cm...@collab.net> wrote:
>
>> If the community decides to simply modify start-commit to run *after* the
>> txn gets created and populated with txnprops, that's fine with me. I
>> assumed that that change was undesirable due to the fact mentioned
>> elsethread about how start-commit would no longer be useful for keeping a
>> would-be read-only repository read-only.
> start-commit does not currently receive the txn-id, so I do not see
> how we can use/move it for svn:log access anyway. How about a single
> new hook called something like start-commit-txn or something like
> that?
>
> FWIW, I agree it would be nice to have access to svn:log before the
> data is sent from the client.
I expect this would mean checking for log-file format conformance?
> People could do a lot with that, and if
> it is easier to attach the client version to the txn properties, then
> that is another good reason.
>
> IMO, we have had a LOT of users@ requests for the client version in
> hook scripts.
Yeah, well, if these requests can also define what the client version
actually is, we can start thinking about a solution. In the meantime,
I'd prefer this information to not be mixed with revision properties;
not least because I can't think of a way of preventing older servers
from just writing such props into the repository.
-- Brane
--
Certified & Supported Apache Subversion Downloads:
http://www.wandisco.com/subversion/download
Re: svn commit: r1375675 - in /subversion/trunk/subversion: include/svn_repos.h
libsvn_repos/fs-wrap.c libsvn_repos/hooks.c libsvn_repos/repos.c libsvn_repos/repos.h
tests/cmdline/commit_tests.py tests/cmdline/svntest/actions.py
Posted by "C. Michael Pilato" <cm...@collab.net>.
On 08/21/2012 03:37 PM, Mark Phippard wrote:
> On Tue, Aug 21, 2012 at 3:29 PM, C. Michael Pilato <cm...@collab.net> wrote:
>
>> If the community decides to simply modify start-commit to run *after* the
>> txn gets created and populated with txnprops, that's fine with me. I
>> assumed that that change was undesirable due to the fact mentioned
>> elsethread about how start-commit would no longer be useful for keeping a
>> would-be read-only repository read-only.
>
> start-commit does not currently receive the txn-id, so I do not see
> how we can use/move it for svn:log access anyway. How about a single
> new hook called something like start-commit-txn or something like
> that?
We've added parameters to the end of a hook before (ACTION was added to
post-revprop-change in Subversion 1.2.0), so I figured we'd just tack txn-id
onto the end of the current start-txn argument list. Existing scripts won't
care about the extra arg; new or updated scripts can use it to do the
svn:log checks.
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Enterprise Cloud Development
Re: svn commit: r1375675 - in /subversion/trunk/subversion:
include/svn_repos.h libsvn_repos/fs-wrap.c libsvn_repos/hooks.c
libsvn_repos/repos.c libsvn_repos/repos.h tests/cmdline/commit_tests.py tests/cmdline/svntest/actions.py
Posted by Mark Phippard <ma...@gmail.com>.
On Tue, Aug 21, 2012 at 3:29 PM, C. Michael Pilato <cm...@collab.net> wrote:
> If the community decides to simply modify start-commit to run *after* the
> txn gets created and populated with txnprops, that's fine with me. I
> assumed that that change was undesirable due to the fact mentioned
> elsethread about how start-commit would no longer be useful for keeping a
> would-be read-only repository read-only.
start-commit does not currently receive the txn-id, so I do not see
how we can use/move it for svn:log access anyway. How about a single
new hook called something like start-commit-txn or something like
that?
FWIW, I agree it would be nice to have access to svn:log before the
data is sent from the client. People could do a lot with that, and if
it is easier to attach the client version to the txn properties, then
that is another good reason.
IMO, we have had a LOT of users@ requests for the client version in
hook scripts.
--
Thanks
Mark Phippard
http://markphip.blogspot.com/
Re: svn commit: r1375675 - in /subversion/trunk/subversion: include/svn_repos.h
libsvn_repos/fs-wrap.c libsvn_repos/hooks.c libsvn_repos/repos.c libsvn_repos/repos.h
tests/cmdline/commit_tests.py tests/cmdline/svntest/actions.py
Posted by "C. Michael Pilato" <cm...@collab.net>.
On 08/21/2012 03:14 PM, Branko Čibej wrote:
> On 21.08.2012 21:07, Mark Phippard wrote:
>> I am also not crazy about introducing a bunch of new hooks that are
>> all more or less what the original start-commit hook was doing.
>
> Have to agree here. I'm a bit worried that we're trying to introduce
> features too specific to what some vendor's client requests. This
> started off with finding ways to communicate the client version to the
> server (doubtful, but marginally acceptable) and suddenly we're talking
> about a new hook which, among other things, would set in stone the
> process that the server must follow during a commit.
>
> I'm not at all sure that enough thought has been put into the idea.
While it is the case that client-version stuff stirred my creativity towards
this change, I want to be clear that I didn't actually decide to make the
change for that reason. I made the change to introduce the hook because
when I mentioned the possibility of blocking a commit destined to fail
before a gig of data was transmitted across the wire, I got feedback from
several folks who were singing the praises of the idea. for the record, I
still am not sure that the client-version stuff is best approached using
txnprops. (But thanks anyway for the public implication that I'm merely a
hive-mind hacker.)
If the community decides to simply modify start-commit to run *after* the
txn gets created and populated with txnprops, that's fine with me. I
assumed that that change was undesirable due to the fact mentioned
elsethread about how start-commit would no longer be useful for keeping a
would-be read-only repository read-only.
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Enterprise Cloud Development
Re: svn commit: r1375675 - in /subversion/trunk/subversion: include/svn_repos.h
libsvn_repos/fs-wrap.c libsvn_repos/hooks.c libsvn_repos/repos.c libsvn_repos/repos.h
tests/cmdline/commit_tests.py tests/cmdline/svntest/actions.py
Posted by Branko Čibej <br...@wandisco.com>.
On 21.08.2012 21:07, Mark Phippard wrote:
> I am also not crazy about introducing a bunch of new hooks that are
> all more or less what the original start-commit hook was doing.
Have to agree here. I'm a bit worried that we're trying to introduce
features too specific to what some vendor's client requests. This
started off with finding ways to communicate the client version to the
server (doubtful, but marginally acceptable) and suddenly we're talking
about a new hook which, among other things, would set in stone the
process that the server must follow during a commit.
I'm not at all sure that enough thought has been put into the idea.
-- Brane
--
Certified & Supported Apache Subversion Downloads:
http://www.wandisco.com/subversion/download
Re: svn commit: r1375675 - in /subversion/trunk/subversion:
include/svn_repos.h libsvn_repos/fs-wrap.c libsvn_repos/hooks.c
libsvn_repos/repos.c libsvn_repos/repos.h tests/cmdline/commit_tests.py tests/cmdline/svntest/actions.py
Posted by Mark Phippard <ma...@gmail.com>.
On Tue, Aug 21, 2012 at 3:00 PM, C. Michael Pilato <cm...@collab.net> wrote:
> On 08/21/2012 02:55 PM, Mark Phippard wrote:
>> On Tue, Aug 21, 2012 at 2:38 PM, C. Michael Pilato <cm...@collab.net> wrote:
>>> On 08/21/2012 02:29 PM, Blair Zajac wrote:
>>>>> I actually considered using "post-create-txn" and renaming "start-commit" to
>>>>> "pre-create-txn" (with code to run "start-commit" iff not "pre-create-txn"
>>>>> hook exists, for compat purposes).
>>>>
>>>> +1. I always have to remember which comes first, start-commit or
>>>> pre-commit, so this renaming helps.
>>>
>>> Cool. Will make that my next set of changes, then. Thanks for the feedback.
>>
>> Who besides a SVN developer would know that pre/post create-txn comes
>> before pre-commit? I do not see how these names are an improvement.
>> I also do not think start-commit, pre-commit were all that confusing
>> to end users. The confusing part is always just understanding what
>> you can do in the hooks.
>
> Hook authorship falls to admins, not mere users. And I think it is fair to
> say that we expect admins to understand the notion of commit transactions as
> "revisions-to-be". In fact, we expose this nomenclature readily through the
> 'svnlook -t TXN_NAME ...' interfaces. You pretty much can't write a
> pre-commit hook script without "getting" the concept of these 'txn' things.
I am simply pointing out that anyone who believes these new names
makes things crystal clear are too close to the underlying code. In
particular as an admin and hook author I would have no reason to
expect that a post-create-txn hook only contains a small subset of the
data that will ultimately go into the transaction. I imagine this is
why you initially were leaning towards "init-txn". I think of
creating the txn as also populating things like the list of files and
likely the data as well. AFAIK, you are proposing to call the hook
after only the basics of the txn have been established.
I am not proposing another hook with this additional data either.
Just trying to inject some skepticism into how clear these names
really make things. In an ideal world, I would have a better name to
propose. I realize that.
I am also not crazy about introducing a bunch of new hooks that are
all more or less what the original start-commit hook was doing.
> One downside of changing the behavior of the start-commit hook is that it
> will no longer be useful for making a repository effectively read-only in
> the way that it is today (in concert with restrictive pre-revprop-change,
> pre-lock, and pre-unlock hooks).
Fair enough.
--
Thanks
Mark Phippard
http://markphip.blogspot.com/
Re: svn commit: r1375675 - in /subversion/trunk/subversion: include/svn_repos.h
libsvn_repos/fs-wrap.c libsvn_repos/hooks.c libsvn_repos/repos.c libsvn_repos/repos.h
tests/cmdline/commit_tests.py tests/cmdline/svntest/actions.py
Posted by "C. Michael Pilato" <cm...@collab.net>.
On 08/21/2012 02:55 PM, Mark Phippard wrote:
> On Tue, Aug 21, 2012 at 2:38 PM, C. Michael Pilato <cm...@collab.net> wrote:
>> On 08/21/2012 02:29 PM, Blair Zajac wrote:
>>>> I actually considered using "post-create-txn" and renaming "start-commit" to
>>>> "pre-create-txn" (with code to run "start-commit" iff not "pre-create-txn"
>>>> hook exists, for compat purposes).
>>>
>>> +1. I always have to remember which comes first, start-commit or
>>> pre-commit, so this renaming helps.
>>
>> Cool. Will make that my next set of changes, then. Thanks for the feedback.
>
> Who besides a SVN developer would know that pre/post create-txn comes
> before pre-commit? I do not see how these names are an improvement.
> I also do not think start-commit, pre-commit were all that confusing
> to end users. The confusing part is always just understanding what
> you can do in the hooks.
Hook authorship falls to admins, not mere users. And I think it is fair to
say that we expect admins to understand the notion of commit transactions as
"revisions-to-be". In fact, we expose this nomenclature readily through the
'svnlook -t TXN_NAME ...' interfaces. You pretty much can't write a
pre-commit hook script without "getting" the concept of these 'txn' things.
> Did you ever say why you ruled out your earlier idea of simply moving
> where the start-commit hook was called to later in the process where
> more information would be available?
>
> If we have a new hook, I do not have an obvious idea as to better new
> name. Maybe init-commit just so that the link to the commit process
> is being maintained. Then we would have to document the order is
> start > init > pre > post
>
> It seems like moving the start-commit hook would be easiest option and
> I do not immediately see how it would break existing hooks. So I do
> not see the downside to that option.
One downside of changing the behavior of the start-commit hook is that it
will no longer be useful for making a repository effectively read-only in
the way that it is today (in concert with restrictive pre-revprop-change,
pre-lock, and pre-unlock hooks).
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Enterprise Cloud Development
Re: svn commit: r1375675 - in /subversion/trunk/subversion:
include/svn_repos.h libsvn_repos/fs-wrap.c libsvn_repos/hooks.c
libsvn_repos/repos.c libsvn_repos/repos.h tests/cmdline/commit_tests.py tests/cmdline/svntest/actions.py
Posted by Mark Phippard <ma...@gmail.com>.
On Tue, Aug 21, 2012 at 2:38 PM, C. Michael Pilato <cm...@collab.net> wrote:
> On 08/21/2012 02:29 PM, Blair Zajac wrote:
>>> I actually considered using "post-create-txn" and renaming "start-commit" to
>>> "pre-create-txn" (with code to run "start-commit" iff not "pre-create-txn"
>>> hook exists, for compat purposes).
>>
>> +1. I always have to remember which comes first, start-commit or
>> pre-commit, so this renaming helps.
>
> Cool. Will make that my next set of changes, then. Thanks for the feedback.
Who besides a SVN developer would know that pre/post create-txn comes
before pre-commit? I do not see how these names are an improvement.
I also do not think start-commit, pre-commit were all that confusing
to end users. The confusing part is always just understanding what
you can do in the hooks.
Did you ever say why you ruled out your earlier idea of simply moving
where the start-commit hook was called to later in the process where
more information would be available?
If we have a new hook, I do not have an obvious idea as to better new
name. Maybe init-commit just so that the link to the commit process
is being maintained. Then we would have to document the order is
start > init > pre > post
It seems like moving the start-commit hook would be easiest option and
I do not immediately see how it would break existing hooks. So I do
not see the downside to that option.
--
Thanks
Mark Phippard
http://markphip.blogspot.com/
Re: svn commit: r1375675 - in /subversion/trunk/subversion: include/svn_repos.h
libsvn_repos/fs-wrap.c libsvn_repos/hooks.c libsvn_repos/repos.c libsvn_repos/repos.h
tests/cmdline/commit_tests.py tests/cmdline/svntest/actions.py
Posted by "C. Michael Pilato" <cm...@collab.net>.
On 08/21/2012 02:29 PM, Blair Zajac wrote:
>> I actually considered using "post-create-txn" and renaming "start-commit" to
>> "pre-create-txn" (with code to run "start-commit" iff not "pre-create-txn"
>> hook exists, for compat purposes).
>
> +1. I always have to remember which comes first, start-commit or
> pre-commit, so this renaming helps.
Cool. Will make that my next set of changes, then. Thanks for the feedback.
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Enterprise Cloud Development
Re: svn commit: r1375675 - in /subversion/trunk/subversion: include/svn_repos.h
libsvn_repos/fs-wrap.c libsvn_repos/hooks.c libsvn_repos/repos.c libsvn_repos/repos.h
tests/cmdline/commit_tests.py tests/cmdline/svntest/actions.py
Posted by Blair Zajac <bl...@orcaware.com>.
On 08/21/2012 11:09 AM, C. Michael Pilato wrote:
> On 08/21/2012 01:54 PM, Blair Zajac wrote:
>> On 08/21/2012 10:29 AM, cmpilato@apache.org wrote:
>>> Author: cmpilato
>>> Date: Tue Aug 21 17:29:40 2012
>>> New Revision: 1375675
>>>
>>> URL: http://svn.apache.org/viewvc?rev=1375675&view=rev
>>> Log:
>>> Introduce a new 'init-commit' hook script which runs immediately after
>>> the commit txn is created and populated with initial txnprops.
>>
>> I'm curious where this is used. This is just after the txn is begun but
>> before any modifications are made in it?
>
> That's correct. Well, not before *any* modifications are made. The
> libsvn_repos-level code which creates commit transactions also sets a bunch
> of txnprops (author, date, plus any user-supplied ones such as log messages
> or custom revprops) on the newly created transaction. This hook runs
> immediately after that initial batch of txnprop changes is made.
Is the use case it to check the txn props that aren't available in
start-commit?
>> Calling this "init" isn't self documenting, it's not clear where in the
>> commit steps it occurs. It doesn't fit in the "pre-" or "post-" convention
>> where it's clear where it runs in the order.
>
> I agree that the name isn't ideal.
>
>> Calling this "post-create", "post-txn-create" would be a clearer name. Given
>> this hook is infrequently used, I would prefer the later.
>
> I actually considered using "post-create-txn" and renaming "start-commit" to
> "pre-create-txn" (with code to run "start-commit" iff not "pre-create-txn"
> hook exists, for compat purposes).
+1. I always have to remember which comes first, start-commit or
pre-commit, so this renaming helps.
Blair
>
> Any thoughts on that?
>
Re: svn commit: r1375675 - in /subversion/trunk/subversion: include/svn_repos.h
libsvn_repos/fs-wrap.c libsvn_repos/hooks.c libsvn_repos/repos.c libsvn_repos/repos.h
tests/cmdline/commit_tests.py tests/cmdline/svntest/actions.py
Posted by "C. Michael Pilato" <cm...@collab.net>.
On 08/21/2012 01:54 PM, Blair Zajac wrote:
> On 08/21/2012 10:29 AM, cmpilato@apache.org wrote:
>> Author: cmpilato
>> Date: Tue Aug 21 17:29:40 2012
>> New Revision: 1375675
>>
>> URL: http://svn.apache.org/viewvc?rev=1375675&view=rev
>> Log:
>> Introduce a new 'init-commit' hook script which runs immediately after
>> the commit txn is created and populated with initial txnprops.
>
> I'm curious where this is used. This is just after the txn is begun but
> before any modifications are made in it?
That's correct. Well, not before *any* modifications are made. The
libsvn_repos-level code which creates commit transactions also sets a bunch
of txnprops (author, date, plus any user-supplied ones such as log messages
or custom revprops) on the newly created transaction. This hook runs
immediately after that initial batch of txnprop changes is made.
> Calling this "init" isn't self documenting, it's not clear where in the
> commit steps it occurs. It doesn't fit in the "pre-" or "post-" convention
> where it's clear where it runs in the order.
I agree that the name isn't ideal.
> Calling this "post-create", "post-txn-create" would be a clearer name. Given
> this hook is infrequently used, I would prefer the later.
I actually considered using "post-create-txn" and renaming "start-commit" to
"pre-create-txn" (with code to run "start-commit" iff not "pre-create-txn"
hook exists, for compat purposes).
Any thoughts on that?
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Enterprise Cloud Development
Re: svn commit: r1375675 - in /subversion/trunk/subversion: include/svn_repos.h
libsvn_repos/fs-wrap.c libsvn_repos/hooks.c libsvn_repos/repos.c libsvn_repos/repos.h
tests/cmdline/commit_tests.py tests/cmdline/svntest/actions.py
Posted by Blair Zajac <bl...@orcaware.com>.
On 08/21/2012 10:29 AM, cmpilato@apache.org wrote:
> Author: cmpilato
> Date: Tue Aug 21 17:29:40 2012
> New Revision: 1375675
>
> URL: http://svn.apache.org/viewvc?rev=1375675&view=rev
> Log:
> Introduce a new 'init-commit' hook script which runs immediately after
> the commit txn is created and populated with initial txnprops.
I'm curious where this is used. This is just after the txn is begun but
before any modifications are made in it?
Calling this "init" isn't self documenting, it's not clear where in the
commit steps it occurs. It doesn't fit in the "pre-" or "post-"
convention where it's clear where it runs in the order.
Calling this "post-create", "post-txn-create" would be a clearer name.
Given this hook is infrequently used, I would prefer the later.
Blair