opendev/git-review
Code Issues Proposed changes

Prefer git-config over git-review config files

Browse Source 
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
This commit is contained in:
Cedric Brandily 2014-08-18 01:38:22 +02:00
parent abe76bfe28
commit 1bc87f26d6
5 changed files with 89 additions and 33 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

22
git-review.1
View File

@ -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.

61
git_review/cmd.py
View File

@ -56,8 +56,7 @@ CONFIGDIR = os.path.expanduser("~/.config/git-review")
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 _main():
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

8
git_review/tests/__init__.py
View File

@ -274,16 +274,18 @@ class BaseGitReviewTestCase(testtools.TestCase, GerritHelpers):
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())

29
git_review/tests/test_git_review.py
View File

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

2
git_review/tests/test_unit.py
View File

@ -46,7 +46,7 @@ class ConfigTestCase(testtools.TestCase):
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)