diff --git a/CHANGELOG.md b/CHANGELOG.md index 27711e79a..6787971bf 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Changed + +- `apm deps update` now skips download and integration for packages whose resolved SHA matches the lockfile SHA, making the common "nothing changed" case near-instant (#495) + ## [0.8.7] - 2026-03-30 ### Fixed diff --git a/docs/src/content/docs/reference/lockfile-spec.md b/docs/src/content/docs/reference/lockfile-spec.md index 36962cd60..bd1a228d0 100644 --- a/docs/src/content/docs/reference/lockfile-spec.md +++ b/docs/src/content/docs/reference/lockfile-spec.md @@ -215,8 +215,10 @@ The dependency resolver interacts with the lock file as follows: 1. **First install** — resolve all refs to commits, write `apm.lock.yaml`. 2. **Subsequent installs** — read `apm.lock.yaml`, reuse locked commits. Only newly added dependencies trigger resolution. -3. **Update** (`--update` flag or `apm deps update`) — re-resolve all refs, - overwrite the lock file with fresh commits. +3. **Update** (`--update` flag or `apm deps update`) -- re-resolve all refs + to their latest commits. If a resolved commit matches the existing lock + file entry and the local checkout is intact, the download is skipped. + Otherwise, the package is re-fetched. The lock file is always refreshed. When a locked commit is no longer reachable (force-pushed branch, deleted tag), APM MUST report an error and refuse to install until the lock file is updated. diff --git a/src/apm_cli/commands/install.py b/src/apm_cli/commands/install.py index 3b5b8ff24..674a71a2a 100644 --- a/src/apm_cli/commands/install.py +++ b/src/apm_cli/commands/install.py @@ -1053,12 +1053,15 @@ def _install_apm_dependencies( lockfile_path = get_lockfile_path(project_root) existing_lockfile = None lockfile_count = 0 - if lockfile_path.exists() and not update_refs: + if lockfile_path.exists(): existing_lockfile = LockFile.read(lockfile_path) if existing_lockfile and existing_lockfile.dependencies: lockfile_count = len(existing_lockfile.dependencies) if logger: - logger.verbose_detail(f"Using apm.lock.yaml ({lockfile_count} locked dependencies)") + if update_refs: + logger.verbose_detail(f"Loaded apm.lock.yaml for SHA comparison ({lockfile_count} dependencies)") + else: + logger.verbose_detail(f"Using apm.lock.yaml ({lockfile_count} locked dependencies)") if logger.verbose: for locked_dep in existing_lockfile.get_all_dependencies(): _sha = locked_dep.resolved_commit[:8] if locked_dep.resolved_commit else "" @@ -1435,23 +1438,27 @@ def _collect_descendants(node, visited=None): # detect_ref_change() handles all transitions including None→ref. _pd_locked_chk = ( existing_lockfile.get_dependency(_pd_key) - if existing_lockfile and not update_refs + if existing_lockfile else None ) _pd_ref_changed = detect_ref_change( _pd_ref, _pd_locked_chk, update_refs=update_refs ) - # Skip if lockfile SHA matches local HEAD (Phase 5 check) - # — but only if the ref itself has not changed in the manifest. - if _pd_path.exists() and existing_lockfile and not update_refs and not _pd_ref_changed: - _pd_locked = existing_lockfile.get_dependency(_pd_key) - if _pd_locked and _pd_locked.resolved_commit and _pd_locked.resolved_commit != "cached": - try: - from git import Repo as _PDGitRepo - if _PDGitRepo(_pd_path).head.commit.hexsha == _pd_locked.resolved_commit: - continue - except Exception: - pass + # Skip if lockfile SHA matches local HEAD. + # Normal mode: only when the ref hasn't changed in the manifest. + # Update mode: defer to the sequential loop which resolves the + # remote ref and compares -- if unchanged, the download is skipped + # entirely; if changed, it falls back to sequential download. + if (_pd_path.exists() and _pd_locked_chk + and _pd_locked_chk.resolved_commit + and _pd_locked_chk.resolved_commit != "cached" + and (update_refs or not _pd_ref_changed)): + try: + from git import Repo as _PDGitRepo + if _PDGitRepo(_pd_path).head.commit.hexsha == _pd_locked_chk.resolved_commit: + continue + except Exception: + pass # Build download ref (use locked commit for reproducibility). # build_download_ref() uses the manifest ref when ref_changed is True. _pd_dlref = build_download_ref( @@ -1668,11 +1675,22 @@ def _collect_descendants(node, visited=None): from apm_cli.models.apm_package import GitReferenceType resolved_ref = None - if dep_ref.reference and dep_ref.get_unique_key() not in _pre_downloaded_keys: - try: - resolved_ref = downloader.resolve_git_reference(dep_ref) - except Exception: - pass # If resolution fails, skip cache (fetch latest) + if dep_ref.get_unique_key() not in _pre_downloaded_keys: + # Resolve when there is an explicit ref, OR when update_refs + # is True AND we have a non-cached lockfile entry to compare + # against (otherwise resolution is wasted work -- the package + # will be downloaded regardless). + _has_lockfile_sha = False + if update_refs and existing_lockfile: + _lck = existing_lockfile.get_dependency(dep_ref.get_unique_key()) + _has_lockfile_sha = bool( + _lck and _lck.resolved_commit and _lck.resolved_commit != "cached" + ) + if dep_ref.reference or (update_refs and _has_lockfile_sha): + try: + resolved_ref = downloader.resolve_git_reference(dep_ref) + except Exception: + pass # If resolution fails, skip cache (fetch latest) # Use cache only for tags and commits (not branches) is_cacheable = resolved_ref and resolved_ref.ref_type in [ @@ -1685,25 +1703,41 @@ def _collect_descendants(node, visited=None): # detect_ref_change() handles all transitions including None→ref. _dep_locked_chk = ( existing_lockfile.get_dependency(dep_ref.get_unique_key()) - if existing_lockfile and not update_refs + if existing_lockfile else None ) ref_changed = detect_ref_change( dep_ref, _dep_locked_chk, update_refs=update_refs ) # Phase 5 (#171): Also skip when lockfile SHA matches local HEAD - # — but not when the manifest ref has changed (user wants different version). + # -- but not when the manifest ref has changed (user wants different version). lockfile_match = False - if install_path.exists() and existing_lockfile and not update_refs and not ref_changed: + if install_path.exists() and existing_lockfile: locked_dep = existing_lockfile.get_dependency(dep_ref.get_unique_key()) if locked_dep and locked_dep.resolved_commit and locked_dep.resolved_commit != "cached": - try: - from git import Repo as GitRepo - local_repo = GitRepo(install_path) - if local_repo.head.commit.hexsha == locked_dep.resolved_commit: - lockfile_match = True - except Exception: - pass # Not a git repo or invalid — fall through to download + if update_refs: + # Update mode: compare resolved remote SHA with lockfile SHA. + # If the remote ref still resolves to the same commit, + # the package content is unchanged -- skip download. + # Also verify local checkout matches to guard against + # corrupted installs that bypassed pre-download checks. + if resolved_ref and resolved_ref.resolved_commit == locked_dep.resolved_commit: + try: + from git import Repo as GitRepo + local_repo = GitRepo(install_path) + if local_repo.head.commit.hexsha == locked_dep.resolved_commit: + lockfile_match = True + except Exception: + pass # Local checkout invalid -- fall through to download + elif not ref_changed: + # Normal mode: compare local HEAD with lockfile SHA. + try: + from git import Repo as GitRepo + local_repo = GitRepo(install_path) + if local_repo.head.commit.hexsha == locked_dep.resolved_commit: + lockfile_match = True + except Exception: + pass # Not a git repo or invalid -- fall through to download skip_download = install_path.exists() and ( (is_cacheable and not update_refs) or already_resolved or lockfile_match ) @@ -1783,8 +1817,10 @@ def _collect_descendants(node, visited=None): source=dep_ref.repo_url, ) - # Create basic resolved reference for cached packages - resolved_ref = ResolvedReference( + # Use resolved reference from ref resolution if available + # (e.g. when update_refs matched the lockfile SHA), + # otherwise create a placeholder for cached packages. + resolved_or_cached_ref = resolved_ref if resolved_ref else ResolvedReference( original_ref=dep_ref.reference or "default", ref_type=GitReferenceType.BRANCH, resolved_commit="cached", # Mark as cached since we don't know exact commit @@ -1794,7 +1830,7 @@ def _collect_descendants(node, visited=None): cached_package_info = PackageInfo( package=cached_package, install_path=install_path, - resolved_reference=resolved_ref, + resolved_reference=resolved_or_cached_ref, installed_at=datetime.now().isoformat(), dependency_ref=dep_ref, # Store for canonical dependency string ) @@ -1810,9 +1846,13 @@ def _collect_descendants(node, visited=None): depth = node.depth if node else 1 resolved_by = node.parent.dependency_ref.repo_url if node and node.parent else None _is_dev = node.is_dev if node else False - # Get commit SHA: callback capture > existing lockfile > explicit reference + # Get commit SHA: resolved ref > callback capture > existing lockfile > explicit reference dep_key = dep_ref.get_unique_key() - cached_commit = callback_downloaded.get(dep_key) + cached_commit = None + if resolved_ref and resolved_ref.resolved_commit and resolved_ref.resolved_commit != "cached": + cached_commit = resolved_ref.resolved_commit + if not cached_commit: + cached_commit = callback_downloaded.get(dep_key) if not cached_commit and existing_lockfile: locked_dep = existing_lockfile.get_dependency(dep_key) if locked_dep: diff --git a/tests/unit/test_install_update.py b/tests/unit/test_install_update.py index 439f8a9bb..05bac905d 100644 --- a/tests/unit/test_install_update.py +++ b/tests/unit/test_install_update.py @@ -77,8 +77,8 @@ def test_cacheable_does_not_skip_with_update(self): ) is False def test_lockfile_match_always_skips(self): - """lockfile_match should always skip (not gated by update_refs because - the lockfile_match check itself is already gated by `not update_refs`).""" + """lockfile_match should always skip (including under update_refs) because + the lockfile_match check now handles both normal and update_refs modes.""" assert self._build_skip_download( install_path_exists=True, is_cacheable=False, @@ -513,3 +513,204 @@ def test_multiple_entries_partial_drift(self): "changed": {"url": "http://old.com"}, } assert detect_config_drift(current_configs, stored) == {"changed"} + + +class TestUpdateRefsShaComparison: + """Tests for the perf optimization: skip download when resolved SHA matches lockfile. + + When ``update_refs=True``, the engine resolves refs to get the latest SHA, + then compares against the lockfile SHA. If they match, the download is skipped. + This avoids re-downloading packages that are already at their latest version. + """ + + @staticmethod + def _build_lockfile_match_update(*, resolved_sha, lockfile_sha, install_exists, + local_head_sha=None): + """Reproduce the update_refs lockfile_match check from install.py. + + In update mode, lockfile_match is True when the resolved remote SHA + matches the lockfile SHA AND local HEAD matches (guarding against + corrupted local checkouts). + """ + resolved_ref = Mock() if resolved_sha else None + if resolved_ref: + resolved_ref.resolved_commit = resolved_sha + + locked_dep = Mock() if lockfile_sha else None + if locked_dep: + locked_dep.resolved_commit = lockfile_sha + + # Default: local HEAD matches lockfile when not explicitly set + if local_head_sha is None: + local_head_sha = lockfile_sha + + lockfile_match = False + if install_exists and locked_dep: + if locked_dep.resolved_commit and locked_dep.resolved_commit != "cached": + # Update mode: compare resolved SHA with lockfile SHA, + # then verify local checkout matches. + if resolved_ref and resolved_ref.resolved_commit == locked_dep.resolved_commit: + if local_head_sha == locked_dep.resolved_commit: + lockfile_match = True + return lockfile_match + + def test_matching_sha_skips_download(self): + """When resolved SHA matches lockfile SHA, lockfile_match is True.""" + assert self._build_lockfile_match_update( + resolved_sha="abc123def456", + lockfile_sha="abc123def456", + install_exists=True, + ) is True + + def test_different_sha_does_not_skip(self): + """When resolved SHA differs, lockfile_match is False (download needed).""" + assert self._build_lockfile_match_update( + resolved_sha="new_sha_789", + lockfile_sha="abc123def456", + install_exists=True, + ) is False + + def test_no_resolved_ref_does_not_skip(self): + """When resolution failed (None), lockfile_match is False.""" + assert self._build_lockfile_match_update( + resolved_sha=None, + lockfile_sha="abc123def456", + install_exists=True, + ) is False + + def test_no_lockfile_entry_does_not_skip(self): + """When no lockfile entry exists (new package), lockfile_match is False.""" + assert self._build_lockfile_match_update( + resolved_sha="abc123def456", + lockfile_sha=None, + install_exists=True, + ) is False + + def test_cached_lockfile_entry_does_not_skip(self): + """When lockfile has 'cached' placeholder, lockfile_match is False.""" + assert self._build_lockfile_match_update( + resolved_sha="abc123def456", + lockfile_sha="cached", + install_exists=True, + ) is False + + def test_no_install_path_does_not_skip(self): + """When install path doesn't exist, lockfile_match is False.""" + assert self._build_lockfile_match_update( + resolved_sha="abc123def456", + lockfile_sha="abc123def456", + install_exists=False, + ) is False + + def test_corrupted_local_checkout_does_not_skip(self): + """When local HEAD differs from lockfile SHA, lockfile_match is False + even if resolved remote matches lockfile (guards against corrupt installs).""" + assert self._build_lockfile_match_update( + resolved_sha="abc123def456", + lockfile_sha="abc123def456", + install_exists=True, + local_head_sha="corrupted_different_sha", + ) is False + + def test_skip_download_with_update_lockfile_match(self): + """End-to-end: skip_download is True when update_refs lockfile_match is True.""" + install_path_exists = True + is_cacheable = False + update_refs = True + already_resolved = False + lockfile_match = True # From the update_refs SHA comparison + + skip_download = install_path_exists and ( + (is_cacheable and not update_refs) or already_resolved or lockfile_match + ) + assert skip_download is True + + def test_no_skip_when_sha_changed_during_update(self): + """When remote SHA changed, skip_download is False (package must be fetched).""" + install_path_exists = True + is_cacheable = False + update_refs = True + already_resolved = False + lockfile_match = False # SHA comparison failed (remote changed) + + skip_download = install_path_exists and ( + (is_cacheable and not update_refs) or already_resolved or lockfile_match + ) + assert skip_download is False + + +class TestPreDownloadUpdateRefsSkip: + """Tests for the pre-download skip optimization when update_refs=True. + + When update_refs=True and the local HEAD matches the lockfile SHA, + the pre-download section skips the package (defers to sequential + resolution for remote SHA comparison). + """ + + @staticmethod + def _should_skip_pre_download(*, update_refs, path_exists, lockfile_sha, + local_head_sha): + """Reproduce the pre-download skip condition for update_refs.""" + locked_dep = Mock() if lockfile_sha else None + if locked_dep: + locked_dep.resolved_commit = lockfile_sha + + if update_refs and path_exists and locked_dep: + if locked_dep.resolved_commit and locked_dep.resolved_commit != "cached": + if local_head_sha == locked_dep.resolved_commit: + return True + return False + + def test_skip_when_head_matches_lockfile(self): + """Skip pre-download when local HEAD matches lockfile SHA.""" + assert self._should_skip_pre_download( + update_refs=True, + path_exists=True, + lockfile_sha="abc123", + local_head_sha="abc123", + ) is True + + def test_no_skip_when_head_differs(self): + """Don't skip pre-download when local HEAD differs (corrupted install).""" + assert self._should_skip_pre_download( + update_refs=True, + path_exists=True, + lockfile_sha="abc123", + local_head_sha="different", + ) is False + + def test_no_skip_when_not_update_mode(self): + """Don't use this optimization in normal install mode.""" + assert self._should_skip_pre_download( + update_refs=False, + path_exists=True, + lockfile_sha="abc123", + local_head_sha="abc123", + ) is False + + def test_no_skip_when_path_missing(self): + """Don't skip when install path doesn't exist.""" + assert self._should_skip_pre_download( + update_refs=True, + path_exists=False, + lockfile_sha="abc123", + local_head_sha="abc123", + ) is False + + def test_no_skip_when_no_lockfile_entry(self): + """Don't skip when there's no lockfile entry (new package).""" + assert self._should_skip_pre_download( + update_refs=True, + path_exists=True, + lockfile_sha=None, + local_head_sha="abc123", + ) is False + + def test_no_skip_when_lockfile_sha_is_cached(self): + """Don't skip when lockfile has 'cached' placeholder.""" + assert self._should_skip_pre_download( + update_refs=True, + path_exists=True, + lockfile_sha="cached", + local_head_sha="cached", + ) is False diff --git a/uv.lock b/uv.lock index 3246ec7c4..c35c123c1 100644 --- a/uv.lock +++ b/uv.lock @@ -179,7 +179,7 @@ wheels = [ [[package]] name = "apm-cli" -version = "0.8.6" +version = "0.8.7" source = { editable = "." } dependencies = [ { name = "click" },