subcmds: force consistent help text format

We're inconsistent with help text as to whether it uses title case and
whether it ends in a period.  Add a test to enforce a standard, and use
the style that Python optparse & argparse use themselves (e.g. with the
--help option): always lowercase, and never trailing period.

Change-Id: Ic1defae23daeac0ac9116aaf487427f50b34050d
Reviewed-on: https://gerrit-review.googlesource.com/c/git-repo/+/305144
Reviewed-by: Raman Tenneti <rtenneti@google.com>
Tested-by: Mike Frysinger <vapier@google.com>

12 files changed, 91 insertions, 60 deletions

diff --git a/subcmds/diff.py b/subcmds/diff.py
index 4966bb1a..b400ccfd 100644
--- a/subcmds/diff.py
+++ b/subcmds/diff.py
@@ -33,7 +33,7 @@ to the Unix 'patch' command.
   def _Options(self, p):
     p.add_option('-u', '--absolute',
                  dest='absolute', action='store_true',
-                 help='Paths are relative to the repository root')
+                 help='paths are relative to the repository root')
 
   def _ExecuteOne(self, absolute, project):
     """Obtains the diff for a specific project.
diff --git a/subcmds/diffmanifests.py b/subcmds/diffmanifests.py
index 392e5972..6f23b345 100644
--- a/subcmds/diffmanifests.py
+++ b/subcmds/diffmanifests.py
@@ -68,10 +68,10 @@ synced and their revisions won't be found.
   def _Options(self, p):
     p.add_option('--raw',
                  dest='raw', action='store_true',
-                 help='Display raw diff.')
+                 help='display raw diff')
     p.add_option('--no-color',
                  dest='color', action='store_false', default=True,
-                 help='does not display the diff in color.')
+                 help='does not display the diff in color')
     p.add_option('--pretty-format',
                  dest='pretty_format', action='store',
                  metavar='<FORMAT>',
diff --git a/subcmds/forall.py b/subcmds/forall.py
index 4a631fb7..0cf3b6a6 100644
--- a/subcmds/forall.py
+++ b/subcmds/forall.py
@@ -131,30 +131,30 @@ without iterating through the remaining projects.
   def _Options(self, p):
     p.add_option('-r', '--regex',
                  dest='regex', action='store_true',
-                 help="Execute the command only on projects matching regex or wildcard expression")
+                 help='execute the command only on projects matching regex or wildcard expression')
     p.add_option('-i', '--inverse-regex',
                  dest='inverse_regex', action='store_true',
-                 help="Execute the command only on projects not matching regex or "
-                      "wildcard expression")
+                 help='execute the command only on projects not matching regex or '
+                      'wildcard expression')
     p.add_option('-g', '--groups',
                  dest='groups',
-                 help="Execute the command only on projects matching the specified groups")
+                 help='execute the command only on projects matching the specified groups')
     p.add_option('-c', '--command',
-                 help='Command (and arguments) to execute',
+                 help='command (and arguments) to execute',
                  dest='command',
                  action='callback',
                  callback=self._cmd_option)
     p.add_option('-e', '--abort-on-errors',
                  dest='abort_on_errors', action='store_true',
-                 help='Abort if a command exits unsuccessfully')
+                 help='abort if a command exits unsuccessfully')
     p.add_option('--ignore-missing', action='store_true',
-                 help='Silently skip & do not exit non-zero due missing '
+                 help='silently skip & do not exit non-zero due missing '
                       'checkouts')
 
     g = p.get_option_group('--quiet')
     g.add_option('-p',
                  dest='project_header', action='store_true',
-                 help='Show project headers before output')
+                 help='show project headers before output')
     p.add_option('--interactive',
                  action='store_true',
                  help='force interactive usage')
diff --git a/subcmds/gitc_delete.py b/subcmds/gitc_delete.py
index 56e0eaba..54b956f7 100644
--- a/subcmds/gitc_delete.py
+++ b/subcmds/gitc_delete.py
@@ -33,7 +33,7 @@ and all locally downloaded sources.
   def _Options(self, p):
     p.add_option('-f', '--force',
                  dest='force', action='store_true',
-                 help='Force the deletion (no prompt).')
+                 help='force the deletion (no prompt)')
 
   def Execute(self, opt, args):
     if not opt.force:
diff --git a/subcmds/info.py b/subcmds/info.py
index 2be56109..f7cf60fc 100644
--- a/subcmds/info.py
+++ b/subcmds/info.py
@@ -48,7 +48,7 @@ class Info(PagedCommand):
                  help=optparse.SUPPRESS_HELP)
     p.add_option('-l', '--local-only',
                  dest="local", action="store_true",
-                 help="Disable all remote operations")
+                 help="disable all remote operations")
 
   def Execute(self, opt, args):
     self.out = _Coloring(self.client.globalConfig)
diff --git a/subcmds/list.py b/subcmds/list.py
index 5cbc0c22..68bcd5e0 100644
--- a/subcmds/list.py
+++ b/subcmds/list.py
@@ -36,22 +36,22 @@ This is similar to running: repo forall -c 'echo "$REPO_PATH : $REPO_PROJECT"'.
   def _Options(self, p):
     p.add_option('-r', '--regex',
                  dest='regex', action='store_true',
-                 help="Filter the project list based on regex or wildcard matching of strings")
+                 help='filter the project list based on regex or wildcard matching of strings')
     p.add_option('-g', '--groups',
                  dest='groups',
-                 help="Filter the project list based on the groups the project is in")
+                 help='filter the project list based on the groups the project is in')
     p.add_option('-a', '--all',
                  action='store_true',
-                 help='Show projects regardless of checkout state')
+                 help='show projects regardless of checkout state')
     p.add_option('-f', '--fullpath',
                  dest='fullpath', action='store_true',
-                 help="Display the full work tree path instead of the relative path")
+                 help='display the full work tree path instead of the relative path')
     p.add_option('-n', '--name-only',
                  dest='name_only', action='store_true',
-                 help="Display only the name of the repository")
+                 help='display only the name of the repository')
     p.add_option('-p', '--path-only',
                  dest='path_only', action='store_true',
-                 help="Display only the path of the repository")
+                 help='display only the path of the repository')
 
   def ValidateOptions(self, opt, args):
     if opt.fullpath and opt.name_only:
diff --git a/subcmds/manifest.py b/subcmds/manifest.py
index e33e683c..965c36e9 100644
--- a/subcmds/manifest.py
+++ b/subcmds/manifest.py
@@ -53,27 +53,27 @@ to indicate the remote ref to push changes to via 'repo upload'.
   def _Options(self, p):
     p.add_option('-r', '--revision-as-HEAD',
                  dest='peg_rev', action='store_true',
-                 help='Save revisions as current HEAD')
+                 help='save revisions as current HEAD')
     p.add_option('-m', '--manifest-name',
                  help='temporary manifest to use for this sync', metavar='NAME.xml')
     p.add_option('--suppress-upstream-revision', dest='peg_rev_upstream',
                  default=True, action='store_false',
-                 help='If in -r mode, do not write the upstream field.  '
-                 'Only of use if the branch names for a sha1 manifest are '
-                 'sensitive.')
+                 help='if in -r mode, do not write the upstream field '
+                 '(only of use if the branch names for a sha1 manifest are '
+                 'sensitive)')
     p.add_option('--suppress-dest-branch', dest='peg_rev_dest_branch',
                  default=True, action='store_false',
-                 help='If in -r mode, do not write the dest-branch field.  '
-                 'Only of use if the branch names for a sha1 manifest are '
-                 'sensitive.')
+                 help='if in -r mode, do not write the dest-branch field '
+                 '(only of use if the branch names for a sha1 manifest are '
+                 'sensitive)')
     p.add_option('--json', default=False, action='store_true',
-                 help='Output manifest in JSON format (experimental).')
+                 help='output manifest in JSON format (experimental)')
     p.add_option('--pretty', default=False, action='store_true',
-                 help='Format output for humans to read.')
+                 help='format output for humans to read')
     p.add_option('-o', '--output-file',
                  dest='output_file',
                  default='-',
-                 help='File to save the manifest to',
+                 help='file to save the manifest to',
                  metavar='-|NAME.xml')
 
   def _Output(self, opt):
diff --git a/subcmds/overview.py b/subcmds/overview.py
index 202a5eba..b28367be 100644
--- a/subcmds/overview.py
+++ b/subcmds/overview.py
@@ -36,7 +36,7 @@ are displayed.
   def _Options(self, p):
     p.add_option('-c', '--current-branch',
                  dest="current_branch", action="store_true",
-                 help="Consider only checked out branches")
+                 help="consider only checked out branches")
     p.add_option('--no-current-branch',
                  dest='current_branch', action='store_false',
                  help='consider all local branches')
diff --git a/subcmds/rebase.py b/subcmds/rebase.py
index e0186d4d..9ce4ecb8 100644
--- a/subcmds/rebase.py
+++ b/subcmds/rebase.py
@@ -46,27 +46,27 @@ branch but need to incorporate new upstream changes "underneath" them.
 
     p.add_option('--fail-fast',
                  dest='fail_fast', action='store_true',
-                 help='Stop rebasing after first error is hit')
+                 help='stop rebasing after first error is hit')
     p.add_option('-f', '--force-rebase',
                  dest='force_rebase', action='store_true',
-                 help='Pass --force-rebase to git rebase')
+                 help='pass --force-rebase to git rebase')
     p.add_option('--no-ff',
                  dest='ff', default=True, action='store_false',
-                 help='Pass --no-ff to git rebase')
+                 help='pass --no-ff to git rebase')
     p.add_option('--autosquash',
                  dest='autosquash', action='store_true',
-                 help='Pass --autosquash to git rebase')
+                 help='pass --autosquash to git rebase')
     p.add_option('--whitespace',
                  dest='whitespace', action='store', metavar='WS',
-                 help='Pass --whitespace to git rebase')
+                 help='pass --whitespace to git rebase')
     p.add_option('--auto-stash',
                  dest='auto_stash', action='store_true',
-                 help='Stash local modifications before starting')
+                 help='stash local modifications before starting')
     p.add_option('-m', '--onto-manifest',
                  dest='onto_manifest', action='store_true',
-                 help='Rebase onto the manifest version instead of upstream '
-                      'HEAD.  This helps to make sure the local tree stays '
-                      'consistent if you previously synced to a manifest.')
+                 help='rebase onto the manifest version instead of upstream '
+                      'HEAD (this helps to make sure the local tree stays '
+                      'consistent if you previously synced to a manifest)')
 
   def Execute(self, opt, args):
     all_projects = self.GetProjects(args)
diff --git a/subcmds/sync.py b/subcmds/sync.py
index e5280865..31760e5c 100644
--- a/subcmds/sync.py
+++ b/subcmds/sync.py
@@ -169,10 +169,11 @@ later is required to fix a server side protocol bug.
   PARALLEL_JOBS = 1
 
   def _CommonOptions(self, p):
-    try:
-      self.PARALLEL_JOBS = self.manifest.default.sync_j
-    except ManifestParseError:
-      pass
+    if self.manifest:
+      try:
+        self.PARALLEL_JOBS = self.manifest.default.sync_j
+      except ManifestParseError:
+        pass
     super()._CommonOptions(p)
 
   def _Options(self, p, show_smart=True):
diff --git a/subcmds/upload.py b/subcmds/upload.py
index c497d877..fc389e42 100644
--- a/subcmds/upload.py
+++ b/subcmds/upload.py
@@ -152,61 +152,61 @@ Gerrit Code Review:  https://www.gerritcodereview.com/
   def _Options(self, p):
     p.add_option('-t',
                  dest='auto_topic', action='store_true',
-                 help='Send local branch name to Gerrit Code Review')
+                 help='send local branch name to Gerrit Code Review')
     p.add_option('--hashtag', '--ht',
                  dest='hashtags', action='append', default=[],
-                 help='Add hashtags (comma delimited) to the review.')
+                 help='add hashtags (comma delimited) to the review')
     p.add_option('--hashtag-branch', '--htb',
                  action='store_true',
-                 help='Add local branch name as a hashtag.')
+                 help='add local branch name as a hashtag')
     p.add_option('-l', '--label',
                  dest='labels', action='append', default=[],
-                 help='Add a label when uploading.')
+                 help='add a label when uploading')
     p.add_option('--re', '--reviewers',
                  type='string', action='append', dest='reviewers',
-                 help='Request reviews from these people.')
+                 help='request reviews from these people')
     p.add_option('--cc',
                  type='string', action='append', dest='cc',
-                 help='Also send email to these email addresses.')
+                 help='also send email to these email addresses')
     p.add_option('--br', '--branch',
                  type='string', action='store', dest='branch',
-                 help='(Local) branch to upload.')
+                 help='(local) branch to upload')
     p.add_option('-c', '--current-branch',
                  dest='current_branch', action='store_true',
-                 help='Upload current git branch.')
+                 help='upload current git branch')
     p.add_option('--no-current-branch',
                  dest='current_branch', action='store_false',
-                 help='Upload all git branches.')
+                 help='upload all git branches')
     # Turn this into a warning & remove this someday.
     p.add_option('--cbr',
                  dest='current_branch', action='store_true',
                  help=optparse.SUPPRESS_HELP)
     p.add_option('--ne', '--no-emails',
                  action='store_false', dest='notify', default=True,
-                 help='If specified, do not send emails on upload.')
+                 help='do not send e-mails on upload')
     p.add_option('-p', '--private',
                  action='store_true', dest='private', default=False,
-                 help='If specified, upload as a private change.')
+                 help='upload as a private change (deprecated; use --wip)')
     p.add_option('-w', '--wip',
                  action='store_true', dest='wip', default=False,
-                 help='If specified, upload as a work-in-progress change.')
+                 help='upload as a work-in-progress change')
     p.add_option('-o', '--push-option',
                  type='string', action='append', dest='push_options',
                  default=[],
-                 help='Additional push options to transmit')
+                 help='additional push options to transmit')
     p.add_option('-D', '--destination', '--dest',
                  type='string', action='store', dest='dest_branch',
                  metavar='BRANCH',
-                 help='Submit for review on this target branch.')
+                 help='submit for review on this target branch')
     p.add_option('-n', '--dry-run',
                  dest='dryrun', default=False, action='store_true',
-                 help='Do everything except actually upload the CL.')
+                 help='do everything except actually upload the CL')
     p.add_option('-y', '--yes',
                  default=False, action='store_true',
-                 help='Answer yes to all safe prompts.')
+                 help='answer yes to all safe prompts')
     p.add_option('--no-cert-checks',
                  dest='validate_certs', action='store_false', default=True,
-                 help='Disable verifying ssl certs (unsafe).')
+                 help='disable verifying ssl certs (unsafe)')
     RepoHook.AddOptionGroup(p, 'pre-upload')
 
   def _SingleBranch(self, opt, branch, people):
diff --git a/tests/test_subcmds.py b/tests/test_subcmds.py
index 2234e646..bc53051a 100644
--- a/tests/test_subcmds.py
+++ b/tests/test_subcmds.py
@@ -14,6 +14,7 @@
 
 """Unittests for the subcmds module (mostly __init__.py than subcommands)."""
 
+import optparse
 import unittest
 
 import subcmds
@@ -41,3 +42,32 @@ class AllCommands(unittest.TestCase):
 
       # Reject internal python paths like "__init__".
       self.assertFalse(cmd.startswith('__'))
+
+  def test_help_desc_style(self):
+    """Force some consistency in option descriptions.
+
+    Python's optparse & argparse has a few default options like --help.  Their
+    option description text uses lowercase sentence fragments, so enforce our
+    options follow the same style so UI is consistent.
+
+    We enforce:
+    * Text starts with lowercase.
+    * Text doesn't end with period.
+    """
+    for name, cls in subcmds.all_commands.items():
+      cmd = cls()
+      parser = cmd.OptionParser
+      for option in parser.option_list:
+        if option.help == optparse.SUPPRESS_HELP:
+          continue
+
+        c = option.help[0]
+        self.assertEqual(
+            c.lower(), c,
+            msg=f'subcmds/{name}.py: {option.get_opt_string()}: help text '
+                f'should start with lowercase: "{option.help}"')
+
+        self.assertNotEqual(
+            option.help[-1], '.',
+            msg=f'subcmds/{name}.py: {option.get_opt_string()}: help text '
+                f'should not end in a period: "{option.help}"')