feat(send): support config files for persistent preferences by omarkohl · Pull Request #26 · omarkohl/jip

omarkohl · 2026-07-04T07:37:59Z

This is a stacked PR¹. Only review commit fff0c49.

PRs:

feat(send): add no-change-comment option #27
➡️ feat(send): support config files for persistent preferences #26 (this PR, base of the stack — can be merged first)

Description

Load flag defaults from ~/.config/jip/config.toml (personal) and
.jip.toml in the repo root (team), repo overriding global and CLI
flags overriding both, so workflow flags like --rebase don't have
to be repeated on every invocation.

Closes #24

Co-Authored-By: Claude Fable 5 [email protected]

A stacked PR is a pull request that depends on other pull requests. The current PR depends on the ones listed below it and MUST NOT be merged before they are merged. The PRs listed above the current one in turn depend on it and won't be merged until the current one is. Learn more about why and how to review. ↩

qodo-code-review · 2026-07-04T07:41:05Z

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (1) 📜 Skill insights (0)

Context used

✅ Tickets: 🎫 Support config file for persistent preferences

1. GlobalPath() ignores XDG fallback 📎 Requirement gap ≡ Correctness

Description

GlobalPath() uses os.UserConfigDir() instead of explicitly resolving
$XDG_CONFIG_HOME/jip/config.toml or ~/.config/jip/config.toml, so it may not search the required
global-config locations. This can prevent global config from loading on platforms where
os.UserConfigDir() is not ~/.config and/or if XDG semantics differ.

Code

internal/config/config.go[R25-35]

