Re: [PATCH v4 00/16] Finish converting git bisect into a built-in

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Mon, Jun 27 2022, Johannes Schindelin via GitGitGadget wrote:

> After three GSoC/Outreachy students spent an incredible effort on this, it
> is finally time to put a neat little bow on it.

I probably just missed this in past iterations, but there's no
GSOC/Outreachy students in SOB, or even Helped-by. Was their code not
usable & this is the rewrite?

If that's the case it's not clear what that has to do with the current
state of this series...

> Changes since v3:
>
>  * Rebased because of merge conflicts with ab/plug-leak-in-revisions.
>  * Fixed the bug that git bisect --bisect-terms 1 2 wanted to auto-start a
>    bisection if running with a git executable built at the in-between state
>    at patch "bisect: move even the command-line parsing to bisect--helper".
>    Since this bug was "fixed" in v3 by the very next patch, "bisect: teach
>    the bisect--helper command to show the correct usage strings", v4 avoids
>    introducing this bug simply by letting these two patches trade places.
>    The range-diff admittedly looks quite awful because both patches overlap
>    quite a bit in the lines they modify. The end result is the same, though,
>    the diff between v3's and v4's builtin/bisect.c would be empty if I
>    hadn't been forced to rebase.
>  * Added a test case to ensure that this bug won't be introduced again. This
>    test case is the only actual difference relative to v3 of this patch
>    series.

I think there's still a lot of unnecessary churn in this series related
to migrating away from parse_options(). Your 13/16 still has a side-note
that I noted as not making sense in a past iteration.

On top of "master" I tried this trivial alternative to what you're doing
here in a *lot* more lines:
	
	diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
	index 8a052c7111f..38b23ee5fd4 100644
	--- a/builtin/bisect--helper.c
	+++ b/builtin/bisect--helper.c
	@@ -1321,6 +1321,29 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
	 		OPT_END()
	 	};
	 	struct bisect_terms terms = { .term_good = NULL, .term_bad = NULL };
	+	int i;
	+	struct strvec args = STRVEC_INIT;
	+	
	+	for (i = 0; i < argc; i++) {
	+		const char *arg = argv[i];
	+		int is_cmd = 0;
	+		const struct option *o = options;
	+
	+		for (; o->type != OPTION_END; o++)
	+			if (o->flags & PARSE_OPT_CMDMODE &&
	+			    !strcmp(o->long_name, arg)) {
	+				is_cmd = 1;
	+				break;
	+			}
	+
	+		if (is_cmd)
	+			strvec_pushf(&args, "--%s", arg);
	+		else
	+			strvec_push(&args, argv[i]);
	+
	+	}
	+	argc = args.nr;
	+	argv = args.v;
	 
	 	argc = parse_options(argc, argv, prefix, options,
	 			     git_bisect_helper_usage,
	diff --git a/git-bisect.sh b/git-bisect.sh
	index 405cf76f2a3..0fd63bf34db 100755
	--- a/git-bisect.sh
	+++ b/git-bisect.sh
	@@ -57,27 +57,10 @@ case "$#" in
	 	case "$cmd" in
	 	help)
	 		git bisect -h ;;
	-	start)
	-		git bisect--helper --bisect-start "$@" ;;
	 	bad|good|new|old|"$TERM_BAD"|"$TERM_GOOD")
	 		git bisect--helper --bisect-state "$cmd" "$@" ;;
	-	skip)
	-		git bisect--helper --bisect-skip "$@" || exit;;
	-	next)
	-		# Not sure we want "next" at the UI level anymore.
	-		git bisect--helper --bisect-next "$@" || exit ;;
	-	visualize|view)
	-		git bisect--helper --bisect-visualize "$@" || exit;;
	-	reset)
	-		git bisect--helper --bisect-reset "$@" ;;
	-	replay)
	-		git bisect--helper --bisect-replay "$@" || exit;;
	-	log)
	-		git bisect--helper --bisect-log || exit ;;
	-	run)
	-		git bisect--helper --bisect-run "$@" || exit;;
	-	terms)
	-		git bisect--helper --bisect-terms "$@" || exit;;
	+	start|skip|next|visualize|view|reset|replay|log|run|terms)
	+		git bisect--helper "--bisect-$cmd" "$@" || exit;;
	 	*)
	 		usage ;;
	 	esac

I.e. you're spending a lot of effort on working around the
parse_options() API supporting "--options-like-this" but not
"options-like-this". In this case it's much easier just to "fake it",
isn't it?

Of course near the end of this series we'd need to adjust that, but
doing so seems pretty easy, either just treat "start" as "--start", or
perhaps patch the relevant bits of parse_options(), but just
constructing a new command-line as I'm doing here seemed easier.

Now, there's a subtle bug in the above, I passed "view" as-is, but this
will error out. I don't think your topic introduces any bugs related to
that (that I've seen), but it goes to the "confidence building" part of [1].

I.e. here we're converting a bunch of code that doesn't have tests, and
as seen above v.s. the relevant parts of your series doing so in a way
that's not the least invasive path to that goal.

In particular I think all of 08/16, 09/16 and 13/16 (and maybe 10/16?)
would be easier & more straightforward to do with such one-pass argv
construction. I.e. if we make "start" a "--start" and e.g. "good" a
["--bisect-state", "good"] shouldn't this all just work without
migrating bisect--helper away from parse_options()?

In that way this seems a bit like two steps forward & one step back,
e.g. with the above patch we could drive 'git bisect' completion via the
usual --git-completion-helper facility (with a trivial s/--//)
workaround.

B.t.w. I think your tip is buggy with regards to that, i.e. don't you
need a NO_PARSEOPT flag in git.c when you s/bisect--helper/bisect/ so
that the completion doesn't try to use both --git-completion-helper and
its own current 'bisect function' (but maybe it works by happy accident
now?). In any case if you're set on converting this away from
parse_options() you should add that flag in git.c.

I think as far as this conversion goes we could say "we'll use
parse_options() again later", even though I think doing so is
unfortunate, as I think you're going the long way around to migrate away
from an API we're generally migrating to, when we can use it rather
easily.

I'm mainly concerned with the lack of testing. You have a new 01/16
here, but as noted in[1] and above that's really the tip of the iceberg.

Maybe you're really confident that that was the last bug, even without
testing basic things like "view" working at all, but the CL here doesn't
really make for a convincing (to me) argument.

1. https://lore.kernel.org/git/220523.865ylwzgji.gmgdl@evledraar.gmail.com/