Skip to content

feat(autosubmit): Check diff for exactly one added block of lines #745

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
perlpunk wants to merge 1 commit into
base: master
Choose a base branch
from
+163 −1
Open

feat(autosubmit): Check diff for exactly one added block of lines #745

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 41 additions & 1 deletion os-autoinst-obs-auto-submit
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -267,6 +267,28 @@ def empty_git_dir(path: pathlib.Path) -> None:
item.unlink()


def diff_has_single_added_block(diff_cmd: list[str]) -> tuple[bool, str]:
"""Return True if the diff contains exactly one contiguous block of added lines."""
in_added_block = False
block_count = 0

res = _run_subprocess(diff_cmd)
for line in res.stdout.splitlines():
# Ignore diff header lines (e.g., '+++ os-autoinst.changes...')
if line.startswith(("+++", "---")):
continue
if line.startswith("-"):
return (False, None)
if line.startswith("+"):
if not in_added_block:
in_added_block = True
block_count += 1
else:
in_added_block = False

Comment on lines +277 to +288

@okurz okurz Oct 6, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should be equivalent

Suggested change
# Ignore diff header lines (e.g., '+++ os-autoinst.changes...')
if line.startswith("+++") or line.startswith("---"):
continue
if line.startswith("-"):
return (False, None)
if line.startswith("+"):
if not in_added_block:
in_added_block = True
block_count += 1
else:
in_added_block = False
lines = [l for l in res.stdout.splitlines() if not l.startswith(("+++", "---"))]
if any(l.startswith("-") for l in lines):
return (False, res.stdout)
blocks = sum(1 for is_add, _ in groupby(lines, key=lambda l: l.startswith("+")) if is_add)

@perlpunk perlpunk Oct 9, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find some of the python stuff very verbose, but in this case I find my version easier to understand

@okurz okurz Oct 10, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fine, you can come up with a different way but needing to read a for loop and how a counter variable is tracked just to understand that what you want is a sum can be improved.

Drop the header lines by skipping everything before the first @@. Then use groupby on the first character of each line, so there are no counters and no header filtering:

from itertools import dropwhile, groupby

def diff_has_single_added_block(diff_cmd: list[str]) -> tuple[bool, subprocess.CompletedProcess[str]]:
    """Return True if the diff only adds one contiguous block of lines."""
    res = _run_subprocess(diff_cmd)
    hunk_lines = dropwhile(lambda line: not line.startswith("@@"), res.stdout.splitlines())
    runs = [kind for kind, _ in groupby(line[:1] for line in hunk_lines)]
    return ("-" not in runs and runs.count("+") == 1, res)

Returning the CompletedProcess keeps the callers' res.stdout working and removes the None path

Also uses dropwhile which I find quite descriptive about what it does. Sure, the first time you see it, you might be surprised but the intention should be very clear just from the function name. Also here reading a for loop and understanding the effect of "continue" might use simple control statements but one still needs to read the whole flow to understand what is going on. dropwhile and groupby in their lines when used make it clear how those lines are processed in the consecutive statements.

return (block_count == 1, res.stdout)


@dataclass
class AutoSubmitter:
"""Automate the preparation and submission of packages."""
Expand Down Expand Up @@ -414,18 +436,27 @@ class AutoSubmitter:
# fork may lack the branch (switch-first fails "invalid reference").
fetch_cmd = [*git_cmd, "fetch", "parent"]
switch_cmd = [*git_cmd, "switch", "-C", branch, f"parent/{branch}"]
diff_cmd = [*git_cmd, "diff", f"{package}.changes"]

if self.dry_run:
log.info("[dry-run] Would execute: %s", " ".join(fetch_cmd))
log.info("[dry-run] Would execute: %s", " ".join(switch_cmd))
self._fetch_lfs_objects(branch)
log.info("[dry-run] Would replace files in clone with files from ../../%s/*", package)
log.info("[dry-run] Would execute: %s", " ".join(diff_cmd))
else:
_run_cmd(fetch_cmd, check=True)
_run_cmd(switch_cmd, check=True)
self._fetch_lfs_objects(branch)
self._copy_files_to_clone(package)

(diff_ok, res) = diff_has_single_added_block(diff_cmd)
if not diff_ok:
log.warning("git diff for %s.changes does not look ok:\n%s", package, res.stdout)
return False

return True

def _fetch_lfs_objects(self, branch: str) -> None:
"""Fetch necessary Git LFS objects from parent before pushing."""
git_cmd = shlex.split(self.git_cmd_str)
Expand Down Expand Up @@ -566,7 +597,8 @@ class AutoSubmitter:
if not self.dry_run:
os.chdir(package)

self._prepare_local_clone(package, branch)
if not self._prepare_local_clone(package, branch):
return False
self._commit_and_push(title, branch)
return self._create_pull_request(title, description, branch)

Expand Down Expand Up @@ -880,6 +912,13 @@ class AutoSubmitter:

if not self.dry_run:
os.chdir(dst_dir)

diff_cmd = [*shlex.split(self.osc_cmd_str), "diff", f"{package}.changes"]
(diff_ok, res) = diff_has_single_added_block(diff_cmd)
if not diff_ok:
log.warning("osc diff for %s.changes does not look ok:\n%s", package, res.stdout)
return False

return self.update_package(package=package)


Expand Down Expand Up @@ -1043,6 +1082,7 @@ def _run_submissions( # ruff: ignore[complex-structure, too-many-branches, too-
os.chdir(tmpdir_path)
success = submitter.handle_auto_submit(package=package)
if not success:
submitter.failed_packages.append(package)
rc = 1

if must_cleanup_staging:
Expand Down
122 changes: 122 additions & 0 deletions tests/test_auto_submit.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,7 @@ def test_prepare_local_clone_fetches_before_switch(mocker: MockerFixture) -> Non
"""Fetch parent before creating the branch so a fork lacking it still works."""
mock_run = mocker.patch("auto_submit.subprocess.run")
mocker.patch("auto_submit.pathlib.Path.iterdir", return_value=[])
mocker.patch("auto_submit.diff_has_single_added_block", return_value=(True, None))
submitter = auto_submit.AutoSubmitter(dst_project="dst", git_cmd_str="git", dir=pathlib.Path(), dry_run=False)
submitter._prepare_local_clone("openQA", "leap-16.0")
git_calls = [call.args[0] for call in mock_run.call_args_list]
Expand Down Expand Up @@ -170,6 +171,21 @@ def test_prepare_local_clone_calls_fetch_lfs(mocker: MockerFixture) -> None:
mock_copy.assert_called_once_with("openQA")


def test_prepare_local_clone_diff_not_ok(mocker: MockerFixture, caplog: pytest.LogCaptureFixture) -> None:
caplog.set_level(logging.INFO)
mock_run = mocker.patch("auto_submit.subprocess.run")
mock_fetch_lfs = mocker.patch("auto_submit.AutoSubmitter._fetch_lfs_objects")
mock_copy = mocker.patch("auto_submit.AutoSubmitter._copy_files_to_clone")
submitter = auto_submit.AutoSubmitter(dst_project="dst", git_cmd_str="git", dir=pathlib.Path(), dry_run=False)
mocker.patch(
"auto_submit.diff_has_single_added_block",
return_value=(False, subprocess.CompletedProcess("", 0, stdout="diff output", stderr="")),
)
res = submitter._prepare_local_clone("pkg", "leap-16.0")
assert res is False
assert caplog.records[2].getMessage() == "git diff for pkg.changes does not look ok:\ndiff output"


def test_fetch_lfs_objects_with_merge_base(mocker: MockerFixture) -> None:
def mocked_run(cmd: list[str], **_kwargs: Any) -> subprocess.CompletedProcess[str]:
if cmd[1] == "merge-base":
Expand Down Expand Up @@ -614,6 +630,7 @@ def test_update_package(
mocker.patch("auto_submit.AutoSubmitter._find_version", return_value="23")
mocker.patch("auto_submit.AutoSubmitter._osc_addremove_and_filter_specs", return_value="23")
mocker.patch("auto_submit.AutoSubmitter._commit_local_changes", return_value=auto_submit.CommitResult.SUCCESS)
mocker.patch("auto_submit.diff_has_single_added_block", return_value=(True, None))
mocker.patch("auto_submit.AutoSubmitter._commit_and_push", return_value=True)
mocker.patch("auto_submit.AutoSubmitter._create_pull_request", return_value=True)
mocker.patch("auto_submit.AutoSubmitter.has_pending_submission", return_value=False)
Expand Down Expand Up @@ -647,6 +664,62 @@ def mocked_clone(package: str) -> str:
assert (tmp_path / "git-repos" / "pkg" / changes_file).exists()


def test_handle_auto_submit_copy_files(
mocker: MockerFixture,
tmp_path: pathlib.Path,
monkeypatch: pytest.MonkeyPatch,
) -> None:
mocker.patch("auto_submit.AutoSubmitter._osc_co", return_value="23")
mocker.patch("auto_submit.AutoSubmitter._is_package_up_to_date", return_value=False)
mocker.patch("auto_submit.AutoSubmitter.update_package", return_value=True)
mocker.patch("auto_submit.diff_has_single_added_block", return_value=(True, None))

monkeypatch.chdir(tmp_path)
dstdir = tmp_path / "dst" / "pkg"
dstdir.mkdir(parents=True, exist_ok=True)
srcdir = tmp_path / "src" / "pkg"
srcdir.mkdir(parents=True, exist_ok=True)
(srcdir / "newfile").write_text("new")
(dstdir / "oldfile").write_text("old")
submitter = auto_submit.AutoSubmitter(
dst_project="dst",
src_project="src",
dry_run=False,
osc_cmd_str="osc",
)
res = submitter.handle_auto_submit("pkg")
assert res is True
assert (dstdir / "newfile").exists()
assert not (dstdir / "oldfile").exists()


def test_handle_auto_submit_diff_not_ok(
caplog: pytest.LogCaptureFixture,
mocker: MockerFixture,
tmp_path: pathlib.Path,
monkeypatch: pytest.MonkeyPatch,
) -> None:
mocker.patch("auto_submit.AutoSubmitter._osc_co", return_value="23")
mocker.patch("auto_submit.AutoSubmitter._is_package_up_to_date", return_value=False)
mocker.patch("auto_submit.AutoSubmitter.update_package", return_value=True)
mocker.patch(
"auto_submit.diff_has_single_added_block",
return_value=(False, subprocess.CompletedProcess("", 0, stdout="diff output", stderr="")),
)

monkeypatch.chdir(tmp_path)
(tmp_path / "dst" / "pkg").mkdir(parents=True, exist_ok=True)
submitter = auto_submit.AutoSubmitter(
dst_project="dst",
src_project="src",
dry_run=False,
osc_cmd_str="osc",
)
res = submitter.handle_auto_submit("pkg")
assert res is False
assert caplog.records[0].getMessage() == "osc diff for pkg.changes does not look ok:\ndiff output"


def test_cpio(tmp_path: pathlib.Path) -> None:
cpio = pathlib.Path() / "tests" / "data" / "test.cpio"

Expand Down Expand Up @@ -700,3 +773,52 @@ def test_replace_node_modules_handling(
assert (
caplog.records[0].getMessage() == "openQA.spec does not contain node_modules specific sources; no changes made."
)


def test_diff_has_single_added_block(mocker: MockerFixture) -> None:
mock_run = mocker.patch("auto_submit._run_subprocess")
diff_header = """
Index: os-autoinst.changes
===================================================================
--- os-autoinst.changes (revision 528)
+++ os-autoinst.changes (working copy)
@@ -1,3 +1,11 @@
+-------------------------------------------------------------------
+- Update to version 5.1790592325.87022d5:
+ * fix: one
+ * fix: two
+
"""
diff = diff_header
mock_run.return_value = subprocess.CompletedProcess(["dummy"], 0, stdout=diff)
(diff_ok, res) = auto_submit.diff_has_single_added_block(["dummy"])
assert diff_ok is True

diff = (
diff_header
+ """
-------------------------------------------------------------------

@@ -5,6 +13,7 @@
* fix: three
+ * added commit
"""
)
mock_run.return_value = subprocess.CompletedProcess(["dummy"], 0, stdout=diff)
(diff_ok, res) = auto_submit.diff_has_single_added_block(["dummy"])
assert diff_ok is False

diff = (
diff_header
+ """
-------------------------------------------------------------------

@@ -5,6 +13,7 @@
* fix: three
- * removed commit
"""
)
mock_run.return_value = subprocess.CompletedProcess(["dummy"], 0, stdout=diff)
(diff_ok, res) = auto_submit.diff_has_single_added_block(["dummy"])
assert diff_ok is False
assert res is None
Loading