fix(offline): honor OFFLINE_WORK in git-ref2info and memoize TTL#9797
fix(offline): honor OFFLINE_WORK in git-ref2info and memoize TTL#9797iav wants to merge 2 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughAdds offline handling for memoized cache reuse and git ref resolution, including pinned SHA lookups from ChangesOffline Mode Support
Sequence Diagram(s)sequenceDiagram
participant BuildScript
participant memoized_git_ref_to_info
participant git_sources_json
participant git_remote
BuildScript->>memoized_git_ref_to_info: request ref info
memoized_git_ref_to_info->>git_sources_json: lookup pinned SHA1
alt pinned SHA1 found
git_sources_json-->>memoized_git_ref_to_info: sha1
memoized_git_ref_to_info-->>BuildScript: return pinned SHA1
else OFFLINE_WORK=yes and no pinned SHA1
memoized_git_ref_to_info-->>BuildScript: exit_with_error
else online
memoized_git_ref_to_info->>git_remote: git ls-remote
git_remote-->>memoized_git_ref_to_info: sha1
memoized_git_ref_to_info-->>BuildScript: return SHA1
end
Estimated code review effort: 3 (Moderate) | ~22 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
size/small
Test resultsStandalone scaffold (
All three guards exercised; |
igorpecovnik
added
08
iav
force-pushed
the
fix/offline-work-respect-cache
branch
from
88aabfd to
accc536
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/functions/general/git-ref2info.sh`:
- Around line 41-43: The offline_pinned assignment can produce multiple SHA
lines when git_sources.json has duplicate source+branch entries; change the jq
invocation in the offline_pinned assignment so it returns only a single match
(the first) for the given MEMO_DICT[GIT_SOURCE] and ref_name instead of multiple
lines (e.g., use jq’s first(...) or index [0] on the matching array) so the
subsequent 40-hex validation sees a single deterministic SHA for offline
resolution.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9fa27bd7-cd6a-4c46-ade3-b86004f8cfba
📒 Files selected for processing (2)
lib/functions/general/git-ref2info.shlib/functions/general/memoize-cached.sh
iav
force-pushed
the
fix/offline-work-respect-cache
branch
2 times, most recently
from
c4cabdc to
a7acb6f
Compare
|
@ColorfulRhino ping |
iav
force-pushed
the
fix/offline-work-respect-cache
branch
4 times, most recently
from
44e689f to
4c8d016
Compare
github-actions
Bot
added
size/medium
|
Refactored the pinned-SHA lookup into a small shared helper ( Beyond removing the duplicated jq call, this addresses a latent issue in the pre-existing block: it interpolated Also rebased onto current main. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/functions/general/git-ref2info.sh`:
- Around line 121-127: The condition checking `cached_revision` from the
`_git_sources_pinned_sha1` function call only validates that it is non-empty,
but does not verify it is a valid SHA1 format. This allows malformed pinned
values to overwrite the already-validated `sha1` from line 99. Add proper
validation of the `cached_revision` value to ensure it matches valid SHA1 format
before assigning it to `sha1`. Only override the `sha1` variable if
`cached_revision` passes the same validation checks that were applied to the
original `sha1` to prevent invalid MEMO_DICT[SHA1] values from being propagated.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e7212a4d-2ee7-413f-ad6e-94852bb1930c
📒 Files selected for processing (2)
lib/functions/general/git-ref2info.shlib/functions/general/memoize-cached.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/functions/general/memoize-cached.sh
iav
force-pushed
the
fix/offline-work-respect-cache
branch
from
4c8d016 to
6ea7313
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
lib/functions/general/git.sh (1)
255-263: Note inconsistent URL handling betweengit.shandgit-ref2info.shwhen looking up pinned SHA1s.While
config/sources/git_sources.jsondoesn't currently exist in the repository, the code is designed to support it as user-configurable (as indicated by comments like "populate cache, or pin sha1 in config/sources/git_sources.json").There is an inconsistency in how the two callers pass URLs to
_git_sources_pinned_sha1:
- git.sh (line 257) passes
"${url}", which has already been transformed by theGITHUB_SOURCEsubstitution on line 89- git-ref2info.sh passes
MEMO_DICT[GIT_SOURCE], which preserves the original URLIf a user configures
GITHUB_MIRROR(e.g., toghproxyorgitclone), the transformed URL in git.sh won't match canonical URLs that would be stored in the JSON file, causing pinned SHA1 lookups to silently fail in git.sh while succeeding in git-ref2info.sh.Consider passing the original (untransformed) URL to the helper, or transform it before writing to JSON, to ensure consistent matching across both call sites.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/functions/general/git.sh` around lines 255 - 263, The _git_sources_pinned_sha1 function call in the branch handling block is passing the already-transformed URL (which has been modified by GITHUB_SOURCE substitution), but this won't match the canonical URLs that would be stored in the configuration JSON file, causing pinned SHA1 lookups to fail when GITHUB_MIRROR is configured. Pass the original untransformed URL to _git_sources_pinned_sha1 instead of the transformed "${url}" variable, ensuring the lookup key matches what users would configure in the JSON file.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@lib/functions/general/git.sh`:
- Around line 255-263: The _git_sources_pinned_sha1 function call in the branch
handling block is passing the already-transformed URL (which has been modified
by GITHUB_SOURCE substitution), but this won't match the canonical URLs that
would be stored in the configuration JSON file, causing pinned SHA1 lookups to
fail when GITHUB_MIRROR is configured. Pass the original untransformed URL to
_git_sources_pinned_sha1 instead of the transformed "${url}" variable, ensuring
the lookup key matches what users would configure in the JSON file.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3fb63c03-93cc-409f-ae1a-afa7f8e414b6
📒 Files selected for processing (1)
lib/functions/general/git.sh
|
Re the git.sh nitpick (pinned-SHA lookup keying on the mirror-transformed URL): this is pre-existing behavior — the block this commit refactored already used the same post- |
Fixes #6439. memoized_git_ref_to_info() unconditionally ran 'git ls-remote' against the upstream source even with OFFLINE_WORK=yes; when the remote was unreachable the build aborted with a misleading network error. Two coordinated guards: * memoize-cached.sh: when OFFLINE_WORK=yes, serve the cache file regardless of TTL (alternative is a network call that will fail). The "stale cache served" event is logged. * git-ref2info.sh: early offline guard before the ls-remote loop. For ref_type=commit the SHA1 is taken from the ref itself (no network); for branch/tag we look up a pinned SHA1 in config/sources/git_sources.json. If neither is available, exit_with_error with a clear message instead of letting curl/ls-remote surface as 502. Same guard applied to the include_makefile_body block (curl to git host) since there is no local-bare fallback in scope here. Behaviour matrix unchanged for OFFLINE_WORK=no (default). Assisted-by: Claude:claude-opus-4.7
fetch_from_repo() carried its own copy of the pinned-SHA lookup from memoized_git_ref_to_info(), with the same shell-interpolated jq program (source/branch concatenated into the filter text). Replace it with the shared _git_sources_pinned_sha1 helper added earlier in this branch, which passes values through jq --arg, and validate the pinned SHA before overriding fetched_revision so a malformed pin fails loudly instead of silently — matching the branch path in git-ref2info.sh. Pure code cleanup: removes the last duplicated jq-injection of this lookup. Assisted-by: Claude:claude-opus-4.8
iav
force-pushed
the
fix/offline-work-respect-cache
branch
from
adca367 to
cd2a802
Compare
2 participants
Summary
Fixes #6439.
memoized_git_ref_to_info()unconditionally rangit ls-remoteagainst theupstream source even with
OFFLINE_WORK=yes; when the remote was unreachable(e.g.
git.kernel.org502) the build aborted with a misleading network errorinstead of honoring the offline flag.
Two coordinated guards:
memoize-cached.sh: whenOFFLINE_WORK=yes, serve the cache fileregardless of TTL (the alternative is a network call that will fail). The
"stale cache served" event is logged via
display_alert.git-ref2info.sh: early offline guard before thels-remoteloop. Forref_type=committhe SHA1 is taken from the ref itself (no network); forbranch/tag we look up a pinned SHA1 in
config/sources/git_sources.json. Ifneither is available,
exit_with_errorwith a clear message instead of lettingcurl/ls-remotesurface as a 502.The same guard is applied to the
include_makefile_bodyblock (curl to githost) since there is no local-bare fallback in scope here.
Behaviour matrix unchanged for
OFFLINE_WORK=no(default).OFFLINE_WORKgit_sources.jsonls-remote(unchanged)ls-remotethen pinned override (unchanged)ls-remote(unchanged)Refactor: shared pinned-SHA helper
The pinned-SHA lookup from
config/sources/git_sources.jsonwas duplicated acrossthree call sites, two of which built the
jqfilter by concatenatingsource/branchdirectly into the program text (breaks or mis-parses on valuescontaining quotes). Extracted into a single
_git_sources_pinned_sha1helper thatpasses values through
jq --arg, now shared by:git-ref2info.sh,fetch_from_repo()ingit.sh.Each call site validates the pinned SHA (40-hex) before use, failing loudly on a
malformed pin instead of silently overriding an already-resolved revision.
Test plan
Verified via standalone scaffold against an isolated
${SRC}/cache/memoize/sandbox:
OFFLINE_WORK=yesbuild with prior populated cache -> uses cache, no network callsOFFLINE_WORK=yesbuild with empty cache + no pinned SHA1 -> fails with clear "OFFLINE_WORK=yes but no SHA1 available ... run online once or pin sha1" messageOFFLINE_WORK=yesbuild with empty cache + pinned inconfig/sources/git_sources.json-> uses pinned SHA1, no network callsOFFLINE_WORK=no(default) build -> behaviour unchanged (onlinels-remotereturned real SHA1)OFFLINE_WORK=yeskernel-config (needs Makefile body) without cache -> fails with clear "Makefile body ... not in cache" messageSummary by CodeRabbit