Merge pull request #339 from ehuss/regress-on-defaults

Add default regression terms based on `--regress` option.

Author: ehuss

guide/src/examples/diagnostics.md

@@ -23,12 +23,14 @@ fi
 echo "$OUTPUT"
 # This indicates a regression when the text "non-ASCII" is in the output.
 #
-# If the regression is when the text is *not* in the output, remove the `!` prefix.
+# If the regression is when the text is *not* in the output, remove the `!` prefix
+# (and customize the `--term-old` and `--term-new` CLI options if you want).
 ! echo "$OUTPUT" | grep "non-ASCII"
 ```
 
 Then run something like:
 
 ```sh
-cargo bisect-rustc --start=1.67.0 --end=1.68.0 --script ./test.sh
+cargo bisect-rustc --start=1.67.0 --end=1.68.0 --script ./test.sh \
+    --term-old="No warning" --term-new="Found non-ASCII warning"
 ```

guide/src/examples/doc-change.md

@@ -27,7 +27,8 @@ fi
 And run with:
 
 ```sh
-cargo bisect-rustc --start 1.68.0 --end 1.69.0 -c rust-docs --script ./test.sh
+cargo bisect-rustc --start 1.68.0 --end 1.69.0 -c rust-docs --script ./test.sh \
+    --term-old="Did not find" --term-new="Found"
 ```
 
 > **Note**: This may not work on all targets since `cargo-bisect-rustc` doesn't properly handle rustup manifests, which alias some targets to other targets.

guide/src/examples/rustdoc.md

@@ -27,5 +27,6 @@ grep "some example text" $CARGO_TARGET_DIR/doc/mycrate/fn.foo.html
 This can be used with the `--script` option:
 
 ```sh
-cargo-bisect-rustc --start=2023-01-22 --end=2023-03-18 --script=./test.sh
+cargo-bisect-rustc --start=2023-01-22 --end=2023-03-18 --script=./test.sh \
+    --term-old="Found example text" --term-new="Failed, or did not find text"
 ```

guide/src/tutorial.md

@@ -138,7 +138,7 @@ cargo bisect-rustc --script=./test.sh \
 
 ## Custom bisection messages
 
-[Available from v0.6.9]
+*Available from v0.6.9*
 
 You can add custom messages when bisecting a regression. Taking inspiration from git-bisect, with `term-new` and `term-old` you can set custom messages to indicate if a regression matches the condition set by the bisection.
 
@@ -151,18 +151,8 @@ cargo bisect-rustc \
     --term-new "Yes, this build reproduces the regression, compile error"
 ```
 
-Note that `--term-{old,new}` are aware of the `--regress` parameter. If the bisection is looking for a build to reproduce a regression (i.e. `--regress {error,ice}`), `--term-old` indicates a point in time where the regression does not reproduce and `--term-new` indicates that it does.
+In other words, `--term-old` is displayed for older compilers that **do not** exhibit the regression. `--term-new` is for newer compilers which do exhibit the regression.
 
-On the other hand, if `--regress {non-error,non-ice,success}` you are looking into bisecting when a condition of error stopped being reproducible (e.g. some faulty code does not produce an error anymore). In this case `cargo-bisect` flips the meaning of these two parameters.
+What counts as a "regression" is defined by the [`--regress`](usage.html#regression-check) CLI option. By default, a regression is a compile-error (which is equivalent to `--term-new`). If you flip the definition of a "regression" with `--regress=success`, then a regression is a successful compile (which is *also* equivalent to `--term-new`).
 
-Example:
-```sh
-cargo bisect-rustc \
-    --start=2018-08-14 \
-    --end=2018-10-11 \
-    --regress=success \
-    --term-old "This build does not compile" \
-    --term-new "This build compiles"
-```
-
-See [`--regress`](usage.html#regression-check) for more details.
+There are default terms based on the current `--regress` setting. Customizing the terms is most useful when using [scripting](#testing-with-a-script). For example, in the [Documentation changes](examples/doc-change.md) example, the customized terms can more clearly express the results of the script of whether or not it found what it was looking for in the documentation.

src/least_satisfying.rs

@@ -1,8 +1,6 @@
 use std::collections::BTreeMap;
 use std::fmt;
 
-use crate::RegressOn;
-
 pub fn least_satisfying<T, P>(slice: &[T], mut predicate: P) -> usize
 where
     T: fmt::Display + fmt::Debug,
@@ -175,27 +173,11 @@ pub enum Satisfies {
 }
 
 impl Satisfies {
-    pub fn msg_with_context(&self, regress: &RegressOn, term_old: &str, term_new: &str) -> String {
-        let msg_yes = if regress == &RegressOn::Error || regress == &RegressOn::Ice {
-            // YES: compiles, does not reproduce the regression
-            term_new
-        } else {
-            // NO: compile error, reproduces the regression
-            term_old
-        };
-
-        let msg_no = if regress == &RegressOn::Error || regress == &RegressOn::Ice {
-            // YES: compile error
-            term_old
-        } else {
-            // NO: compiles
-            term_new
-        };
-
+    pub fn msg_with_context<'a>(&self, term_old: &'a str, term_new: &'a str) -> &'a str {
         match self {
-            Self::Yes => msg_yes.to_string(),
-            Self::No => msg_no.to_string(),
-            Self::Unknown => "Unable to figure out if the condition matched".to_string(),
+            Self::Yes => term_new,
+            Self::No => term_old,
+            Self::Unknown => "Unable to figure out if the condition matched",
         }
     }
 }

src/main.rs

@@ -193,15 +193,13 @@ a date (YYYY-MM-DD), git tag name (e.g. 1.58.0) or git commit SHA."
 
     #[arg(
         long,
-        default_value = "Test condition NOT matched",
-        help = "Text shown when a test fails to match the condition requested"
+        help = "Text shown when a test does match the condition requested"
     )]
     term_new: Option<String>,
 
     #[arg(
         long,
-        default_value = "Test condition matched",
-        help = "Text shown when a test does match the condition requested"
+        help = "Text shown when a test fails to match the condition requested"
     )]
     term_old: Option<String>,
 }
