Skip to content

Fix switch/list between workspaces that share a commit - #71

Open
banjerluke wants to merge 1 commit into
eersnington:mainfrom
banjerluke:fix/shared-working-copy-switch
Open

Fix switch/list between workspaces that share a commit#71
banjerluke wants to merge 1 commit into
eersnington:mainfrom
banjerluke:fix/shared-working-copy-switch

Conversation

@banjerluke

Copy link
Copy Markdown

First: thank you for making jj-navi! I was delighted to find it and bring some of that worktrunk DX to jj.

I bumped into a problem where it wouldn't switch workspaces anymore. Turns out it was because I had set more than one workspace to the same change, which broke navi switch/navi list. target.current_working_copy() is commit-based, so it can match multiple workspace names after jj edit other@.

This PR fixes that, matching by jj workspace root --name <workspace> when available, then falling back to current_working_copy() only for degraded/older repos where named workspace roots are unavailable (e.g. my main repo for some reason, maybe because it's old).

It also reuses the same root-matching helper for fallback path validation, and adds regressions for switch and list.

Changes made with GPT 5.5 high and tested with cargo test, cargo clippy --all-targets -- -D warnings, and manual verification.

Prefer matching workspace roots by JJ's recorded path when available, but gracefully fall back to JJ's current-working-copy marker when older or degraded workspace metadata lacks recorded paths.

Use the same root-matching helper for fallback path validation so switch and list continue to work when multiple workspaces point at the same working-copy commit.

Add regression coverage for switch and list with shared working-copy commits and missing workspace path records.

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review completed

Re-trigger cubic

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2 issues found across 5 files

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="tests/switch_tests.rs">

<violation number="1" location="tests/switch_tests.rs:52">
P2: Test missing stderr assertion to prevent silent fallback regression from going undetected</violation>
</file>

<file name="src/repo/jj.rs">

<violation number="1" location="src/repo/jj.rs:156">
P2: `workspace_root()` failures are broadly swallowed, risking incorrect commit-based fallback instead of propagating real errors</violation>
</file>

Reply with feedback, questions, or to request a fix.

Re-trigger cubic

Comment thread tests/switch_tests.rs
.args(["switch", "feature-auth"])
.assert()
.success()
.stdout(predicate::eq(format!(

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: Test missing stderr assertion to prevent silent fallback regression from going undetected

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/switch_tests.rs, line 52:

<comment>Test missing stderr assertion to prevent silent fallback regression from going undetected</comment>

<file context>
@@ -37,6 +37,46 @@ fn switch_existing_prints_relative_path() {
+        .args(["switch", "feature-auth"])
+        .assert()
+        .success()
+        .stdout(predicate::eq(format!(
+            "../{}.feature-auth\n",
+            repo.repo_name()
</file context>

Comment thread src/repo/jj.rs
Comment on lines +156 to +158
let Ok(root) = self.workspace_root(&workspace) else {
continue;
};

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: workspace_root() failures are broadly swallowed, risking incorrect commit-based fallback instead of propagating real errors

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/repo/jj.rs, line 156:

<comment>`workspace_root()` failures are broadly swallowed, risking incorrect commit-based fallback instead of propagating real errors</comment>

<file context>
@@ -143,19 +143,44 @@ impl<'a> JjClient<'a> {
-            .ok_or(Error::OrphanedWorkspace)?;
+        for name in output.lines().filter(|line| !line.is_empty()) {
+            let workspace = WorkspaceName::new(name.to_owned())?;
+            let Ok(root) = self.workspace_root(&workspace) else {
+                continue;
+            };
</file context>
Suggested change
let Ok(root) = self.workspace_root(&workspace) else {
continue;
};
+ let root = match self.workspace_root(&workspace) {
+ Ok(root) => root,
+ Err(Error::JjCommandFailed { ref stderr, .. })
+ if stderr.contains("no path recorded") || stderr.contains("unknown flag") =>
+ {
+ continue;
+ }
+ Err(e) => return Err(e),
+ };

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant