[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: mod-dav-svn high level logging compatibility (long)

From: Eric Gillespie <epg_at_pretzelnet.org>
Date: 2007-12-19 02:37:35 CET

Eric Gillespie <epg@pretzelnet.org> writes:

> This may seem out-of-scope given the focus on the 1.5 release,
> but since we've already changed the logging for 1.5, I think we
> need to consider these things before we ship. The hard part was
> digging out all this information; I spent my evening playing code
> archaeologist. I don't think fixing all this will be a large
> change, if my suggestions seem like the right way to go.

I've finally returned to this.

> - The biggest problem I've found is we don't log the repository!
> DOH! Instead of adding this the SVN-ACTION environment
> variable, I suggest adding two more variables, SVN-REPOS
> (containing the full path) and SVN-REPOS-NAME (containing only
> the basename). We use the basename for other things already,
> such as authz with ParentPath repositories. Admins using
> ParentPath (and some who aren't, e.g. one repository per vhost)
> probably just want to log the basename, not clutter up the log
> with the full path.

I'll commit this if davautocheck passes; see patch and log
message below, if interested. dav_svn__operational_log isn't the
greatest name in the world; speak up if you have a better idea.

> - Do the '' quotes aid or hinder readability? For me, they're a
> big hindrance.

Removed in r28543.

[[[
Consolidate operational logging and log repository path and name.

* subversion/mod_dav_svn/util.c
  (dav_svn__operational_log): Add helper function to log to SVN-ACTION
    environment variable as before, but also log repository in
    SVN-REPOS and SVN-REPOS-NAME.

* subversion/mod_dav_svn/dav_svn.h
  (dav_svn__operational_log): Declare.

* subversion/mod_dav_svn/deadprops.c
  (save_value): Use dav_svn__operational_log instead of calling
    setting SVN-ACTION directly here.

* subversion/mod_dav_svn/lock.c
  (append_locks, remove_lock): same

* subversion/mod_dav_svn/repos.c
  (do_walk): same

* subversion/mod_dav_svn/version.c
  (merge): same

* subversion/mod_dav_svn/reports/file-revs.c
  (dav_svn__file_revs_report): same

* subversion/mod_dav_svn/reports/log.c
  (dav_svn__log_report): same

* subversion/mod_dav_svn/reports/mergeinfo.c
  (dav_svn__get_mergeinfo_report): same

* subversion/mod_dav_svn/reports/replay.c
  (dav_svn__replay_report): same

* subversion/mod_dav_svn/reports/update.c
  (dav_svn__update_report): same

* subversion/tests/cmdline/davautocheck.sh
  Use SVN-REPOS-NAME in ops log.
]]]

Index: subversion/mod_dav_svn/deadprops.c
===================================================================
--- subversion/mod_dav_svn/deadprops.c (revision 28543)
+++ subversion/mod_dav_svn/deadprops.c (working copy)
@@ -170,12 +170,13 @@
            db->resource->pool);
 
         /* Tell the logging subsystem about the revprop change. */
- apr_table_set(db->resource->info->r->subprocess_env, "SVN-ACTION",
- apr_psprintf(db->resource->pool,
- "revprop-change r%ld %s",
+ dav_svn__operational_log(db->resource->info,
+ apr_psprintf(db->resource->pool,
+ "revprop-change r%ld %s %s",
                                    db->resource->info->root.rev,
                                    svn_path_uri_encode(propname,
- db->resource->pool)));
+ db->resource->pool),
+ db->resource->info->repos->repo_name));
       }
   else
     serr = svn_repos_fs_change_node_prop(db->resource->info->root.root,
Index: subversion/mod_dav_svn/dav_svn.h
===================================================================
--- subversion/mod_dav_svn/dav_svn.h (revision 28538)
+++ subversion/mod_dav_svn/dav_svn.h (working copy)
@@ -743,6 +743,11 @@
                                    ap_filter_t *output,
                                    apr_pool_t *pool);
 
