Use git push-url instead of a second remote

Having a gerrit remote means that we fetch changes twice. It's also
not necessary since the push-url can be set on the origin remote. As
a first step, add a config option that changes the default for defaultremote
to origin and then reworks the logic to add the remote url to origin as a
push-url instead of creating a gerrit remote.

Since this will cause people with an existing gerrit remote to have a
push url added and the gerrit remote orpahned, a follow on commit will
come that will look for a gerrit remote and, if it exists, use it as the
source of url for the push-url and then delete the old remote.

Change-Id: Ief5d092a796516de9605b7df64e7b902c3b47351

git_review/cmd.py

@@ -57,7 +57,7 @@ GLOBAL_CONFIG = "/etc/git-review/git-review.conf"
 USER_CONFIG = os.path.join(CONFIGDIR, "git-review.conf")
 DEFAULTS = dict(scheme='ssh', hostname=False, port=None, project=False,
                 branch='master', remote="gerrit", rebase="1",
-                track="0")
+                track="0", usepushurl="0")
 
 _branch_name = None
 _has_color = None
@@ -369,7 +369,7 @@ def make_remote_url(scheme, username, hostname, port, project):
         return "%s://%s@%s/%s" % (scheme, username, hostport, project)
 
 
-def add_remote(scheme, hostname, port, project, remote):
+def add_remote(scheme, hostname, port, project, remote, usepushurl):
     """Adds a gerrit remote."""
     asked_for_username = False
 
@@ -392,11 +392,15 @@ def add_remote(scheme, hostname, port, project, remote):
                                      "%s" % remote_url)
         asked_for_username = True
 
-    print("Creating a git remote called \"%s\" that maps to:" % remote)
+    if usepushurl:
+        cmd = "git remote set-url --push %s %s" % (remote, remote_url)
+        print("Adding a git push url to '%s' that maps to:" % remote)
+    else:
+        cmd = "git remote add -f %s %s" % (remote, remote_url)
+        print("Creating a git remote called '%s' that maps to:" % remote)
     print("\t%s" % remote_url)
-    cmd = "git remote add -f %s %s" % (remote, remote_url)
-    (status, remote_output) = run_command_status(cmd)
 
+    (status, remote_output) = run_command_status(cmd)
     if status != 0:
         raise CommandFailed(status, remote_output, cmd, {})
 
@@ -678,6 +682,7 @@ def load_config_file(config_file):
         'remote': 'defaultremote',
         'rebase': 'defaultrebase',
         'track': 'track',
+        'usepushurl': 'usepushurl',
     }
     config = {}
     for config_key, option_name in options.items():
@@ -732,42 +737,58 @@ def resolve_tracking(remote, branch):
     return remote, branch
 
 
-def check_remote(branch, remote, scheme, hostname, port, project):
+def check_remote(branch, remote, scheme, hostname, port, project,
+                 usepushurl=False):
     """Check that a Gerrit Git remote repo exists, if not, set one."""
 
-    has_color = check_color_support()
-    if has_color:
-        color_never = "--color=never"
+    if usepushurl:
+        push_url = git_config_get_value('remote.%s' % remote, 'pushurl', None)
+        if push_url:
+            return
     else:
-        color_never = ""
+        has_color = check_color_support()
+        if has_color:
+            color_never = "--color=never"
+        else:
+            color_never = ""
 
-    if remote in run_command("git remote").split("\n"):
+        if remote in run_command("git remote").split("\n"):
 
-        remotes = run_command("git branch -a %s" % color_never).split("\n")
-        for current_remote in remotes:
-            if (current_remote.strip() == "remotes/%s/%s" % (remote, branch)
-                    and not UPDATE):
-                return
-        # We have the remote, but aren't set up to fetch. Fix it
-        if VERBOSE:
-            print("Setting up gerrit branch tracking for better rebasing")
-        update_remote(remote)
-        return
+            remotes = run_command("git branch -a %s" % color_never).split("\n")
+            for current_remote in remotes:
+                remote_string = "remotes/%s/%s" % (remote, branch)
+                if (current_remote.strip() == remote_string and not UPDATE):
+                    return
+            # We have the remote, but aren't set up to fetch. Fix it
+            if VERBOSE:
+                print("Setting up gerrit branch tracking for better rebasing")
+            update_remote(remote)
+            return
 
     if hostname is False or project is False:
         # This means there was no .gitreview file
         printwrap("No '.gitreview' file found in this repository. We don't "
-                  "know where your gerrit is. Please manually create a remote "
-                  "named \"%s\" and try again." % remote)
+                  "know where your gerrit is.")
+        if usepushurl:
+            printwrap("Please set the push-url on your origin remote to the "
+                      "location of your gerrit server and try again")
+        else:
+            printwrap("Please manually create a remote "
+                      "named \"%s\" and try again." % remote)
         sys.exit(1)
 
     # Gerrit remote not present, try to add it
     try:
