split out cli validation from executionv1.13.5

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>

12 files changed, 55 insertions, 52 deletions

diff --git a/command.py b/command.py
index 8c5e2461..f7d20a22 100644
--- a/command.py
+++ b/command.py
@@ -98,6 +98,16 @@ class Command(object):
     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 889fc216..2ab79b57 100755
--- a/main.py
+++ b/main.py
@@ -197,6 +197,7 @@ class _Repo(object):
     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 319262bd..cd1d0c40 100644
--- a/subcmds/abandon.py
+++ b/subcmds/abandon.py
@@ -37,19 +37,19 @@ It is equivalent to "git branch -D <branchname>".
                  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 51ac4833..c8a09a8e 100644
--- a/subcmds/checkout.py
+++ b/subcmds/checkout.py
@@ -34,10 +34,11 @@ The command is equivalent to:
   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 43215f91..a541a040 100644
--- a/subcmds/cherry_pick.py
+++ b/subcmds/cherry_pick.py
@@ -37,10 +37,11 @@ change id will be added.
   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 cae776a7..b999699e 100644
--- a/subcmds/diffmanifests.py
+++ b/subcmds/diffmanifests.py
@@ -176,10 +176,11 @@ synced and their revisions won't be found.
             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 3e6007ba..0be8d3bc 100644
--- a/subcmds/forall.py
+++ b/subcmds/forall.py
@@ -177,10 +177,11 @@ without iterating through the remaining projects.
       '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 eaa6da50..32663a04 100644
--- a/subcmds/init.py
+++ b/subcmds/init.py
@@ -436,18 +436,17 @@ to update the working directory files.
       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 961b1956..00172f0e 100644
--- a/subcmds/list.py
+++ b/subcmds/list.py
@@ -49,6 +49,10 @@ This is similar to running: repo forall -c 'echo "$REPO_PATH : $REPO_PROJECT"'.
                  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 @@ This is similar to running: repo forall -c 'echo "$REPO_PATH : $REPO_PROJECT"'.
       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 07fa323a..768f072e 100644
--- a/subcmds/manifest.py
+++ b/subcmds/manifest.py
@@ -73,14 +73,9 @@ in a Git repository for use during future 'repo init' invocations.
     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 0c60d78c..5d4c9c01 100644
--- a/subcmds/start.py
+++ b/subcmds/start.py
@@ -41,15 +41,16 @@ revision specified in the manifest.
                  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 3eab2fcf..5655a1e6 100644
--- a/subcmds/sync.py
+++ b/subcmds/sync.py
@@ -738,36 +738,30 @@ later is required to fix a server side protocol bug.
       fd.close()
     return 0
 
-  def Execute(self, opt, args):
-    if opt.jobs:
-      self.jobs = opt.jobs
-    if self.jobs > 1:
-      soft_limit, _ = _rlimit_nofile()
-      self.jobs = min(self.jobs, (soft_limit - 5) // 3)
-
+  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:
-      print('error: cannot combine -n and -d', file=sys.stderr)
-      sys.exit(1)
+      self.OptionParser.error('cannot combine -n and -d')
     if opt.network_only and opt.local_only:
-      print('error: cannot combine -n and -l', file=sys.stderr)
-      sys.exit(1)
+      self.OptionParser.error('cannot combine -n and -l')
     if opt.manifest_name and opt.smart_sync:
-      print('error: cannot combine -m and -s', file=sys.stderr)
-      sys.exit(1)
+      self.OptionParser.error('cannot combine -m and -s')
     if opt.manifest_name and opt.smart_tag:
-      print('error: cannot combine -m and -t', file=sys.stderr)
-      sys.exit(1)
+      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):
-        print('error: -u and -p may only be combined with -s or -t',
-              file=sys.stderr)
-        sys.exit(1)
+        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]:
-        print('error: both -u and -p must be given', file=sys.stderr)
-        sys.exit(1)
+        self.OptionParser.error('both -u and -p must be given')
+
+  def Execute(self, opt, args):
+    if opt.jobs:
+      self.jobs = opt.jobs
+    if self.jobs > 1:
+      soft_limit, _ = _rlimit_nofile()
+      self.jobs = min(self.jobs, (soft_limit - 5) // 3)
 
     if opt.manifest_name:
       self.manifest.Override(opt.manifest_name)