tools: add helper tool about verify backports#3900
Open
ffromani wants to merge 1 commit into
Open
Conversation
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ffromani The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
Member
Author
|
/hold self-review pending, reviewers please ignore till WIP is removed |
openshift-ci
Bot
added
the
do-not-merge/hold
|
Warning Rate limit exceeded
To continue reviewing without waiting, purchase usage credits in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (13)
📝 WalkthroughWalkthroughAdds a new command-line tool ChangesVerify-Backport Tool
Sequence Diagram(s)sequenceDiagram
participant User as User
participant CLI as verify-backport
participant Git as Git
participant FS as FileSystem/Output
User->>CLI: invoke with flags (branch/range, --json, --verbose)
CLI->>Git: fetch commit hashes, patch-ids, messages for ranges
Git-->>CLI: return commit metadata and patch diffs
CLI->>CLI: parse patches, compute patch comparisons, match commits
CLI->>FS: write JSON or print human-readable report (stdout)
CLI-->>User: exit code (0/1) based on missing/unmatched
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
ffromani
force-pushed
the
tool-verify-backport
branch
from
3885a6d to
93ff134
Compare
ffromani
changed the title
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
Member
Author
|
example run: |
ffromani
force-pushed
the
tool-verify-backport
branch
from
93ff134 to
22618a5
Compare
Member
Author
|
/hold cancel |
openshift-ci
Bot
removed
the
do-not-merge/hold
ffromani
force-pushed
the
tool-verify-backport
branch
from
22618a5 to
46be65f
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
tools/verify-backport/verify-backport.go (1)
864-892: 💤 Low value
parsePatchFilesmishandles a few real-world diff inputs.Two issues worth tightening before this is depended on:
- Filenames with spaces.
strings.Fields(lines[0])treatsdiff --git a/path with space b/path with spaceas five tokens;parts[1]becomespath(not the full b-path). Git quotes such names ("a/path\twith"), so consider parsing the header explicitly or stripping thea//b/halves.- Content lines starting with
+++/---.strings.HasPrefix(line, "+++")/"---"was meant to skip the file-header markers, but it also drops legitimate added/removed content whose first chars are++or--(think config files, banners, table separators). The header lines always come immediately afterdiff --git/index …; either match them strictly ("+++ "/"--- ") or track whether you have already seen the header pair for the current chunk.Both gaps will silently undercount changes and skew the additive/edits classification.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tools/verify-backport/verify-backport.go` around lines 864 - 892, The parsePatchFiles function misparses file paths with spaces and incorrectly drops content lines that start with "+++"/"---"; update parsePatchFiles to (1) parse the diff header line more robustly instead of using strings.Fields(lines[0]) — locate the a/ and b/ path tokens (or use a regex) and extract the full b-path into fp.path, stripping an optional leading "b/" and any surrounding quotes/escaping so filenames with spaces are preserved (refer to lines[0] and fp.path), and (2) only treat header markers as file headers when they are the exact header form (e.g. start with "+++ " or "--- " including the trailing space) or by tracking that the header pair has been consumed for the current chunk; this ensures lines beginning with "+++" or "---" that are real content still get recorded into addedLines/removedLines (refer to addedLines and removedLines).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tools/verify-backport/README.md`:
- Around line 15-17: Update the README wording to avoid conflating whitespace
normalization with the stability behavior of git patch-id; specifically, change
the sentence about "`git patch-id --stable` produces the same id regardless of
whitespace differences" to state that whitespace is normalized by default in all
patch-id modes (except `--verbatim`) and that `--stable`'s primary purpose is to
ensure hash stability across file/diff reordering, and also note that there is
no separate `--ignore-whitespace` flag — whitespace handling is determined by
the mode (`--stable`, `--unstable`, `--verbatim`).
In `@tools/verify-backport/verify-backport.go`:
- Around line 240-244: The git invocations in getHashes, getMessages, and
getCommitPatch pass user-supplied revisions (gitRange and hash) directly to
exec.Command which allows values beginning with "-" to be interpreted as flags;
modify each exec.Command call that builds git log / git diff-tree to insert a
"--" separator immediately after the revision argument (e.g., pass "git", "log",
"--no-merges", "--format=%H", gitRange, "--") so git treats the value as a
revision, and add a simple validation to reject empty revisions or
whitespace-only values before invoking the command.
- Around line 355-423: The text report's matched metric (in printReport using
matchCount) only counts StatusMatch, but the JSON report's Summary.Matched and
the exit-code logic treat StatusMismatchMessage as matched — make the definition
consistent by changing the JSON/path that populates Summary.Matched
(printJSONReport or the code that builds the Summary) to count only results
where r.Status == StatusMatch (same as printReport's matchCount), and ensure any
exit decision or Summary construction (the code that sets Summary.Matched) uses
that same predicate so StatusMismatchMessage is not treated as matched.
- Around line 256-278: getPatchIDs currently builds a shell pipeline string and
calls exec.Command("sh", "-c", pipeline), which interpolates gitRange into a
shell command (unsafe and brittle); change it to run the two git commands
directly and wire their stdio with an io.Pipe (or use cmd.StdoutPipe +
cmd2.Stdin); specifically, replace the sh -c invocation with
exec.Command("git","log","-p","--no-merges", gitRange) and
exec.Command("git","patch-id","--stable"), connect the first command's Stdout to
the second command's Stdin, start both processes, wait for completion, capture
output/errors (use buffers for Stderr on each command and return a wrapped error
including stderr if either fails), and keep the rest of getPatchIDs logic
(parsing patch-id output) unchanged.
---
Nitpick comments:
In `@tools/verify-backport/verify-backport.go`:
- Around line 864-892: The parsePatchFiles function misparses file paths with
spaces and incorrectly drops content lines that start with "+++"/"---"; update
parsePatchFiles to (1) parse the diff header line more robustly instead of using
strings.Fields(lines[0]) — locate the a/ and b/ path tokens (or use a regex) and
extract the full b-path into fp.path, stripping an optional leading "b/" and any
surrounding quotes/escaping so filenames with spaces are preserved (refer to
lines[0] and fp.path), and (2) only treat header markers as file headers when
they are the exact header form (e.g. start with "+++ " or "--- " including the
trailing space) or by tracking that the header pair has been consumed for the
current chunk; this ensures lines beginning with "+++" or "---" that are real
content still get recorded into addedLines/removedLines (refer to addedLines and
removedLines).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: a6abdd72-2b7b-4d08-afe6-523afdb6ae29
📒 Files selected for processing (2)
tools/verify-backport/README.mdtools/verify-backport/verify-backport.go
ffromani
force-pushed
the
tool-verify-backport
branch
from
46be65f to
63f2d3e
Compare
Member
Author
|
/hold |
openshift-ci
Bot
added
the
do-not-merge/hold
ffromani
force-pushed
the
tool-verify-backport
branch
from
63f2d3e to
582cd31
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
tools/verify-backport/verify-backport.go (2)
360-364:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAlign “matched” semantics across text summary, JSON summary, and exit code.
Text output and exit behavior treat
MISMATCH-MESSAGEas not matched, but JSON currently counts it as matched. This can mislead automation consuming--json.Proposed fix
switch r.Status { - case StatusMatch, StatusMismatchMessage: + case StatusMatch: report.Summary.Matched++ jr.Main = &jsonCommit{Hash: r.MainMatch.Hash, Subject: r.MainMatch.Subject()} + case StatusMismatchMessage: + jr.Main = &jsonCommit{Hash: r.MainMatch.Hash, Subject: r.MainMatch.Subject()} case StatusUnmatched:Also applies to: 829-831
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tools/verify-backport/verify-backport.go` around lines 360 - 364, The JSON summary currently treats MISMATCH-MESSAGE as a match while the text summary and exit code do not; update the counting logic in the loop that iterates over results (the matchCount increment inside the for _, r := range results switch on r.Status) to only increment for the explicit StatusMatch value and not for MISMATCH-MESSAGE or any other non-matching status, and apply the same change to the analogous counting block referenced around the second occurrence (the other loop handling results at the later location) so the JSON summary, text output, and exit code share identical "matched" semantics.
239-240:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winHarden git invocation paths: remove shell execution and terminate revision args safely.
gitRangeis interpolated intosh -c(Line 256-259), which is command-injection prone; and revision/hash args are passed without--, so flag-like input can be parsed as git options (Line 240, Line 281, Line 864).Proposed fix
import ( + "bytes" "encoding/json" "flag" "fmt" "os" "os/exec" @@ "golang.org/x/term" ) @@ +func validateRevisionArg(name, v string) error { + v = strings.TrimSpace(v) + if v == "" { + return fmt.Errorf("%s cannot be empty", name) + } + if strings.HasPrefix(v, "-") { + return fmt.Errorf("%s cannot start with '-': %q", name, v) + } + return nil +} + func getHashes(gitRange string) ([]string, error) { - cmd := exec.Command("git", "log", "--no-merges", "--format=%H", gitRange) + if err := validateRevisionArg("git range", gitRange); err != nil { + return nil, err + } + cmd := exec.Command("git", "log", "--no-merges", "--format=%H", gitRange, "--") @@ func getPatchIDs(gitRange string) (map[string]string, error) { - pipeline := fmt.Sprintf("git log -p --no-merges %s | git patch-id --stable", gitRange) - cmd := exec.Command("sh", "-c", pipeline) - out, err := cmd.Output() - if err != nil { - return nil, fmt.Errorf("patch-id pipeline: %w: %s", err, cmdStderr(err)) - } + if err := validateRevisionArg("git range", gitRange); err != nil { + return nil, err + } + logCmd := exec.Command("git", "log", "-p", "--no-merges", gitRange, "--") + idCmd := exec.Command("git", "patch-id", "--stable") + pipe, err := logCmd.StdoutPipe() + if err != nil { + return nil, fmt.Errorf("stdout pipe: %w", err) + } + idCmd.Stdin = pipe + var out bytes.Buffer + var idErr bytes.Buffer + var logErr bytes.Buffer + idCmd.Stdout = &out + idCmd.Stderr = &idErr + logCmd.Stderr = &logErr + if err := logCmd.Start(); err != nil { + return nil, fmt.Errorf("git log: %w", err) + } + if err := idCmd.Run(); err != nil { + return nil, fmt.Errorf("git patch-id: %w: %s", err, strings.TrimSpace(idErr.String())) + } + if err := logCmd.Wait(); err != nil { + return nil, fmt.Errorf("git log: %w: %s", err, strings.TrimSpace(logErr.String())) + } @@ func getMessages(gitRange string) (map[string]string, error) { - cmd := exec.Command("git", "log", "--no-merges", "--format=%H%x00%B%x00", gitRange) + if err := validateRevisionArg("git range", gitRange); err != nil { + return nil, err + } + cmd := exec.Command("git", "log", "--no-merges", "--format=%H%x00%B%x00", gitRange, "--") @@ func getCommitPatch(hash string) (string, error) { - cmd := exec.Command("git", "diff-tree", "-p", hash) + if err := validateRevisionArg("commit hash", hash); err != nil { + return "", err + } + cmd := exec.Command("git", "diff-tree", "-p", hash, "--")#!/bin/bash set -euo pipefail echo "1) Shell-based git command execution:" rg -n 'exec\.Command\("sh",\s*"-c"' --type go echo echo "2) git command callsites using revision/hash args:" rg -n 'exec\.Command\("git",\s*"(log|diff-tree)".*(gitRange|hash)' --type go echo echo "3) Current implementations around affected ranges:" fd -i 'verify-backport.go' --exec sed -n '235,290p' {} fd -i 'verify-backport.go' --exec sed -n '860,870p' {}Also applies to: 256-259, 281-282, 863-865
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tools/verify-backport/verify-backport.go` around lines 239 - 240, The git invocation is using shell execution and passing revision/hash args without a "--", which allows command injection and lets inputs be interpreted as git options; update getHashes and other callsites that use exec.Command("sh", "-c", ...) and exec.Command("git", ...) to call git directly with distinct args and terminate revisions with "--". Concretely, stop using exec.Command("sh","-c", ...) and instead call exec.Command("git", "log", "--no-merges", "--format=%H", "--", gitRange) in getHashes, and similarly add a "--" before any revision/hash args in git diff-tree / other git invocations (e.g., where exec.Command("git","diff-tree",..., hash) is used) so the revision is passed as a single safe argument rather than parsed as an option.
🧹 Nitpick comments (1)
tools/verify-backport/verify-backport_test.go (1)
491-527: ⚡ Quick winAdd a regression case for main-only edit labels.
Given main-only unmatched entries are possible, include a
fileResult{mainPath: "...", bpPath: ""}case to lock expected label output and prevent filename loss regressions.Suggested test case
{ name: "edits both sides", fr: fileResult{bpPath: "foo.go", changeType: changeEdits, onlyBP: []string{"a"}, onlyMain: []string{"b", "c"}}, want: "foo.go (+1 only in backport, +2 only in main)", }, + { + name: "edits main-only path fallback", + fr: fileResult{mainPath: "main_only.go", changeType: changeEdits, onlyMain: []string{"x"}}, + want: "main_only.go (+0 only in backport, +1 only in main)", + }, }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tools/verify-backport/verify-backport_test.go` around lines 491 - 527, Add a regression test in TestFileResultLabel to cover main-only unmatched entries: add a case named like "main-only edit" where fileResult has mainPath set (e.g., "old/foo.go"), bpPath empty, changeType set to changeEdits (or changeAdditive if appropriate), and onlyMain populated (e.g., []string{"a"}); set want to the expected label that preserves mainPath (e.g., "old/foo.go (+1 only in main)") to prevent filename-loss regressions—look for TestFileResultLabel and the fileResult fields mainPath, bpPath, changeType, onlyMain to place the new test case.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tools/verify-backport/verify-backport_githistory_test.go`:
- Around line 169-216: The tests incorrectly assume matchCommits returns matched
results then unmatched main commits; instead matchCommits returns matched
results first and unmatched backport commits as the second return. Update the
two failing subtests to assert the second return value refers to unmatched
backport commits (not unmatched main) — e.g., check that unmatched
contains/backport hash commitMakefileHash or commitCherryPickHash as
appropriate, and adjust the Status expectations (use StatusUnmatched for results
where patch-ids differ and StatusMatch where patch-ids equal) while referencing
matchCommits, results, unmatched, StatusMatch, StatusUnmatched,
commitDockerfileHash, commitMakefileHash, commitCherryPickHash and
commitOriginalHash to locate and correct the assertions.
In `@tools/verify-backport/verify-backport.go`:
- Around line 559-570: The code currently builds bpByBase and mainByBase as
map[string]filePatch which causes basename collisions to overwrite candidates;
change these to map[string][]filePatch (populate from bpFiles and mainFiles) so
all same basenames are preserved, then in the rename-detection logic
(referencing bpByBase, mainByBase and filePatch) resolve basename candidate
lists by computing and preferring the best match (e.g., exact line-set equality
or highest overlap) instead of a single-value lookup; apply the same change to
the other similar maps created at the second location (the block around the
other bpByBase/mainByBase usage).
- Around line 483-490: The report path selection currently sets path :=
fr.bpPath and only handles RENAMED by combining fr.mainPath -> fr.bpPath, which
causes main-only entries (where fr.bpPath == "") to lose the filename; update
the logic in the block using fr.bpPath/fr.mainPath/fr.relation (the variable
named path near bpCount/mainCount) to prefer a non-empty fr.bpPath but fall back
to fr.mainPath when bpPath is empty, and still format RENAMED as "main -> bp"
only when both are present; apply the same fix to the analogous code around the
second occurrence that the reviewer noted.
---
Duplicate comments:
In `@tools/verify-backport/verify-backport.go`:
- Around line 360-364: The JSON summary currently treats MISMATCH-MESSAGE as a
match while the text summary and exit code do not; update the counting logic in
the loop that iterates over results (the matchCount increment inside the for _,
r := range results switch on r.Status) to only increment for the explicit
StatusMatch value and not for MISMATCH-MESSAGE or any other non-matching status,
and apply the same change to the analogous counting block referenced around the
second occurrence (the other loop handling results at the later location) so the
JSON summary, text output, and exit code share identical "matched" semantics.
- Around line 239-240: The git invocation is using shell execution and passing
revision/hash args without a "--", which allows command injection and lets
inputs be interpreted as git options; update getHashes and other callsites that
use exec.Command("sh", "-c", ...) and exec.Command("git", ...) to call git
directly with distinct args and terminate revisions with "--". Concretely, stop
using exec.Command("sh","-c", ...) and instead call exec.Command("git", "log",
"--no-merges", "--format=%H", "--", gitRange) in getHashes, and similarly add a
"--" before any revision/hash args in git diff-tree / other git invocations
(e.g., where exec.Command("git","diff-tree",..., hash) is used) so the revision
is passed as a single safe argument rather than parsed as an option.
---
Nitpick comments:
In `@tools/verify-backport/verify-backport_test.go`:
- Around line 491-527: Add a regression test in TestFileResultLabel to cover
main-only unmatched entries: add a case named like "main-only edit" where
fileResult has mainPath set (e.g., "old/foo.go"), bpPath empty, changeType set
to changeEdits (or changeAdditive if appropriate), and onlyMain populated (e.g.,
[]string{"a"}); set want to the expected label that preserves mainPath (e.g.,
"old/foo.go (+1 only in main)") to prevent filename-loss regressions—look for
TestFileResultLabel and the fileResult fields mainPath, bpPath, changeType,
onlyMain to place the new test case.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: e3f0f476-7673-46e7-882f-708b6013d738
📒 Files selected for processing (13)
tools/verify-backport/README.mdtools/verify-backport/testdata/json_all_matched.golden.jsontools/verify-backport/testdata/json_githistory_matched.golden.jsontools/verify-backport/testdata/json_githistory_unmatched_closest.golden.jsontools/verify-backport/testdata/json_mismatch_message.golden.jsontools/verify-backport/testdata/json_missing_backports.golden.jsontools/verify-backport/testdata/json_missing_not_reported.golden.jsontools/verify-backport/testdata/json_mixed_statuses.golden.jsontools/verify-backport/testdata/json_unmatched_no_closest.golden.jsontools/verify-backport/verify-backport.gotools/verify-backport/verify-backport_githistory_test.gotools/verify-backport/verify-backport_json_test.gotools/verify-backport/verify-backport_test.go
✅ Files skipped from review due to trivial changes (2)
- tools/verify-backport/testdata/json_mismatch_message.golden.json
- tools/verify-backport/testdata/json_unmatched_no_closest.golden.json
Comment on lines
+169
to
+216
| t.Run("cherry-pick with diverged patch-id is unmatched", func(t *testing.T) { | ||
| if !commitExists(commitCherryPickHash) || !commitExists(commitOriginalHash) { | ||
| t.Skip("cherry-pick or original commit not found in local history") | ||
| } | ||
|
|
||
| cpMsg := getCommitMessage(t, commitCherryPickHash) | ||
| origMsg := getCommitMessage(t, commitOriginalHash) | ||
|
|
||
| bp := []CommitInfo{{Hash: commitCherryPickHash, PatchID: commitCherryPickPatchID, Message: cpMsg}} | ||
| main := []CommitInfo{{Hash: commitOriginalHash, PatchID: commitOriginalPatchID, Message: origMsg}} | ||
|
|
||
| results, _ := matchCommits(bp, main) | ||
|
|
||
| if len(results) != 1 { | ||
| t.Fatalf("expected 1 result, got %d", len(results)) | ||
| } | ||
| if results[0].Status != StatusUnmatched { | ||
| t.Errorf("status = %v, want StatusUnmatched (different patch-ids)", results[0].Status) | ||
| } | ||
| if results[0].ClosestMatch == nil { | ||
| t.Errorf("ClosestMatch should be set via subject fallback (same subject)") | ||
| } | ||
| }) | ||
|
|
||
| t.Run("two commits with distinct patch-ids produce correct unmatched main", func(t *testing.T) { | ||
| if !commitExists(commitMakefileHash) || !commitExists(commitDockerfileHash) { | ||
| t.Skip("required commits not found") | ||
| } | ||
|
|
||
| makefileMsg := getCommitMessage(t, commitMakefileHash) | ||
| dockerfileMsg := getCommitMessage(t, commitDockerfileHash) | ||
|
|
||
| bp := []CommitInfo{ | ||
| {Hash: commitMakefileHash, PatchID: commitMakefilePatchID, Message: makefileMsg}, | ||
| } | ||
| main := []CommitInfo{ | ||
| {Hash: commitMakefileHash, PatchID: commitMakefilePatchID, Message: makefileMsg}, | ||
| {Hash: commitDockerfileHash, PatchID: commitDockerfilePatchID, Message: dockerfileMsg}, | ||
| } | ||
|
|
||
| results, unmatched := matchCommits(bp, main) | ||
|
|
||
| if len(results) != 1 || results[0].Status != StatusMatch { | ||
| t.Errorf("expected 1 MATCH result, got %v", results) | ||
| } | ||
| if len(unmatched) != 1 || unmatched[0].Hash != commitDockerfileHash { | ||
| t.Errorf("expected dockerfile commit as unmatched main, got %v", unmatched) | ||
| } |
There was a problem hiding this comment.
matchCommits contract is asserted incorrectly in these subtests.
These assertions expect unmatched entries in results and treat the second return as “unmatched main”, but matchCommits returns matched results first and unmatched backport commits second. This will make these tests fail against the current function behavior.
Proposed test-fix direction
- results, _ := matchCommits(bp, main)
- if len(results) != 1 { ... }
- if results[0].Status != StatusUnmatched { ... }
- if results[0].ClosestMatch == nil { ... }
+ results, unmatched := matchCommits(bp, main)
+ if len(results) != 0 {
+ t.Fatalf("expected 0 matched results, got %d", len(results))
+ }
+ if len(unmatched) != 1 || unmatched[0].Hash != commitCherryPickHash {
+ t.Fatalf("expected cherry-pick in unmatched backports, got %v", unmatched)
+ }- results, unmatched := matchCommits(bp, main)
- if len(results) != 1 || results[0].Status != StatusMatch { ... }
- if len(unmatched) != 1 || unmatched[0].Hash != commitDockerfileHash { ... }
+ results, unmatched := matchCommits(bp, main)
+ if len(results) != 1 || results[0].Status != StatusMatch {
+ t.Errorf("expected 1 MATCH result, got %v", results)
+ }
+ if len(unmatched) != 0 {
+ t.Errorf("expected 0 unmatched backport commits, got %v", unmatched)
+ }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tools/verify-backport/verify-backport_githistory_test.go` around lines 169 -
216, The tests incorrectly assume matchCommits returns matched results then
unmatched main commits; instead matchCommits returns matched results first and
unmatched backport commits as the second return. Update the two failing subtests
to assert the second return value refers to unmatched backport commits (not
unmatched main) — e.g., check that unmatched contains/backport hash
commitMakefileHash or commitCherryPickHash as appropriate, and adjust the Status
expectations (use StatusUnmatched for results where patch-ids differ and
StatusMatch where patch-ids equal) while referencing matchCommits, results,
unmatched, StatusMatch, StatusUnmatched, commitDockerfileHash,
commitMakefileHash, commitCherryPickHash and commitOriginalHash to locate and
correct the assertions.
Comment on lines
+483
to
+490
| path := fr.bpPath | ||
| if fr.mainPath != "" && fr.relation == "RENAMED" { | ||
| path = fmt.Sprintf("%s -> %s", fr.mainPath, fr.bpPath) | ||
| } | ||
|
|
||
| bpCount := len(fr.onlyBP) | ||
| mainCount := len(fr.onlyMain) | ||
|
|
There was a problem hiding this comment.
Preserve file path for main-only unmatched entries.
label() falls back to bpPath only. For main-only unpaired files, bpPath is empty, so report lines can lose the filename.
Proposed fix
func (fr fileResult) label() string {
path := fr.bpPath
+ if path == "" {
+ path = fr.mainPath
+ }
if fr.mainPath != "" && fr.relation == "RENAMED" {
path = fmt.Sprintf("%s -> %s", fr.mainPath, fr.bpPath)
}Also applies to: 690-696
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tools/verify-backport/verify-backport.go` around lines 483 - 490, The report
path selection currently sets path := fr.bpPath and only handles RENAMED by
combining fr.mainPath -> fr.bpPath, which causes main-only entries (where
fr.bpPath == "") to lose the filename; update the logic in the block using
fr.bpPath/fr.mainPath/fr.relation (the variable named path near
bpCount/mainCount) to prefer a non-empty fr.bpPath but fall back to fr.mainPath
when bpPath is empty, and still format RENAMED as "main -> bp" only when both
are present; apply the same fix to the analogous code around the second
occurrence that the reviewer noted.
Comment on lines
+559
to
+570
| bpByBase := make(map[string]filePatch) | ||
| for _, fp := range bpFiles { | ||
| bpByPath[fp.path] = fp | ||
| bpByBase[filepath.Base(fp.path)] = fp | ||
| } | ||
|
|
||
| mainByPath := make(map[string]filePatch) | ||
| mainByBase := make(map[string]filePatch) | ||
| for _, fp := range mainFiles { | ||
| mainByPath[fp.path] = fp | ||
| mainByBase[filepath.Base(fp.path)] = fp | ||
| } |
There was a problem hiding this comment.
Avoid basename-collision overwrite during rename detection.
Using map[string]filePatch by basename drops all but one candidate per basename. In commits with repeated basenames across dirs, rename pairing becomes nondeterministic and can misclassify files.
Refactor direction
- bpByBase := make(map[string]filePatch)
+ bpByBase := make(map[string][]filePatch)
...
- bpByBase[filepath.Base(fp.path)] = fp
+ base := filepath.Base(fp.path)
+ bpByBase[base] = append(bpByBase[base], fp)
- mainByBase := make(map[string]filePatch)
+ mainByBase := make(map[string][]filePatch)
...
- mainByBase[filepath.Base(fp.path)] = fp
+ base := filepath.Base(fp.path)
+ mainByBase[base] = append(mainByBase[base], fp)Then resolve basename matches from candidate slices (prefer highest overlap / exact line-set match) instead of single-value lookup.
Also applies to: 594-599
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tools/verify-backport/verify-backport.go` around lines 559 - 570, The code
currently builds bpByBase and mainByBase as map[string]filePatch which causes
basename collisions to overwrite candidates; change these to
map[string][]filePatch (populate from bpFiles and mainFiles) so all same
basenames are preserved, then in the rename-detection logic (referencing
bpByBase, mainByBase and filePatch) resolve basename candidate lists by
computing and preferring the best match (e.g., exact line-set equality or
highest overlap) instead of a single-value lookup; apply the same change to the
other similar maps created at the second location (the block around the other
bpByBase/mainByBase usage).
Add an (AI-assisted) tool to help users verify a backport. The tool scans all the commits in a backport branch, summarize the changes, flags the language-agnostic mechanical changes (e.g. file rename) and code movements, and helps prioritize the review effort. The tool intentionally builds on `git` tooling, acting an orchestrator and custom-built porcelain layer. The tool includes a LLM-optimized JSON output. This can help LLM-driven review in the folling ways: 1. Reducing orchestration (single command vs multi-step git pipelines) 2. Returning pre-correlated commit relationships 3. Providing stable machine-parseable output Token savings are possible but scenario-dependent and are not guaranteed for every run; benchmarking and measuring over time and multiple usages are the only way to know for sure. See the included README.md for the full details. AI-Attribution: AIA PAI Nc Hin R claude-4.6-opus-1M v1.0 Signed-off-by: Francesco Romani <fromani@redhat.com>
ffromani
force-pushed
the
tool-verify-backport
branch
from
582cd31 to
ee0f822
Compare
Member
Author
|
/test ci-e2e-install-hypershift |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add an (AI-assisted) tool to help users verify a backport.
The tool scans all the commits in a backport branch,
summarize the changes, flags the language-agnostic mechanical changes
(e.g. file rename) and code movements, and helps prioritize
the review effort.
The tool intentionally builds on
gittooling, actingan orchestrator and custom-built porcelain layer.
The tool includes a LLM-optimized JSON output.
This can help LLM-driven review in the folling ways:
Token savings are possible but scenario-dependent and are not guaranteed
for every run; benchmarking and measuring over time and multiple usages
are the only way to know for sure.
See the included README.md for the full details.