[v2,0/3] Convert git-bisect--helper to OPT_SUBCOMMAND

Message

Đoàn Trần Công Danh Nov. 5, 2022, 5:03 p.m. UTC

This series aims to fix the problem that bisect--helper incorrectly consumes
"--log" when running:

	git bisect run cmd --log

This also clears a way for turning git-bisect into a built-in in a later day.

The series that convert git-bisect to builtin will be posted later as a reply
to this series

Đoàn Trần Công Danh (3):
  bisect--helper: remove unused options
  bisect--helper: move all subcommands into their own functions
  bisect--helper: parse subcommand with OPT_SUBCOMMAND

 builtin/bisect--helper.c    | 229 ++++++++++++++++++++----------------
 git-bisect.sh               |  23 +---
 t/t6030-bisect-porcelain.sh |  10 ++
 3 files changed, 142 insertions(+), 120 deletions(-)

Range-diff against v1:
1:  abe480d1c9 ! 1:  6b80fd9398 bisect--helper: remove unused options
    @@ Metadata
      ## Commit message ##
         bisect--helper: remove unused options
     
    +    '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 <[e-mail]>
     
      ## builtin/bisect--helper.c ##
2:  94ac8cb999 = 2:  9d8a3cdd7a bisect--helper: move all subcommands into their own functions
3:  ae967cacd3 ! 3:  a62ac72e24 bisect--helper: parse subcommand with OPT_SUBCOMMAND
    @@ builtin/bisect--helper.c: static int cmd_bisect__run(int argc, const char **argv
     -			 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_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,
    @@ builtin/bisect--helper.c: static int cmd_bisect__run(int argc, const char **argv
     
      ## git-bisect.sh ##
     @@ git-bisect.sh: case "$#" in
    + 	case "$cmd" in
      	help)
      		git bisect -h ;;
    - 	start)
    +-	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)
    +-	skip)
     -		git bisect--helper --bisect-skip "$@" || exit;;
    -+		git bisect--helper bisect-skip "$@" || exit;;
    - 	next)
    - 		# Not sure we want "next" at the UI level anymore.
    +-	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)
    +-	visualize|view)
     -		git bisect--helper --bisect-visualize "$@" || exit;;
    -+		git bisect--helper bisect-visualize "$@" || exit;;
    - 	reset)
    +-	reset)
     -		git bisect--helper --bisect-reset "$@" ;;
    -+		git bisect--helper bisect-reset "$@" ;;
    - 	replay)
    +-	replay)
     -		git bisect--helper --bisect-replay "$@" || exit;;
    -+		git bisect--helper bisect-replay "$@" || exit;;
    ++		git bisect--helper state "$cmd" "$@" ;;
      	log)
     -		git bisect--helper --bisect-log || exit ;;
    -+		git bisect--helper bisect-log || exit ;;
    - 	run)
    +-	run)
     -		git bisect--helper --bisect-run "$@" || exit;;
    -+		git bisect--helper bisect-run "$@" || exit;;
    - 	terms)
    +-	terms)
     -		git bisect--helper --bisect-terms "$@" || exit;;
    -+		git bisect--helper bisect-terms "$@" || exit;;
    ++		git bisect--helper log || exit ;;
      	*)
    - 		usage ;;
    +-		usage ;;
    ++		git bisect--helper "$cmd" "$@" ;;
      	esac
    + esac
     
      ## t/t6030-bisect-porcelain.sh ##
     @@ t/t6030-bisect-porcelain.sh: test_expect_success '"git bisect run" simple case' '
      	git bisect reset
      '
      
    -+# We want to make sure --log is not eaten
    ++# 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\n" --log >my_bisect_log.txt &&
    -+	grep -e --log my_bisect_log.txt &&
    ++	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
     +'
     +

Comments

Taylor Blau Nov. 5, 2022, 11:18 p.m. UTC | #1

On Sun, Nov 06, 2022 at 12:03:33AM +0700, Đoàn Trần Công Danh wrote:
>  builtin/bisect--helper.c    | 229 ++++++++++++++++++++----------------
>  git-bisect.sh               |  23 +---
>  t/t6030-bisect-porcelain.sh |  10 ++
>  3 files changed, 142 insertions(+), 120 deletions(-)

Thanks, this version looks good to me. Will queue.

Thanks,
Taylor