ciq-cherry-pick.py: Fix two commit message defects - #83
Merged
Conversation
The trailer-indent loop in CIQ_cherry_pick_commit_standardization() is meant to stop indenting once it hits the marker line that git cherry-pick -x writes, but it tests for a line starting with "cherry picked from commit" while git actually writes "(cherry picked from commit <sha>)". The leading parenthesis means the break never fires, so the loop runs past the marker and tab-indents the backporter's own Signed-off-by that cherry-pick -s appends below it. As a result, every backport made through this helper ends up with its trailing Signed-off-by indented. Fix it by matching the line git actually writes. Trailers above the marker still get their tab, while the marker and everything below it are now left untouched. Add a test covering both sides of the marker. Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
When a cherry-pick hits a conflict, git appends a comment block to
MERGE_MSG ("# Conflicts:" followed by the conflicted paths).
manage_commit_message() reads MERGE_MSG, inserts the CIQ header block,
and writes the whole thing back, conflict comments included. The script
then exits telling the user to resolve the conflict and commit, and
committing with -F keeps those lines, since -F's default cleanup mode
only strips whitespace, not comments. The conflict block winds up in the
final commit message verbatim.
Fix it by dropping the tail of the message starting at the
"# Conflicts:" line before standardizing. Only the trailing
git-generated block goes; legitimate body lines starting with "#"
earlier in the message survive.
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR fixes two defects in the standardized commit message handling used by ciq-cherry-pick.py, ensuring backport trailers are formatted correctly and that git’s conflict-comment block does not leak into the final commit message.
Changes:
- Fix trailer indentation logic to stop indenting once the actual git cherry-pick marker line (
(cherry picked from commit …)) is reached. - Strip the
# Conflicts:comment block fromMERGE_MSGduring commit message management to prevent it from being committed viagit commit -F. - Add pytest coverage validating that upstream trailers are indented while the marker and backporter’s
Signed-off-byremain unindented.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
kt/ktlib/ciq_helpers.py |
Corrects the marker detection so the indentation loop stops at git’s real cherry-pick marker format. |
ciq-cherry-pick.py |
Drops the # Conflicts: tail block from MERGE_MSG to avoid committing conflict comments when using -F. |
tests/kt/ktlib/test_ciq_helpers.py |
Adds tests asserting upstream trailer indentation and that indenting stops at the cherry-pick marker. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
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
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.
Both fixes target the standardized commit message that
ciq-cherry-pick.pywrites to MERGE_MSG. I noticed the first defect while reviewing the gve backport series on ctrliq/kernel-src-tree PRs 1496, 1498, and 1523: every backport in the series has its trailing Signed-off-by tab-indented (e.g., d5596b7488851, b862126f2c9ce, 2d48036cc4997) when it should sit at column 0.The trailer-indent loop never stops at the cherry-pick marker. The loop in
CIQ_cherry_pick_commit_standardization()is meant to quit indenting once it reaches the marker line, but it tests for a line starting withcherry picked from commitwhile git writes(cherry picked from commit <sha>). The leading parenthesis means the break never fires, so the loop runs past the marker and indents the backporter's own Signed-off-by, whichcherry-pick -sappends below the marker. Fixed by matching the line git actually writes.git's conflict comment block leaks into the final message. On a conflicted pick, git appends
# Conflicts:plus#<TAB><path>comment lines to MERGE_MSG.manage_commit_message()reads MERGE_MSG, inserts the CIQ header, and writes the whole thing back, conflict comments included. The script then exits telling the user to resolve and commit, and committing withgit commit -F .git/MERGE_MSGkeeps those lines, since -F's default cleanup mode only strips whitespace, not comments. I reproduced this on a live conflicted pick: the# Conflicts:block landed in the final commit message verbatim. Fixed by dropping the tail of the message starting at the line that is exactly# Conflicts:. The fix deliberately doesn't filter every#line, since legitimate upstream commit bodies can contain them.The first fix comes with tests in
tests/kt/ktlib/test_ciq_helpers.py, following the existing pytest layout.manage_commit_message()isn't importable from a test (the module buildsgit.Repo(os.getcwd())and importsjiraat load time), so I verified it by exec'ing the function in isolation against a synthetic conflicted MERGE_MSG, alongside a negative control showing the old marker check misbehaving:Generated with Claude Code
Coverage Report