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

2013-11-11 Thread Felipe Contreras 
Richard Hansen wrote:
 Signed-off-by: Richard Hansen rhan...@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:

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(' ')
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.

-- 
Felipe Contreras
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2013-11-11 Thread Richard Hansen 
On 2013-11-11 06:51, Felipe Contreras wrote:
 Richard Hansen wrote:
 Signed-off-by: Richard Hansen rhan...@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.

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2013-11-11 Thread Felipe Contreras 
On Mon, Nov 11, 2013 at 12:12 PM, Richard Hansen rhan...@bbn.com wrote:
 On 2013-11-11 06:51, Felipe Contreras wrote:

 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__()?

I don't, actually. I'm fine with either way.

Cheers.

-- 
Felipe Contreras
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2013-11-10 Thread Richard Hansen 
Signed-off-by: Richard Hansen rhan...@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)
+
 def ref_is_valid(name):
 return not True in [c in name for c in '~^: \\']
 
@@ -882,6 +910,7 @@ def main(args):
 global is_tmp
 global branches, peers
 global transports
+global force
 
 alias = args[1]
 url = args[2]
@@ -895,6 +924,7 @@ def main(args):
 branches = {}
 peers = {}
 transports = []
+force = False
 
 if alias[5:] == url:
 is_tmp = True
@@ -930,6 +960,8 @@ def main(args):
 do_import(parser)
 elif parser.check('export'):
 do_export(parser)
+elif parser.check('option'):
+do_option(parser)
 else:
 die('unhandled command: %s' % line)
 sys.stdout.flush()
diff --git a/contrib/remote-helpers/test-bzr.sh 
b/contrib/remote-helpers/test-bzr.sh
index ea597b0..7d7778f 100755
--- a/contrib/remote-helpers/test-bzr.sh
+++ b/contrib/remote-helpers/test-bzr.sh
@@ -69,13 +69,33 @@ test_expect_success 'pushing' '
test_cmp expected actual
 '
 
+test_expect_success 'forced pushing' '
+   (
+   cd gitrepo 
+   echo three-new content 
+   git commit -a --amend -m three-new 
+   git push -f
+   ) 
+
+   (
+   cd bzrrepo 
+   # the forced update overwrites the bzr branch but not the bzr
+   # working directory (it tries to merge instead)
+   bzr revert
+   ) 
+
+   echo three-new expected 
+   cat bzrrepo/content actual 
+   test_cmp expected actual
+'
+
 test_expect_success 'roundtrip' '
(
cd gitrepo 
git pull 
git log --format=%s -1 origin/master actual
) 
-   echo three expected 
+   echo three-new expected 
test_cmp expected actual 
 
(cd gitrepo  git push  git pull) 
-- 
1.8.5.rc1.207.gc17dd22

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html