Repository navigation
Conversation
DRAFT (local): ResolveRef read rev-parse's exit 1 for <ref>^{commit} as
not found, also for an existing branch or tag whose commit object is
missing. Now an existing ref, or a full id, that leads to an unreadable
commit is an error.
Change-Id: I89e91d6f73d8a70ebc6ac7df81837206535eb17f
Signed-off-by: Thomas Kosiewski <tk@coder.com>
…g headers
DRAFT (local): replace the tag-header walk with rev-parse <id>^{} and
cat-file -t, as restore-commit's target check does.
Change-Id: Iaf3b2c4f6922a6396f2da23ac77b5bdc96768809
Signed-off-by: Thomas Kosiewski <tk@coder.com>
…pack DRAFT (local): the probe as a test. Red on main's gitread (404 on all seven), green with the fix. Change-Id: Iae1d9f4f167af5331abf9d8198fc650ecc8f5fd5 Signed-off-by: Thomas Kosiewski <tk@coder.com>
…an error silent end and an exit. It now also writes after a heap cap, a crash and a canceled context, so a request that declined any of them fails the test without a delay overlay. The comment on request's failed write names close as the other way the write fails. Change-Id: I9b3e71af0af77592d6f95e5c308fcc54c2a2de7b Signed-off-by: Thomas Kosiewski <tk@coder.com>
…orrupt header restoreTargetTree reads cat-file -e exit 1 as "no such object", which holds only for a full object id: it now asserts one, as resolveTree does. A restore target whose loose object file is there but whose header git cannot read (a bad size, a file that is not zlib data, or a tag of such a commit) makes rev-parse exit 128, and stays an error (500), as on main. Change-Id: I7c3db4f8df96b1698254846d5e87d420ae4be0f2 Signed-off-by: Thomas Kosiewski <tk@coder.com>
…error
A fake git fails rev-parse ^{commit} of a tree (exit 128, killed),
cat-file -e (exit 128, killed) and cat-file -t (exit 128, killed), and
makes rev-parse ^{commit} of a readable commit exit 1. Each is an error,
never "not found". Five mutants of restoreTargetTree passed every test
before: a ^{commit} failure that falls through, a cat-file -e failure
read as "not found" or swallowed, a swallowed cat-file -t failure, and
a commit at the end of the chain read as "not found".
Change-Id: Id7adda38285b94b6f473cd7b4c272031f2eed411
Signed-off-by: Thomas Kosiewski <tk@coder.com>
…an error
The same checks as restoreTargetTree's: a fake git fails the read of
the ref's value, cat-file -e (of a ref's object and of a full id),
the peel to the end of the chain, and cat-file -t, and makes
rev-parse ^{commit} of a readable commit exit 1. Each is an error,
never ErrNotFound. Five of these six mutants of commitUnreadable passed
every gitread test before.
Change-Id: Ibba0a2b2de99cab307135aedd92905a239bded35
Signed-off-by: Thomas Kosiewski <tk@coder.com>
Change-Id: Id4680df8a9d217ce726f253ecc20db9f963e8ded Signed-off-by: Thomas Kosiewski <tk@coder.com>
|
@codex review |
|
@codex security review |
Codex Review Summary: ✅ CompletedThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 59b04114aa
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
🛡️ Codex Security ReviewSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
Codex round 1 on #782: an abbreviation of a tag whose commit is missing answered 404, while its full id and its ref name answered 500. commitUnreadable returned nil for every abbreviation. It now resolves an abbreviation without a peel, as it reads a ref's value: rev-parse exits 1 only when no object, or more than one, has it. The one object it names gets the same checks as a full id. Change-Id: I66abe1e43b4c24c60690573af90dea8af78c678a Signed-off-by: Thomas Kosiewski <tk@coder.com>
|
@codex review |
|
@codex security review |
|
Codex Review: Didn't find any major issues. Keep them coming! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
🛡️ Codex Security ReviewSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
… only what exists The #782 advisor's finding: commitUnreadable ran after every candidate whose peel exited 1, so a read by full id took 6 git runs (main 4) and a read by tag 4 (main 3). ResolveRef now reads every ref candidate that exists, with its value, in one for-each-ref, and gives rev-parse the ref's object id, never the name. Each name goes as a glob whose last byte is escaped, so that it matches the whole name only, not the refs under it. A branch, a tag and a full id each take 2 git runs. A ref that exists and whose commit git cannot read is still an error, also when a tag or an id of its name resolves. check-ref-format is gone: no name reaches rev-parse. Change-Id: Ia671d6336e234b58effa4a3e457e0459e9b5f7f7 Signed-off-by: Thomas Kosiewski <tk@coder.com>
…specs Every for-each-ref pattern matches one whole name, so a line for any other ref, or one that is no "<name> <id>", is now git's failure, not skipped. A spec with a glob character names no ref, though as a pattern it would match main. Change-Id: Ied15ef9c44cb635abe6a9199c8056c4ff0bd4a58 Signed-off-by: Thomas Kosiewski <tk@coder.com>
…and fix the fallback comments The #782 verifier's counts: an unknown name, an abbreviation, a tree id and a blob abbreviation took more git runs than on main. commitUnreadable now reads whether an id names one object, and its type, with one cat-file --batch-check ("<id> missing", "<id> ambiguous" or "<oid> <type>"), and peels only a tag. Every read in TestResolveRefGitRuns takes at most main's runs: a branch, a tag, a full id and an abbreviation of a commit 2, an unknown name 1, a tree id and an abbreviation of a blob 3. create-branch's and fork's comments: a base or fork point that names no ref falls back or is a 400, and one whose commit git cannot read is a 500. Change-Id: Ibd7d38c12c8211ad1c50f49a3fc2a7860f007010 Signed-off-by: Thomas Kosiewski <tk@coder.com>
With base_ref naming a tag whose commit is in no pack and preferred_base main, create-branch answers 500. On main's gitread it fell back to main and answered 200. A base that names no ref still falls back (200). Change-Id: I8551165b1cb1ca398c7faea376e83c30e3e62da7 Signed-off-by: Thomas Kosiewski <tk@coder.com>
|
@codex review |
|
@codex security review |
|
Codex Review: Didn't find any major issues. You're on a roll. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
🛡️ Codex Security ReviewSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
…refs The #782 round-3 verifier's blocker: for-each-ref reads the loose-ref directories for every pattern. At 180k refs a read took 5-7 seconds, and at 540k it exceeded the production memory cap. There is no ref cap. ResolveRef now reads each candidate with rev-parse --verify --quiet without a peel, so that a ref's value still resolves when its object is missing. One cat-file --batch-check reads the full id's type and its ^{commit} and ^{} peels. A missing object for a ref, a broken tag chain, and a commit git cannot read are errors. Every lookup is direct, no command lists refs, and git's DWIM rules stay in use (refs/r can resolve refs/heads/refs/r). A guard refuses revision operators and control bytes before the name reaches git. Each ordinary read runs git at most as often as on main. The 20k-loose-ref test records git's ref debug trace: no iterator steps, and adding 20k unrelated refs adds no direct reads. It fails on round 3 with 9 iterator steps, and the DWIM test fails on round 3 for all three names. The tag-chain-end-commit injection from the verifier is pinned by the batch answer test. The grep cursor test now injects its stalled git at the raw anchor lookup, because ^{commit} goes on stdin. Change-Id: Ifb37cb85ef63f07a55320b11bf97ce34cebbe025 Signed-off-by: Thomas Kosiewski <tk@coder.com>
|
@codex review |
|
@codex security review |
|
Codex Review: Didn't find any major issues. Nice work! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
🛡️ Codex Security ReviewSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
A commit that shares its abbreviation with a blob or a tree made the
plain rev-parse say ambiguous (exit 1), and ResolveRef answered 404
where main answered 200. The hex abbreviation is now read as
<abbrev>^{commit} first, which picks the commit. Only on git's exit 1
does it get the plain lookup, so an abbreviation of a tag whose commit
git cannot read is still an error. A DEL byte, like every other control
byte, never reaches git.
Change-Id: I8f64ec8b191b84d7b4ded49ea2fa5173ebde4155
Signed-off-by: Thomas Kosiewski <tk@coder.com>
|
@codex review |
|
@codex security review |
|
Codex Review: Didn't find any major issues. 🎉 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
🛡️ Codex Security ReviewSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
Why
An existing branch or tag whose commit git cannot read must answer 500, not 404. On main before this PR,
ResolveRefpeeled each candidate withrev-parse --verify --quiet <candidate>^{commit}and read exit 1 as "no such ref". git also exits 1 for a ref that exists but whose commit is missing, or whose tag chain git cannot read.Round 3 tried to save git spawns with
for-each-ref. The root verifier found a blocker: it reads loose-ref directories for any pattern. At 180k loose refs a read took 5–7 seconds, and at 540k it exceeded the production 32 MiB memory cap. There is no ref cap. Round 4 removesfor-each-reffrom resolution. Every candidate lookup is direct, with no ref-directory listing.Round 4 read a hex abbreviation with a plain
rev-parse --verify --quiet <abbrev>. When a commit shares the abbreviation with a blob or a tree, git calls that name ambiguous and exits 1, soGET /repos/commit?ref=<abbrev>answered 404 where main answers 200. Round 5 reads an abbreviation as<abbrev>^{commit}first, as main does.What changes
ResolveRefreads each candidate name withrev-parse --verify --quiet --end-of-options <candidate>, without a peel. A ref whose object is missing still resolves to its full id. A name that does not resolve exits 1. This keeps git's short-name DWIM rules, includingrefs/rresolvingrefs/heads/refs/r.<abbrev>^{commit}first, as on main: only the peel picks a commit that shares the abbreviation with a blob or a tree. Only after git's exit 1 does the abbreviation get the plain lookup and the batch-check, so an abbreviation of a tag whose commit is missing is still an error. Any other failure of the peel is an error. A full id gets the plain lookup only.~,^,:,@{,.., glob characters and backslash) and control bytes (DEL included) before a name reaches git. None can be in a ref's name. git rejects other malformed names itself. The hex fallback is the only object revision.resolveCommitObjectsends three requests to onecat-file --batch-check: the full id,<id>^{commit}and<id>^{}. Its structured answers give the object's type and the two peels. An unknown full id, and a tree, a blob or a tag of one, stay "not found". A ref whose object is missing, a commit that cannot peel to a readable commit, and a broken tag chain are errors. Nothing reads a tag's message or classifies stderr.restoreTargetTree(fix(api): restore only a commit, and answer 500 for one git cannot read #771) asserts that its target is a full object id, asresolveTreedoes:cat-file -eexit 1 means "no such object" only for a full id.treeSource.request's comment namescloseas the other way its write fails (fix(gitread): decline a tree walk whose cat-file ended before the request #774 verifier).base_ref, and fork with an unreadable source fork point, now answer 500. Before, create-branch fell back topreferred_base(200), and fork answered 400. A base that names no ref still falls back, and a fork point that names no ref is still a 400. The handler comments and the PR body state the distinction.docs/reference/http-api.mdstate the lookup rules and the 500 for a ref whose commit git cannot read.Verification
TestResolveRefUnreadableCommitIsAnError: a branch, full ref, tag, tag of a tag, and the tag's full id (lower and upper case) and abbreviations (12 and 7 characters), whose commit is missing, are errors. Unknown names, a tree, a tag of a tree or blob, an unknown full id, and the missing commit's own id stayErrNotFound. A broken branchdupbeside a readable tagdup, and a broken branch named like a readable commit's full id, are errors.TestResolveRefManyLooseRefsDoNotAddWorkrecords git's ref debug trace. Adding 20,000 irrelevant loose refs adds no direct ref reads, and the trace records no ref-iterator steps. Round 3 (f3cf0c7) fails with 9 iterator steps. No process environment is recorded.TestResolveRefKeepsGitDWIM:refs/r,randrefs/tresolverefs/heads/refs/r,refs/heads/refs/heads/randrefs/tags/refs/t. All three fail on round 3.TestResolveRefRevisionSyntaxSkipsGitpins that revision operators and control bytes never start git. Round 5 adds a DEL (0x7f) row.TestResolveRefAbbreviationSharedWithABlob(round 5) writes a blob whose id shares c1's first 4 characters. It first checks that git's plainrev-parse --verify --quiet <prefix>exits 1 there, then expects the 4-character prefix, a 12-character abbreviation and the full id to resolve to c1. Round 4 (2e9f9fe) answersErrNotFound.TestResolveRefOnlyExitOneIsNotFoundgains two rows: a peel that exits 128 and a peel whose git is killed are errors, notErrNotFound.TestResolveRefGitRunscounts git spawns. Every read that finds a commit takes at most main's runs. An abbreviation of a blob, which answers 404, takes one run more than main: the peel, then the plain lookup and the batch-check.TestResolveCommitObjectGitFailures: a failed batch-check (exit 128 or a kill), an answer for another object, an incomplete answer, an invalid peel answer, an unreadable commit, and a tag chain that is unreadable or ends at a commit after its commit peel failed, are errors. The last case pins the verifier's surviving mutant (the oldcat-file -tcheck read a commit at the chain end as "not found").TestResolveRefAmbiguousAbbreviationIsNotFoundbuilds blobs with a shared 4-character prefix and pinsErrNotFound.TestReadEndpointsAnswer500ForARefGitCannotRead(api, PostgreSQL and SQLite): a tag whose commit is in no pack answers 500 oncommits(name, full id, abbreviation),commit,files,file,blameanddiff. Main's gitread answers 404. create-branch with thatbase_refandpreferred_basemain answers 500 (main's gitread: 200), and with an unknown base, 200.TestRestoreTargetTreeGitFailuresandTestRestoreTargetTreeTellsMissingFromBroken: the five injection-test gaps from fix(api): restore only a commit, and answer 500 for one git cannot read #771's verifier, a corrupt loose-object header (500, as on main), and the full-id assertion.TestTreeSourceRequestAfterGitEndedalso writes after a heap cap, crash and canceled context: each stays an error (fix(gitread): decline a tree walk whose cat-file ended before the request #774 verifier).The grep cursor's existing failure-injection test now stalls the raw anchor lookup. Its
^{commit}value goes on stdin, not argv. The full SQLite api run caught this stale injection before publication.Five round-5 mutants each fail a test at 3c61289: round 4's
ResolveRef(it failsTestResolveRefAbbreviationSharedWithABlob,TestResolveRefGitRunsandTestResolveRefOnlyExitOneIsNotFound), the peel used for full ids too, exit 1 of the peel read as not found without the plain lookup, any other failure of the peel read as not found, and a DEL byte let through to git.Nine round-4 mutants each failed a test: revision operators allowed, control bytes allowed, raw lookup with a peel, a ref's missing object read as not found, no id check on the type answer, no peel-answer validation, an unreadable tag chain read as not found, a tag of a blob treated as unreadable, and a non-commit object treated as an error.
At 2e9f9fe (round 4), with
-race: build, vet, golangci-lint (0 issues), the doc-link check and both--fingerprints-onlychecks pass.internal/gitread/...passes.internal/apiandcmd/openstoragepass in three runs, two of them with-tags=openstorage_viewcheck.internal/apiruns each integration test on PostgreSQL and SQLite (runBackends). Neither package readsOPENSTORAGE_TEST_BACKEND, so the three runs cover the same two backends. No lane ran S3-WAL.At 3c61289 (round 5: a67529e merged with main 3ceb600), with
-race -tags=openstorage_viewcheck: build, vet, golangci-lint (0 issues), the doc-link check and both--fingerprints-onlychecks pass.internal/gitread/...passes, and so do the focused tests (shared prefix, git-run counts, 20,000 loose refs, revision syntax, peel failures).internal/apiandcmd/openstoragepass on PostgreSQL and SQLite under the container lock. No lane ran S3-WAL.Two tests at 3c61289 passed only on the rerun. The first gate run there ran at host load 98 to 198.
TestLsTreePageCursorOnARootFolderOverHalfTheHeapCapran out of its 1-minute git budget after 67 s, andinternal/apihit its 60-minute test timeout while a 1-second-old subtest ran. This PR changes only a comment intree_resume.goand does not touch that test. The rerun passedinternal/gitread/...in 481 s, andinternal/apiwithcmd/openstoragein 569 s (with a 90-minute timeout).Size
754 changed lines: 163 of production Go, 585 of tests and 6 of docs.
Generated with
xum• Model:anthropic:claude-opus-5-5• Thinking:xhigh