Prefer git-config over git-review config files

git-review gets its configuration from multiple sources (by priority):

 * command line options
 * git-config
  * options: username, rebase
  * with local/global/system config
 * git-review config files
  * options: scheme, host, port, project, defaultbranch/remote/rebase
  * locally using .gitreview
  * globally using ~/.config/gitreview/git-review.conf
  * system using /etc/git-review/git-review.conf

.gitreview file is commonly provided versioned in the git repo, where
other git-review config files and git-config is done by developers. It
implies for developers multiple places to define local/global/system
configuration options but some can only be changed by updating
.gitreview (scheme/port...) which is under versioning.

This change proposes to use as configuration sources (by priority):

 * command line options
 * git-config for developer local/global/system config
  * options: username, rebase and all .gitreview options
 * .gitreview local file for repo provided specific config

and deprecates git-review global/system files in order to reduce
configuration sources and allow to overload .gitreview configuration
without updating it.

Change-Id: I24aba0fa089de57d51a79049d9c192f76d576f69

git-review.1

@@ -181,6 +181,24 @@ file:
 [gitreview]
 username=\fImygerrituser\fP
 .Ed
+.It gitreview.scheme
+This setting determines the default scheme (ssh/http/https) of gerrit remote
+.Ed
+.It gitreview.host
+This setting determines the default hostname of gerrit remote
+.Ed
+.It gitreview.port
+This setting determines the default port of gerrit remote
+.Ed
+.It gitreview.project
+This setting determines the default name of gerrit git repo
+.Ed
+.It gitreview.remote
+This setting determines the default name to use for gerrit remote
+.Ed
+.It gitreview.branch
+This setting determines the default branch
+.Ed
 .It gitreview.rebase
 This setting determines whether changes submitted will
 be rebased to the newest state of the branch.
@@ -232,6 +250,7 @@ not to rebase changes by default (same as the
 command line option)
 .Bd -literal -offset indent
 [gerrit]
+scheme=ssh
 host=review.example.com
 port=29418
 project=department/project.git
@@ -240,6 +259,9 @@ defaultremote=review
 defaultrebase=0
 .Ed
 .Pp
+When the same option is provided through FILES and CONFIGURATION, the
+CONFIGURATION value wins.
+.Pp
 .Sh DIAGNOSTICS
 .Pp
 Normally, exit status is 0 if executed successfully.

git_review/cmd.py

@@ -56,8 +56,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,
-                defaultbranch='master', defaultremote="gerrit",
-                defaultrebase="1")
+                branch='master', remote="gerrit", rebase="1")
 
 _branch_name = None
 _has_color = None
@@ -240,6 +239,29 @@ def git_config_get_value(section, option, default=None, as_bool=False):
         raise
 
 
+class Config(object):
+    """Expose as dictionary configuration options."""
+
+    def __init__(self, config_file=None):
+        self.config = DEFAULTS.copy()
+        filenames = [] if LOCAL_MODE else [GLOBAL_CONFIG, USER_CONFIG]
+        if config_file:
+            filenames.append(config_file)
+        for filename in filenames:
+            if os.path.exists(filename):
+                if filename != config_file:
+                    msg = ("Using global/system git-review config files (%s) "
+                           "is deprecated")
+                    print(msg % filename)
+                self.config.update(load_config_file(filename))
+
+    def __getitem__(self, key):
+        value = git_config_get_value('gitreview', key)
+        if value is None:
+            value = self.config[key]
+        return value
+
+
 class GitConfigException(CommandFailed):
     """Git config value retrieval failed."""
     EXIT_CODE = 128
@@ -519,22 +541,6 @@ def check_color_support():
     return _has_color
 
 
-def get_config(config_file=None):
-    """Generate the configuration map by starting with some built-in defaults
-    and then loading GLOBAL_CONFIG, USER_CONFIG, and a repository-specific
-    .gitreview file, if they exist. In case of conflict, the configuration file
-    with the narrowest scope wins.
-    """
-    config = DEFAULTS.copy()
-    filenames = [] if LOCAL_MODE else [GLOBAL_CONFIG, USER_CONFIG]
-    if config_file:
-        filenames.append(config_file)
-    for filename in filenames:
-        if os.path.exists(filename):
-            config.update(load_config_file(filename))
-    return config
-
-
 def load_config_file(config_file):
     """Load configuration options from a file."""
     configParser = ConfigParser.ConfigParser()
@@ -544,9 +550,9 @@ def load_config_file(config_file):
         'hostname': 'host',
         'port': 'port',
         'project': 'project',
-        'defaultbranch': 'defaultbranch',
-        'defaultremote': 'defaultremote',
-        'defaultrebase': 'defaultrebase',
+        'branch': 'defaultbranch',
+        'remote': 'defaultremote',
+        'rebase': 'defaultrebase',
     }
     config = {}
     for config_key, option_name in options.items():
@@ -1047,7 +1053,7 @@ def finish_branch(target_branch):
 
 def convert_bool(one_or_zero):
     "Return a bool on a one or zero string."
-    return one_or_zero in ["1", "true", "True"]
+    return str(one_or_zero) in ["1", "true", "True"]
 
 
 def _main():
@@ -1167,13 +1173,10 @@ def __call__(self, parser, namespace, values, option_string=None):
         pass
     else:
         no_git_dir = False
-        config = get_config(os.path.join(top_dir, ".gitreview"))
-        defaultrebase = convert_bool(
-            git_config_get_value("gitreview", "rebase",
-                                 default=str(config['defaultrebase'])))
-        parser.set_defaults(rebase=defaultrebase,
-                            branch=config['defaultbranch'],
-                            remote=config['defaultremote'])
+        config = Config(os.path.join(top_dir, ".gitreview"))
+        parser.set_defaults(branch=config['branch'],
+                            rebase=convert_bool(config['rebase']),
+                            remote=config['remote'])
     options = parser.parse_args()
     if no_git_dir:
         raise no_git_dir

git_review/tests/__init__.py

@@ -274,16 +274,18 @@ def _pick_gerrit_port_and_dir(self):
         host = '127.%s.%s.%s' % (self._test_counter, pid >> 8, pid & 255)
         return host, 29418, host, 8080, self._dir('gerrit', 'site-' + host)
 
-    def _create_gitreview_file(self):
+    def _create_gitreview_file(self, **kwargs):
         cfg = ('[gerrit]\n'
                'scheme=%s\n'
                'host=%s\n'
                'port=%s\n'
-               'project=test/test_project.git')
+               'project=test/test_project.git\n'
+               '%s')
         parsed = urlparse(self.project_uri)
         host_port = parsed.netloc.rpartition('@')[-1]
         host, __, port = host_port.partition(':')
-        cfg %= parsed.scheme, host, port
+        extra = '\n'.join('%s=%s' % kv for kv in kwargs.items())
+        cfg %= parsed.scheme, host, port, extra
         utils.write_to_file(self._dir('test', '.gitreview'), cfg.encode())
 
 

git_review/tests/test_git_review.py

@@ -302,6 +302,35 @@ def test_git_review_F_norebase(self):
     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)
+
 
 class HttpGitReviewTestCase(tests.HttpMixin, GitReviewTestCase):
     """Class for the git-review tests over HTTP(S)."""

git_review/tests/test_unit.py

@@ -46,7 +46,7 @@ def test_git_local_mode(self, run_mock, dir_mock):
                 mock.PropertyMock(return_value=True))
     @mock.patch('os.path.exists', return_value=False)
     def test_gitreview_local_mode(self, exists_mock):
-        cmd.get_config()
+        cmd.Config()
         self.assertFalse(exists_mock.called)