fix: balance conditional nesting when removing spec sections

[trungams]

collectSectionRanges now adjusts each section range to maintain balanced %if/%endif nesting before removal. This prevents producing invalid specs when sections are adjacent to or wrapped in conditional blocks.

Three cases are handled automatically:

• Balanced conditionals inside a section (no change needed)
• Trailing %if openers belonging to the next section (trimmed)
• Trailing %endif from a wrapping conditional block (trimmed, leaving an empty but valid conditional wrapper)

Conditionals interleaved with section content (spanning case) are detected and reported with an [ErrConditionalSpansSections] error.

Fixes #144

[Copilot]

Pull request overview

This PR updates RPM spec section/subpackage removal to preserve balanced %if/%endif nesting when removing section ranges, addressing invalid specs caused by boundary conditionals.

Changes:

• Adds conditional pair collection and range balancing before section removal.
• Updates RemoveSubpackage documentation and overlay docs for conditional handling.
• Adds tests for conditionalized subpackage removal scenarios.

Reviewed changes

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

File	Description
internal/rpm/spec/edit.go	Adds conditional balancing logic for section range removal.
internal/rpm/spec/edit_test.go	Adds coverage for conditional removal cases.
docs/user/reference/config/overlays.md	Documents conditional behavior for subpackage removal overlays.

Comments suppressed due to low confidence (1)

internal/rpm/spec/edit.go:1004

• This validation only handles straddling %if lines. When the straddling line is a %endif and there is real content later in the same section range, balanceRange trims at the %endif and leaves that later section content behind even though the caller requested the whole section/subpackage be removed. Either treat content after a straddling %endif as ErrConditionalSpansSections, or use a removal representation that can preserve the %endif while still deleting the remaining section body.

		if ifInside {
			// The %if is inside our range but %endif is outside. Check if there's
			// real content between the %if and the range end — that would mean the
			// conditional protects section content that spans into the next section.
			for i := p.ifLine + 1; i < r.end; i++ {
				if !isBlankOrComment(rawLines[i]) && conditionalDepthChange(rawLines[i]) == 0 {
					return r, fmt.Errorf(

[Copilot]

Pull request overview

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

[Copilot]

Pull request overview

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

[reubeno]

Can you tell if spec-append-lines et al. suffer from the same problem? I'm assuming we may need to update them to use this code path too. Otherwise, the lines might get appended after an %ifdef that wraps a succeeding section.

IIRC, that was one of the reasons that Tobias had added spec-insert-tag as a workaround.

I'm okay if you want to split that off as a separate PR, but since you're already in this code path / logic would be great if you could sort that out too.

[trungams]

I think it does, since the limitation initially comes from Spec.Visit not accounting for conditionals in section ranges. I need a bit more time to think of a direct improvement to Spec.Visit, so I'd prefer to do it in a follow-up PR. The solution I'm adding here is meant to go on top of collectSectionRanges to have as little impact on existing spec files as possible.

[reubeno]

Fair point. I'd like to see you continue working in this area after this PR -- to clean it up a bit. If we get more tests in place, we can have the confidence to better unify/factor the logic so it doesn't continue to accrete complexity.

[reubeno]

Maybe I'm missing something but don't we want to use the same logic here when %endif was inside but its starting %if wasn't? I'd only expect that to be okay if there's no content after.

[trungams]

This is already handled later in the function, on line 1035

[reubeno]

This is already handled later in the function, on line 1035

Understood, but isn't the conditional unneeded complexity? Or is there some other case I'm not thinking of?

[trungams]

I'm trying to cover the general cases so yes, it's very unlikely this happens and implies a spec is badly written, so we need to throw an error.

[reubeno]

I actually do like the trimming, but for consistency's sake can we keep this trimming consistent across both the case where we need to rebalance and we don't? (My preference would be to do it in both places.)

[trungams]

I'm going to add a post-processing step that cleans up the specs after all overlays are applied

[reubeno]

What kind of post processing?

[trungams]

I re-read the code and realized "for clean output" was a bit misleading and that trimming logic didn't actually do anything useful, so I removed it. Output should look consistent now

I think it's better to remove the trimming at least for this PR, it's sending me down a rabbit hole trying to properly define section boundaries and accounting for comments/whitespaces. I'll try to keep this PR simple for now

[Copilot]

Pull request overview

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

[Copilot]

Pull request overview

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

[reubeno]

This will introduce more deviations from input upstream specs than strictly needed; is this necessary? I don't think we should be in the business of rewriting the specs for readability, unless it's scoped to the specific overlay changes we're making.

I think the "trim" you had been doing for section collection was fine because it didn't actually remove any content, it just considered the section to end a bit earlier so that appended lines / removed sections preserve the space between the sections.

(Minor note: it's also quite inconsistent that this logic gets applied unilaterally to all parts of the spec, including sections not overlaid -- but is only triggered if a certain subset of overlays are requested.)

[trungams]

I was overthinking it - 2am thoughts :(

[reubeno]

No worries. I think there's a compromise somewhere in there that's reasonable -- and fine for a subsequent PR.

[reubeno]

Changes are looking good; primarily waiting on microsoft/azurelinux#17259 to go in first so this won't automatically be pulled in. (Looks like that would result in 2 specs rendering differently and resulting errors. We'll want to ingest this change and update those 2 rendered specs in a single PR to microsoft/azurelinux.)