On 01/13/2010 05:07 PM, Kamesh Jayachandran wrote:
On 01/13/2010 12:08 AM, C. Michael Pilato wrote:
Kamesh Jayachandran wrote:
Hi All,
Last week I posted the patch to implement 'svn client' to identify the
svn operation they are about to do with a given ra_session.
Following thread has the detailed discussion.
http://mail-archives.apache.org/mod_mbox/subversion-dev/201001.mbox/%3c4b448959.1070...@collab.net%3e
Below is the summary:
Concern/Suggestion 1:
Michael Pilato and Philip Martin were suggesting to tweak
libsvn_ra_(serf|neon) to detect the operation rather than making a
change across all layers from the simplicity point of view.
My answer to 1:
I feel it would be too hackish to tweak one general API inside these
modules to flag 'commit or write' operation to the server when the
solution I propose handles this in a transparent way.
I'm sorry, but did you say "transparent"? What's transparent about
bubbling
an RA-layer hack all the way up into the client?! A "transparent"
solution
This patch is *not* an RA layer hack rather a transparent generic
feature so I see nothing wrong in propagating it to higher layers.
is one that preserves the existing transparency of the mirroring
subsystem.
I do *not* like to expose the back-end internals to higher layers
especially when it is not so common setup and not normally supposed to
know(I mean why my client should know the repository it commits to is
a mirror directly or indirectly).
A "transparent" solution is one that doesn't allow ignorant
third-party
consumers of the Subversion APIs to accidentally break their proxy
setups
because they decide they wanted to pass "checkin" instead of "commit"
as the
innocuous-appearing 'high_level_svn_operation' parameter.
Yes I agree. I was concerned about the *magical flag deep inside the
code* for only one operation, now it looks like the only way to go.
There *must* be a better way to do this.
subversion/libsvn_ra_neon/commit.c:apply_revprops() has this comment:
/* ### we should use DAV:apply-to-version on the CHECKOUT so we
can skip
### retrieval of the baseline */
I looked a little bit into this. IIUC, we can theoretically avoid the
problematic PROPFIND altogether by passing this special (yet
part-of-an-existing-standard) flag in the CHECKOUT request. Currently,
though, mod_dav_svn doesn't handle the flag. So there's still a
server-side
change needed, but at least its one that would take us closer to better
WebDAV handling. Maybe you could explore this option instead?
I will take a fresh look at this problem and a independent patch in
this way.
Thanks
With regards
Kamesh Jayachandran
Attached patch just fixes this at ra_neon layer alone(ra_serf do not
need the fix at least on trunk, as it do not use the PROPFIND for this
rather uses POST which do not suffer from this issue as it simply
proxies to MASTER).
Have not tested the full test suite, I hope this is final unless I learn
something new.
With regards
Kamesh Jayachandran
Index: subversion/libsvn_ra_neon/ra_neon.h
===================================================================
--- subversion/libsvn_ra_neon/ra_neon.h (revision 900319)
+++ subversion/libsvn_ra_neon/ra_neon.h (working copy)
@@ -447,6 +447,7 @@
const char *url,
int depth,
const char *label,
+ svn_boolean_t for_commit,
const ne_propname *which_props,
apr_pool_t *pool);
@@ -455,6 +456,7 @@
svn_ra_neon__session_t *sess,
const char *url,
const char *label,
+ svn_boolean_t for_commit,
const ne_propname *which_props,
apr_pool_t *pool);
@@ -491,6 +493,7 @@
svn_ra_neon__session_t *sess,
const char *url,
const char *label,
+ svn_boolean_t for_commit,
const ne_propname *propname,
apr_pool_t *pool);
Index: subversion/libsvn_ra_neon/props.c
===================================================================
--- subversion/libsvn_ra_neon/props.c (revision 900319)
+++ subversion/libsvn_ra_neon/props.c (working copy)
@@ -495,6 +495,7 @@
const char *url,
int depth,
const char *label,
+ svn_boolean_t for_commit,
const ne_propname *which_props,
apr_pool_t *pool)
{
@@ -507,6 +508,10 @@
/* If we have a label, use it. */
if (label != NULL)
apr_hash_set(extra_headers, "Label", 5, label);
+ /* If for_commit is TRUE, set SVN-COMMIT so that PROXY can handle
+ it accordingly. */
+ if (for_commit)
+ apr_hash_set(extra_headers, "SVN-COMMIT", APR_HASH_KEY_STRING, "1");
/* It's easier to roll our own PROPFIND here than use neon's current
interfaces. */
@@ -561,6 +566,7 @@
svn_ra_neon__session_t *sess,
const char *url,
const char *label,
+ svn_boolean_t for_commit,
const ne_propname *which_props,
apr_pool_t *pool)
{
@@ -572,7 +578,7 @@
url_path[len - 1] = '\0';
SVN_ERR(svn_ra_neon__get_props(&props, sess, url_path,
SVN_RA_NEON__DEPTH_ZERO,
- label, which_props, pool));
+ label, for_commit, which_props, pool));
/* ### HACK. We need to have the client canonicalize paths, get rid
of double slashes and such. This check is just a check against
@@ -612,6 +618,7 @@
svn_ra_neon__session_t *sess,
const char *url,
const char *label,
+ svn_boolean_t for_commit,
const ne_propname *propname,
apr_pool_t *pool)
{
@@ -621,8 +628,8 @@
const svn_string_t *value;
props[0] = *propname;
- SVN_ERR(svn_ra_neon__get_props_resource(&rsrc, sess, url, label, props,
- pool));
+ SVN_ERR(svn_ra_neon__get_props_resource(&rsrc, sess, url, label, for_commit,
+ props, pool));
name = apr_pstrcat(pool, propname->nspace, propname->name, NULL);
value = apr_hash_get(rsrc->propset, name, APR_HASH_KEY_STRING);
@@ -647,7 +654,7 @@
svn_string_t *propval;
SVN_ERR(svn_ra_neon__get_props_resource(rsrc, sess, url, label,
- starting_props, pool));
+ FALSE, starting_props, pool));
/* Cache some of the resource information. */
@@ -917,13 +924,14 @@
DAV:baseline-collection property. */
/* ### should wrap this with info about rsrc==VCC */
SVN_ERR(svn_ra_neon__get_one_prop(&baseline, sess, vcc, NULL,
- &svn_ra_neon__checked_in_prop, pool));
+ FALSE, &svn_ra_neon__checked_in_prop,
+ pool));
/* ### do we want to optimize the props we fetch, based on what the
### user asked for? i.e. omit version-name if latest_rev is NULL */
SVN_ERR(svn_ra_neon__get_props_resource(&rsrc, sess,
baseline->data, NULL,
- which_props, pool));
+ FALSE, which_props, pool));
}
else
{
@@ -938,7 +946,7 @@
/* ### do we want to optimize the props we fetch, based on what the
### user asked for? i.e. omit version-name if latest_rev is NULL */
SVN_ERR(svn_ra_neon__get_props_resource(&rsrc, sess, vcc, label,
- which_props, pool));
+ FALSE, which_props, pool));
}
/* Return the baseline rsrc, which now contains whatever set of
@@ -1281,7 +1289,7 @@
/* Depth-zero PROPFIND is the One True DAV Way. */
err = svn_ra_neon__get_props(&resources, ras, final_url,
SVN_RA_NEON__DEPTH_ZERO,
- NULL, NULL /* all props */, pool);
+ NULL, FALSE, NULL /* all props */, pool);
if (err)
{
if (err->apr_err == SVN_ERR_FS_NOT_FOUND)
Index: subversion/libsvn_ra_neon/commit.c
===================================================================
--- subversion/libsvn_ra_neon/commit.c (revision 900319)
+++ subversion/libsvn_ra_neon/commit.c (working copy)
@@ -237,7 +237,7 @@
/* Get the DAV:checked-in property, which contains the URL of the
Version Resource */
SVN_ERR(svn_ra_neon__get_props_resource(&propres, cc->ras, url,
- NULL, fetch_props, pool));
+ NULL, TRUE, fetch_props, pool));
url_str = apr_hash_get(propres->propset,
SVN_RA_NEON__PROP_CHECKED_IN,
APR_HASH_KEY_STRING);
@@ -1387,7 +1387,8 @@
This should give us the HEAD revision of the moment. */
SVN_ERR(svn_ra_neon__get_one_prop(&baseline_url, cc->ras,
vcc, NULL,
- &svn_ra_neon__checked_in_prop, pool));
+ TRUE, &svn_ra_neon__checked_in_prop,
+ pool));
baseline_rsrc.pool = pool;
baseline_rsrc.vsn_url = baseline_url->data;
Index: subversion/libsvn_ra_neon/fetch.c
===================================================================
--- subversion/libsvn_ra_neon/fetch.c (revision 900319)
+++ subversion/libsvn_ra_neon/fetch.c (working copy)
@@ -719,6 +719,7 @@
ras,
final_url,
NULL,
+ FALSE,
&md5_propname,
pool);
@@ -769,7 +770,8 @@
if (props)
{
SVN_ERR(svn_ra_neon__get_props_resource(&rsrc, ras, final_url,
- NULL, NULL /* all props */,
+ NULL, FALSE,
+ NULL /* all props */,
pool));
*props = apr_hash_make(pool);
SVN_ERR(filter_props(*props, rsrc, TRUE, pool));
@@ -836,7 +838,7 @@
const svn_string_t *deadprop_count;
SVN_ERR(svn_ra_neon__get_props_resource(&rsrc, ras,
- final_url, NULL,
+ final_url, NULL, FALSE,
deadprop_count_support_props,
pool));
@@ -935,7 +937,7 @@
PROPFIND on the directory of depth 1. */
SVN_ERR(svn_ra_neon__get_props(&resources, ras,
final_url, SVN_RA_NEON__DEPTH_ONE,
- NULL, which_props, pool));
+ NULL, FALSE, which_props, pool));
/* Count the number of path components in final_url. */
final_url_n_components = svn_path_component_count(final_url);
@@ -1073,7 +1075,8 @@
if (props)
{
SVN_ERR(svn_ra_neon__get_props_resource(&rsrc, ras, final_url,
- NULL, NULL /* all props */,
+ NULL, FALSE,
+ NULL /* all props */,
pool));
*props = apr_hash_make(pool);
@@ -1197,7 +1200,7 @@
{
const char *url = apr_psprintf(pool, "%s/%ld", ras->rev_stub, rev);
SVN_ERR(svn_ra_neon__get_props_resource(&bln, ras, url,
- NULL, NULL, pool));
+ NULL, FALSE, NULL, pool));
}
else
{
@@ -1584,7 +1587,7 @@
rb->ras,
bc_url,
SVN_RA_NEON__DEPTH_ONE,
- NULL, NULL /* allprops */,
+ NULL, FALSE, NULL /* allprops */,
TOP_DIR(rb).pool));
/* re-index the results into a more usable hash.
@@ -1898,6 +1901,7 @@
rb->ras,
rb->href->data,
NULL,
+ FALSE,
NULL,
pool));
props = rsrc->propset;
@@ -1924,8 +1928,8 @@
SVN_ERR(svn_ra_neon__get_props_resource(&rsrc,
rb->ras,
TOP_DIR(rb).vsn_url,
+ NULL, FALSE,
NULL,
- NULL,
pool));
props = rsrc->propset;
}
Index: subversion/mod_dav_svn/mirror.c
===================================================================
--- subversion/mod_dav_svn/mirror.c (revision 900319)
+++ subversion/mod_dav_svn/mirror.c (working copy)
@@ -30,7 +30,54 @@
#include "dav_svn.h"
+#define USERDATA_KEY "mod_dav_svn_userdata_key"
+typedef struct
+{
+ int mk_activity_has_happened;
+} ctx_data;
+
+static apr_status_t
+initialize_ctx_data(ctx_data **data,
+ request_rec *r,
+ apr_pool_t *pool)
+{
+ ctx_data *ctx = apr_pcalloc(pool, sizeof(*ctx));
+
+ if (apr_pool_userdata_set(ctx, USERDATA_KEY, NULL, pool) != APR_SUCCESS)
+ {
+ ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r,
+ "mod_dav_svn: Error setting connection pool userdata");
+ return HTTP_INTERNAL_SERVER_ERROR;
+ }
+ ctx->mk_activity_has_happened = 0;
+ *data = ctx;
+ return APR_SUCCESS;
+}
+
+static int
+get_userdata(ctx_data **ctx,
+ request_rec *r)
+{
+ void *data;
+ apr_pool_t *pool = r->connection->pool;
+
+ if (apr_pool_userdata_get(&data, USERDATA_KEY, pool) != APR_SUCCESS)
+ {
+ ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r,
+ "mod_dav_svn: Error fetching connection userdata");
+ return HTTP_INTERNAL_SERVER_ERROR;
+ }
+
+ if (data)
+ *ctx = data;
+ else
+ initialize_ctx_data(ctx, r, pool);
+
+ return OK;
+}
+
+
/* Tweak the request record R, and add the necessary filters, so that
the request is ready to be proxied away. MASTER_URI is the URI
specified in the SVNMasterURI Apache configuration value.
@@ -64,6 +111,14 @@
if (root_dir && master_uri) {
const char *seg;
+ const char *action;
+ int is_commit = 0;
+ if (r->method_number == M_MKACTIVITY) {
+ ctx_data *ctx;
+ int ret = get_userdata(&ctx, r);
+ if (ret == OK)
+ ctx->mk_activity_has_happened = 1;
+ }
/* We know we can always safely handle these. */
if (r->method_number == M_REPORT ||
@@ -71,6 +126,17 @@
return OK;
}
+ action = apr_table_get(r->headers_in, "SVN-COMMIT");
+ if (action) {
+ is_commit = 1;
+ } else {
+ ctx_data *ctx;
+ int ret = get_userdata(&ctx, r);
+ if (ret == OK && ctx->mk_activity_has_happened == 1)
+ is_commit = 1;
+ }
+
+
/* These are read-only requests -- the kind we like to handle
ourselves -- but we need to make sure they aren't aimed at
working resource URIs before trying to field them. Why?
@@ -79,12 +145,18 @@
of the repository isn't guaranteed to have on hand. */
if (r->method_number == M_PROPFIND ||
r->method_number == M_GET) {
- seg = ap_strstr(r->unparsed_uri, root_dir);
- if (seg && ap_strstr_c(seg,
- apr_pstrcat(r->pool, special_uri,
- "/wrk/", NULL))) {
- seg += strlen(root_dir);
- proxy_request_fixup(r, master_uri, seg);
+ if (is_commit) {
+ seg = ap_strstr(r->unparsed_uri, root_dir);
+ if (seg && (ap_strstr_c(seg,
+ apr_pstrcat(r->pool, special_uri,
+ "/wrk/", NULL)) ||
+ ap_strstr_c(seg,
+ apr_pstrcat(r->pool, special_uri,
+ "/vcc/", NULL))
+ )) {
+ seg += strlen(root_dir);
+ proxy_request_fixup(r, master_uri, seg);
+ }
}
return OK;
}
