split out cli validation from execution
A common pattern in our subcommands is to verify the arguments &
options before executing things. For some subcommands, that check
stage is quite long which makes the execution function even bigger.
Lets split that logic out of the execute phase so it's easier to
manage these.
This is most noticeable in the sync subcommand whose Execute func
is quite large, and the option checking makes up ~15% of it.
The manifest command's Execute can be simplified significantly as
the optparse configuration always sets output_file to a string.
Change-Id: I7097847ff040e831345e63de6b467ee17609990e
Reviewed-on: https://gerrit-review.googlesource.com/c/git-repo/+/234834
Reviewed-by: David Pursehouse <dpursehouse@collab.net>
Tested-by: Mike Frysinger <vapier@google.com>
diff --git a/command.py b/command.py
index 8c5e246..f7d20a2 100644
--- a/command.py
+++ b/command.py
@@ -98,6 +98,16 @@
self.OptionParser.print_usage()
sys.exit(1)
+ def ValidateOptions(self, opt, args):
+ """Validate the user options & arguments before executing.
+
+ This is meant to help break the code up into logical steps. Some tips:
+ * Use self.OptionParser.error to display CLI related errors.
+ * Adjust opt member defaults as makes sense.
+ * Adjust the args list, but do so inplace so the caller sees updates.
+ * Try to avoid updating self state. Leave that to Execute.
+ """
+
def Execute(self, opt, args):
"""Perform the action, after option parsing is complete.
"""
diff --git a/main.py b/main.py
index 889fc21..2ab79b5 100755
--- a/main.py
+++ b/main.py
@@ -197,6 +197,7 @@
cmd_event = cmd.event_log.Add(name, event_log.TASK_COMMAND, start)
cmd.event_log.SetParent(cmd_event)
try:
+ cmd.ValidateOptions(copts, cargs)
result = cmd.Execute(copts, cargs)
except (DownloadError, ManifestInvalidRevisionError,
NoManifestException) as e:
diff --git a/subcmds/abandon.py b/subcmds/abandon.py
index 319262b..cd1d0c4 100644
--- a/subcmds/abandon.py
+++ b/subcmds/abandon.py
@@ -37,19 +37,19 @@
dest='all', action='store_true',
help='delete all branches in all projects')
- def Execute(self, opt, args):
+ def ValidateOptions(self, opt, args):
if not opt.all and not args:
self.Usage()
if not opt.all:
nb = args[0]
if not git.check_ref_format('heads/%s' % nb):
- print("error: '%s' is not a valid name" % nb, file=sys.stderr)
- sys.exit(1)
+ self.OptionParser.error("'%s' is not a valid branch name" % nb)
else:
- args.insert(0,None)
- nb = "'All local branches'"
+ args.insert(0, "'All local branches'")
+ def Execute(self, opt, args):
+ nb = args[0]
err = defaultdict(list)
success = defaultdict(list)
all_projects = self.GetProjects(args[1:])
diff --git a/subcmds/checkout.py b/subcmds/checkout.py
index 51ac483..c8a09a8 100644
--- a/subcmds/checkout.py
+++ b/subcmds/checkout.py
@@ -34,10 +34,11 @@
repo forall [<project>...] -c git checkout <branchname>
"""
- def Execute(self, opt, args):
+ def ValidateOptions(self, opt, args):
if not args:
self.Usage()
+ def Execute(self, opt, args):
nb = args[0]
err = []
success = []
diff --git a/subcmds/cherry_pick.py b/subcmds/cherry_pick.py
index 43215f9..a541a04 100644
--- a/subcmds/cherry_pick.py
+++ b/subcmds/cherry_pick.py
@@ -37,10 +37,11 @@
def _Options(self, p):
pass
- def Execute(self, opt, args):
+ def ValidateOptions(self, opt, args):
if len(args) != 1:
self.Usage()
+ def Execute(self, opt, args):
reference = args[0]
p = GitCommand(None,
diff --git a/subcmds/diffmanifests.py b/subcmds/diffmanifests.py
index cae776a..b999699 100644
--- a/subcmds/diffmanifests.py
+++ b/subcmds/diffmanifests.py
@@ -176,10 +176,11 @@
self.printText(log)
self.out.nl()
- def Execute(self, opt, args):
+ def ValidateOptions(self, opt, args):
if not args or len(args) > 2:
- self.Usage()
+ self.OptionParser.error('missing manifests to diff')
+ def Execute(self, opt, args):
self.out = _Coloring(self.manifest.globalConfig)
self.printText = self.out.nofmt_printer('text')
if opt.color:
diff --git a/subcmds/forall.py b/subcmds/forall.py
index 3e6007b..0be8d3b 100644
--- a/subcmds/forall.py
+++ b/subcmds/forall.py
@@ -177,10 +177,11 @@
'worktree': project.worktree,
}
- def Execute(self, opt, args):
+ def ValidateOptions(self, opt, args):
if not opt.command:
self.Usage()
+ def Execute(self, opt, args):
cmd = [opt.command[0]]
shell = True
diff --git a/subcmds/init.py b/subcmds/init.py
index eaa6da5..32663a0 100644
--- a/subcmds/init.py
+++ b/subcmds/init.py
@@ -436,18 +436,17 @@
print(' rm -r %s/.repo' % self.manifest.topdir)
print('and try again.')
- def Execute(self, opt, args):
- git_require(MIN_GIT_VERSION, fail=True)
-
+ def ValidateOptions(self, opt, args):
if opt.reference:
opt.reference = os.path.expanduser(opt.reference)
# Check this here, else manifest will be tagged "not new" and init won't be
# possible anymore without removing the .repo/manifests directory.
if opt.archive and opt.mirror:
- print('fatal: --mirror and --archive cannot be used together.',
- file=sys.stderr)
- sys.exit(1)
+ self.OptionParser.error('--mirror and --archive cannot be used together.')
+
+ def Execute(self, opt, args):
+ git_require(MIN_GIT_VERSION, fail=True)
self._SyncManifest(opt)
self._LinkManifest(opt.manifest_name)
diff --git a/subcmds/list.py b/subcmds/list.py
index 961b195..00172f0 100644
--- a/subcmds/list.py
+++ b/subcmds/list.py
@@ -49,6 +49,10 @@
dest='path_only', action='store_true',
help="Display only the path of the repository")
+ def ValidateOptions(self, opt, args):
+ if opt.fullpath and opt.name_only:
+ self.OptionParser.error('cannot combine -f and -n')
+
def Execute(self, opt, args):
"""List all projects and the associated directories.
@@ -60,11 +64,6 @@
opt: The options.
args: Positional args. Can be a list of projects to list, or empty.
"""
-
- if opt.fullpath and opt.name_only:
- print('error: cannot combine -f and -n', file=sys.stderr)
- sys.exit(1)
-
if not opt.regex:
projects = self.GetProjects(args, groups=opt.groups)
else:
diff --git a/subcmds/manifest.py b/subcmds/manifest.py
index 07fa323..768f072 100644
--- a/subcmds/manifest.py
+++ b/subcmds/manifest.py
@@ -73,14 +73,9 @@
if opt.output_file != '-':
print('Saved manifest to %s' % opt.output_file, file=sys.stderr)
- def Execute(self, opt, args):
+ def ValidateOptions(self, opt, args):
if args:
self.Usage()
- if opt.output_file is not None:
- self._Output(opt)
- return
-
- print('error: no operation to perform', file=sys.stderr)
- print('error: see repo help manifest', file=sys.stderr)
- sys.exit(1)
+ def Execute(self, opt, args):
+ self._Output(opt)
diff --git a/subcmds/start.py b/subcmds/start.py
index 0c60d78..5d4c9c0 100644
--- a/subcmds/start.py
+++ b/subcmds/start.py
@@ -41,15 +41,16 @@
dest='all', action='store_true',
help='begin branch in all projects')
- def Execute(self, opt, args):
+ def ValidateOptions(self, opt, args):
if not args:
self.Usage()
nb = args[0]
if not git.check_ref_format('heads/%s' % nb):
- print("error: '%s' is not a valid name" % nb, file=sys.stderr)
- sys.exit(1)
+ self.OptionParser.error("'%s' is not a valid name" % nb)
+ def Execute(self, opt, args):
+ nb = args[0]
err = []
projects = []
if not opt.all:
diff --git a/subcmds/sync.py b/subcmds/sync.py
index 3eab2fc..5655a1e 100644
--- a/subcmds/sync.py
+++ b/subcmds/sync.py
@@ -738,6 +738,24 @@
fd.close()
return 0
+ def ValidateOptions(self, opt, args):
+ if opt.force_broken:
+ print('warning: -f/--force-broken is now the default behavior, and the '
+ 'options are deprecated', file=sys.stderr)
+ if opt.network_only and opt.detach_head:
+ self.OptionParser.error('cannot combine -n and -d')
+ if opt.network_only and opt.local_only:
+ self.OptionParser.error('cannot combine -n and -l')
+ if opt.manifest_name and opt.smart_sync:
+ self.OptionParser.error('cannot combine -m and -s')
+ if opt.manifest_name and opt.smart_tag:
+ self.OptionParser.error('cannot combine -m and -t')
+ if opt.manifest_server_username or opt.manifest_server_password:
+ if not (opt.smart_sync or opt.smart_tag):
+ self.OptionParser.error('-u and -p may only be combined with -s or -t')
+ if None in [opt.manifest_server_username, opt.manifest_server_password]:
+ self.OptionParser.error('both -u and -p must be given')
+
def Execute(self, opt, args):
if opt.jobs:
self.jobs = opt.jobs
@@ -745,30 +763,6 @@
soft_limit, _ = _rlimit_nofile()
self.jobs = min(self.jobs, (soft_limit - 5) // 3)
- if opt.force_broken:
- print('warning: -f/--force-broken is now the default behavior, and the '
- 'options are deprecated', file=sys.stderr)
- if opt.network_only and opt.detach_head:
- print('error: cannot combine -n and -d', file=sys.stderr)
- sys.exit(1)
- if opt.network_only and opt.local_only:
- print('error: cannot combine -n and -l', file=sys.stderr)
- sys.exit(1)
- if opt.manifest_name and opt.smart_sync:
- print('error: cannot combine -m and -s', file=sys.stderr)
- sys.exit(1)
- if opt.manifest_name and opt.smart_tag:
- print('error: cannot combine -m and -t', file=sys.stderr)
- sys.exit(1)
- if opt.manifest_server_username or opt.manifest_server_password:
- if not (opt.smart_sync or opt.smart_tag):
- print('error: -u and -p may only be combined with -s or -t',
- file=sys.stderr)
- sys.exit(1)
- if None in [opt.manifest_server_username, opt.manifest_server_password]:
- print('error: both -u and -p must be given', file=sys.stderr)
- sys.exit(1)
-
if opt.manifest_name:
self.manifest.Override(opt.manifest_name)
