| Message ID | pull.1132.v6.git.1661885419.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Finish converting git bisect into a built-in | expand |
"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com> writes: > Changes since v5: > > * Fixed the commit message of "bisect run: fix the error message" that had > become stale as of v3 due to 80c2e9657f2 "overtaking" it and doing half > of its job. > * Skipped translation usage strings in "bisect--helper: migrate to > OPT_SUBCOMMAND()" that do not contain translatable parts. > * Minor white-space clean-up in "bisect--helper: migrate to > OPT_SUBCOMMAND()". Other than "deliberately breaking the first steps that tried a nice libification that passed 'terms' pair through the codepath could not be a good idea" thing, we did not see any comments on this round. Do people not care about this topic, are people all silently happy, or what?
After three GSoC/Outreachy students spent an incredible effort on this, it is finally time to put a neat little bow on it, or maybe more like a big bow, maybe even a very large one, seeing as it takes quite a while to tie (half a year at the time of writing)... Changes since v5: * Fixed the commit message of "bisect run: fix the error message" that had become stale as of v3 due to 80c2e9657f2 "overtaking" it and doing half of its job. * Skipped translation usage strings in "bisect--helper: migrate to OPT_SUBCOMMAND()" that do not contain translatable parts. * Minor white-space clean-up in "bisect--helper: migrate to OPT_SUBCOMMAND()". Changes since v4: * rebased onto sg/parse-options-subcommand * migrated to OPT_SUBCOMMAND(). * As a consequence, this patch series is now unfortunately very large. And the range-diff is much less useful than I'd like because of the extensive changes that were de facto made a precondition to moving this patch series further. Junio, I would have liked to keep the scope (and burden for the reviewers) substantially smaller, maybe you can help with the review? 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--helper: retire the --no-log option bisect--helper: really retire --bisect-next-check bisect--helper: really retire `--bisect-autostart` bisect--helper: simplify exit code computation bisect--helper: make `terms` an explicit singleton bisect--helper: make the order consistently `argc, argv` bisect--helper: migrate to OPT_SUBCOMMAND() 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: calling `bisect_state()` without an argument is a bug bisect--helper: make `state` optional 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} | 679 ++++++++++++++----------- git-bisect.sh | 84 --- git.c | 2 +- t/t6030-bisect-porcelain.sh | 21 +- 7 files changed, 406 insertions(+), 388 deletions(-) rename builtin/{bisect--helper.c => bisect.c} (67%) delete mode 100755 git-bisect.sh base-commit: 8f9d80f6c06369b563c76ec46c462e740a1a2cf0 Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1132%2Fdscho%2Fbisect-in-c-v6 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1132/dscho/bisect-in-c-v6 Pull-Request: https://github.com/gitgitgadget/git/pull/1132 Range-diff vs v5: 1: 05262b6a7d1 = 1: 05262b6a7d1 bisect--helper: retire the --no-log option 2: 1e43148864a = 2: 1e43148864a bisect--helper: really retire --bisect-next-check 3: 1a1649d9d0d = 3: 1a1649d9d0d bisect--helper: really retire `--bisect-autostart` 4: 9ab30552c6a = 4: 9ab30552c6a bisect--helper: simplify exit code computation 5: 92b3b116ef8 = 5: 92b3b116ef8 bisect--helper: make `terms` an explicit singleton 6: c9dc0281e38 = 6: c9dc0281e38 bisect--helper: make the order consistently `argc, argv` 7: 5b7a3d58b4f ! 7: e97e187bbec bisect--helper: migrate to OPT_SUBCOMMAND() @@ Commit message `bisect--helper` learns about it in preparation for replacing the `git-bisect.sh` script altogether. + As a consequence, the usage strings are copied over from the scripted + version of the `git bisect` command. To avoid regressing on 959d670d1a4 + (i18n: remove from i18n strings that do not hold translatable parts, + 2022-01-31), we specifically do not enclose usage strings in `N_(...)` + that do not contain any translatable parts. + Note that a couple of `bisect_*()` functions are not converted into `cmd_bisect_*()` functions directly, as they have callers other than the `OPT_SUBCOMMAND()` one (and the original functions did not expect @@ builtin/bisect--helper.c: static GIT_PATH_FUNC(git_path_bisect_names, "BISECT_NA + " [<pathspec>...]"), + N_("git bisect (bad|new) [<rev>]"), + N_("git bisect (good|old) [<rev>...]"), -+ N_("git bisect terms [--term-good | --term-bad]"), ++ "git bisect terms [--term-good | --term-bad]", + N_("git bisect skip [(<rev>|<range>)...]"), -+ N_("git bisect next"), ++ "git bisect next", + N_("git bisect reset [<commit>]"), -+ N_("git bisect (visualize|view)"), ++ "git bisect (visualize|view)", + N_("git bisect replay <logfile>"), -+ N_("git bisect log"), ++ "git bisect log", + N_("git bisect run <cmd>..."), + NULL +}; @@ builtin/bisect--helper.c: static GIT_PATH_FUNC(git_path_bisect_names, "BISECT_NA +}; + +static const char * const bisect_terms_usage[] = { -+ N_("git bisect terms [--term-good | --term-bad]"), ++ "git bisect terms [--term-good | --term-bad]", + NULL +}; + @@ builtin/bisect--helper.c: static GIT_PATH_FUNC(git_path_bisect_names, "BISECT_NA +}; + +static const char * const bisect_next_usage[] = { -+ N_("git bisect next"), ++ "git bisect next", + NULL +}; + @@ builtin/bisect--helper.c: static GIT_PATH_FUNC(git_path_bisect_names, "BISECT_NA +}; + +static const char * const bisect_visualize_usage[] = { -+ N_("git bisect visualize"), ++ "git bisect visualize", + NULL +}; + @@ builtin/bisect--helper.c: static GIT_PATH_FUNC(git_path_bisect_names, "BISECT_NA +}; + +static const char * const bisect_log_usage[] = { -+ N_("git bisect log"), ++ "git bisect log", + NULL +}; + @@ builtin/bisect--helper.c: static int bisect_reset(const char *commit) + OPT_END() + }; + argc = parse_options(argc, argv, prefix, options, -+ bisect_reset_usage, PARSE_OPT_STOP_AT_NON_OPTION); ++ bisect_reset_usage, PARSE_OPT_STOP_AT_NON_OPTION); + + if (argc > 1) + usage_msg_opt(_("requires either no argument or a commit"), @@ builtin/bisect--helper.c: finish: + }; + + argc = parse_options(argc, argv, prefix, options, -+ bisect_terms_usage, PARSE_OPT_STOP_AT_NON_OPTION); ++ bisect_terms_usage, PARSE_OPT_STOP_AT_NON_OPTION); + + if (argc > 1) + usage_msg_opt(_("terms: requires 0 or 1 argument"), @@ builtin/bisect--helper.c: static enum bisect_error bisect_state(int argc, const + get_terms(); + + argc = parse_options(argc, argv, prefix, options, -+ bisect_state_usage, PARSE_OPT_STOP_AT_NON_OPTION); ++ bisect_state_usage, PARSE_OPT_STOP_AT_NON_OPTION); + + if (!argc) + usage_msg_opt(_("need at least one argument"), @@ builtin/bisect--helper.c: static enum bisect_error bisect_state(int argc, const const char* filename = git_path_bisect_log(); + argc = parse_options(argc, argv, prefix, options, -+ bisect_log_usage, PARSE_OPT_STOP_AT_NON_OPTION); ++ bisect_log_usage, PARSE_OPT_STOP_AT_NON_OPTION); + + if (argc > 0) + usage_msg_opt(_("require 0 argument"), bisect_log_usage, @@ builtin/bisect--helper.c: static int process_replay_line(struct strbuf *line) + set_terms("bad", "good"); + argc = parse_options(argc, argv, prefix, options, -+ bisect_reset_usage, PARSE_OPT_STOP_AT_NON_OPTION); ++ bisect_reset_usage, PARSE_OPT_STOP_AT_NON_OPTION); + + if (argc != 1) + usage_msg_opt(_("no logfile given"), bisect_replay_usage, @@ builtin/bisect--helper.c: static enum bisect_error bisect_replay(const char *fil + set_terms("bad", "good"); + get_terms(); + argc = parse_options(argc, argv, prefix, options, -+ bisect_skip_usage, PARSE_OPT_STOP_AT_NON_OPTION); ++ bisect_skip_usage, PARSE_OPT_STOP_AT_NON_OPTION); + strvec_push(&argv_state, "skip"); @@ builtin/bisect--helper.c: static int bisect_run(int argc, const char **argv) + get_terms(); + argc = parse_options(argc, argv, prefix, options, -+ bisect_run_usage, PARSE_OPT_STOP_AT_NON_OPTION); ++ bisect_run_usage, PARSE_OPT_STOP_AT_NON_OPTION); + if (bisect_next_check(NULL)) return BISECT_FAILED; 8: ba537af7066 = 8: 30c87f2e92e bisect: verify that a bogus option won't try to start a bisection 9: 409492ad830 ! 9: 4696652b99c bisect run: fix the error message @@ Commit message However, the error message was supposed to print out whether the state was "good" or "bad", but used a bogus (because non-populated) `args` - variable for it. + variable for it. This was fixed in 80c2e9657f2 (bisect--helper: report + actual bisect_state() argument on error, 2022-01-18), but the error + message still talks about `bisect--helper`, which is an implementation + detail that should not concern end users. + + Fix that, and add a regression test to ensure that the intended form of + the error message. Helped-by: Elijah Newren <newren@gmail.com> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> 10: bc5efc8fbfe = 10: b202a0e386c bisect: avoid double-quoting when printing the failed command 11: 8a0adfe3867 = 11: 3376b450867 bisect--helper: calling `bisect_state()` without an argument is a bug 12: 189d2b3ba46 = 12: e7623508f90 bisect--helper: make `state` optional 13: 32bf74e3050 = 13: 3f052580c95 bisect: move even the command-line parsing to `bisect--helper` 14: a8f08f5e0cb = 14: a83fe3dc3c2 Turn `git bisect` into a full built-in 15: a96489310d3 = 15: f2132b61ff7 bisect: remove Cogito-related code 16: bfa7aa19f03 = 16: 4f93692e071 bisect: no longer try to clean up left-over `.git/head-name` files