Skip to content

Commit 05a6c5b

Browse files
committed
fix(pm): address PR #330 review feedback
- tests: assert 'from lock' on rebuild instead of vacuous 'ls-remote' - prepare: dedupe cache-key formula via computeGitRoot(resolvedGitRev) - prepare: add anchor.url == spec.git to offline anchor match - prepare: drop 'mcpp update' from offline diagnostics (offline no-op) - commands: refresh stale comment in cmd_update about lockfile semantics - lock_io: split fragment on last '@' (branch names with '@') and reset empty resolvedCommit so the offline branch prints the right hint
1 parent c3e7095 commit 05a6c5b

4 files changed

Lines changed: 27 additions & 22 deletions

File tree

src/build/prepare.cppm

Lines changed: 11 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -3020,7 +3020,9 @@ prepare_build(bool print_fingerprint,
30203020
std::optional<std::string> lockedCommit;
30213021
if (auto it = gitLockAnchors.find(name); it != gitLockAnchors.end()) {
30223022
auto const& anchor = it->second;
3023-
if (anchor.refKind == spec.gitRefKind && anchor.ref == spec.gitRev) {
3023+
if (anchor.refKind == spec.gitRefKind
3024+
&& anchor.ref == spec.gitRev
3025+
&& anchor.url == spec.git) {
30243026
lockedCommit = anchor.resolvedCommit;
30253027
}
30263028
}
@@ -3063,13 +3065,13 @@ prepare_build(bool print_fingerprint,
30633065
if (lockedCommit) {
30643066
return std::unexpected(std::format(
30653067
"git dep '{}' locked to commit {} but its local cache is missing or stale; "
3066-
"run without --offline to refresh, or `mcpp update {}` to re-resolve",
3067-
name, *lockedCommit, name));
3068+
"run without --offline to refresh.",
3069+
name, *lockedCommit));
30683070
} else {
30693071
return std::unexpected(std::format(
30703072
"git dep '{}' uses branch '{}' and mcpp.lock has no commit; "
3071-
"cannot resolve offline. Run `mcpp update {}` or build without --offline.",
3072-
name, spec.gitRev, name));
3073+
"cannot resolve offline. Run without --offline.",
3074+
name, spec.gitRev));
30733075
}
30743076
}
30753077
auto cmd = std::format(
@@ -3102,18 +3104,14 @@ prepare_build(bool print_fingerprint,
31023104
mcpp::platform::env::offline_mode()) {
31033105
return std::unexpected(std::format(
31043106
"git dep '{}' is locked but its local cache is missing; "
3105-
"run without --offline to clone, or `mcpp update {}` to re-resolve.",
3106-
name, name));
3107+
"run without --offline to clone.",
3108+
name));
31073109
}
31083110
}
31093111

31103112
// Cache key: hash(url + refkind + declared ref + resolved commit).
31113113
// For fixed rev/tag deps the declared ref is also the resolved ref.
3112-
std::hash<std::string> H;
3113-
auto urlHash = std::format("{:016x}",
3114-
H(spec.git + "|" + spec.gitRefKind + "|" + spec.gitRev
3115-
+ "|" + resolvedGitRev));
3116-
auto gitRoot = mcppHome / "git" / urlHash;
3114+
auto gitRoot = computeGitRoot(resolvedGitRev);
31173115
std::error_code ec;
31183116
std::filesystem::create_directories(gitRoot.parent_path(), ec);
31193117
if (!std::filesystem::exists(gitRoot / ".git")) {
@@ -3150,6 +3148,7 @@ prepare_build(bool print_fingerprint,
31503148
}
31513149
}
31523150
if (item.consumerDepIndex == kMainConsumer) {
3151+
std::hash<std::string> H;
31533152
auto source = std::format("git+{}#{}={}",
31543153
spec.git, spec.gitRefKind, spec.gitRev);
31553154
if (spec.gitRefKind == "branch") source += "@" + resolvedGitRev;

src/pm/commands.cppm

Lines changed: 9 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -412,14 +412,15 @@ inline int cmd_update(const mcpplibs::cmdline::ParsedArgs& parsed) {
412412

413413
// Refresh the index FIRST (#315/D6).
414414
//
415-
// This command used to only drop lock entries and tell the user to run
416-
// `mcpp build` — but the build path never reads mcpp.lock (prepare writes
417-
// it and nothing on that path loads it), so the whole command was a no-op:
418-
// it changed no behaviour whatsoever. It is also the only command whose
419-
// stated purpose is "get me newer dependencies", which since #315 is
420-
// exactly what an index refresh is for. Explicit intent, so no debounce and
421-
// no TTL — but still refused when offline, loudly, rather than silently
422-
// doing nothing again.
415+
// Since #330 the build path reads mcpp.lock to short-circuit `git
416+
// ls-remote` for branch deps that are already resolved to a commit
417+
// with a matching local cache. `mcpp update <dep>` therefore no
418+
// longer "refreshes" a git branch dep — it only drops the lock
419+
// entry, which forces the next build to call `ls-remote` again.
420+
// The index refresh below is unrelated: it covers the registry-served
421+
// version deps only. Skipped when offline — loudly, not silently —
422+
// since #315 is exactly about not paying a network round-trip to
423+
// achieve nothing.
423424
// Skipped when nothing in the project is served by the shared registry:
424425
// syncing it does nothing for path deps, git deps or a project `[indices]`
425426
// entry, and paying a multi-repo network round-trip to achieve nothing is

src/pm/lock_io.cppm

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -196,12 +196,17 @@ std::optional<LockedGitSource> parse_git_source(std::string_view source) {
196196
return std::nullopt;
197197

198198
auto refPart = fragment.substr(eqPos + 1);
199-
auto atPos = refPart.find('@');
199+
// Split on the LAST `@` so branch names containing `@` (e.g. "feat@v2")
200+
// match the write format in prepare.cppm (`ref + "@" + commit`).
201+
auto atPos = refPart.rfind('@');
200202
if (atPos == std::string_view::npos) {
201203
out.ref = std::string(refPart);
202204
} else {
203205
out.ref = std::string(refPart.substr(0, atPos));
204206
out.resolvedCommit = std::string(refPart.substr(atPos + 1));
207+
// A bare `branch=foo@` would otherwise yield an empty commit and
208+
// confuse the offline anchor into reporting "locked to commit ".
209+
if (out.resolvedCommit->empty()) out.resolvedCommit.reset();
205210
}
206211

207212
if (out.ref.empty()) return std::nullopt;

tests/e2e/24_git_dependency.sh

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -155,7 +155,7 @@ out=$(${triple}${fp_dir}/bin/branchapp)
155155

156156
# Second build with the lock in place must not hit the network for ls-remote.
157157
build2=$("$MCPP" build 2>&1)
158-
echo "$build2" | grep -q 'ls-remote' && { echo "FAIL: branch dep re-ls-remoted on rebuild"; exit 1; } || true
158+
echo "$build2" | grep -q 'from lock' || { echo "FAIL: branch dep not resolved from lock on rebuild"; cat <<<"$build2"; exit 1; }
159159
echo "$build2" | grep -q 'Cloning' && { echo "FAIL: branch dep re-cloned on rebuild"; exit 1; } || true
160160

161161
grep -q 'source = "git+' mcpp.lock || {

0 commit comments

Comments
 (0)