git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Junio C Hamano <gitster@pobox.com>
Date
Sep 2, 2026, 22:30 UTC
Message-ID
<xmqqse3rffr4.fsf@gitster.g>
In-Reply-To
<20260902161047.476753-3-christian.couder@gmail.com>
Christian Couder <christian.couder@gmail.com> writes:
Show 9 quoted lines
> `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.

It may be theoretically true, but I wonder how much practical value it has to correctly parse "--term-good --" as "Ah, the user wants to mark good revisions as '--' instead of 'good' or 'old'"? Even though "refs/bisect/--" is *not* forbidden, how likely is it for users to do that?

This is not like "git grep -e --" which does have much more pracical value.

Show 69 quoted lines
>  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*' '
Previous: Christian CouderNext: Christian Couder
Message 7 of 21 in “Standardize early option scanning to fix argument parsing bugs”
  1. 0/6 Standardize early option scanning to fix argument parsing bugsChristian Couder, Sep 2, 2026
  2. 1/6 parse-options: add early_scan_options()Christian Couder, Sep 2, 2026
  3. Junio C HamanoSep 2, 2026
  4. Christian CouderSep 23, 2026
  5. Junio C HamanoSep 23, 2026
  6. 2/6 bisect: fix "--" detection when a term name is "--"Christian Couder, Sep 2, 2026
  7. Junio C HamanoSep 2, 2026
  8. Christian CouderSep 23, 2026
  9. Junio C HamanoSep 23, 2026
  10. 3/6 rev-parse: fix "--" detection when it is an option valueChristian Couder, Sep 2, 2026
  11. 4/6 parse-options: add parse_options_takes_argument()Christian Couder, Sep 2, 2026
  12. 5/6 parse-options: build early scan options from a struct option arrayChristian Couder, Sep 2, 2026
  13. 6/6 fast-import: use early_scan_options() for --allow-unsafe-featuresChristian Couder, Sep 2, 2026
  14. Junio C HamanoSep 4, 2026
  15. Junio C HamanoSep 2, 2026
  16. Christian CouderSep 23, 2026
  17. 0/3 Standardize early option scanningChristian Couder, Sep 23, 2026
  18. 1/3 parse-options: add parse_options_takes_argument()Christian Couder, Sep 23, 2026
  19. 2/3 parse-options: add early_scan_options()Christian Couder, Sep 23, 2026
  20. Kaartic SivaraamSep 30, 2026
  21. 3/3 fast-import: use early_scan_options() for --allow-unsafe-featuresChristian Couder, Sep 23, 2026

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.