Skip to content

Use libgit2 first then fallback to the git command on failure#1786

Closed
zanieb wants to merge 6 commits intomainfrom
zb/git-libgit2-fallback
Closed

Use libgit2 first then fallback to the git command on failure#1786
zanieb wants to merge 6 commits intomainfrom
zb/git-libgit2-fallback

Conversation

@zanieb
Copy link
Copy Markdown
Member

@zanieb zanieb commented Feb 20, 2024

Some necessary context in #1781.

This should be faster in the happy path but robust to less common authentication schemes. Cargo uses an environment variable to toggle instead of this "fallback" approach. I'm hesitant to require that from our users.

The downside of this is that we will always retry with the git command even if the failure is not related to authentication. This could result in confusing logs and is, of course, slower. In practice, I think the error messages from the git command are better as they give you something reproducible to test with and have more context, the libgit2 errors are very opaque.

I've also added a rough way to reuse the discovered correct strategy for fetching (df4382f) we could extend this to avoid unnecessary calls to libgit2 in the future.

We can definitely say this is overkill and just drop fetching with libgit2 entirely in favor of the git CLI. We could also only use the git command if we detect authentication via some heuristic?

Comment thread crates/uv-git/src/source.rs Outdated
Comment thread crates/uv-git/src/git.rs Outdated
@zanieb zanieb force-pushed the zb/git-libgit2-fallback branch from 17e924d to c3710ac Compare February 21, 2024 00:01
@zanieb zanieb force-pushed the zb/git-libgit2-fallback branch from c3710ac to e28245e Compare February 21, 2024 00:13
@zanieb zanieb added the internal A refactor or improvement that is not user-facing label Feb 21, 2024
Comment thread crates/uv/tests/pip_install.rs
@zanieb
Copy link
Copy Markdown
Member Author

zanieb commented Feb 21, 2024

Currently a big problem here is that libgit2 will retry several times before we try git.

Base automatically changed from zb/git-auth-ssh to main February 21, 2024 18:44
zanieb added a commit that referenced this pull request Feb 21, 2024
…st SSH support (#1781)

Closes #1775
Closes #1452
Closes #1514
Follows #1717

libgit2 does not support host names with extra identifiers during SSH
lookup (e.g. [`github.com-some_identifier`](

https://docs.github.com/en/authentication/connecting-to-github-with-ssh/managing-deploy-keys#using-multiple-repositories-on-one-server))
so we use the `git` command instead for fetching. This is required for
`pip` parity.

See the [Cargo
documentation](https://doc.rust-lang.org/nightly/cargo/reference/config.html#netgit-fetch-with-cli)
for more details on using the `git` CLI instead of libgit2. We may want
to try to use libgit2 first in the future, as it is more performant
(#1786).

We now support authentication with:

```
git+ssh://git@<hostname>/...
git+ssh://git@<hostname>-<identifier>/...
```

Tested with a deploy key e.g.

```
cargo run -- \
    pip install uv-private-pypackage@git+ssh://git@github.com-test-uv-private-pypackage/astral-test/uv-private-pypackage.git \
    --reinstall --no-cache -v
```

and

```
cargo run -- \
    pip install uv-private-pypackage@git+ssh://git@github.com/astral-test/uv-private-pypackage.git \
    --reinstall --no-cache -v     
```

with a ssh config like

```
Host github.com
        Hostname github.com
        IdentityFile=/Users/mz/.ssh/id_ed25519

Host github.com-test-uv-private-pypackage
        Hostname github.com
        IdentityFile=/Users/mz/.ssh/id_ed25519
```

It seems quite hard to add test coverage for this to the test suite, as
we'd need to add the SSH key and I don't know how to isolate that from
affecting other developer's machines.
This avoids redoing the fast-path and such and only actually retries the fetch itself

# Conflicts:
#	crates/uv-git/src/git.rs

# Conflicts:
#	crates/uv-git/src/git.rs
# Conflicts:
#	crates/uv/tests/pip_install.rs
@zanieb zanieb force-pushed the zb/git-libgit2-fallback branch from acc4ff3 to 34f8999 Compare February 21, 2024 19:00
@zanieb zanieb closed this Mar 4, 2024
@zanieb
Copy link
Copy Markdown
Member Author

zanieb commented Mar 4, 2024

To be explored another day..

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

internal A refactor or improvement that is not user-facing

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant