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

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

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...

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/