avoid fetching an exact, cached commit, even if it isn't locked

[oconnor663]

This is a rough draft of one approach we might take to fixing #13513

Here's a complete repro of the bug (arguably) in that ticket:

$ cd `mktemp -d`
$ cat << EOF > requirements.txt                                                                           
feedparser @ git+https://github.com/kurtmckee/feedparser@6c2a412a1569303c6c02d82ef38c08e19f81315e
EOF
$ uv venv
...
$ uv pip install -r requirements.txt 2>&1 | cat
   Updating https://github.com/kurtmckee/feedparser (6c2a412a1569303c6c02d82ef38c08e19f81315e)
    Updated https://github.com/kurtmckee/feedparser (6c2a412a1569303c6c02d82ef38c08e19f81315e)
Resolved 7 packages in 12ms
Installed 7 packages in 7ms
...
$ uv pip install -r requirements.txt 2>&1 | cat
   Updating https://github.com/kurtmckee/feedparser (6c2a412a1569303c6c02d82ef38c08e19f81315e)
    Updated https://github.com/kurtmckee/feedparser (6c2a412a1569303c6c02d82ef38c08e19f81315e)
Resolved 7 packages in 18ms
Audited 7 packages in 0.02ms

The problem is that we keep fetching the repo every time, even though this commit is definitely in cache. With the changes in this PR, the extra fetch is gone:

$ uv pip install -r requirements.txt 2>&1 | cat
Resolved 7 packages in 15ms
Audited 7 packages in 0.07ms

This draft PR notices that the ref we're trying to fetch looks exactly like a commit, and it checks (rev-parse) whether that commit is already present in the cached repo and interprets it as a locked (precise) ref if so. The implementation is kind of sloppy as is, and it should probably be factored into smaller functions out and also tested, but I need feedback on the overall approach.

Another approach we could take here could be to change our interpretation of these looks-like-a-commit refs such that we always assume they're commits at parse time. (In other words, mark the with precise OIDs as soon as we parse them.) If we did that, we'd probably want to add checks to make sure that we catch it if that assumption is ever violated. It looks like we used to behave more like this, but then we replaced that with the current behavior, and if we want to go this way I'll need to make sure I understand exactly why we did that.

@ibraheemdev would you have time to take a look at this? Also cc @charliermarsh and @zanieb.

[oconnor663]

Hmm, I'm seeing this test failure in CI (weirdly only on Ubuntu?):

        FAIL [   2.359s] uv::it sync::sync_script
──── STDOUT:             uv::it sync::sync_script

running 1 test
━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ Snapshot Summary ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
Snapshot: sync_script-5
Source: crates/uv/tests/it/sync.rs:8128
────────────────────────────────────────────────────────────────────────────────
Expression: snapshot
────────────────────────────────────────────────────────────────────────────────
-old snapshot
+new results
────────────┬───────────────────────────────────────────────────────────────────
    1     1 │ exit_code: 2
    2     2 │ ----- stdout -----
    3     3 │ 
    4     4 │ ----- stderr -----
    5       │-Using script environment at: [CACHE_DIR]/environments-v2/script-[HASH]
          5 │+Recreating script environment at: [CACHE_DIR]/environments-v2/script-[HASH]
    6     6 │ error: `uv sync --locked` requires a script lockfile; run `uv lock --script script.py` to lock the script
────────────┴───────────────────────────────────────────────────────────────────
Stopped on the first failure. Run `cargo insta test` to run all snapshots.
test sync::sync_script ... FAILED

But I'm not able to repro the failure locally with cargo test sync_script. Does this look like the kind of thing that could be a CI flake?

[konstin]

That's one the common CI flake unfortunately, one that we should invest into fixing: #13745

[oconnor663]

Indeed, I just reran it and got green :p

[zanieb]

I probably would label this as an enhancement rather than a bug, since there's not actually a correctness issue.

[zanieb]

This seems reasonable to me. Are there any downsides?

[charliermarsh]

I think this is generally correct and what I was looking for.

[oconnor663]

I've added strict GitOid parsing to this PR, since it simplifies the new code in sources.rs and it doesn't seem to break anything.

[oconnor663]

This test occasionally flakes for me, and I can't understand why. Sometimes, rarely, the Resolved 1 package line comes after the Updating/Updated lines. It doesn't look like that should be possible when I read the code, because this call to operations::resolve:

uv/crates/uv/src/commands/pip/install.rs

Line 466 in dc3fd46

let resolution = match operations::resolve(

...happens strictly before this call to operations::install...

uv/crates/uv/src/commands/pip/install.rs

Line 508 in dc3fd46

match operations::install(

I'm clearly overlooking something, but I'm not sure what. Maybe the answer is to just filter out the "Resolved..." line, but I want to understand it.

[konstin]

Is that the one resolved by UV_NO_GITHUB_FAST_PATH?

[oconnor663]

Are there any downsides?

The only downside I can think of (other than added code complexity) is the cost of shelling out to git rev-parse before we fetch. That's obviously worth it in the cached case where it lets us skip the rest of the fetch entirely, but it's an additional cost in the initial, uncached case that we weren't paying before. That said, the cost of the fetch that you're going to do in that case will dwarf the rev-parse, so I don't think it matters much?

[oconnor663]

Still not sure about the flake I mentioned above, but still I'm moving this out of Draft Status :) I found it hard to get rid of that temporary closure I'm using, so that's still in from the draft, though the GitOid change makes it at bit better.

[oconnor663]

I finally tracked down the test flake. The common case would take this path (-v logs):

DEBUG At least one requirement is not satisfied: git+https://github.com/astral-test/uv-public-pypackage@b270df1a2fb5d012294e9aaf05e7e0bab1e6a389
DEBUG Using request timeout of 30s
DEBUG Attempting GitHub fast path for: git+https://github.com/astral-test/uv-public-pypackage@b270df1a2fb5d012294e9aaf05e7e0bab1e6a389
DEBUG Querying GitHub for commit at: https://api.github.com/repos/astral-test/uv-public-pypackage/commits/b270df1a2fb5d012294e9aaf05e7e0bab1e6a389
DEBUG Attempting to fetch `pyproject.toml` from: https://raw.githubusercontent.com/astral-test/uv-public-pypackage/b270df1a2fb5d012294e9aaf05e7e0bab1e6a389/pyproject.toml
DEBUG Found static metadata via GitHub fast path for: git+https://github.com/astral-test/uv-public-pypackage@b270df1a2fb5d012294e9aaf05e7e0bab1e6a389

But the uncommon case (which happened to trigger here in CI, maybe for rate limit reasons) would take this path:

DEBUG At least one requirement is not satisfied: git+https://github.com/astral-test/uv-public-pypackage@b270df1a2fb5d012294e9aaf05e7e0bab1e6a389
DEBUG Using request timeout of 30s
DEBUG Attempting GitHub fast path for: git+https://github.com/astral-test/uv-public-pypackage@b270df1a2fb5d012294e9aaf05e7e0bab1e6a389
DEBUG Querying GitHub for commit at: https://api.github.com/repos/astral-test/uv-public-pypackage/commits/b270df1a2fb5d012294e9aaf05e7e0bab1e6a389
DEBUG No netrc file found
DEBUG GitHub API request failed for: https://api.github.com/repos/astral-test/uv-public-pypackage/commits/b270df1a2fb5d012294e9aaf05e7e0bab1e6a389 (403 Forbidden)
DEBUG Fetching source distribution from Git: https://github.com/astral-test/uv-public-pypackage

This failure in the GitHub fast path would lead us to fetch the repo as part of resolution, instead of a part of building, which appeared as a reordering of the Resolved and Updating/Updated logs. Disabling the GitHub fast path entirely currently makes the test reliable.

@charliermarsh @ibraheemdev, this is ready for a re-review.

[zanieb]

I think you're fine to merge unless you think any reviewers have outstanding concerns or you feel uncertain about something in particular.

[oconnor663]

This should also fix #12746.