ciq_tag: lib and command line tool to operate on CIQ commit tags#48
ciq_tag: lib and command line tool to operate on CIQ commit tags#48pvts-mat wants to merge 3 commits intoctrliq:mainlinefrom
Conversation
|
No docs in the code - will be added if the tool finds acceptance. Same for the usage examples from the library POV |
pvts-mat
force-pushed
the
commit-msg-lib
branch
from
034fc8d to
cc1a2f0
Compare
|
I am not sure how often I would use this tool, except for maybe some mistakes like modifying the upstream-diff comment later. The rest of the tags should be automated. But, I think the library would be very useful to improve the current tooling. Can you separate the library from the CLI tool to make things easier? |
Add this as dependency in pyproject.toml. As explained here. I realized there's no readme with this, will add this soon. And solve the formatting error in the pr_check. |
roxanan1996
left a comment
roxanan1996
left a comment
There was a problem hiding this comment.
Will continue reviewing and testing tomorrow, but first the formatting has to be addressed.
ciq_tag.py
Outdated
| args = read_args() | ||
| for c in Parameter: | ||
| logging.debug(f"{c}: {c.get_val(args)}") | ||
| return CommandRoot.get_by_cmd(args.CommandRoot).process(args) |
There was a problem hiding this comment.
Any reason why this was implemented from scratch? Why not use argparse or even better click (https://click.palletsprojects.com/en/stable/)
I want to change all argparse usages to click, but haven't had the time.
There was a problem hiding this comment.
It is argparse. I didn't implement anything from scratch that wasn't implemented by argparse already. What I implemented was a translation from flexible data structures to inflexible argparse procedure calls.
With regards to click I didn't know about this tool. Maybe it provides what I needed, maybe not. I would have to evaluate it.
ciq_tag.py
Outdated
|
|
||
| DEFAULT_LOGLEVEL = "INFO" | ||
|
|
||
| LOGLEVEL = os.environ.get( |
There was a problem hiding this comment.
I would use this as a param to the tool (the logging level)
There was a problem hiding this comment.
This would limit the log setting ability to the CLI tool only. What abut python modules which would be including this one?
Some switches might be provided, but the modules would have to be aware of them and consciously propagate end-user's logging level wish down to the library.
Environment variables cross the application-library barrier effortlessly, so the module's users don't have to deal with these settings. For example, a ciq_tag_user.py program may look like
#!/usr/bin/env python3
import ciq_tag
print(ciq_tag.add_tag("""
sunrpc: handle SVC_GARBAGE during svc auth processing as auth error
tianshuo han reported a remotely-triggerable crash if the client sends a
kernel RPC server a specially crafted packet. If decoding the RPC reply
fails in such a way that SVC_GARBAGE is returned without setting the
rq_accept_statp pointer, then that pointer can be dereferenced and a
value stored there.
""", ciq_tag.CiqTag.CVE, "CVE-2025-38089"))
and calling it as
LOGS=DEBUG ./ciq_tag_user.py
would show debug messages from the ciq_tag library without engaging ciq_tag_user.py in it.
If the programs using ciq_tag wished they may "plug" themselves into this LOGS variable logic as well, of course, so they recognize the logging level wanted by the end user in the same way. It is a form of very simple global logging setup, generally advised for python logging.
Having said that, the logging setup I did was really poor and lazy. Rewrote it.
ciq_tag.py
Outdated
| return (True, (message[:deleted_tag_pos.keyword_start] + | ||
| omit_prefixing_empty_lines(message[deleted_tag_pos.boundary_end:]))) | ||
| else: | ||
| return (False, message) |
There was a problem hiding this comment.
I haven't checked this properly, but my personal preference is to avoid multiple indents and do early returns if possible. What do you think?
There was a problem hiding this comment.
I guess we both want the code to look good. My criteria for code aesthetic is based on how well it exposes the behavioral complexity of an algorithm. Nested if/else statements make the task of answering key questions like "what are the conditions for function f to return value y?" as simple as taking the conjunction of conditions up the branching hierarchy from the picked exit point, while the existence of early returns necessitate mental code execution, which is prone to errors, takes more time and unnecessary effort.
This particular code also doesn't deal with eliminating error cases which the early returns seem to be most suited for.
pvts-mat
force-pushed
the
commit-msg-lib
branch
from
cc1a2f0 to
54142e6
Compare
|
Thanks for looking into it @roxanan1996. Let me address the comments
The library was the main goal. I wanted to create an abstraction layer over CIQ meta-data operations, hoping to reduce code duplication, wheel reinvention and the associated mental load. The development of the command line tool wasn't driven by some expected use cases, but rather guided by the principle of computational freedom. The aim was to transfer 1-to-1 the functionality of the library to the computation environment most natural and default for many tasks - the shell. I try not to mold the developed tools into imagined usecases because my imagination is limited and I do not wish to impose it on others. Regarding the typical commit backporting workflow I don't think the tool is much useful, but it does open the door to automation of many kinds, which may be very useful in larger PRs like the recent Additionaly the CLI frontend is very useful in testing the library, so I would have written it anyway.
You mean the "things" here are the decision about what to keep and what to reject? Because if it's about usage then these two do not conflict in any way, you can |
Done. |
Yeah, completely agree, that's what I meant. It will help improving the existing tooling a lot. As for the separation, I think it's usually best practice to separate the lib and the tool, if we need to import the lib in another tool we have. I am 100% sure the lib would be used further, so it's just easier to do separate them now then in the future. |
|
I am a bit busy today. but I'll continue looking at this tomorrow. |
- Rewrote the tags parsing logic. The previous one was based on the on-demand, separate, atomic message parsing for each tag operation. It was kinda raw and liberal, avoiding building additional data structures. The goal was to keep it flexible, but it turned out to be too flexible, recognizing parts of upstream message as CIQ tags, like in the case of the commit 081056dc00a27bccb55ccc3c6f230a3d5fd3f7e0. In the new version the CIQ tags are required to consitute a continuous block at the very beginning of git message. The message is parsed in whole, all tags tracked no matter what operation on which of them is requested. An explicit list of tags for the given commit message is build, which allows for simplification of a lot of editing code. From the API perspective there is no longer a loose bag of functions but a CiqMsg class encapsulating the operations as modifier methods, with the `get_message()' getter method to obtain the end result of all carried out operations (adding/editing/deleting tags). The command line interface remained exactly the same. - Extracted the CLI to a separate file ciq_tag.py: library ciq-tag.py: executable CLI - Rewrote arguments parsing from argpars to click
pvts-mat
force-pushed
the
commit-msg-lib
branch
from
bea0b3b to
a269fd3
Compare
|
roxanan1996
left a comment
roxanan1996
left a comment
There was a problem hiding this comment.
hi. Sorry it took so long.
I left some very minor comments.
Overall it looks ok to me and it would be useful to add it.
People can iterate over it once it gets used.
So feel free to push it.
ciq_tag.py
Outdated
| UPSTREAM_DIFF = (["upstream-diff"], True) | ||
|
|
||
| def __init__(self, keywords: List[str], multiline: bool = False): | ||
| assert len(keywords) > 0 |
There was a problem hiding this comment.
I should return an Exception here or sth, I use assert only for tests.
There was a problem hiding this comment.
If this particular condition is false it means the enum entry was defined wrong, so this assert is more of a buggy code canary than an error path of a regular program execution. Switched the asserts to exceptions though, if you prefer.
While we are at it - how much arguments checking do you want for the public functions? Currently there is almost none. All of them? Only the least obvious? Do you want the types checked too or just values?
|
|
||
| JIRA = ["jira"] | ||
| SUBSYSTEM_SYNC = ["subsystem-sync"] | ||
| SUBSYSTEM_UPDATE = ["subsystem-update"] |
There was a problem hiding this comment.
Did you check if all of these are present? I am not familiar with all of them.
Also, since this is mostly for Cves, maybe start small and use the tags only relevant only for cve backports in lts.
There was a problem hiding this comment.
Yes, all of these tags were found in the wild, I didn't introduce to CiqTag any new tag upfront. Examples:
subsystem-sync: ctrliq/kernel-src-tree@57100dcsubsystem-update: ctrliq/kernel-src-tree@be19508
Part of the intent behind this lib was to systematize and catalogue the CIQ tags existing in the repos, so I included everything I could find. It's a good place to document them, for example, like what do they mean, when to use which, etc.
Another thing is that lack of completeness undermines the purpose of the library, which is simplifying programmer's work. If some tags are supported and some not then it puts on the user the burden of keeping the mental track of which tags the library can be used for and for which it cannot. This can be thought of as part of lib's implicit interface, which is simplified by completing the list of supported tags.
There is also a technical issue of the library needing to know about the lesser used tags to properly parse also the more common ones. In the example commit above, if subsystem-sync was not recognized it would signify the beginning of the message and the following commit line would be understood as part of the message as well. On the other hand if the tags were allowed to be all over the commit message and not in a single block then we would occasionally have a problem with messages like this one ctrliq/kernel-src-tree@081056d where there is an accidental line starting with commit word but it's not a CIQ tag. And if the block of consecutive tags wasn't recognized by explicitly identifying them, but by some typographic features like "forming one paragraph", then we would have troubles with messages without any tags (yet) - they would also have some paragraph in the beginning, although not of CIQ tags, which again could have some accidental false positives.
|
|
||
|
|
||
| def process_in_out(input, output, result_to_output_map, ciq_msg_method, *method_args_pos, **method_args_key): | ||
| with open_input(input) as in_file: |
There was a problem hiding this comment.
nitpick, input does not have to stay open after input_str is read.
|
2 participants
About
A library and a command line tool to get, insert, modify and delete CIQ tags from commit messages.
Features:
Pure python 3
Minimal dependencies
(can be avoided if required)
Tools-indepentent
gitunderneath (or any other external commands for that matter).Robust
cve-bf,cve-bugfix,cve-update, …):)upstream-diffregardless of their formatting and indentation.Handling the formatting
Full-featured
upstream-diff)jiratags).Extendable.
By encapsulating the tags editing operations this tool allows for automating those revision history maintenance tasks which previously had to be done by hand or with the use of ad-hoc
grepsandseds..Examples
Adding tags
Input
Output
Automatic tracking of the proper tags order
(
jiratag should be beforecve)Adding multiple instances of the same tag if necessary
Modifying existing tags
Input
Output
Return nonzero exit code if tag not found
(but produce the output unchanged)
Use
setto insert anyway if not existThis differs from
addwhich would always insert the tag, whilesetfirst attempts to modify the existing one and only if that didn't succeed - to insert the new instance.Deleting tags
Input
Basic usage
Handling the multi-line properties well
Clean all metadata
Shring the vertical separation if needed
Getting the values of tags
Input
Basic usage
Return error if tag not found
Extract the raw
upstream-diffAvoid the indentation
Unwrap the text to abstract from formatting where it get in the way
Handle also other multiline properties
Input:
Output:
Autoformatting of values
Input
Set
upstream-diffright from the command lineSet indentation to a fixed value
Set indent to align with the keyword
Control the width