Repository navigation
fix: report extraction failures the output parsing does not recognise - #549
liuwenhuan wants to merge 2 commits into
Conversation
Reviewer's GuideAPIDefine the reset and exit-code mapping hooks that CLI plugins and job callers use to clear stale results and classify unrecognised extraction failures.
Uses of the APIReset reused interfaces once when a job starts, preserving errors discovered later within that job.
ImplementationImplement result resetting and convert unrecognised normal extraction exits of 2 or higher into failures with a plugin-provided error type.
Classify unrar exit codes and recognise localized 7-Zip disk-full output, recording insufficient disk space when detected.
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Codex Review Summary: ✅ CompletedThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: liuwenhuan The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Hi @liuwenhuan. Thanks for your PR. I'm waiting for a linuxdeepin member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="3rdparty/interface/archiveinterface/cliinterface.cpp" line_range="763-764" />
<code_context>
Q_ASSERT(!m_process);
+ // Start each process without the error type of the previous operation on this (reused) interface
+ m_eErrorType = ET_NoError;
+
QString programPath = QStandardPaths::findExecutable(programName);
if (programPath.isEmpty()) {
</code_context>
<issue_to_address>
**Later extractions remain failed**
When the same interface runs a successful extraction after an earlier extraction set `m_finishType` to `PFT_Error`, `runProcess()` resets `m_eErrorType` but leaves `m_finishType` unchanged, so `extractProcessFinished()` preserves the earlier failure and reports the successful extraction as failed.
Reset `m_finishType` to its normal state when starting a new operation.
Also at `3rdparty/interface/archiveinterface/cliinterface.cpp:1515`.
</issue_to_address>![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7841c09b70
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // checksum error and 5 when it cannot write, 7z exits 2 after any fatal error. Exit code 1 is | ||
| // "warnings" for both (the files were extracted), so it stays a success. | ||
| if (QProcess::NormalExit == exitStatus && exitCode >= 2 && PFT_Nomral == m_finishType) { | ||
| m_finishType = PFT_Error; |
There was a problem hiding this comment.
After this fallback marks an unrecognized extraction failure as PFT_Error, a subsequent successful extraction on the same open archive is also reported as failed. ArchiveManager::extractFiles() retains the interface, runProcess() clears only m_eErrorType, and the exit-code-0 branch in extractProcessFinished() refuses to overwrite PFT_Error. Consequently, even after resolving the original problem, extraction emits an error with ET_NoError until the archive is reopened. Reset m_finishType before starting each new operation while preserving errors detected within that operation.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed, fixed by the same change: the finish type and the error type are both reset when a job starts (resetResult(), called from SingleJob::start() and UpdateJob::start()), not in runProcess().
| Q_ASSERT(!m_process); | ||
|
|
||
| // Start each process without the error type of the previous operation on this (reused) interface | ||
| m_eErrorType = ET_NoError; |
There was a problem hiding this comment.
For archives whose long filenames are truncated by handleLongNameExtract(), extractFiles() sets ET_LongNameError and immediately calls list() to refresh the archive. That internal listing reaches runProcess(), so this unconditional reset erases the current extraction's result rather than an error from a previous operation. MainWindow::showSuccessInfo() consequently no longer displays the warning that filenames were truncated, leaving renamed output reported as an ordinary successful extraction. Clear error state at a new operation boundary or preserve the long-name result across this refresh.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed. The reset moved from runProcess() to the start of the job, so the list() that extractFiles() runs after handleLongNameExtract() (same job) keeps ET_LongNameError.
|
I have read the CLA Document and I hereby sign the CLA. |
1. ArchiveManager reuses the plugin interface for later operations on the same archive. The finish type and the error type of a failed operation stayed on the interface, so the next operation could end with the old error: after a wrong password, extracting again with the right password reported "The archive is damaged". 2. ReadOnlyArchiveInterface::resetResult() clears the error type; CliInterface also clears the finish type. SingleJob::start() and UpdateJob::start() call it, once per job, so an error found earlier in the same job (for example ET_LongNameError before the long-name extraction refreshes the list) is kept. Log: Forget the result of the previous operation when a new one starts. Influence: 1. Extract an encrypted archive with a wrong password, then with the right one: the second extraction succeeds. 2. Extracting a file with a long name still reports the long-name case. fix: 任务开始时重置复用插件的结果 1. ArchiveManager 对同一压缩包的后续操作复用插件对象。失败操作的结束类型 和错误类型留在插件对象上，下一次操作可能带着旧错误结束：输错密码后再用 正确密码解压，提示"压缩包已损坏"。 2. ReadOnlyArchiveInterface::resetResult() 清除错误类型，CliInterface 还 清除结束类型。SingleJob::start() 和 UpdateJob::start() 每个任务调用一 次，因此同一任务中较早发现的错误（如长文件名解压刷新列表前设置的 ET_LongNameError）得以保留。 Log: 新操作开始时清除上一次操作的结果。 Influence: 1. 加密压缩包先输错密码再输对密码解压：第二次解压成功。 2. 解压长文件名文件仍按长文件名情况处理。
1. An extraction with unrar or 7z ended as a success whenever the tool's output had no message the plugin knows, whatever the exit status. A RAR with a checksum error (unrar exits 3) ended on "Extraction successful". 7z on a full disk (exits 2) prints "errno=28 : <strerror in the user's language>", which the plugin only knew in English; "Extract here" ended as a success, closed the window and left the partial files. 2. CliInterface::extractProcessFinished(): a normal exit with code 2 or more and no recognised error is a failure. The plugin names the error with errorTypeForExitCode() (unrar: 5 and 9 cannot write, 11 wrong password, otherwise the archive is damaged). Exit code 1 is "warnings" for both tools and the files were extracted, so it stays a success. 3. Cli7zPlugin recognises "errno=28" (ENOSPC) as a full disk and records the error. Log: Report damaged archives and a full disk instead of a successful extraction. Influence: 1. Extract a RAR with a corrupted volume: "The archive is damaged". 2. Extract a 7z, damaged: "The archive is damaged"; onto a full disk: "Insufficient disk space". 3. Normal RAR/7z extraction still succeeds. fix: 报告输出解析未识别的解压失败 1. unrar、7z 解压时，只要输出里没有插件认识的提示，不论退出码如何都按 成功处理。校验和错误的 RAR（unrar 退出码 3）显示"解压成功"。7z 磁盘 写满（退出码 2）输出"errno=28 : <本地语言的错误描述>"，插件只认英文 描述；"解压到当前文件夹"按成功处理，关闭窗口并留下不完整的文件。 2. CliInterface::extractProcessFinished()：正常退出、退出码 >= 2 且没有 识别出错误时按失败处理，错误类型由插件的 errorTypeForExitCode() 给出 （unrar：5、9 为写入失败，11 为密码错误，其余为压缩包损坏）。两个工具 的退出码 1 都表示警告，文件已解出，仍按成功处理。 3. Cli7zPlugin 把 "errno=28"（ENOSPC）识别为磁盘已满并记录错误。 Log: 压缩包损坏、磁盘已满时报告失败，而不是显示解压成功。 Influence: 1. 解压分卷损坏的 RAR：提示压缩包已损坏。 2. 解压损坏的 7z：提示压缩包已损坏；解压到已满的磁盘：提示磁盘空间不足。 3. 正常的 RAR/7z 解压仍然成功。
7841c09 to
84fb665
Compare
An extraction with unrar or 7z ends as a success whenever the tool's output has no message the plugin knows, whatever the exit status.
CliInterface::extractProcessFinished()only sets the finish type when the exit code is 0, so a non-zero exit keeps the initialPFT_Nomral.Reproduced (deepin-compressor 6.5.32.3, unrar 7.1.8, 7zip 25.01):
pack/c.bin - checksum error)errno=28 : 设备上没有空间)This PR:
extractProcessFinished(), treats a normal exit with code 2 or more and no recognised error as a failure. The plugin names the error with a new virtual,errorTypeForExitCode():ET_ArchiveDamaged.ET_FileWriteError, 11 givesET_WrongPassword.resetResult(), fromSingleJob::start()andUpdateJob::start()). The interface is reused for later operations on the same archive, so without it a successful extraction after a failed one still ended as a failure. Resetting per job rather than per process keepsET_LongNameError, which the long-name extraction sets before it refreshes the list.Cli7zPlugin::isDiskFullMsg()also matcherrno=28. 7-Zip printsstrerror()in the user's language, so "No space left on device" only matched in English. The disk-full branch now recordsET_InsufficientDiskSpace.Compiled on master (6.5.40), together with #547. Tested in a VM on 6.5.32.3 with this change backported; after the review, with the revised change, also a damaged
-hpRAR ("The archive is damaged") and a 7z entry with a 300-byte name (success, name shortened notice kept).unrar、7z 解压时，只要输出里没有插件认识的提示，就不论退出码都按成功处理。实测：分卷损坏的 RAR 显示"解压成功"；7z 解压到已满的磁盘时，中文环境下
errno=28的提示认不出来，"解压到当前文件夹"直接按成功关闭窗口并留下不完整的文件。本 PR 把退出码 >= 2 且未识别出错误的情况按失败处理（退出码 1 是警告，仍算成功），并识别errno=28。Summary by Sourcery
Ensure command-line archive extraction failures are reported reliably, including errors not identified by output parsing.
Bug Fixes:
Enhancements: