Remove the hokey add_lockfile_sources

This method shouldn't be necessary as it should be possible to simply add all
sources known in the lockfile to a package registry, and
core::{resolve, registry} should take care of weeding them out if necessary.

In the process of doing so, this actually ends up fixing a problem with
rewriting a dependency to a new one which shares transitive deps. Previously all
the transitive deps were updated, but now the transitive deps remain locked at
where they were previously locked.

diff --git a/src/cargo/ops/cargo_fetch.rs b/src/cargo/ops/cargo_fetch.rs
index 3f04af2951c013b82c63dd108021b95b2fa6bc09..e2457bc5d9f668c666319eb75e33f439eb253b3d 100644 (file)
--- a/src/cargo/ops/cargo_fetch.rs
+++ b/src/cargo/ops/cargo_fetch.rs
@@ -1,6 +1,4 @@
-use std::collections::{HashSet, HashMap};
-
-use core::{MultiShell, Package, PackageId, Summary};
+use core::{MultiShell, Package};
 use core::registry::PackageRegistry;
 use core::resolver::{mod, Resolve};
 use core::source::Source;
@@ -34,9 +32,10 @@ pub fn resolve_and_fetch(registry: &mut PackageRegistry, package: &Package)
     let source_id = package.get_package_id().get_source_id();
     let previous_resolve = try!(ops::load_lockfile(&lockfile, source_id));
     match previous_resolve {
-        Some(ref r) => try!(add_lockfile_sources(registry, package, r)),
-        None => try!(registry.add_sources(package.get_source_ids())),
-    }
+        Some(ref r) => r.iter().map(|p| p.get_source_id().clone()).collect(),
+        None => package.get_source_ids(),
+    };
+    try!(registry.add_sources(sources));
 
     let mut resolved = try!(resolver::resolve(package.get_summary(),
                                               resolver::ResolveEverything,
@@ -48,64 +47,3 @@ pub fn resolve_and_fetch(registry: &mut PackageRegistry, package: &Package)
     try!(ops::write_resolve(package, &resolved));
     Ok(resolved)
 }
-
-/// When a lockfile is present, we want to keep as many dependencies at their
-/// original revision as possible. We need to account, however, for
-/// modifications to the manifest in terms of modifying, adding, or deleting
-/// dependencies.
-///
-/// This method will add any appropriate sources from the lockfile into the
-/// registry, and add all other sources from the root package to the registry.
-/// Any dependency which has not been modified has its source added to the
-/// registry (to retain the precise field if possible). Any dependency which
-/// *has* changed has its source id listed in the manifest added and all of its
-/// transitive dependencies are blacklisted to not be added from the lockfile.
-///
-/// TODO: this won't work too well for registry-based packages, but we don't
-///       have many of those anyway so we should be ok for now.
-fn add_lockfile_sources(registry: &mut PackageRegistry,
-                        root: &Package,
-                        resolve: &Resolve) -> CargoResult<()> {
-    let deps = resolve.deps(root.get_package_id()).into_iter().flat_map(|deps| {
-        deps.map(|d| (d.get_name(), d))
-    }).collect::<HashMap<_, &PackageId>>();
-
-    let mut sources = vec![root.get_package_id().get_source_id().clone()];
-    let mut to_avoid = HashSet::new();
-    let mut to_add = HashSet::new();
-    for dep in root.get_dependencies().iter() {
-        match deps.find(&dep.get_name()) {
-            Some(&lockfile_dep) => {
-                let summary = Summary::new(lockfile_dep.clone(), Vec::new(),
-                                           HashMap::new()).unwrap();
-                if dep.matches(&summary) {
-                    fill_with_deps(resolve, lockfile_dep, &mut to_add);
-                } else {
-                    fill_with_deps(resolve, lockfile_dep, &mut to_avoid);
-                    sources.push(dep.get_source_id().clone());
-                }
-            }
-            None => sources.push(dep.get_source_id().clone()),
-        }
-    }
-
-    // Only afterward once we know the entire blacklist are the lockfile
-    // sources added.
-    for addition in to_add.iter() {
-        if !to_avoid.contains(addition) {
-            sources.push(addition.get_source_id().clone());
-        }
-    }
-
-    return registry.add_sources(sources);
-
-    fn fill_with_deps<'a>(resolve: &'a Resolve, dep: &'a PackageId,
-                          set: &mut HashSet<&'a PackageId>) {
-        if !set.insert(dep) { return }
-        for mut deps in resolve.deps(dep).into_iter() {
-            for dep in deps {
-                fill_with_deps(resolve, dep, set);
-            }
-        }
-    }
-}

diff --git a/tests/test_cargo_compile_git_deps.rs b/tests/test_cargo_compile_git_deps.rs
index 8b4da434cd23f65e236e8db2533f5b6492dde60c..777c9125439bce738b16575b004af0b011214687 100644 (file)
--- a/tests/test_cargo_compile_git_deps.rs
+++ b/tests/test_cargo_compile_git_deps.rs
@@ -1427,3 +1427,80 @@ test!(update_one_dep_in_repo_with_many_deps {
 Updating git repository `{}`
 ", foo.url())));
 })
+
+test!(switch_deps_does_not_update_transitive {
+    let transitive = git_repo("transitive", |project| {
+        project.file("Cargo.toml", r#"
+            [package]
+            name = "transitive"
+            version = "0.5.0"
+            authors = ["wycats@example.com"]
+        "#)
+        .file("src/lib.rs", "")
+    }).assert();
+    let dep1 = git_repo("dep1", |project| {
+        project.file("Cargo.toml", format!(r#"
+            [package]
+            name = "dep"
+            version = "0.5.0"
+            authors = ["wycats@example.com"]
+
+            [dependencies.transitive]
+            git = '{}'
+        "#, transitive.url()).as_slice())
+        .file("src/lib.rs", "")
+    }).assert();
+    let dep2 = git_repo("dep2", |project| {
+        project.file("Cargo.toml", format!(r#"
+            [package]
+            name = "dep"
+            version = "0.5.0"
+            authors = ["wycats@example.com"]
+
+            [dependencies.transitive]
+            git = '{}'
+        "#, transitive.url()).as_slice())
+        .file("src/lib.rs", "")
+    }).assert();
+
+    let p = project("project")
+        .file("Cargo.toml", format!(r#"
+            [project]
+            name = "project"
+            version = "0.5.0"
+            authors = []
+            [dependencies.dep]
+            git = '{}'
+        "#, dep1.url()).as_slice())
+        .file("src/main.rs", "fn main() {}");
+
+    p.build();
+    assert_that(p.process(cargo_dir().join("cargo")).arg("build"),
+                execs().with_status(0)
+                       .with_stdout(format!("\
+Updating git repository `{}`
+Updating git repository `{}`
+{compiling} transitive [..]
+{compiling} dep [..]
+{compiling} project [..]
+", dep1.url(), transitive.url(), compiling = COMPILING)));
+
+    // Update the dependency to point to the second repository, but this
+    // shouldn't update the transitive dependency which is the same.
+    File::create(&p.root().join("Cargo.toml")).write_str(format!(r#"
+            [project]
+            name = "project"
+            version = "0.5.0"
+            authors = []
+            [dependencies.dep]
+            git = '{}'
+    "#, dep2.url()).as_slice()).unwrap();
+
+    assert_that(p.process(cargo_dir().join("cargo")).arg("build"),
+                execs().with_status(0)
+                       .with_stdout(format!("\
+Updating git repository `{}`
+{compiling} dep [..]
+{compiling} project [..]
+", dep2.url(), compiling = COMPILING)));
+})