[PATCH 2/6] bisect: fix "--" detection when a term name is "--"

[Christian Couder <christian.couder@gmail.com>]

`bisect_start()` walks its arguments twice. The second loop actually
parses the options, and it knows that `--term-good`, `--term-old`,
`--term-bad` and `--term-new` take their value as a separate argument,
so it skips that value.

The first loop, which only looks for the "--" separating revisions from
paths, doesn't know about these options. So when such an option is given
"--" as its value, that "--" is mistaken for the separator and
`has_double_dash` is wrongly set.

This matters because `has_double_dash` makes the second loop die on an
argument that is not a valid revision, instead of treating it as the
first path. So:

  $ git bisect start --term-good -- notarev
  fatal: 'notarev' does not appear to be a valid revision

while the very same command line with any other term name happily takes
"notarev" as a path.

Let's fix this by using early_scan_options(), telling it about the
options taking their value as a separate argument, so that it can skip
those values.

Note: One might argue that accepting a term name that looks like an
option (such as "--") is a misfeature and should be forbidden entirely.
However, whether we should tighten the validation rules for bisect
terms is a separate UI issue that can be dealt with independently. For
now, this commit simply ensures the parser correctly implements the
existing rules.

Signed-off-by: Christian Couder <christian.couder@gmail.com>
---
 builtin/bisect.c            | 27 +++++++++++++++++++++------
 t/t6030-bisect-porcelain.sh |  8 ++++++++
 2 files changed, 29 insertions(+), 6 deletions(-)

diff --git a/builtin/bisect.c b/builtin/bisect.c
index 1cfb8a794b..ad089b289f 100644
--- a/builtin/bisect.c
+++ b/builtin/bisect.c
@@ -803,6 +803,19 @@ static enum bisect_error bisect_auto_next(struct bisect_terms *terms,
 	return bisect_next(terms, prefix);
 }
 
+/*
+ * The options "git bisect start" accepts. Only the ones taking their
+ * value as a separate argument matter to the scan looking for "--" below,
+ * as their value has to be skipped along with them.
+ */
+static const struct early_scan_option bisect_start_early_options[] = {
+	EARLY_SCAN_SKIP_VALUE("term-good"),
+	EARLY_SCAN_SKIP_VALUE("term-old"),
+	EARLY_SCAN_SKIP_VALUE("term-bad"),
+	EARLY_SCAN_SKIP_VALUE("term-new"),
+	EARLY_SCAN_END()
+};
+
 static enum bisect_error bisect_start(struct bisect_terms *terms, int argc,
 				      const char **argv)
 {
@@ -825,13 +838,15 @@ static enum bisect_error bisect_start(struct bisect_terms *terms, int argc,
 
 	/*
 	 * Check for one bad and then some good revisions
+	 *
+	 * The scan below has to know about the options taking their value
+	 * as a separate argument, or such a value that happens to be "--"
+	 * would be mistaken for the "--" separating revisions from paths.
 	 */
-	for (i = 0; i < argc; i++) {
-		if (!strcmp(argv[i], "--")) {
-			has_double_dash = 1;
-			break;
-		}
-	}
+	i = early_scan_options(argc, argv, bisect_start_early_options,
+			       EARLY_SCAN_STOP_AT_DASHDASH, NULL, NULL);
+	if (i < argc)
+		has_double_dash = 1;
 
 	for (i = 0; i < argc; i++) {
 		const char *arg = argv[i];
diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh
index a7588222a8..464ca53b42 100755
--- a/t/t6030-bisect-porcelain.sh
+++ b/t/t6030-bisect-porcelain.sh
@@ -1297,6 +1297,14 @@ test_expect_success 'bisect start takes options and revs in any order' '
 	test_cmp expected actual
 '
 
+test_expect_success 'bisect start with "--" as a term name' '
+	git bisect reset &&
+	git bisect start --term-good -- hello &&
+	git bisect terms --term-good >actual &&
+	echo -- >expected &&
+	test_cmp expected actual
+'
+
 # Bisect is started with --term-new and --term-old arguments,
 # then skip. The HEAD should be changed.
 test_expect_success 'bisect skip works with --term*' '
-- 
2.55.0.787.g3f9e2241eb.dirty