-
Notifications
You must be signed in to change notification settings - Fork 3
[1/n] add git ref storage for older API versions #38
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
[1/n] add git ref storage for older API versions #38
Conversation
Created using spr 1.3.6-beta.1
Created using spr 1.3.6-beta.1
Created using spr 1.3.6-beta.1
|
Wow, that is awesome. Didn’t know you can do that. So the gitref effectively takes the old file out of the current list of files, allowing git to treat the change as a move? Edit: discussed in chat. Answer is yes. |
Created using spr 1.3.6-beta.1
Created using spr 1.3.6-beta.1
Created using spr 1.3.6-beta.1
Created using spr 1.3.6-beta.1
Created using spr 1.3.6-beta.1
Created using spr 1.3.6-beta.1
Created using spr 1.3.6-beta.1
|
Still reviewing, but this is a summary CC put together as part of the process. Thought it was worth saving. Put it under PR overview by Claude Code w/ Opus 4.5OverviewThis PR adds "git ref storage" for versioned APIs. When enabled, older (non-latest) API versions are stored as The motivation is to make Git treat API version changes as renames rather than add/delete pairs, producing cleaner diffs that show only the actual schema changes. Logic flow and reading orderThe core decision: should this version be a gitref?The key insight is comparing "first commits" - the commit where each version was originally introduced. If an older version's first commit differs from the latest version's first commit, it should be stored as a gitref (when git ref storage is enabled). Reading order
About
|
| Test | Scenario |
|---|---|
test_conversion |
Basic JSON → gitref conversion when adding new version |
test_same_first_commit_no_conversion |
No conversion when all versions share same commit |
test_lockstep_never_converted_to_git_ref |
Lockstep APIs excluded |
test_mixed_first_commits_selective_conversion |
Only different-commit versions converted |
test_git_ref_check_after_conversion |
Gitref tracking across multiple commits |
test_no_conversion_without_git_ref_enabled |
Feature disabled = no conversion |
test_convert_to_json_when_disabled |
Gitref → JSON when feature disabled |
test_duplicates |
Both JSON and gitref exist: correct one is kept |
test_remove_readd |
Remove/re-add version uses latest addition commit |
test_latest_removed |
Previous-latest converts back to JSON |
test_latest_removed_same_commit |
Same-commit versions also convert back |
test_git_error_reports_problem |
Git failures produce GitRefFirstCommitUnknown |
test_shallow_clone_creates_incorrect_git_refs |
Documents shallow clone limitation |
Test infrastructure
-
TestEnvironmentgains helpers:versioned_git_ref_exists(),read_versioned_git_ref(),read_git_ref_content(),make_shallow(), etc. -
fake-gitbinary: Fails on--diff-filter=Ato test error handling. -
New fixtures:
versioned_health_no_v1,versioned_health_v1_only, and various*_git_ref_*variants.
Relationship to PR #39
PR #39 handles unparseable local files (e.g., conflict markers) and merge conflict scenarios. It builds on this PR and covers:
- Rename conflict resolution when branches add different versions
- Blessed version conflicts with git refs enabled
- MERGE_HEAD awareness for accurate merge-base computation
- Tests for both git and jj workflows
| if any_uses_git_ref && is_shallow_clone(&env.repo_root) { | ||
| notes.push(Note::ShallowClone); | ||
| } | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this blow up? Shallow clones seem like they have a pretty low chance of working. Is this going to be a problem in CI?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know, to be honest. It can work in some situations depending on if the requisite objects are available. I'll try doing a few more tests to see what happens.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It worked on test PRs in Omicron. Probably fine to leave it as a warning and then see if that's confusing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah it depends on if the objects are actually available in the repo -- in particular, if --no-local is passed into a local git clone, it fails. Made this an error -- I'd rather be conservative and relax as necessary.
david-crespo
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work. The added complexity is, to me, very worth it. The Omicron PRs we’ve had recently where one line API changes cause 30000 line diffs are untenable. We need to be able to review the API schema diff when a PR changes the external API. We also don't want to add 800kb to the repo on every API change.
Created using spr 1.3.6-beta.1
Created using spr 1.3.6-beta.1
Created using spr 1.3.6-beta.1
When enabled, older (non-latest) blessed API versions are stored as
.gitreffiles containing acommit:pathreference instead of the fullJSON content. This allows Git to detect new API versions as renames. (The format is designed to be fed into
git show).The git ref uses the first commit that introduced the file (not the current merge-base) to ensure references remain stable as history evolves.
Git ref storage is disabled by default. Enable with
ManagedApis::with_git_ref_storage(), or per-API withManagedApi::use_git_ref_storage().Demo in Omicron:
For more information, see RFD 634.
TODO:
allow per-version customization of gitref storage (useful when particular fixed versions are special in some way)will do this when we need it