refactor: remove monolithic clearReference#1678
Conversation
The old clearReference implementation always uses the default remote repo and ignores repo priority, which is not expected when the builder also supports repository priorities. We use clearReferenceLocal for local-only resolution and lookup remote reference via latestRemoteReference. Signed-off-by: reddevillg <reddevillg@gmail.com>
![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 refactors the dependency pulling and reference clearing logic by replacing the clearReference method with clearReferenceLocal across the codebase, including CLI, package manager, and runtime components. It also introduces a new clearDependency helper to handle local and remote dependency resolution. The review feedback highlights two critical issues: first, a potential silent failure in clearDependency where a successful result is incorrectly wrapped in an error after a local reference reset; second, a potential out-of-bounds vector access in semanticMatch when checking record.arch[0] without verifying if the vector is empty.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| std::optional<package::Reference> localRef; | ||
| auto localResult = repo.clearReference( | ||
| *fuzzyRef, | ||
| { .forceRemote = false, .fallbackToRemote = false, .semanticMatching = true }); | ||
| auto localResult = repo.clearReferenceLocal(*fuzzyRef, true); | ||
| if (localResult) { | ||
| localRef = std::move(localResult).value(); | ||
| } | ||
| if (localRef && module && !repo.getLayerDir(*localRef, *module)) { | ||
| localRef.reset(); | ||
| } | ||
|
|
||
| if (localRef && fuzzyRef->version && *fuzzyRef->version == localRef->version.toString()) { | ||
| return DependencyReference{ std::nullopt, std::move(localRef) }; | ||
| } | ||
|
|
||
| if (!useRemote) { | ||
| if (localRef) { | ||
| return std::move(*localRef); | ||
| return DependencyReference{ std::nullopt, std::move(localRef) }; | ||
| } | ||
| return LINGLONG_ERR(fmt::format("failed to get local ref {}", fuzzyRef->toString()), | ||
| localResult); | ||
| } | ||
|
|
||
| std::optional<package::Reference> remoteRef; | ||
| auto remoteResult = repo.clearReference( | ||
| *fuzzyRef, | ||
| { .forceRemote = true, .fallbackToRemote = true, .semanticMatching = true }); | ||
| std::optional<package::ReferenceWithRepo> remoteRef; | ||
| auto remoteResult = repo.latestRemoteReference(*fuzzyRef); | ||
| if (remoteResult) { | ||
| remoteRef = std::move(remoteResult).value(); | ||
| } | ||
|
|
||
| bool preferRemote = remoteRef && (!localRef || remoteRef->version > localRef->version); | ||
| auto &bestRef = preferRemote ? remoteRef : localRef; | ||
|
|
||
| if (!bestRef) { | ||
| bool preferRemote = | ||
| remoteRef && (!localRef || remoteRef->reference.version > localRef->version); | ||
| if (!preferRemote && !localRef) { | ||
| return LINGLONG_ERR(fmt::format("ref doesn't exist {}", fuzzyRef->toString())); | ||
| } | ||
|
|
||
| auto pullRes = pullResolvedRef(*bestRef, repo, module); | ||
| if (pullRes) { | ||
| return std::move(*bestRef); | ||
| if (!preferRemote) { | ||
| remoteRef.reset(); | ||
| } | ||
|
|
||
| return DependencyReference{ std::move(remoteRef), std::move(localRef) }; | ||
| } |
There was a problem hiding this comment.
In clearDependency, if localRef is reset because repo.getLayerDir fails, localResult still holds a successful result. Returning LINGLONG_ERR(..., localResult) when !useRemote is true will wrap a successful result, causing the caller to receive an error with a success code (0), which can lead to undefined behavior or silent failures. Additionally, if both local and remote resolution fail, the actual error from remoteResult (such as a network error) is discarded and replaced with a generic 'ref doesn't exist' message.
We should track the getLayerDir result and propagate the correct error, as well as propagate remoteResult's error if remote resolution fails.
std::optional<package::Reference> localRef;
auto localResult = repo.clearReferenceLocal(*fuzzyRef, true);
if (localResult) {
localRef = std::move(localResult).value();
}
utils::error::Result<package::LayerDir> layerDirResult = LINGLONG_OK;
if (localRef && module) {
layerDirResult = repo.getLayerDir(*localRef, *module);
if (!layerDirResult) {
localRef.reset();
}
}
if (localRef && fuzzyRef->version && *fuzzyRef->version == localRef->version.toString()) {
return DependencyReference{ std::nullopt, std::move(localRef) };
}
if (!useRemote) {
if (localRef) {
return DependencyReference{ std::nullopt, std::move(localRef) };
}
if (!localResult) {
return LINGLONG_ERR(fmt::format("failed to get local ref {}", fuzzyRef->toString()),
localResult);
}
return LINGLONG_ERR(fmt::format("failed to get local ref {} module {}", fuzzyRef->toString(), *module),
layerDirResult);
}
std::optional<package::ReferenceWithRepo> remoteRef;
auto remoteResult = repo.latestRemoteReference(*fuzzyRef);
if (remoteResult) {
remoteRef = std::move(remoteResult).value();
}
bool preferRemote =
remoteRef && (!localRef || remoteRef->reference.version > localRef->version);
if (!preferRemote && !localRef) {
if (!remoteResult) {
return LINGLONG_ERR(fmt::format("failed to get remote ref {}", fuzzyRef->toString()),
remoteResult);
}
return LINGLONG_ERR(fmt::format("ref doesn't exist {}", fuzzyRef->toString()));
}
if (!preferRemote) {
remoteRef.reset();
}
return DependencyReference{ std::move(remoteRef), std::move(localRef) };| if (fuzzy.arch && fuzzy.arch->toString() != record.arch[0]) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
If fuzzy.arch is specified but record.arch is empty, accessing record.arch[0] will cause an out-of-bounds access on the std::vector, leading to undefined behavior or a crash. We should check if record.arch is empty before accessing its first element.
if (fuzzy.arch && (record.arch.empty() || fuzzy.arch->toString() != record.arch[0])) {
return false;
}
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: dengbo11, reddevillg The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
3 participants
The old clearReference implementation always uses the default remote repo and ignores repo priority, which is not expected when the builder also supports repository priorities. We use clearReferenceLocal for local-only resolution and lookup remote reference via latestRemoteReference.