Read git metadata from the Repos API for Git CLI folders - #6303
Conversation
## Why A Git folder that has Git CLI access is a workspace DIRECTORY whose git metadata is not stored on the workspace object, so `workspace/get-status?return_git_info=true` returns only `id` and `path` for it - no `branch`, `head_commit_id` or `url`. Those fields are declared unconditionally in the API, and `fetchRepositoryInfoAPI` copied them straight into `RepositoryInfo`, so on DBR `bundle.git.branch`, `.commit` and `.origin_url` silently became empty. New Git folders are created this way as `wsfs_git_cli` ramps up (SDR-3545), which is what broke TestFetchRepositoryInfoAPI_FromRepo on all clouds. It is not a propagation race: polling never fills the fields in, so there is nothing to retry. ## Changes When get-status returns a git folder id but no metadata, read it from `Repos.Get`. `git_info.id` is the id of the Git folder root even when the queried path is a subdirectory, so both the root and subdir cases resolve. A legacy REPO folder still answers from get-status alone and makes no extra call. This also reverts the GCP skip from #6038 - that skip described this same gap, which reached GCP first. ## Tests New unit tests for `fetchRepositoryInfoAPI` covering full metadata (and asserting no Repos call), the fallback, a subdirectory, a failing `Repos.Get`, and a path with no git info. `integration/libs/git` passes against aws, azure and gcp.
| } | ||
|
|
||
| // A path outside a Git folder has no git_info and must not trigger a Repos lookup. | ||
| func TestFetchRepositoryInfoAPINoGitInfo(t *testing.T) { |
There was a problem hiding this comment.
Is this still true with Git folders now that can be creaed outside /Repos?
Integration test reportCommit: b7651e4
9 interesting tests: 4 SKIP, 3 RECOVERED, 1 flaky, 1 KNOWN
Top 11 slowest tests (at least 2 minutes):
|
## Why The fallback added in the previous commit was only covered by unit tests with a hand-rolled fake, and by an integration test that mocks the runtime in Go. Both miss what the CLI actually does end to end, and neither can run against a real workspace and a fake one from the same assertions. ## Changes Teach the testserver about `return_git_info`: get-status now reports a git_info block for a path in a Git folder, resolving the containing folder for paths inside one. A standard Git folder under /Repos reports the full metadata; one outside it has Git CLI access and reports only the id and path, which is the shape that needs the Repos API fallback. Repo creation also reports a head_commit_id now, as the real API does. A Git folder root is resolved before any directory entry for the same path, since `mkdirs` of a path inside a Git folder seeds its ancestors and would otherwise shadow the root and report the wrong object id. `bundle debug fetch-repository-info` reports what FetchRepositoryInfo resolves for a path. Nothing in the product calls it; it exists so that function can be driven from a test. `--workspace-api` selects the workspace-API path directly, which is otherwise only reachable on a Databricks Runtime. ## Tests New acceptance test covering a Git folder under /Repos, a subdirectory of it, a Git folder outside /Repos, and a path in no Git folder. The same golden passes locally and against aws, azure and gcp: the two shapes resolve the same metadata, which is the property the fallback exists to provide. The libs/git unit tests now use the testserver instead of their own fake, so they exercise the same modelled behavior. Removing the fallback fails the Git-CLI-folder and subdirectory cases while the /Repos case keeps passing, confirming the two paths are covered separately.
The wrapper only paired the API call with the fs.ErrNotExist normalization that FetchRepositoryInfo already does, so exporting the function directly says the same thing with less indirection and leaves FetchRepositoryInfo's body untouched. Callers that want "a missing path is not a repository" go through FetchRepositoryInfo, which the doc comment now points out.
| >>> [CLI] repos get [NUMID] -o json | ||
| { | ||
| "branch": "main", | ||
| "head_commit_id": "77e0c1a3cba9bd6d8b5c9d0e2f4a6b8c1d3e5f70", |
There was a problem hiding this comment.
Should we do the repl here?
There was a problem hiding this comment.
no, it's a fixed commit:
libs/testserver/repos.go:const fakeHeadCommitID = "77e0c1a3cba9bd6d8b5c9d0e2f4a6b8c1d3e5f70"
| >>> [CLI] repos create https://github.com/databricks/databricks-empty-ide-project.git --path /Repos/me@databricks.com/test-repo | ||
| { | ||
| "branch": "main", | ||
| "head_commit_id": "77e0c1a3cba9bd6d8b5c9d0e2f4a6b8c1d3e5f70", |
| return client | ||
| } | ||
|
|
||
| func TestFetchRepositoryInfoAPI_Repo(t *testing.T) { |
There was a problem hiding this comment.
Isn't it all covered by acceptance tests already? Or is it just to have it as a lib unit test?
There was a problem hiding this comment.
good point, cleaned this one up.
| // [git.FetchRepositoryInfo] resolves for a path. It exists so that function can | ||
| // be exercised through the CLI on both a fake and a real workspace; nothing in | ||
| // the product calls it. | ||
| func NewFetchRepositoryInfoCommand() *cobra.Command { |
There was a problem hiding this comment.
Why do we have it if nothing in product calls it? For acceptance tests? We seem to have TestFetchRepositoryInfoAPI* unit tests which covers (somewhat?) the same
There was a problem hiding this comment.
for acc tests so we can have local + cloud test and deprecate corresponding integration/ tests
## Changes The command always reads the workspace API and always prints JSON. The path it selected before had one caller passing one value, and reading .git is already covered by the TestFetchRepositoryInfoDotGit integration tests. libs/git/info_test.go keeps only what the acceptance test cannot reach: the fs.ErrNotExist contract and its normalization, a failing Repos read, and ensureWorkspacePrefix. The four cases that resolved metadata successfully were the same four the acceptance test runs against a fake and a real workspace. The fake head commit is now marked synthetic by a counting prefix, since nothing is cloned and the remote's real HEAD is unknowable. It avoids runs of three digits, which the test replacements rewrite to [NUMID]. Also switches the test.toml Env entry to dotted form, so a top-level key added below it is not silently absorbed into [Env].
## Why The real get-status reports git_info.path without the /Workspace mount prefix, whichever spelling the folder was created with, and FetchRepositoryInfo re-adds it. The fake echoed the path as created, so nothing exercised that. ## Changes The fake strips the prefix, as the API does. The golden is unchanged, since the prefix is re-added before it is printed: breaking ensureWorkspacePrefix now fails the acceptance test locally instead of only against a real workspace. libs/git/info_test.go is down to the failure of the Repos read. Resolving metadata is covered by the acceptance test against both a fake and a real workspace, and a missing path by TestFetchRepositoryInfoAPI_FromNonRepo, so the rest was duplicating coverage rather than adding it.
fault.py can inject the failure into the testserver, so this no longer needs a Go test: a local-only test asserts the command still reports the worktree root, with the metadata empty, and captures the warning the user sees. That last part the Go test did not check. libs/git/info_test.go is gone; every case it held is now covered through the CLI.
The subdirectory and omitted-git_info cases were covered by the acceptance tests through the resolved metadata, so they are gone. What is left is the part those tests read past: the object type, directory_info, and whether git_info is served at all. A wrong object_type passes the whole acceptance suite, since nothing reading git metadata looks at it.
MSYS_NO_PATHCONV also rewrites the path Git Bash passes for a helper's own
script, so fault.py could not be found on Windows:
python3.exe: can't open file 'C:\\c\\a\\cli\\cli\\acceptance\\bin\\fault.py'
The fault is registered before the variable is exported, which the CLI needs for
the /Workspace path it is given. Verified on Windows.
Why
A Git folder that has Git CLI access is a workspace DIRECTORY whose git metadata is
not stored on the workspace object, so
workspace/get-status?return_git_info=truereturns only
idandpathfor it - nobranch,head_commit_idorurl. Thosefields are declared unconditionally in the API, and
fetchRepositoryInfoAPIcopiedthem straight into
RepositoryInfo, so on DBRbundle.git.branch,.commitand.origin_urlsilently became empty.New Git folders are created this way as the
wsfs_git_clipublic preview ramps up(SDR-3545), which is what broke
TestFetchRepositoryInfoAPI_FromRepoon all clouds.It is not a propagation race: polling get-status for 90s never fills the fields in,
so there is nothing to retry.
Changes
When get-status returns a git folder id but no metadata, read it from
Repos.Get.git_info.idis the id of the Git folder root even when the queried path is asubdirectory, so both the root and subdir cases resolve. A legacy REPO folder still
answers from get-status alone and makes no extra call.
This also reverts the GCP skip from #6038 -
that skip described this same gap, which reached GCP first.
Tests
New unit tests for
fetchRepositoryInfoAPIcovering full metadata (asserting noRepos call), the fallback, a subdirectory, a failing
Repos.Get, and a path with nogit info.
integration/libs/gitpasses against aws, azure and gcp.