On 21/01/2009, at 12.13, Greg Stein wrote:
> 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.
Ours are like the above. Some projects is just a single module, others
share one or more modules. And everything is in the same subversion
repository.
> 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
Thats already possible. You dont have to get all 4, for each of the 4
you can
choose to get them or not. Though the header name will be like those
names
i choose when i made it to fit our usage. I just figured that others
would like
the feature.
> 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.
The patch can have individual group configuration. However, to get
emails
for a new branch by using a new group you have to write a new group into
the configuration file. We dont have to do that, because if someone
makes
a new branch, then the name will be put into the header of the email
automatically.
> 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).
I see your point of passing 5 seperate parameters, but someone reading
the code can understand what this parameter does, and i was expecting
everyone to always use all available headers, so i figured i should just
pass them all along.
>> +
>> 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:
I'm not sure i am ready to do what you ask.
> 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.
Why? Only the emails use it
>
>> ...
>> @@ -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.
what [x]? i do not understand. I stole the if_then_else from some other
part of the script. I think it was where the group setup the from
address.
>
>> + # 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
yeah, and much more readable. I will change that.
>
>> ...
>> +++ 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.
That may be so, but author only started showing up when i did group and
the other headers. I certainly dont remember seeing author before, and i
didnt make changes to the underlying SMTP system. But i maybe just have
overlooked the author field. But why would the SMTP system put in an
author
field? wouldnt it just put in an from address?
JonB
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=1040916
Received on 2009-01-21 12:53:02 CET