-        add_remote(scheme, hostname, port, project, remote)
+        add_remote(scheme, hostname, port, project, remote, usepushurl)
     except Exception:
         print(sys.exc_info()[2])
-        printwrap("We don't know where your gerrit is. Please manually create "
-                  "a remote named \"%s\" and try again." % remote)
+        if usepushurl:
+            printwrap("We don't know where your gerrit is. Please manually"
+                      " add a push-url to the '%s' remote and try again."
+                      % remote)
+        else:
+            printwrap("We don't know where your gerrit is. Please manually"
+                      " create a remote named '%s' and try again." % remote)
         raise
 
 
@@ -1294,6 +1315,10 @@ def _main():
                         help="Regenerate Change-id before submitting")
     parser.add_argument("-r", "--remote", dest="remote",
                         help="git remote to use for gerrit")
+    parser.add_argument("--use-pushurl", dest="usepushurl",
+                        action="store_true",
+                        help="Use remote push-url logic instead of separate"
+                             " remotes")
 
     rebase_group = parser.add_mutually_exclusive_group()
     rebase_group.add_argument("-R", "--no-rebase", dest="rebase",
@@ -1400,7 +1425,8 @@ def _main():
         config = Config(os.path.join(top_dir, ".gitreview"))
         parser.set_defaults(rebase=convert_bool(config['rebase']),
                             track=convert_bool(config['track']),
-                            remote=config['remote'])
+                            remote=None,
+                            usepushurl=convert_bool(config['usepushurl']))
     options = parser.parse_args()
     if no_git_dir:
         raise no_git_dir
@@ -1421,6 +1447,11 @@ def _main():
     VERBOSE = options.verbose
     UPDATE = options.update
     remote = options.remote
+    if not remote:
+        if options.usepushurl:
+            remote = 'origin'
+        else:
+            remote = config['remote']
     yes = options.yes
     status = 0
 
@@ -1428,7 +1459,8 @@ def _main():
         remote, branch = resolve_tracking(remote, branch)
 
     check_remote(branch, remote, config['scheme'],
-                 config['hostname'], config['port'], config['project'])
+                 config['hostname'], config['port'], config['project'],
+                 usepushurl=options.usepushurl)
 
     if options.color:
         set_color_output(options.color)

git_review/tests/__init__.py

@@ -131,6 +131,7 @@ class BaseGitReviewTestCase(testtools.TestCase, GerritHelpers):
     """Base class for the git-review tests."""
 
     _test_counter = 0
+    _remote = 'gerrit'
 
     @property
     def project_uri(self):
@@ -201,12 +202,18 @@ class BaseGitReviewTestCase(testtools.TestCase, GerritHelpers):
 
         # go to the just cloned test Git repository
         self._run_git('clone', self.project_uri)
-        self._run_git('remote', 'add', 'gerrit', self.project_uri)
+        self.configure_gerrit_remote()
         self.addCleanup(shutil.rmtree, self.test_dir)
 
         # ensure user is configured for all tests
         self._configure_gitreview_username()
 
+    def set_remote(self, uri):
+        self._run_git('remote', 'set-url', self._remote, uri)
+
+    def reset_remote(self):
+        self._run_git('remote', 'rm', self._remote)
+
     def attach_on_exception(self, filename):
         @self.addOnException
         def attach_file(exc_info):
@@ -290,6 +297,9 @@ class BaseGitReviewTestCase(testtools.TestCase, GerritHelpers):
         os.environ['PATH'] = self.ssh_dir + os.pathsep + os.environ['PATH']
         os.environ['GIT_SSH'] = self._dir('ssh', 'ssh')
 
+    def configure_gerrit_remote(self):
+        self._run_git('remote', 'add', self._remote, self.project_uri)
+
     def _configure_gitreview_username(self):
         self._run_git('config', 'gitreview.username', 'test_user')
 

git_review/tests/test_git_review.py

@@ -23,6 +23,39 @@ from git_review import tests
 from git_review.tests import utils
 
 
+class ConfigTestCase(tests.BaseGitReviewTestCase):
+    """Class for config tests."""
+
+    def test_get_config_from_cli(self):
+        self.reset_remote()
+        self._run_git('remote', 'rm', 'origin')
+        self._create_gitreview_file(defaultremote='remote-file')
+        self._run_git('config', 'gitreview.remote', 'remote-gitconfig')
+        self._run_git_review('-s', '-r', 'remote-cli')
+
+        remote = self._run_git('remote').strip()
+        self.assertEqual('remote-cli', remote)
+
+    def test_get_config_from_gitconfig(self):
+        self.reset_remote()
+        self._run_git('remote', 'rm', 'origin')
+        self._create_gitreview_file(defaultremote='remote-file')
+        self._run_git('config', 'gitreview.remote', 'remote-gitconfig')
+        self._run_git_review('-s')
+
+        remote = self._run_git('remote').strip()
+        self.assertEqual('remote-gitconfig', remote)
+
+    def test_get_config_from_file(self):
+        self.reset_remote()
+        self._run_git('remote', 'rm', 'origin')
+        self._create_gitreview_file(defaultremote='remote-file')
+        self._run_git_review('-s')
+
+        remote = self._run_git('remote').strip()
+        self.assertEqual('remote-file', remote)
+
+
 class GitReviewTestCase(tests.BaseGitReviewTestCase):
     """Class for the git-review tests."""
 
@@ -35,14 +68,14 @@ class GitReviewTestCase(tests.BaseGitReviewTestCase):
 
     def test_git_review_s(self):
         """Test git-review -s."""
-        self._run_git('remote', 'rm', 'gerrit')
+        self.reset_remote()
         self._run_git_review('-s')
         self._simple_change('test file modified', 'test commit message')
         self.assertIn('Change-Id:', self._run_git('log', '-1'))
 
     def test_git_review_s_in_detached_head(self):
         """Test git-review -s in detached HEAD state."""
-        self._run_git('remote', 'rm', 'gerrit')
+        self.reset_remote()
         master_sha1 = self._run_git('rev-parse', 'master')
         self._run_git('checkout', master_sha1)
         self._run_git_review('-s')
@@ -56,14 +89,14 @@ class GitReviewTestCase(tests.BaseGitReviewTestCase):
         self._run_git('reset', '--hard', 'HEAD^')
 
         # Review setup with an outdated repo
-        self._run_git('remote', 'rm', 'gerrit')
+        self.reset_remote()
         self._run_git_review('-s')
         self._simple_change('test file modified', 'test commit message 2')
         self.assertIn('Change-Id:', self._run_git('log', '-1'))
 
     def test_git_review_s_from_subdirectory(self):
         """Test git-review -s from subdirectory."""
-        self._run_git('remote', 'rm', 'gerrit')
+        self.reset_remote()
         utils.run_cmd('mkdir', 'subdirectory', chdir=self.test_dir)
         self._run_git_review(
             '-s', chdir=os.path.join(self.test_dir, 'subdirectory'))
@@ -81,7 +114,7 @@ class GitReviewTestCase(tests.BaseGitReviewTestCase):
 
         # download clean Git repository and fresh change from Gerrit to it
         self._run_git('clone', self.project_uri)
-        self._run_git('remote', 'add', 'gerrit', self.project_uri)
+        self.configure_gerrit_remote()
         self._run_git_review('-d', change_id)
         self.assertIn('test commit message', self._run_git('log', '-1'))
 
@@ -94,7 +127,7 @@ class GitReviewTestCase(tests.BaseGitReviewTestCase):
         # and branch is tracking
         head = self._run_git('symbolic-ref', '-q', 'HEAD')
         self.assertIn(
-            'refs/remotes/gerrit/master',
+            'refs/remotes/%s/master' % self._remote,
             self._run_git("for-each-ref", "--format='%(upstream)'", head))
 
     def test_multiple_changes(self):
@@ -164,9 +197,11 @@ class GitReviewTestCase(tests.BaseGitReviewTestCase):
         """Test message displayed where no remote branch exists."""
         self._run_git_review('-s')
         self._run_git('checkout', '-b', 'new_branch')
+        self._simple_change('simple message',
+                            'need to avoid noop message')
         exc = self.assertRaises(Exception, self._run_git_review, 'new_branch')
         self.assertIn("The branch 'new_branch' does not exist on the given "
-                      "remote 'gerrit'", exc.args[0])
+                      "remote '%s'" % self._remote, exc.args[0])
 
     def test_need_rebase_no_upload(self):
         """Test change needing a rebase does not upload."""
