git cl patch: when URL matches both codereview systems, choose rietveld.

This is temporary, and will be changed in subsequent CLs. For now,
it suffices to stop relying on pseudo-random dictionary order in tests
and prod.

R=jochen@chromium.org
BUG=706406

Change-Id: I26c467a28bc63b5f81d20fc222a2b6f0511c507f
Reviewed-on: https://chromium-review.googlesource.com/472750
Reviewed-by: Jochen Eisinger <jochen@chromium.org>
Commit-Queue: Andrii Shyshkalov <tandrii@chromium.org>

git_cl.py

@@ -1061,11 +1061,19 @@ def ParseIssueNumberArgument(arg):
     parsed_url = urlparse.urlparse(url)
   except ValueError:
     return fail_result
-  for cls in _CODEREVIEW_IMPLEMENTATIONS.itervalues():
-    tmp = cls.ParseIssueURL(parsed_url)
-    if tmp is not None:
-      return tmp
-  return fail_result
+
+  results = {}
+  for name, cls in _CODEREVIEW_IMPLEMENTATIONS.iteritems():
+    parsed = cls.ParseIssueURL(parsed_url)
+    if parsed is not None:
+      results[name] = parsed
+
+  if not results:
+    return fail_result
+  if len(results) == 1:
+    return results.values()[0]
+  # Choose Rietveld if there are two.
+  return results['rietveld']
 
 
 class GerritChangeNotExists(Exception):
@@ -4410,7 +4418,7 @@ def CMDdescription(parser, args):
   if len(args) > 0:
     target_issue_arg = ParseIssueNumberArgument(args[0])
     if not target_issue_arg.valid:
-      parser.print_help()
+      parser.error('invalid codereview url or CL id')
       return 1
 
   auth_config = auth.extract_auth_config_from_options(options)
@@ -5739,7 +5747,7 @@ def CMDcheckout(parser, args):
 
   issue_arg = ParseIssueNumberArgument(args[0])
   if not issue_arg.valid:
-    parser.print_help()
+    parser.error('invalid codereview url or CL id')
     return 1
   target_issue = str(issue_arg.issue)
 

tests/git_cl_test.py

@@ -301,9 +301,10 @@ class TestGitClBasic(unittest.TestCase):
     test('123a', fail=True)
     test('ssh://chrome-review.source.com/#/c/123/4/', fail=True)
     # Rietveld.
+    test('https://codereview.source.com/www123', fail=True)
+    # Matches both, but we take Rietveld now.
     test('https://codereview.source.com/123',
          123, None, 'codereview.source.com')
-    test('https://codereview.source.com/www123', fail=True)
     # Gerrrit.
     test('https://chrome-review.source.com/c/123/4',
          123, 4, 'chrome-review.source.com')
@@ -2265,7 +2266,7 @@ class TestGitCl(TestCase):
     self.mock(git_cl.Changelist, 'GetDescription', lambda *args: 'foobar')
 
     self.assertEqual(0, git_cl.main([
-        'description', 'https://code.review.org/123123', '-d', '--rietveld']))
+        'description', '-d', '--rietveld', 'https://code.review.org/123123']))
     self.assertEqual('foobar\n', out.getvalue())
 
   def test_StatusFieldOverrideIssueMissingArgs(self):