+// GlobalPath returns the path of the global config file.
+func GlobalPath() (string, error) {
+	dir := Dir
+	if dir == "" {
+		var err error
+		dir, err = os.UserConfigDir()
+		if err != nil {
+			return "", err
+		}
+	}
+	return filepath.Join(dir, "jip", "config.toml"), nil

Evidence

PR Compliance ID 1 requires loading global config from $XDG_CONFIG_HOME/jip/config.toml when set,
otherwise ~/.config/jip/config.toml. The added GlobalPath() implementation does not reference
XDG_CONFIG_HOME or ~/.config and instead uses os.UserConfigDir(), which may resolve to a
different directory and therefore not meet the required search paths.

Load global config from XDG path or ~/.config
internal/config/config.go[25-35]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`internal/config.GlobalPath()` must attempt to load the global config from `$XDG_CONFIG_HOME/jip/config.toml` when `XDG_CONFIG_HOME` is set, otherwise from `~/.config/jip/config.toml`. The current implementation relies on `os.UserConfigDir()`, which is not guaranteed to map to these required paths.

## Issue Context
Compliance requires specific global config search paths (XDG base dir or `~/.config` fallback). This should be implemented explicitly rather than via `os.UserConfigDir()`.

## Fix Focus Areas
- internal/config/config.go[25-35]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

2. ~~Config dir error aborts~~ ✓ Resolved 🐞 Bug ☼ Reliability

Description

runSend now unconditionally calls config.Load(...) and returns its error, but config.Load
fails when os.UserConfigDir() cannot be resolved (via GlobalPath). This can make jip send fail
to start even when no config files exist and defaults would otherwise work.

Code

cmd/send.go[R121-134]

func runSend(cmd *cobra.Command, args []string) error {
+	cwd, err := os.Getwd()
+	if err != nil {
+		return fmt.Errorf("getting cwd: %w", err)
+	}
+
+	// Apply config file values to flags not set on the command line.
+	cfg, err := config.Load(config.FindRepoRoot(cwd))
+	if err != nil {
+		return err
+	}
+	if err := applySendConfig(cmd.Flags(), cfg); err != nil {
+		return err
+	}

Evidence

runSend now returns immediately on any config.Load error, and config.Load currently errors out
if GlobalPath() fails (which depends on os.UserConfigDir() succeeding), even though missing
config files are explicitly non-fatal.

cmd/send.go[121-134]
internal/config/config.go[25-66]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`jip send` now calls `config.Load(...)` before doing any work. `config.Load` always tries to compute the global config path via `os.UserConfigDir()`, and returns an error if that resolution fails. Because config is an optional feature, this should degrade to “no global config” instead of aborting `send`.

### Issue Context
- `cmd/runSend` returns the error from `config.Load` directly.
- `internal/config.Load` calls `GlobalPath()`, which returns an error when `os.UserConfigDir()` fails, and this prevents even repo-local config (`.jip.toml`) from being used.

### Fix Focus Areas
- internal/config/config.go[25-66]
- cmd/send.go[121-134]

### Suggested fix
- In `config.Load`, treat `GlobalPath()` failures as non-fatal (equivalent to no global config), and continue loading repo config if `repoRoot != ""`.
- Keep parse/read errors for actual config files as fatal (so real misconfigurations are still surfaced).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

omarkohl · 2026-07-04T08:05:38Z

GlobalPath() ignores XDG fallback 📎 Requirement gap ≡ Correctness

os.UserConfigDir() already implements exact XDG_CONFIG_HOME-or-~/.config semantics on Linux; Qodo's suggested fix would have broken jip's documented cross-platform behavior by forcing non-native config paths on macOS/Windows.

omarkohl · 2026-07-04T08:12:19Z

Changes since last push

internal/config/config.go (+11, -10)

index 68c65e2a40..b0af90c0bb 100644
--- a/internal/config/config.go
+++ b/internal/config/config.go
@@ -42,16 +42,17 @@
 func Load(repoRoot string) (map[string]string, error) {
 	merged := make(map[string]string)
 
-	globalPath, err := GlobalPath()
-	if err != nil {
-		return nil, fmt.Errorf("locating global config: %w", err)
-	}
-	global, err := loadFile(globalPath)
-	if err != nil {
-		return nil, err
-	}
-	for k, v := range global {
-		merged[k] = v
+	// The global config is an optional convenience: if its location can't be
+	// determined (e.g. os.UserConfigDir() fails because $HOME is unset),
+	// proceed as if there were no global config rather than aborting.
+	if globalPath, err := GlobalPath(); err == nil {
+		global, err := loadFile(globalPath)
+		if err != nil {
+			return nil, err
+		}
+		for k, v := range global {
+			merged[k] = v
+		}
 	}
 
 	if repoRoot != "" {

internal/config/config_test.go (+36, -0)

index 6dfd492ee8..50553bc9ad 100644
--- a/internal/config/config_test.go
+++ b/internal/config/config_test.go
@@ -89,6 +89,42 @@
 	}
 }
 
+// TestLoad_GlobalConfigDirUnresolvable simulates an environment where
+// os.UserConfigDir() cannot resolve a directory (e.g. $HOME unset in a
+// minimal container). Load should degrade to "no global config" instead of
+// failing outright, since the global config is optional.
+func TestLoad_GlobalConfigDirUnresolvable(t *testing.T) {
+	old := Dir
+	Dir = ""
+	t.Cleanup(func() { Dir = old })
+
+	for _, key := range []string{"HOME", "XDG_CONFIG_HOME", "APPDATA"} {
+		if val, ok := os.LookupEnv(key); ok {
+			if err := os.Unsetenv(key); err != nil {
+				t.Fatal(err)
+			}
+			t.Cleanup(func() {
+				if err := os.Setenv(key, val); err != nil {
+					t.Fatal(err)
+				}
+			})
+		}
+	}
+
+	if _, err := os.UserConfigDir(); err == nil {
+		t.Skip("os.UserConfigDir() did not fail with env vars unset; cannot simulate on this platform")
+	}
+
+	root := writeRepoConfig(t, "base = \"dev\"\n")
+	cfg, err := Load(root)
+	if err != nil {
+		t.Fatalf("Load: %v", err)
+	}
+	if cfg["base"] != "dev" {
+		t.Errorf("base = %q, want %q", cfg["base"], "dev")
+	}
+}
+
 func TestLoad_InvalidTOML(t *testing.T) {
 	setGlobalConfig(t, "")
 	root := writeRepoConfig(t, "rebase = \n")

_{View the diff on GitHub (may include unrelated changes due to rebases since GitHub does not currently implement git range-diff).

View the diff locally (will only work if you fetched the older commit at some point):

git range-diff main d807c1c a73869d

jj interdiff -f d807c1c -t a73869d}

omarkohl · 2026-07-04T09:02:35Z

Changes since last push

cmd/send.go (+3, -18)

index 0000000000..bd2ad62750 100644
--- a/cmd/send.go
+++ b/cmd/send.go
@@ -119,13 +119,13 @@
 }
 
 func runSend(cmd *cobra.Command, args []string) error {
-	cwd, err := os.Getwd()
+	runner, repoRoot, err := workspaceRunner()
 	if err != nil {
-		return fmt.Errorf("getting cwd: %w", err)
+		return err
 	}
 
 	// Apply config file values to flags not set on the command line.
-	cfg, err := config.Load(config.FindRepoRoot(cwd))
+	cfg, err := config.Load(repoRoot)
 	if err != nil {
 		return err
 	}
@@ -167,21 +167,6 @@
 	_, _ = fmt.Fprintf(w, "Auth: %s\n", source)
 
 	// 2. Detect repo from remote.
-<<<<<<< conflict 1 of 1
-+++++++ spnlkopr 9990c9b1 "fix(send): work when run from a repo subdirectory" (new parents)
-	runner, _, err := workspaceRunner()
-	if err != nil {
-		return err
-	}
-%%%%%%% diff from: kmyoknmt 333d59df "build(deps): bump actions/setup-go from 6.4.0 to 6.5.0" (original parents)
-\\\\\\\        to: trpvwuty a73869d0 "feat(send): support config files for persistent preferences" (original revision)
--	cwd, err := os.Getwd()
--	if err != nil {
--		return fmt.Errorf("getting cwd: %w", err)
--	}
- 	runner := jj.NewRunner(cwd)
->>>>>>> conflict 1 of 1 ends
-
 	remoteData, err := runner.GitRemoteList()
 	if err != nil {
 		return fmt.Errorf("listing remotes: %w", err)

internal/config/config.go (+0, -15)

index b0af90c0bb..dbc3040d04 100644
--- a/internal/config/config.go
+++ b/internal/config/config.go
@@ -117,18 +117,3 @@
 		return "", fmt.Errorf("unsupported value type %T (use a string, boolean, integer, or array of strings)", val)
 	}
 }
-
-// FindRepoRoot walks up from dir looking for a .jj directory and returns the
-// containing directory, or "" if dir is not inside a jj repository.
-func FindRepoRoot(dir string) string {
-	for {
-		if fi, err := os.Stat(filepath.Join(dir, ".jj")); err == nil && fi.IsDir() {
-			return dir
-		}
-		parent := filepath.Dir(dir)
-		if parent == dir {
-			return ""
-		}
-		dir = parent
-	}
-}

internal/config/config_test.go (+0, -18)

index 50553bc9ad..e95bf03ac4 100644
--- a/internal/config/config_test.go
+++ b/internal/config/config_test.go
@@ -145,21 +145,3 @@
 		t.Fatal("expected error for nested table")
 	}
 }
-
-func TestFindRepoRoot(t *testing.T) {
-	root := t.TempDir()
-	if err := os.MkdirAll(filepath.Join(root, ".jj"), 0o700); err != nil {
-		t.Fatal(err)
-	}
-	nested := filepath.Join(root, "a", "b")
-	if err := os.MkdirAll(nested, 0o700); err != nil {
-		t.Fatal(err)
-	}
-
-	if got := FindRepoRoot(nested); got != root {
-		t.Errorf("FindRepoRoot(%q) = %q, want %q", nested, got, root)
-	}
-	if got := FindRepoRoot(t.TempDir()); got != "" {
-		t.Errorf("FindRepoRoot outside a repo = %q, want empty", got)
-	}
-}

_{View the diff on GitHub (may include unrelated changes due to rebases since GitHub does not currently implement git range-diff).

View the diff locally (will only work if you fetched the older commit at some point):

git range-diff main a73869d 013ebaf

jj interdiff -f a73869d -t 013ebaf}

Load flag defaults from ~/.config/jip/config.toml (personal) and .jip.toml in the repo root (team), repo overriding global and CLI flags overriding both, so workflow flags like --rebase don't have to be repeated on every invocation. Closes #24 Co-Authored-By: Claude Fable 5 <[email protected]>

omarkohl · 2026-07-04T09:32:21Z

Changes since last push

No code changes (likely just a rebase).

_{View the diff on GitHub (may include unrelated changes due to rebases since GitHub does not currently implement git range-diff).

View the diff locally (will only work if you fetched the older commit at some point):

git range-diff main 013ebaf fff0c49

jj interdiff -f 013ebaf -t fff0c49}

omarkohl mentioned this pull request Jul 4, 2026

feat(send): add no-change-comment option #27

Merged

omarkohl force-pushed the jip/support-config-files-for/trpvwuty branch 2 times, most recently from a73869d to 013ebaf Compare July 4, 2026 09:02

omarkohl mentioned this pull request Jul 4, 2026

fix(send): work when run from a repo subdirectory #28

Merged

omarkohl force-pushed the jip/support-config-files-for/trpvwuty branch from 013ebaf to fff0c49 Compare July 4, 2026 09:32

omarkohl merged commit fff0c49 into main Jul 4, 2026
1 check passed

omarkohl deleted the jip/support-config-files-for/trpvwuty branch July 4, 2026 09:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(send): support config files for persistent preferences#26

feat(send): support config files for persistent preferences#26
omarkohl merged 1 commit into
mainfrom
jip/support-config-files-for/trpvwuty

omarkohl commented Jul 4, 2026 •

edited

Loading

Uh oh!

qodo-code-review Bot commented Jul 4, 2026 •

edited

Loading

Uh oh!

omarkohl commented Jul 4, 2026

Uh oh!

omarkohl commented Jul 4, 2026

Uh oh!

omarkohl commented Jul 4, 2026

Uh oh!

omarkohl commented Jul 4, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

omarkohl commented Jul 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Footnotes

Uh oh!

qodo-code-review Bot commented Jul 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review by Qodo

Uh oh!

omarkohl commented Jul 4, 2026

Uh oh!

omarkohl commented Jul 4, 2026

Changes since last push

Uh oh!

omarkohl commented Jul 4, 2026

Changes since last push

Uh oh!

omarkohl commented Jul 4, 2026

Changes since last push

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

omarkohl commented Jul 4, 2026 •

edited

Loading

qodo-code-review Bot commented Jul 4, 2026 •

edited

Loading