cargo update: Don't fetch from the remote repo if the revision is already present locally

[jyn514]

This makes cargo update much faster when there are many git deps in a repo and they're already
cached locally.

There was an existing check for this, but it didn't quite hit all cases.
The code looked like this:

        let (db, actual_rev) = match (self.locked_rev, db) {
            (Some(rev), Some(db)) if db.contains(rev) => (db, rev),
            (None, Some(db)) if self.config.offline() => {
                let rev = db.resolve(&self.manifest_reference)?;
                (db, rev)
            }
            (locked_rev, db) => self.remote.checkout(),
        };

The problem was that when locked_rev.is_none() && db.is_some() and --offline wasn't passed,
cargo would skip the db.resolve() check, and issue a fetch from the remote even if it wasn't
necessary.

This changes the db.resolve() check to happen even in online mode.
This brings cargo update down from 53 to 6 seconds for me on a codebase with 10+ git dependencies.

[rust-highfive]

r? @alexcrichton

(rust-highfive has picked a reviewer for you, use r? to override)

[jyn514]

Oh interesting, there's already a check for this at https://github.com/rust-lang/cargo/blob/ba9f2f80dd439bf38e81dcf907c1c06cfe9bf371/src/cargo/sources/git/source.rs#L114-L123 - I think I can move that into fetch so that it's cached consistently.

[jyn514]

Ah wait no, cargo update already goes through that codepath, so it should be hitting that check already. The slowdown I was hitting is when locked_rev is None, because https://github.com/rust-lang/cargo/blob/ba9f2f80dd439bf38e81dcf907c1c06cfe9bf371/src/cargo/sources/git/source.rs#L128-L133 only gets hit when running in offline mode. If it checked db.resolve whenever db.is_some() I think it would fix my issue.

[alexcrichton]

I am not following this super closely and it seems like this is rapidly changing. I wanted to say though that as a heads up the premise of this PR I think is flawed. A reference to something like a branch cannot check to see if there's a local copy since we need to check the remote to see if there's a more up-to-date version. The only case we can skip the remote update is if we have a locked git sha and that's already local. Just because we have refs/heads/main locally doesn't mean it's up-to-date.

[jyn514]

The only case we can skip the remote update is if we have a locked git sha and that's already local. Just because we have refs/heads/main locally doesn't mean it's up-to-date.

Ok, I can ensure this only happens when the revision is a locked sha and not a branch. The reason I opened this in the first place though is because cargo was updating the remote even when it's locked to a sha.

[jyn514]

I'm done changing this for now (except for addressing review comments) :)

[weihanglo]

A git revision can be a tag or sha or anything. I don't think we can simply search it in the local db. Especially when it cannot be parsed as a commit object.

For instance, the rev is a tag and chances are that the remote tag has been updated to point to another commit but our local database is still stale.

[weihanglo]

Maybe something like this to check if the rev is a commit sha

https://github.com/rust-lang/cargo/pull/10079/files#diff-0e22d3c91f7eac28d92382b445103b421ef842eef760bdc1885be073117f3e9dR1090-R1092

[jyn514]

I found this doesn't actually fix my problem. Sorry for the noise.