@@ -890,8 +888,44 @@ impl Config {
         dl_spec: &DownloadParams,
     ) -> Result<Satisfies, InstallError> {
         let regress = self.args.regress;
-        let term_old = self.args.term_old.clone().unwrap_or_default();
-        let term_new = self.args.term_new.clone().unwrap_or_default();
+        let term_old = self.args.term_old.as_deref().unwrap_or_else(|| {
+            if self.args.script.is_some() {
+                match regress {
+                    RegressOn::Error => "Script returned success",
+                    RegressOn::Success => "Script returned error",
+                    RegressOn::Ice => "Script did not ICE",
+                    RegressOn::NonIce => "Script found ICE",
+                    RegressOn::NonError => "Script returned error (no ICE)",
+                }
+            } else {
+                match regress {
+                    RegressOn::Error => "Successfully compiled",
+                    RegressOn::Success => "Compile error",
+                    RegressOn::Ice => "Did not ICE",
+                    RegressOn::NonIce => "Found ICE",
+                    RegressOn::NonError => "Compile error (no ICE)",
+                }
+            }
+        });
+        let term_new = self.args.term_new.as_deref().unwrap_or_else(|| {
+            if self.args.script.is_some() {
+                match regress {
+                    RegressOn::Error => "Script returned error",
+                    RegressOn::Success => "Script returned success",
+                    RegressOn::Ice => "Script found ICE",
+                    RegressOn::NonIce => "Script did not ICE",
+                    RegressOn::NonError => "Script returned success or ICE",
+                }
+            } else {
+                match regress {
+                    RegressOn::Error => "Compile error",
+                    RegressOn::Success => "Successfully compiled",
+                    RegressOn::Ice => "Found ICE",
+                    RegressOn::NonIce => "Did not ICE",
+                    RegressOn::NonError => "Successfully compiled or ICE",
+                }
+            }
+        });
         match t.install(&self.client, dl_spec) {
             Ok(()) => {
                 let outcome = t.test(self);
@@ -903,7 +937,7 @@ impl Config {
                 eprintln!(
                     "RESULT: {}, ===> {}",
                     t,
-                    r.msg_with_context(&regress, &term_old, &term_new)
+                    r.msg_with_context(term_old, term_new)
                 );
                 remove_toolchain(self, t, dl_spec);
                 eprintln!();

tests/cmd/h.stdout

@@ -28,10 +28,8 @@ Options:
   -t, --timeout <TIMEOUT>       Assume failure after specified number of seconds (for bisecting
                                 hangs)
       --target <TARGET>         Cross-compilation target platform
-      --term-new <TERM_NEW>     Text shown when a test fails to match the condition requested
-                                [default: "Test condition NOT matched"]
-      --term-old <TERM_OLD>     Text shown when a test does match the condition requested [default:
-                                "Test condition matched"]
+      --term-new <TERM_NEW>     Text shown when a test does match the condition requested
+      --term-old <TERM_OLD>     Text shown when a test fails to match the condition requested
       --test-dir <TEST_DIR>     Root directory for tests [default: .]
   -v, --verbose...              
   -V, --version                 Print version

tests/cmd/help.stdout

@@ -92,14 +92,10 @@ Options:
           Cross-compilation target platform
 
       --term-new <TERM_NEW>
-          Text shown when a test fails to match the condition requested
-          
-          [default: "Test condition NOT matched"]
+          Text shown when a test does match the condition requested
 
       --term-old <TERM_OLD>
-          Text shown when a test does match the condition requested
-          
-          [default: "Test condition matched"]
+          Text shown when a test fails to match the condition requested
 
       --test-dir <TEST_DIR>
           Root directory for tests