jj bisect run --trust-endpoints by badp · Pull Request #10016 · jj-vcs/jj

badp · 2026-08-19T21:45:51Z

This commit implements #9188. This used to be a "backend" commit and a "frontend" commit; unfortunately, both commits ended up modifying both src/bisect.rs and commands/bisect/run.rs; I have then decided to squash them together.

The most interesting implementation decision has been determining what exactly IS an endpoint; particularly, identifying the revisions that we can expect to be bad. According to git grep, the entire concept of "endpoints" is novel to this codebase. (Not sure I like the name...)

My first choice was parents(roots(REVSETS)). There's not much to say about it; as a choice, it seemed to fit every test that was already in the codebase. Martin, however, pointed out a new scenario: what if you take the setup in test_bisect_nonlinear and bisect over commits 0|1|3|5|7? It would be desirable to check that commit 7's other parent, commit 6, is also good. (Let's call this the missing parent usecase.)

Martin then suggested I use a slightly different expression: parents(REVSETS) ~ REVSETS. That does handle the missing parent usecase! But it also broke the "Gaps in the input range are allowed" usecase in test_bisect_linear: if REVSET is "1|2|4|7", that makes the bad endpoints 0, 3 and 6. But if we assert that commit 6 has to be good, that should imply that the first bad commit has to be commit 7. Instead, the bisection_steps remain evaluating commits 2 and 4.

This reveals another implementation decision that I have (implicitly) made: the results of the precondition checks are NOT used to populate the rest of the bisector. This turns out to matter depending on how you define an endpoint!

The next idea, and what's currently implemented, is then parents(connected(REVSETS)) ~ connected(REVSETS). I'm not well versed enough in the revset language to determine if this any different to parents(roots(REVSETS)); both will handle gaps in the input, but both will also fail to handle missing parents! connected(0|1|3|5|7) does include commit 6.

Given that we weren't able (yet) to find a good/simple definition of these endpoints (and, following feedback from Joseph, we actually do quote these definitions in the --help blurb for jj bisect run), I wonder if these expressions should've been added to the revset language itself; something like "root_endpoints()" and "head_endpoints()"...

martinvonz

Thanks

martinvonz · 2026-08-31T05:42:28Z

+    let root_commit_range = ResolvedRevsetExpression::commit(root_commit.id().clone());
+    let commit6_range = ResolvedRevsetExpression::commit(commit6.id().clone());
+    let commit7_range = ResolvedRevsetExpression::commit(commit7.id().clone());


nit: Let's replace _range by _expr here to better match how we typically name this kind of variables. The _range confused me because it made me think they're revset ranges (things like x..y)

martinvonz · 2026-08-31T05:50:18Z

+    let input_range = commit7_range.ancestors();
+    let input_range_without_root = input_range.minus(&root_commit_range);
+    let input_range_without_merge = input_range.minus(&commit7_range);
+    let input_range_single_branch = commit6_range.ancestors().union(&commit7_range);


Something like root_commit_expr.range(&commit_6_expr).union(&commit7_expr) should be better if we want to test that commit 5 gets verified. If we include the root commit as well (as .ancestors() does), then the connected() thing we added ends up including all commits. (And when you pass a non-contiguous range like 0::6|7, then you're basically telling jj bisect run to trust you that the first bad commit is one of the commits the range, not somewhere in a gap in the range.)

whoa! if I use that as the REVSET, every single commit on the other side of the diamond gets tested.

it's obvious that I don't understand jj revsets nearly as well as I thought I did

I don't see why that happens. Let us know if you figure it out.

This commit implements jj-vcs#9188. This used to be a "backend" commit and a "frontend" commit; unfortunately, both commits ended up modifying both src/bisect.rs and commands/bisect/run.rs; I have then decided to squash them together. The most interesting implementation decision has been determining what exactly IS an endpoint; particularly, identifying the revisions that we can expect to be bad. According to git grep, the entire concept of "endpoints" is novel to this codebase. (Not sure I like the name...) My first choice was `parents(roots(REVSETS))`. There's not much to say about it; as a choice, it seemed to fit every test that was already in the codebase. Martin, however, pointed out a new scenario: what if you take the setup in test_bisect_nonlinear and bisect over commits 0|1|3|5|7? It would be desirable to check that commit 7's other parent, commit 6, is also good. (Let's call this the missing parent usecase.) Martin then suggested I use a slightly different expression: `parents(REVSETS) ~ REVSETS`. That does handle the missing parent usecase! But it also broke the "Gaps in the input range are allowed" usecase in test_bisect_linear: if REVSET is "1|2|4|7", that makes the bad endpoints 0, 3 and 6. But if we assert that commit 6 has to be good, that should imply that the first bad commit has to be commit 7. Instead, the bisection_steps remain evaluating commits 2 and 4. This reveals another implementation decision that I have (implicitly) made: the results of the precondition checks are NOT used to populate the rest of the bisector. This turns out to matter depending on how you define an endpoint! The next idea, and what's currently implemented, is then `parents(connected(REVSETS)) ~ connected(REVSETS)`. I'm not well versed enough in the revset language to determine if this any different to `parents(roots(REVSETS))`; both will handle gaps in the input, but both will also fail to handle missing parents! connected(0|1|3|5|7) *does* include commit 6. Given that we weren't able (yet) to find a good/simple definition of these endpoints (and, following feedback from Joseph, we actually do quote these definitions in the --help blurb for `jj bisect run`), I wonder if these expressions should've been added to the revset language itself; something like "root_endpoints()" and "head_endpoints()"...

badp requested a review from a team as a code owner August 19, 2026 21:45

badp changed the title ~~Bisect/verify endpoints~~ jj bisect run --verify-endpoints Aug 19, 2026

badp marked this pull request as draft August 19, 2026 22:00

badp force-pushed the bisect/verify-endpoints branch 5 times, most recently from 68b41c1 to 820d512 Compare August 19, 2026 23:07

badp marked this pull request as ready for review August 19, 2026 23:09

josephlou5 reviewed Aug 20, 2026

View reviewed changes

badp force-pushed the bisect/verify-endpoints branch 2 times, most recently from 86142eb to 4198f68 Compare August 20, 2026 08:16

josephlou5 reviewed Aug 20, 2026

View reviewed changes

Comment thread cli/src/commands/bisect/run.rs Outdated

badp force-pushed the bisect/verify-endpoints branch 2 times, most recently from 6a69afb to 6fb6f0e Compare August 20, 2026 22:31

badp changed the title ~~jj bisect run --verify-endpoints~~ jj bisect run --trust-endpoints Aug 20, 2026

josephlou5 reviewed Aug 20, 2026

View reviewed changes

Comment thread cli/src/commands/bisect/run.rs Outdated

Comment thread cli/tests/test_bisect_command.rs Outdated

Comment thread CHANGELOG.md Outdated

martinvonz reviewed Aug 22, 2026

View reviewed changes

Comment thread cli/src/commands/bisect/run.rs Outdated

Comment thread cli/src/commands/bisect/run.rs Outdated

Comment thread cli/tests/test_bisect_command.rs Outdated

Comment thread cli/tests/test_bisect_command.rs

badp force-pushed the bisect/verify-endpoints branch 6 times, most recently from 2f29240 to f08672d Compare August 27, 2026 21:24

martinvonz reviewed Aug 27, 2026

View reviewed changes

Comment thread cli/src/commands/bisect/run.rs Outdated

Comment thread cli/src/commands/bisect/run.rs Outdated

badp force-pushed the bisect/verify-endpoints branch 5 times, most recently from f47c085 to eb34bf2 Compare August 29, 2026 21:55

josephlou5 reviewed Aug 29, 2026

View reviewed changes

Comment thread CHANGELOG.md

Comment thread cli/tests/test_bisect_command.rs

badp force-pushed the bisect/verify-endpoints branch 2 times, most recently from 876c9dd to 2dfe5b7 Compare August 30, 2026 10:53

martinvonz approved these changes Aug 31, 2026

View reviewed changes

badp force-pushed the bisect/verify-endpoints branch from 2dfe5b7 to 3b30a6c Compare August 31, 2026 06:03

badp force-pushed the bisect/verify-endpoints branch from 3b30a6c to 78d5adf Compare August 31, 2026 06:16

Uh oh!

Conversation

badp commented Aug 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

martinvonz left a comment

Choose a reason for hiding this comment

Uh oh!

martinvonz Aug 31, 2026

Choose a reason for hiding this comment

Uh oh!

martinvonz Aug 31, 2026

Choose a reason for hiding this comment

Uh oh!

badp Aug 31, 2026

Choose a reason for hiding this comment

Uh oh!

martinvonz Aug 31, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

badp commented Aug 19, 2026 •

edited

Loading