Phase 2: Refactor zip_working_dir to use git ls-files - #294
Phase 2: Refactor zip_working_dir to use git ls-files#294GargiGupta-io wants to merge 2 commits into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces Git-aware packaging in kinetic/utils/packager.py by utilizing git ls-files to package only tracked and non-ignored files, falling back to os.walk if Git is unavailable. The review feedback highlights a robustness issue where path exclusions can fail if relative and absolute paths are mixed. To resolve this, it is recommended to use os.path.abspath instead of os.path.normpath when normalizing paths in both _path_is_excluded and zip_working_dir.
| def _path_is_excluded(path: str, exclude_paths: set[str]) -> bool: | ||
| if not exclude_paths: | ||
| return False | ||
| normalized_path = os.path.normpath(path) | ||
| return any( | ||
| normalized_path == excluded or normalized_path.startswith(excluded + os.sep) | ||
| for excluded in exclude_paths | ||
| ) |
There was a problem hiding this comment.
Comparing relative paths with absolute paths in _path_is_excluded will cause exclusions to fail when base_dir is specified as a relative path (e.g., . or ./), while exclude_paths contains absolute paths (which is the default for resolved Data objects). Converting all paths to absolute paths using os.path.abspath ensures robust and consistent comparison.
| def _path_is_excluded(path: str, exclude_paths: set[str]) -> bool: | |
| if not exclude_paths: | |
| return False | |
| normalized_path = os.path.normpath(path) | |
| return any( | |
| normalized_path == excluded or normalized_path.startswith(excluded + os.sep) | |
| for excluded in exclude_paths | |
| ) | |
| def _path_is_excluded(path: str, exclude_paths: set[str]) -> bool: | |
| if not exclude_paths: | |
| return False | |
| normalized_path = os.path.abspath(path) | |
| return any( | |
| normalized_path == excluded or normalized_path.startswith(excluded + os.sep) | |
| for excluded in exclude_paths | |
| ) |
References
- Demand Robustness: Do not accept fragile code. If the proposed code is not robust enough or lacks proper error handling, explicitly tell the author why the current approach is brittle and what must be done to reinforce it. (link)
| normalized_excludes = {os.path.normpath(p) for p in exclude_paths} | ||
|
|
||
| with zipfile.ZipFile(output_path, "w", zipfile.ZIP_DEFLATED) as zipf: | ||
| for root, dirs, files in os.walk(base_dir): | ||
| # Exclude .git, __pycache__, and Data-referenced directories | ||
| dirs[:] = [ | ||
| d | ||
| for d in dirs | ||
| if d not in [".git", "__pycache__"] | ||
| and os.path.normpath(os.path.join(root, d)) not in normalized_excludes | ||
| ] | ||
|
|
||
| for file in files: | ||
| file_path = os.path.join(root, file) | ||
| if os.path.normpath(file_path) in normalized_excludes: | ||
| continue | ||
| archive_name = os.path.relpath(file_path, base_dir) | ||
| zipf.write(file_path, archive_name) | ||
| git_files = _list_git_files(base_dir) | ||
| if git_files is not None: | ||
| _write_git_files(zipf, base_dir, git_files, normalized_excludes) | ||
| else: | ||
| for root, dirs, files in os.walk(base_dir): | ||
| dirs[:] = [ | ||
| d | ||
| for d in dirs | ||
| if d not in [".git", "__pycache__"] | ||
| and os.path.normpath(os.path.join(root, d)) not in normalized_excludes | ||
| ] | ||
|
|
||
| for file in files: | ||
| file_path = os.path.join(root, file) | ||
| if os.path.normpath(file_path) in normalized_excludes: | ||
| continue | ||
| archive_name = os.path.relpath(file_path, base_dir) | ||
| zipf.write(file_path, archive_name) |
There was a problem hiding this comment.
To ensure that path exclusions work reliably regardless of whether base_dir is specified as a relative or absolute path, we should normalize all excluded paths to absolute paths using os.path.abspath instead of os.path.normpath.
normalized_excludes = {os.path.abspath(p) for p in exclude_paths}
with zipfile.ZipFile(output_path, "w", zipfile.ZIP_DEFLATED) as zipf:
git_files = _list_git_files(base_dir)
if git_files is not None:
_write_git_files(zipf, base_dir, git_files, normalized_excludes)
else:
for root, dirs, files in os.walk(base_dir):
dirs[:] = [
d
for d in dirs
if d not in [".git", "__pycache__"]
and os.path.abspath(os.path.join(root, d)) not in normalized_excludes
]
for file in files:
file_path = os.path.join(root, file)
if os.path.abspath(file_path) in normalized_excludes:
continue
archive_name = os.path.relpath(file_path, base_dir)
zipf.write(file_path, archive_name)References
- Poke Holes in the Implementation: Your goal is to critically test the logic. Actively search for and point out failing edge cases, race conditions, or unhandled exceptions in the implementation. (link)
Adds helper functions to support git-aware packaging: - _list_git_files(): Lists tracked and non-ignored untracked files - _path_is_excluded(): Checks if a path should be excluded - _write_git_files(): Recursively writes files to ZIP respecting exclusions These functions enable respecting .gitignore when zipping working directories.
- Uses git ls-files when in a git repository to respect .gitignore - Falls back to directory traversal with os.walk when not in a git repo - Maintains all existing features: empty directory preservation, secret detection, path exclusion - Updated docstring to document git integration
4ffb14b to
d1f1dda
Compare
|
Merge order note: These 4 phases should be merged in sequence:
All code is ready and tested. Phases 1-3 have been pushed and are ready to merge. |
Implementation Note: 4-Phase Git IntegrationThis is Phase 2 of 4 for adding git-aware packaging with .gitignore support. Merge Order Required:
Status:✅ All code is complete and tested How to Proceed:Either:
All phases tested locally and verified to merge without conflicts when applied in order. |
1 participant
Integrates git ls-files into zip_working_dir():
Part of #288 (Phase 2 of 4). Merge after #293.