This is a patch for issue #1601. It makes Subversion do deltification
automatically again, instead of from the post-commit hook. It
compiles and passes basic hand tests. I haven't finished running
'make check' yet, but wanted to get this in front of Old Hundred Eyes
as soon as possible, so here it is.
Greg Hudson, is it too conservative in the svnserve case? It
deltifies after responding to the client only in daemon mode -- all
other modes deltify before responding. This seems right, even for
(say) the inetd case, but maybe I'm being paranoid? Let me know what
you think.
-Karl
--------------------8-<-------cut-here---------8-<-----------------------
Fix issue #1601: do deltification in core code instead of in
post-commit hook, but (where possible) deltify after responding to the
client, to avoid user-visible delays.
Thanks to Greg Hudson and Mike Pilato for assistance.
Changes to svnserve:
* subversion/svnserve/server.h
(serve): Add new boolean parameter 'daemon'.
* subversion/svnserve/serve.c
(server_baton_t): New boolean field 'daemon'.
(serve): Store new 'daemon' argument in baton.
(commit): Run deltification before or after sending the response,
depending on b->daemon.
* subversion/svnserve/main.c
(stuct serve_thread_t): New boolean field 'daemon'.
(serve_thread): Pass daemon boolean along to serve.
(main): Set thread_data->daemon to daemon_mode, and pass daemon_mode
to serve() where calling it directly.
Changes to mod_dav_svn:
* subversion/mod_dav_svn/version.c
(struct cleanup_deltify_baton): New baton type for below.
(cleanup_deltify): New pool cleanup function.
(register_deltification_cleanup): New function to register above.
(dav_svn_checkin, dav_svn_merge): Register deltification cleanup.
Changes to ra_local:
* subversion/libsvn_ra_local/ra_plugin.c
(struct deltify_etc_baton): New baton type.
(deltify_etc): New function, wraps original commit callback.
(svn_ra_local__get_commit_editor): Initialize a deltify_etc_baton,
use deltify_etc/baton in place of original callback/baton.
Tweak hook template to no longer suggest deltification:
* subversion/libsvn_repos/repos.c
(create_hooks): Remove call to 'svnadmin deltify' from the
post-commit hook template.
Index: subversion/libsvn_ra_local/ra_plugin.c
===================================================================
--- subversion/libsvn_ra_local/ra_plugin.c (revision 7978)
+++ subversion/libsvn_ra_local/ra_plugin.c (working copy)
@@ -307,6 +307,41 @@
}
+struct deltify_etc_baton
+{
+ svn_fs_t *fs; /* the fs to deltify in */
+ apr_pool_t *pool; /* pool for scratch work */
+ svn_commit_callback_t callback; /* the original callback */
+ void *callback_baton; /* the original callback's baton */
+};
+
+/* This implements 'svn_commit_callback_t'. Its invokes the original
+ (wrapped) callback, but also does deltification on the new revision.
+ BATON is 'struct deltify_etc_baton *'. */
+static svn_error_t *
+deltify_etc (svn_revnum_t new_revision,
+ const char *date,
+ const char *author,
+ void *baton)
+{
+ struct deltify_etc_baton *db = baton;
+ svn_error_t *err1, *err2;
+
+ /* Invoke the original callback first, in case someone's waiting to
+ know the revision number so they can go off and annotate an
+ issue or something. */
+ err1 = (*db->callback) (new_revision, date, author, db->callback_baton);
+ err2 = svn_fs_deltify_revision (db->fs, new_revision, db->pool);
+
+ /* It's more interesting if the original callback failed, so let
+ that dominate. */
+ if (err1)
+ return err1;
+
+ return err2;
+}
+
+
static svn_error_t *
svn_ra_local__get_commit_editor (void *session_baton,
const svn_delta_editor_t **editor,
@@ -317,12 +352,18 @@
apr_pool_t *pool)
{
svn_ra_local__session_baton_t *sess = session_baton;
-
+ struct deltify_etc_baton *db = apr_palloc (pool, sizeof(*db));
+
+ db->fs = sess->fs;
+ db->pool = pool;
+ db->callback = callback;
+ db->callback_baton = callback_baton;
+
/* Get the repos commit-editor */
SVN_ERR (svn_repos_get_commit_editor (editor, edit_baton, sess->repos,
sess->repos_url, sess->fs_path,
sess->username, log_msg,
- callback, callback_baton, pool));
+ deltify_etc, db, pool));
return SVN_NO_ERROR;
}
Index: subversion/mod_dav_svn/version.c
===================================================================
--- subversion/mod_dav_svn/version.c (revision 7978)
+++ subversion/mod_dav_svn/version.c (working copy)
@@ -19,6 +19,7 @@
#include <httpd.h>
+#include <http_log.h>
#include <mod_dav.h>
#include <apr_tables.h>
#include <apr_uuid.h>
@@ -587,6 +588,88 @@
return dav_svn_working_to_regular_resource(resource);
}
+
+/* Closure object for cleanup_deltify. */
+struct cleanup_deltify_baton
+{
+ /* The repository in which to deltify. We use a path instead of an
+ object, because it's difficult to obtain a repos or fs object
+ with the right lifetime guarantees. */
+ const char *repos_path;
+
+ /* The revision number against which to deltify. */
+ svn_revnum_t revision;
+
+ /* The pool to use for all temporary allocation while working. This
+ may or may not be the same as the pool on which the cleanup is
+ registered, but obviously it must have a lifetime at least as
+ long as that pool. */
+ apr_pool_t *pool;
+};
+
+
+/* APR pool cleanup function to deltify against a just-committed
+ revision. DATA is a 'struct cleanup_deltify_baton *'.
+
+ If any errors occur, log them in the httpd server error log, but
+ return APR_SUCCESS no matter what, as this is a pool cleanup
+ function and deltification is not a matter of correctness
+ anyway. */
+static apr_status_t cleanup_deltify(void *data)
+{
+ struct cleanup_deltify_baton *cdb = data;
+ svn_repos_t *repos;
+ svn_error_t *err;
+
+ /* Yes, opening a repository registers a cleanup on the pool from
+ which we are already running as a cleanup. But these days that's
+ safe. A big +smooch+ to Sander Striker -- I'm not sure it was
+ he who implemented this, but he told me about it anyway :-). */
+ err = svn_repos_open(&repos, cdb->repos_path, cdb->pool);
+ if (err)
+ {
+ ap_log_perror(APLOG_MARK, APLOG_ERR, err->apr_err, cdb->pool,
+ "cleanup_deltify: error opening repository '%s'",
+ cdb->repos_path);
+ return APR_SUCCESS;
+ }
+
+ err = svn_fs_deltify_revision(svn_repos_fs(repos),
+ cdb->revision, cdb->pool);
+ if (err)
+ {
+ ap_log_perror(APLOG_MARK, APLOG_ERR, err->apr_err, cdb->pool,
+ "cleanup_deltify: error deltifying against revision %"
+ SVN_REVNUM_T_FMT " in repository '%s'",
+ cdb->revision, cdb->repos_path);
+ }
+
+ return APR_SUCCESS;
+}
+
+
+/* Register the cleanup_deltify function on POOL, which should be the
+ connection pool for the request. This way the time needed for
+ deltification won't delay the response to the client.
+
+ REPOS is the repository in which deltify, and REVISION is the
+ revision against which to deltify. POOL is both the pool on which
+ to register the cleanup function and the pool that will be used for
+ temporary allocations while deltifying. */
+static void register_deltification_cleanup(svn_repos_t *repos,
+ svn_revnum_t revision,
+ apr_pool_t *pool)
+{
+ struct cleanup_deltify_baton *cdb = apr_palloc(pool, sizeof(*cdb));
+
+ cdb->repos_path = svn_repos_path(repos, pool);
+ cdb->revision = revision;
+ cdb->pool = pool;
+
+ apr_pool_cleanup_register(pool, cdb, cleanup_deltify, apr_pool_cleanup_null);
+}
+
+
dav_error *dav_svn_checkin(dav_resource *resource,
int keep_checked_out,
dav_resource **version_resource)
@@ -654,7 +737,10 @@
return dav_svn_convert_err(serr, HTTP_CONFLICT, msg);
}
- /* Commit was successful. */
+ /* Commit was successful, so schedule deltification. */
+ register_deltification_cleanup(resource->info->repos->repos,
+ new_rev,
+ resource->info->r->connection->pool);
/* If caller wants it, return the new VR that was created by
the checkin. */
@@ -901,6 +987,10 @@
return dav_svn_convert_err(serr, HTTP_CONFLICT, msg);
}
+ /* Commit was successful, so schedule deltification. */
+ register_deltification_cleanup(source->info->repos->repos, new_rev,
+ source->info->r->connection->pool);
+
/* Check the dav_resource->info area for information about the
special X-SVN-Options: header that may have come in the http
request. If the header contains "no-merge-response", then pass
Index: subversion/libsvn_repos/repos.c
===================================================================
--- subversion/libsvn_repos/repos.c (revision 7978)
+++ subversion/libsvn_repos/repos.c (working copy)
@@ -637,11 +637,6 @@
"REV=\"$2\""
APR_EOL_STR
APR_EOL_STR
- "# Deltify predecessors of things changed in this revision."
- APR_EOL_STR
- "nice -2 svnadmin deltify \"$REPOS\" -r \"$REV\" &"
- APR_EOL_STR
- APR_EOL_STR
"commit-email.pl \"$REPOS\" \"$REV\" commit-watchers@example.org"
APR_EOL_STR
"log-commit.py --repository \"$REPOS\" --revision \"$REV\""
Index: subversion/svnserve/main.c
===================================================================
--- subversion/svnserve/main.c (revision 7978)
+++ subversion/svnserve/main.c (working copy)
@@ -145,6 +145,7 @@
struct serve_thread_t {
const char *root;
svn_ra_svn_conn_t *conn;
+ svn_boolean_t daemon;
svn_boolean_t read_only;
svn_boolean_t believe_username;
apr_pool_t *pool;
@@ -155,7 +156,7 @@
{
struct serve_thread_t *d = data;
- svn_error_clear(serve(d->conn, d->root, FALSE, d->read_only,
+ svn_error_clear(serve(d->conn, d->root, d->daemon, FALSE, d->read_only,
d->believe_username, d->pool));
svn_pool_destroy(d->pool);
@@ -267,7 +268,7 @@
apr_file_open_stdin(&in_file, pool);
apr_file_open_stdout(&out_file, pool);
conn = svn_ra_svn_create_conn(NULL, in_file, out_file, pool);
- svn_error_clear(serve(conn, root, tunnel_mode, read_only,
+ svn_error_clear(serve(conn, root, daemon_mode, tunnel_mode, read_only,
believe_username, pool));
exit(0);
}
@@ -339,8 +340,8 @@
if (listen_once)
{
- err = serve(conn, root, FALSE, read_only, believe_username,
- connection_pool);
+ err = serve(conn, root, daemon_mode, FALSE, read_only,
+ believe_username, connection_pool);
if (listen_once && err
&& err->apr_err != SVN_ERR_RA_SVN_CONNECTION_CLOSED)
@@ -359,7 +360,7 @@
status = apr_proc_fork(&proc, connection_pool);
if (status == APR_INCHILD)
{
- svn_error_clear(serve(conn, root, FALSE, read_only,
+ svn_error_clear(serve(conn, root, daemon_mode, FALSE, read_only,
believe_username, connection_pool));
apr_socket_close(usock);
exit(0);
@@ -399,6 +400,7 @@
thread_data = apr_palloc(connection_pool, sizeof(*thread_data));
thread_data->conn = conn;
thread_data->root = root;
+ thread_data->daemon = daemon_mode;
thread_data->read_only = read_only;
thread_data->believe_username = believe_username;
thread_data->pool = connection_pool;
@@ -415,8 +417,8 @@
case connection_mode_single:
/* Serve one connection at a time. */
- svn_error_clear(serve(conn, root, FALSE, read_only, believe_username,
- connection_pool));
+ svn_error_clear(serve(conn, root, daemon_mode, FALSE, read_only,
+ believe_username, connection_pool));
svn_pool_destroy(connection_pool);
}
}
Index: subversion/svnserve/serve.c
===================================================================
--- subversion/svnserve/serve.c (revision 7978)
+++ subversion/svnserve/serve.c (working copy)
@@ -52,6 +52,7 @@
const char *repos_url; /* Decoded URL to base of repository */
const char *fs_path; /* Decoded base path inside repository */
const char *user;
+ svn_boolean_t daemon; /* Running as daemon, allow asynch deltification */
svn_boolean_t tunnel; /* Tunneled through login agent; allow EXTERNAL */
svn_boolean_t read_only; /* Disallow write access (global flag) */
svn_boolean_t believe; /* Believe ANONYMOUS usernames */
@@ -547,8 +548,20 @@
if (!aborted)
{
SVN_ERR(trivial_auth_request(conn, pool, b));
+
+ /* If in daemon mode, answer the client before deltifying, so
+ the user doesn't wait for the deltification. But if not in
+ daemon mode, deltify first, so the process doesn't appear to
+ be done before it's really done. */
+
+ if (! b->daemon)
+ SVN_ERR(svn_fs_deltify_revision(b->fs, new_rev, pool));
+
SVN_ERR(svn_ra_svn_write_tuple(conn, pool, "r(?c)(?c)",
new_rev, date, author));
+
+ if (b->daemon)
+ SVN_ERR(svn_fs_deltify_revision(b->fs, new_rev, pool));
}
return SVN_NO_ERROR;
}
@@ -1031,8 +1044,9 @@
}
svn_error_t *serve(svn_ra_svn_conn_t *conn, const char *root,
- svn_boolean_t tunnel, svn_boolean_t read_only,
- svn_boolean_t believe_username, apr_pool_t *pool)
+ svn_boolean_t daemon, svn_boolean_t tunnel,
+ svn_boolean_t read_only, svn_boolean_t believe_username,
+ apr_pool_t *pool)
{
svn_error_t *err, *io_err;
apr_uint64_t ver;
@@ -1042,6 +1056,7 @@
svn_boolean_t success;
svn_ra_svn_item_t *item, *first;
+ b.daemon = daemon;
b.tunnel = tunnel;
b.read_only = read_only;
b.believe = believe_username;
Index: subversion/svnserve/server.h
===================================================================
--- subversion/svnserve/server.h (revision 7978)
+++ subversion/svnserve/server.h (working copy)
@@ -28,8 +28,9 @@
#endif /* __cplusplus */
svn_error_t *serve(svn_ra_svn_conn_t *conn, const char *root,
- svn_boolean_t tunnel, svn_boolean_t read_only,
- svn_boolean_t believe_username, apr_pool_t *pool);
+ svn_boolean_t daemon, svn_boolean_t tunnel,
+ svn_boolean_t read_only, svn_boolean_t believe_username,
+ apr_pool_t *pool);
#ifdef __cplusplus
}
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Fri Dec 12 00:02:04 2003