-
Notifications
You must be signed in to change notification settings - Fork 1
ciq-cherry-pick.sh: Some improvements #45
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: mainline
Are you sure you want to change the base?
Conversation
ciq_helpers.py
Outdated
| """ | ||
|
|
||
| # First check if the commit has been backported by CIQ | ||
| output = CIQ_run_git(repo, ["log", pr_branch, "--grep", "commit " + upstream_hash_]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hopefully to deal with false positives we should use "^commit " ... since --grep on log will search the whole thing
Example
commit 00832a61791990b69edb7c498962e6b69ea68d3e (HEAD -> ciqlts9_2, origin/ciqlts9_2)
Author: Shreeya Patel <spatel@ciq.com>
Date: Wed Nov 12 13:12:33 2025 +0000
crypto: xts - Handle EBUSY correctly
jira VULN-157046
cve CVE-2023-53494
commit-author Herbert Xu <herbert@gondor.apana.org.au>
commit 51c082514c2dedf2711c99d93c196cc4eedceb40
As it is xts only handles the special return value of EINPROGRESS,
which means that in all other cases it will free data related to the
request.
However, as the caller of xts may specify MAY_BACKLOG, we also need
to expect EBUSY and treat it in the same way. Otherwise backlogged
requests will trigger a use-after-free.
Fixes: 8083b1bf8163 ("crypto: xts - add support for ciphertext stealing")
This grep pattern finds 1372a51b88fa0d5a8ed2803e4975c98da3f08463 which is not a CIQ commit
commit 1372a51b88fa0d5a8ed2803e4975c98da3f08463
Author: Daniel Axtens <dja@axtens.net>
Date: Wed Jan 8 16:06:46 2020 +1100
crypto: vmx - reject xts inputs that are too short
When the kernel XTS implementation was extended to deal with ciphertext
stealing in commit 8083b1bf8163 ("crypto: xts - add support for ciphertext
stealing"), a check was added to reject inputs that were too short.
However, in the vmx enablement - commit 239668419349 ("crypto: vmx/xts -
use fallback for ciphertext stealing"), that check wasn't added to the
vmx implementation. This disparity leads to errors like the following:
whcih is from upsteam
[jmaple@devbox kernel-src-tree-build]$ git describe --contains 1372a51b88fa0d5a8ed2803e4975c98da3f08463
v5.6-rc1~152^2~42
Using the carrot at the start for start of line
[jmaple@devbox kernel-src-tree-build]$ git log --grep "^commit 8083b1bf8163"
[jmaple@devbox kernel-src-tree-build]$
This example isn't perfect (as I didn't want to spend forever on finding the perfect exacmple) because for Rocky9 and Rocky10 we're actually forked off of Centos-Streams which has its own evolving header like we do, but they indent the body of the original message.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I'll add it. Just a note, the commit is the full hash so your example doesn't hold and usually people use the short hash in the commit message, but still, I agree I should use the start of the line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed!
|
|
||
|
|
||
| def CIQ_get_current_branch(repo): | ||
| return CIQ_run_git(repo, ["branch", "--show-current"]).strip() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no try catch here, in the even that the call out fails for what ever magical reason.
ciq-cherry-pick.py
Outdated
|
|
||
| for fix in fixes: | ||
| if not CIQ_commit_exists_in_current_branch(os.getcwd(), fix): | ||
| raise RuntimeError(f"The commit you want to cherry pick references a Fixes {fix}: but this is not here") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please, include an override switch, by default we should abort like this but we should also let it do all the annoying work for us and correct failures as we see fit.
In addition since we're in a loop we should check all of them to see if there is one or the other, a cherry-pick for a CVE in our kernel may only need 1 of the fixes and we'll have to upstream-diff as the other fix that does not exist in our kernel is not relevant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please, include an override switch, by default we should abort like this but we should also let it do all the annoying work for us and correct failures as we see fit.
You mean to add an cli option to override this? I do not see why we need this at the moment. If the Fixes commit is not there, it means this commit should not be cherry picked, right? Is there a reason why we should cherry pick it?
In addition since we're in a loop we should check all of them to see if there is one or the other, a cherry-pick for a CVE in our kernel may only need 1 of the fixes and we'll have to upstream-diff as the other fix that does not exist in our kernel is not relevant.
I see. I haven't thought of the case where we are interested in one of the fixes only. Will fix it, but first I want to see if commits have multiple fixes upstream, just curious.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Its not Super common but I've run into it before, here is the first example I've run into
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, you're right. I haven't thought we may be interested in only one. Great suggestion.
I checked too and there are multiple likes this upstream.
You shouldn't need the Yeah, I meant more that this is used underneath. I'll rephrase it. Btw, this was used in the code and I kept it like this. |
|
How was it tested? Can you provide an example PR that has all this? |
|
b72ea01: "Cherry pick commit only if the Fixes: references are commited" Typo in commit message:
|
d3e9fd7 to
5f7212c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR enhances the cherry-pick workflow by adding automated checks for commit dependencies, automatic cherry-picking of fix commits, and improved conflict handling. The changes consolidate duplicated code between ciq-cherry-pick.sh and check_kernel_commits.sh by extracting common functionality into shared helper functions in ciq_helpers.py.
Key Changes:
- Validates that commits referenced in "Fixes:" tags exist in the current branch before cherry-picking
- Automatically cherry-picks additional commits that fix the target commit (tagged as "cve-bf")
- Adds "upstream-diff" marker to merge messages when conflicts occur to guide manual resolution
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 18 comments.
| File | Description |
|---|---|
| ciq_helpers.py | Adds shared helper functions for git operations, commit validation, and finding fix dependencies to eliminate code duplication |
| ciq-cherry-pick.py | Refactored to use shared helpers; adds check_fixes validation, automatic cherry-picking of fix commits, and improved commit message management |
| check_kernel_commits.py | Updated to use shared helper functions from ciq_helpers.py instead of local implementations |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
5f7212c to
cc49be4
Compare
My last pull requests used this. ctrliq/kernel-src-tree#703 |
cc49be4 to
8c2e04d
Compare
Fixed. |
8c2e04d to
62922b6
Compare
62922b6 to
805df64
Compare
ede745f to
0e43986
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 10 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
0e43986 to
d7ff907
Compare
d7ff907 to
6562c0f
Compare
6562c0f to
6850007
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 8 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…are committed If the commit that needs to be cherry picked has "Fixes:" references in the commit body, there is now a check in place that verifies if those commits are present in the current branch. At the moment, the script returns an Exception because the developer must check why the commit has to be cherry picked for a bug fix or cve fix if the actual commit that introduced the bug/cve was not commited. If the commit does not reference any Fixes:, an warning is shown to make the developer aware that they have to double check if it makes sense to cherry pick this commit. The script continues as this can be reviewed after. This is common in the linux kernel community. Not all fixes have a Fixes: reference. Checking if a commit is part of the branch has now improved. It checks if either the commit was backported by our team, or if the commit came from upstream. Note: The implementation reuses some of the logic in the check_kernel_commits.py. Those have been moved to ciq_helper.py. This commit address the small refactor in check_kernel_commits.py as well. Signed-off-by: Roxana Nicolescu <rnicolescu@ciq.com>
It now automatically cherry picks the Fixes: dependencies. To accomodate this, CIQ_find_mainline_fixes was moved to ciq_helpers. And an extra argument for upstream-ref was introduced, the default being origin/kernel-mainline, as the dependencies are looked up there. To simplify things and keep main cleaner, separate functions were used. If one of the commits (the original and its dependencies) cannot be applied, the return code will be 1. This is useful when ciq-cherry-pick.py is called from other script. This also removed redundant prints. Signed-off-by: Roxana Nicolescu <rnicolescu@ciq.com>
…f conflict If the conflict is solved manually, then developer use ``` git commit ``` which will use the MERGE_MSG file for the commit message by default. Equivalent of ``` git commit -F MERGE_MSG ``` Therefore it makes sense to include "upstream-diff |" in the MERGE_MSG. In case the conflict is solved by cherry picking other commits, ciq-cherry-pick.py will be called again and MERGE_MSG will be rewritten, including the "upstream-diff" part. Signed-off-by: Roxana Nicolescu <rnicolescu@ciq.com>
6850007 to
f4a2b42
Compare
Signed-off-by: Roxana Nicolescu <rnicolescu@ciq.com>
Signed-off-by: Roxana Nicolescu <rnicolescu@ciq.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 11 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| def cherry_pick_fixes(sha, ciq_tags, jira_ticket, upstream_ref, ignore_fixes_check): | ||
| """ | ||
| It checks upstream_ref for commits that have this reference: |
Copilot
AI
Nov 24, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Grammatical error in docstring: "It checks" should match the pattern established in other docstrings. Consider changing to "Checks upstream_ref for commits..." for consistency with the check_fixes function docstring.
| It checks upstream_ref for commits that have this reference: | |
| Checks upstream_ref for commits that have this reference: |
|
|
||
| def full_cherry_pick(sha, ciq_tags, jira_ticket, upstream_ref, ignore_fixes_check): | ||
| """ | ||
| It cherry picks a commit from upstream-ref along with its Fixes: references. |
Copilot
AI
Nov 24, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Grammatical error in docstring: "It cherry picks" should be "Cherry picks" for consistency with other docstring conventions in this file.
| It cherry picks a commit from upstream-ref along with its Fixes: references. | |
| Cherry picks a commit from upstream-ref along with its Fixes: references. |
ciq_helpers.py
Outdated
|
|
||
|
|
||
| def CIQ_reset_HEAD(repo): | ||
| return CIQ_run_git(repo=repo, args=["reset", " --hard", "HEAD"]) |
Copilot
AI
Nov 24, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Keyword argument 'repo' is not a supported parameter name of function CIQ_run_git.
| return CIQ_run_git(repo=repo, args=["reset", " --hard", "HEAD"]) | |
| return CIQ_run_git(repo, ["reset", "--hard", "HEAD"]) |
All exceptions are propapagated to main and handled there. Also added some comments to full_cherry_pick function to explain that if one of the cherry pick fails, the previous successful ones are left intact. This can be improved in the future by making it interactive. Signed-off-by: Roxana Nicolescu <rnicolescu@ciq.com>
f7f8c46 to
fa772f6
Compare
…s are not in the tree This can be useful in cases where there are multiple commits this one is trying to fix, and only one if of interest for us. Signed-off-by: Roxana Nicolescu <rnicolescu@ciq.com>
fa772f6 to
7a8f7f8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Each commit is separated by a NUL character and a newline | ||
| commits = output.split("\x00\x0a") |
Copilot
AI
Nov 24, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The split pattern "\x00\x0a" (NUL followed by newline) does not match the actual output format. The --format string "%H %h %s (%an)%x0a%B%x00" produces output where commit bodies (which typically end with a newline) are followed by a NUL character, then the next commit starts. This creates the sequence \n\x00, not \x00\n. The split should use split("\x00") instead, which would correctly separate commits by the NUL delimiter.
| # Each commit is separated by a NUL character and a newline | |
| commits = output.split("\x00\x0a") | |
| # Each commit is separated by a NUL character | |
| commits = output.split("\x00") |
| def cherry_pick(sha, ciq_tags, jira_ticket, ignore_fixes_check): | ||
| """ | ||
| Cherry picks a commit and it adds the ciq standardized format | ||
| In case of error (cherry pick conflict): | ||
| - MERGE_MSG.bak contains the original commit message | ||
| - MERGE_MSG contains the standardized commit message | ||
| - Conflict has to be solved manually | ||
| In case runtime errors that are not cherry pick conflicts, the cherry | ||
| pick changes are reverted. (git reset --hard HEAD) | ||
| In case of success: | ||
| - the commit is cherry picked | ||
| - MERGE_MSG.bak is deleted | ||
| - You can still see MERGE_MSG for the original message | ||
| """ |
Copilot
AI
Nov 24, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The docstring claims "MERGE_MSG.bak is deleted" on success (line 100), but there's no code that actually deletes MERGE_MSG_BAK anywhere in the function or after a successful cherry-pick. The backup file is created at line 57 but never cleaned up.
| def CIQ_find_fixes_in_mainline(repo, pr_branch, upstream_ref, hash_): | ||
| """ | ||
| Return unique commits in upstream_ref that have Fixes: <N chars of hash_> in their message, case-insensitive, | ||
| if they have not been committed in the pr_branch. | ||
| Start from 12 chars and work down to 6, but do not include duplicates if already found at a longer length. | ||
| Returns a list of tuples: (full_hash, display_string) | ||
| """ | ||
| results = [] | ||
|
|
||
| # Prepare hash prefixes from 12 down to 6 | ||
| hash_prefixes = [hash_[:index] for index in range(12, 5, -1)] | ||
|
|
||
| # Get all commits with 'Fixes:' in the message | ||
| output = CIQ_run_git( | ||
| repo, | ||
| [ | ||
| "log", | ||
| upstream_ref, | ||
| "--grep", | ||
| "Fixes:", | ||
| "-i", | ||
| "--format=%H %h %s (%an)%x0a%B%x00", | ||
| ], | ||
| ).strip() | ||
| if not output: | ||
| return [] | ||
|
|
||
| # Each commit is separated by a NUL character and a newline | ||
| commits = output.split("\x00\x0a") | ||
| for commit in commits: | ||
| if not commit.strip(): | ||
| continue | ||
|
|
||
| lines = commit.splitlines() | ||
| # The first line is the summary, the rest is the body | ||
| header = lines[0] | ||
| full_hash, display_string = (lambda h: (h[0], " ".join(h[1:])))(header.split()) | ||
| fixes = CIQ_extract_fixes_references_from_commit_body_lines(lines=lines[1:]) | ||
| for fix in fixes: | ||
| for prefix in hash_prefixes: | ||
| if fix.lower().startswith(prefix.lower()): | ||
| if not CIQ_commit_exists_in_branch(repo, pr_branch, full_hash): | ||
| results.append((full_hash, display_string)) | ||
| break | ||
|
|
||
| return results |
Copilot
AI
Nov 24, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function CIQ_find_fixes_in_mainline may return duplicate commits in the results list. When a commit has multiple "Fixes:" references that match different prefixes of hash_, it will be added to results multiple times (once per matching Fixes reference). Consider using a set or checking if full_hash is already in results before appending.
| The new standardized commit message is written to MERGE_MSG | ||
| """ | ||
|
|
Copilot
AI
Nov 24, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If git cherry-pick -nsx fails (line 115), the MERGE_MSG file may not exist, causing the cp command at line 57 to fail. This would trigger the exception handler at line 123-125 which calls CIQ_reset_HEAD, but the cp failure happens before reading MERGE_MSG. Consider checking if MERGE_MSG exists before attempting to copy it, or handle the case where cherry-pick fails to create MERGE_MSG.
| if not os.path.exists(MERGE_MSG): | |
| raise RuntimeError(f"Cannot copy commit message: {MERGE_MSG} does not exist. Cherry-pick may have failed.") |
| if not CIQ_commit_exists_in_current_branch(os.getcwd(), fix): | ||
| not_present_fixes.append(fix) | ||
|
|
||
| err = f"The commit you want to cherry pick has the following Fixes: references that are not part of the tree {not_present_fixes}" |
Copilot
AI
Nov 24, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error message is missing proper formatting. It should include a colon after "tree" and format the list more clearly. Consider: f"The commit you want to cherry pick has the following Fixes: references that are not part of the tree: {not_present_fixes}"
| err = f"The commit you want to cherry pick has the following Fixes: references that are not part of the tree {not_present_fixes}" | |
| err = ( | |
| "The commit you want to cherry pick has the following Fixes: references that are not part of the tree: " | |
| f"{', '.join(not_present_fixes)}" | |
| ) |
It adds extra fixes to the way we cherry pick commits.
check if the commit fixes another commit(s) and if so, it check if those commits were actually commited beforehand. By either a ciq backport or by rebasing from upstream.
As seen when check_kernel_commits is run, some commits require extra fixes themselves. We tag them as cve-bf.
Those are now automatically cherry-picked.
In case of conflicts, upstream-diff is added automatically to the merge commit in MERGE_MSG. If the commit is solved manually, the developer can just add their comments. If the commit is solved by cherry-picking some other commits, the old commit message in MERGE_MSG will be overridden and the developer will call the ciq-cherry-pick.sh again, so it does not matter.
Note1: The code is not in perfect state as I would use gitpython in the future and some other things. But it works well and it removed duplicates between ciq-cherry-pick.sh and check_kernel_commits.sh
Note2: In case one of the cherry picks fail, the scripts returns right away, without reverting the previous successful cherry pick (if any). This is intentional at the moment since it makes the logic more complicated. And in case we do this in a batch, it's quickly fixed by using an intermediate branch.
To avoid these intermediate states in the future, an interactive approach will be implemented, but this will be done in a latter merge request.