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)