gitoxide integration: fetch by Byron · Pull Request #11448 · rust-lang/cargo

Byron · 2022-12-01T18:00:12Z

This PR is the first step towards resolving #1171.

In order to get there, we integrate gitoxide into cargo in such a way that one can control its usage in nightly via -Zgitoxide or Zgitoxide=<feature>[,featureN].

Planned features are:

fetch - all fetches are done with gitxide (this PR)
shallow_index - the crates index will be a shallow clone (planned)
shallow_deps - git dependencies will be a shallow clone (planned)
checkout - plain checkouts with gitoxide (planned)

The above list is a prediction and might change as we understand the requirements better.

Testing and Transitioning

By default, everything stays as is. However, relevant tests can be re-runwith gitoxide using

__CARGO_USE_GITOXIDE_INSTEAD_OF_GIT2=1 cargo test git

There are about 200 tests with 'git' in their name and I plan to enable them one by one. That way the costs for CI stay managable (my first measurement with one test was 2min 30s), while allowing to take one step at a time.

Custom tests shall be added once we realize that more coverage is needed.

That way we should be able to maintain running git2 and gitoxide side by side until we are willing to switch over to gitoxide entirely on stable cargo. Then turning on git2 might be a feature toggle for a while until we finally remove it from the codebase.

Please see the above paragraph as invitation for discussion, it's merely a basis to explore from and improve upon.

Tasks

Postponed Tasks

I suggest to go breadth-first and implement the most valuable features first, and then aim for a broad replacement of git2. What's left is details and improved compatibility with the git2 implementation that will be required once gitoxide should become the default implementation on stable to complete the transition.

built-in support for serving the file protocol (i.e. without using git). Simple cases like clone can probably be supported quickly, fetch needs more work though due to negotiation.
SSH name fallbacks via a native (probably ~~libssh~~ (avoid LGPL) libssh2 based) transport. Look at this issue for some history.
additional tasks from this tracking issue

Proposed Workflow

I am now using stacked git to keep commits meaningful during development. This will also mean that before reviews I will force-push a lot as changes will be bucketed into their respective commits.

Once review officially begins I will stop force-pushing and create small commits to address review comments. That way it should be easier to understand how things change over time.

Those review-comments can certainly be squashed into one commit before merging.

Please let me know if this is feasible or if there are other ways of working you prefer.

Development notes

unrelated: this line refers to an issue that has since been resolved in curl.
Additional tasks related to a correct fetch implementation are collected in this tracking issue. These affect how well the HTTP transport can be configured, needs work
authentication is quite complex and centred around making SSH connections work. This feature is currently the weakest in gitoxide as it simply uses ssh (the program) and calls it a day. No authentication flows are supported there yet and the goal would be to match git there at least (which it might already do by just calling ssh). Needs investigation. Once en-par with git I think cargo can restart the whole fetch operation to try different user names like before.
- the built-in ssh-program based transport can now understand permission-denied errors, but the capability isn't used after all since a builtin ssh transport is required.
It would be possible to implement git::Progress and just ignore most of the calls, but that's known to be too slow as the implementation assumes a Progress::inc() call is as fast as an atomic increment and makes no attempt to reduce its calls to it.
learning about a way to get custom traits in thiserror could make spurious error checks nicer and less error prone during maintenance. It's not a problem though.
I am using RUSTFLAGS=--cfg to influence the entire build and unit-tests as environment variables didn't get through to the binary built and run for tests.

Questions

The way gitoxide is configured the user has the opportunity to override these values using more specific git options, for example using url specific http settings. This looks like a feature to me, but if it's not gitoxide needs to provide a way to disable applying these overrides. Please let me know what's desired here - my preference is to allow overrides.
gitoxide currently opens repositories similar to how git does which respects git specific environment variables. This might be a deviation from how it was before and can be turned off. My preference is to see it as a feature.

Prerequisite PRs

make util::Progress thread-safe as prerequisite of #11448 #11602

rustbot · 2022-12-01T18:00:17Z

r? @epage

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

Byron · 2022-12-05T14:50:26Z

Very early results for the MVP of fetch (a lot is missing, but it works already).

78s cargo (stable)
36s cargo (nightly) (2.2x faster)

And for additional reference:

32s git (2.4x faster)
31s gix (2.5x faster)

Measurements

Gix