@@ -179,8 +214,9 @@ class GitReviewTestCase(tests.BaseGitReviewTestCase):
                             'create conflict with master')
 
         exc = self.assertRaises(Exception, self._run_git_review)
-        self.assertIn("Errors running git rebase -p -i remotes/gerrit/master",
-                      exc.args[0])
+        self.assertIn(
+            "Errors running git rebase -p -i remotes/%s/master" % self._remote,
+            exc.args[0])
         self.assertIn("It is likely that your change has a merge conflict.",
                       exc.args[0])
 
@@ -196,8 +232,9 @@ class GitReviewTestCase(tests.BaseGitReviewTestCase):
                             self._dir('test', 'new_test_file.txt'))
 
         review_res = self._run_git_review('-v')
-        self.assertIn("Running: git rebase -p -i remotes/gerrit/master",
-                      review_res)
+        self.assertIn(
+            "Running: git rebase -p -i remotes/%s/master" % self._remote,
+            review_res)
         self.assertEqual(self._run_git('rev-parse', 'HEAD^1'), head_1)
 
     def test_uploads_with_nondefault_rebase(self):
@@ -473,53 +510,43 @@ class GitReviewTestCase(tests.BaseGitReviewTestCase):
     def test_git_review_F_R(self):
         self.assertRaises(Exception, self._run_git_review, '-F', '-R')
 
-    def test_get_config_from_cli(self):
-        self._run_git('remote', 'rm', 'origin')
-        self._run_git('remote', 'rm', 'gerrit')
-        self._create_gitreview_file(defaultremote='remote-file')
-        self._run_git('config', 'gitreview.remote', 'remote-gitconfig')
-        self._run_git_review('-s', '-r', 'remote-cli')
-
-        remote = self._run_git('remote').strip()
-        self.assertEqual('remote-cli', remote)
-
-    def test_get_config_from_gitconfig(self):
-        self._run_git('remote', 'rm', 'origin')
-        self._run_git('remote', 'rm', 'gerrit')
-        self._create_gitreview_file(defaultremote='remote-file')
-        self._run_git('config', 'gitreview.remote', 'remote-gitconfig')
-        self._run_git_review('-s')
-
-        remote = self._run_git('remote').strip()
-        self.assertEqual('remote-gitconfig', remote)
-
-    def test_get_config_from_file(self):
-        self._run_git('remote', 'rm', 'origin')
-        self._run_git('remote', 'rm', 'gerrit')
-        self._create_gitreview_file(defaultremote='remote-file')
-        self._run_git_review('-s')
-
-        remote = self._run_git('remote').strip()
-        self.assertEqual('remote-file', remote)
-
     def test_config_instead_of_honored(self):
-        self._run_git('remote', 'add', 'alias', 'test_project_url')
+        self.set_remote('test_project_url')
 
-        exc = self.assertRaises(Exception, self._run_git_review,
-                                '-l', '-r', 'alias')
-        self.assertIn("'test_project_url' does not appear to be a git "
-                      "repository", exc.args[0])
+        self.assertRaises(Exception, self._run_git_review, '-l')
 
         self._run_git('config', '--add', 'url.%s.insteadof' % self.project_uri,
                       'test_project_url')
-        self._run_git_review('-l', '-r', 'alias')
+        self._run_git_review('-l')
+
+    def test_config_pushinsteadof_honored(self):
+        self.set_remote('test_project_url')
+
+        self.assertRaises(Exception, self._run_git_review, '-l')
 
-        self._run_git('config', '--unset',
-                      'url.%s.insteadof' % self.project_uri)
         self._run_git('config', '--add',
                       'url.%s.pushinsteadof' % self.project_uri,
                       'test_project_url')
-        self._run_git_review('-l', '-r', 'alias')
+        self._run_git_review('-l')
+
+
+class PushUrlTestCase(GitReviewTestCase):
+    """Class for the git-review tests using origin push-url."""
+
+    _remote = 'origin'
+
+    def set_remote(self, uri):
+        self._run_git('remote', 'set-url', '--push', self._remote, uri)
+
+    def reset_remote(self):
+        self._run_git('config', '--unset', 'remote.%s.pushurl' % self._remote)
+
+    def configure_gerrit_remote(self):
+        self.set_remote(self.project_uri)
+        self._run_git('config', 'gitreview.usepushurl', '1')
+
+    def test_config_pushinsteadof_honored(self):
+        self.skipTest("pushinsteadof doesn't rewrite pushurls")
 
 
 class HttpGitReviewTestCase(tests.HttpMixin, GitReviewTestCase):