subcmds: delete redundant dest= settings

Add a test to enforce this too.

Change-Id: I80b5cf567aa33db9c24b53428c66d69f9c1d8d74
Reviewed-on: https://gerrit-review.googlesource.com/c/git-repo/+/478481
Commit-Queue: Mike Frysinger <vapier@google.com>
Tested-by: Mike Frysinger <vapier@google.com>
Reviewed-by: Scott Lee <ddoman@google.com>

subcmds/abandon.py

@@ -48,7 +48,6 @@ It is equivalent to "git branch -D <branchname>".
     def _Options(self, p):
         p.add_option(
             "--all",
-            dest="all",
             action="store_true",
             help="delete all branches in all projects",
         )

subcmds/diff.py

@@ -35,7 +35,6 @@ to the Unix 'patch' command.
         p.add_option(
             "-u",
             "--absolute",
-            dest="absolute",
             action="store_true",
             help="paths are relative to the repository root",
         )

subcmds/diffmanifests.py

@@ -67,7 +67,9 @@ 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"
+            "--raw",
+            action="store_true",
+            help="display raw diff",
         )
         p.add_option(
             "--no-color",
@@ -78,7 +80,6 @@ synced and their revisions won't be found.
         )
         p.add_option(
             "--pretty-format",
-            dest="pretty_format",
             action="store",
             metavar="<FORMAT>",
             help="print the log using a custom git pretty format string",

subcmds/download.py

@@ -60,7 +60,6 @@ If no project is specified try to use current directory as a project.
         p.add_option(
             "-r",
             "--revert",
-            dest="revert",
             action="store_true",
             help="revert instead of checkout",
         )

subcmds/forall.py

@@ -133,7 +133,7 @@ without iterating through the remaining projects.
 
     @staticmethod
     def _cmd_option(option, _opt_str, _value, parser):
-        setattr(parser.values, option.dest, list(parser.rargs))
+        setattr(parser.values, option.dest or "command", list(parser.rargs))
         while parser.rargs:
             del parser.rargs[0]
 
@@ -141,7 +141,6 @@ without iterating through the remaining projects.
         p.add_option(
             "-r",
             "--regex",
-            dest="regex",
             action="store_true",
             help="execute the command only on projects matching regex or "
             "wildcard expression",
@@ -149,7 +148,6 @@ without iterating through the remaining projects.
         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",
@@ -157,7 +155,6 @@ without iterating through the remaining projects.
         p.add_option(
             "-g",
             "--groups",
-            dest="groups",
             help="execute the command only on projects matching the specified "
             "groups",
         )
@@ -165,14 +162,12 @@ without iterating through the remaining projects.
             "-c",
             "--command",
             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",
         )

subcmds/grep.py

@@ -120,7 +120,6 @@ contain a line that matches both expressions:
         g.add_option(
             "-r",
             "--revision",
-            dest="revision",
             action="append",
             metavar="TREEish",
             help="Search TREEish, instead of the work tree",

subcmds/info.py

@@ -43,14 +43,12 @@ class Info(PagedCommand):
         p.add_option(
             "-o",
             "--overview",
-            dest="overview",
             action="store_true",
             help="show overview of all local commits",
         )
         p.add_option(
             "-c",
             "--current-branch",
-            dest="current_branch",
             action="store_true",
             help="consider only checked out branches",
         )

subcmds/list.py

@@ -40,7 +40,6 @@ This is similar to running: repo forall -c 'echo "$REPO_PATH : $REPO_PROJECT"'.
         p.add_option(
             "-r",
             "--regex",
-            dest="regex",
             action="store_true",
             help="filter the project list based on regex or wildcard matching "
             "of strings",
@@ -48,7 +47,6 @@ This is similar to running: repo forall -c 'echo "$REPO_PATH : $REPO_PROJECT"'.
         p.add_option(
             "-g",
             "--groups",
-            dest="groups",
             help="filter the project list based on the groups the project is "
             "in",
         )
@@ -61,21 +59,18 @@ This is similar to running: repo forall -c 'echo "$REPO_PATH : $REPO_PROJECT"'.
         p.add_option(
             "-n",
             "--name-only",
-            dest="name_only",
             action="store_true",
             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",
         )
         p.add_option(
             "-f",
             "--fullpath",
-            dest="fullpath",
             action="store_true",
             help="display the full work tree path instead of the relative path",
         )

subcmds/manifest.py

@@ -134,7 +134,6 @@ human-readable variations.
         p.add_option(
             "-o",
             "--output-file",
-            dest="output_file",
             default="-",
             help="file to save the manifest to. (Filename prefix for "
             "multi-tree.)",

subcmds/overview.py

@@ -37,7 +37,6 @@ are displayed.
         p.add_option(
             "-c",
             "--current-branch",
-            dest="current_branch",
             action="store_true",
             help="consider only checked out branches",
         )

subcmds/rebase.py

@@ -47,21 +47,18 @@ branch but need to incorporate new upstream changes "underneath" them.
         g.add_option(
             "-i",
             "--interactive",
-            dest="interactive",
             action="store_true",
             help="interactive rebase (single project only)",
         )
 
         p.add_option(
             "--fail-fast",
-            dest="fail_fast",
             action="store_true",
             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",
         )
@@ -74,27 +71,23 @@ branch but need to incorporate new upstream changes "underneath" them.
         )
         p.add_option(
             "--autosquash",
-            dest="autosquash",
             action="store_true",
             help="pass --autosquash to git rebase",
         )
         p.add_option(
             "--whitespace",
-            dest="whitespace",
             action="store",
             metavar="WS",
             help="pass --whitespace to git rebase",
         )
         p.add_option(
             "--auto-stash",
-            dest="auto_stash",
             action="store_true",
             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 "

subcmds/selfupdate.py

@@ -54,7 +54,6 @@ need to be performed by an end-user.
         )
         g.add_option(
             "--repo-upgraded",
-            dest="repo_upgraded",
             action="store_true",
             help=optparse.SUPPRESS_HELP,
         )

subcmds/stage.py

@@ -46,7 +46,6 @@ The '%prog' command stages files to prepare the next commit.
         g.add_option(
             "-i",
             "--interactive",
-            dest="interactive",
             action="store_true",
             help="use interactive staging",
         )

subcmds/start.py

@@ -51,7 +51,6 @@ revision specified in the manifest.
     def _Options(self, p):
         p.add_option(
             "--all",
-            dest="all",
             action="store_true",
             help="begin branch in all projects",
         )

subcmds/status.py

@@ -82,7 +82,6 @@ the following meanings:
         p.add_option(
             "-o",
             "--orphans",
-            dest="orphans",
             action="store_true",
             help="include objects in working directory outside of repo "
             "projects",

subcmds/sync.py

@@ -373,19 +373,16 @@ later is required to fix a server side protocol bug.
         p.add_option(
             "-f",
             "--force-broken",
-            dest="force_broken",
             action="store_true",
             help="obsolete option (to be deleted in the future)",
         )
         p.add_option(
             "--fail-fast",
-            dest="fail_fast",
             action="store_true",
             help="stop syncing after first error is hit",
         )
         p.add_option(
             "--force-sync",
-            dest="force_sync",
             action="store_true",
             help="overwrite an existing git directory if it needs to "
             "point to a different object directory. WARNING: this "
@@ -393,7 +390,6 @@ later is required to fix a server side protocol bug.
         )
         p.add_option(
             "--force-checkout",
-            dest="force_checkout",
             action="store_true",
             help="force checkout even if it results in throwing away "
             "uncommitted modifications. "
@@ -401,7 +397,6 @@ later is required to fix a server side protocol bug.
         )
         p.add_option(
             "--force-remove-dirty",
-            dest="force_remove_dirty",
             action="store_true",
             help="force remove projects with uncommitted modifications if "
             "projects no longer exist in the manifest. "
@@ -409,7 +404,6 @@ later is required to fix a server side protocol bug.
         )
         p.add_option(
             "--rebase",
-            dest="rebase",
             action="store_true",
             help="rebase local commits regardless of whether they are "
             "published",
@@ -417,7 +411,6 @@ later is required to fix a server side protocol bug.
         p.add_option(
             "-l",
             "--local-only",
-            dest="local_only",
             action="store_true",
             help="only update working tree, don't fetch",
         )
@@ -433,7 +426,6 @@ later is required to fix a server side protocol bug.
         p.add_option(
             "-n",
             "--network-only",
-            dest="network_only",
             action="store_true",
             help="fetch only, don't update working tree",
         )
@@ -460,7 +452,6 @@ later is required to fix a server side protocol bug.
         p.add_option(
             "-m",
             "--manifest-name",
-            dest="manifest_name",
             help="temporary manifest to use for this sync",
             metavar="NAME.xml",
         )
@@ -479,19 +470,16 @@ later is required to fix a server side protocol bug.
             "-u",
             "--manifest-server-username",
             action="store",
-            dest="manifest_server_username",
             help="username to authenticate with the manifest server",
         )
         p.add_option(
             "-p",
             "--manifest-server-password",
             action="store",
-            dest="manifest_server_password",
             help="password to authenticate with the manifest server",
         )
         p.add_option(
             "--fetch-submodules",
-            dest="fetch_submodules",
             action="store_true",
             help="fetch submodules from server",
         )
@@ -515,7 +503,6 @@ later is required to fix a server side protocol bug.
         )
         p.add_option(
             "--optimized-fetch",
-            dest="optimized_fetch",
             action="store_true",
             help="only fetch projects fixed to sha1 if revision does not exist "
             "locally",
@@ -554,7 +541,6 @@ later is required to fix a server side protocol bug.
             p.add_option(
                 "-s",
                 "--smart-sync",
-                dest="smart_sync",
                 action="store_true",
                 help="smart sync using manifest from the latest known good "
                 "build",
@@ -562,7 +548,6 @@ later is required to fix a server side protocol bug.
             p.add_option(
                 "-t",
                 "--smart-tag",
-                dest="smart_tag",
                 action="store",
                 help="smart sync using manifest from a known tag",
             )
@@ -577,7 +562,6 @@ later is required to fix a server side protocol bug.
         )
         g.add_option(
             "--repo-upgraded",
-            dest="repo_upgraded",
             action="store_true",
             help=optparse.SUPPRESS_HELP,
         )

subcmds/upload.py

@@ -267,7 +267,6 @@ Gerrit Code Review:  https://www.gerritcodereview.com/
             "--cc",
             type="string",
             action="append",
-            dest="cc",
             help="also send email to these email addresses",
         )
         p.add_option(
@@ -281,7 +280,6 @@ Gerrit Code Review:  https://www.gerritcodereview.com/
         p.add_option(
             "-c",
             "--current-branch",
-            dest="current_branch",
             action="store_true",
             help="upload current git branch",
         )
@@ -310,7 +308,6 @@ Gerrit Code Review:  https://www.gerritcodereview.com/
             "-p",
             "--private",
             action="store_true",
-            dest="private",
             default=False,
             help="upload as a private change (deprecated; use --wip)",
         )
@@ -318,7 +315,6 @@ Gerrit Code Review:  https://www.gerritcodereview.com/
             "-w",
             "--wip",
             action="store_true",
-            dest="wip",
             default=False,
             help="upload as a work-in-progress change",
         )

tests/test_subcmds.py

@@ -89,3 +89,44 @@ class AllCommands(unittest.TestCase):
                         msg=f"subcmds/{name}.py: {opt}: only use dashes in "
                         "options, not underscores",
                     )
+
+    def test_cli_option_dest(self):
+        """Block redundant dest= arguments."""
+
+        def _check_dest(opt):
+            if opt.dest is None or not opt._long_opts:
+                return
+
+            long = opt._long_opts[0]
+            assert long.startswith("--")
+            # This matches optparse's behavior.
+            implicit_dest = long[2:].replace("-", "_")
+            if implicit_dest == opt.dest:
+                bad_opts.append((str(opt), opt.dest))
+
+        # Hook the option check list.
+        optparse.Option.CHECK_METHODS.insert(0, _check_dest)
+
+        # Gather all the bad options up front so people can see all bad options
+        # instead of failing at the first one.
+        all_bad_opts = {}
+        for name, cls in subcmds.all_commands.items():
+            bad_opts = all_bad_opts[name] = []
+            cmd = cls()
+            # Trigger construction of parser.
+            cmd.OptionParser
+
+        errmsg = None
+        for name, bad_opts in sorted(all_bad_opts.items()):
+            if bad_opts:
+                if not errmsg:
+                    errmsg = "Omit redundant dest= when defining options.\n"
+                errmsg += f"\nSubcommand {name} (subcmds/{name}.py):\n"
+                errmsg += "".join(
+                    f"    {opt}: dest='{dest}'\n" for opt, dest in bad_opts
+                )
+        if errmsg:
+            self.fail(errmsg)
+
+        # Make sure we aren't popping the wrong stuff.
+        assert optparse.Option.CHECK_METHODS.pop(0) is _check_dest