You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by "C. Michael Pilato" <cm...@collab.net> on 2006/07/24 17:44:33 UTC
r20600 broke mailer.py's diff url promises
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.
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Distributed Development On Demand
Re: r20600 broke mailer.py's diff url promises
Posted by "C. Michael Pilato" <cm...@collab.net>.
Blair Zajac wrote:
> 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.
I feel good about your patch, because it is almost exactly like the one I
have in progress here. I made a minor misstep in mine, though, that yours
doesn't make. I'll apply and test your patch a little later today, after my
morning mail slog. Thanks, Blair.
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Distributed Development On Demand
Re: r20600 broke mailer.py's diff url promises
Posted by "C. Michael Pilato" <cm...@collab.net>.
Blair Zajac wrote:
> I nominated r20841 as a part of this patch, which has whitespace only
> changes for backport. I don't know if 20854 will merge cleanly without
> this (haven't tested it) and the whitespace merge may help future merges.
/me stops trying to verify that very thing; deletes his full checkout of
branches/1.4.x...
Thanks, Blair.
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Distributed Development On Demand
Re: r20600 broke mailer.py's diff url promises
Posted by Blair Zajac <bl...@orcaware.com>.
C. Michael Pilato wrote:
> Blair Zajac wrote:
>> 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.
>
> Reviewed the patch. Looks good. Committed in r20854. Will close issue
> #2587 now, and propose 20854 for backport to 1.4.x.
>
I nominated r20841 as a part of this patch, which has whitespace only changes
for backport. I don't know if 20854 will merge cleanly without this (haven't
tested it) and the whitespace merge may help future merges.
Regards,
Blair
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: r20600 broke mailer.py's diff url promises
Posted by "C. Michael Pilato" <cm...@collab.net>.
Blair Zajac wrote:
> 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.
Reviewed the patch. Looks good. Committed in r20854. Will close issue
#2587 now, and propose 20854 for backport to 1.4.x.
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Distributed Development On Demand
Re: r20600 broke mailer.py's diff url promises
Posted by Blair Zajac <bl...@orcaware.com>.
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.
<bl...@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