Release v1.3.6#84
Merged
Merged
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - 我发现了 1 个问题，并给出了一些总体反馈：
- 在 CalculateDirDiff 中，addedDirs/deletedDirs 切片是通过 map 遍历构建的，因此它们的顺序是非确定性的；建议在返回前对它们进行排序，这样 changes.json 和归档内容在多次运行之间就能保持稳定且可重现。
- 当前的 extractDirs 会针对每个文件向上遍历所有父目录；如果要在大型目录树上运行，你可以通过缓存已访问的目录，或者根据文件数量预先分配 map 来做一些微优化，以减少重复工作和内存分配。
供 AI 代理使用的提示
请根据这次代码评审中的评论进行修改：
## 总体评论
- 在 CalculateDirDiff 中，addedDirs/deletedDirs 切片是通过 map 遍历构建的，因此它们的顺序是非确定性的；建议在返回前对它们进行排序，这样 changes.json 和归档内容在多次运行之间就能保持稳定且可重现。
- 当前的 extractDirs 会针对每个文件向上遍历所有父目录；如果要在大型目录树上运行，你可以通过缓存已访问的目录，或者根据文件数量预先分配 map 来做一些微优化，以减少重复工作和内存分配。
## 逐条评论
### 评论 1
<location path="internal/pkg/patcher/patcher.go" line_range="328-330" />
<code_context>
}
}
+ // create added directories in temp root so they appear in the archive
+ for _, d := range addedDirs {
+ dirPath := filepath.Join(root, d)
+ if err := os.MkdirAll(dirPath, os.ModePerm); err != nil {
+ return fmt.Errorf("failed to create added dir: %w", err)
+ }
</code_context>
<issue_to_address>
**suggestion:** 当 MkdirAll 失败时，在错误信息中包含目录路径
当前的错误信息没有指出是哪个目录创建失败。在包装错误时包含路径（例如 `fmt.Errorf("failed to create added dir %q: %w", dirPath, err)`）会让定位权限或路径问题变得容易得多。
```suggestion
if err := os.MkdirAll(dirPath, os.ModePerm); err != nil {
return fmt.Errorf("failed to create added dir %q: %w", dirPath, err)
}
```
</issue_to_address>帮我变得更有用！请在每条评论上点 👍 或 👎，我会根据你的反馈改进后续评审。
Original comment in English
Hey - I've found 1 issue, and left some high level feedback:
- The addedDirs/deletedDirs slices are built from map iteration in CalculateDirDiff, so their order is nondeterministic; consider sorting them before returning so changes.json and archive contents are stable and reproducible across runs.
- extractDirs currently walks up every parent directory for each file; if this runs over large trees, you could micro-optimize by caching visited directories or preallocating the map based on the number of files to reduce repeated work and allocations.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The addedDirs/deletedDirs slices are built from map iteration in CalculateDirDiff, so their order is nondeterministic; consider sorting them before returning so changes.json and archive contents are stable and reproducible across runs.
- extractDirs currently walks up every parent directory for each file; if this runs over large trees, you could micro-optimize by caching visited directories or preallocating the map based on the number of files to reduce repeated work and allocations.
## Individual Comments
### Comment 1
<location path="internal/pkg/patcher/patcher.go" line_range="328-330" />
<code_context>
}
}
+ // create added directories in temp root so they appear in the archive
+ for _, d := range addedDirs {
+ dirPath := filepath.Join(root, d)
+ if err := os.MkdirAll(dirPath, os.ModePerm); err != nil {
+ return fmt.Errorf("failed to create added dir: %w", err)
+ }
</code_context>
<issue_to_address>
**suggestion:** Include directory path in the error message when MkdirAll fails
The error message doesn’t identify which directory failed. Including the path in the wrapped error (e.g. `fmt.Errorf("failed to create added dir %q: %w", dirPath, err)`) will make it much easier to diagnose permission or path issues.
```suggestion
if err := os.MkdirAll(dirPath, os.ModePerm); err != nil {
return fmt.Errorf("failed to create added dir %q: %w", dirPath, err)
}
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+328
to
+330
| if err := os.MkdirAll(dirPath, os.ModePerm); err != nil { | ||
| return fmt.Errorf("failed to create added dir: %w", err) | ||
| } |
There was a problem hiding this comment.
suggestion: 当 MkdirAll 失败时，在错误信息中包含目录路径
当前的错误信息没有指出是哪个目录创建失败。在包装错误时包含路径（例如 fmt.Errorf("failed to create added dir %q: %w", dirPath, err)）会让定位权限或路径问题变得容易得多。
Suggested change
| if err := os.MkdirAll(dirPath, os.ModePerm); err != nil { | |
| return fmt.Errorf("failed to create added dir: %w", err) | |
| } | |
| if err := os.MkdirAll(dirPath, os.ModePerm); err != nil { | |
| return fmt.Errorf("failed to create added dir %q: %w", dirPath, err) | |
| } |
Original comment in English
suggestion: Include directory path in the error message when MkdirAll fails
The error message doesn’t identify which directory failed. Including the path in the wrapped error (e.g. fmt.Errorf("failed to create added dir %q: %w", dirPath, err)) will make it much easier to diagnose permission or path issues.
Suggested change
| if err := os.MkdirAll(dirPath, os.ModePerm); err != nil { | |
| return fmt.Errorf("failed to create added dir: %w", err) | |
| } | |
| if err := os.MkdirAll(dirPath, os.ModePerm); err != nil { | |
| return fmt.Errorf("failed to create added dir %q: %w", dirPath, err) | |
| } |
There was a problem hiding this comment.
Pull request overview
This PR enhances incremental patch generation by tracking directory-level changes and ensuring archives include explicit directory entries, with accompanying end-to-end tests.
Changes:
- Add directory diff calculation (
CalculateDirDiff) and includeadded_dir/deleted_dirinchanges.json. - Update patch generation to create added directories so they are emitted into the resulting archive.
- Normalize archive entry paths and emit directory entries in ZIP/TAR.GZ compression; add comprehensive patcher tests.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| internal/pkg/patcher/patcher_test.go | New unit/integration-style tests covering diff, dir diff, and full patch pipeline for zip/tgz. |
| internal/pkg/patcher/patcher.go | Adds dir diff support and propagates added/deleted dirs into patch generation + changes.json. |
| internal/pkg/archiver/archiver.go | Normalizes entry paths to / and explicitly writes directory entries for zip/tar.gz. |
| internal/logic/version.go | Wires directory diff into incremental package creation flow. |
Comments suppressed due to low confidence (1)
internal/pkg/patcher/patcher.go:271
appendChangesRecordcreateschanges.jsonwithos.Createbut then ignores that handle and writes viaos.WriteFile. Besides being redundant, this means the final file permissions come from the initialos.Create(umask/0666) rather than the intended0644passed toWriteFile. Remove the unusedos.Createand write once (or write to the created file handle) to avoid incorrect perms and double opens.
func appendChangesRecord(root string, changes []Change, addedDirs, deletedDirs []string) error {
path := filepath.Join(root, "changes.json")
data := getChangesInfo(changes, addedDirs, deletedDirs)
f, err := os.Create(path)
if err != nil {
return fmt.Errorf("failed to create changes.json file: %w", err)
}
defer func(f *os.File) {
_ = f.Close()
}(f)
buf, err := sonic.Marshal(data)
if err != nil {
return fmt.Errorf("failed to marshal changes to JSON: %w", err)
}
if err := os.WriteFile(path, buf, 0644); err != nil {
return fmt.Errorf("failed to write JSON to file: %w", err)
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
154
to
+167
| relPath, err := filepath.Rel(srcDir, path) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| // normalize to forward slashes for standard zip entry names | ||
| relPath = filepath.ToSlash(relPath) | ||
|
|
||
| if info.IsDir() { | ||
| if relPath == "." { | ||
| return nil | ||
| } | ||
| _, err := writer.Create(relPath + "/") | ||
| return err | ||
| } |
Comment on lines
220
to
+236
| relPath, err := filepath.Rel(srcDir, path) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| // normalize to forward slashes for standard tar entry names | ||
| relPath = filepath.ToSlash(relPath) | ||
|
|
||
| if info.IsDir() { | ||
| return nil | ||
| if relPath == "." { | ||
| return nil | ||
| } | ||
| header, err := tar.FileInfoHeader(info, "") | ||
| if err != nil { | ||
| return err | ||
| } | ||
| header.Name = relPath + "/" | ||
| return tarWriter.WriteHeader(header) |
Comment on lines
+83
to
+97
| func CalculateDirDiff(newHashes, oldHashes map[string]string) (addedDirs, deletedDirs []string) { | ||
| newDirs := extractDirs(newHashes) | ||
| oldDirs := extractDirs(oldHashes) | ||
|
|
||
| for d := range newDirs { | ||
| if _, exists := oldDirs[d]; !exists { | ||
| addedDirs = append(addedDirs, d) | ||
| } | ||
| } | ||
| for d := range oldDirs { | ||
| if _, exists := newDirs[d]; !exists { | ||
| deletedDirs = append(deletedDirs, d) | ||
| } | ||
| } | ||
| return |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
在补丁生成过程中添加目录级别的差异追踪，并确保归档文件中对新增/删除的文件夹包含显式的目录条目。
新功能：
zip和tar.gz格式。增强：
zip和tar.gz归档在增量补丁中为新添加的目录包含显式的目录条目。测试：
zip和tar.gz格式的端到端完整流程，包括无变更、全部新增、全部删除、重命名以及仅根目录文件等边界情况。Original summary in English
Summary by Sourcery
Add directory-level diff tracking to patch generation and ensure archives contain explicit directory entries for added/removed folders.
New Features:
Enhancements:
Tests: