Thanks for your review. Comments blow and new patch attached. On Fri, Aug 20, 2010 at 8:49 PM, Marcus Hüwe wrote:

Hi,

On 2010-08-20 20:10:40 +0800, JF Ding wrote:

...

 Please help me to review this patch to make "osc vc" to  support the MeeGo style changelog, thanks!

I've some small remarks about the your patch:

+            fn_changelog = glob.glob('*.changes')[0]

You should also take the "args" argument into consideration. "args" can be a path to dir, a path to a *.changes file or it's omitted. Otherwise this code might break the old behaviour if there are multiple *.changes files in the package. Yes, as your reminding, I checked the code of /usr/lib/build/vc and found args was important here. But for MeeGo's vc scripts, no args needed. So I decide to detect MeeGo style only when no args specified.

+        if not meego_style: +            if opts.just_edit: +                cmd_list.append("-e")

It's probably better to do something like:

if meego_style and opts.just_edit:    raise oscerr.WrongOptions(...) elif opts.just_edit:    cmd_list.append('-e')

For MeeGo's vc script, only "-m" option is needed, but it's rarely used. So I decide to ignore all options for MeeGo's vc script.

Otherwise the patch looks good to me.

The new patch is attached and pasted in body, please help me review again, thanks!

From f5c60dda64229558f84b6fb4236eca89351b0ecf Mon Sep 17 00:00:00 2001 From: JF Ding Date: Fri, 20 Aug 2010 19:00:57 +0800 Subject: [PATCH] vc: to support meego changelog style if detected

need "vc" command shipped with meego-packaging-tools --- osc/commandline.py | 59 ++++++++++++++++++++++++++++++++++++--------------- 1 files changed, 41 insertions(+), 18 deletions(-) diff --git a/osc/commandline.py b/osc/commandline.py index d50b45f..34e1db7 100644 --- a/osc/commandline.py +++ b/osc/commandline.py @@ -5618,12 +5618,31 @@ Please submit there instead, or use --nodevelproject to force direct submission. from subprocess import Popen, PIPE - if not os.path.exists('/usr/lib/build/vc'): - print >>sys.stderr, 'Error: you need build.rpm with version 2009.04.17 or newer' - print >>sys.stderr, 'See http://download.opensuse.org/repositories/openSUSE:/Tools/' - return 1 + meego_style = False + if not args: + import glob, re + try: + fn_changelog = glob.glob('*.changes')[0] + fp = file(fn_changelog) + titleline = fp.readline() + fp.close() + if re.match('^\*\W+(.+\W+\d{1,2}\W+20\d{2})\W+(.+)\W+<(.+)>\W+(.+)$', titleline): + meego_style = True + except IndexError: + pass - cmd_list = ["/usr/lib/build/vc", ] + if meego_style: + if not os.path.exists('/usr/bin/vc'): + print >>sys.stderr, 'Error: you need meego-packaging-tools for /usr/bin/vc command' + return 1 + cmd_list = ["/usr/bin/vc", ] + else: + if not os.path.exists('/usr/lib/build/vc'): + print >>sys.stderr, 'Error: you need build.rpm with version 2009.04.17 or newer' + print >>sys.stderr, 'See http://download.opensuse.org/repositories/openSUSE:/Tools/' + return 1 + + cmd_list = ["/usr/lib/build/vc", ] if len(args) > 0: arg = args[0] @@ -5640,24 +5659,28 @@ Please submit there instead, or use --nodevelproject to force direct submission. user = conf.get_apiurl_usr(apiurl) - # work with all combinations of URL with or withouth the ending slash + try: + os.environ['mailaddr'], os.environ['username'] = get_user_data(apiurl, user, 'email', 'realname') + except Exception, e: + sys.exit('%s\nget_user_data(email) failed. Try env mailaddr=....\n' % e) + + # mailaddr can be overrided by config one if conf.config['api_host_options'][apiurl].has_key('email'): os.environ['mailaddr'] = conf.config['api_host_options'][apiurl]['email'] - else: - try: - os.environ['mailaddr'] = get_user_data(apiurl, user, 'email')[0] - except Exception, e: - sys.exit('%s\nget_user_data(email) failed. Try env mailaddr=....\n' % e) - if opts.message: - cmd_list.append("-m") - cmd_list.append(opts.message) + if meego_style: + if opts.message or opts.just_edit: + print >>sys.stderr, 'Warning: to edit MeeGo style changelog, opts will be ignored.' + else: + if opts.message: + cmd_list.append("-m") + cmd_list.append(opts.message) - if opts.just_edit: - cmd_list.append("-e") + if opts.just_edit: + cmd_list.append("-e") - if args: - cmd_list.extend(args) + if args: + cmd_list.extend(args) vc = Popen(cmd_list) vc.wait() -- 1.7.0.4