Re: r20600 broke mailer.py's diff url promises

C. Michael Pilato wrote:
> mailer.py's sample configuration file -- and, until r20600, behavior --
> gives the following expected behavior for the diff_*_url options:
>
> # Diff URL construction. For the configured diff URL types, the diff
> # section (which follows the message header) will include the URL
> # relevant to the change type, even if actual diff generation for that
> # change type is disabled (per the generate_diffs option).
> #
> # Available substitution variables are: path, base_path, rev, base_rev
>
> But as of r20600, those substitution variables (path, base_path, rev,
> base_rev) no longer work.
>
> I haven't really jumped into a debugging session, but my guess is that the
> substitution is now happening earlier than it was before (in the calls to
> cfg.get()), specifically, too early for the above variables to have been
> defined.
>
> I'm tracking this regression in issue #2587.

So r20600 worked for me because I was using mappings in our production
environment, which differed the parameter substitution till later, hence I
didn't see the bug.

The below patch, which I can apply when people are happy with it, defers the
parameter retrieval.

I tested this patch with a mailer.conf that uses mapping and one that does not
for the diff_*_url values.

Regards,
Blair

-- 
Blair Zajac, Ph.D.
<blair@orcaware.com>
Subversion training, consulting and support
http://www.orcaware.com/svn/
[[[
Fix issue 2587, where r20600 broke parameter substitution for the diff
URLs when mappings were not used.
For the diff URLs, defer the parameter substitution until the URLs are
needed, so that the parameters from the configuration file and from
the path and revision in the commit may be joined.
* tools/hook-scripts/mailer/mailer.py
   (Config.get):
     If a value is mapped, then apply the parameter substitution to the
       value returned from the mapping.  This allows
       DiffURLSelections._get_url to not have to do this work.
   (DiffURLSelections.__init):
     Do not get the diff_add_url, diff_copy_url, diff_delete_url and
       the diff_modify_url values, just cache the configuration, group
       and parameter objects for later use.
   (DiffURLSelections._get_url):
     Given the action name, merge the cached parameters with the
       parameters for the path and revision in the commit and generate
       the URL.
   (DiffURLSelections.get_add_url),
   (DiffURLSelections.get_copy_url),
   (DiffURLSelections.get_delete_url),
   (DiffURLSelections.get_modify_url):
     Helper functions that call DiffURLSelections._get_url with the
       appropriate action name.
   (DiffGenerator._gen_url):
     Delete.
]]]
Index: tools/hook-scripts/mailer/mailer.py
===================================================================
--- tools/hook-scripts/mailer/mailer.py (revision 20841)
+++ tools/hook-scripts/mailer/mailer.py (working copy)
@@ -573,12 +573,34 @@
  class DiffURLSelections:
    def __init__(self, cfg, group, params):
-    self.add = cfg.get('diff_add_url', group, params)
-    self.copy = cfg.get('diff_copy_url', group, params)
-    self.delete = cfg.get('diff_delete_url', group, params)
-    self.modify = cfg.get('diff_modify_url', group, params)
+    self.cfg = cfg
+    self.group = group
+    self.params = params
+  def _get_url(self, action, repos_rev, change):
+    # The parameters for the URLs generation need to be placed in the
+    # parameters for the configuration module, otherwise we may get
+    # KeyError exceptions.
+    params = self.params.copy()
+    params['path'] = change.path and urllib.quote(change.path) or None
+    params['base_path'] = change.base_path and urllib.quote(change.base_path) 
or None
+    params['rev'] = repos_rev
+    params['base_rev'] = change.base_rev
+    return self.cfg.get("diff_%s_url" % action, self.group, params)
+
+  def get_add_url(self, repos_rev, change):
+    return self._get_url('add', repos_rev, change)
+
+  def get_copy_url(self, repos_rev, change):
+    return self._get_url('copy', repos_rev, change)
+
+  def get_delete_url(self, repos_rev, change):
+    return self._get_url('delete', repos_rev, change)
+
+  def get_modify_url(self, repos_rev, change):
+    return self._get_url('modify', repos_rev, change)
+
  def generate_content(renderer, cfg, repos, changelist, group, params, paths,
                       pool):
@@ -674,17 +696,6 @@
      # we always have some items
      return True
-  def _gen_url(self, urlstr, repos_rev, change):
-    if not len(urlstr):
-      return None
-    args = {
-      'path' : change.path and urllib.quote(change.path) or None,
-      'base_path' : change.base_path and urllib.quote(change.base_path) or None,
-      'rev' : repos_rev,
-      'base_rev' : change.base_rev,
-      }
-    return urlstr % args
-
    def __getitem__(self, idx):
      while 1:
        if self.idx == len(self.changelist):
@@ -717,10 +728,8 @@
          # it was delete.
          kind = 'D'
-        # show the diff url?
-        if self.diffurls.delete:
-          diff_url = self._gen_url(self.diffurls.delete,
-                                   self.repos.rev, change)
+        # get the diff url, if any is specified
+        diff_url = self.diffurls.get_delete_url(self.repos.rev, change)
          # show the diff?
          if self.diffsels.delete:
@@ -739,10 +748,8 @@
            # any diff of interest?
            if change.text_changed:
-            # show the diff url?
-            if self.diffurls.copy:
-              diff_url = self._gen_url(self.diffurls.copy,
-                                       self.repos.rev, change)
+            # get the diff url, if any is specified
+            diff_url = self.diffurls.get_copy_url(self.repos.rev, change)
              # show the diff?
              if self.diffsels.copy:
@@ -757,10 +764,8 @@
            # the file was added.
            kind = 'A'
-          # show the diff url?
-          if self.diffurls.add:
-            diff_url = self._gen_url(self.diffurls.add,
-                                     self.repos.rev, change)
+          # get the diff url, if any is specified
+          diff_url = self.diffurls.get_add_url(self.repos.rev, change)
            # show the diff?
            if self.diffsels.add:
@@ -777,10 +782,8 @@
          # a simple modification.
          kind = 'M'
-        # show the diff url?
-        if self.diffurls.modify:
-          diff_url = self._gen_url(self.diffurls.modify,
-                                   self.repos.rev, change)
+        # get the diff url, if any is specified
+        diff_url = self.diffurls.get_modify_url(self.repos.rev, change)
          # show the diff?
          if self.diffsels.modify:
@@ -1082,6 +1085,11 @@
      if mapper is not None:
        value = mapper(value)
+      # Apply any parameters that may now be available for
+      # substitution that were not before the mapping.
+      if value is not None and params is not None:
+        value = value % params
+
      return value
    def get_diff_cmd(self, group, args):
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org