refactor: extract common helpers for sync-branch hook checks (bd-e0o7)
Extract two helper functions from checkSyncBranchHookQuick and checkSyncBranchHookCompatibility to reduce code duplication: 1. getPrePushHookPath(path) - resolves pre-push hook path handling both standard .git/hooks and shared hooks via core.hooksPath 2. extractBdHookVersion(content) - parses version from hook content looking for bd-hooks-version: marker Also documented the intentional behavior difference: - Quick check: returns OK for custom hooks (silent) - Full check: returns Warning for custom hooks (user awareness) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
This commit is contained in:
147
cmd/bd/doctor.go
147
cmd/bd/doctor.go
@@ -608,6 +608,58 @@ func checkHooksQuick() string {
|
|||||||
return fmt.Sprintf("Git hooks outdated: %s (%s → %s)", strings.Join(outdatedHooks, ", "), oldestVersion, Version)
|
return fmt.Sprintf("Git hooks outdated: %s (%s → %s)", strings.Join(outdatedHooks, ", "), oldestVersion, Version)
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// getPrePushHookPath resolves the pre-push hook path for a git repository.
|
||||||
|
// Handles both standard .git/hooks and shared hooks via core.hooksPath.
|
||||||
|
// Returns (hookPath, error) where error is set if not a git repo.
|
||||||
|
// (bd-e0o7: extracted common helper)
|
||||||
|
func getPrePushHookPath(path string) (string, error) {
|
||||||
|
// Get git directory (handles worktrees where .git is a file)
|
||||||
|
cmd := exec.Command("git", "rev-parse", "--git-dir")
|
||||||
|
cmd.Dir = path
|
||||||
|
output, err := cmd.Output()
|
||||||
|
if err != nil {
|
||||||
|
return "", fmt.Errorf("not a git repository")
|
||||||
|
}
|
||||||
|
gitDir := strings.TrimSpace(string(output))
|
||||||
|
if !filepath.IsAbs(gitDir) {
|
||||||
|
gitDir = filepath.Join(path, gitDir)
|
||||||
|
}
|
||||||
|
|
||||||
|
// Check for shared hooks via core.hooksPath first
|
||||||
|
hooksPathCmd := exec.Command("git", "config", "--get", "core.hooksPath")
|
||||||
|
hooksPathCmd.Dir = path
|
||||||
|
if hooksPathOutput, err := hooksPathCmd.Output(); err == nil {
|
||||||
|
sharedHooksDir := strings.TrimSpace(string(hooksPathOutput))
|
||||||
|
if !filepath.IsAbs(sharedHooksDir) {
|
||||||
|
sharedHooksDir = filepath.Join(path, sharedHooksDir)
|
||||||
|
}
|
||||||
|
return filepath.Join(sharedHooksDir, "pre-push"), nil
|
||||||
|
}
|
||||||
|
|
||||||
|
// Use standard .git/hooks location
|
||||||
|
return filepath.Join(gitDir, "hooks", "pre-push"), nil
|
||||||
|
}
|
||||||
|
|
||||||
|
// extractBdHookVersion extracts the version from a bd hook's content.
|
||||||
|
// Returns empty string if not a bd hook or version cannot be determined.
|
||||||
|
// (bd-e0o7: extracted common helper)
|
||||||
|
func extractBdHookVersion(content string) string {
|
||||||
|
if !strings.Contains(content, "bd-hooks-version:") {
|
||||||
|
return "" // Not a bd hook
|
||||||
|
}
|
||||||
|
|
||||||
|
for _, line := range strings.Split(content, "\n") {
|
||||||
|
if strings.Contains(line, "bd-hooks-version:") {
|
||||||
|
parts := strings.SplitN(line, ":", 2)
|
||||||
|
if len(parts) == 2 {
|
||||||
|
return strings.TrimSpace(parts[1])
|
||||||
|
}
|
||||||
|
break
|
||||||
|
}
|
||||||
|
}
|
||||||
|
return ""
|
||||||
|
}
|
||||||
|
|
||||||
// checkSyncBranchHookQuick does a fast check for sync-branch hook compatibility (issue #532).
|
// checkSyncBranchHookQuick does a fast check for sync-branch hook compatibility (issue #532).
|
||||||
// Returns empty string if OK, otherwise returns issue description.
|
// Returns empty string if OK, otherwise returns issue description.
|
||||||
func checkSyncBranchHookQuick(path string) string {
|
func checkSyncBranchHookQuick(path string) string {
|
||||||
@@ -617,56 +669,19 @@ func checkSyncBranchHookQuick(path string) string {
|
|||||||
return "" // sync-branch not configured, nothing to check
|
return "" // sync-branch not configured, nothing to check
|
||||||
}
|
}
|
||||||
|
|
||||||
// Get git directory
|
hookPath, err := getPrePushHookPath(path)
|
||||||
cmd := exec.Command("git", "rev-parse", "--git-dir")
|
|
||||||
cmd.Dir = path
|
|
||||||
output, err := cmd.Output()
|
|
||||||
if err != nil {
|
if err != nil {
|
||||||
return "" // Not a git repo, skip
|
return "" // Not a git repo, skip
|
||||||
}
|
}
|
||||||
gitDir := strings.TrimSpace(string(output))
|
|
||||||
if !filepath.IsAbs(gitDir) {
|
|
||||||
gitDir = filepath.Join(path, gitDir)
|
|
||||||
}
|
|
||||||
|
|
||||||
// Find pre-push hook (check shared hooks first)
|
|
||||||
var hookPath string
|
|
||||||
hooksPathCmd := exec.Command("git", "config", "--get", "core.hooksPath")
|
|
||||||
hooksPathCmd.Dir = path
|
|
||||||
if hooksPathOutput, err := hooksPathCmd.Output(); err == nil {
|
|
||||||
sharedHooksDir := strings.TrimSpace(string(hooksPathOutput))
|
|
||||||
if !filepath.IsAbs(sharedHooksDir) {
|
|
||||||
sharedHooksDir = filepath.Join(path, sharedHooksDir)
|
|
||||||
}
|
|
||||||
hookPath = filepath.Join(sharedHooksDir, "pre-push")
|
|
||||||
} else {
|
|
||||||
hookPath = filepath.Join(gitDir, "hooks", "pre-push")
|
|
||||||
}
|
|
||||||
|
|
||||||
content, err := os.ReadFile(hookPath) // #nosec G304 - path is controlled
|
content, err := os.ReadFile(hookPath) // #nosec G304 - path is controlled
|
||||||
if err != nil {
|
if err != nil {
|
||||||
return "" // No pre-push hook, covered by other checks
|
return "" // No pre-push hook, covered by other checks
|
||||||
}
|
}
|
||||||
|
|
||||||
// Check if bd hook and extract version
|
hookVersion := extractBdHookVersion(string(content))
|
||||||
hookStr := string(content)
|
|
||||||
if !strings.Contains(hookStr, "bd-hooks-version:") {
|
|
||||||
return "" // Not a bd hook, can't check
|
|
||||||
}
|
|
||||||
|
|
||||||
var hookVersion string
|
|
||||||
for _, line := range strings.Split(hookStr, "\n") {
|
|
||||||
if strings.Contains(line, "bd-hooks-version:") {
|
|
||||||
parts := strings.SplitN(line, ":", 2)
|
|
||||||
if len(parts) == 2 {
|
|
||||||
hookVersion = strings.TrimSpace(parts[1])
|
|
||||||
}
|
|
||||||
break
|
|
||||||
}
|
|
||||||
}
|
|
||||||
|
|
||||||
if hookVersion == "" {
|
if hookVersion == "" {
|
||||||
return "" // Can't determine version
|
return "" // Not a bd hook or can't determine version
|
||||||
}
|
}
|
||||||
|
|
||||||
// Check if version < minSyncBranchHookVersion (when sync-branch bypass was added)
|
// Check if version < minSyncBranchHookVersion (when sync-branch bypass was added)
|
||||||
@@ -2021,6 +2036,7 @@ func checkGitHooks() doctorCheck {
|
|||||||
// checkSyncBranchHookCompatibility checks if pre-push hook is compatible with sync-branch mode.
|
// checkSyncBranchHookCompatibility checks if pre-push hook is compatible with sync-branch mode.
|
||||||
// When sync-branch is configured, the pre-push hook must have the sync-branch bypass logic
|
// When sync-branch is configured, the pre-push hook must have the sync-branch bypass logic
|
||||||
// (added in version 0.29.0). Without it, users experience circular "bd sync" failures (issue #532).
|
// (added in version 0.29.0). Without it, users experience circular "bd sync" failures (issue #532).
|
||||||
|
// (bd-e0o7: refactored to use extracted helpers)
|
||||||
func checkSyncBranchHookCompatibility(path string) doctorCheck {
|
func checkSyncBranchHookCompatibility(path string) doctorCheck {
|
||||||
// Check if sync-branch is configured
|
// Check if sync-branch is configured
|
||||||
syncBranch := syncbranch.GetFromYAML()
|
syncBranch := syncbranch.GetFromYAML()
|
||||||
@@ -2032,11 +2048,8 @@ func checkSyncBranchHookCompatibility(path string) doctorCheck {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
// sync-branch is configured - check pre-push hook version
|
// Get pre-push hook path using common helper
|
||||||
// Get actual git directory (handles worktrees where .git is a file)
|
hookPath, err := getPrePushHookPath(path)
|
||||||
cmd := exec.Command("git", "rev-parse", "--git-dir")
|
|
||||||
cmd.Dir = path
|
|
||||||
output, err := cmd.Output()
|
|
||||||
if err != nil {
|
if err != nil {
|
||||||
return doctorCheck{
|
return doctorCheck{
|
||||||
Name: "Sync Branch Hook Compatibility",
|
Name: "Sync Branch Hook Compatibility",
|
||||||
@@ -2044,27 +2057,6 @@ func checkSyncBranchHookCompatibility(path string) doctorCheck {
|
|||||||
Message: "N/A (not a git repository)",
|
Message: "N/A (not a git repository)",
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
gitDir := strings.TrimSpace(string(output))
|
|
||||||
if !filepath.IsAbs(gitDir) {
|
|
||||||
gitDir = filepath.Join(path, gitDir)
|
|
||||||
}
|
|
||||||
|
|
||||||
// Check for pre-push hook in standard location or shared hooks location
|
|
||||||
var hookPath string
|
|
||||||
|
|
||||||
// First check if core.hooksPath is configured (shared hooks)
|
|
||||||
hooksPathCmd := exec.Command("git", "config", "--get", "core.hooksPath")
|
|
||||||
hooksPathCmd.Dir = path
|
|
||||||
if hooksPathOutput, err := hooksPathCmd.Output(); err == nil {
|
|
||||||
sharedHooksDir := strings.TrimSpace(string(hooksPathOutput))
|
|
||||||
if !filepath.IsAbs(sharedHooksDir) {
|
|
||||||
sharedHooksDir = filepath.Join(path, sharedHooksDir)
|
|
||||||
}
|
|
||||||
hookPath = filepath.Join(sharedHooksDir, "pre-push")
|
|
||||||
} else {
|
|
||||||
// Use standard .git/hooks location
|
|
||||||
hookPath = filepath.Join(gitDir, "hooks", "pre-push")
|
|
||||||
}
|
|
||||||
|
|
||||||
hookContent, err := os.ReadFile(hookPath) // #nosec G304 - path is controlled
|
hookContent, err := os.ReadFile(hookPath) // #nosec G304 - path is controlled
|
||||||
if err != nil {
|
if err != nil {
|
||||||
@@ -2076,10 +2068,13 @@ func checkSyncBranchHookCompatibility(path string) doctorCheck {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
// Check if this is a bd hook and extract version
|
// Extract version using common helper
|
||||||
hookStr := string(hookContent)
|
hookVersion := extractBdHookVersion(string(hookContent))
|
||||||
if !strings.Contains(hookStr, "bd-hooks-version:") {
|
|
||||||
// Not a bd hook - can't determine compatibility
|
// Not a bd hook - this is intentionally a Warning (vs OK in quick check)
|
||||||
|
// because the full doctor check wants to alert users to potential issues
|
||||||
|
// with custom hooks that may not be sync-branch compatible
|
||||||
|
if !strings.Contains(string(hookContent), "bd-hooks-version:") {
|
||||||
return doctorCheck{
|
return doctorCheck{
|
||||||
Name: "Sync Branch Hook Compatibility",
|
Name: "Sync Branch Hook Compatibility",
|
||||||
Status: statusWarning,
|
Status: statusWarning,
|
||||||
@@ -2088,18 +2083,6 @@ func checkSyncBranchHookCompatibility(path string) doctorCheck {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
// Extract version from hook
|
|
||||||
var hookVersion string
|
|
||||||
for _, line := range strings.Split(hookStr, "\n") {
|
|
||||||
if strings.Contains(line, "bd-hooks-version:") {
|
|
||||||
parts := strings.SplitN(line, ":", 2)
|
|
||||||
if len(parts) == 2 {
|
|
||||||
hookVersion = strings.TrimSpace(parts[1])
|
|
||||||
}
|
|
||||||
break
|
|
||||||
}
|
|
||||||
}
|
|
||||||
|
|
||||||
if hookVersion == "" {
|
if hookVersion == "" {
|
||||||
return doctorCheck{
|
return doctorCheck{
|
||||||
Name: "Sync Branch Hook Compatibility",
|
Name: "Sync Branch Hook Compatibility",
|
||||||
|
|||||||
Reference in New Issue
Block a user