+/* In INFO->r->subprocess_env set "SVN-ACTION" to LINE, "SVN-REPOS" to
+ * INFO->repos->fs_path, and "SVN-REPOS-NAME" to INFO->repos->repo_name. */
+void
+dav_svn__operational_log(struct dav_resource_private *info, const char *line);
+
 /*** mirror.c ***/
 
 /* Perform the fixup hook for the R request. */
Index: subversion/mod_dav_svn/version.c
===================================================================
--- subversion/mod_dav_svn/version.c (revision 28543)
+++ subversion/mod_dav_svn/version.c (working copy)
@@ -1325,11 +1325,11 @@
                                  source->info->r->connection->pool);
 
   /* We've detected a 'high level' svn action to log. */
- apr_table_set(target->info->r->subprocess_env, "SVN-ACTION",
- apr_psprintf(target->info->r->pool,
- "commit %s r%ld",
- target->info->repos_path,
- new_rev));
+ dav_svn__operational_log(target->info,
+ apr_psprintf(target->info->r->pool,
+ "commit %s r%ld",
+ target->info->repos_path,
+ new_rev));
 
   /* Since the commit was successful, the txn ID is no longer valid.
      Store an empty txn ID in the activity database so that when the
Index: subversion/mod_dav_svn/lock.c
===================================================================
--- subversion/mod_dav_svn/lock.c (revision 28543)
+++ subversion/mod_dav_svn/lock.c (working copy)
@@ -758,10 +758,10 @@
                  slock->owner);
 
   /* Log the locking as a 'high-level' action. */
