refactor: consolidate hardcoded values and eliminate code redundancy

wasabeef · wasabeef · commit 8624a51f20ba · 2025-07-01T03:50:17.000+09:00
diff --git a/src/commands.rs b/src/commands.rs
@@ -26,7 +26,10 @@ use std::time::Duration;
 use unicode_width::UnicodeWidthStr;
 
 use crate::config::Config;
-use crate::constants::{section_header, CONFIG_FILE_NAME, GIT_REMOTE_PREFIX, WORKTREES_SUBDIR};
+use crate::constants::{
+    section_header, CONFIG_FILE_NAME, GIT_CRITICAL_DIRS, GIT_REMOTE_PREFIX, GIT_RESERVED_NAMES,
+    INVALID_FILESYSTEM_CHARS, MAX_WORKTREE_NAME_LENGTH, WINDOWS_RESERVED_CHARS, WORKTREES_SUBDIR,
+};
 use crate::file_copy;
 use crate::git::{GitWorktreeManager, WorktreeInfo};
 use crate::hooks::{self, HookContext};
@@ -1847,20 +1850,19 @@ pub fn validate_worktree_name(name: &str) -> Result<String> {
     }
 
     // Check for invalid filesystem characters
-    const INVALID_CHARS: &[char] = &['/', '\\', ':', '*', '?', '"', '<', '>', '|', '\0'];
-    if name.chars().any(|c| INVALID_CHARS.contains(&c)) {
+    if name.chars().any(|c| INVALID_FILESYSTEM_CHARS.contains(&c)) {
         return Err(anyhow!(
             "Worktree name contains invalid characters: {}",
-            INVALID_CHARS.iter().collect::<String>()
+            INVALID_FILESYSTEM_CHARS.iter().collect::<String>()
         ));
     }
 
     // Check for names that could conflict with git internals (case insensitive)
-    const RESERVED_NAMES: &[&str] = &[".git", "HEAD", "refs", "hooks", "info", "objects", "logs"];
     let name_lower = name.to_lowercase();
-    if RESERVED_NAMES
-        .iter()
-        .any(|&reserved| reserved.to_lowercase() == name_lower)
+    if name_lower == ".git"
+        || GIT_RESERVED_NAMES
+            .iter()
+            .any(|&reserved| reserved.to_lowercase() == name_lower)
     {
         return Err(anyhow!("Worktree name '{name}' is reserved by git"));
     }
@@ -1896,8 +1898,10 @@ pub fn validate_worktree_name(name: &str) -> Result<String> {
     }
 
     // Check for extremely long names
-    if name.len() > 255 {
-        return Err(anyhow!("Worktree name is too long (max 255 characters)"));
+    if name.len() > MAX_WORKTREE_NAME_LENGTH {
+        return Err(anyhow!(
+            "Worktree name is too long (max {MAX_WORKTREE_NAME_LENGTH} characters)"
+        ));
     }
 
     Ok(name.to_string())
@@ -1981,11 +1985,10 @@ pub fn validate_custom_path(path: &str) -> Result<()> {
     }
 
     // Check for Windows reserved characters (for cross-platform compatibility)
-    const RESERVED_CHARS: &[char] = &['<', '>', ':', '"', '|', '?', '*'];
-    if path.chars().any(|c| RESERVED_CHARS.contains(&c)) {
+    if path.chars().any(|c| WINDOWS_RESERVED_CHARS.contains(&c)) {
         return Err(anyhow!(
             "Custom path contains reserved characters: {}",
-            RESERVED_CHARS.iter().collect::<String>()
+            WINDOWS_RESERVED_CHARS.iter().collect::<String>()
         ));
     }
 
@@ -2015,15 +2018,34 @@ pub fn validate_custom_path(path: &str) -> Result<()> {
 
                 // Check for reserved names in path components
                 if let Some(name_str) = name.to_str() {
-                    const RESERVED_NAMES: &[&str] =
-                        &[".git", "HEAD", "refs", "hooks", "info", "objects", "logs"];
                     let name_lower = name_str.to_lowercase();
-                    if RESERVED_NAMES
+                    if GIT_RESERVED_NAMES
                         .iter()
                         .any(|&reserved| reserved.to_lowercase() == name_lower)
                     {
                         return Err(anyhow!("Path component '{name_str}' is reserved by git"));
                     }
+
+                    // Special check for .git paths - only prevent critical Git metadata directories
+                    if name_str == ".git" {
+                        let components: Vec<_> = path_obj.components().collect();
+                        if components.len() > 1 {
+                            // Check if trying to create in critical Git directories
+                            if let Some(std::path::Component::Normal(next)) = components.get(1) {
+                                if let Some(next_str) = next.to_str() {
+                                    // These directories contain critical Git metadata and should not be used for worktrees
+                                    if GIT_CRITICAL_DIRS.contains(&next_str) {
+                                        return Err(anyhow!(
+                                            "Cannot create worktree in .git/{} directory. \
+                                            This directory contains critical Git metadata.",
+                                            next_str
+                                        ));
+                                    }
+                                }
+                            }
+                        }
+                        // Allow other .git subdirectories (like .git/worktrees-custom, etc.)
+                    }
                 }
             }
             std::path::Component::ParentDir => {
diff --git a/src/constants.rs b/src/constants.rs
@@ -26,26 +26,6 @@ pub const TIME_FORMAT: &str = "%Y-%m-%d %H:%M";
 // Switch Marker
 pub const SWITCH_TO_PREFIX: &str = "SWITCH_TO:";
 
-// Common Messages - Not used currently but available for future refactoring
-#[allow(dead_code)]
-pub const MSG_NO_WORKTREES_FOUND: &str = "• No worktrees found.";
-#[allow(dead_code)]
-pub const MSG_NO_WORKTREES_TO_SEARCH: &str = "• No worktrees to search.";
-#[allow(dead_code)]
-pub const MSG_NO_WORKTREES_TO_DELETE: &str = "• No worktrees to delete.";
-#[allow(dead_code)]
-pub const MSG_NO_WORKTREES_AVAILABLE: &str = "• No worktrees available for deletion.";
-#[allow(dead_code)]
-pub const MSG_NO_WORKTREES_SELECTED: &str = "• No worktrees selected.";
-#[allow(dead_code)]
-pub const MSG_NO_WORKTREES_TO_RENAME: &str = "• No worktrees to rename.";
-#[allow(dead_code)]
-pub const MSG_CURRENT_WORKTREE: &str = "• Already in this worktree.";
-#[allow(dead_code)]
-pub const MSG_CANNOT_DELETE_CURRENT: &str = "  (Cannot delete the current worktree)";
-#[allow(dead_code)]
-pub const MSG_CANNOT_RENAME_CURRENT: &str = "  (Cannot rename the current worktree)";
-
 /// Creates a section header with title and separator
 pub fn section_header(title: &str) -> String {
     let title_formatted = title.bright_cyan().bold();
@@ -72,3 +52,25 @@ pub const DEFAULT_BRANCH_MASTER: &str = "master";
 
 // Configuration
 pub const CONFIG_FILE_NAME: &str = ".git-workers.toml";
+
+// Git internals
+pub const GIT_RESERVED_NAMES: &[&str] = &["HEAD", "refs", "hooks", "info", "objects", "logs"];
+pub const GIT_CRITICAL_DIRS: &[&str] = &["objects", "refs", "hooks", "info", "logs", "worktrees"];
+
+// Filesystem limits
+pub const MAX_WORKTREE_NAME_LENGTH: usize = 255;
+pub const MAX_FILE_SIZE_MB: u64 = 100;
+
+// Special characters
+pub const INVALID_FILESYSTEM_CHARS: &[char] = &['/', '\\', ':', '*', '?', '"', '<', '>', '|', '\0'];
+pub const WINDOWS_RESERVED_CHARS: &[char] = &['<', '>', ':', '"', '|', '?', '*'];
+
+// Timeouts
+pub const STALE_LOCK_TIMEOUT_SECS: u64 = 300; // 5 minutes
+
+// Git constants
+pub const COMMIT_ID_SHORT_LENGTH: usize = 8;
+pub const LOCK_FILE_NAME: &str = "git-workers-worktree.lock";
+
+// Directory depth limits
+pub const MAX_DIRECTORY_DEPTH: usize = 50;
diff --git a/src/file_copy.rs b/src/file_copy.rs
@@ -10,7 +10,7 @@ use std::fs;
 use std::path::{Path, PathBuf};
 
 use crate::config::FilesConfig;
-use crate::constants::WORKTREES_SUBDIR;
+use crate::constants::{MAX_DIRECTORY_DEPTH, MAX_FILE_SIZE_MB, WORKTREES_SUBDIR};
 use crate::git::{GitWorktreeManager, WorktreeInfo};
 
 /// Copies configured files from source to destination worktree
@@ -232,11 +232,8 @@ fn find_source_in_regular_repo(manager: &GitWorktreeManager) -> Result<PathBuf>
     Ok(repo_workdir.to_path_buf())
 }
 
-/// Maximum directory recursion depth to prevent infinite loops
-const MAX_DIRECTORY_DEPTH: usize = 50;
-
-/// Maximum file size for automatic copying (100MB)
-const MAX_FILE_SIZE: u64 = 100 * 1024 * 1024;
+/// Maximum file size for automatic copying in bytes
+const MAX_FILE_SIZE: u64 = MAX_FILE_SIZE_MB * 1024 * 1024;
 
 /// Calculates the total size of a file or directory
 ///
diff --git a/src/git.rs b/src/git.rs
@@ -40,18 +40,13 @@ use std::path::{Path, PathBuf};
 use std::time::Duration;
 
 use crate::constants::{
-    DEFAULT_AUTHOR_UNKNOWN, DEFAULT_BRANCH_DETACHED, DEFAULT_BRANCH_UNKNOWN, DEFAULT_MESSAGE_NONE,
-    GIT_DEFAULT_MAIN_WORKTREE, GIT_REMOTE_PREFIX, TIME_FORMAT,
+    COMMIT_ID_SHORT_LENGTH, DEFAULT_AUTHOR_UNKNOWN, DEFAULT_BRANCH_DETACHED,
+    DEFAULT_BRANCH_UNKNOWN, DEFAULT_MESSAGE_NONE, GIT_DEFAULT_MAIN_WORKTREE, GIT_REMOTE_PREFIX,
+    LOCK_FILE_NAME, STALE_LOCK_TIMEOUT_SECS, TIME_FORMAT,
 };
 
-// Git-specific constants
-const COMMIT_ID_SHORT_LENGTH: usize = 8;
-
-// Lock file name used in .git directory
-const LOCK_FILE_NAME: &str = "git-workers-worktree.lock";
-
-// Maximum age for a lock file before it's considered stale (5 minutes)
-const STALE_LOCK_TIMEOUT: Duration = Duration::from_secs(300);
+// Create Duration from constant for stale lock timeout
+const STALE_LOCK_TIMEOUT: Duration = Duration::from_secs(STALE_LOCK_TIMEOUT_SECS);
 
 /// Simple lock structure for worktree operations
 struct WorktreeLock {
diff --git a/tests/create_worktree_integration_test.rs b/tests/create_worktree_integration_test.rs
@@ -59,13 +59,20 @@ fn test_create_worktree_internal_with_first_pattern() -> Result<()> {
 
     // Verify it's at the same level as the repository
     // The worktree should be a sibling to the test-repo directory
-    let current_dir = std::env::current_dir()?.canonicalize()?;
+    let current_dir = std::env::current_dir()?;
     let repo_parent = current_dir.parent().unwrap();
 
     // Both should have the same parent directory
+    // Use canonicalize to resolve any symlinks for comparison
+    let worktree_parent = worktree_path
+        .canonicalize()?
+        .parent()
+        .unwrap()
+        .to_path_buf();
+    let expected_parent = repo_parent.canonicalize()?;
+
     assert_eq!(
-        worktree_path.canonicalize()?.parent().unwrap(),
-        repo_parent,
+        worktree_parent, expected_parent,
         "Worktree should be at the same level as the repository"
     );
 
diff --git a/tests/integration_test.rs b/tests/integration_test.rs
@@ -4,18 +4,18 @@ use std::process::{Command, Stdio};
 #[test]
 #[ignore = "Interactive test - requires terminal and is time-consuming"]
 fn test_esc_key_handling() {
-    // テスト用のgitリポジトリを作成
+    // Create a git repository for testing
     let temp_dir = tempfile::tempdir().unwrap();
     let repo_path = temp_dir.path();
 
-    // gitリポジトリを初期化
+    // Initialize git repository
     Command::new("git")
         .args(["init"])
         .current_dir(repo_path)
         .output()
         .expect("Failed to init git repo");
 
-    // 初期コミットを作成
+    // Create initial commit
     std::fs::write(repo_path.join("README.md"), "# Test").unwrap();
     Command::new("git")
         .args(["add", "."])
@@ -29,10 +29,10 @@ fn test_esc_key_handling() {
         .output()
         .expect("Failed to commit");
 
-    // バイナリのパスを取得
+    // Get binary path
     let binary_path = std::env::current_dir().unwrap().join("target/debug/gw");
 
-    // プロセスを起動
+    // Start process
     let mut child = Command::new(&binary_path)
         .current_dir(repo_path)
         .stdin(Stdio::piped())
@@ -43,13 +43,13 @@ fn test_esc_key_handling() {
 
     let stdin = child.stdin.as_mut().expect("Failed to get stdin");
 
-    // 1. Create worktreeを選択 (4番目の項目)
+    // 1. Select Create worktree (4th item)
     stdin.write_all(b"\x1b[B\x1b[B\x1b[B\r").unwrap(); // 下矢印3回 + Enter
 
-    // 2. ESCキーを送信してキャンセル
+    // 2. Send ESC key to cancel
     stdin.write_all(b"\x1b").unwrap(); // ESC
 
-    // 3. Exitを選択
+    // 3. Select Exit
     stdin
         .write_all(b"\x1b[B\x1b[B\x1b[B\x1b[B\x1b[B\r")
         .unwrap(); // 下矢印5回 + Enter
@@ -67,7 +67,7 @@ fn test_esc_key_handling() {
     println!("STDOUT:\n{}", stdout);
     println!("STDERR:\n{}", stderr);
 
-    // ESCキーでキャンセルされたことを確認
+    // Confirm that it was cancelled with ESC key
     assert!(stdout.contains("Operation cancelled") || stderr.contains("Operation cancelled"));
 }
 
@@ -77,7 +77,7 @@ fn test_search_esc_handling() {
     let temp_dir = tempfile::tempdir().unwrap();
     let repo_path = temp_dir.path();
 
-    // gitリポジトリを初期化
+    // Initialize git repository
     Command::new("git")
         .args(["init"])
         .current_dir(repo_path)
@@ -109,13 +109,13 @@ fn test_search_esc_handling() {
 
     let stdin = child.stdin.as_mut().expect("Failed to get stdin");
 
-    // 1. Search worktreesを選択 (3番目の項目)
+    // 1. Select Search worktrees (3rd item)
     stdin.write_all(b"\x1b[B\x1b[B\r").unwrap(); // 下矢印2回 + Enter
 
-    // 2. ESCキーを送信してキャンセル
+    // 2. Send ESC key to cancel
     stdin.write_all(b"\x1b").unwrap(); // ESC
 
-    // 3. Exitを選択
+    // 3. Select Exit
     stdin
         .write_all(b"\x1b[B\x1b[B\x1b[B\x1b[B\x1b[B\x1b[B\r")
         .unwrap(); // 下矢印6回 + Enter
@@ -133,6 +133,6 @@ fn test_search_esc_handling() {
     println!("STDOUT:\n{}", stdout);
     println!("STDERR:\n{}", stderr);
 
-    // ESCキーでキャンセルされたことを確認
+    // Confirm that it was cancelled with ESC key
     assert!(stdout.contains("Search cancelled") || stderr.contains("Search cancelled"));
 }
diff --git a/tests/validate_custom_path_test.rs b/tests/validate_custom_path_test.rs
@@ -1,5 +1,6 @@
 use anyhow::Result;
 use git_workers::commands::validate_custom_path;
+use git_workers::constants::GIT_RESERVED_NAMES;
 
 #[test]
 fn test_validate_custom_path_valid_paths() -> Result<()> {
@@ -133,9 +134,7 @@ fn test_validate_custom_path_path_traversal() -> Result<()> {
 
 #[test]
 fn test_validate_custom_path_reserved_names() -> Result<()> {
-    let reserved_names = vec![".git", "HEAD", "refs", "hooks", "info", "objects", "logs"];
-
-    for name in reserved_names {
+    for name in GIT_RESERVED_NAMES {
         // Test reserved name as path component
         let path = format!("test/{}/worktree", name);
         let result = validate_custom_path(&path);
@@ -163,6 +162,33 @@ fn test_validate_custom_path_edge_cases() -> Result<()> {
     // Test path that goes up then down (should be valid)
     assert!(validate_custom_path("../sibling/worktrees/feature").is_ok());
 
+    // Test .git directory - allowed for custom subdirectories but not Git's reserved ones
+    assert!(validate_custom_path(".git/my-custom-worktrees/feature").is_ok());
+    assert!(validate_custom_path(".git/feature-branches/test").is_ok());
+
+    // But critical Git directories should still be protected
+    let result = validate_custom_path(".git/objects/test");
+    assert!(result.is_err());
+    assert!(result
+        .unwrap_err()
+        .to_string()
+        .contains("critical Git metadata"));
+
+    let result = validate_custom_path(".git/refs/worktree");
+    assert!(result.is_err());
+    assert!(result
+        .unwrap_err()
+        .to_string()
+        .contains("critical Git metadata"));
+
+    // Test that .git/worktrees is protected
+    let result = validate_custom_path(".git/worktrees/test");
+    assert!(result.is_err());
+    assert!(result
+        .unwrap_err()
+        .to_string()
+        .contains("critical Git metadata"));
+
     Ok(())
 }
 
diff --git a/tests/validate_worktree_name_test.rs b/tests/validate_worktree_name_test.rs
@@ -1,5 +1,6 @@
 use anyhow::Result;
 use git_workers::commands;
+use git_workers::constants::MAX_WORKTREE_NAME_LENGTH;
 
 #[test]
 fn test_validate_worktree_name_with_ascii() -> Result<()> {
@@ -59,11 +60,11 @@ fn test_validate_worktree_name_reserved() -> Result<()> {
 #[test]
 fn test_validate_worktree_name_length() -> Result<()> {
     // Very long names should fail
-    let long_name = "a".repeat(256);
+    let long_name = "a".repeat(MAX_WORKTREE_NAME_LENGTH + 1);
     assert!(commands::validate_worktree_name(&long_name).is_err());
 
-    // Max length (255) should pass
-    let max_name = "a".repeat(255);
+    // Max length should pass
+    let max_name = "a".repeat(MAX_WORKTREE_NAME_LENGTH);
     assert_eq!(commands::validate_worktree_name(&max_name)?, max_name);
 
     Ok(())
