Improve multi-remote handling

- fix: Skip adding remotes when repo already exists during clone
- feat: Respect remote names and default to origin when cloning

Author: timabell

src/git.rs

@@ -9,7 +9,13 @@ pub trait Git {
 	fn read_url(&self, path: String, remote_name: String) -> Result<String, GitopolisError>;
 	fn read_all_remotes(&self, path: String) -> Result<BTreeMap<String, String>, GitopolisError>;
 	fn add_remote(&self, path: &str, remote_name: &str, url: &str);
-	fn clone(&self, path: &str, url: &str) -> Result<(), GitopolisError>;
+	/// Clone a repo. Returns Ok(true) if cloned, Ok(false) if already exists (skipped).
+	fn clone(
+		&self,
+		path: &str,
+		url: &str,
+		remote_name: Option<&str>,
+	) -> Result<bool, GitopolisError>;
 }
 
 pub struct GitImpl {}
@@ -69,14 +75,26 @@ impl Git for GitImpl {
 		}
 	}
 
-	fn clone(&self, path: &str, url: &str) -> Result<(), GitopolisError> {
+	fn clone(
+		&self,
+		path: &str,
+		url: &str,
+		remote_name: Option<&str>,
+	) -> Result<bool, GitopolisError> {
 		if Path::new(path).exists() {
 			println!("🏢 {path}> Already exists, skipped.");
-			return Ok(());
+			return Ok(false);
 		}
 		println!("🏢 {path}> Cloning {url} ...");
+		let mut args = vec!["clone"];
+		if let Some(name) = remote_name {
+			args.push("--origin");
+			args.push(name);
+		}
+		args.push(url);
+		args.push(path);
 		let output = Command::new("git")
-			.args(["clone".to_string(), url.to_string(), path.to_string()].to_vec())
+			.args(args)
 			.output()
 			.expect("Error running git clone");
 		let stdout = String::from_utf8(output.stdout).expect("Error converting stdout to string");
@@ -90,6 +108,6 @@ impl Git for GitImpl {
 			});
 		}
 
-		Ok(())
+		Ok(true)
 	}
 }

src/gitopolis.rs

@@ -101,15 +101,22 @@ impl Gitopolis {
 
 			if let Some(clone_remote) = repo.remotes.get(clone_remote_name) {
 				// Clone the repo
-				match self.git.clone(repo.path.as_str(), &clone_remote.url) {
-					Ok(()) => {
-						// Add all other remotes
+				match self.git.clone(
+					repo.path.as_str(),
+					&clone_remote.url,
+					Some(clone_remote_name),
+				) {
+					Ok(true) => {
+						// Clone succeeded, add all other remotes
 						for (name, remote) in &repo.remotes {
 							if name != clone_remote_name {
 								self.git.add_remote(&repo.path, name, &remote.url);
 							}
 						}
 					}
+					Ok(false) => {
+						// Repo already exists, skip adding remotes
+					}
 					Err(_) => {
 						eprintln!("Warning: Could not clone {}", repo.path);
 						error_count += 1;
@@ -238,7 +245,7 @@ impl Gitopolis {
 		};
 
 		// Clone the repository
-		self.git.clone(&folder_name, url)?;
+		self.git.clone(&folder_name, url, None)?;
 
 		// Add the repository to gitopolis
 		self.add(folder_name.clone())?;

tests/end_to_end_tests.rs

@@ -2552,3 +2552,192 @@ url = \"git://example.org/repo3_origin\"
 	let remotes3 = String::from_utf8(output3.stdout).expect("utf8 conversion failed");
 	assert!(remotes3.is_empty());
 }
+
+#[test]
+fn clone_multiple_remotes_prefers_origin() {
+	// Test that when cloning a repo with remotes aaa, origin, zzz:
+	// - It clones using the origin remote
+	// - It adds aaa and zzz as additional remotes
+	let temp = temp_folder();
+
+	// Create source repos for each remote
+	create_local_repo(&temp, "source_aaa");
+	create_local_repo(&temp, "source_origin");
+	create_local_repo(&temp, "source_zzz");
+
+	// Create state with multiple remotes including origin
+	let initial_state_toml = r#"[[repos]]
+path = "cloned_repo"
+tags = []
+
+[repos.remotes.aaa]
+name = "aaa"
+url = "source_aaa"
+
+[repos.remotes.origin]
+name = "origin"
+url = "source_origin"
+
+[repos.remotes.zzz]
+name = "zzz"
+url = "source_zzz"
+"#;
+	write_gitopolis_state_toml(&temp, initial_state_toml);
+
+	// Run clone
+	gitopolis_executable()
+		.current_dir(&temp)
+		.args(vec!["clone"])
+		.assert()
+		.success()
+		.stdout(predicate::str::contains("Cloning source_origin"));
+
+	// Verify all three remotes exist in cloned repo with correct names
+	let cloned_path = temp.path().join("cloned_repo");
+
+	let remotes_output = Command::new("git")
+		.current_dir(&cloned_path)
+		.args(vec!["remote", "--verbose"])
+		.output()
+		.expect("git remote --verbose failed");
+	let remotes = String::from_utf8(remotes_output.stdout).expect("utf8 conversion failed");
+
+	// git clone converts relative local paths to absolute, so strip the temp path prefix
+	// Use canonicalize to resolve symlinks (e.g., /var -> /private/var on macOS)
+	let temp_path_canonical = temp.path().canonicalize().unwrap();
+	let temp_path_str = temp_path_canonical.to_str().unwrap();
+	let remotes = remotes.replace(&format!("{}/", temp_path_str), "");
+
+	let expected_remotes = r#"aaa	source_aaa (fetch)
+aaa	source_aaa (push)
+origin	source_origin (fetch)
+origin	source_origin (push)
+zzz	source_zzz (fetch)
+zzz	source_zzz (push)
+"#;
+	assert_eq!(expected_remotes, remotes);
+}
+
+#[test]
+fn clone_multiple_remotes_uses_first_when_no_origin() {
+	// Test that when cloning a repo with remotes aaa, bbb (no origin):
+	// - It clones using the first remote (aaa) alphabetically
+	// - The remote is named "aaa" (not "origin")
+	// - It adds bbb as an additional remote
+	let temp = temp_folder();
+
+	// Create source repos for each remote
+	create_local_repo(&temp, "source_aaa");
+	create_local_repo(&temp, "source_bbb");
+
+	// Create state with multiple remotes, none named origin
+	let initial_state_toml = r#"[[repos]]
+path = "cloned_repo"
+tags = []
+
+[repos.remotes.aaa]
+name = "aaa"
+url = "source_aaa"
+
+[repos.remotes.bbb]
+name = "bbb"
+url = "source_bbb"
+"#;
+	write_gitopolis_state_toml(&temp, initial_state_toml);
+
+	// Run clone
+	gitopolis_executable()
+		.current_dir(&temp)
+		.args(vec!["clone"])
+		.assert()
+		.success()
+		.stdout(predicate::str::contains("Cloning source_aaa"));
+
+	// Verify remotes in cloned repo
+	let cloned_path = temp.path().join("cloned_repo");
+
+	let remotes_output = Command::new("git")
+		.current_dir(&cloned_path)
+		.args(vec!["remote", "--verbose"])
+		.output()
+		.expect("git remote --verbose failed");
+	let remotes = String::from_utf8(remotes_output.stdout).expect("utf8 conversion failed");
+
+	// git clone converts relative local paths to absolute, so strip the temp path prefix
+	// Use canonicalize to resolve symlinks (e.g., /var -> /private/var on macOS)
+	let temp_path_canonical = temp.path().canonicalize().unwrap();
+	let temp_path_str = temp_path_canonical.to_str().unwrap();
+	let remotes = remotes.replace(&format!("{}/", temp_path_str), "");
+
+	// Should have aaa and bbb remotes, NOT origin
+	let expected_remotes = r#"aaa	source_aaa (fetch)
+aaa	source_aaa (push)
+bbb	source_bbb (fetch)
+bbb	source_bbb (push)
+"#;
+	assert_eq!(expected_remotes, remotes);
+}
+
+#[test]
+fn clone_skips_adding_remotes_when_repo_exists() {
+	// Test that when a repo already exists, clone skips it entirely
+	// and doesn't try to add remotes (which would fail with "remote already exists")
+	let temp = temp_folder();
+
+	// Create source repos
+	create_local_repo(&temp, "source_origin");
+	create_local_repo(&temp, "source_upstream");
+
+	// Pre-create the target repo with origin remote already configured
+	let cloned_path = temp.path().join("cloned_repo");
+	fs::create_dir_all(&cloned_path).expect("create repo dir failed");
+	Command::new("git")
+		.current_dir(&cloned_path)
+		.args(vec!["init", "--initial-branch", "main"])
+		.output()
+		.expect("git init failed");
+	Command::new("git")
+		.current_dir(&cloned_path)
+		.args(vec!["remote", "add", "origin", "existing_origin_url"])
+		.output()
+		.expect("git remote add failed");
+
+	// Create state with multiple remotes
+	let initial_state_toml = r#"[[repos]]
+path = "cloned_repo"
+tags = []
+
+[repos.remotes.origin]
+name = "origin"
+url = "source_origin"
+
+[repos.remotes.upstream]
+name = "upstream"
+url = "source_upstream"
+"#;
+	write_gitopolis_state_toml(&temp, initial_state_toml);
+
+	// Run clone - should skip the existing repo and NOT try to add remotes
+	gitopolis_executable()
+		.current_dir(&temp)
+		.args(vec!["clone"])
+		.assert()
+		.success()
+		.stdout(predicate::str::contains("Already exists, skipped"))
+		// Should NOT have any warnings about failed remote adds
+		.stderr(predicate::str::contains("Warning").not());
+
+	// Verify the original remote is unchanged (not overwritten)
+	let remotes_output = Command::new("git")
+		.current_dir(&cloned_path)
+		.args(vec!["remote", "--verbose"])
+		.output()
+		.expect("git remote --verbose failed");
+	let remotes = String::from_utf8(remotes_output.stdout).expect("utf8 conversion failed");
+
+	// Should only have the original origin remote, not the upstream from config
+	let expected_remotes = r#"origin	existing_origin_url (fetch)
+origin	existing_origin_url (push)
+"#;
+	assert_eq!(expected_remotes, remotes);
+}

tests/gitopolis_tests.rs

@@ -514,8 +514,13 @@ impl Git for FakeGit {
 		// No-op for fake implementation
 	}
 
-	fn clone(&self, path: &str, url: &str) -> Result<(), GitopolisError> {
+	fn clone(
+		&self,
+		path: &str,
+		url: &str,
+		_remote_name: Option<&str>,
+	) -> Result<bool, GitopolisError> {
 		(self.clone_callback)(path.to_owned(), url.to_owned());
-		Ok(())
+		Ok(true)
 	}
 }