| Message ID | pull.1132.v4.git.1656354677.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Finish converting git bisect into a built-in | expand |
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/
After three GSoC/Outreachy students spent an incredible effort on this, it is finally time to put a neat little bow on it. 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. Changes since v2: * We're now careful to provide identical usage strings upon git bisect -h and git bisect bogus. * When a bogus command is provided, we now error out instead of trying to start a git bisect run. * Rebased onto main to avoid plenty of merge conflicts with rs/bisect-executable-not-found, ac/usage-string-fixups and with cd/bisect-messages-from-pre-flight-states. Changes since v1: * Added a regression test to "bisect run: fix the error message". * Added a patch to address an error message that double-single-quoted the command. * Reworked the logic in "bisect--helper: make --bisect-state optional" to delay showing the usage upon an unknown command, which should make the code a lot less confusing. * Split out the change that moved the BISECT_STATE case to the end of the switch block. * Added a patch that replaces the return error() calls in cmd_bisect_helper() with die() calls, to avoid returning -1 as an exit code. * Dropped the use of parse_options() for the single purpose of handling -h; This is now done explicitly. * Simplified the diff of "bisect: move even the option parsing to bisect--helper" by modifying argc and argv instead of modifying all the function calls using those variables. * In the "Turn git bisect into a full built-in" patch, changed the name of the variable holding the usage to use the builtin_ prefix used in other built-ins, too. * Removed the trailing dot from the commit message of "Turn git bisect into a full built-in". Johannes Schindelin (16): bisect: verify that a bogus option won't try to start a bisection bisect run: fix the error message bisect: avoid double-quoting when printing the failed command bisect--helper: retire the --no-log option bisect--helper: really retire --bisect-next-check bisect--helper: really retire `--bisect-autostart` bisect--helper: using `--bisect-state` without an argument is a bug bisect--helper: align the sub-command order with git-bisect.sh bisect--helper: make `--bisect-state` optional bisect--helper: move the `BISECT_STATE` case to the end bisect--helper: return only correct exit codes in `cmd_*()` bisect: teach the `bisect--helper` command to show the correct usage strings bisect: move even the command-line parsing to `bisect--helper` Turn `git bisect` into a full built-in bisect: remove Cogito-related code bisect: no longer try to clean up left-over `.git/head-name` files Makefile | 3 +- bisect.c | 3 - builtin.h | 2 +- builtin/{bisect--helper.c => bisect.c} | 202 +++++++++++-------------- git-bisect.sh | 84 ---------- git.c | 2 +- t/t6030-bisect-porcelain.sh | 21 ++- 7 files changed, 110 insertions(+), 207 deletions(-) rename builtin/{bisect--helper.c => bisect.c} (89%) delete mode 100755 git-bisect.sh base-commit: 8168d5e9c23ed44ae3d604f392320d66556453c9 Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1132%2Fdscho%2Fbisect-in-c-v4 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1132/dscho/bisect-in-c-v4 Pull-Request: https://github.com/gitgitgadget/git/pull/1132 Range-diff vs v3: -: ----------- > 1: 30ddbd7affc bisect: verify that a bogus option won't try to start a bisection 1: cf6034625dd = 2: 97dd2da8f89 bisect run: fix the error message 2: 955ccd4d8c8 = 3: 5571e0f76ff bisect: avoid double-quoting when printing the failed command 3: abcbc25966c = 4: 5bfaf0334c3 bisect--helper: retire the --no-log option 4: af60ef1b5a4 = 5: e85f236304b bisect--helper: really retire --bisect-next-check 5: 07a92c58f8e = 6: b94b7bb4fd0 bisect--helper: really retire `--bisect-autostart` 6: 04ba0950b85 = 7: aad3c9a0850 bisect--helper: using `--bisect-state` without an argument is a bug 7: 6847af9d485 = 8: 375a46dca9f bisect--helper: align the sub-command order with git-bisect.sh 8: b7bc53b9cb6 = 9: c57f63f6a61 bisect--helper: make `--bisect-state` optional 9: 1919237a819 = 10: 87f53469a72 bisect--helper: move the `BISECT_STATE` case to the end 10: 1236a731903 = 11: ce508583e45 bisect--helper: return only correct exit codes in `cmd_*()` 12: ac472aefb6a ! 12: 5dbe233e4ec bisect: teach the `bisect--helper` command to show the correct usage strings @@ Commit message `bisect` command, we hereby teach it to print the very same usage strings as the scripted `git-bisect.sh` does. - With this patch, the `bisect--helper` command is able to do everything - that the `git-bisect.sh` script could, leaving as last step only to - retire that script at long last, which we will do in the next commit. - - Note: Since we cannot use the `parse-options` API to handle the - subcommands of `git bisect` anyway, we no longer use it even just to - show the usage string anymore, either. - Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> ## builtin/bisect--helper.c ## @@ builtin/bisect--helper.c: static GIT_PATH_FUNC(git_path_bisect_names, "BISECT_NA - N_("git bisect--helper --bisect-skip [(<rev>|<range>)...]"), - "git bisect--helper --bisect-visualize", - N_("git bisect--helper --bisect-run <cmd>..."), -- NULL --}; +static const char *bisect_usage = + N_("git bisect [help|start|bad|good|new|old|terms|skip|next|reset|" + "visualize|view|replay|log|run]"); + -+static const char *bisect_long_usage = ++static const char * const bisect_long_usage[] = { + N_("git bisect [help|start|bad|good|new|old|terms|skip|next|reset|" + "visualize|view|replay|log|run]\n" + "\n" @@ builtin/bisect--helper.c: static GIT_PATH_FUNC(git_path_bisect_names, "BISECT_NA + "git bisect run <cmd>...\n" + "\tuse <cmd>... to automatically bisect.\n" + "\n" -+ "Please use \"git help bisect\" to get the full man page."); ++ "Please use \"git help bisect\" to get the full man page."), + NULL + }; - struct add_bisect_ref_data { - struct rev_info *revs; -@@ builtin/bisect--helper.c: static int bisect_run(struct bisect_terms *terms, const char **argv, int argc) - int cmd_bisect__helper(int argc, const char **argv, const char *prefix) - { - int res = 0; -- struct option options[] = { -- OPT_END() -- }; +@@ builtin/bisect--helper.c: int cmd_bisect__helper(int argc, const char **argv, const char *prefix) struct bisect_terms terms = { .term_good = NULL, .term_bad = NULL }; - const char *command = argc > 1 ? argv[1] : "help"; - if (!strcmp("-h", command) || !strcmp("help", command)) -- usage_with_options(git_bisect_helper_usage, options); -+ usage(bisect_long_usage); + argc = parse_options(argc, argv, prefix, options, +- git_bisect_helper_usage, ++ bisect_long_usage, + PARSE_OPT_KEEP_DASHDASH | PARSE_OPT_KEEP_UNKNOWN); - argc -= 2; - argv += 2; + switch (cmdmode ? cmdmode : BISECT_STATE) { @@ builtin/bisect--helper.c: int cmd_bisect__helper(int argc, const char **argv, const char *prefix) - get_terms(&terms); res = bisect_run(&terms, argv, argc); - } else { -+ if (!file_is_not_empty(git_path_bisect_start()) && -+ !one_of(command, "bad", "good", "new", "old", NULL)) + break; + case BISECT_STATE: ++ if (argc && ++ !file_is_not_empty(git_path_bisect_start()) && ++ !one_of(argv[0], "bad", "good", "new", "old", NULL)) + usage(bisect_usage); set_terms(&terms, "bad", "good"); get_terms(&terms); -- if (check_and_set_terms(&terms, command)) { -- char *msg = xstrfmt(_("unknown command: '%s'"), command); + if (!cmdmode && +- (!argc || check_and_set_terms(&terms, argv[0]))) { +- char *msg = xstrfmt(_("unknown command: '%s'"), argv[0]); - usage_msg_opt(msg, git_bisect_helper_usage, options); - } -+ if (check_and_set_terms(&terms, command)) ++ (!argc || check_and_set_terms(&terms, argv[0]))) + usage(bisect_usage); - /* shift the `command` back in */ - argc++; - argv--; + res = bisect_state(&terms, argv, argc); + break; + default: 11: 4ae78d37d04 ! 13: d56f2a14060 bisect: move even the command-line parsing to `bisect--helper` @@ Commit message bisect: move even the command-line parsing to `bisect--helper` On our journey to a fully built-in `git bisect`, this is the - one of the last steps. + last step. Side note: The `parse-options` API is not at all set up to parse subcommands such as `git bisect start`, `git bisect reset`, etc. @@ Commit message Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> ## builtin/bisect--helper.c ## +@@ builtin/bisect--helper.c: static const char *bisect_usage = + N_("git bisect [help|start|bad|good|new|old|terms|skip|next|reset|" + "visualize|view|replay|log|run]"); + +-static const char * const bisect_long_usage[] = { ++static const char *bisect_long_usage = + N_("git bisect [help|start|bad|good|new|old|terms|skip|next|reset|" + "visualize|view|replay|log|run]\n" + "\n" +@@ builtin/bisect--helper.c: static const char * const bisect_long_usage[] = { + "git bisect run <cmd>...\n" + "\tuse <cmd>... to automatically bisect.\n" + "\n" +- "Please use \"git help bisect\" to get the full man page."), +- NULL +-}; ++ "Please use \"git help bisect\" to get the full man page."); + + struct add_bisect_ref_data { + struct rev_info *revs; @@ builtin/bisect--helper.c: static int bisect_run(struct bisect_terms *terms, const char **argv, int argc) int cmd_bisect__helper(int argc, const char **argv, const char *prefix) @@ builtin/bisect--helper.c: static int bisect_run(struct bisect_terms *terms, cons - BISECT_RUN, - } cmdmode = 0; int res = 0; - struct option options[] = { +- struct option options[] = { - OPT_CMDMODE(0, "bisect-start", &cmdmode, - N_("start the bisect session"), BISECT_START), - OPT_CMDMODE(0, "bisect-state", &cmdmode, @@ builtin/bisect--helper.c: static int bisect_run(struct bisect_terms *terms, cons - N_("list the bisection steps so far"), BISECT_LOG), - OPT_CMDMODE(0, "bisect-run", &cmdmode, - N_("use <cmd>... to automatically bisect"), BISECT_RUN), - OPT_END() - }; +- OPT_END() +- }; struct bisect_terms terms = { .term_good = NULL, .term_bad = NULL }; + const char *command = argc > 1 ? argv[1] : "help"; ++ ++ if (!strcmp("-h", command) || !strcmp("help", command)) ++ usage(bisect_long_usage); - argc = parse_options(argc, argv, prefix, options, -- git_bisect_helper_usage, +- bisect_long_usage, - PARSE_OPT_KEEP_DASHDASH | PARSE_OPT_KEEP_UNKNOWN); -+ if (!strcmp("-h", command) || !strcmp("help", command)) -+ usage_with_options(git_bisect_helper_usage, options); ++ argc -= 2; ++ argv += 2; - switch (cmdmode ? cmdmode : BISECT_STATE) { - case BISECT_START: -+ argc -= 2; -+ argv += 2; -+ + if (!strcmp("start", command)) { set_terms(&terms, "bad", "good"); res = bisect_start(&terms, argv, argc); @@ builtin/bisect--helper.c: static int bisect_run(struct bisect_terms *terms, cons res = bisect_run(&terms, argv, argc); - break; - case BISECT_STATE: +- if (argc && +- !file_is_not_empty(git_path_bisect_start()) && +- !one_of(argv[0], "bad", "good", "new", "old", NULL)) + } else { ++ if (!file_is_not_empty(git_path_bisect_start()) && ++ !one_of(command, "bad", "good", "new", "old", NULL)) + usage(bisect_usage); set_terms(&terms, "bad", "good"); get_terms(&terms); - if (!cmdmode && -- (!argc || check_and_set_terms(&terms, argv[0]))) { -- char *msg = xstrfmt(_("unknown command: '%s'"), argv[0]); -+ if (check_and_set_terms(&terms, command)) { -+ char *msg = xstrfmt(_("unknown command: '%s'"), command); - usage_msg_opt(msg, git_bisect_helper_usage, options); - } +- (!argc || check_and_set_terms(&terms, argv[0]))) ++ if (check_and_set_terms(&terms, command)) + usage(bisect_usage); + /* shift the `command` back in */ + argc++; + argv--; 13: 85f5c256ae3 = 14: 378d6d22737 Turn `git bisect` into a full built-in 14: 289917e96af = 15: 33566b86d77 bisect: remove Cogito-related code 15: 8f8d2ba0fe4 = 16: 334664f23a8 bisect: no longer try to clean up left-over `.git/head-name` files