fix: Skip adding remotes when repo already exists during clone

When cloning, if a repo already exists, the clone operation is skipped
entirely - including not attempting to add any remotes. Previously,
clone would skip the git clone but still try to add remotes, which
would fail with "remote xxx already exists" warnings.

Changed Git::clone() to return Result<bool, GitopolisError> where true
means the repo was cloned and false means it was skipped (already exists).

Prompts:
- "now fix this bug [shouldn't add extra remotes to existing repo when cloning · Issue #247 · timabell/gitopolis](#247)"
- in response to suggestion about checking existing remotes: "simpler than that, if a repo already exists don't clone it and don't add any remotes"

Fixes #247

Co-Authored-By: Claude <noreply@anthropic.com>

Author: timabell

src/git.rs

@@ -9,8 +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, remote_name: Option<&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 {}
@@ -75,10 +80,10 @@ impl Git for GitImpl {
 		path: &str,
 		url: &str,
 		remote_name: Option<&str>,
-	) -> Result<(), GitopolisError> {
+	) -> 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"];
@@ -103,6 +108,6 @@ impl Git for GitImpl {
 			});
 		}
 
-		Ok(())
+		Ok(true)
 	}
 }

src/gitopolis.rs

@@ -106,14 +106,17 @@ impl Gitopolis {
 					&clone_remote.url,
 					Some(clone_remote_name),
 				) {
-					Ok(()) => {
-						// Add all other remotes
+					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;

tests/end_to_end_tests.rs

@@ -2673,3 +2673,67 @@ 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

@@ -519,8 +519,8 @@ impl Git for FakeGit {
 		path: &str,
 		url: &str,
 		_remote_name: Option<&str>,
-	) -> Result<(), GitopolisError> {
+	) -> Result<bool, GitopolisError> {
 		(self.clone_callback)(path.to_owned(), url.to_owned());
-		Ok(())
+		Ok(true)
 	}
 }