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

refactor: centralize git checkouts and db paths #13187

Merged
merged 1 commit into from
Dec 22, 2023

Conversation

weihanglo
Copy link
Member

What does this PR try to resolve?

This moves the logic of getting $CARGO_HOME/git/checkouts and $CARGO_HOME/.cargo/git/db to a central place.

How should we test and review this PR?

There is no functional change.

Additional information

@rustbot
Copy link
Collaborator

rustbot commented Dec 20, 2023

r? @ehuss

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added A-configuration Area: cargo config files and env vars A-git Area: anything dealing with git S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 20, 2023
Comment on lines +371 to +381
/// Gets the directory of code sources Cargo checkouts from Git bare repos
/// (`<cargo_home>/git/checkouts`).
pub fn git_checkouts_path(&self) -> Filesystem {
self.git_path().join("checkouts")
}

/// Gets the directory for all Git bare repos Cargo clones
/// (`<cargo_home>/git/db`).
pub fn git_db_path(&self) -> Filesystem {
self.git_path().join("db")
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How appropriate is it to join on a Filesystem and pass that around?

From what little I've interacted with Filesystem, it seems like its meant to be the root for (e.g. its whats lockable) which makes it feel strange to creating Filesystems for directories we don't lock (instead we lock the parent)

Copy link
Member Author

@weihanglo weihanglo Dec 21, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see it like “hey this path might be locked so be careful when manipulate stuff under the path.” registry_index_path is constructed in the same way, though this doesn't justify it is correct.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, the idea is to only interact with the filesystem using Filesystem, and never use raw Path. That helps ensure you only access files with appropriate locking. Unfortunately we're not very good with that. Whenever into_path_unlocked is used, that indicates the abstraction is leaking. It is necessary in many cases, since other libraries (like libgit2) won't take a Filesystem, but I thinkinto_path_unlocked is used a little too often. There are also many places that don't use Filesystem. For example, Layout has its own locking mechanism, and thus doesn't need it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which lock is being held for git checkouts and git dbs?

If its not the individual db's and checkouts, then this seems like the wrong API in the first place as we are saying "this is lockable" when, in reality, locking it would be the wrong decision. One idea for improving this (if that is the case) is to "lock project" and allow Filesystem.join to carry-forward the path to the lockable resource.

@ehuss
Copy link
Contributor

ehuss commented Dec 21, 2023

Thanks!

@bors r+

@bors
Copy link
Contributor

bors commented Dec 21, 2023

📌 Commit 0a4ce46 has been approved by ehuss

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 21, 2023
@bors
Copy link
Contributor

bors commented Dec 21, 2023

⌛ Testing commit 0a4ce46 with merge 60053f7...

bors added a commit that referenced this pull request Dec 21, 2023
refactor: centralize git checkouts and db paths
@bors
Copy link
Contributor

bors commented Dec 21, 2023

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Dec 21, 2023
@weihanglo
Copy link
Member Author

Rust version was updated to 1.77 hours ago rust-lang/rust#119181. I'll bump the version of Cargo today.

@weihanglo
Copy link
Member Author

Blocked on #13192

@weihanglo
Copy link
Member Author

@bors retry

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 22, 2023
@bors
Copy link
Contributor

bors commented Dec 22, 2023

⌛ Testing commit 0a4ce46 with merge 363a2d1...

@bors
Copy link
Contributor

bors commented Dec 22, 2023

☀️ Test successful - checks-actions
Approved by: ehuss
Pushing 363a2d1 to master...

@bors bors merged commit 363a2d1 into rust-lang:master Dec 22, 2023
20 checks passed
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 22, 2023
Update cargo

10 commits in 1a2666ddd14cf0a255d4ddb61c63531c259a7b39..363a2d11320faf531f6aacd1ea067c6bc08343b9
2023-12-17 17:53:53 +0000 to 2023-12-22 03:12:42 +0000
- refactor: centralize git checkouts and db paths (rust-lang/cargo#13187)
- Bump to 0.78.0; update changelog (rust-lang/cargo#13192)
- refactor: custom error types for `cargo-util-schemas` (rust-lang/cargo#13186)
- chore(deps): update rust crate handlebars to `v4.5.0` (rust-lang/cargo#13168)
- Hold the mutate exclusive lock when vendoring (rust-lang/cargo#12509)
- refactor: clean up package metadata (rust-lang/cargo#13184)
- ci: check SemVer for cargo-util-schemas on CI (rust-lang/cargo#13185)
- refactor(schemas): Pull out as `cargo-util-schemas` (rust-lang/cargo#13178)
- chore(rustfix): rename Readme.md to README.md (rust-lang/cargo#13181)
- chore(rustfix): remove useless clippy rules and fix a typo (rust-lang/cargo#13182)

r? ghost
@rustbot rustbot added this to the 1.77.0 milestone Dec 23, 2023
@weihanglo weihanglo deleted the git-paths branch December 28, 2023 02:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-configuration Area: cargo config files and env vars A-git Area: anything dealing with git S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants