Deprecate PackageManager.loadSCMPackage & other Dependency-related refactoring (dlang#2312)

* Trivial: Take a nested function out of a foreach loop

* Deprecate PackageManager.loadSCMPackage's Dependency overload, use Repository

We're moving away from using `Dependency.m_range` to store the git ref,
and the `Repository` type has already been updated to include the information,
but some places still read it from `m_range`, including this one.

* Trivial: Forward Dependency's string ctor to VersionRange ctor

* Trivial: Simplify comparison with `VersionRange.Invalid` within `VersionRange`

Author: Geod24

source/dub/dependency.d

@@ -69,7 +69,7 @@ struct Dependency {
 	*/
 	this(string spec)
 	{
-		this.m_range = VersionRange.fromString(spec);
+		this(VersionRange.fromString(spec));
 	}
 
 	/** Constructs a new dependency specification that matches a specific
@@ -982,7 +982,7 @@ private struct VersionRange
 
 		string r;
 
-		if (this == Dependency.invalid.m_range) return "invalid";
+		if (this == Invalid) return "invalid";
 		if (this.isExactVersion() && m_inclusiveA && m_inclusiveB) {
 			// Special "==" case
 			if (m_versA == Version.masterBranch) return "~master";

source/dub/dub.d

@@ -528,7 +528,7 @@ class Dub {
 					try if (m_packageManager.getOrLoadPackage(path)) continue;
 					catch (Exception e) { logDebug("Failed to load path based selection: %s", e.toString().sanitize); }
 				} else if (!dep.repository.empty) {
-					if (m_packageManager.loadSCMPackage(getBasePackageName(p), dep))
+					if (m_packageManager.loadSCMPackage(getBasePackageName(p), dep.repository))
 						continue;
 				} else {
 					if (m_packageManager.getPackage(p, dep.version_)) continue;
@@ -591,7 +591,7 @@ class Dub {
 					continue;
 				}
 			} else if (!ver.repository.empty) {
-				pack = m_packageManager.loadSCMPackage(p, ver);
+				pack = m_packageManager.loadSCMPackage(p, ver.repository);
 			} else {
 				assert(ver.isExactVersion, "Resolved dependency is neither path, nor repository, nor exact version based!?");
 				pack = m_packageManager.getPackage(p, ver.version_);
@@ -1718,7 +1718,7 @@ private class DependencyVersionResolver : DependencyResolver!(Dependency, Depend
 			return m_rootPackage.basePackage;
 
 		if (!dep.repository.empty) {
-			auto ret = m_dub.packageManager.loadSCMPackage(name, dep);
+			auto ret = m_dub.packageManager.loadSCMPackage(name, dep.repository);
 			return ret !is null && dep.matches(ret.version_) ? ret : null;
 		} else if (!dep.path.empty) {
 			try {

source/dub/packagemanager.d

@@ -8,6 +8,7 @@
 module dub.packagemanager;
 
 import dub.dependency;
+static import dub.dependency;
 import dub.internal.utils;
 import dub.internal.vibecompat.core.file;
 import dub.internal.vibecompat.core.log;
@@ -273,15 +274,21 @@ class PackageManager {
 			The package loaded from the given SCM repository or null if the
 			package couldn't be loaded.
 	*/
+	deprecated("Use the overload that accepts a `dub.dependency : Repository`")
 	Package loadSCMPackage(string name, Dependency dependency)
 	in { assert(!dependency.repository.empty); }
+	do { return this.loadSCMPackage(name, dependency.repository); }
+
+	/// Ditto
+	Package loadSCMPackage(string name, dub.dependency.Repository repo)
+	in { assert(!repo.empty); }
 	do {
         Package pack;
 
-        with (dependency.repository) final switch (kind)
+        final switch (repo.kind)
         {
-            case Kind.git:
-                pack = loadGitPackage(name, dependency.versionSpec, dependency.repository.remote);
+            case repo.Kind.git:
+                pack = loadGitPackage(name, repo.ref_, repo.remote);
         }
         if (pack !is null) {
             addPackages(m_temporaryPackages, pack);

source/dub/project.d

@@ -473,6 +473,10 @@ shared static this() {
 		m_missingDependencies = [];
 		m_packageManager.refresh(false);
 
+		Package resolveSubPackage(Package p, string subname, bool silentFail) {
+			return subname.length ? m_packageManager.getSubPackage(p, subname, silentFail) : p;
+		}
+
 		void collectDependenciesRec(Package pack, int depth = 0)
 		{
 			auto indent = replicate("  ", depth);
@@ -490,10 +494,6 @@ shared static this() {
 				// need to be satisfied
 				bool is_desired = !vspec.optional || m_selections.hasSelectedVersion(basename) || (vspec.default_ && m_selections.bare);
 
-				Package resolveSubPackage(Package p, in bool silentFail) {
-					return subname.length ? m_packageManager.getSubPackage(p, subname, silentFail) : p;
-				}
-
 				if (dep.name == m_rootPackage.basePackage.name) {
 					vspec = Dependency(m_rootPackage.version_);
 					p = m_rootPackage.basePackage;
@@ -511,26 +511,26 @@ shared static this() {
 						auto path = vspec.path;
 						if (!path.absolute) path = m_rootPackage.path ~ path;
 						p = m_packageManager.getOrLoadPackage(path, NativePath.init, true);
-						p = resolveSubPackage(p, true);
+						p = resolveSubPackage(p, subname, true);
 					} else if (!vspec.repository.empty) {
-						p = m_packageManager.loadSCMPackage(basename, vspec);
-						p = resolveSubPackage(p, true);
+						p = m_packageManager.loadSCMPackage(basename, vspec.repository);
+						p = resolveSubPackage(p, subname, true);
 					} else {
 						p = m_packageManager.getBestPackage(dep.name, vspec);
 					}
 				} else if (m_dependencies.canFind!(d => getBasePackageName(d.name) == basename)) {
 					auto idx = m_dependencies.countUntil!(d => getBasePackageName(d.name) == basename);
 					auto bp = m_dependencies[idx].basePackage;
 					vspec = Dependency(bp.path);
-					p = resolveSubPackage(bp, false);
+					p = resolveSubPackage(bp, subname, false);
 				} else {
 					logDiagnostic("%sVersion selection for dependency %s (%s) of %s is missing.",
 						indent, basename, dep.name, pack.name);
 				}
 
 				if (!p && !vspec.repository.empty) {
-					p = m_packageManager.loadSCMPackage(basename, vspec);
-					resolveSubPackage(p, false);
+					p = m_packageManager.loadSCMPackage(basename, vspec.repository);
+					resolveSubPackage(p, subname, false);
 				}
 
 				if (!p && !vspec.path.empty && is_desired) {
@@ -542,7 +542,7 @@ shared static this() {
 						logWarn("%sSub package %s must be referenced using the path to it's parent package.", indent, dep.name);
 						p = p.parentPackage;
 					}
-					p = resolveSubPackage(p, false);
+					p = resolveSubPackage(p, subname, false);
 					enforce(p.name == dep.name,
 						format("Path based dependency %s is referenced with a wrong name: %s vs. %s",
 							path.toNativeString(), dep.name, p.name));