Skip to content

fix: Prevent overwriting error states on successful exit codes in Cli…#401

Merged
deepin-bot[bot] merged 1 commit intolinuxdeepin:masterfrom
dengzhongyuan365-dev:fix-4-30
Apr 28, 2026
Merged

fix: Prevent overwriting error states on successful exit codes in Cli…#401
deepin-bot[bot] merged 1 commit intolinuxdeepin:masterfrom
dengzhongyuan365-dev:fix-4-30

Conversation

@dengzhongyuan365-dev
Copy link
Copy Markdown
Contributor

@dengzhongyuan365-dev dengzhongyuan365-dev commented Apr 28, 2026

…Interface

  • Updated the processFinished and extractProcessFinished methods to retain error states detected from stdout, even when the exit code is 0.
  • This change ensures that errors such as long file name issues are not masked by a successful exit code, improving error handling and user feedback.

Enhances reliability in archive processing by maintaining accurate error reporting.

bug: https://pms.uniontech.com/bug-view-338535.html

Summary by Sourcery

Bug Fixes:

  • Avoid clearing previously detected extraction errors when the CLI tool returns exit code 0, ensuring issues like long file name failures are still reported.

…Interface

- Updated the processFinished and extractProcessFinished methods to retain error states detected from stdout, even when the exit code is 0.
- This change ensures that errors such as long file name issues are not masked by a successful exit code, improving error handling and user feedback.

Enhances reliability in archive processing by maintaining accurate error reporting.

bug: https://pms.uniontech.com/bug-view-338535.html
@sourcery-ai
Copy link
Copy Markdown

sourcery-ai Bot commented Apr 28, 2026

Reviewer's guide (collapsed on small PRs)

Reviewer's Guide

Adjusts CLI archive interface completion handling so that error states detected from stdout are preserved and not overwritten when the underlying CLI tool exits with code 0, improving accuracy of extraction/processing results.

Sequence diagram for updated CLI process finish handling

sequenceDiagram
    participant QProcess
    participant CliInterface
    participant StdoutParser

    QProcess->>CliInterface: readyReadStandardOutput()
    CliInterface->>StdoutParser: parseStdout(output)
    StdoutParser-->>CliInterface: detectedError
    CliInterface->>CliInterface: set m_finishType = PFT_Error

    QProcess->>CliInterface: finished(exitCode = 0, exitStatus)
    CliInterface->>CliInterface: processFinished(exitCode, exitStatus)
    alt exitCode == 0 and m_finishType != PFT_Error and m_finishType != PFT_Cancel
        CliInterface->>CliInterface: m_finishType = PFT_Nomral
    else existing error or cancel state
        CliInterface->>CliInterface: keep current m_finishType
    end
Loading

Updated class diagram for CliInterface finish type handling

classDiagram
    class CliInterface {
        - bool m_isCorruptArchive
        - FinishType m_finishType
        + void processFinished(int exitCode, QProcess_ExitStatus exitStatus)
        + void extractProcessFinished(int exitCode, QProcess_ExitStatus exitStatus)
    }

    class FinishType {
        <<enumeration>>
        PFT_Nomral
        PFT_Error
        PFT_Cancel
    }

    CliInterface --> FinishType : uses

    class QProcess_ExitStatus {
        <<enumeration>>
        NormalExit
        CrashExit
    }
Loading

File-Level Changes

Change Details Files
Preserve previously-detected error or cancel states when the CLI tool exits with code 0, instead of unconditionally marking the job as normal.
  • Updated process completion logic to only set the finish type to normal on exit code 0 if the finish type is not already error or cancel.
  • Applied the same guarded finish-type update to extraction-specific completion handling to keep stdout-derived errors when exitCode is 0.
  • Documented the rationale with comments, noting that some tools (e.g. unrar) may return exit code 0 even if individual entries failed, such as long file name issues.
3rdparty/interface/archiveinterface/cliinterface.cpp

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

@deepin-ci-robot
Copy link
Copy Markdown

deepin pr auto review

这段代码的修改主要是为了处理命令行工具(CLI)返回状态码与实际执行状态不一致的情况,特别是在处理某些归档工具(如 unrar)时,即使部分文件解压失败(例如文件名过长),工具也可能返回退出码 0(成功)。

以下是对该 diff 的详细审查意见:

1. 语法逻辑审查

现状
修改后的逻辑引入了对 m_finishType 的前置检查。

if (exitCode == 0 && m_finishType != PFT_Error && m_finishType != PFT_Cancel) {
    m_finishType = PFT_Nomral;
}

分析

  • 逻辑正确性:逻辑是合理的。如果 stdout 解析器已经检测到了错误并将 m_finishType 设置为 PFT_Error,或者用户取消了操作(PFT_Cancel),那么即使进程返回了 0,也不应该将状态重置为 PFT_Nomral。这防止了 CLI 工具的"虚假成功"覆盖了已知的错误状态。
  • 拼写错误:代码中使用了 PFT_Nomral,这在英文语境下是拼写错误,正确的拼写应为 PFT_Normal
    • 注意:如果这是项目既有的宏定义或常量名称,那么语法上没问题。但如果是新定义的,建议修正为 Normal

2. 代码质量审查

  • 可读性:添加的注释非常清晰,解释了为什么要进行这个修改(例如提到了 unrar 和长文件名的问题),这大大提高了代码的可维护性。
  • 一致性:修改在 processFinishedextractProcessFinished 两个函数中保持了一致性,这是很好的实践。

3. 代码性能审查

  • 影响:这段代码仅在进程结束时执行一次,且仅增加了两个简单的整数比较操作(!=)。对性能的影响可以忽略不计。

4. 代码安全审查

  • 健壮性:这个修改提高了程序的健壮性。它解决了"静默失败"的风险——即程序报告操作成功,但实际上部分文件未能处理。这对于归档软件至关重要,因为用户需要知道是否所有文件都已正确解压。
  • 状态管理:这种修改依赖于 m_finishType 在解析 stdout 时被正确设置。如果解析器漏掉了某些错误,CLI 返回 0 依然会被视为成功。因此,确保 stdout 解析逻辑的覆盖率是安全性的关键,但这属于代码的其他部分。

改进建议

  1. 修正拼写
    建议检查 PFT_Nomral 是否为拼写错误。如果是,应全局替换为 PFT_Normal,以符合标准的命名规范。

  2. 枚举类型检查
    建议确认 m_finishType 的类型是否为 enumenum class。如果是,使用枚举比较比整数比较更安全且语义更强。虽然现在的写法在 C++ 中也是合法的,但使用枚举值可以避免魔术数字带来的混淆。

  3. 逻辑封装(可选)
    如果这段判断逻辑在多处复用,可以考虑将其提取为一个私有辅助函数,例如:

    bool CliInterface::shouldReportSuccess(int exitCode) const {
        return exitCode == 0 && m_finishType != PFT_Error && m_finishType != PFT_Cancel;
    }

    然后在调用处写为:

    if (shouldReportSuccess(exitCode)) {
        m_finishType = PFT_Nomral; // 或 PFT_Normal
    }

    这样可以进一步减少重复代码,提高可读性。

总结

这个 diff 是一个高质量的修复,主要针对边缘情况(CLI 工具误报成功)进行了处理,显著提升了软件的可靠性。除了需要确认常量名的拼写外,没有明显的逻辑或安全隐患。

Copy link
Copy Markdown

@sourcery-ai sourcery-ai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey - I've left some high level feedback:

  • The success-handling condition (exitCode == 0 && m_finishType != PFT_Error && m_finishType != PFT_Cancel) is now duplicated in both processFinished and extractProcessFinished; consider extracting this into a small helper (e.g. setFinishedOnSuccess(exitCode)) to keep the success/error logic consistent in one place.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- The success-handling condition (`exitCode == 0 && m_finishType != PFT_Error && m_finishType != PFT_Cancel`) is now duplicated in both `processFinished` and `extractProcessFinished`; consider extracting this into a small helper (e.g. `setFinishedOnSuccess(exitCode)`) to keep the success/error logic consistent in one place.

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

@deepin-ci-robot
Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: dengzhongyuan365-dev, lzwind

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@dengzhongyuan365-dev
Copy link
Copy Markdown
Contributor Author

/merge

@deepin-bot deepin-bot Bot merged commit 2e99537 into linuxdeepin:master Apr 28, 2026
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants