bisect: --trust-endpoints

This commit holds the rest of the implementation for --trust-endpoints: 
the new argument, the new documentation, the new tests.

Author: badp

CHANGELOG.md

@@ -15,6 +15,10 @@ to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
 * `jj git import` in non-colocated repositories no longer imports commits from a
   detached Git HEAD branch.
 
+* `jj bisect run` now runs some consistency checks before proceeding to bisect,
+  making sure that the provided command can indeed tell good and bad commits apart.
+  Use the new flag `--trust-endpoints` to disable these checks.
+ 
 ### Deprecations
 
 ### New features

cli/src/commands/bisect/run.rs

@@ -87,6 +87,12 @@ pub(crate) struct BisectRunArgs {
     /// will abort the bisection, and any other non-zero exit status means the
     /// revision is "bad".
     ///
+    /// In order for bisection to be meaningful, `COMMAND` must succeed for
+    /// every revision in `heads(REVSETS)`, and it must fail for every revision
+    /// in `parents(roots(REVSETS))`; if you are using `--find-good`, these
+    /// checks are reversed. (Note that `parents(roots(REVSETS))` contains
+    /// revisions that are not in `REVSETS`.)
+    ///
     /// The target's commit ID is available to the command in the
     /// `$JJ_BISECT_TARGET` environment variable.
     #[arg(value_name = "COMMAND")]
@@ -106,6 +112,15 @@ pub(crate) struct BisectRunArgs {
     /// good.
     #[arg(long, value_name = "TARGET", default_value_t = false)]
     find_good: bool,
+
+    /// Skip the pre-bisection checks
+    ///
+    /// By default, `COMMAND` will be run on every revision `jj bisect run`
+    /// assumes to be good or bad before bisection actually begins, as detailed
+    /// under the documentation for `COMMAND`. This flag disables these
+    /// checks.
+    #[arg(long)]
+    trust_endpoints: bool,
 }
 
 #[instrument(skip_all)]
@@ -132,7 +147,9 @@ pub(crate) async fn cmd_bisect_run(
 
     let initial_repo = workspace_command.repo().clone();
 
-    let mut bisector = Bisector::new(initial_repo.as_ref(), input_range, false).await?;
+    let mut bisector =
+        Bisector::new(initial_repo.as_ref(), input_range, !args.trust_endpoints).await?;
+
     let bisection_result = loop {
         match bisector.next_step().await? {
             jj_lib::bisect::NextStep::Verify {
@@ -146,7 +163,7 @@ pub(crate) async fn cmd_bisect_run(
                     let mut formatter = ui.stdout_formatter();
                     writeln!(
                         formatter,
-                        "Pre-bisection check: ensuring this commit is {expected:#?}:"
+                        "Pre-bisection check: ensuring this commit is {expected}:"
                     )?;
                     let commit_template = workspace_command.commit_summary_template();
                     commit_template.format(&commit, formatter.as_mut())?;
@@ -163,8 +180,8 @@ pub(crate) async fn cmd_bisect_run(
                     let mut formatter = ui.stdout_formatter();
                     writeln!(
                         formatter,
-                        "Cannot bisect: this commit was expected to be {expected:#?}, but was \
-                         found to be {actual:#?} (exit status: {exit_code}) instead."
+                        "Cannot bisect: this commit was expected to be {expected}, but was found \
+                         to be {actual} (exit status: {exit_code}) instead."
                     )?;
                     break BisectionResult::VerificationFailed;
                 }
@@ -240,7 +257,11 @@ pub(crate) async fn cmd_bisect_run(
         short_operation_hash(initial_repo.op_id())
     )?;
 
-    let target = if args.find_good { "good" } else { "bad" };
+    let target = if args.find_good {
+        Evaluation::Good
+    } else {
+        Evaluation::Bad
+    };
     match bisection_result {
         BisectionResult::VerificationFailed => {
             return Err(user_error("Bisection preconditions failed"));

cli/tests/cli-reference@.md.snap

@@ -362,6 +362,8 @@ cargo test"
 
    Each revision being checked will be directly edited (will become the current working copy) before running this command. The exit status of the command will be used to mark revisions as "good" or "bad": status 0 means "good", 125 means to skip the revision, 127 (command not found) will abort the bisection, and any other non-zero exit status means the revision is "bad".
 
+   In order for bisection to be meaningful, `COMMAND` must succeed for every revision in `heads(REVSETS)`, and it must fail for every revision in `parents(roots(REVSETS))`; if you are using `--find-good`, these checks are reversed. (Note that `parents(roots(REVSETS))` contains revisions that are not in `REVSETS`.)
+
    The target's commit ID is available to the command in the `$JJ_BISECT_TARGET` environment variable.
 * `<ARGS>` — Arguments to pass to the command
 
@@ -379,6 +381,9 @@ cargo test"
    The interpretation of exit statuses will be inverted (excluding special exit statuses), so status 0 means bad and other non-zero statuses mean good.
 
   Default value: `false`
+* `--trust-endpoints` — Skip the pre-bisection checks
+
+   By default, `COMMAND` will be run on every revision `jj bisect run` assumes to be good or bad before bisection actually begins, as detailed under the documentation for `COMMAND`. This flag disables these checks.
 
 
 

cli/tests/test_bisect_command.rs

@@ -71,7 +71,7 @@ fn test_bisect_run() -> TestResult {
     create_commit(&work_dir, "f", &["e"]);
 
     std::fs::write(&bisection_script, ["fail"].join("\0"))?;
-    insta::assert_snapshot!(work_dir.run_jj(["bisect", "run", "--range=..", &bisector_path]), @"
+    insta::assert_snapshot!(work_dir.run_jj(["bisect", "run", "--range=..", "--trust-endpoints", &bisector_path]), @"
     Bisecting: 5 revisions left to test after this (roughly 3 steps)
     Now evaluating: royxmykx dffaa0d4 c | c
     fake-bisector testing commit dffaa0d4daccf6cee70bac3498fae3b3fd5d6b5b
@@ -111,7 +111,7 @@ fn test_bisect_run() -> TestResult {
     // Try with legacy command argument
     std::fs::write(&bisection_script, ["fail"].join("\0"))?;
     // Testing only stderr to avoid a variable op id in the stdout.
-    insta::assert_snapshot!(work_dir.run_jj(["bisect", "run", "--range=..", "--command", &bisector_path]).success().stderr, @"
+    insta::assert_snapshot!(work_dir.run_jj(["bisect", "run", "--range=..", "--trust-endpoints", "--command", &bisector_path]).success().stderr, @"
     Warning: `--command` is deprecated; use positional arguments instead: `jj bisect run --range=... -- $FAKE_BISECTOR_PATH`
     Working copy  (@) now at: nkmrtpmo 1601f7b4 (empty) (no description set)
     Parent commit (@-)      : royxmykx dffaa0d4 c | c
@@ -136,6 +136,169 @@ fn test_bisect_run() -> TestResult {
     Ok(())
 }
 
+#[test]
+// like test_bisect::test_bisect_nonlinear
+// bisecting over commits 0,1,2,3,4,5,6 with 5 being the first bad commit
+// this fails because 6 should also be bad!
+fn test_bisect_run_nonlinear_no_merge() -> TestResult {
+    let mut test_env = TestEnvironment::default();
+    let bisector_path = fake_bisector_path();
+    test_env.set_up_fake_bisector();
+    test_env.run_jj_in(".", ["git", "init", "repo"]).success();
+    let work_dir = test_env.work_dir("repo");
+
+    // h
+    // |\
+    // f g
+    // | |
+    // d e
+    // | |
+    // b c
+    // |/
+    // a
+
+    create_commit(&work_dir, "a", &[]);
+    create_commit(&work_dir, "b", &["a"]);
+    create_commit(&work_dir, "c", &["a"]);
+    create_commit(&work_dir, "d", &["b"]);
+    create_commit(&work_dir, "e", &["c"]);
+    create_commit(&work_dir, "f", &["d"]);
+    create_commit(&work_dir, "g", &["e"]);
+    create_commit(&work_dir, "h", &["f", "g"]);
+
+    // rather than complicating fake_bisector, we try and --find-good
+    insta::assert_snapshot!(work_dir.run_jj(["bisect", "run", "--range=ancestors(parents(h))", "--find-good", "--", &bisector_path, "--require-file=f"]), @"
+    Pre-bisection check: ensuring this commit is good:
+    kmkuslsw 3548fddf f | f
+    fake-bisector testing commit 3548fddfc5b69ba4c5b78a19c1cc6d39e52e5a22
+    Pre-bisection check: ensuring this commit is good:
+    lylxulpl 97949f0f g | g
+    fake-bisector testing commit 97949f0f52afedfc72ef44d63d21266bbfc42774
+    Cannot bisect: this commit was expected to be good, but was found to be bad (exit status: 1) instead.
+    Search complete. To discard any revisions created during search, run:
+      jj op restore 7d0937f0feb3
+    [EOF]
+    ------- stderr -------
+    Working copy  (@) now at: xznxytkn 08be40de (empty) (no description set)
+    Parent commit (@-)      : kmkuslsw 3548fddf f | f
+    Added 0 files, modified 0 files, removed 4 files
+    Working copy  (@) now at: smwtzssm fc3a931c (empty) (no description set)
+    Parent commit (@-)      : lylxulpl 97949f0f g | g
+    Added 3 files, modified 0 files, removed 3 files
+    Error: Bisection preconditions failed
+    [EOF]
+    [exit status: 1]
+    ");
+    insta::assert_snapshot!(get_log_output(&work_dir), @"
+    @  smwtzssmuxzk fc3a931c8da8 '' files:
+    │ ○  nkmrtpmomlro b47137c13576 'h' files: h
+    ╭─┤
+    ○ │  lylxulplsnyw 97949f0f52af 'g' files: g
+    ○ │  znkkpsqqskkl efcb3d331401 'e' files: e
+    ○ │  royxmykxtrkr 991a7501d660 'c' files: c
+    │ ○  kmkuslswpqwq 3548fddfc5b6 'f' files: f
+    │ ○  vruxwmqvtpmx 01a5f35e756c 'd' files: d
+    │ ○  zsuskulnrvyr 123b4d91f6e5 'b' files: b
+    ├─╯
+    ○  rlvkpnrzqnoo 7d980be7a1d4 'a' files: a
+    ◆  zzzzzzzzzzzz 000000000000 '' files:
+    [EOF]
+    ");
+    Ok(())
+}
+// like test_bisect_run_nonlinear_with_merge, but we include the merge commit.
+// that makes it okay for commit f to be the first bad one.
+#[test]
+fn test_bisect_run_nonlinear_with_merge() -> TestResult {
+    let mut test_env = TestEnvironment::default();
+    let bisector_path = fake_bisector_path();
+    test_env.set_up_fake_bisector();
+    test_env.run_jj_in(".", ["git", "init", "repo"]).success();
+    let work_dir = test_env.work_dir("repo");
+
+    // h
+    // |\
+    // f g
+    // | |
+    // d e
+    // | |
+    // b c
+    // |/
+    // a
+
+    create_commit(&work_dir, "a", &[]);
+    create_commit(&work_dir, "b", &["a"]);
+    create_commit(&work_dir, "c", &["a"]);
+    create_commit(&work_dir, "d", &["b"]);
+    create_commit(&work_dir, "e", &["c"]);
+    create_commit(&work_dir, "f", &["d"]);
+    create_commit(&work_dir, "g", &["e"]);
+    create_commit(&work_dir, "h", &["f", "g"]);
+
+    insta::assert_snapshot!(work_dir.run_jj(["bisect", "run", "--range=..", "--find-good", "--", &bisector_path, "--require-file=f"]), @"
+    Pre-bisection check: ensuring this commit is good:
+    nkmrtpmo b47137c1 h | h
+    fake-bisector testing commit b47137c135761c932a7d0c1619d6121868eb7df9
+    Bisecting: 8 revisions left to test after this (roughly 4 steps)
+    Now evaluating: royxmykx 991a7501 c | c
+    fake-bisector testing commit 991a7501d660abb6a80e8b00f77c651d76d845d7
+    The revision is bad.
+
+    Bisecting: 5 revisions left to test after this (roughly 3 steps)
+    Now evaluating: znkkpsqq efcb3d33 e | e
+    fake-bisector testing commit efcb3d33140196e8aa5157e4c865e9a4c513f8bb
+    The revision is bad.
+
+    Bisecting: 4 revisions left to test after this (roughly 3 steps)
+    Now evaluating: vruxwmqv 01a5f35e d | d
+    fake-bisector testing commit 01a5f35e756cec7f8543120aeedcce0d086a2504
+    The revision is bad.
+
+    Bisecting: 2 revisions left to test after this (roughly 2 steps)
+    Now evaluating: kmkuslsw 3548fddf f | f
+    fake-bisector testing commit 3548fddfc5b69ba4c5b78a19c1cc6d39e52e5a22
+    The revision is good.
+
+    Search complete. To discard any revisions created during search, run:
+      jj op restore 7d0937f0feb3
+    The first good revision is: kmkuslsw 3548fddf f | f
+    [EOF]
+    ------- stderr -------
+    Working copy  (@) now at: xznxytkn 8d14f10f (empty) (no description set)
+    Parent commit (@-)      : nkmrtpmo b47137c1 h | h
+    Working copy  (@) now at: smwtzssm 38d94e5b (empty) (no description set)
+    Parent commit (@-)      : royxmykx 991a7501 c | c
+    Added 0 files, modified 0 files, removed 6 files
+    Working copy  (@) now at: zlusorwl 00dc6326 (empty) (no description set)
+    Parent commit (@-)      : znkkpsqq efcb3d33 e | e
+    Added 1 files, modified 0 files, removed 0 files
+    Working copy  (@) now at: ukzzzykq 6e876421 (empty) (no description set)
+    Parent commit (@-)      : vruxwmqv 01a5f35e d | d
+    Added 2 files, modified 0 files, removed 2 files
+    Working copy  (@) now at: lqoxyrsk 4967f850 (empty) (no description set)
+    Parent commit (@-)      : kmkuslsw 3548fddf f | f
+    Added 1 files, modified 0 files, removed 0 files
+    [EOF]
+    ");
+
+    insta::assert_snapshot!(get_log_output(&work_dir), @"
+    @  lqoxyrskwpry 4967f85032d5 '' files:
+    │ ○  nkmrtpmomlro b47137c13576 'h' files: h
+    ╭─┤
+    │ ○  lylxulplsnyw 97949f0f52af 'g' files: g
+    │ ○  znkkpsqqskkl efcb3d331401 'e' files: e
+    │ ○  royxmykxtrkr 991a7501d660 'c' files: c
+    ○ │  kmkuslswpqwq 3548fddfc5b6 'f' files: f
+    ○ │  vruxwmqvtpmx 01a5f35e756c 'd' files: d
+    ○ │  zsuskulnrvyr 123b4d91f6e5 'b' files: b
+    ├─╯
+    ○  rlvkpnrzqnoo 7d980be7a1d4 'a' files: a
+    ◆  zzzzzzzzzzzz 000000000000 '' files:
+    [EOF]
+    ");
+    Ok(())
+}
+
 #[test]
 fn test_bisect_run_find_first_good() {
     let mut test_env = TestEnvironment::default();
@@ -151,7 +314,7 @@ fn test_bisect_run_find_first_good() {
     create_commit(&work_dir, "e", &["d"]);
     create_commit(&work_dir, "f", &["e"]);
 
-    insta::assert_snapshot!(work_dir.run_jj(["bisect", "run", "--range=..", "--find-good", &bisector_path]), @"
+    insta::assert_snapshot!(work_dir.run_jj(["bisect", "run", "--range=..", "--find-good", "--trust-endpoints", &bisector_path]), @"
     Bisecting: 5 revisions left to test after this (roughly 3 steps)
     Now evaluating: royxmykx dffaa0d4 c | c
     fake-bisector testing commit dffaa0d4daccf6cee70bac3498fae3b3fd5d6b5b
@@ -202,7 +365,13 @@ fn test_bisect_run_missing_bisector() {
     create_commit(&work_dir, "e", &["d"]);
     create_commit(&work_dir, "f", &["e"]);
 
-    let output = work_dir.run_jj(["bisect", "run", "--range=..", "nonexistent"]);
+    let output = work_dir.run_jj([
+        "bisect",
+        "run",
+        "--range=..",
+        "--trust-endpoints",
+        "nonexistent",
+    ]);
     if cfg!(unix) {
         insta::assert_snapshot!(output, @r"
         Bisecting: 5 revisions left to test after this (roughly 3 steps)
@@ -249,7 +418,7 @@ fn test_bisect_run_with_args() {
     create_commit(&work_dir, "e", &["d"]);
     create_commit(&work_dir, "f", &["e"]);
 
-    insta::assert_snapshot!(work_dir.run_jj(["bisect", "run", "--range=..", "--find-good", "--", &bisector_path, "--require-file=c"]), @"
+    insta::assert_snapshot!(work_dir.run_jj(["bisect", "run", "--range=..", "--find-good", "--trust-endpoints", "--", &bisector_path, "--require-file=c"]), @"
     Bisecting: 5 revisions left to test after this (roughly 3 steps)
     Now evaluating: royxmykx dffaa0d4 c | c
     fake-bisector testing commit dffaa0d4daccf6cee70bac3498fae3b3fd5d6b5b
@@ -312,7 +481,7 @@ fn test_bisect_run_crash() -> TestResult {
 
     // bisector crash is equivalent to a failure
     std::fs::write(&bisection_script, ["crash"].join("\0"))?;
-    insta::assert_snapshot!(work_dir.run_jj(["bisect", "run", "--range=..", &bisector_path]), @"
+    insta::assert_snapshot!(work_dir.run_jj(["bisect", "run", "--range=..", "--trust-endpoints", &bisector_path]), @"
     Bisecting: 5 revisions left to test after this (roughly 3 steps)
     Now evaluating: royxmykx dffaa0d4 c | c
     fake-bisector testing commit dffaa0d4daccf6cee70bac3498fae3b3fd5d6b5b
@@ -353,7 +522,7 @@ fn test_bisect_run_abort() -> TestResult {
 
     // stop immediately on failure
     std::fs::write(&bisection_script, ["abort"].join("\0"))?;
-    insta::assert_snapshot!(work_dir.run_jj(["bisect", "run", "--range=..", &bisector_path]), @"
+    insta::assert_snapshot!(work_dir.run_jj(["bisect", "run", "--range=..", "--trust-endpoints", &bisector_path]), @"
     Bisecting: 2 revisions left to test after this (roughly 2 steps)
     Now evaluating: rlvkpnrz 7d980be7 a | a
     fake-bisector testing commit 7d980be7a1d499e4d316ab4c01242885032f7eaf
@@ -386,7 +555,7 @@ fn test_bisect_run_skip() -> TestResult {
     create_commit(&work_dir, "b", &["a"]);
 
     std::fs::write(&bisection_script, ["skip"].join("\0"))?;
-    insta::assert_snapshot!(work_dir.run_jj(["bisect", "run", "--range=..", &bisector_path]), @"
+    insta::assert_snapshot!(work_dir.run_jj(["bisect", "run", "--range=..", "--trust-endpoints", &bisector_path]), @"
     Bisecting: 1 revisions left to test after this (roughly 1 steps)
     Now evaluating: rlvkpnrz 7d980be7 a | a
     fake-bisector testing commit 7d980be7a1d499e4d316ab4c01242885032f7eaf
@@ -422,7 +591,7 @@ fn test_bisect_run_multiple_results() {
     create_commit(&work_dir, "c", &["a"]);
     create_commit(&work_dir, "d", &["c"]);
 
-    insta::assert_snapshot!(work_dir.run_jj(["bisect", "run", "--range=a|b|c|d", &bisector_path]), @"
+    insta::assert_snapshot!(work_dir.run_jj(["bisect", "run", "--range=a|b|c|d", "--trust-endpoints", &bisector_path]), @"
     Bisecting: 2 revisions left to test after this (roughly 2 steps)
     Now evaluating: rlvkpnrz 7d980be7 a | a
     fake-bisector testing commit 7d980be7a1d499e4d316ab4c01242885032f7eaf
@@ -468,7 +637,7 @@ fn test_bisect_run_write_file() -> TestResult {
         &bisection_script,
         ["write new-file\nsome contents", "fail"].join("\0"),
     )?;
-    insta::assert_snapshot!(work_dir.run_jj(["bisect", "run", "--range=..", &bisector_path]), @"
+    insta::assert_snapshot!(work_dir.run_jj(["bisect", "run", "--range=..", "--trust-endpoints", &bisector_path]), @"
     Bisecting: 4 revisions left to test after this (roughly 3 steps)
     Now evaluating: zsuskuln 123b4d91 b | b
     fake-bisector testing commit 123b4d91f6e5e39bfed39bae3bacf9380dc79078
@@ -534,7 +703,7 @@ fn test_bisect_run_jj_command() -> TestResult {
     create_commit(&work_dir, "e", &["d"]);
 
     std::fs::write(&bisection_script, ["jj new -mtesting", "fail"].join("\0"))?;
-    insta::assert_snapshot!(work_dir.run_jj(["bisect", "run", "--range=..", &bisector_path]), @"
+    insta::assert_snapshot!(work_dir.run_jj(["bisect", "run", "--range=..", "--trust-endpoints", &bisector_path]), @"
     Bisecting: 4 revisions left to test after this (roughly 3 steps)
     Now evaluating: zsuskuln 123b4d91 b | b
     fake-bisector testing commit 123b4d91f6e5e39bfed39bae3bacf9380dc79078