On Mon, Jan 19, 2009 at 16:37, Jon Bendtsen <jbendtsen_at_laerdal.dk> wrote:
> Hi
>
> Here is a new patch that makes some small optimizations to avoid
> wasting space.
>
> <module> / <branch> / <submodule> / ... / <file>
>
> If branch is trunk it is not included, because default is assumed to
> be trunk
>
> if submodule and file are identical, only the file header is included.
>
> If the group is None, there is no X-group header.
Hmm. Most repositories that I've seen do not have <module>. They just
start with /trunk. The above form only matches for the typical /tags
and /branches.
Rather than applying specific names to the components, how about a
more general solution such as (for your example above):
path-headers = module:0 branch:1 submodule:2 mysubsub:3 file:-1
Or something like that. Basically: NAME:SEGMENT-NUMBER. Of course, the
problem here is that "submodule" is sometimes at segment 1 of the path
(under /trunk), but at segment 2 for /tags and /branches.
The difference between /trunk and /tags and /branches could be handled
with a separate [group] which matches specific paths using for_paths.
Some review of the patch, independent of the above discussion:
>...
> +++ mailer.py (working copy)
>...
> @@ -250,7 +250,13 @@
> and self.reply_to[2] == ']':
> self.reply_to = self.reply_to[3:]
>
> - def mail_headers(self, group, params):
> + def mail_headers(self, group, params, header_list):
> + header_detail = header_list[0]
> + modules = header_list[1]
> + branches = header_list[2]
> + submodules = header_list[3]
> + files = header_list[4]
I dislike this form of "bundling up" parameters. When you look at the
invocation, you really *are* passing five separate parameters. This
bundling is simply used to create an abbreviation around the method
signatures within the module. I'd suggest spelling out all five, but
if we follow the above suggestion and create a mechanism allowing the
configuration to specify name/value, then maybe we would pass some
kind of a HeaderSpec object (to be designed).
> +
> subject = self.make_subject(group, params)
> try:
> subject.encode('ascii')
> @@ -262,23 +268,39 @@
> 'Subject: %s\n' \
> 'MIME-Version: 1.0\n' \
> 'Content-Type: text/plain; charset=UTF-8\n' \
> - 'Content-Transfer-Encoding: 8bit\n' \
> + 'Content-Transfer-Encoding: 8bit' \
> % (self.from_addr, ', '.join(self.to_addrs), subject)
> if self.reply_to:
> - hdrs = '%sReply-To: %s\n' % (hdrs, self.reply_to)
> + hdrs = '%s\nReply-To: %s' % (hdrs, self.reply_to)
> + for header in header_detail:
> + if 'group' == header:
> + if None != group:
> + hdrs = '%s\nX-group: %s' % (hdrs, group)
> + if 'module' == header:
> + for m in modules:
> + hdrs = '%s\nX-module: %s' % (hdrs, m)
> + if 'branch' == header:
> + for b in branches:
> + hdrs = '%s\nX-branch: %s' % (hdrs, b)
> + if 'submodule' == header:
> + for s in submodules:
> + hdrs = '%s\nX-submod: %s' % (hdrs, s)
> + if 'file' == header:
> + for f in files:
> + hdrs = '%s\nX-file: %s' % (hdrs, f)
> return hdrs + '\n'
Since the construction of this string object is much more complicated
(now), then all of this concatenation building the string is
troublesome. Seems it should switch over to a list object of "Name:
Value" items and then joined with "\n". I think that would make the
function quite a bit clearer, too. It would also be possible to do
something like:
hdrs.extend(['X-module: %s' % m for m in modules])
with the final:
return '\n'.join(hdrs) + '\n'
>...
> class SMTPOutput(MailedOutput):
> "Deliver a mail message to an MTA using SMTP."
>
> - def start(self, group, params):
> + def start(self, group, params, header_list):
> MailedOutput.start(self, group, params)
header_list should be passed to the superclass.
>...
> @@ -429,7 +451,46 @@
> renderer = TextCommitRenderer(self.output)
>
> for (group, param_tuple), (params, paths) in self.groups.items():
> - self.output.start(group, params)
> + header_list = []
> + module = []
> + branch = []
> + submodule = []
> + file = []
> + h_detail = self.cfg.get('header_detail', group, params)
> + if len(h_detail) >= 3 and h_detail[0] == '[' \
> + and h_detail[2] == ']':
> + header_detail = \
> + [_f for _f in h_detail[3:].split(h_detail[1]) if _f]
> + else:
> + header_detail = [_f for _f in h_detail.split() if _f]
What is this stuff with [x] at the beginning? I see no doc about it,
or other references/use.
> + # we should only generate the header details we truely want, but
> + # since we split the entire path, we might as well make all sets
> + for p in paths:
> + plist = p.split('/')
> + try:
> + module = module[:] + [plist[0]]
> + except IndexError:
> + dummy = 0
This is much more easily written as:
try:
module.append(plist[0])
except IndexError:
pass
>...
> +++ mailer.conf.example (working copy)
> @@ -243,6 +243,18 @@
> # Set to 0 to turn off.
> #truncate_subject = 200
>
> +# specify which information you want included in extra headers
> +# for some reason author is always in the header as Author:
Hunh? We don't create an Author: header anywhere in the message. That
must be the underlying SMTP transport.
Cheers,
-g
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=1040872
Received on 2009-01-21 12:13:54 CET