mirror of
https://gerrit.googlesource.com/git-repo
synced 2026-10-15 01:47:08 +00:00
hooks: add --fix option to auto-apply hook fixes
Pass the --fix flag as a keyword argument "fix" to the hook main function. This allows hooks (such as git-repohooks) to decouple automated fix application from the -y/--yes flag so that -y can answer yes to upload confirmation prompts without triggering file mutations. Companion change in git-repohooks: https://gerrit-review.googlesource.com/c/git-repohooks/+/621761 Bug: 546510319 Test: python3 -m pytest tests/test_hooks.py Change-Id: If0288d4791fc0a2aba6e88854e3aa81b0923b664 Reviewed-on: https://gerrit-review.googlesource.com/c/git-repo/+/619281 Commit-Queue: Rahul Yadav <yadavrah@google.com> Tested-by: Rahul Yadav <yadavrah@google.com> Reviewed-by: Gavin Mak <gavinmak@google.com>
This commit is contained in:
1 parent
09914bcab7
commit
e6ad708009
6 files changed
+98
-7
No files matched your search
@@ -400,6 +400,7 @@ _repo() {
|
|||||||
'--no-verify[Do not verify]' \
|
'--no-verify[Do not verify]' \
|
||||||
'--verify[Verify]' \
|
'--verify[Verify]' \
|
||||||
'--ignore-hooks[Ignore hooks]' \
|
'--ignore-hooks[Ignore hooks]' \
|
||||||
|
'--fix[Automatically fix]' \
|
||||||
'*: :->project'
|
'*: :->project'
|
||||||
;;
|
;;
|
||||||
version)
|
version)
|
||||||
|
|||||||
+10
-2
@@ -88,7 +88,14 @@ be useful when deploying automatic fixes.
|
|||||||
If the repo command that triggered the hook supports a "yes" option (e.g.,
|
If the repo command that triggered the hook supports a "yes" option (e.g.,
|
||||||
`repo upload --yes`), this option is propagated to the hook's `main` function
|
`repo upload --yes`), this option is propagated to the hook's `main` function
|
||||||
as `yes` parameter (defaulting to `False`). Hooks can use this to bypass
|
as `yes` parameter (defaulting to `False`). Hooks can use this to bypass
|
||||||
interactive confirmation prompts when they can automatically fix issues.
|
interactive confirmation prompts for safe non-modifying operations.
|
||||||
|
|
||||||
|
### Automated Fixes
|
||||||
|
|
||||||
|
If the repo command that triggered the hook supports a "fix" option (e.g.,
|
||||||
|
`repo upload --fix`), this option is propagated to the hook's `main` function
|
||||||
|
as `fix` parameter (defaulting to `False`). Hooks can use this to automatically
|
||||||
|
apply fixes without prompting the user.
|
||||||
|
|
||||||
### Shebang Handling
|
### Shebang Handling
|
||||||
|
|
||||||
@@ -126,7 +133,7 @@ This hook runs when people run `repo upload`.
|
|||||||
The `pre-upload.py` file should be defined like:
|
The `pre-upload.py` file should be defined like:
|
||||||
|
|
||||||
```py
|
```py
|
||||||
def main(project_list, worktree_list=None, yes=False, **kwargs):
|
def main(project_list, worktree_list=None, fix=False, yes=False, **kwargs):
|
||||||
"""Main function invoked directly by repo.
|
"""Main function invoked directly by repo.
|
||||||
|
|
||||||
We must use the name "main" as that is what repo requires.
|
We must use the name "main" as that is what repo requires.
|
||||||
@@ -137,6 +144,7 @@ def main(project_list, worktree_list=None, yes=False, **kwargs):
|
|||||||
project_list, so that each entry in project_list matches with a
|
project_list, so that each entry in project_list matches with a
|
||||||
directory in worktree_list. If None, we will attempt to calculate
|
directory in worktree_list. If None, we will attempt to calculate
|
||||||
the directories automatically.
|
the directories automatically.
|
||||||
|
fix: Whether to automatically apply fixes without prompting.
|
||||||
yes: Whether to answer yes to all safe prompts (see
|
yes: Whether to answer yes to all safe prompts (see
|
||||||
[Safe Prompts](#safe-prompts)).
|
[Safe Prompts](#safe-prompts)).
|
||||||
kwargs: Leave this here for forward-compatibility.
|
kwargs: Leave this here for forward-compatibility.
|
||||||
|
|||||||
@@ -12,6 +12,7 @@
|
|||||||
# See the License for the specific language governing permissions and
|
# See the License for the specific language governing permissions and
|
||||||
# limitations under the License.
|
# limitations under the License.
|
||||||
|
|
||||||
|
import optparse
|
||||||
import os
|
import os
|
||||||
import re
|
import re
|
||||||
import sys
|
import sys
|
||||||
@@ -69,6 +70,7 @@ class RepoHook:
|
|||||||
ignore_hooks=False,
|
ignore_hooks=False,
|
||||||
abort_if_user_denies=False,
|
abort_if_user_denies=False,
|
||||||
yes=False,
|
yes=False,
|
||||||
|
fix=False,
|
||||||
):
|
):
|
||||||
"""RepoHook constructor.
|
"""RepoHook constructor.
|
||||||
|
|
||||||
@@ -91,6 +93,7 @@ class RepoHook:
|
|||||||
abort_if_user_denies: If True, we'll abort running the hook if the
|
abort_if_user_denies: If True, we'll abort running the hook if the
|
||||||
user doesn't allow us to run the hook.
|
user doesn't allow us to run the hook.
|
||||||
yes: If True, then 'Yes' is assumed for any prompts.
|
yes: If True, then 'Yes' is assumed for any prompts.
|
||||||
|
fix: If True, then 'Fix' is assumed for any fixup prompts.
|
||||||
"""
|
"""
|
||||||
self._hook_type = hook_type
|
self._hook_type = hook_type
|
||||||
self._hooks_project = hooks_project
|
self._hooks_project = hooks_project
|
||||||
@@ -102,6 +105,7 @@ class RepoHook:
|
|||||||
self._ignore_hooks = ignore_hooks
|
self._ignore_hooks = ignore_hooks
|
||||||
self._abort_if_user_denies = abort_if_user_denies
|
self._abort_if_user_denies = abort_if_user_denies
|
||||||
self._yes = yes
|
self._yes = yes
|
||||||
|
self._fix = fix
|
||||||
|
|
||||||
# Store the full path to the script for convenience.
|
# Store the full path to the script for convenience.
|
||||||
self._script_fullpath = None
|
self._script_fullpath = None
|
||||||
@@ -380,6 +384,7 @@ class RepoHook:
|
|||||||
kwargs = {
|
kwargs = {
|
||||||
**kwargs,
|
**kwargs,
|
||||||
"hook_should_take_kwargs": True,
|
"hook_should_take_kwargs": True,
|
||||||
|
"fix": self._fix,
|
||||||
"yes": self._yes,
|
"yes": self._yes,
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -504,12 +509,17 @@ class RepoHook:
|
|||||||
).url,
|
).url,
|
||||||
"bug_url": manifest.contactinfo.bugurl,
|
"bug_url": manifest.contactinfo.bugurl,
|
||||||
"yes": getattr(opt, "yes", False),
|
"yes": getattr(opt, "yes", False),
|
||||||
|
"fix": getattr(opt, "fix", False),
|
||||||
}
|
}
|
||||||
)
|
)
|
||||||
return cls(*args, **kwargs)
|
return cls(*args, **kwargs)
|
||||||
|
|
||||||
@staticmethod
|
@staticmethod
|
||||||
def AddOptionGroup(parser, name):
|
def AddOptionGroup(
|
||||||
|
parser: optparse.OptionParser,
|
||||||
|
name: str,
|
||||||
|
allow_fix: bool = False,
|
||||||
|
) -> None:
|
||||||
"""Help options relating to the various hooks."""
|
"""Help options relating to the various hooks."""
|
||||||
|
|
||||||
# Note that verify and no-verify are NOT opposites of each other, which
|
# Note that verify and no-verify are NOT opposites of each other, which
|
||||||
@@ -533,3 +543,10 @@ class RepoHook:
|
|||||||
action="store_true",
|
action="store_true",
|
||||||
help="Do not abort if %s hooks fail." % name,
|
help="Do not abort if %s hooks fail." % name,
|
||||||
)
|
)
|
||||||
|
if allow_fix:
|
||||||
|
group.add_option(
|
||||||
|
"--fix",
|
||||||
|
action="store_true",
|
||||||
|
default=False,
|
||||||
|
help="Automatically apply %s fixes without prompting." % name,
|
||||||
|
)
|
||||||
+4
-1
@@ -1,5 +1,5 @@
|
|||||||
.\" DO NOT MODIFY THIS FILE! It was generated by help2man.
|
.\" DO NOT MODIFY THIS FILE! It was generated by help2man.
|
||||||
.TH REPO "1" "June 2026" "repo upload" "Repo Manual"
|
.TH REPO "1" "August 2026" "repo upload" "Repo Manual"
|
||||||
.SH NAME
|
.SH NAME
|
||||||
repo \- repo upload - manual page for repo upload
|
repo \- repo upload - manual page for repo upload
|
||||||
.SH SYNOPSIS
|
.SH SYNOPSIS
|
||||||
@@ -112,6 +112,9 @@ Run the pre\-upload hook without prompting.
|
|||||||
.TP
|
.TP
|
||||||
\fB\-\-ignore\-hooks\fR
|
\fB\-\-ignore\-hooks\fR
|
||||||
Do not abort if pre\-upload hooks fail.
|
Do not abort if pre\-upload hooks fail.
|
||||||
|
.TP
|
||||||
|
\fB\-\-fix\fR
|
||||||
|
Automatically apply pre\-upload fixes without prompting.
|
||||||
.PP
|
.PP
|
||||||
Run `repo help upload` to view the detailed manual.
|
Run `repo help upload` to view the detailed manual.
|
||||||
.SH DETAILS
|
.SH DETAILS
|
||||||
|
|||||||
+1
-1
@@ -379,7 +379,7 @@ Gerrit Code Review: https://www.gerritcodereview.com/
|
|||||||
default=True,
|
default=True,
|
||||||
help="disable verifying ssl certs (unsafe)",
|
help="disable verifying ssl certs (unsafe)",
|
||||||
)
|
)
|
||||||
RepoHook.AddOptionGroup(p, "pre-upload")
|
RepoHook.AddOptionGroup(p, "pre-upload", allow_fix=True)
|
||||||
|
|
||||||
def _SingleBranch(self, opt, branch, people):
|
def _SingleBranch(self, opt, branch, people):
|
||||||
project = branch.project
|
project = branch.project
|
||||||
|
|||||||
+64
-2
@@ -15,6 +15,7 @@
|
|||||||
"""Unittests for the hooks.py module."""
|
"""Unittests for the hooks.py module."""
|
||||||
|
|
||||||
from io import StringIO
|
from io import StringIO
|
||||||
|
from pathlib import Path
|
||||||
import sys
|
import sys
|
||||||
|
|
||||||
import pytest
|
import pytest
|
||||||
@@ -108,11 +109,11 @@ def test_post_sync_argument_validation() -> None:
|
|||||||
|
|
||||||
|
|
||||||
@pytest.mark.parametrize("yes_val", (True, False))
|
@pytest.mark.parametrize("yes_val", (True, False))
|
||||||
def test_repo_upload_yes_arg(tmp_path, yes_val: bool) -> None:
|
def test_repo_upload_yes_arg(tmp_path: Path, yes_val: bool) -> None:
|
||||||
"""Test that yes is passed in kwargs during hook execution."""
|
"""Test that yes is passed in kwargs during hook execution."""
|
||||||
|
|
||||||
class FakeProject:
|
class FakeProject:
|
||||||
def __init__(self, worktree):
|
def __init__(self, worktree: str) -> None:
|
||||||
self.worktree = worktree
|
self.worktree = worktree
|
||||||
self.enabled_repo_hooks = ["pre-upload"]
|
self.enabled_repo_hooks = ["pre-upload"]
|
||||||
self.config = None
|
self.config = None
|
||||||
@@ -139,3 +140,64 @@ def main(project_list, **kwargs):
|
|||||||
|
|
||||||
assert res is True
|
assert res is True
|
||||||
assert project_list == [yes_val]
|
assert project_list == [yes_val]
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.parametrize("fix_val", (True, False))
|
||||||
|
def test_repo_upload_fix_arg(tmp_path: Path, fix_val: bool) -> None:
|
||||||
|
"""Test that fix is passed in kwargs during hook execution."""
|
||||||
|
|
||||||
|
class FakeProject:
|
||||||
|
def __init__(self, worktree: str) -> None:
|
||||||
|
self.worktree = worktree
|
||||||
|
self.enabled_repo_hooks = ["pre-upload"]
|
||||||
|
self.config = None
|
||||||
|
|
||||||
|
hook_file = tmp_path / "pre-upload.py"
|
||||||
|
|
||||||
|
hook_content = """
|
||||||
|
def main(project_list, **kwargs):
|
||||||
|
project_list.append(kwargs.get("fix"))
|
||||||
|
"""
|
||||||
|
hook_file.write_text(hook_content)
|
||||||
|
|
||||||
|
hook = hooks.RepoHook(
|
||||||
|
hook_type="pre-upload",
|
||||||
|
hooks_project=FakeProject(str(tmp_path)),
|
||||||
|
repo_topdir=str(tmp_path),
|
||||||
|
manifest_url="https://gerrit",
|
||||||
|
allow_all_hooks=True,
|
||||||
|
fix=fix_val,
|
||||||
|
)
|
||||||
|
|
||||||
|
project_list = []
|
||||||
|
res = hook.Run(project_list=project_list, worktree_list=[])
|
||||||
|
|
||||||
|
assert res is True
|
||||||
|
assert project_list == [fix_val]
|
||||||
|
|
||||||
|
|
||||||
|
def test_from_subcmd_without_fix_option() -> None:
|
||||||
|
"""Test that FromSubcmd works when opt does not have fix attribute."""
|
||||||
|
|
||||||
|
class Remote:
|
||||||
|
url = "https://gerrit"
|
||||||
|
|
||||||
|
class FakeManifest:
|
||||||
|
repo_hooks_project = None
|
||||||
|
topdir = "/fake/topdir"
|
||||||
|
|
||||||
|
class manifestProject:
|
||||||
|
@staticmethod
|
||||||
|
def GetRemote(name: str) -> "Remote":
|
||||||
|
return Remote()
|
||||||
|
|
||||||
|
class contactinfo:
|
||||||
|
bugurl = "https://bugs"
|
||||||
|
|
||||||
|
class FakeOpt:
|
||||||
|
bypass_hooks = False
|
||||||
|
allow_all_hooks = False
|
||||||
|
ignore_hooks = False
|
||||||
|
|
||||||
|
hook = hooks.RepoHook.FromSubcmd(FakeManifest(), FakeOpt(), "post-sync")
|
||||||
|
assert hook._fix is False
|
||||||
Reference in new issue
Block a user