Re: [PATCH v5 12/10] remote-bzr: support the new 'force' option

[Richard Hansen <rhansen@bbn.com>]

On 2013-11-11 06:51, Felipe Contreras wrote:
> Richard Hansen wrote:
>> Signed-off-by: Richard Hansen <rhansen@bbn.com>
>> ---
>>  contrib/remote-helpers/git-remote-bzr | 34 +++++++++++++++++++++++++++++++++-
>>  contrib/remote-helpers/test-bzr.sh    | 22 +++++++++++++++++++++-
>>  2 files changed, 54 insertions(+), 2 deletions(-)
>>
>> diff --git a/contrib/remote-helpers/git-remote-bzr b/contrib/remote-helpers/git-remote-bzr
>> index 7e34532..ba693d1 100755
>> --- a/contrib/remote-helpers/git-remote-bzr
>> +++ b/contrib/remote-helpers/git-remote-bzr
>> @@ -42,6 +42,7 @@ import json
>>  import re
>>  import StringIO
>>  import atexit, shutil, hashlib, urlparse, subprocess
>> +import types
>>  
>>  NAME_RE = re.compile('^([^<>]+)')
>>  AUTHOR_RE = re.compile('^([^<>]+?)? ?[<>]([^<>]*)(?:$|>)')
>> @@ -684,7 +685,8 @@ def do_export(parser):
>>                  peer = bzrlib.branch.Branch.open(peers[name],
>>                                                   possible_transports=transports)
>>                  try:
>> -                    peer.bzrdir.push_branch(branch, revision_id=revid)
>> +                    peer.bzrdir.push_branch(branch, revision_id=revid,
>> +                                            overwrite=force)
>>                  except bzrlib.errors.DivergedBranches:
>>                      print "error %s non-fast forward" % ref
>>                      continue
>> @@ -718,8 +720,34 @@ def do_capabilities(parser):
>>          print "*import-marks %s" % path
>>      print "*export-marks %s" % path
>>  
>> +    print "option"
>>      print
>>  
>> +class InvalidOptionValue(Exception):
>> +    pass
>> +
>> +def do_option(parser):
>> +    (opt, val) = parser[1:3]
>> +    handler = globals().get('do_option_' + opt)
>> +    if handler and type(handler) == types.FunctionType:
>> +        try:
>> +            handler(val)
>> +        except InvalidOptionValue:
>> +            print "error '%s' is not a valid value for option '%s'" % (val, opt)
>> +    else:
>> +        print "unsupported"
>> +
>> +def do_bool_option(val):
>> +    if val == 'true': ret = True
>> +    elif val == 'false': ret = False
>> +    else: raise InvalidOptionValue()
>> +    print "ok"
>> +    return ret
>> +
>> +def do_option_force(val):
>> +    global force
>> +    force = do_bool_option(val)
>> +
> 
> While this organization has merit, I think it's overkill for a single option,
> or just a couple of them. If in the future we add more, we might revisit this,
> for the moment something like this would suffice:

OK, I'll reroll.

> 
>     class InvalidOptionValue(Exception):
> 	pass
> 
>     def get_bool_option(val):
> 	if val == 'true':
> 	    return True
> 	elif val == 'false':
> 	    return False
> 	else:
> 	    raise InvalidOptionValue()
> 
>     def do_option(parser):
> 	global force
> 	_, key, value = parser.line.split(' ')

I'm surprised you prefer this over 'key, val = parser[1:3]' or even
'_, key, val = parser[:]'.  Are you intending to eventually remove
Parser.__getitem__()?

Thanks,
Richard


> 	try:
> 	    if key == 'force':
> 		force = get_bool_option(value)
> 		print 'ok'
> 	    else:
> 		print 'unsupported'
> 	except InvalidOptionValue:
> 	    print "error '%s' is not a valid value for option '%s'" % (value, key)
> 
> Cheers.