| Message ID | 76c585d22bf415e719762fcf5decf4b71efa5bfb.1667561761.git.congdanhqx@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Convert git-bisect--helper to OPT_SUBCOMMAND | expand |
On Fri, Nov 04, 2022 at 06:40:12PM +0700, Đoàn Trần Công Danh wrote: > 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. Right. > 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 "--". I think it won't interfere. A user can't put "--" as a separator now; it gets taken as part of the command. So if we consistently added one in the caller and stripped it away in bisect--helper, that would work correctly (and if the user did have one later in their command, it would be preserved as it is now). That said, I do think your solution is nicer, and is potentially fixing similar problems in other subcommand modes, too. > Let's parse subcommand with OPT_SUBCOMMAND since that API was born for > this specific use-case. The implication here being that OPT_SUBCOMMAND stops parsing as soon as it hits a subcommand, I assume. > builtin/bisect--helper.c | 86 +++++++------------------------------ > git-bisect.sh | 20 ++++----- The patch here looks sensible, but... > +# We want to make sure --log is not 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\n" --log >my_bisect_log.txt && > + grep -e --log my_bisect_log.txt && > + git bisect reset > +' ...since you removed --log in the first commit, I think this would pass even before this patch. You'd need to use another option like --bisect-reset to show the problem. Of course then it is trivial that the patch fixes it, since "--bisect-reset" becomes "bisect-reset" afterwards. So there are no options left to parse at that point. But it would be the best we could do to demonstrate it. -Peff
On Fri, Nov 04 2022, Đoàn Trần Công Danh wrote: > 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. If we go for this approch over my series, let's pretty please... > + OPT_SUBCOMMAND("bisect-reset", &fn, cmd_bisect__reset), > + OPT_SUBCOMMAND("bisect-terms", &fn, cmd_bisect__terms), Not call this "bisect-reset" etc, but just "reset", the whole point of the greater problem here is... > - git bisect--helper --bisect-start "$@" ;; > + git bisect--helper bisect-start "$@" ;; ...to be able to eventually remove this shimmy layer completely, which... > bad|good|new|old|"$TERM_BAD"|"$TERM_GOOD") > - git bisect--helper --bisect-state "$cmd" "$@" ;; > + git bisect--helper bisect-state "$cmd" "$@" ;; ...as you can see in my https://lore.kernel.org/git/patch-12.13-13745e3f18f-20221104T132118Z-avarab@gmail.com/ we still need to handle this special snowflake, but... > skip) > - git bisect--helper --bisect-skip "$@" || exit;; > + git bisect--helper bisect-skip "$@" || exit;; > next) > # Not sure we want "next" at the UI level anymore. > - git bisect--helper --bisect-next "$@" || exit ;; > + git bisect--helper bisect-next "$@" || exit ;; > visualize|view) > - git bisect--helper --bisect-visualize "$@" || exit;; > + git bisect--helper bisect-visualize "$@" || exit;; > reset) > - git bisect--helper --bisect-reset "$@" ;; > + git bisect--helper bisect-reset "$@" ;; > replay) > - git bisect--helper --bisect-replay "$@" || exit;; > + git bisect--helper bisect-replay "$@" || exit;; ...instead of doing all of this, get rid of most of this case statement, and just do: bad|good|...) [...] *) git bisect--helper "$cmd" "$@" ;; > log) > - git bisect--helper --bisect-log || exit ;; > + git bisect--helper bisect-log || exit ;; But note that there are subtle behavior differences in some, e.g. because we do the "|| exit" we'll eat the exit code, and this one also doesn't get parameters, so it should be left out of such a list (see tests in my topic for a regression check for that, we're currently flying blind in that area). > +# We want to make sure --log is not 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\n" --log >my_bisect_log.txt && > + grep -e --log my_bisect_log.txt && > + git bisect reset > +' Check out my 1/13 to see all the cases you missed: https://lore.kernel.org/git/patch-01.13-beb1ea22a27-20221104T132117Z-avarab@gmail.com/ :)
On 2022-11-04 14:46:18+0100, Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote: > > On Fri, Nov 04 2022, Đoàn Trần Công Danh wrote: > > > 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. > > If we go for this approch over my series, let's pretty please... > > > + OPT_SUBCOMMAND("bisect-reset", &fn, cmd_bisect__reset), > > + OPT_SUBCOMMAND("bisect-terms", &fn, cmd_bisect__terms), > > Not call this "bisect-reset" etc, but just "reset", the whole point of > the greater problem here is... Yes, we should strip "bisect-" from those sub-commands. > > > - git bisect--helper --bisect-start "$@" ;; > > + git bisect--helper bisect-start "$@" ;; > > ...to be able to eventually remove this shimmy layer completely, > which... > > > bad|good|new|old|"$TERM_BAD"|"$TERM_GOOD") > > - git bisect--helper --bisect-state "$cmd" "$@" ;; > > + git bisect--helper bisect-state "$cmd" "$@" ;; > > ...as you can see in my > https://lore.kernel.org/git/patch-12.13-13745e3f18f-20221104T132118Z-avarab@gmail.com/ > we still need to handle this special snowflake, but... > > > > skip) > > - git bisect--helper --bisect-skip "$@" || exit;; > > + git bisect--helper bisect-skip "$@" || exit;; > > next) > > # Not sure we want "next" at the UI level anymore. > > - git bisect--helper --bisect-next "$@" || exit ;; > > + git bisect--helper bisect-next "$@" || exit ;; > > visualize|view) > > - git bisect--helper --bisect-visualize "$@" || exit;; > > + git bisect--helper bisect-visualize "$@" || exit;; > > reset) > > - git bisect--helper --bisect-reset "$@" ;; > > + git bisect--helper bisect-reset "$@" ;; > > replay) > > - git bisect--helper --bisect-replay "$@" || exit;; > > + git bisect--helper bisect-replay "$@" || exit;; > > ...instead of doing all of this, get rid of most of this case statement, and just do: > > bad|good|...) > [...] > *) > git bisect--helper "$cmd" "$@" > ;; > > > log) > > - git bisect--helper --bisect-log || exit ;; > > + git bisect--helper bisect-log || exit ;; > > But note that there are subtle behavior differences in some, > e.g. because we do the "|| exit" we'll eat the exit code, and this one No, there're nothing different between "|| exit" and simply run the command, since "|| exit" will exit with old exit status code. I don't think there're any other different. > also doesn't get parameters, so it should be left out of such a list > (see tests in my topic for a regression check for that, we're currently > flying blind in that area). > > > +# We want to make sure --log is not 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\n" --log >my_bisect_log.txt && > > + grep -e --log my_bisect_log.txt && > > + git bisect reset > > +' > > Check out my 1/13 to see all the cases you missed: > https://lore.kernel.org/git/patch-01.13-beb1ea22a27-20221104T132117Z-avarab@gmail.com/ > :)
diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c index 292c6fd1dd..c37cb89a90 100644 --- a/builtin/bisect--helper.c +++ b/builtin/bisect--helper.c @@ -1392,84 +1392,30 @@ 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 <cmd>... to automatically bisect"), BISECT_RUN), + OPT_SUBCOMMAND("bisect-reset", &fn, cmd_bisect__reset), + OPT_SUBCOMMAND("bisect-terms", &fn, cmd_bisect__terms), + OPT_SUBCOMMAND("bisect-start", &fn, cmd_bisect__start), + OPT_SUBCOMMAND("bisect-next", &fn, cmd_bisect__next), + OPT_SUBCOMMAND("bisect-state", &fn, cmd_bisect__state), + OPT_SUBCOMMAND("bisect-log", &fn, cmd_bisect__log), + OPT_SUBCOMMAND("bisect-replay", &fn, cmd_bisect__replay), + OPT_SUBCOMMAND("bisect-skip", &fn, cmd_bisect__skip), + OPT_SUBCOMMAND("bisect-visualize", &fn, cmd_bisect__visualize), + OPT_SUBCOMMAND("bisect-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..07a7257be4 100755 --- a/git-bisect.sh +++ b/git-bisect.sh @@ -58,26 +58,26 @@ case "$#" in help) git bisect -h ;; start) - git bisect--helper --bisect-start "$@" ;; + git bisect--helper bisect-start "$@" ;; bad|good|new|old|"$TERM_BAD"|"$TERM_GOOD") - git bisect--helper --bisect-state "$cmd" "$@" ;; + git bisect--helper bisect-state "$cmd" "$@" ;; skip) - git bisect--helper --bisect-skip "$@" || exit;; + git bisect--helper bisect-skip "$@" || exit;; next) # Not sure we want "next" at the UI level anymore. - git bisect--helper --bisect-next "$@" || exit ;; + git bisect--helper bisect-next "$@" || exit ;; visualize|view) - git bisect--helper --bisect-visualize "$@" || exit;; + git bisect--helper bisect-visualize "$@" || exit;; reset) - git bisect--helper --bisect-reset "$@" ;; + git bisect--helper bisect-reset "$@" ;; replay) - git bisect--helper --bisect-replay "$@" || exit;; + git bisect--helper bisect-replay "$@" || exit;; log) - git bisect--helper --bisect-log || exit ;; + git bisect--helper bisect-log || exit ;; run) - git bisect--helper --bisect-run "$@" || exit;; + git bisect--helper bisect-run "$@" || exit;; terms) - git bisect--helper --bisect-terms "$@" || exit;; + git bisect--helper bisect-terms "$@" || exit;; *) usage ;; esac diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh index 83931d482f..a17027d574 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 --log is not 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\n" --log >my_bisect_log.txt && + grep -e --log 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"' '
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 <ldoktor@redhat.com> Signed-off-by: Đoàn Trần Công Danh <congdanhqx@gmail.com> --- builtin/bisect--helper.c | 86 +++++++------------------------------ git-bisect.sh | 20 ++++----- t/t6030-bisect-porcelain.sh | 10 +++++ 3 files changed, 36 insertions(+), 80 deletions(-)