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

Implementation of shallow libgit2 fetches behind an unstable flag #13252

Merged
merged 5 commits into from
Jan 12, 2024

Conversation

connorworley
Copy link
Contributor

This is my first contribution, so guidance is appreciated.

Fixes #1171 by moving the shallow-index and shallow-deps aspects of -Zgitoxide to a new -Zgit unstable flag.

The only change in interaction with libgit2 happens in src/cargo/sources/git/utils.rs, where we set the depth fetch option if applicable.

Shallow fetch tests for gitoxide continue to pass, but libgit2 is harder to test as it silently ignores the depth option for local fetches. I would love any ideas on how to test it in a lightweight way or whether it's OK to wait for an upstream fix.

@rustbot
Copy link
Collaborator

rustbot commented Jan 5, 2024

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @weihanglo (or someone else) soon.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

  • @rustbot author: the review is finished, PR author should check the comments and take action accordingly
  • @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

@rustbot rustbot added A-documenting-cargo-itself Area: Cargo's documentation A-git Area: anything dealing with git A-registries Area: registries A-unstable Area: nightly unstable support S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 5, 2024
@connorworley connorworley marked this pull request as ready for review January 5, 2024 16:32
Copy link
Member

@weihanglo weihanglo left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. Looks pretty neat!

Shallow fetch tests for gitoxide continue to pass, but libgit2 is harder to test as it silently ignores the depth option for local fetches. I would love any ideas on how to test it in a lightweight way or whether it's OK to wait for an upstream fix.

Do you have the issue link so we can track it?

src/cargo/sources/git/mod.rs Show resolved Hide resolved
src/cargo/core/features.rs Show resolved Hide resolved
@connorworley
Copy link
Contributor Author

Thanks for the PR. Looks pretty neat!

Shallow fetch tests for gitoxide continue to pass, but libgit2 is harder to test as it silently ignores the depth option for local fetches. I would love any ideas on how to test it in a lightweight way or whether it's OK to wait for an upstream fix.

Do you have the issue link so we can track it?

libgit2/libgit2#6634

@connorworley connorworley force-pushed the shallow_libgit2_fetches branch from 9a6dc0f to 5eb2899 Compare January 11, 2024 08:11
@weihanglo weihanglo mentioned this pull request Jan 11, 2024
3 tasks
Copy link
Member

@weihanglo weihanglo left a comment

Choose a reason for hiding this comment

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

I've tested it manually, libgit2 shallow clone seems to work for both registries and git dependencies 👍🏾.

Just one concern, maybe not really relevant. When testing -Zgitoxide I used to run git rev-list --all to check what is inside a shallow repo for multiple subsequent clones. For gitoxide it outputs the two commits we've fetched, but with libgit2 we've an error.

# Cargo.toml
[package]
name = "foo"
edition = "2021"

[dependencies]
tokio = { git = "https://github.com/tokio-rs/tokio.git", rev = "3275cfb638fe6cac6b0bbb1f60ee59eb499f6c2a" }
tokio2 = { package = "tokio", git = "https://github.com/tokio-rs/tokio.git", rev = "84c5674c601dfc36ab417ff0ec01763c2dd30a5c" }
$ cd ~/.cargo/git/db/tokio-377c595163f99a10-shallow/
$ git rev-list -all

# gitoxide
84c5674c601dfc36ab417ff0ec01763c2dd30a5c
3275cfb638fe6cac6b0bbb1f60ee59eb499f6c2a

# git2
84c5674c601dfc36ab417ff0ec01763c2dd30a5c
error: Could not read 9780bf491f0a69986f9f35b9c6a81ac951356aff
fatal: Failed to traverse parents of commit 3275cfb638fe6cac6b0bbb1f60ee59eb499f6c2a

Funniest, I've tried Git CLI git fetch --depth 1. It retrieved nothing without an error.

$ git init --bare foo && foo
$ git fetch https://github.com/tokio-rs/tokio.git --depth 1 3275cfb638fe6cac6b0bbb1f60ee59eb499f6c2a
$ git fetch https://github.com/tokio-rs/tokio.git --depth 1 84c5674c601dfc36ab417ff0ec01763c2dd30a5c
$ git rev-list --all
# nothing

Besides, git CLI, libgit2, and gitoxide all got different packfiles and indexes for the subsequent fetch on 84c5674c601dfc36ab417ff0ec01763c2dd30a5c. I don't really know if it is a bug or just an implementation detail.

That said, this looks good to me. We could setup some minimal tests with docker or something lighweight in follow-ups.

src/doc/src/reference/unstable.md Outdated Show resolved Hide resolved
src/doc/src/reference/unstable.md Outdated Show resolved Hide resolved
@weihanglo
Copy link
Member

Although there is a CLI option change for an unstable feature in this PR, it should pretty easy to adapt to the new interface. Hence, I decide not to do a FCP and just merge it as-is.

@bors r+


If you used -Zgitoxide previously, here is how you can migrate to the new -Zgit + -Zgitoxide.

  • -Zgitoxide-Zgitoxide -Zgit
  • -Zgitoxide=fetch,shallow-deps-Zgitoxide=fetch -Zgit=shallow-deps
  • -Zgitoxide=fetch,shallow-index -Zgitoxide=fetch -Zgit=shallow-index
  • No gitoxide at all but libgit2 shallow clones→ -Zgit

@bors
Copy link
Contributor

bors commented Jan 12, 2024

📌 Commit 426b9e3 has been approved by weihanglo

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 Jan 12, 2024
@bors
Copy link
Contributor

bors commented Jan 12, 2024

⌛ Testing commit 426b9e3 with merge bf27f90...

@bors
Copy link
Contributor

bors commented Jan 12, 2024

☀️ Test successful - checks-actions
Approved by: weihanglo
Pushing bf27f90 to master...

@bors bors merged commit bf27f90 into rust-lang:master Jan 12, 2024
20 checks passed
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 13, 2024
Update cargo

8 commits in 3e428a38a34e820a461d2cc082e726d3bda71bcb..84976cd699f4aea56cb3a90ce3eedeed9e20d5a5
2024-01-09 20:46:36 +0000 to 2024-01-12 15:55:43 +0000
- fix(resolver): do not panic when sorting empty summaries (rust-lang/cargo#13287)
- Implementation of shallow libgit2 fetches behind an unstable flag (rust-lang/cargo#13252)
- Add documentation entry for unstable `--output-format` flag (rust-lang/cargo#13284)
- doc: add `public` info in `cargo-add` man page. (rust-lang/cargo#13272)
- More docs on prerelease compat (rust-lang/cargo#13286)
- Add unstable `--output-format` option to  `cargo rustdoc` (rust-lang/cargo#12252)
- feat: Add `rustc` style errors for manifest parsing (rust-lang/cargo#13172)
- Document why `du` function uses mutex (rust-lang/cargo#13273)

r? ghost
@rustbot rustbot added this to the 1.77.0 milestone Jan 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-documenting-cargo-itself Area: Cargo's documentation A-git Area: anything dealing with git A-registries Area: registries A-unstable Area: nightly unstable support 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.

Allow specifying shallow clones in .config
5 participants