refactor(config): route all git calls through internal/git.Git#54
refactor(config): route all git calls through internal/git.Git#54cds-amal wants to merge 1 commit intoboneskull:mainfrom
Conversation
config.Config previously shelled out to git directly via exec.Command, bypassing the safeexec.LookPath resolution that internal/git provides. This adds ConfigGet/ConfigSet/ConfigUnset/ConfigSetWithComment/ConfigGetRegexp methods to git.Git, rewires Config to accept a *git.Git dependency (Load(path) becomes New(g)), and flips the construction order in all cmd/ call sites so git.New runs first.
There was a problem hiding this comment.
Pull request overview
This PR refactors the config package to route all git configuration calls through internal/git.Git, ensuring consistent PATH resolution via safeexec.LookPath for security. Previously, config.Config directly invoked exec.Command("git"), bypassing the security measures in place.
Changes:
- Added five new config-related methods to git.Git (ConfigGet, ConfigSet, ConfigUnset, ConfigSetWithComment, ConfigGetRegexp) plus IsAncestor and GetForkPoint helper methods
- Refactored config.Config to accept a *git.Git dependency instead of a repoPath, changing Load(path) to New(g)
- Updated all cmd/ files to instantiate git.New before config.New and pass the git instance as a dependency
Reviewed changes
Copilot reviewed 23 out of 23 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| internal/git/git.go | Added ConfigGet/Set/Unset/SetWithComment/GetRegexp methods, plus IsAncestor and GetForkPoint helpers |
| internal/config/config.go | Refactored Config struct to hold git.Git instead of repoPath; replaced direct exec.Command calls with g.Config methods; removed unnecessary string trimming |
| internal/config/config_test.go | Updated all tests to use git.New() and config.New(g); added test for SetForkPointWithComment |
| internal/tree/tree_test.go | Updated setupTestRepo to return *git.Git and use config.New(g) |
| cmd/*.go (11 files) | Flipped initialization order to call git.New before config.New in all commands |
| cmd/*_test.go (8 files) | Updated test setup to use git.New() before config.New(g) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "testing" | ||
|
|
||
| "github.com/boneskull/gh-stack/internal/config" | ||
| "github.com/boneskull/gh-stack/internal/git" |
There was a problem hiding this comment.
It's pulling in the Git wrapper defined in this commit.
| // SetForkPointWithComment stores the fork point SHA with an inline comment. | ||
| // Requires Git 2.45+. The comment appears after the value on the same line. | ||
| // Returns an error with stderr content on failure, so callers can distinguish | ||
| // "unknown option" (old git) from other failures. | ||
| func (c *Config) SetForkPointWithComment(branch, sha, comment string) error { | ||
| key := "branch." + branch + ".stackForkPoint" | ||
| return c.g.ConfigSetWithComment(key, sha, comment) |
There was a problem hiding this comment.
I don't understand what this is for -- we only seem to be using it in testing?
There was a problem hiding this comment.
It's used by the doctor command in the next PR in the stack (feat/whats-up-doc). When doctor fixes a stale fork point, it records the old SHA as an inline git config comment for provenance:
// cmd/doctor.go (in the next PR)
func setForkPointWithComment(s *style.Style, cfg *config.Config, branch, oldSHA, newSHA string) error {
comment := fmt.Sprintf("replaces %s (%s)",
git.AbbrevSHA(oldSHA),
time.Now().UTC().Format(time.RFC3339),
)
err := cfg.SetForkPointWithComment(branch, newSHA, comment)
// ... falls back to plain SetForkPoint on Git < 2.45
}I introduced the plumbing here (rather than in the doctor PR) since this PR is already rewiring all the config methods through git.Git; it seemed cleaner to add ConfigSetWithComment alongside ConfigGet/ConfigSet/ConfigUnset in one pass rather than touching git.go again in a follow-up. Feel free to remove it, but if you decide to go with the doctor pr, it will be another conflict touchpoint to resolve (if removed).
config.Config previously shelled out to git directly via exec.Command, bypassing the safeexec.LookPath resolution that internal/git provides. This adds ConfigGet/ConfigSet/ConfigUnset/ConfigSetWithComment/ConfigGetRegexp methods to git.Git, rewires Config to accept a *git.Git dependency (Load(path) becomes New(g)), and flips the construction order in all cmd/ call sites so git.New runs first.