fix: check all git config scopes, not just --global#21
fix: check all git config scopes, not just --global#21jerryjrxie wants to merge 4 commits intoopenbootdotdev:mainfrom
Conversation
The previous implementation only checked --global git config, but users may have their git identity configured in: - Local repository config (.git/config) - System config (/etc/gitconfig) - Other scopes This change first tries --global, then falls back to checking all scopes if --global returns empty. This ensures we detect git configuration regardless of where it's set. Closes git config detection issue
|
👋 Thanks for opening this pull request! 🎉 This is your first PR to OpenBoot — welcome! Before merging:
@fullstackjam will review this soon. Thanks for contributing! 🚀 |
There was a problem hiding this comment.
Pull request overview
Updates git identity detection to avoid false “Git user information is not configured” warnings by checking additional git config scopes beyond --global.
Changes:
- Update
GetGitConfigto trygit config --global <key>first, then fall back togit config <key>(any scope) when global is empty/unset.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
internal/system/system.go
Outdated
|
|
||
| // Fall back to any available config (local, system, etc.) | ||
| output, err = RunCommandSilent("git", "config", key) | ||
| if err == nil { | ||
| return output | ||
| } | ||
| return output | ||
|
|
||
| return "" |
There was a problem hiding this comment.
The blank lines in this function contain trailing whitespace (tabs/spaces). Please run gofmt / remove the whitespace so the file stays clean and avoids noisy diffs in future changes.
| func GetGitConfig(key string) string { | ||
| // Try global first (most common) | ||
| output, err := RunCommandSilent("git", "config", "--global", key) | ||
| if err != nil { | ||
| return "" | ||
| if err == nil && output != "" { | ||
| return output | ||
| } | ||
|
|
||
| // Fall back to any available config (local, system, etc.) | ||
| output, err = RunCommandSilent("git", "config", key) | ||
| if err == nil { | ||
| return output |
There was a problem hiding this comment.
This change adds new behavior (fallback to non-global git config scopes), but the existing tests only validate the global case. Please add a test that sets user.name/user.email in a local repo config (or system config) while keeping global unset, and assert GetGitConfig returns the non-global value to prevent regressions of the original bug.
Adds TestGetGitConfig_FallsBackToAnyScope to verify that GetGitConfig checks all git config scopes (global, local, system) when looking for user.name and user.email, not just --global. Related to git config detection issue
The diffDotfiles function was always checking the local ~/.dotfiles git state, even when comparing snapshots with empty dotfiles URLs. This caused test failures when the user's actual dotfiles repo had uncommitted changes. Fix: Only check dotfiles repo state if at least one URL is configured. If both system and reference have empty dotfiles URLs, skip the git state check entirely. This makes the tests deterministic and not dependent on the user's local dotfiles repo state.
…t config fallback
fullstackjam
left a comment
fullstackjam
left a comment
There was a problem hiding this comment.
The underlying problem is real, but the fix misses the actual callsite:
[CRITICAL] doctor.go's checkGit() is not updated
The warning users actually see comes from checkGit() in internal/cli/doctor.go, which still hardcodes --global. The PR fixes the underlying GetGitConfig helper but leaves the user-facing check broken. Please update checkGit() as well.
[HIGH] Falling back to local scope is semantically wrong here
openboot is a machine setup tool. If a user runs it from inside a repo that has a local user.name set, the fallback will find that value and skip the global git configuration wizard — leaving them with no global identity configured. The fallback should be --global → --system only, not the bare git config which includes local scope.
[LOW] os.Chdir in tests is fragile
os.Chdir modifies process-global state and will cause race conditions if tests ever run in parallel. Consider passing a working directory as a parameter or using dependency injection instead.
Summary
Fixes false-positive "Git user information is not configured" warnings when git identity is set in non-global scopes.
Problem
The previous implementation only checked
--globalgit config:But users may have their git identity configured in:
.git/config)/etc/gitconfig)Solution
Now checks
--globalfirst, then falls back to checking all scopes if empty:Testing
go vetpassesNote
Based on user report that git config warning appears despite git being configured. The fix ensures we detect git identity regardless of where it's set.