Skip to content
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

Remove ReadonlyRepo::repo_path #4355

Merged
merged 6 commits into from
Sep 7, 2024
Merged

Remove ReadonlyRepo::repo_path #4355

merged 6 commits into from
Sep 7, 2024

Conversation

martinvonz
Copy link
Member

Checklist

If applicable:

  • I have updated CHANGELOG.md
  • I have updated the documentation (README.md, docs/, demos/)
  • I have updated the config schema (cli/src/config-schema.json)
  • I have added tests to cover my changes

@martinvonz
Copy link
Member Author

Looks like I messed up canonicalization. Will look into it later

@martinvonz
Copy link
Member Author

Looks like I messed up canonicalization. Will look into it later

That should be done now. I added canonicalization in TestBackend.

lib/src/repo.rs Outdated Show resolved Hide resolved
lib/testutils/src/test_backend.rs Outdated Show resolved Hide resolved
lib/tests/test_git.rs Outdated Show resolved Hide resolved
lib/testutils/src/lib.rs Outdated Show resolved Hide resolved
lib/src/workspace.rs Show resolved Hide resolved
lib/tests/test_init.rs Show resolved Hide resolved
…epo`

I'd like to remove `ReadonlyRepo::repo_path()` since it doesn't make
sense when the repo is stored in a database.
`RepoLoader` can be used on a server. You would then call
`RepoLoader::new()`. Let's clarify what `init()` does by renaming it
and documenting it.

I think `WorkspaceLoader`, OTOH, is only useful for loading a
workspace from disk. I also documented that.
In `TestBackend`, we simply use the path as key to look up the storage
in memory. That means we can't rely on the file system to look find
the same path given two different representaitons of the same path. We
therefore need to canonicalize the paths if we want to allow the
caller to pass in different paths for the same real path.
As I mentioned in recent patches, I'm about to remove
`ReadonlyRepo::repo_path()`.
When loading repos on a server, there is no repo path. We currently
use a placeholder at Google. This patch will let us not do that.

I think we can remove the paths from `Workspace` too, but I'll leave
that for later, if at all.
@martinvonz martinvonz enabled auto-merge (rebase) September 7, 2024 19:12
@martinvonz martinvonz merged commit 4fb7dba into main Sep 7, 2024
31 checks passed
@martinvonz martinvonz deleted the push-zmxutxospqvz branch September 7, 2024 19:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants