[PATCH v6 00/16] Finish converting git bisect into a built-in
- From
Johannes Schindelin via GitGitGadget <gitgitgadget@gmail.com>
- Date
- Aug 30, 2022, 18:50 UTC
- Message-ID
- <pull.1132.v6.git.1661885419.gitgitgadget@gmail.com>
- In-Reply-To
- <pull.1132.v5.git.1661604264.gitgitgadget@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, 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.shbase-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-- gitgitgadget