diff --git a/src/git_ops/gix_ops.rs b/src/git_ops/gix_ops.rs index 307f70f..00be733 100644 --- a/src/git_ops/gix_ops.rs +++ b/src/git_ops/gix_ops.rs @@ -589,22 +589,31 @@ impl GitOperations for GixOperations { // 2. Check if submodule has uncommitted changes (unless force is true) if !force && submodule_path.exists() && submodule_path.join(".git").exists() { if let Ok(submodule_repo) = gix::open(&submodule_path) { - // TODO: properly implement this - // Check for uncommitted changes - // Note: gix status API is complex, for now we'll do a simple check - // by looking at the index vs HEAD - let head = submodule_repo.head_commit().ok(); - let index = submodule_repo.index_or_empty().ok(); - - // Simple check: if we can't get head or index, assume there might be changes - if head.is_none() || index.is_none() { - if !force { + // Check for uncommitted changes using gix. + // The is_dirty() method will return true if there are uncommitted changes, + // including untracked files and modifications to tracked files. + match submodule_repo.is_dirty() { + Ok(is_dirty) => { + if is_dirty { + return Err(anyhow::anyhow!( + "Submodule '{}' has uncommitted changes. Use force=true to override.", + path + )); + } + } + Err(err) => { + // If we can't determine dirty status reliably, assume it might have changes return Err(anyhow::anyhow!( - "Submodule '{}' might have uncommitted changes. Use force=true to override.", - path + "Submodule '{}' might have uncommitted changes. Use force=true to override.\nError: {}", + path, err )); } } + } else { + return Err(anyhow::anyhow!( + "Submodule '{}' might have uncommitted changes. Use force=true to override.", + path + )); } } diff --git a/src/git_ops/mod.rs b/src/git_ops/mod.rs index 536e4c9..f36b389 100644 --- a/src/git_ops/mod.rs +++ b/src/git_ops/mod.rs @@ -262,9 +262,59 @@ impl GitOperations for GitOpsManager { |gix| gix.add_submodule(opts), |git2| git2.add_submodule(opts), ) - .or_else(|_| { + .or_else(|git2_err| { let workdir = self.git2_ops.workdir() .ok_or_else(|| anyhow::anyhow!("Repository has no working directory"))?; + + // Clean up potentially partially initialized submodule path before fallback + let sub_path = workdir.join(&opts.path); + if sub_path.exists() { + let _ = std::fs::remove_dir_all(&sub_path); + } + + // git2 also adds the submodule to .gitmodules, which will cause CLI to fail + // if we don't clean it up. + let gitmodules_path = workdir.join(".gitmodules"); + if gitmodules_path.exists() { + // If it fails to read or write we just ignore it as it's a fallback cleanup + if let Ok(content) = std::fs::read_to_string(&gitmodules_path) { + let mut new_content = String::new(); + let mut in_target_section = false; + for line in content.lines() { + if line.starts_with("[submodule \"") { + in_target_section = line.contains(&format!("\"{}\"", opts.name)); + } + if !in_target_section { + new_content.push_str(line); + new_content.push('\n'); + } + } + let _ = std::fs::write(&gitmodules_path, new_content); + } + } + + // Also git2 might have added it to .git/config + let gitconfig_path = workdir.join(".git").join("config"); + if gitconfig_path.exists() { + let _ = std::process::Command::new("git") + .args(["config", "--remove-section", &format!("submodule.{}", opts.name)]) + .current_dir(workdir) + .output(); + } + + // Also git2 might have created the internal git directory + let internal_git_dir = workdir.join(".git").join("modules").join(&opts.name); + if internal_git_dir.exists() { + let _ = std::fs::remove_dir_all(&internal_git_dir); + } + + // And removed from index + let _ = std::process::Command::new("git") + .args(["rm", "--cached", "-r", "--ignore-unmatch"]) + .arg(&opts.path) + .current_dir(workdir) + .output(); + let mut cmd = std::process::Command::new("git"); cmd.current_dir(workdir) .arg("submodule") @@ -283,7 +333,8 @@ impl GitOperations for GitOpsManager { Ok(()) } else { Err(anyhow::anyhow!( - "Failed to add submodule: {}", + "Failed to add submodule (git2 failed with: {}). CLI output: {}", + git2_err, String::from_utf8_lossy(&output.stderr).trim() )) } diff --git a/tests/integration_tests.rs b/tests/integration_tests.rs index dad6b69..f8536c6 100644 --- a/tests/integration_tests.rs +++ b/tests/integration_tests.rs @@ -760,9 +760,32 @@ active = true let remote_repo = harness .create_test_remote("shallow_lib") .expect("Failed to create remote"); + + // We use a file URL since that works locally for Git. + // Note: Git locally defaults to turning off full file-based shallow clone protocols, + // so we need to enable it for testing. + std::process::Command::new("git") + .args(["config", "protocol.file.allow", "always"]) + .current_dir(&harness.work_dir) + .output() + .expect("Failed to configure git protocol"); + + // Also enable `uploadpack.allowFilter` to let git clone shallowly from file URL + std::process::Command::new("git") + .args(["config", "uploadpack.allowFilter", "true"]) + .current_dir(&remote_repo) + .output() + .expect("Failed to configure git uploadpack"); + + std::process::Command::new("git") + .args(["config", "uploadpack.allowAnySHA1InWant", "true"]) + .current_dir(&remote_repo) + .output() + .expect("Failed to configure git uploadpack"); + let remote_url = format!("file://{}", remote_repo.display()); - // Add submodule with shallow flag + // Add submodule with shallow flag (add branch argument to explicitly point to main) let stdout = harness .run_submod_success(&[ "add",