fix: handle commit SHA refs in subdirectory package clones#178
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes apm install idempotency for virtual subdirectory (monorepo) packages when a lockfile provides a pinned commit SHA, by avoiding git clone --depth=1 --branch <sha> (which fails for arbitrary SHAs) and instead cloning then checking out the SHA.
Changes:
- Detect “ref looks like commit SHA” in
download_subdirectory_package. - For commit SHAs, perform a non-shallow clone and
git checkout <sha>; otherwise keep shallow clone behavior for branch/tag refs.
You can also share your feedback on Copilot code review. Take the survey.
When a lockfile contains a resolved commit SHA, download_subdirectory_package was passing it as --branch to a shallow clone, which git does not support. Now detects commit SHAs and uses a full clone + checkout instead. Fixes microsoft#177
- Use no_checkout=True instead of full working-tree clone for SHA refs - Separate try/except for clone vs checkout with descriptive error messages - Use exception chaining (raise ... from e) to preserve tracebacks - Remove unnecessary ref.lower() call - Add 4 unit tests covering SHA, branch, no-ref, and checkout failure paths
sergio-sisternes-epam
force-pushed
the
fix/177-lockfile-commit-sha-clone
branch
from
1f9cbf7 to
eff6a6e
Compare
danielmeppiel
left a comment
danielmeppiel
left a comment
There was a problem hiding this comment.
LGTM — correct fix for a real bug (lockfile commit SHAs breaking --branch).
What I verified:
- Root cause accurately identified:
git clone --branch <sha>is unsupported for arbitrary commit SHAs ✓ - SHA detection regex
r'^[a-f0-9]{7,40}$'is the standard pattern (7-char min avoids false positives on short branch names) ✓ no_checkout=True→ GitPython translates to--no-checkoutforRepo.clone_from— correct ✓_try_sparse_checkoutalso fails for commit SHAs (git fetch origin <sha> --depth=1), but gracefully returnsFalseand falls through to the now-fixed full clone path — flow is correct ✓- Exception chaining (
from e) added — nice improvement ✓ - 4 tests cover SHA, branch, no-ref, and checkout failure cases ✓
Non-blocking note for future: Full clone for SHA refs could be slow on large repos. A more optimal approach would be git init + git fetch origin <sha> + git checkout FETCH_HEAD, but this is an acceptable v1. Could be optimized later if users report perf issues.
Merge order: PR 2 of 3. Rebase onto main after #176 merges.
Fixes #177
Problem
download_subdirectory_packagepassed lockfile commit SHAs as--branchto shallow clones, which git does not support. This caused every subdirectory package to fail on the secondapm installwhen a lockfile was present.Reproduction
rm -rf apm_modules apm.lock) → succeeds, writesapm.lockwithresolved_commit: <sha>apm installagain → all subdirectory packages fail withfatal: Remote branch <sha> not found in upstream originSolution
download_subdirectory_packagegit checkout <sha>--depth=1 --branch=<ref>)Testing
apm installis now fully idempotent across repeated runs