Update for feedback & remove --branch option

Make for the `--unmanaged` flag on `add` to have an optional argument of
a commit or a branch. However this lead to complications around other
flags being passed such as `add blah --unmanaged -u -v`.

To resolve that issue, the properties on `Option` were move to `public`
so the add command could try its best to still apply arguments when
they were inadvertantly swallowed by the now `--unmanaged` optional
value.

Also the `unmanagedValue` is tracked in the `.modulo` file like `branch`
was but is more generic supporting an entire commit hash or a branch
name

Author: dmiedema

Modules/ELCLI/ELCLI/Options.swift

@@ -11,10 +11,10 @@ import Foundation
 public typealias OptionClosure = (_ option: String?, _ value: String?) -> Void
 
 public struct Option {
-    let usage: String?
-    let flags: Array<String>?
-    let valueSignatures: Array<String>?
-    let closure: OptionClosure
+    public let usage: String?
+    public let flags: Array<String>?
+    public let valueSignatures: Array<String>?
+    public let closure: OptionClosure
 
     init(flags: Array<String>?, usage: String?, valueSignatures: Array<String>?, closure: @escaping OptionClosure) {
         self.flags = flags

ModuloKit/Actions.swift

@@ -22,8 +22,8 @@ open class Actions {
         }
     }
 
-    open func addDependency(_ url: String, version: SemverRange?, branch: String?, unmanaged: Bool) -> ErrorCode {
-        let dep = DependencySpec(repositoryURL: url, version: version, branch: branch)
+    open func addDependency(_ url: String, version: SemverRange?, unmanagedValue: String?, unmanaged: Bool) -> ErrorCode {
+        let dep = DependencySpec(repositoryURL: url, version: version, unmanagedValue: unmanagedValue)
         if var workingSpec = ModuleSpec.workingSpec() {
             // does this dep already exist in here??
             if let _ = workingSpec.dependencyForURL(url) {
@@ -77,8 +77,8 @@ open class Actions {
                         exit(checkoutResult.errorMessage())
                     }
                 }
-                if let branch = dep.branch {
-                    let checkoutResult = scm.checkout(branch: branch, path: clonePath)
+                if let unmanagedValue = dep.unmanagedValue {
+                    let checkoutResult = scm.checkout(branchOrHash: unmanagedValue, path: clonePath)
                     if checkoutResult != .success {
                         exit(checkoutResult.errorMessage())
                     }
@@ -102,7 +102,7 @@ open class Actions {
                 }
 
                 // if they're unmanaged and on a branch, tracking a remote, just do a pull
-                if dep.unmanaged == true, let currentBranch = scm.branchName(clonePath) {
+                if dep.unmanaged == true && dep.unmanagedValue == nil, let currentBranch = scm.branchName(clonePath) {
                     if scm.remoteTrackingBranch(currentBranch) != nil {
                         let pullResult = scm.pull(clonePath, remoteData: nil)
                         if pullResult != .success {
@@ -116,13 +116,13 @@ open class Actions {
                         if checkoutResult != .success {
                             exit(checkoutResult.errorMessage())
                         }
-                    } else if let branch = dep.branch {
-                        let checkoutResult = scm.checkout(branch: branch, path: clonePath)
+                    } else if let unmanagedValue = dep.unmanagedValue {
+                        let checkoutResult = scm.checkout(branchOrHash: unmanagedValue, path: clonePath)
                         if checkoutResult != .success {
                             exit(checkoutResult.errorMessage())
                         }
                     } else {
-                        exit("\(dep.name()) doesn't have a version, branch, and isn't marked as 'unmanaged', not sure what to do.")
+                        exit("\(dep.name()) doesn't have a version and isn't marked as 'unmanaged', not sure what to do.")
                     }
                 }
             }

ModuloKit/Commands/AddCommand.swift

@@ -16,10 +16,10 @@ import Foundation
 open class AddCommand: NSObject, Command {
     // internal properties
     fileprivate var version: SemverRange? = nil
-    fileprivate var branch: String? = nil
     fileprivate var repositoryURL: String! = nil
     fileprivate var shouldUpdate: Bool = false
     fileprivate var unmanaged: Bool = false
+    fileprivate var unmanagedValue: String? = nil
 
     // Protocol conformance
     open var name: String { return "add" }
@@ -41,15 +41,24 @@ open class AddCommand: NSObject, Command {
                 self.version = SemverRange(value)
             }
         }
-
-        addOptionValue(["--branch"], usage: "specify the branch to track", valueSignature: "<branch>") { (option, value) in
-            if let value = value {
-                self.branch = value
-            }
-        }
 
-        addOption(["--unmanaged"], usage: "specifies that this module will be unmanaged") { (option, value) in
+        addOptionValue(["--unmanaged"], usage: "specifies that this module will be unmanaged", valueSignature: "<[hash|branch|nothing]>") { (option, value) -> Void in
             self.unmanaged = true
+            if let value = value {
+                if !value.hasPrefix("-") {
+                    self.unmanagedValue = value
+                }  else {
+                    if self.verbose {
+                        writeln(.stderr, "Assuming '\(value)' is a flag and not a branch/commit hash since it begins with '-' and will not track it.")
+                    }
+                    // TODO: Somehow reprocess this `value` as a flag
+                    self.options.filter({ (option) -> Bool in
+                        option.flags?.contains(value) ??  false
+                    }).forEach({ (option) in
+                        option.closure(value, nil)
+                    })
+                }
+            }
         }
 
         addOption(["-u", "--update"], usage: "performs the update command after adding a module") { (option, value) in
@@ -64,8 +73,8 @@ open class AddCommand: NSObject, Command {
     open func execute(_ otherParams: Array<String>?) -> Int {
         let actions = Actions()
 
-        if version == nil && branch == nil && unmanaged == false {
-            writeln(.stderr, "A version or range must be specified via --version, a branch must be specified via --branch, or --unmanaged must be used.")
+        if version == nil && unmanaged == false {
+            writeln(.stderr, "A version or range must be specified via --version or --unmanaged must be used.")
             return ErrorCode.commandError.rawValue
         }
 
@@ -76,7 +85,7 @@ open class AddCommand: NSObject, Command {
             }
         }
 
-        let result = actions.addDependency(repositoryURL, version: version, branch: branch, unmanaged: unmanaged)
+        let result = actions.addDependency(repositoryURL, version: version, unmanagedValue: unmanagedValue, unmanaged: unmanaged)
         if result == .success {
             if shouldUpdate {
                 writeln(.stdout, "Added \(String(describing: repositoryURL)).")

ModuloKit/SCM/Git.swift

@@ -176,7 +176,7 @@ open class Git: SCM {
         return .success
     }
 
-    open func checkout(branch: String, path: String) -> SCMResult {
+    open func checkout(branchOrHash: String, path: String) -> SCMResult {
         if !FileManager.fileExists(path) {
             return .error(code: 1, message: "Module path '\(path)' does not exist.")
         }
@@ -186,25 +186,20 @@ open class Git: SCM {
         let initialWorkingPath = FileManager.workingPath()
         FileManager.setWorkingPath(path)
 
-        let existingBranches = branches(".")
+        // try fetching it directly
+        let fetchResult = runCommand("git fetch origin \(branchOrHash)")
 
-        var neededFetch = false
-        var fetchResult: Int32? = nil
-        if !existingBranches.contains(branch) {
-            // try fetching it directly
-            fetchResult = runCommand("git fetch origin \(branch)")
-            neededFetch = true
+        if branches(".").contains(branchOrHash) {
+            checkoutCommand = "git checkout origin/\(branchOrHash) --quiet"
+        } else {
+            checkoutCommand = "git checkout \(branchOrHash) --quiet"
         }
 
-        checkoutCommand = "git checkout origin/\(branch) --quiet"
-
-        if neededFetch,
-            let fetchResult = fetchResult,
-            fetchResult != 0 {
+        if fetchResult != 0 {
             if verbose {
-                writeln(.stderr, "Unable to find branch '\(branch)'.")
+                writeln(.stderr, "Unable to find unmanaged value '\(branchOrHash)'.")
             }
-            return .error(code: SCMDefaultError, message: "Unable to find a match for \(branch).")
+            return .error(code: SCMDefaultError, message: "Unable to find a match for \(branchOrHash).")
         }
 
         let status = runCommand(checkoutCommand)
@@ -214,7 +209,7 @@ open class Git: SCM {
         FileManager.setWorkingPath(initialWorkingPath)
 
         if status != 0 {
-            return .error(code: status, message: "Unable to checkout '\(branch)'.")
+            return .error(code: status, message: "Unable to checkout '\(branchOrHash)'.")
         }
 
         if submodulesResult != .success {

ModuloKit/SCM/SCM.swift

@@ -76,7 +76,9 @@ public protocol SCM {
     func fetch(_ path: String) -> SCMResult
     func pull(_ path: String, remoteData: String?) -> SCMResult
     func checkout(version: SemverRange, path: String) -> SCMResult
-    func checkout(branch: String, path: String) -> SCMResult
+    /// Check out an arbitrary point or the HEAD of a branch (in git)
+    /// or the equivalent in other SCM solutions
+    func checkout(branchOrHash: String, path: String) -> SCMResult
     func remove(_ path: String) -> SCMResult
     func adjustIgnoreFile(pattern: String, removing: Bool) -> SCMResult
     func checkStatus(_ path: String) -> SCMResult

ModuloKit/Specs/DependencySpec.swift

@@ -17,12 +17,13 @@ public struct DependencySpec {
     var repositoryURL: String
     // version or version range
     var version: SemverRange?
-    /// Branch to track
-    var branch: String?
+    /// Optional unmanaged property to track
+    /// such as a branch name, commit hash, or nothing
+    var unmanagedValue: String?
 
     var unmanaged: Bool {
         get {
-            return (version == nil) && (branch == nil)
+            return (version == nil)
         }
     }
 }
@@ -32,7 +33,7 @@ extension DependencySpec: ELDecodable {
         return try DependencySpec(
             repositoryURL: json ==> "repositoryURL",
             version: json ==> "version",
-            branch: json ==> "branch"
+            unmanagedValue: json ==> "unmanagedValue"
         )
     }
 
@@ -46,7 +47,7 @@ extension DependencySpec: ELEncodable {
         return try encodeToJSON([
             "repositoryURL" <== repositoryURL,
             "version" <== version,
-            "branch" <== branch
+            "unmanagedValue" <== unmanagedValue
         ])
     }
 }

ModuloKitTests/TestAdd.swift

@@ -79,15 +79,15 @@ class TestAdd: XCTestCase {
         FileManager.setWorkingPath("..")
     }
 
-    func testAddModuleByBranch() {
+    func testAddUnmanagedModuleWithBranch() {
         let status = Git().clone("[email protected]:modulo-dm/test-add.git", path: "test-add")
         XCTAssertTrue(status == .success)
 
         FileManager.setWorkingPath("test-add")
 
         let repoURL = "[email protected]:modulo-dm/test-add-update.git"
 
-        let result = Modulo.run(["add", repoURL, "--branch", "master", "-v"])
+        let result = Modulo.run(["add", repoURL, "--unmanaged", "master", "-v"])
         XCTAssertTrue(result == .success)
 
 
@@ -99,9 +99,37 @@ class TestAdd: XCTestCase {
             XCTFail("Failed to find dependency for url \(repoURL) in spec \(spec)")
             return }
         XCTAssertNil(dep.version)
-        XCTAssertFalse(dep.unmanaged)
-        XCTAssertNotNil(dep.branch)
-        XCTAssertTrue(dep.branch == "master")
+        XCTAssertTrue(dep.unmanaged)
+        XCTAssertNotNil(dep.unmanagedValue)
+        XCTAssertTrue(dep.unmanagedValue == "master")
+
+        FileManager.setWorkingPath("..")
+
+        Git().remove("test-add")
+    }
+
+    func testAddModuleUnmanagedNoArgs() {
+        let status = Git().clone("[email protected]:modulo-dm/test-add.git", path: "test-add")
+        XCTAssertTrue(status == .success)
+
+        FileManager.setWorkingPath("test-add")
+
+        let repoURL = "[email protected]:modulo-dm/test-add-update.git"
+
+        let result = Modulo.run(["add", repoURL, "--unmanaged", "-v"])
+        XCTAssertTrue(result == .success)
+
+
+        guard let spec = ModuleSpec.load(contentsOfFile: specFilename) else {
+            XCTFail("Failed to get spec from file \(specFilename)")
+            return }
+        XCTAssertTrue(spec.dependencies.count > 0)
+        guard let dep = spec.dependencyForURL(repoURL) else {
+            XCTFail("Failed to find dependency for url \(repoURL) in spec \(spec)")
+            return }
+        XCTAssertNil(dep.version)
+        XCTAssertTrue(dep.unmanaged)
+        XCTAssertNil(dep.unmanagedValue)
 
         FileManager.setWorkingPath("..")
 

ModuloKitTests/TestDummyApp.swift

@@ -41,7 +41,7 @@ class TestDummyApp: XCTestCase {
         XCTAssertTrue(spec!.dependencyForURL("[email protected]:modulo-dm/test-add-update.git") != nil)
 
         let checkedOut = Git().branchName("modules/test-add-update")
-        XCTAssertTrue(checkedOut == "master")
+        XCTAssertTrue(checkedOut == "master", "checkedOut should have been 'master' but was '\(String(describing: checkedOut))'")
 
         XCTAssertTrue(FileManager.fileExists("modules/test-add-update"))
         XCTAssertTrue(FileManager.fileExists("modules/test-dep1"))