From ce89fe9d65559b655493a7382faa15949e8f8558 Mon Sep 17 00:00:00 2001 From: Yuya Nishihara Date: Wed, 12 Aug 2026 22:09:20 +0900 Subject: [PATCH 1/3] revset: ensure at_operation(op, filter..) resolution is scoped Since at_operation() creates a resolution scope, the WithinReference node shouldn't be omitted even if no referenced commits exist in it. It prevents outer referenced commits from being propagated to the inner node: at_operation(op, all()) & outer_ref ----- shouldn't include outer_ref --- lib/src/revset.rs | 46 ++++++++++++++++++++++++++++++---------- lib/tests/test_revset.rs | 31 +++++++++++++++++++++++++++ 2 files changed, 66 insertions(+), 11 deletions(-) diff --git a/lib/src/revset.rs b/lib/src/revset.rs index 886a6538ba0..a32cb7fa1a3 100644 --- a/lib/src/revset.rs +++ b/lib/src/revset.rs @@ -342,6 +342,8 @@ pub enum RevsetExpression { commits: Vec, }, /// Resolves visibility within the specified repo state. + /// + /// Commits referenced by the `candidates` expression are also scoped. WithinVisibility { candidates: Arc, /// Copy of `repo.view().heads()` at the operation. @@ -1931,6 +1933,15 @@ where /// User symbols and `at_operation()` scopes should have been resolved. fn resolve_referenced_commits( expression: &Arc>, +) -> TransformedExpression { + // Omit empty node to keep test/debug output concise + let omit_empty = true; + resolve_referenced_commits_sub(expression, omit_empty) +} + +fn resolve_referenced_commits_sub( + expression: &Arc>, + omit_empty: bool, ) -> TransformedExpression { // Trust precomputed value if any if matches!( @@ -1958,12 +1969,15 @@ fn resolve_referenced_commits( } => { // ::visible_heads shouldn't be filtered out by outer inner_commits.extend_from_slice(visible_heads); - let transformed = resolve_referenced_commits(candidates); + // Empty WithinReference node shouldn't be omitted because + // at_operation() creates a new resolution scope. + let transformed = resolve_referenced_commits_sub(candidates, false); // Referenced commits shouldn't be filtered out by outer - if let RevsetExpression::WithinReference { commits, .. } = - transformed.as_deref().unwrap_or(candidates) - { - inner_commits.extend_from_slice(commits); + match transformed.as_deref().unwrap_or(candidates) { + RevsetExpression::WithinReference { commits, .. } => { + inner_commits.extend_from_slice(commits); + } + _ => unreachable!("WithinReference should never be omitted"), } ControlFlow::Break(transformed.map(|candidates| { Arc::new(RevsetExpression::WithinVisibility { @@ -1985,8 +1999,7 @@ fn resolve_referenced_commits( // Commits could be deduplicated here, but they'll be concatenated with // the visible heads later, which may have duplicates. outer_commits.extend(inner_commits); - if outer_commits.is_empty() { - // Omit empty node to keep test/debug output concise + if omit_empty && outer_commits.is_empty() { return transformed; } Some(Arc::new(RevsetExpression::WithinReference { @@ -4593,7 +4606,9 @@ mod tests { ) "#); - // Inner scope has no references, so WithinReference should be omitted. + // Inner scope has no references, but WithinReference isn't omitted + // because commits referenced by sibling expressions shouldn't be + // visible to the inner expression. insta::assert_debug_snapshot!( resolve_referenced_commits( &visibility2 @@ -4605,7 +4620,10 @@ mod tests { candidates: Union( Intersection( WithinVisibility { - candidates: Filter(HasConflict), + candidates: WithinReference { + candidates: Filter(HasConflict), + commits: [], + }, visible_heads: [ CommitId("200000"), ], @@ -4655,7 +4673,10 @@ mod tests { ], }, WithinVisibility { - candidates: Filter(HasConflict), + candidates: WithinReference { + candidates: Filter(HasConflict), + commits: [], + }, visible_heads: [ CommitId("200000"), ], @@ -4899,7 +4920,10 @@ mod tests { })), @r#" WithinReference { candidates: WithinVisibility { - candidates: CommitRef(Bookmarks(Pattern(Substring("")))), + candidates: WithinReference { + candidates: CommitRef(Bookmarks(Pattern(Substring("")))), + commits: [], + }, visible_heads: [ CommitId("012345"), ], diff --git a/lib/tests/test_revset.rs b/lib/tests/test_revset.rs index 8ddc3cabee7..1b96a07c2a0 100644 --- a/lib/tests/test_revset.rs +++ b/lib/tests/test_revset.rs @@ -4239,6 +4239,37 @@ fn test_evaluate_expression_at_operation() -> TestResult { ] ); + // Visibility and referenced commits resolution between sub expressions: + // each at_operation() node should create its own scope. + assert_eq!( + resolve_commit_ids( + repo2.as_ref(), + "at_operation(@, all()) & at_operation(@-, all())" + ), + vec![commit2_op1.id().clone(), root_commit.id().clone()] + ); + assert_eq!( + resolve_commit_ids( + repo2.as_ref(), + "at_operation(@, commit1_ref) & at_operation(@-, commit1_ref)" + ), + vec![] + ); + assert_eq!( + resolve_commit_ids( + repo2.as_ref(), + "at_operation(@, commit1_ref) & at_operation(@-, all())" + ), + vec![] + ); + assert_eq!( + resolve_commit_ids( + repo2.as_ref(), + "at_operation(@, all()) & at_operation(@-, commit1_ref)" + ), + vec![] + ); + // Operation is resolved relative to the outer ReadonlyRepo. assert_eq!( resolve_commit_ids(repo2.as_ref(), "at_operation(@-, at_operation(@-, all()))"), From 8c19c96164b0ea8d526bd74ee403a3dc2b3d89bc Mon Sep 17 00:00:00 2001 From: Yuya Nishihara Date: Wed, 12 Aug 2026 22:11:37 +0900 Subject: [PATCH 2/3] revset: extract helper that wraps expression in "resolved" at_operation() node --- lib/src/revset.rs | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/lib/src/revset.rs b/lib/src/revset.rs index a32cb7fa1a3..c22504430bd 100644 --- a/lib/src/revset.rs +++ b/lib/src/revset.rs @@ -616,6 +616,18 @@ impl RevsetExpression { }) } + /// Resolves visibility within the given repo. + /// + /// This is equivalent to `at_operation(repo.op_id(), self)`, except that + /// `CommitRef`s are resolved separately. Commits referenced by the `self` + /// expression are also scoped. + pub fn within_visibility(self: &Arc, repo: &dyn Repo) -> Arc { + Arc::new(Self::WithinVisibility { + candidates: self.clone(), + visible_heads: repo.view().heads().iter().cloned().collect(), + }) + } + /// Suppresses name resolution error within `self`. pub fn present(self: &Arc) -> Arc { Arc::new(Self::Present(self.clone())) @@ -3092,12 +3104,9 @@ impl ExpressionStateFolder let repo = reload_repo_at_operation(self.repo(), operation)?; self.repo_stack.push(repo); let candidates = self.fold_expression(candidates)?; - let visible_heads = self.repo().view().heads().iter().cloned().collect(); + let expression = candidates.within_visibility(self.repo()); self.repo_stack.pop(); - Ok(Arc::new(RevsetExpression::WithinVisibility { - candidates, - visible_heads, - })) + Ok(expression) } } From 0d0bc3c3c815dd4b33cc5773ead4839d2f525968 Mon Sep 17 00:00:00 2001 From: Yuya Nishihara Date: Wed, 12 Aug 2026 20:23:32 +0900 Subject: [PATCH 3/3] cli: log: resolve query intersected with prioritize expression independently Alternatively, we could insert a WithinVisibility node by resolve_user_expression(). It works, but it can prevent optimization. Fixes #9975 --- CHANGELOG.md | 4 +++ cli/src/commands/log.rs | 17 +++++++++--- cli/tests/test_log_command.rs | 52 +++++++++++++++++++++++++++++++++++ 3 files changed, 69 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 408c8a47efe..41550cac397 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -39,6 +39,10 @@ to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). Git HEAD to the parent of the fresh working-copy commit. [#9936](https://github.com/jj-vcs/jj/issues/9936) +* Fixed crash in `jj log` involving hidden revisions and the + `log-graph-prioritize` revset. + [#9975](https://github.com/jj-vcs/jj/issues/9975) + ## [0.44.0] - 2026-08-05 ### Release highlights diff --git a/cli/src/commands/log.rs b/cli/src/commands/log.rs index 56a2ea42865..c12363e22e6 100644 --- a/cli/src/commands/log.rs +++ b/cli/src/commands/log.rs @@ -183,13 +183,22 @@ pub(crate) async fn cmd_log( return Ok(()); } - let prio_revset = settings.get_string("revsets.log-graph-prioritize")?; - let mut prio_revset = workspace_command.parse_revset(ui, &RevisionArg::from(prio_revset))?; - prio_revset.intersect_with(revset_expression.expression()); - let repo = workspace_command.repo(); let matcher = fileset_expression.to_matcher(); + let prio_revset = { + let text = settings.get_string("revsets.log-graph-prioritize")?; + let mut prio_revset = workspace_command.parse_revset(ui, &RevisionArg::from(text))?; + // Resolve the query within a separate scope so its visibility and + // referenced commits aren't affected by the prioritize expression: + // ` & at_operation(@, )` + let query_expr = revset_expression + .expression() + .within_visibility(repo.as_ref()); + prio_revset.intersect_with(&query_expr); + prio_revset + }; + let store = repo.store(); let diff_renderer = workspace_command.diff_renderer_for_log(&args.diff_format, args.patch)?; let graph_style = GraphStyle::from_settings(settings)?; diff --git a/cli/tests/test_log_command.rs b/cli/tests/test_log_command.rs index 6a9860cb6bc..c413e24cfaa 100644 --- a/cli/tests/test_log_command.rs +++ b/cli/tests/test_log_command.rs @@ -808,6 +808,58 @@ fn test_log_prefix_highlight_counts_hidden_commits() { [EOF] "); } +#[test] +fn test_log_graph_prioritize_hidden_commits() { + let test_env = TestEnvironment::default(); + test_env.run_jj_in(".", ["git", "init", "repo"]).success(); + let work_dir = test_env.work_dir("repo"); + + work_dir.run_jj(["describe", "-m1"]).success(); + work_dir.run_jj(["describe", "-m2"]).success(); + + // Sanity check: the revision "1" should be displayed first + let output = work_dir.run_jj([ + "log", + "--config=revsets.log-graph-prioritize=at_operation(@-, @)", + "-r::(@|at_operation(@-, @))", + ]); + insta::assert_snapshot!(output, @" + ○ qpvuntsm/1 test.user@example.com 2001-02-03 08:05:08 884fe9b9 (hidden) + │ (empty) 1 + │ @ qpvuntsm test.user@example.com 2001-02-03 08:05:09 4e123bae + ├─╯ (empty) 2 + ◆ zzzzzzzz root() 00000000 + [EOF] + "); + + // The revision "1" shouldn't be prioritized because it isn't included in + // the query. + let output = work_dir.run_jj([ + "log", + "--config=revsets.log-graph-prioritize=at_operation(@-, @ | subject(1))", + "-rall()", + ]); + insta::assert_snapshot!(output, @" + @ qpvuntsm test.user@example.com 2001-02-03 08:05:09 4e123bae + │ (empty) 2 + ◆ zzzzzzzz root() 00000000 + [EOF] + "); + + // The revision "2" shouldn't be prioritized because it isn't included in + // the query. + let output = work_dir.run_jj([ + "log", + "--config=revsets.log-graph-prioritize=@ | subject(2)", + "-rat_operation(@-, all())", + ]); + insta::assert_snapshot!(output, @" + ○ qpvuntsm/1 test.user@example.com 2001-02-03 08:05:08 884fe9b9 (hidden) + │ (empty) 1 + ◆ zzzzzzzz root() 00000000 + [EOF] + "); +} #[test] fn test_log_author_format() {