upload: reject --fix with --ignore-hooks or --no-verify

--fix asks the pre-upload hook to apply its fixups, but:
* --no-verify skips the hook entirely, so --fix silently does nothing.
* --ignore-hooks uploads the original commits even if the hook fails,
  so any fixups applied to the working tree are not part of the
  upload. Whether --fix should then apply fixups or be ignored is
  ambiguous.

Reject both combinations up front, before any hook runs, instead of
picking an interpretation.

Bug: 562921767
Test: python3 -m pytest tests/test_subcmds_upload.py
Change-Id: Ib00ae69004642f2e763212b82e7c39f3b4da52eb
Reviewed-on: https://gerrit-review.googlesource.com/c/git-repo/+/642664
Tested-by: Rahul Yadav <yadavrah@google.com>
Reviewed-by: Mike Frysinger <vapier@google.com>
Commit-Queue: Rahul Yadav <yadavrah@google.com>

subcmds/upload.py

@@ -742,6 +742,22 @@ Gerrit Code Review:  https://www.gerritcodereview.com/
             avail = project.GetUploadableBranches(opt.branch)
         return (project_idx, avail, cbr)
 
+    def ValidateOptions(self, opt: optparse.Values, args: List[str]) -> None:
+        if opt.fix:
+            if opt.ignore_hooks:
+                self.OptionParser.error(
+                    "--fix and --ignore-hooks are mutually exclusive options: "
+                    "with --ignore-hooks, the original commits are uploaded, "
+                    "so fixups would only modify the working tree and would "
+                    "not be part of the upload"
+                )
+            if opt.bypass_hooks:
+                self.OptionParser.error(
+                    "--fix and --no-verify are mutually exclusive options: "
+                    "--no-verify skips the pre-upload hook, so --fix would "
+                    "have no effect"
+                )
+
     def Execute(self, opt, args):
         projects = self.GetProjects(
             args, all_manifests=not opt.this_manifest_only

tests/test_subcmds_upload.py

@@ -233,3 +233,45 @@ def test_MultipleBranches_yes_without_branch_or_cbr_uses_editor(
         cmd._MultipleBranches(opt, pending, _STUB_PEOPLE)
         mock_edit.assert_called_once()
         mock_upload.assert_called_once_with(opt, [branch1], _STUB_PEOPLE)
+
+
+@pytest.mark.parametrize(
+    "argv, expected",
+    (
+        (["--fix", "--ignore-hooks"], "not be part of the upload"),
+        (["--fix", "--no-verify"], "--fix would have no effect"),
+        (
+            ["--fix", "--ignore-hooks", "--no-verify"],
+            "--fix and --ignore-hooks are mutually exclusive",
+        ),
+    ),
+)
+def test_ValidateOptions_fix_conflicts(
+    cmd: upload.Upload,
+    capsys: pytest.CaptureFixture,
+    argv: List[str],
+    expected: str,
+) -> None:
+    """Check --fix is rejected with --ignore-hooks or --no-verify."""
+    opt, args = cmd.OptionParser.parse_args(argv)
+    with pytest.raises(SystemExit):
+        cmd.ValidateOptions(opt, args)
+    assert expected in capsys.readouterr().err
+
+
+@pytest.mark.parametrize(
+    "argv",
+    (
+        [],
+        ["--fix"],
+        ["--fix", "--verify"],
+        ["--fix", "-y"],
+        ["--ignore-hooks"],
+        ["--no-verify"],
+        ["--ignore-hooks", "--no-verify"],
+    ),
+)
+def test_ValidateOptions_allowed(cmd: upload.Upload, argv: List[str]) -> None:
+    """Check valid hook option combinations are accepted."""
+    opt, args = cmd.OptionParser.parse_args(argv)
+    cmd.ValidateOptions(opt, args)