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

Error message regression in Cargo 1.80 when fetching particular git revision #14621

Closed
RobJellinghaus opened this issue Sep 30, 2024 · 10 comments · Fixed by #14806
Closed

Error message regression in Cargo 1.80 when fetching particular git revision #14621

RobJellinghaus opened this issue Sep 30, 2024 · 10 comments · Fixed by #14806
Labels
A-diagnostics Area: Error and warning messages generated by Cargo itself. A-git Area: anything dealing with git C-bug Category: bug E-easy Experience: Easy S-accepted Status: Issue or feature is accepted, and has a team member available to help mentor or review

Comments

@RobJellinghaus
Copy link

RobJellinghaus commented Sep 30, 2024

Problem

I am working on Dependabot support for Rust. Currently Dependabot includes Rust 1.79. I was testing with Rust 1.80 and discovered that a Dependabot unit test started failing.

The specific failing Dependabot unit test exercises fetching a nonexistent revision from a git dependency: https://github.com/dependabot/dependabot-core/blob/main/cargo/spec/dependabot/cargo/update_checker/version_resolver_spec.rb#L355

I created a minimal repro with a .zip file below.

With Cargo 1.79, the final reported error from git is:
revspec '11111b376b93484341c68fbca3ca110ae5cd2708' not found; class=Reference (4); code=NotFound (-3)

With Cargo 1.80, the final reported error becomes:
process didn't exit successfully: ``git fetch --verbose --force --update-head-ok 'https://github.com/BurntSushi/utf8-ranges' '+11111b376b93484341c68fbca3ca110ae5cd2708:refs/commit/11111b376b93484341c68fbca3ca110ae5cd2708'`` (exit status: 128)

This updated error message doesn't describe the nature of the error as usefully as the preceding error, and can't be matched by Dependabot's error message parsing.

Evidently error message fidelity is not necessarily a Cargo goal in all cases, so if this is considered not to be a bug, I will work on a change to make Dependabot able to tolerate the new behavior. But it definitely is a regression from a user perspective to lose clarity on the actual git-level problem.

This was discovered on Linux (where Dependabot runs) but my most recent testing was on Windows; same behavior.

Steps

  1. Unzip dbot_git_locked_ref_unit_test_break.zip
  2. Run cargo update -p time:0.1.39 -vv

Expected outcome (with 1.79):

Updating crates.io index
    Updating git repository `https://github.com/BurntSushi/utf8-ranges`
     Running `git fetch --tags --verbose --force --update-head-ok 'https://github.com/BurntSushi/utf8-ranges' '+refs/heads/*:refs/remotes/origin/*' '+HEAD:refs/remotes/origin/HEAD'`
POST git-upload-pack (332 bytes)
From https://github.com/BurntSushi/utf8-ranges
 = [up to date]      master     -> origin/master
 = [up to date]                 -> origin/HEAD
 = [up to date]      0.1.1      -> 0.1.1
 = [up to date]      0.1.2      -> 0.1.2
 = [up to date]      0.1.3      -> 0.1.3
 = [up to date]      1.0.0      -> 1.0.0
 = [up to date]      1.0.1      -> 1.0.1
 = [up to date]      1.0.2      -> 1.0.2
 = [up to date]      1.0.3      -> 1.0.3
 = [up to date]      1.0.4      -> 1.0.4
     Running `git fetch --tags --verbose --force --update-head-ok 'https://github.com/BurntSushi/utf8-ranges' '+refs/heads/*:refs/remotes/origin/*' '+HEAD:refs/remotes/origin/HEAD'`
POST git-upload-pack (332 bytes)
POST git-upload-pack (617 bytes)
From https://github.com/BurntSushi/utf8-ranges
 * [new branch]      master     -> origin/master
 * [new ref]                    -> origin/HEAD
 * [new tag]         0.1.1      -> 0.1.1
 * [new tag]         0.1.2      -> 0.1.2
 * [new tag]         0.1.3      -> 0.1.3
 * [new tag]         1.0.0      -> 1.0.0
 * [new tag]         1.0.1      -> 1.0.1
 * [new tag]         1.0.2      -> 1.0.2
 * [new tag]         1.0.3      -> 1.0.3
 * [new tag]         1.0.4      -> 1.0.4
error: failed to get `utf8-ranges` as a dependency of package `dependabot v0.1.0 (dependabot_tmp_dir)`

Caused by:
  failed to load source for dependency `utf8-ranges`

Caused by:
  Unable to update https://github.com/BurntSushi/utf8-ranges#11111b37

Caused by:
  revspec '11111b376b93484341c68fbca3ca110ae5cd2708' not found; class=Reference (4); code=NotFound (-3)

Actual outcome (with 1.80.1):

C:\dev\dbot_git_locked_ref_unit_test_break>cargo update -p time:0.1.39 -vv
    Updating crates.io index
    Updating git repository `https://github.com/BurntSushi/utf8-ranges`
     Running `git fetch --verbose --force --update-head-ok https://github.com/BurntSushi/utf8-ranges +11111b376b93484341c68fbca3ca110ae5cd2708:refs/commit/11111b376b93484341c68fbca3ca110ae5cd2708`
POST git-upload-pack (112 bytes)
POST git-upload-pack (618 bytes)
fatal: remote error: upload-pack: not our ref 11111b376b93484341c68fbca3ca110ae5cd2708
error: failed to get `utf8-ranges` as a dependency of package `dependabot v0.1.0 (C:\dev\dbot_git_locked_ref_unit_test_break)`

Caused by:
  failed to load source for dependency `utf8-ranges`

Caused by:
  Unable to update https://github.com/BurntSushi/utf8-ranges#11111b37

Caused by:
  failed to fetch into: C:\users\rjelling\.cargo\git\db\utf8-ranges-c7ae21d5ebe06ddf

Caused by:
  process didn't exit successfully: `git fetch --verbose --force --update-head-ok https://github.com/BurntSushi/utf8-ranges +11111b376b93484341c68fbca3ca110ae5cd2708:refs/commit/11111b376b93484341c68fbca3ca110ae5cd2708` (exit code: 128)

This more minimal repro by @arlosi also works:
cargo install --git "https://github.com/BurntSushi/utf8-ranges" --rev 11111b376b93484341c68fbca3ca110ae5cd2708

Possible Solution(s)

The new 1.80 output does include this line, which was not previously present:

fatal: remote error: upload-pack: not our ref 11111b376b93484341c68fbca3ca110ae5cd2708

It looks likely that Dependabot can be updated to look for this error text instead, which may be the simplest solution if this is deemed a won't-fix.

Notes

No response

Version

C:\dev\dbot_git_locked_ref_unit_test_break>cargo version --verbose
cargo 1.80.1
release: 1.80.1
host: x86_64-pc-windows-msvc
libgit2: 1.7.2 (sys:0.18.3 vendored)
libcurl: 8.6.0-DEV (sys:0.4.72+curl-8.6.0 vendored ssl:Schannel)
os: Windows 10.0.22631 (Windows 11 Enterprise) [64-bit]

@RobJellinghaus RobJellinghaus added C-bug Category: bug S-triage Status: This issue is waiting on initial triage. labels Sep 30, 2024
@arlosi
Copy link
Contributor

arlosi commented Sep 30, 2024

This is a difference in the failure behavior for fetching a commit that does not exist from a git repository. In the previous behavior, Cargo was less specific to git about the refs it needed fetched, so the failure occurred later.

The difference is the git command line that cargo is generating:

git fetch --tags --verbose --force --update-head-ok https://github.com/BurntSushi/utf8-ranges +refs/heads/*:refs/remotes/origin/* +HEAD:refs/remotes/origin/HEAD

vs

git fetch --verbose --force --update-head-ok https://github.com/BurntSushi/utf8-ranges +11111b376b93484341c68fbca3ca110ae5cd2708:refs/commit/11111b376b93484341c68fbca3ca110ae5cd2708

Cargo attempts to fetch the minimal amount of additional data from git. In this case, Cargo is able to be specific about which commits it needs fetched. Since those commits don't exist, git fails.

I believe the behavior change in this case was likely introduced by this PR: #13946

The old error message is possibly slightly better but is largely equivalent. Changes in error message are generally not covered by stability guarantees. The new error message does contain the similar level of detail further up in the output from git:

fatal: remote error: upload-pack: not our ref 11111b376b93484341c68fbca3ca110ae5cd2708

To restore the previous behavior, Cargo would have to parse the output of git, and then generate a fake libgit2-style error message.

As Cargo migrates to using gitoxide as the git backend, further error message changes in in this area should be expected.

@weihanglo
Copy link
Member

The message looks worse without CARGO_NET_GIT_FETCH_WITH_CLI=true.

$ cargo +1.80.1 install --git "https://github.com/BurntSushi/utf8-ranges" --rev 11111b376b93484341c68fbca3ca110ae5cd2708
    Updating git repository `https://github.com/BurntSushi/utf8-ranges`
warning: spurious network error (3 tries remaining): could not read from remote repository; class=Net (12); code=Eof (-20)
warning: spurious network error (2 tries remaining): could not read from remote repository; class=Net (12); code=Eof (-20)
warning: spurious network error (1 tries remaining): could not read from remote repository; class=Net (12); code=Eof (-20)
error: failed to fetch into: /home/weihanglo/.cargo/git/db/utf8-ranges-c7ae21d5ebe06ddf

Caused by:
  network failure seems to have happened
  if a proxy or similar is necessary `net.git-fetch-with-cli` may help here
  https://doc.rust-lang.org/cargo/reference/config.html#netgit-fetch-with-cli

Caused by:
  could not read from remote repository; class=Net (12); code=Eof (-20)

compare to

$ cargo +1.79.0 install --git "https://github.com/BurntSushi/utf8-ranges" --rev 11111b376b93484341c68fbca3ca110ae5cd2708
    Updating git repository `https://github.com/BurntSushi/utf8-ranges`
error: revspec '11111b376b93484341c68fbca3ca110ae5cd2708' not found; class=Reference (4); code=NotFound (-3)

@weihanglo weihanglo added A-git Area: anything dealing with git A-diagnostics Area: Error and warning messages generated by Cargo itself. S-needs-design Status: Needs someone to work further on the design for the feature or fix. NOT YET accepted. and removed S-triage Status: This issue is waiting on initial triage. labels Sep 30, 2024
@arlosi
Copy link
Contributor

arlosi commented Sep 30, 2024

The message looks worse without CARGO_NET_GIT_FETCH_WITH_CLI=true.

Good point, the message in the non-git-CLI case is far worse (and makes pointless retries).

@RobJellinghaus
Copy link
Author

It's also interesting to note that leaving the last digit off the one-line repro actually gives a better error message with 1.80.1.

Actual repro:

C:\dev\dbot_git_locked_ref_unit_test_break>cargo install --git "https://github.com/BurntSushi/utf8-ranges" --rev 11111b376b93484341c68fbca3ca110ae5cd2708
    Updating git repository `https://github.com/BurntSushi/utf8-ranges`
fatal: remote error: upload-pack: not our ref 11111b376b93484341c68fbca3ca110ae5cd2708
error: failed to fetch into: C:\rust\.cargo\git\db\utf8-ranges-c7ae21d5ebe06ddf

Caused by:
  process didn't exit successfully: `git fetch --force --update-head-ok https://github.com/BurntSushi/utf8-ranges +11111b376b93484341c68fbca3ca110ae5cd2708:refs/commit/11111b376b93484341c68fbca3ca110ae5cd2708` (exit code: 128)

Leave off the last digit of the revision (which I did by accident at one point):

C:\dev\dbot_git_locked_ref_unit_test_break>cargo install --git "https://github.com/BurntSushi/utf8-ranges" --rev 11111b376b93484341c68fbca3ca110ae5cd270
    Updating git repository `https://github.com/BurntSushi/utf8-ranges`
warning: fetch normally indicates which branches had a forced update,
but that check has been disabled; to re-enable, use '--show-forced-updates'
flag or run 'git config fetch.showForcedUpdates true'
remote: Enumerating objects: 146, done.
remote: Counting objects: 100% (19/19), done.
remote: Compressing objects: 100% (16/16), done.
remote: Total 146 (delta 8), reused 11 (delta 3), pack-reused 127 (from 1)
Receiving objects: 100% (146/146), 37.33 KiB | 3.73 MiB/s, done.
Resolving deltas: 100% (70/70), done.
From https://github.com/BurntSushi/utf8-ranges
 * [new branch]      master     -> origin/master
 * [new ref]         HEAD       -> origin/HEAD
 * [new tag]         0.1.1      -> 0.1.1
 * [new tag]         0.1.2      -> 0.1.2
 * [new tag]         0.1.3      -> 0.1.3
 * [new tag]         1.0.0      -> 1.0.0
 * [new tag]         1.0.1      -> 1.0.1
 * [new tag]         1.0.2      -> 1.0.2
 * [new tag]         1.0.3      -> 1.0.3
 * [new tag]         1.0.4      -> 1.0.4
warning: fetch normally indicates which branches had a forced update,
but that check has been disabled; to re-enable, use '--show-forced-updates'
flag or run 'git config fetch.showForcedUpdates true'
error: revspec '11111b376b93484341c68fbca3ca110ae5cd270' not found; class=Reference (4); code=NotFound (-3)

@weihanglo
Copy link
Member

It's also interesting to note that leaving the last digit off the one-line repro actually gives a better error message with 1.80.1.

This is expected btw, because Cargo check if the rev is a full commit hash.

@weihanglo
Copy link
Member

I can't really tell which message is worse. In 1.79 it'll fetch everything. If there are 100 tags, they will be shown all at once. In 1.80, the message “not our ref xxx” is a bit unclear comparing to “revspec 'xxx' not found”, which I am fine with it.

The worse part is fetching with libgit2 (#14621 (comment)). This needs a fix. Perhaps we can check if Cargo has gone through GitHub fast path (this match arm in particular), we add an error context telling that “rev xxxxx not found on github.com/name/repo”

@weihanglo weihanglo added S-accepted Status: Issue or feature is accepted, and has a team member available to help mentor or review E-easy Experience: Easy and removed S-needs-design Status: Needs someone to work further on the design for the feature or fix. NOT YET accepted. labels Oct 3, 2024
@BLANKatGITHUB
Copy link

Hi , I am kind of new to open source I would like to help , can you please guide me .

@weihanglo
Copy link
Member

Sure.

When fixing a bug in Cargo, we usually write a test first that demonstrates the current buggy behavior, and passes the test suite, followed by other commits fixing both the behavior and the test. This pattern assures that the first commit contains a minimal reproduction. See this contribution guideline for more.

In Cargo we don't want any test to acces public network. However, for this issue specifically, since it tests against GitHub fast path, which requires to talk to Cargo, so we need this public_network_test attribute macro when writing our tests. You can check this test as a reference. The test you are going to add can be put under the same file.

The fix in my mind needs to tweak the cargo::sources::git::utils::fetch function a bit.

  • When this branch was run, GitHub fast path should have been gone through. So probably and a flag here to determine whether it was run.
  • When got Err on these branches and this function call, we might want to append an extra anyhow::Context showing the error is related to a fetch via GitHub fast path.

There might be more stuff I am not aware of. Feel free to ask questions here, on Zulip, or come to our office hours.

@BLANKatGITHUB
Copy link

well, hi I hope I am not bothering you, well I am undergrad student and don't have much experience with big project like this and most of the code goes above my head , but I want to help and learn , I would appreciate if you could give me some direction on how to get started , like first few steps

@weihanglo
Copy link
Member

@BLANKatGITHUB Not sure what skills you have now. The first thing to do is following the steps here to compile Cargo from source. Would highly recommend reading "Cargo Contributor Guide". If you need more helps than that, ping me on Zulip or attend our office hours.

dacianpascu06 added a commit to dacianpascu06/cargo that referenced this issue Nov 17, 2024
dacianpascu06 added a commit to dacianpascu06/cargo that referenced this issue Nov 17, 2024
dacianpascu06 added a commit to dacianpascu06/cargo that referenced this issue Nov 17, 2024
dacianpascu06 added a commit to dacianpascu06/cargo that referenced this issue Nov 17, 2024
dacianpascu06 added a commit to dacianpascu06/cargo that referenced this issue Nov 17, 2024
@weihanglo weihanglo linked a pull request Nov 17, 2024 that will close this issue
dacianpascu06 added a commit to dacianpascu06/cargo that referenced this issue Nov 17, 2024
dacianpascu06 added a commit to dacianpascu06/cargo that referenced this issue Nov 17, 2024
dacianpascu06 added a commit to dacianpascu06/cargo that referenced this issue Nov 17, 2024
github-merge-queue bot pushed a commit that referenced this issue Nov 17, 2024
### What does this PR try to resolve?

Fixes Issue : #14621

Adds more error context for fetching a commit that doesn't exists.

### How should we test and review this PR?

I've created two tests, one for fast_path and one for libgit2.

r? @weihanglo
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-diagnostics Area: Error and warning messages generated by Cargo itself. A-git Area: anything dealing with git C-bug Category: bug E-easy Experience: Easy S-accepted Status: Issue or feature is accepted, and has a team member available to help mentor or review
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants