From 58786d73ba4a70e57aa4f45b1bfb696f2c420575 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C4=90o=C3=A0n=20Tr=E1=BA=A7n=20C=C3=B4ng=20Danh?= Date: Thu, 10 Nov 2022 23:36:20 +0700 Subject: [PATCH 1/3] bisect--helper: remove unused options MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 'git-bisect.sh' used to have a 'bisect_next_check' to check if we have both good/bad, old/new terms set or not. In commit 129a6cf344 (bisect--helper: `bisect_next_check` shell function in C, 2019-01-02), a subcommand for bisect--helper was introduced to port the check to C. Since d1bbbe45df (bisect--helper: reimplement `bisect_run` shell function in C, 2021-09-13), all users of 'bisect_next_check' was re-implemented in C, this subcommand was no longer used but we forgot to remove '--bisect-next-check'. 'git-bisect.sh' also used to have a 'bisect_write' function, whose third positional parameter was a "nolog" flag. This flag was only used when 'bisect_start' invoked 'bisect_write' to write the starting good and bad revisions. Then 0f30233a11 (bisect--helper: `bisect_write` shell function in C, 2019-01-02) ported it to C as a command mode of 'bisect--helper', which (incorrectly) added the '--no-log' option, and convert the only place ('bisect_start') that call 'bisect_write' with 'nolog' to 'git bisect--helper --bisect-write' with 'nolog' instead of '--no-log', since 'bisect--helper' has command modes not subcommands, all other command modes see and handle that option as well. This bogus state didn't last long, however, because in the same patch series 06f5608c14 (bisect--helper: `bisect_start` shell function partially in C, 2019-01-02) the C reimplementation of bisect_start() started calling the bisect_write() C function, this time with the right 'nolog' function parameter. From then on there was no need for the '--no-log' option in 'bisect--helper'. Eventually all bisect subcommands were ported to C as 'bisect--helper' command modes, each calling the bisect_write() C function instead, but when the '--bisect-write' command mode was removed in 68efed8c8a (bisect--helper: retire `--bisect-write` subcommand, 2021-02-03) it forgot to remove that '--no-log' option. '--no-log' option had never been used and it's unused now. Let's remove --bisect-next-check and --no-log from option parsing. Signed-off-by: Đoàn Trần Công Danh Signed-off-by: Taylor Blau --- builtin/bisect--helper.c | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c index 28ef7ec2a4..968357c984 100644 --- a/builtin/bisect--helper.c +++ b/builtin/bisect--helper.c @@ -1281,7 +1281,6 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix) { enum { BISECT_RESET = 1, - BISECT_NEXT_CHECK, BISECT_TERMS, BISECT_START, BISECT_AUTOSTART, @@ -1293,12 +1292,10 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix) BISECT_VISUALIZE, BISECT_RUN, } cmdmode = 0; - int res = 0, nolog = 0; + int res = 0; struct option options[] = { OPT_CMDMODE(0, "bisect-reset", &cmdmode, N_("reset the bisection state"), BISECT_RESET), - OPT_CMDMODE(0, "bisect-next-check", &cmdmode, - N_("check whether bad or good terms exist"), BISECT_NEXT_CHECK), OPT_CMDMODE(0, "bisect-terms", &cmdmode, N_("print out the bisect terms"), BISECT_TERMS), OPT_CMDMODE(0, "bisect-start", &cmdmode, @@ -1317,8 +1314,6 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix) N_("visualize the bisection"), BISECT_VISUALIZE), OPT_CMDMODE(0, "bisect-run", &cmdmode, N_("use ... to automatically bisect"), BISECT_RUN), - OPT_BOOL(0, "no-log", &nolog, - N_("no log for BISECT_WRITE")), OPT_END() }; struct bisect_terms terms = { .term_good = NULL, .term_bad = NULL }; From 464ce0aba8526a287c13f96f8d774d7374b647b3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C4=90o=C3=A0n=20Tr=E1=BA=A7n=20C=C3=B4ng=20Danh?= Date: Thu, 10 Nov 2022 23:36:21 +0700 Subject: [PATCH 2/3] bisect--helper: move all subcommands into their own functions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In a later change, we will use OPT_SUBCOMMAND to parse sub-commands to avoid consuming non-option opts. Since OPT_SUBCOMMAND needs a function pointer to operate, let's move it now. Signed-off-by: Đoàn Trần Công Danh Signed-off-by: Taylor Blau --- builtin/bisect--helper.c | 155 ++++++++++++++++++++++++++++++--------- 1 file changed, 121 insertions(+), 34 deletions(-) diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c index 968357c984..95a5102c3c 100644 --- a/builtin/bisect--helper.c +++ b/builtin/bisect--helper.c @@ -1277,6 +1277,117 @@ static int bisect_run(struct bisect_terms *terms, const char **argv, int argc) return res; } +static int cmd_bisect__reset(int argc, const char **argv, const char *prefix UNUSED) +{ + if (argc > 1) + return error(_("--bisect-reset requires either no argument or a commit")); + return bisect_reset(argc ? argv[0] : NULL); +} + +static int cmd_bisect__terms(int argc, const char **argv, const char *prefix UNUSED) +{ + int res; + struct bisect_terms terms = { 0 }; + + if (argc > 1) + return error(_("--bisect-terms requires 0 or 1 argument")); + res = bisect_terms(&terms, argc == 1 ? argv[0] : NULL); + free_terms(&terms); + return res; +} + +static int cmd_bisect__start(int argc, const char **argv, const char *prefix UNUSED) +{ + int res; + struct bisect_terms terms = { 0 }; + + set_terms(&terms, "bad", "good"); + res = bisect_start(&terms, argv, argc); + free_terms(&terms); + return res; +} + +static int cmd_bisect__next(int argc, const char **argv UNUSED, const char *prefix) +{ + int res; + struct bisect_terms terms = { 0 }; + + if (argc) + return error(_("--bisect-next requires 0 arguments")); + get_terms(&terms); + res = bisect_next(&terms, prefix); + free_terms(&terms); + return res; +} + +static int cmd_bisect__state(int argc, const char **argv, const char *prefix UNUSED) +{ + int res; + struct bisect_terms terms = { 0 }; + + set_terms(&terms, "bad", "good"); + get_terms(&terms); + res = bisect_state(&terms, argv, argc); + free_terms(&terms); + return res; +} + +static int cmd_bisect__log(int argc, const char **argv UNUSED, const char *prefix UNUSED) +{ + if (argc) + return error(_("--bisect-log requires 0 arguments")); + return bisect_log(); +} + +static int cmd_bisect__replay(int argc, const char **argv, const char *prefix UNUSED) +{ + int res; + struct bisect_terms terms = { 0 }; + + if (argc != 1) + return error(_("no logfile given")); + set_terms(&terms, "bad", "good"); + res = bisect_replay(&terms, argv[0]); + free_terms(&terms); + return res; +} + +static int cmd_bisect__skip(int argc, const char **argv, const char *prefix UNUSED) +{ + int res; + struct bisect_terms terms = { 0 }; + + set_terms(&terms, "bad", "good"); + get_terms(&terms); + res = bisect_skip(&terms, argv, argc); + free_terms(&terms); + return res; +} + +static int cmd_bisect__visualize(int argc, const char **argv, const char *prefix UNUSED) +{ + int res; + struct bisect_terms terms = { 0 }; + + get_terms(&terms); + res = bisect_visualize(&terms, argv, argc); + free_terms(&terms); + return res; +} + +static int cmd_bisect__run(int argc, const char **argv, const char *prefix UNUSED) +{ + int res; + struct bisect_terms terms = { 0 }; + + if (!argc) + return error(_("bisect run failed: no command provided.")); + get_terms(&terms); + res = bisect_run(&terms, argv, argc); + free_terms(&terms); + return res; +} + int cmd_bisect__helper(int argc, const char **argv, const char *prefix) { enum { @@ -1316,8 +1427,6 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix) N_("use ... to automatically bisect"), BISECT_RUN), OPT_END() }; - struct bisect_terms terms = { .term_good = NULL, .term_bad = NULL }; - argc = parse_options(argc, argv, prefix, options, git_bisect_helper_usage, PARSE_OPT_KEEP_DASHDASH | PARSE_OPT_KEEP_UNKNOWN_OPT); @@ -1327,60 +1436,38 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix) switch (cmdmode) { case BISECT_RESET: - if (argc > 1) - return error(_("--bisect-reset requires either no argument or a commit")); - res = bisect_reset(argc ? argv[0] : NULL); + res = cmd_bisect__reset(argc, argv, prefix); break; case BISECT_TERMS: - if (argc > 1) - return error(_("--bisect-terms requires 0 or 1 argument")); - res = bisect_terms(&terms, argc == 1 ? argv[0] : NULL); + res = cmd_bisect__terms(argc, argv, prefix); break; case BISECT_START: - set_terms(&terms, "bad", "good"); - res = bisect_start(&terms, argv, argc); + res = cmd_bisect__start(argc, argv, prefix); break; case BISECT_NEXT: - if (argc) - return error(_("--bisect-next requires 0 arguments")); - get_terms(&terms); - res = bisect_next(&terms, prefix); + res = cmd_bisect__next(argc, argv, prefix); break; case BISECT_STATE: - set_terms(&terms, "bad", "good"); - get_terms(&terms); - res = bisect_state(&terms, argv, argc); + res = cmd_bisect__state(argc, argv, prefix); break; case BISECT_LOG: - if (argc) - return error(_("--bisect-log requires 0 arguments")); - res = bisect_log(); + res = cmd_bisect__log(argc, argv, prefix); break; case BISECT_REPLAY: - if (argc != 1) - return error(_("no logfile given")); - set_terms(&terms, "bad", "good"); - res = bisect_replay(&terms, argv[0]); + res = cmd_bisect__replay(argc, argv, prefix); break; case BISECT_SKIP: - set_terms(&terms, "bad", "good"); - get_terms(&terms); - res = bisect_skip(&terms, argv, argc); + res = cmd_bisect__skip(argc, argv, prefix); break; case BISECT_VISUALIZE: - get_terms(&terms); - res = bisect_visualize(&terms, argv, argc); + res = cmd_bisect__visualize(argc, argv, prefix); break; case BISECT_RUN: - if (!argc) - return error(_("bisect run failed: no command provided.")); - get_terms(&terms); - res = bisect_run(&terms, argv, argc); + res = cmd_bisect__run(argc, argv, prefix); break; default: BUG("unknown subcommand %d", cmdmode); } - free_terms(&terms); /* * Handle early success From e9011b6092411ca802907a1236d3137bf09081b6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C4=90o=C3=A0n=20Tr=E1=BA=A7n=20C=C3=B4ng=20Danh?= Date: Thu, 10 Nov 2022 23:36:22 +0700 Subject: [PATCH 3/3] bisect--helper: parse subcommand with OPT_SUBCOMMAND MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit As of it is, we're parsing subcommand with OPT_CMDMODE, which will continue to parse more options even if the command has been found. When we're running "git bisect run" with a command that expecting a "--log" or "--no-log" arguments, or one of those "--bisect-..." arguments, bisect--helper may mistakenly think those options are bisect--helper's option. We may fix those problems by passing "--" when calling from git-bisect.sh, and skip that "--" in bisect--helper. However, it may interfere with user's "--". Let's parse subcommand with OPT_SUBCOMMAND since that API was born for this specific use-case. Reported-by: Lukáš Doktor Signed-off-by: Đoàn Trần Công Danh Signed-off-by: Taylor Blau --- builtin/bisect--helper.c | 87 ++++++++----------------------------- git-bisect.sh | 23 ++-------- t/t6030-bisect-porcelain.sh | 10 +++++ 3 files changed, 30 insertions(+), 90 deletions(-) diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c index 95a5102c3c..3bbe159a8d 100644 --- a/builtin/bisect--helper.c +++ b/builtin/bisect--helper.c @@ -1390,84 +1390,31 @@ static int cmd_bisect__run(int argc, const char **argv, const char *prefix UNUSE int cmd_bisect__helper(int argc, const char **argv, const char *prefix) { - enum { - BISECT_RESET = 1, - BISECT_TERMS, - BISECT_START, - BISECT_AUTOSTART, - BISECT_NEXT, - BISECT_STATE, - BISECT_LOG, - BISECT_REPLAY, - BISECT_SKIP, - BISECT_VISUALIZE, - BISECT_RUN, - } cmdmode = 0; int res = 0; + parse_opt_subcommand_fn *fn = NULL; struct option options[] = { - OPT_CMDMODE(0, "bisect-reset", &cmdmode, - N_("reset the bisection state"), BISECT_RESET), - OPT_CMDMODE(0, "bisect-terms", &cmdmode, - N_("print out the bisect terms"), BISECT_TERMS), - OPT_CMDMODE(0, "bisect-start", &cmdmode, - N_("start the bisect session"), BISECT_START), - OPT_CMDMODE(0, "bisect-next", &cmdmode, - N_("find the next bisection commit"), BISECT_NEXT), - OPT_CMDMODE(0, "bisect-state", &cmdmode, - N_("mark the state of ref (or refs)"), BISECT_STATE), - OPT_CMDMODE(0, "bisect-log", &cmdmode, - N_("list the bisection steps so far"), BISECT_LOG), - OPT_CMDMODE(0, "bisect-replay", &cmdmode, - N_("replay the bisection process from the given file"), BISECT_REPLAY), - OPT_CMDMODE(0, "bisect-skip", &cmdmode, - N_("skip some commits for checkout"), BISECT_SKIP), - OPT_CMDMODE(0, "bisect-visualize", &cmdmode, - N_("visualize the bisection"), BISECT_VISUALIZE), - OPT_CMDMODE(0, "bisect-run", &cmdmode, - N_("use ... to automatically bisect"), BISECT_RUN), + OPT_SUBCOMMAND("reset", &fn, cmd_bisect__reset), + OPT_SUBCOMMAND("terms", &fn, cmd_bisect__terms), + OPT_SUBCOMMAND("start", &fn, cmd_bisect__start), + OPT_SUBCOMMAND("next", &fn, cmd_bisect__next), + OPT_SUBCOMMAND("state", &fn, cmd_bisect__state), + OPT_SUBCOMMAND("log", &fn, cmd_bisect__log), + OPT_SUBCOMMAND("replay", &fn, cmd_bisect__replay), + OPT_SUBCOMMAND("skip", &fn, cmd_bisect__skip), + OPT_SUBCOMMAND("visualize", &fn, cmd_bisect__visualize), + OPT_SUBCOMMAND("view", &fn, cmd_bisect__visualize), + OPT_SUBCOMMAND("run", &fn, cmd_bisect__run), OPT_END() }; argc = parse_options(argc, argv, prefix, options, - git_bisect_helper_usage, - PARSE_OPT_KEEP_DASHDASH | PARSE_OPT_KEEP_UNKNOWN_OPT); + git_bisect_helper_usage, 0); - if (!cmdmode) + if (!fn) usage_with_options(git_bisect_helper_usage, options); + argc--; + argv++; - switch (cmdmode) { - case BISECT_RESET: - res = cmd_bisect__reset(argc, argv, prefix); - break; - case BISECT_TERMS: - res = cmd_bisect__terms(argc, argv, prefix); - break; - case BISECT_START: - res = cmd_bisect__start(argc, argv, prefix); - break; - case BISECT_NEXT: - res = cmd_bisect__next(argc, argv, prefix); - break; - case BISECT_STATE: - res = cmd_bisect__state(argc, argv, prefix); - break; - case BISECT_LOG: - res = cmd_bisect__log(argc, argv, prefix); - break; - case BISECT_REPLAY: - res = cmd_bisect__replay(argc, argv, prefix); - break; - case BISECT_SKIP: - res = cmd_bisect__skip(argc, argv, prefix); - break; - case BISECT_VISUALIZE: - res = cmd_bisect__visualize(argc, argv, prefix); - break; - case BISECT_RUN: - res = cmd_bisect__run(argc, argv, prefix); - break; - default: - BUG("unknown subcommand %d", cmdmode); - } + res = fn(argc, argv, prefix); /* * Handle early success diff --git a/git-bisect.sh b/git-bisect.sh index 405cf76f2a..dfce4b4f44 100755 --- a/git-bisect.sh +++ b/git-bisect.sh @@ -57,28 +57,11 @@ 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;; + git bisect--helper state "$cmd" "$@" ;; log) - git bisect--helper --bisect-log || exit ;; - run) - git bisect--helper --bisect-run "$@" || exit;; - terms) - git bisect--helper --bisect-terms "$@" || exit;; + git bisect--helper log || exit ;; *) - usage ;; + git bisect--helper "$cmd" "$@" ;; esac esac diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh index 83931d482f..6dbbe62eb2 100755 --- a/t/t6030-bisect-porcelain.sh +++ b/t/t6030-bisect-porcelain.sh @@ -266,6 +266,16 @@ test_expect_success '"git bisect run" simple case' ' git bisect reset ' +# We want to make sure no arguments has been eaten +test_expect_success '"git bisect run" simple case' ' + git bisect start && + git bisect good $HASH1 && + git bisect bad $HASH4 && + git bisect run printf "%s %s\n" reset --bisect-skip >my_bisect_log.txt && + grep -e "reset --bisect-skip" my_bisect_log.txt && + git bisect reset +' + # We want to automatically find the commit that # added "Ciao" into hello. test_expect_success '"git bisect run" with more complex "git bisect start"' '