[Svnmerge] [PATCH] Prompt for source branch when multiple sourcesexist

[Daniel Rall]

On Fri, 31 Mar 2006, Jim Lawton wrote:

> Giovanni,
> 
> I've attached the same patch regenerated against r19109 to make things easier.
> Is it fit to be committed?

I'm not a fan of this interactivity (why is it necessary?), but will
review this patch.  Some comments:


Index: contrib/client-side/svnmerge.py
===================================================================
--- contrib/client-side/svnmerge.py	(revision 19109)
+++ contrib/client-side/svnmerge.py	(working copy)
@@ -727,6 +727,9 @@
     props = branch_props.copy()
     dir = url_to_rlpath(target_to_url(branch_dir))
 
+    index = 0
+    source = props.keys()[0]
+

These local variables aren't used until the "else" block below.  Why
scope'em so wide?

     # To make bidirectional merges easier, find the target's
     # repository local path so it can be removed from the list of
     # possible integration sources.
@@ -734,10 +737,38 @@
         del props[dir]
 
     if len(props) > 1:
-        error('multiple heads found. '
-              'Explicit head argument (-S/--head) required.')
+        # Multiple sources found.
+        if not opts["interactive"]:

Why use negative logic here?  I'd prefer the block order simply be
swapped, and an 'if opts["interactive"]:' be used for the flow
control.

+            # Non-interactive, bail.
+            error('multiple heads found. '
+                  'Explicit head argument (-S/--head) required.')
+        else:
+            # Interactive, promot for a source.
+            print 'Multiple sources found. Please select one:'
+            for prop in props.keys():
+                print ' [' + str(index) + '] ' + prop
+                index = index + 1
+
+            input = None
+            while input is None:
+                input = raw_input ('0-' + str(len(props)-1) + ' [0]: ')

Why 0-N instead of 1-(N+1)?

+                if not input:
+                    input = "0"

...which would mean the default would change to 1.

+                    break
+                try:
+                    input = int(input)
+                    if not 0 <= input < len(props):
+                        raise ValueError
+                except ValueError:
+                    print 'Please enter a value between 0 and ' + str(len(props)-1) + '!'
+                    input = None

Is the use of exceptions appropriate here (e.g. does it make the code
simpler or something)?

+                else:
+                    break
+
+            select = int(input)
+            source = props.keys()[select]
 
The local variable "select" isn't necessary, but seems to be used to
improve comprehensibility.  Along those lines, how about "selection"
instead of "select", and "src_url" instead of "source"?

-    return props.keys()[0]
+    return source
 
 def check_old_prop_version(branch_dir, props):
     """Check if props (of branch_dir) are svnmerge properties in old format,
@@ -1402,6 +1433,10 @@
            help="show subversion commands that make changes"),
     Option("-v", "--verbose",
            help="verbose mode: output more information about progress"),
+    Option("-i", "--interactive",
+           value=True,
+           default=False,

Why wouldn't interactivity be the default?  I'd rather specify
--non-interactive (Subversion-style), or --interactive=false in a
script than have to constantly specify it on the command-line.

+           help="interactive mode: prompt for input if necessary"),
 ]
 
 common_opts = [

-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 191 bytes
Desc: not available
Url : /pipermail/svnmerge/attachments/20060412/96a63428/attachment.pgp