- apr_table_set(resource->info->r->subprocess_env, "SVN-ACTION",
- apr_psprintf(resource->info->r->pool,
- "lock %s",
- svn_path_uri_encode(slock->path,
+ dav_svn__operational_log(resource->info,
+ apr_psprintf(resource->info->r->pool,
+ "lock %s",
+ svn_path_uri_encode(slock->path,
                                                  resource->info->r->pool)));
 
   return 0;
@@ -845,8 +845,8 @@
                                     resource->pool);
 
       /* Log the unlocking as a 'high-level' action. */
- apr_table_set(resource->info->r->subprocess_env, "SVN-ACTION",
- apr_psprintf(resource->info->r->pool,
+ dav_svn__operational_log(resource->info,
+ apr_psprintf(resource->info->r->pool,
                                  "unlock %s",
                                  svn_path_uri_encode(resource->info->repos_path,
                                                      resource->info->r->pool)));
Index: subversion/mod_dav_svn/repos.c
===================================================================
--- subversion/mod_dav_svn/repos.c (revision 28543)
+++ subversion/mod_dav_svn/repos.c (working copy)
@@ -3461,8 +3461,8 @@
      Note: if we cared, we could look at the 'User-Agent:' request
      header and distinguish an svn client ('svn ls') from a generic
      DAV client. */
- apr_table_set(ctx->info.r->subprocess_env, "SVN-ACTION",
- apr_psprintf(params->pool,
+ dav_svn__operational_log(&ctx->info,
+ apr_psprintf(params->pool,
                              "list-dir %s r%ld",
                              svn_path_uri_encode(ctx->info.repos_path,
                                                  params->pool),
Index: subversion/mod_dav_svn/reports/log.c
===================================================================
--- subversion/mod_dav_svn/reports/log.c (revision 28543)
+++ subversion/mod_dav_svn/reports/log.c (working copy)
@@ -425,9 +425,8 @@
                         include_merged_revisions ? "-merge-sensitive" : "",
                         comma_separated_paths->data, start, end,
                         comma_separated_revprops->data);
- apr_table_set(resource->info->r->subprocess_env, "SVN-ACTION", action);
+ dav_svn__operational_log(resource->info, action);
 
-
   /* Flush the contents of the brigade (returning an error only if we
      don't already have one). */
   if (!lrb.needs_header)
Index: subversion/mod_dav_svn/reports/update.c
===================================================================
--- subversion/mod_dav_svn/reports/update.c (revision 28543)
+++ subversion/mod_dav_svn/reports/update.c (working copy)
@@ -1362,7 +1362,7 @@
           }
       }
 
- apr_table_set(resource->info->r->subprocess_env, "SVN-ACTION", action);
+ dav_svn__operational_log(resource->info, action);
   }
 
   /* this will complete the report, and then drive our editor to generate
Index: subversion/mod_dav_svn/reports/mergeinfo.c
===================================================================
--- subversion/mod_dav_svn/reports/mergeinfo.c (revision 28543)
+++ subversion/mod_dav_svn/reports/mergeinfo.c (working copy)
@@ -187,9 +187,8 @@
                                               (paths, 0, const char *),
                                               resource->pool));
 
- apr_table_set(resource->info->r->subprocess_env, "SVN-ACTION", action);
+ dav_svn__operational_log(resource->info, action);
 
-
   /* Flush the contents of the brigade (returning an error only if we
      don't already have one). */
   if ((apr_err = ap_fflush(output, bb)) && !derr)
Index: subversion/mod_dav_svn/reports/file-revs.c
===================================================================
--- subversion/mod_dav_svn/reports/file-revs.c (revision 28543)
+++ subversion/mod_dav_svn/reports/file-revs.c (working copy)
@@ -322,8 +322,8 @@
  cleanup:
 
   /* We've detected a 'high level' svn action to log. */
- apr_table_set(resource->info->r->subprocess_env, "SVN-ACTION",
- apr_psprintf(resource->pool, "blame%s %s r%ld:%ld",
+ dav_svn__operational_log(resource->info,
+ apr_psprintf(resource->pool, "blame%s %s r%ld:%ld",
                              include_merged_revisions ?
                                                     "-merge-sensitive" : "",
                              svn_path_uri_encode(path, resource->pool),
Index: subversion/mod_dav_svn/reports/replay.c
===================================================================
--- subversion/mod_dav_svn/reports/replay.c (revision 28543)
+++ subversion/mod_dav_svn/reports/replay.c (working copy)
@@ -495,7 +495,7 @@
     else
       action = apr_psprintf(resource->info->r->pool, "replay r%ld", rev);
 
- apr_table_set(resource->info->r->subprocess_env, "SVN-ACTION", action);
+ dav_svn__operational_log(resource->info, action);
   }
 
   ap_fflush(output, bb);
Index: subversion/mod_dav_svn/util.c
===================================================================
--- subversion/mod_dav_svn/util.c (revision 28538)
+++ subversion/mod_dav_svn/util.c (working copy)
@@ -479,3 +479,12 @@
 
   return svn_base64_encode(stream, pool);
 }
+
+void
+dav_svn__operational_log(struct dav_resource_private *info, const char *line)
+{
+ apr_table_set(info->r->subprocess_env, "SVN-ACTION", line);
+ apr_table_set(info->r->subprocess_env, "SVN-REPOS", info->repos->fs_path);
+ apr_table_set(info->r->subprocess_env, "SVN-REPOS-NAME",
+ info->repos->repo_basename);
+}
Index: subversion/tests/cmdline/davautocheck.sh
===================================================================
--- subversion/tests/cmdline/davautocheck.sh (revision 28538)
+++ subversion/tests/cmdline/davautocheck.sh (working copy)
@@ -267,7 +267,7 @@
 HostNameLookups Off
 LogFormat "%h %l %u %t \"%r\" %>s %b \"%{Referer}i\" \"%{User-Agent}i\"" format
 CustomLog "$HTTPD_ROOT/req" format
-CustomLog "$HTTPD_ROOT/ops" "%t %u %{SVN-ACTION}e" env=SVN-ACTION
+CustomLog "$HTTPD_ROOT/ops" "%t %u %{SVN-REPOS-NAME}e %{SVN-ACTION}e" env=SVN-ACTION
 
 <Directory />
   AllowOverride none

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Wed Dec 19 02:37:48 2007

This is an archived mail posted to the Subversion Dev mailing list.