fix: account for conditionals in Source9999 placement (#466)

Author: tobiasb-ms

internal/rpm/spec/edit.go

@@ -187,6 +187,31 @@ func tagFamily(tag string) string {
 	return lower[:end]
 }
 
+// conditionalDepthChange returns +1 for lines that open a conditional block (%if, %ifarch, etc.),
+// -1 for %endif, and 0 for everything else. Comments are ignored.
+func conditionalDepthChange(rawLine string) int {
+	trimmed := strings.TrimSpace(rawLine)
+	if strings.HasPrefix(trimmed, "#") {
+		return 0
+	}
+
+	token := strings.Fields(trimmed)
+	if len(token) == 0 {
+		return 0
+	}
+
+	lower := strings.ToLower(token[0])
+	if lower == "%endif" {
+		return -1
+	}
+
+	if strings.HasPrefix(lower, "%if") {
+		return 1
+	}
+
+	return 0
+}
+
 // InsertTag inserts a tag into the spec, placing it after the last existing tag from the
 // same "family" (e.g., Source9999 is placed after the last Source* tag). If no tags from
 // the same family exist, the tag is placed after the last tag of any kind. If there are no
@@ -196,6 +221,9 @@ func tagFamily(tag string) string {
 // (case-insensitive). For example, "Source0", "Source1", and "Source" all belong to the
 // "source" family.
 //
+// If the chosen insertion point falls inside a conditional block (%if/%endif), the tag is
+// placed after the closing %endif instead, so it remains unconditional.
+//
 // Note: When inserting into a sub-package (non-empty packageName), the corresponding
 // %package section must already exist in the spec; otherwise, an [ErrSectionNotFound]
 // error is returned.
@@ -205,30 +233,75 @@ func (s *Spec) InsertTag(packageName string, tag string, value string) error {
 	family := tagFamily(tag)
 	newLine := fmt.Sprintf("%s: %s", tag, value)
 
-	// First pass: find the best insertion point.
-	// Track both the last same-family tag line and the last tag of any kind.
-	lastFamilyTagLineNum := -1
-	lastAnyTagLineNum := -1
-	sectionFound := false
-
 	sectionName := ""
 	if packageName != "" {
 		sectionName = "%package"
 	}
 
+	result, err := s.findInsertTagPosition(sectionName, packageName, family)
+	if err != nil {
+		return err
+	}
+
+	// Determine insertion point: prefer same-family, then any tag, then fall back to AddTag.
+	insertAfterLine := result.lastFamilyTagLineNum
+	if insertAfterLine < 0 {
+		insertAfterLine = result.lastAnyTagLineNum
+	}
+
+	if insertAfterLine < 0 {
+		// No tags at all — fall back to AddTag behavior.
+		return s.AddTag(packageName, tag, value)
+	}
+
+	// If the insertion point is inside a conditional block, move it forward past the
+	// closing %endif so the new tag doesn't become conditional.
+	insertAfterLine = s.skipPastConditional(insertAfterLine, result.sectionEndLineNum)
+
+	// Insert after the found line (0-indexed, so insertAfterLine+1).
+	s.InsertLinesAt([]string{newLine}, insertAfterLine+1)
+
+	return nil
+}
+
+// insertTagScanResult holds the results of scanning a spec for a tag insertion point.
+type insertTagScanResult struct {
+	lastFamilyTagLineNum int
+	lastAnyTagLineNum    int
+	sectionEndLineNum    int
+}
+
+// findInsertTagPosition scans the spec to find the best insertion point for a tag of the
+// given family within the specified section/package. Returns the scan results or an error
+// if the target section is not found.
+func (s *Spec) findInsertTagPosition(
+	sectionName, packageName, family string,
+) (insertTagScanResult, error) {
+	result := insertTagScanResult{
+		lastFamilyTagLineNum: -1,
+		lastAnyTagLineNum:    -1,
+		sectionEndLineNum:    len(s.rawLines),
+	}
+
+	sectionFound := false
+
 	err := s.Visit(func(ctx *Context) error {
-		// Track whether the target section exists.
 		if ctx.Target.TargetType == SectionStartTarget {
 			if ctx.CurrentSection.SectName == sectionName && ctx.CurrentSection.Package == packageName {
 				sectionFound = true
 			}
 		}
 
+		if ctx.Target.TargetType == SectionEndTarget {
+			if ctx.CurrentSection.SectName == sectionName && ctx.CurrentSection.Package == packageName {
+				result.sectionEndLineNum = ctx.CurrentLineNum
+			}
+		}
+
 		if ctx.Target.TargetType != SectionLineTarget {
 			return nil
 		}
 
-		// Only consider tags in the target section/package.
 		if ctx.CurrentSection.SectName != sectionName || ctx.CurrentSection.Package != packageName {
 			return nil
 		}
@@ -242,37 +315,50 @@ func (s *Spec) InsertTag(packageName string, tag string, value string) error {
 			return nil
 		}
 
-		lastAnyTagLineNum = ctx.CurrentLineNum
+		result.lastAnyTagLineNum = ctx.CurrentLineNum
 
 		if tagFamily(tagLine.Tag) == family {
-			lastFamilyTagLineNum = ctx.CurrentLineNum
+			result.lastFamilyTagLineNum = ctx.CurrentLineNum
 		}
 
 		return nil
 	})
 	if err != nil {
-		return fmt.Errorf("failed to scan spec for tag insertion point:\n%w", err)
+		return result, fmt.Errorf("failed to scan spec for tag insertion point:\n%w", err)
 	}
 
 	if !sectionFound {
-		return fmt.Errorf("section %#q (package=%#q) not found:\n%w", sectionName, packageName, ErrSectionNotFound)
+		return result, fmt.Errorf("section %#q (package=%#q) not found:\n%w", sectionName, packageName, ErrSectionNotFound)
 	}
 
-	// Determine insertion point: prefer same-family, then any tag, then fall back to AddTag.
-	insertAfterLine := lastFamilyTagLineNum
-	if insertAfterLine < 0 {
-		insertAfterLine = lastAnyTagLineNum
+	return result, nil
+}
+
+// skipPastConditional checks whether lineNum falls inside a conditional block by computing
+// the conditional nesting depth from the start of the file up to that line. If depth > 0,
+// it scans forward to find the %endif that brings depth back to 0 and returns that line
+// number. Otherwise it returns lineNum unchanged.
+func (s *Spec) skipPastConditional(lineNum int, sectionEnd int) int {
+	// Compute conditional depth at the insertion point by scanning from the start.
+	depth := 0
+	for i := 0; i <= lineNum && i < len(s.rawLines); i++ {
+		depth += conditionalDepthChange(s.rawLines[i])
 	}
 
-	if insertAfterLine < 0 {
-		// No tags at all — fall back to AddTag behavior.
-		return s.AddTag(packageName, tag, value)
+	if depth <= 0 {
+		return lineNum
 	}
 
-	// Insert after the found line (0-indexed, so insertAfterLine+1).
-	s.InsertLinesAt([]string{newLine}, insertAfterLine+1)
+	// Scan forward to find the %endif that closes the conditional.
+	for i := lineNum + 1; i < sectionEnd && i < len(s.rawLines); i++ {
+		depth += conditionalDepthChange(s.rawLines[i])
+		if depth <= 0 {
+			return i
+		}
+	}
 
-	return nil
+	// Could not find a closing %endif within the section; return the original position.
+	return lineNum
 }
 
 // PrependLinesToSection prepends the given lines to the start of the specified section, placing

internal/rpm/spec/edit_test.go

@@ -702,6 +702,78 @@ Source9999: macros.azl.macros
 
 %description
 Some description
+`,
+			tag:   "Source9999",
+			value: "macros.azl.macros",
+		},
+		{
+			name: "insert after Source inside conditional block",
+			input: `Name: test
+Source0: test-1.0.tar.gz
+%if %{with extra}
+Source31: extra-aarch64.h
+Source32: extra-x86_64.h
+%endif
+BuildRequires: gcc
+`,
+			expectedOutput: `Name: test
+Source0: test-1.0.tar.gz
+%if %{with extra}
+Source31: extra-aarch64.h
+Source32: extra-x86_64.h
+%endif
+Source9999: macros.azl.macros
+BuildRequires: gcc
+`,
+			tag:   "Source9999",
+			value: "macros.azl.macros",
+		},
+		{
+			name: "insert after Source inside nested conditional",
+			input: `Name: test
+Source0: test-1.0.tar.gz
+%if %{with jit}
+%ifarch x86_64
+Source31: jit-x86.h
+%endif
+Source32: jit-common.h
+%endif
+BuildRequires: gcc
+`,
+			expectedOutput: `Name: test
+Source0: test-1.0.tar.gz
+%if %{with jit}
+%ifarch x86_64
+Source31: jit-x86.h
+%endif
+Source32: jit-common.h
+%endif
+Source9999: macros.azl.macros
+BuildRequires: gcc
+`,
+			tag:   "Source9999",
+			value: "macros.azl.macros",
+		},
+		{
+			name: "insert with mixed conditional and unconditional sources",
+			input: `Name: test
+Source0: test-1.0.tar.gz
+Source1: extra.tar.gz
+%if %{with jit}
+Source31: jit-aarch64.h
+Source32: jit-x86_64.h
+%endif
+BuildRequires: gcc
+`,
+			expectedOutput: `Name: test
+Source0: test-1.0.tar.gz
+Source1: extra.tar.gz
+%if %{with jit}
+Source31: jit-aarch64.h
+Source32: jit-x86_64.h
+%endif
+Source9999: macros.azl.macros
+BuildRequires: gcc
 `,
 			tag:   "Source9999",
 			value: "macros.azl.macros",