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

On 2013-11-11 06:51, Felipe Contreras wrote:

Richard Hansen wrote:

quoted

Signed-off-by: Richard Hansen <redacted>
---
 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.