❯ gix clone --bare https://github.com/rust-lang/crates.io-index  crates-index
 14:36:44 read pack done 299.8MB in 27.80s (10.8MB/s)
 14:36:44  indexing 928260 objects were resolved into 464130 objects during thin-pack resolution
 14:36:44  indexing done 464.1k objects in 27.75s (16.7k objects/s)
 14:36:44 decompressing done 1.6GB in 27.75s (57.0MB/s)
 14:36:47     Resolving done 464.1k objects in 2.54s (183.1k objects/s)
 14:36:47      Decoding done 17.0GB in 2.54s (6.7GB/s)
 14:36:47 writing index file done 13.0MB in 0.02s (748.1MB/s)
 14:36:47  create index file done 464.1k objects in 30.36s (15.3k objects/s)
+refs/heads/*:refs/remotes/origin/*
        a44803046c31473544456b8d9ea089a868da2691 refs/heads/master -> refs/remotes/origin/master [new]

prodash ( main) [$?] took 31s

`cargo` (stable)

❯ cargo fetch
warning: file found to be present in multiple build targets: /Users/byron/dev/github.com/Byron/prodash/examples/dashboard.rs
    Updating crates.io index

prodash ( main) [$?] took 1m18s

`git`

❯ git clone --bare https://github.com/rust-lang/crates.io-index  crates-index
Cloning into bare repository 'crates-index'...
remote: Enumerating objects: 464130, done.
remote: Counting objects: 100% (55/55), done.
remote: Compressing objects: 100% (55/55), done.
remote: Total 464130 (delta 29), reused 26 (delta 0), pack-reused 464075
Receiving objects: 100% (464130/464130), 285.87 MiB | 11.45 MiB/s, done.
Resolving deltas: 100% (320065/320065), done.

prodash ( main) [$?] took 32s

`cargo` (nightly)

❯ /Users/byron/dev/github.com/rust-lang/cargo/target/release/cargo -Z gitoxide=fetch fetch
warning: file found to be present in multiple build targets: /Users/byron/dev/github.com/Byron/prodash/examples/dashboard.rs
    Updating crates.io index

prodash ( main) [$?] took 36s

bors · 2022-12-29T17:53:59Z

☔ The latest upstream changes (presumably #10771) made this pull request unmergeable. Please resolve the merge conflicts.

src/cargo/core/features.rs

src/doc/contrib/src/tests/running.md

tests/testsuite/bad_config.rs

src/cargo/sources/git/oxide.rs

src/cargo/lib.rs

src/cargo/sources/git/oxide.rs

src/cargo/core/features.rs

epage · 2023-02-10T17:58:45Z

Will admit at this point in the review, my eyes are starting to glaze over the more complex parts (progress), particularly

@weihanglo could you serve as a second pair of eyes on that?

Byron · 2023-02-10T20:47:05Z

Thanks for the swift review! I am working on resolving the issues one by one, probably creating a commit for each of them while pushing from time to time while adding replies to the conversations. Apologies for the notifications that this might generate - I wish there was a way to bundle that but don't know of any.

I am collecting a comments on conversations on what GitHub calls 'outdated' items as it won't let me answer there directly.

commented out dbg!()
- That has been removed already, and since I would squash in the end I hope I can avoid rewriting this commit. @epage What should I do?
about making explicit how a TODO was resolved
- I have trouble finding context for this, but know the progress handling was fundamentally altered in 431d89c. It belonged to the 'bring down progress thread before continuing to prevent multiple progress bars to clobber each other' in my own TODO list that was generated from the first review notes. @epage Sorry for the hassle. I think what happened here is that I have rewritten part of the history and added two new commits on top, which most certainly is responsible making it difficult to figure out where comments were addressed (for everyone).
calling to_ssl_version for an error message
- That's a great catch, thanks so much. I blindly ported the code over to the 'config tree' in gitoxide without realizing that just-in-time error handling is now done by validate_assignment_fmt(…). Fixed in 1994ad2.

I probably won't manage to address all comments today but hope to get it done by Monday.

Review Report

All comments have been addressed with a commit except for the following, which have received a response instead.

If I am not missing anything, you can re-check, @epage. Thanks for your patience - I am rarely as fast as I hope to be.

bors · 2023-02-11T19:02:36Z

☔ The latest upstream changes (presumably #11699) made this pull request unmergeable. Please resolve the merge conflicts.

.github/workflows/main.yml

bors · 2023-02-14T11:06:12Z

☔ The latest upstream changes (presumably #11646) made this pull request unmergeable. Please resolve the merge conflicts.

Byron · 2023-02-14T11:21:14Z

Regarding the merge-conflict: I have already rebased onto master locally but am holding back the commits to not break the commit references in the comments.

weihanglo

Thank you @Byron and sorry I haven't got enough time to go deeper. This review is only on the surface of styling and git/http config.

src/cargo/core/features.rs

tests/testsuite/git_auth.rs

src/cargo/ops/registry.rs

src/cargo/sources/git/oxide.rs

Byron · 2023-02-17T16:17:26Z

Thanks @weihanglo for the review! I think I have addressed all issues, with only one question remaining here.

weihanglo

Finished the review of the progress bar part. Looking great in general! At least #9444 will be resolved after merge.

Just post the result of cargo tree -p prodash here for people to check the dependency tree of the crate gathering progress information. Cargo may not need some of those features (e.g. human_format) but it is fine with me btw.

cargo tree -p prodash

prodash v23.0.0
├── bytesize v1.1.0
├── dashmap v5.4.0
│   ├── cfg-if v1.0.0
│   ├── hashbrown v0.12.3
│   ├── lock_api v0.4.9
│   │   └── scopeguard v1.1.0
│   │   [build-dependencies]
│   │   └── autocfg v1.1.0
│   ├── once_cell v1.17.1
│   └── parking_lot_core v0.9.7
│       ├── cfg-if v1.0.0
│       ├── libc v0.2.139
│       └── smallvec v1.10.0
├── human_format v1.0.3
└── parking_lot v0.11.2
    ├── instant v0.1.12
    │   └── cfg-if v1.0.0
    ├── lock_api v0.4.9 (*)
    └── parking_lot_core v0.8.6
        ├── cfg-if v1.0.0
        ├── instant v0.1.12 (*)
        ├── libc v0.2.139
        └── smallvec v1.10.0

src/cargo/sources/git/oxide.rs

weihanglo · 2023-02-21T14:12:34Z

src/cargo/sources/git/utils.rs

+                        let url_for_authentication = &mut *url_for_authentication;
+                        let remote = repo
+                            .remote_at(orig_url)?
+                            .with_refspecs(


Should we call with_fetch_tags here to align what Cargo does now?

cargo/src/cargo/sources/git/utils.rs

Lines 881 to 883 in ddfe87f

if tags {

opts.download_tags(git2::AutotagOption::All);

}

That's a great catch! Indeed downloading included tags is the default, but users could override this setting with their git configuration, so better set it explicitly. It's done in ef04a36 (not pushed yet, didn't want to destroy previous references after rebase).

Is gix::remote::fetch::Tags::Included equivalent to git2::AutotagOption::All? I think you're correct. It makes sense not to fetch unreachable refs, though I don't know if anyone depends on the behaviour.

That's an amazing catch! And I don't even know why I put this there but can imagine that I was so focussed on the Included case after implementing it, that I put it there without re-checking how it's used. c585dc4 offers an alternative implementation which should match the one in git2.

bors · 2023-02-22T00:20:31Z

☔ The latest upstream changes (presumably #11725) made this pull request unmergeable. Please resolve the merge conflicts.

Byron · 2023-02-25T16:13:43Z

weihanglo

I think I've finished the review of the credential part. That means the review on my side of this PR has been done 😆.

Great work @Byron!

weihanglo · 2023-02-27T15:02:35Z

src/cargo/sources/git/utils.rs

+                            .to_owned();
+                        let connection =
+                            remote.connect(gix::remote::Direction::Fetch, &mut progress)?;
+                        let mut authenticate = connection.configured_credentials(url)?;


with_authentication prioritize SSH over credential.helper. Maybe I understand it wrongly but it seems that the same rule doesn't hold in gix default authentication callback.

In addition, I have no idea if it works but git::Cred::default() seems to respect NTLM and Kerberos. Do we have that via the default credential helper?

It's totally reasonable to follow what git does, yet this is undoubtedly a breaking change if the old rule doesn't hold. I do feel when we stabilize gioxide feature in Cargo, we can advertise it follows what git does and ditch custom rules.

Indeed, by nature these implementations will be different, even though I hope ultimately they have the same result. Due to a lack of tests (due to the incredible difficulty of setting them up) it's not easy to say what behaviour would actually occour and I am not able to run these test cases by hand either.

Due to gitoxide being by default like git but flexible enough to probably be like git2 as well, I believe we can get close to what git2 does once it's a bit clearer what that is.

In addition, I have no idea if it works but git::Cred::default() seems to respect NTLM and Kerberos. Do we have that via the default credential helper?

That depends on the credentials helper that is ultimately invoked, but I don't know. It seems these are specifically added by git2 and I don't know what it takes to add them to gitoxide - it mostly delegates to the credentials helper and that seems like the right thing to do.

In any case, I think it's worth keeping an eye on this topic and get to the trial stage to have users report the cases they need to work, and then we make them work as needed. If ultimately that means to setup the credential helper correctly, that seems like a fair thing to do as it's something most people have to do once for git anyway.

Byron · 2023-03-01T08:36:25Z

Once again, I believe I have addressed all review notes :). Here is an overview of the broad strokes taken, and many commits have been added for the individual steps. a4492ce contains an overview of how the dependency tree was cleared.

Thanks everybody :).

weihanglo

Thank you again, @Byron! I believe this PR is good to merge. Some regressions about progress bar could be resolved afterward.

@epage, do you have any concern? I don't feel like the merge needs an FCP but if you like.

Just notice that gix depends on crossbeam-utils and crossbeam-channel. For channel (1.67) and scoped thread (1.63) they have been ported to std in recent. We could track them and perhaps someday remove them from gix.

weihanglo · 2023-03-01T11:16:32Z

src/cargo/sources/git/oxide.rs

+            let (rate, unit) = human_readable_bytes(counter.rate() as u64);
+            let msg = format!(", {rate:.2}{unit}/s");
+
+            progress_bar.tick(objects, total_objects, &msg)?;


(Not really a blocker. We can fix this in follow-ups I believe.)

There are still some subtle changes/regressions regarding the progress bar.

In the old implementation, we didn't split download and resolving deltas into two steps; they shared the one progress. In this PR we have different upper bounds for each task. Is it possible to follow the old behavior?

It seems like the delta_index_objects task always gets stuck in 50%. Is that a bug in gix or just we display the wrong thing?

I don't mind if we eventually get two progresses but the progress reports should be correct and make sense to users.

Here is a record you can take as a reference: https://asciinema.org/a/563786?t=3

A great catch, I just (tried) to go by keeping the code similar and called it a day, but didn't actually compare if it is the same.

In the old implementation, we didn't split download and resolving deltas into two steps; they shared the one progress. In this PR we have different upper bounds for each task. Is it possible to follow the old behavior?

That should be possible, I will give it a another shot.

It seems like the delta_index_objects task always gets stuck in 50%. Is that a bug in gix or just we display the wrong thing?

I think it's a display issue, as it doesn't get stuck when cloning with gix as far as I can tell. Maybe it's fixed as soon as I take a closer look at the above issue.

Here is a record you can take as a reference: https://asciinema.org/a/563786?t=3

Whoop whoop, that will be so helpful, thanks a lot!

I will be back once this is fixed, with a video for comparison :).

Byron · 2023-03-01T13:09:22Z

Thanks so much! I will definitely take a look at the progress bar issue now that I know it exists.

Just notice that gix depends on crossbeam-utils and crossbeam-channel. For channel (1.67) and scoped thread (1.63) they have been ported to std in recent. We could track them and perhaps someday remove them from gix.

A great catch indeed! With an MSRV of 1.64 I have just ditched the direct crossbeam-utils dependency in favor of the (even better) std::thread::scope(). For crossbeam-channel to fall though, I'd need single-produce-multi-consumer channels, which I couldn't find in the latest std docs either. Could you give me a hint how to do that within the confines of std? Then I can create a tracking issue and link it here. Thank you.

weihanglo · 2023-03-01T13:39:52Z

For crossbeam-channel to fall though, I'd need single-produce-multi-consumer channels, which I couldn't find in the latest std docs either. Could you give me a hint how to do that within the confines of std? Then I can create a tracking issue and link it here. Thank you.

Forget it. I was wrong. Only mpsc was ported to std 😅.

epage · 2023-03-01T15:20:47Z

@epage, do you have any concern? I don't feel like the merge needs an FCP but if you like.

I'm ready to merge. Just waiting to hear back if we should block the progress bar improvements on it.

bors · 2023-03-01T18:43:14Z

☔ The latest upstream changes (presumably #11764) made this pull request unmergeable. Please resolve the merge conflicts.

Byron · 2023-03-02T11:31:06Z

@weihanglo and @epage and I agree it's for the better to merge right away and provide fixes to the progress bar (and everything else) in follow-up PRs accordingly. Thus I have now squashed all commits, hopefully with a useful commit-message as well.

Thanks everyone for lending your expertise to help me get this far. gitoxide is already so much better thanks to it, and I look forward to seeing where this road leads to.

This allows to use `gitoxide` for all fetch operations, boosting performance for fetching the `crates.io` index by a factor of 2.2x, while being consistent on all platforms. For trying it, nightly builds of `cargo` can specify `-Zgitoxide=fetch`. It's also possible to set the `__CARGO_USE_GITOXIDE_INSTEAD_OF_GIT2=1` environment variable (value matters), which is when `-Zgitoxide=none` can be used to use `git2` instead. Limitations ----------- Note that what follows are current shortcomings that will be addressed in future PRs. - it's likely that authentication around the `ssh` protocol will work differently in practice as it uses the `ssh` program. - clones from `file://` based crates indices will need the `git` binary to serve the locatl repository. - the progress bar shown when fetching doesn't work like the orgiinal, but should already feel 'faster'.

weihanglo · 2023-03-02T12:12:45Z

This is soon to be merged. @Byron wrote a decent commit message cfffda9, calling out important stuff to follow up. Let me repost them here:

Limitations

Note that what follows are current shortcomings that will be addressed in future PRs.

it's likely that authentication around the ssh protocol will work differently in practice
as it uses the ssh program.

clones from file:// based crates indices will need the git binary to serve the locatl repository.

the progress bar shown when fetching doesn't work like the orgiinal, but should already feel 'faster'.

One thing also needs attentions is the behavior change of git credential helper mentioned in #11448 (comment). -Zgitoxide follows what git CLI does, but it is better mentioning the change when calling for testing.

Thank you to everyone involved in this pull request!

@bors r+

bors · 2023-03-02T12:12:47Z

📌 Commit cfffda9 has been approved by weihanglo

It is now in the queue for this repository.

bors · 2023-03-02T12:12:55Z

⌛ Testing commit cfffda9 with merge 7cba527...

bors · 2023-03-02T13:03:16Z

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

rustbot assigned epage Dec 1, 2022

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 1, 2022

Byron force-pushed the integrate-gitoxide branch from 0c9d0c6 to a305af5 Compare December 1, 2022 18:00

Byron mentioned this pull request Dec 1, 2022

[integration] Shallow clones for cargo GitoxideLabs/gitoxide#449

Open

9 tasks

Byron force-pushed the integrate-gitoxide branch 2 times, most recently from 62af2de to 1759263 Compare December 5, 2022 14:50

Byron force-pushed the integrate-gitoxide branch 18 times, most recently from 2552fec to e8c4201 Compare December 11, 2022 20:46

Byron force-pushed the integrate-gitoxide branch from e8c4201 to fb11423 Compare December 15, 2022 19:34

Byron force-pushed the integrate-gitoxide branch 2 times, most recently from b903e2d to 759c264 Compare December 26, 2022 09:03

epage reviewed Feb 10, 2023

View reviewed changes

fbstj reviewed Feb 13, 2023

View reviewed changes

.github/workflows/main.yml Show resolved Hide resolved

weihanglo reviewed Feb 16, 2023

View reviewed changes

weihanglo mentioned this pull request Feb 17, 2023

Unsupported protocol with sparse registry #11726

Closed

weihanglo reviewed Feb 21, 2023

View reviewed changes

epage mentioned this pull request Feb 24, 2023

chore(ci): Enforce cargo-deny in CI #11761

Merged

weihanglo reviewed Feb 27, 2023

View reviewed changes

weihanglo approved these changes Mar 1, 2023

View reviewed changes

weihanglo mentioned this pull request Mar 2, 2023

Update cargo rust-lang/rust#108665

Merged

Byron mentioned this pull request Mar 9, 2023

Tracking Issue for -Zgitoxide #11813

Open

15 tasks

konstin mentioned this pull request Oct 26, 2023

Support VCS dependencies astral-sh/uv#202

Closed

Conversation

Byron commented Dec 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Testing and Transitioning

Tasks

Postponed Tasks

Proposed Workflow

Development notes

Questions

Prerequisite PRs

Uh oh!

rustbot commented Dec 1, 2022

Uh oh!

Byron commented Dec 5, 2022

Gix

cargo (stable)

git

cargo (nightly)

Uh oh!

bors commented Dec 29, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

epage commented Feb 10, 2023

Uh oh!

Byron commented Feb 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review Report

Uh oh!

bors commented Feb 11, 2023

Uh oh!

Uh oh!

bors commented Feb 14, 2023

Uh oh!

Byron commented Feb 14, 2023

Uh oh!

weihanglo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Byron commented Feb 17, 2023

Uh oh!

weihanglo left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bors commented Feb 22, 2023

Uh oh!

Byron commented Feb 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

review tasks

Out of scope

Uh oh!

weihanglo left a comment

Choose a reason for hiding this comment

Uh oh!

Byron commented Dec 1, 2022 •

edited

Loading

`cargo` (stable)

`git`

`cargo` (nightly)

Byron commented Feb 10, 2023 •

edited

Loading

weihanglo left a comment •

edited

Loading

Byron commented Feb 25, 2023 •

edited

Loading