Skip to content

fix(add-patch): Make patch discovery work across whole spec#18

Merged
reubeno merged 1 commit intomicrosoft:mainfrom
binujp:bphilip/patch-add-fix
Mar 20, 2026
Merged

fix(add-patch): Make patch discovery work across whole spec#18
reubeno merged 1 commit intomicrosoft:mainfrom
binujp:bphilip/patch-add-fix

Conversation

@binujp
Copy link
Contributor

@binujp binujp commented Mar 20, 2026

"patch-add" overlay type assumes patch numbering is per-package section. This means a patchX defined inside a sub-package can be identified as the highest patch number. This can cause a collision with another patchX defined elsewhere in the spec.

This PR makes patch-add look for highest existing patchN across the spec, the same for patch-remove and also some tests to verify the funcitonality. To avoid overloading existing methods new ones were added. While this is a few extra lines of code, maintenance and feature addition will become much easier.

Copilot AI review requested due to automatic review settings March 20, 2026 16:44
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Fixes patch overlay behavior by making patch tag discovery/removal operate across the entire RPM spec (all packages) to avoid PatchN collisions when sub-packages define their own Patch entries.

Changes:

  • Change patch tag scanning to find the highest PatchN across the whole spec (not per-package).
  • Update patch removal to remove matching PatchN tags across all packages (and update overlay call sites).
  • Add/adjust tests and introduce VisitTags / VisitTagsPackage split to support both global and package-scoped traversal.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
internal/rpm/spec/edit.go Updates traversal APIs and patch add/remove logic to operate across the whole spec for PatchN discovery/removal.
internal/rpm/spec/edit_test.go Updates existing tests for new semantics and adds coverage for tag visiting and multi-package patch removal.
internal/app/azldev/core/sources/overlays.go Updates overlay application to use the new RemovePatchEntry signature.


matched, matchErr := doublestar.Match(pattern, tagLine.Value)
if matchErr != nil {
return nil
Copy link

Copilot AI Mar 20, 2026

Choose a reason for hiding this comment

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

Errors from doublestar.Match (e.g., invalid glob patterns) are currently swallowed, which can lead to misleading outcomes (such as returning "no patches matching" when the real issue is an invalid pattern). Propagate matchErr so callers get a clear failure reason (or wrap it with context including the pattern).

Suggested change
return nil
return fmt.Errorf("failed to match glob pattern %#q against %#q:\n%w", pattern, tagLine.Value, matchErr)

Copilot uses AI. Check for mistakes.
@binujp binujp force-pushed the bphilip/patch-add-fix branch from 6916d5e to afd4b4e Compare March 20, 2026 17:00
@reubeno
Copy link
Member

reubeno commented Mar 20, 2026

Changes look good, @binujp. We'll get it merged after the latest round of PR checks pass.

"patch-add" overlay type assumes patch numbering is per-package
section. This means a patchX defined inside a sub-package can be
identified as the highest patch number. This can cause a collision
with another patchX defined elsewhere in the spec.

This PR makes patch-add look for highest existing patchN across the
spec, the same for patch-remove and also some tests to verify the
funcitonality. To avoid overloading existing methods new ones were
added. While this is a few extra lines of code, maintenance and
feature addition will become much easier.
Copilot AI review requested due to automatic review settings March 20, 2026 19:01
@binujp binujp force-pushed the bphilip/patch-add-fix branch from afd4b4e to fe2e646 Compare March 20, 2026 19:01
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

}
case projectconfig.ComponentOverlayRemovePatch:
err := openedSpec.RemovePatchEntry(overlay.PackageName, overlay.Filename)
err := openedSpec.RemovePatchEntry(overlay.Filename)
Copy link

Copilot AI Mar 20, 2026

Choose a reason for hiding this comment

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

patch-remove overlays now call openedSpec.RemovePatchEntry(overlay.Filename) and ignore overlay.PackageName, so any package = "..." setting in config will have no effect. Either restore package-scoped removal (e.g., keep a package-aware API and call it here) or update the user docs/schema to explicitly state that patch-remove always scans/removes across the whole spec regardless of package to avoid surprising users.

Suggested change
err := openedSpec.RemovePatchEntry(overlay.Filename)
err := openedSpec.RemovePatchEntry(overlay.PackageName, overlay.Filename)

Copilot uses AI. Check for mistakes.
@reubeno reubeno merged commit 6de8576 into microsoft:main Mar 20, 2026
18 of 19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants