Skip to content

fix: Simplify header name assignment in FileTask#347

Merged
deepin-bot[bot] merged 1 commit intolinuxdeepin:develop/snipefrom
dengzhongyuan365-dev:pzip-cpp
Jan 16, 2026
Merged

fix: Simplify header name assignment in FileTask#347
deepin-bot[bot] merged 1 commit intolinuxdeepin:develop/snipefrom
dengzhongyuan365-dev:pzip-cpp

Conversation

@dengzhongyuan365-dev
Copy link
Contributor

@dengzhongyuan365-dev dengzhongyuan365-dev commented Jan 16, 2026

  • Removed the addition of the base directory name when calculating the header name.
  • Directly set the header name to the relative path, streamlining the logic for path handling.

Summary by Sourcery

Bug Fixes:

  • Ensure generated archive header names no longer include an extra base directory segment when a relative path is available.

- Removed the addition of the base directory name when calculating the header name.
- Directly set the header name to the relative path, streamlining the logic for path handling.
@sourcery-ai
Copy link

sourcery-ai bot commented Jan 16, 2026

Reviewer's guide (collapsed on small PRs)

Reviewer's Guide

Simplifies how FileTask determines the ZIP header name by using the computed relative path directly, rather than prefixing it with the base directory name.

Class diagram for updated FileTask header name assignment

classDiagram
    class FileTask {
        +Error reset(fs::path filePath, fs::path relativeTo)
    }

    class Header {
        +string name
    }

    class Utils {
        +string toZipPath(fs::path path)
    }

    class FileSystemPath {
        +string filename()
    }

    FileTask --> Header : sets
    FileTask --> Utils : uses
    FileTask --> FileSystemPath : path_and_relPath
    Utils --> FileSystemPath : converts
Loading

File-Level Changes

Change Details Files
Use the relative path directly for ZIP header names instead of prefixing with the base directory name when a relative path is available.
  • Remove computation of the base directory name from the reference path
  • Eliminate concatenation of the base directory name with the relative path
  • Assign header.name using utils::toZipPath(relPath) when a relative path is successfully computed
3rdparty/pzip/src/file_task.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

deepin pr auto review

这段代码修改主要涉及文件路径处理逻辑的变更。以下是对该 git diff 的详细审查意见,涵盖语法逻辑、代码质量、代码性能和代码安全四个方面:

1. 语法逻辑

  • 修改内容:代码移除了 fs::path baseName = relativeTo.filename(); 以及将 baseName 拼接到 relPath 的逻辑,直接使用 utils::toZipPath(relPath)
  • 逻辑影响
    • 修改前:生成的 Zip 文件内的目录结构会包含 relativeTo 的最后一级目录名。例如,如果 relativeTo/home/user/project,且文件是 /home/user/project/src/file.cpp,Zip 中的路径会是 project/src/file.cpp
    • 修改后:生成的 Zip 文件内的目录结构将从 relativeTo 之后的路径开始。同上例,Zip 中的路径会变成 src/file.cpp
  • 审查意见
    • 潜在风险:这一改动改变了 Zip 文件的内部结构。如果依赖方(如解压脚本或加载逻辑)期望 Zip 根目录包含项目文件夹名(即 project/...),此修改将导致逻辑错误或文件找不到。
    • 建议:请确认调用 FileTask::reset 的业务场景。如果目的是打包整个项目并保留项目根目录名,应回退此修改;如果目的是仅打包项目内容(不包含根目录容器),则此修改是正确的。建议检查相关的单元测试是否覆盖了路径结构验证。

2. 代码质量

  • 代码简洁性:修改后的代码更加简洁,移除了不必要的中间变量 baseName,减少了代码行数,提高了可读性。
  • 注释一致性:原注释 // 加上基础目录名 已被删除,这是合理的,因为代码逻辑不再执行此操作。但建议确认是否需要在其他地方(如 API 文档或函数头注释)说明 relativeTo 参数的具体行为(即是否包含基础目录名)。

3. 代码性能

  • 路径操作
    • 移除了 relativeTo.filename() 的调用,这是一个轻量级的操作(通常只是提取字符串视图),因此性能提升微乎其微。
    • 移除了 baseName / relPath 的路径拼接操作,这会减少一次字符串构造和内存分配。
  • 审查意见:性能略有正向优化,但在文件 I/O 和压缩操作的整体开销中,这种优化几乎可以忽略不计。不过,减少不必要的对象构造总是好的。

4. 代码安全

  • 路径遍历安全
    • 修改前后的代码都依赖 fs::pathutils::toZipPath 来处理路径。只要 utils::toZipPath 能够正确处理路径分隔符转换(如将 \ 转为 /)并规范化路径(防止 ../ 路径遍历攻击),安全性没有显著变化。
    • 审查意见:请确保 utils::toZipPath 内部实现了路径规范化,防止 Zip Slip 漏洞(即通过构造包含 ../ 的文件名,解压时覆盖系统上的任意文件)。如果 relPath 来自外部不可信输入,这一点尤为重要。

总结与建议

这段代码修改在语法性能上是正向的(更简洁、开销更小),但核心问题在于业务逻辑的改变。

最终建议

  1. 确认需求:必须确认业务上是否需要保留 relativeTo 的最后一级目录名作为 Zip 包的根目录。
  2. 更新测试:如果确认修改是符合预期的,请务必更新相关的自动化测试用例,断言生成的 Zip 目录结构符合新的预期(即不包含基础目录名)。
  3. 检查依赖:全局搜索引用 FileTask 或解析该 Zip 包的代码,确保没有代码硬编码依赖旧的目录结构(例如 zip.read("project/config.ini") 可能需要改为 zip.read("config.ini"))。

Copy link

@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:

  • Consider explicitly handling the case where relPath is empty (or .) after fs::relative, since removing the baseName / relPath composition can now lead to an empty or unexpected header.name instead of falling back to the base directory name.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- Consider explicitly handling the case where `relPath` is empty (or `.`) after `fs::relative`, since removing the `baseName / relPath` composition can now lead to an empty or unexpected `header.name` instead of falling back to the base directory name.

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

[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
Contributor Author

/forcemerge

@deepin-bot deepin-bot bot merged commit 487cc03 into linuxdeepin:develop/snipe Jan 16, 2026
15 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