Avoid unnecessary re-parsing in GetPackageInfoTool

[nforro]

No description provided.

[qodo-for-packit]

PR Summary by Qodo

Avoid redundant patch path expansion when parsing specfiles

✨ Enhancement 🕐 Less than 10 minutes

AI Description

• Filter valid patches using raw spec location, not expanded filesystem paths
• Reuse expanded patch paths only where strictly needed for file list generation
• Reduce repeated path expansion work during specfile package info extraction

Diagram

graph TD
  A["GetPackageInfoTool"] --> B["specfile._run()"] --> C["Specfile"] --> D["get_all_patches()"] --> E["valid_patches"] --> F["patch_files"]
  E --> G["_extract_strip_levels()"]

  subgraph Legend
    direction LR
    _tool([Tool]) ~~~ _mod["Module/Function"] ~~~ _data[(Derived data)]
  end

Loading

High-Level Assessment

The chosen approach (filter/map using raw location, and only expand paths where required) is the simplest way to avoid redundant work without changing downstream behavior. Alternatives like memoizing expanded_location or changing get_all_patches() to precompute/return cached expansions would add complexity and broader surface area for minimal additional gain.

Files changed (1) +2 / -2

Enhancement (1) +2 / -2

specfile.pyStop forcing early patch path expansion during filtering/mapping +2/-2

Stop forcing early patch path expansion during filtering/mapping

• Switches patch validity filtering from 'expanded_location' to raw 'location', preventing unnecessary expansion during list comprehension evaluation. Updates the patch-number-to-filename map to store 'location' instead of 'expanded_location', reducing repeated expansion while keeping 'patch_files' generation based on 'expanded_location'.

ymir/tools/unprivileged/specfile.py

[qodo-for-packit]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📜 Skill insights (0)

1. Strip-level keys mismatch ✓ Resolved 🐞 Bug ≡ Correctness

Description

GetPackageInfoTool now builds number_to_filename (and thus patch_strip_levels) from
Patch.location but returns patch_files from Patch.expanded_location, so downstream strip
lookups will miss and fall back to -p1 when location != expanded_location (e.g., macro patch
names). This can apply patches with the wrong -p level and cause patch application failures.

Code

ymir/tools/unprivileged/specfile.py[R119-121]

+                valid_patches = [p for p in get_all_patches(spec) if p.valid and p.location]
                patch_files = [p.expanded_location for p in valid_patches]
-                number_to_filename = {p.number: p.expanded_location for p in valid_patches}
+                number_to_filename = {p.number: p.location for p in valid_patches}

Relevance

●●● Strong

Correctness regression causing wrong strip-level lookup; team usually accepts defensive bugfixes in
tools.

PR-#541
PR-#655

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The PR changed number_to_filename to use p.location while patch_files still uses
p.expanded_location, creating two different filename representations. Downstream, strip level is
looked up by the patch_file string taken from patch_files, so keys based on location won’t
match when patch names are macro-expanded; the existing unit test explicitly asserts expanded macro
names are used as strip-level keys.

ymir/tools/unprivileged/specfile.py[116-131]
ymir/tools/unprivileged/upstream_tools.py[619-632]
ymir/tools/unprivileged/tests/unit/test_specfile.py[307-333]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`GetPackageInfoTool` returns `patch_files` based on `p.expanded_location`, but computes `patch_strip_levels` keys from `p.location` via `number_to_filename`. Downstream code looks up strip levels using the strings from `patch_files`, so the lookup fails whenever `location` differs from `expanded_location` (notably when the Patch tag contains macros).

### Issue Context
- `ApplyDownstreamPatchesTool` does: `strip = (tool_input.patch_strip_levels or {}).get(patch_file, 1)` where `patch_file` comes from `patch_files`.
- Unit test fixture `spec_with_macro_patches` expects macro expansion (`mypackage-3.5.3-...`) to be used consistently for both `patch_files` and `patch_strip_levels` keys.

### Fix Focus Areas
- ymir/tools/unprivileged/specfile.py[116-131]

### Suggested change
Keep using expanded filenames for both `patch_files` and `number_to_filename`, but avoid repeated property access by caching expanded values once:

```py
patches = [p for p in get_all_patches(spec) if p.valid and p.expanded_location]
patch_files = [p.expanded_location for p in patches]
number_to_filename = {p.number: p.expanded_location for p in patches}
```

(or build a `[(p.number, p.expanded_location)]` list once and derive both structures from it).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

Context used

✅ Compliance rules (platform): 7 rules

Tip of the day

💡 Did you know, you can group findings by type and pick your Finding display, from Minimal to Full

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

[nforro]

/agentic_review

[qodo-for-packit]

Code review by qodo was updated up to the latest commit 6c3a1fc

[nforro]

/agentic_review

[qodo-for-packit]

Code review by qodo was updated up to the latest commit 7b873f7