{"thread":{"id":"55448","subject":"[PATCH v2 0/4] Finish converting git bisect to C part 4","startedAt":"2021-04-07T17:34:31Z","lastAt":"2021-04-11T10:04:40Z","messageCount":11,"participants":["Miriam Rubio","Junio C Hamano","Đoàn Trần Công Danh","Miriam R."],"isPatch":true,"patchVersion":2,"patchTotal":4},"messages":[{"id":"421127","messageId":"20210407173334.68222-1-mirucam@gmail.com","threadId":"55448","inReplyTo":null,"subject":"[PATCH v2 0/4] Finish converting git bisect to C part 4","fromName":"Miriam Rubio","fromEmail":"mirucam@gmail.com","sentAt":"2021-04-07T17:33:29Z","receivedAt":"2021-04-07T17:34:31Z","isPatch":true,"sender":{"key":"mirucam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56339109?v=4"},"body":"These patches correspond to a fourth part of patch series \nof Outreachy project \"Finish converting `git bisect` from shell to C\" \nstarted by Pranit Bauva and Tanushree Tumane\n(https://public-inbox.org/git/pull.117.git.gitgitgadget@gmail.com) and\ncontinued by me.\n\nThis fourth part is formed by reimplementations of some `git bisect` \nsubcommands and removal of some temporary subcommands.\n\nThese patch series emails were generated from:\nhttps://gitlab.com/mirucam/git/commits/git-bisect-work-part4-v2.\n\nGeneral changes\n---------------\n* Rebase on master branch: 2e36527f23 (The sixth batch, 2021-04-02).\n* Change argv_array structs to strvec.\n\nSpecific changes\n----------------\n\n[2/4] bisect--helper: reimplement `bisect_visualize()`shell function in C\n* Add `|| exit` to bisect_visualize call in shell script.\n---\n\n[3/4] bisect--helper: reimplement `bisect_run` shell function in C\n* Add `|| exit` to bisect_run call in shell script.\n---\n\n\nMiriam Rubio (1):\n  bisect--helper: retire `--bisect-next-check` subcommand\n\nPranit Bauva (2):\n  run-command: make `exists_in_PATH()` non-static\n  bisect--helper: reimplement `bisect_visualize()`shell function in C\n\nTanushree Tumane (1):\n  bisect--helper: reimplement `bisect_run` shell function in C\n\n builtin/bisect--helper.c | 124 ++++++++++++++++++++++++++++++++++++---\n git-bisect.sh            |  87 +--------------------------\n run-command.c            |   2 +-\n run-command.h            |  10 ++++\n 4 files changed, 129 insertions(+), 94 deletions(-)\n\n-- \n2.29.2\n\n"},{"id":"421128","messageId":"20210407173334.68222-2-mirucam@gmail.com","threadId":"55448","inReplyTo":"20210407173334.68222-1-mirucam@gmail.com","subject":"[PATCH v2 1/4] run-command: make `exists_in_PATH()` non-static","fromName":"Miriam Rubio","fromEmail":"mirucam@gmail.com","sentAt":"2021-04-07T17:33:30Z","receivedAt":"2021-04-07T17:34:33Z","isPatch":true,"sender":{"key":"mirucam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56339109?v=4"},"body":"From: Pranit Bauva <pranit.bauva@gmail.com>\n\nRemoves the `static` keyword from `exists_in_PATH()` function\nand declares the function in `run-command.h` file.\nThe function will be used in bisect_visualize() in a later\ncommit.\n\n`exists_in_PATH()` and `locate_in_PATH()` functions don't\ndepend on file-local variables.\n\nMentored by: Christian Couder <chriscool@tuxfamily.org>\nMentored by: Johannes Schindelin <Johannes.Schindelin@gmx.de>\nSigned-off-by: Tanushree Tumane <tanushreetumane@gmail.com>\nSigned-off-by: Miriam Rubio <mirucam@gmail.com>\n---\n run-command.c |  2 +-\n run-command.h | 10 ++++++++++\n 2 files changed, 11 insertions(+), 1 deletion(-)\n\ndiff --git a/run-command.c b/run-command.c\nindex be6bc128cd..210b8858f7 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -211,7 +211,7 @@ static char *locate_in_PATH(const char *file)\n \treturn NULL;\n }\n \n-static int exists_in_PATH(const char *file)\n+int exists_in_PATH(const char *file)\n {\n \tchar *r = locate_in_PATH(file);\n \tint found = r != NULL;\ndiff --git a/run-command.h b/run-command.h\nindex d08414a92e..a520ad1342 100644\n--- a/run-command.h\n+++ b/run-command.h\n@@ -179,6 +179,16 @@ void child_process_clear(struct child_process *);\n \n int is_executable(const char *name);\n \n+/**\n+ * Returns if a $PATH given by parameter is found or not (it is NULL). This\n+ * function uses locate_in_PATH() function that emulates the path search that\n+ * execvp would perform. Memory used to store the resultant path is freed by\n+ * the function.\n+ *\n+ * The caller should ensure that $PATH contains no directory separators.\n+ */\n+int exists_in_PATH(const char *file);\n+\n /**\n  * Start a sub-process. Takes a pointer to a `struct child_process`\n  * that specifies the details and returns pipe FDs (if requested).\n-- \n2.29.2\n\n"},{"id":"421129","messageId":"20210407173334.68222-4-mirucam@gmail.com","threadId":"55448","inReplyTo":"20210407173334.68222-1-mirucam@gmail.com","subject":"[PATCH v2 3/4] bisect--helper: reimplement `bisect_run` shell function in C","fromName":"Miriam Rubio","fromEmail":"mirucam@gmail.com","sentAt":"2021-04-07T17:33:32Z","receivedAt":"2021-04-07T17:34:38Z","isPatch":true,"sender":{"key":"mirucam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56339109?v=4"},"body":"From: Tanushree Tumane <tanushreetumane@gmail.com>\n\nReimplement the `bisect_run()` shell function\nin C and also add `--bisect-run` subcommand to\n`git bisect--helper` to call it from git-bisect.sh.\n\nMentored-by: Christian Couder <chriscool@tuxfamily.org>\nSigned-off-by: Tanushree Tumane <tanushreetumane@gmail.com>\nSigned-off-by: Miriam Rubio <mirucam@gmail.com>\n---\n builtin/bisect--helper.c | 71 +++++++++++++++++++++++++++++++++++++++-\n git-bisect.sh            | 62 +----------------------------------\n 2 files changed, 71 insertions(+), 62 deletions(-)\n\ndiff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\nindex b77c4f6b29..31c5f99660 100644\n--- a/builtin/bisect--helper.c\n+++ b/builtin/bisect--helper.c\n@@ -31,6 +31,7 @@ static const char * const git_bisect_helper_usage[] = {\n \tN_(\"git bisect--helper --bisect-replay <filename>\"),\n \tN_(\"git bisect--helper --bisect-skip [(<rev>|<range>)...]\"),\n \tN_(\"git bisect--helper --bisect-visualize\"),\n+\tN_(\"git bisect--helper --bisect-run <cmd>...\"),\n \tNULL\n };\n \n@@ -1073,6 +1074,65 @@ static int bisect_visualize(struct bisect_terms *terms, const char **argv, int a\n \treturn res;\n }\n \n+static int bisect_run(struct bisect_terms *terms, const char **argv, int argc)\n+{\n+\tint res = BISECT_OK;\n+\tstruct strbuf command = STRBUF_INIT;\n+\tstruct strvec args = STRVEC_INIT;\n+\tstruct strvec run_args = STRVEC_INIT;\n+\n+\tif (bisect_next_check(terms, NULL))\n+\t\treturn BISECT_FAILED;\n+\n+\tif (argc)\n+\t\tsq_quote_argv(&command, argv);\n+\telse\n+\t\treturn BISECT_FAILED;\n+\n+\trun_args.v[0] = xstrdup(command.buf);\n+\trun_args.nr = 1;\n+\n+\twhile (1) {\n+\t\tstrvec_clear(&args);\n+\n+\t\tprintf(_(\"running %s\"), command.buf);\n+\t\tres = run_command_v_opt(run_args.v, RUN_USING_SHELL);\n+\n+\t\tif (res < 0 && res >= 128) {\n+\t\t\terror(_(\"bisect run failed: exit code %d from\"\n+\t\t\t\t\" '%s' is < 0 or >= 128\"), res, command.buf);\n+\t\t\tstrbuf_release(&command);\n+\t\t\treturn res;\n+\t\t}\n+\n+\t\tif (res == 125)\n+\t\t\tstrvec_push(&args, \"skip\");\n+\t\telse if (res > 0)\n+\t\t\tstrvec_push(&args, terms->term_bad);\n+\t\telse\n+\t\t\tstrvec_push(&args, terms->term_good);\n+\n+\t\tres = bisect_state(terms, args.v, args.nr);\n+\n+\t\tif (res == BISECT_INTERNAL_SUCCESS_MERGE_BASE) {\n+\t\t\tprintf(_(\"bisect run success\"));\n+\t\t\tres = BISECT_OK;\n+\t\t} else if (res == BISECT_ONLY_SKIPPED_LEFT)\n+\t\t\terror(_(\"bisect run cannot continue any more\"));\n+\t\telse if (res)\n+\t\t\terror(_(\"bisect run failed:'git bisect--helper --bisect-state\"\n+\t\t\t\t\" %s' exited with error code %d\"), args.v[0], res);\n+\t\telse\n+\t\t\tcontinue;\n+\n+\t\tstrbuf_release(&command);\n+\t\tstrvec_clear(&args);\n+\t\tstrvec_clear(&run_args);\n+\n+\t\treturn res;\n+\t}\n+}\n+\n int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n {\n \tenum {\n@@ -1086,7 +1146,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n \t\tBISECT_LOG,\n \t\tBISECT_REPLAY,\n \t\tBISECT_SKIP,\n-\t\tBISECT_VISUALIZE\n+\t\tBISECT_VISUALIZE,\n+\t\tBISECT_RUN,\n \t} cmdmode = 0;\n \tint res = 0, nolog = 0;\n \tstruct option options[] = {\n@@ -1110,6 +1171,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n \t\t\t N_(\"skip some commits for checkout\"), BISECT_SKIP),\n \t\tOPT_CMDMODE(0, \"bisect-visualize\", &cmdmode,\n \t\t\t N_(\"visualize the bisection\"), BISECT_VISUALIZE),\n+\t\tOPT_CMDMODE(0, \"bisect-run\", &cmdmode,\n+\t\t\t N_(\"use <cmd>... to automatically bisect.\"), BISECT_RUN),\n \t\tOPT_BOOL(0, \"no-log\", &nolog,\n \t\t\t N_(\"no log for BISECT_WRITE\")),\n \t\tOPT_END()\n@@ -1174,6 +1237,12 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n \t\tget_terms(&terms);\n \t\tres = bisect_visualize(&terms, argv, argc);\n \t\tbreak;\n+\tcase BISECT_RUN:\n+\t\tif (!argc)\n+\t\t\treturn error(_(\"bisect run failed: no command provided.\"));\n+\t\tget_terms(&terms);\n+\t\tres = bisect_run(&terms, argv, argc);\n+\t\tbreak;\n \tdefault:\n \t\tBUG(\"unknown subcommand %d\", cmdmode);\n \t}\ndiff --git a/git-bisect.sh b/git-bisect.sh\nindex 95f7f3fb8c..e83d011e17 100755\n--- a/git-bisect.sh\n+++ b/git-bisect.sh\n@@ -39,66 +39,6 @@ _x40=\"$_x40$_x40$_x40$_x40$_x40$_x40$_x40$_x40\"\n TERM_BAD=bad\n TERM_GOOD=good\n \n-bisect_run () {\n-\tgit bisect--helper --bisect-next-check $TERM_GOOD $TERM_BAD fail || exit\n-\n-\ttest -n \"$*\" || die \"$(gettext \"bisect run failed: no command provided.\")\"\n-\n-\twhile true\n-\tdo\n-\t\tcommand=\"$@\"\n-\t\teval_gettextln \"running \\$command\"\n-\t\t\"$@\"\n-\t\tres=$?\n-\n-\t\t# Check for really bad run error.\n-\t\tif [ $res -lt 0 -o $res -ge 128 ]\n-\t\tthen\n-\t\t\teval_gettextln \"bisect run failed:\n-exit code \\$res from '\\$command' is < 0 or >= 128\" >&2\n-\t\t\texit $res\n-\t\tfi\n-\n-\t\t# Find current state depending on run success or failure.\n-\t\t# A special exit code of 125 means cannot test.\n-\t\tif [ $res -eq 125 ]\n-\t\tthen\n-\t\t\tstate='skip'\n-\t\telif [ $res -gt 0 ]\n-\t\tthen\n-\t\t\tstate=\"$TERM_BAD\"\n-\t\telse\n-\t\t\tstate=\"$TERM_GOOD\"\n-\t\tfi\n-\n-\t\tgit bisect--helper --bisect-state $state >\"$GIT_DIR/BISECT_RUN\"\n-\t\tres=$?\n-\n-\t\tcat \"$GIT_DIR/BISECT_RUN\"\n-\n-\t\tif sane_grep \"first $TERM_BAD commit could be any of\" \"$GIT_DIR/BISECT_RUN\" \\\n-\t\t\t>/dev/null\n-\t\tthen\n-\t\t\tgettextln \"bisect run cannot continue any more\" >&2\n-\t\t\texit $res\n-\t\tfi\n-\n-\t\tif [ $res -ne 0 ]\n-\t\tthen\n-\t\t\teval_gettextln \"bisect run failed:\n-'bisect-state \\$state' exited with error code \\$res\" >&2\n-\t\t\texit $res\n-\t\tfi\n-\n-\t\tif sane_grep \"is the first $TERM_BAD commit\" \"$GIT_DIR/BISECT_RUN\" >/dev/null\n-\t\tthen\n-\t\t\tgettextln \"bisect run success\"\n-\t\t\texit 0;\n-\t\tfi\n-\n-\tdone\n-}\n-\n get_terms () {\n \tif test -s \"$GIT_DIR/BISECT_TERMS\"\n \tthen\n@@ -137,7 +77,7 @@ case \"$#\" in\n \tlog)\n \t\tgit bisect--helper --bisect-log || exit ;;\n \trun)\n-\t\tbisect_run \"$@\" ;;\n+\t\tgit bisect--helper --bisect-run \"$@\" || exit;;\n \tterms)\n \t\tgit bisect--helper --bisect-terms \"$@\" || exit;;\n \t*)\n-- \n2.29.2\n\n"},{"id":"421130","messageId":"20210407173334.68222-3-mirucam@gmail.com","threadId":"55448","inReplyTo":"20210407173334.68222-1-mirucam@gmail.com","subject":"[PATCH v2 2/4] bisect--helper: reimplement `bisect_visualize()`shell function in C","fromName":"Miriam Rubio","fromEmail":"mirucam@gmail.com","sentAt":"2021-04-07T17:33:31Z","receivedAt":"2021-04-07T17:34:38Z","isPatch":true,"sender":{"key":"mirucam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56339109?v=4"},"body":"From: Pranit Bauva <pranit.bauva@gmail.com>\n\nReimplement the `bisect_visualize()` shell function\nin C and also add `--bisect-visualize` subcommand to\n`git bisect--helper` to call it from git-bisect.sh.\n\nMentored-by: Christian Couder <chriscool@tuxfamily.org>\nMentored-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>\nSigned-off-by: Tanushree Tumane <tanushreetumane@gmail.com>\nSigned-off-by: Miriam Rubio <mirucam@gmail.com>\n---\n builtin/bisect--helper.c | 48 +++++++++++++++++++++++++++++++++++++++-\n git-bisect.sh            | 25 +--------------------\n 2 files changed, 48 insertions(+), 25 deletions(-)\n\ndiff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\nindex 1fdb7d9d10..b77c4f6b29 100644\n--- a/builtin/bisect--helper.c\n+++ b/builtin/bisect--helper.c\n@@ -30,6 +30,7 @@ static const char * const git_bisect_helper_usage[] = {\n \tN_(\"git bisect--helper --bisect-state (good|old) [<rev>...]\"),\n \tN_(\"git bisect--helper --bisect-replay <filename>\"),\n \tN_(\"git bisect--helper --bisect-skip [(<rev>|<range>)...]\"),\n+\tN_(\"git bisect--helper --bisect-visualize\"),\n \tNULL\n };\n \n@@ -1034,6 +1035,44 @@ static enum bisect_error bisect_skip(struct bisect_terms *terms, const char **ar\n \treturn res;\n }\n \n+static int bisect_visualize(struct bisect_terms *terms, const char **argv, int argc)\n+{\n+\tstruct strvec args = STRVEC_INIT;\n+\tint flags = RUN_COMMAND_NO_STDIN, res = 0;\n+\tstruct strbuf sb = STRBUF_INIT;\n+\n+\tif (bisect_next_check(terms, NULL) != 0)\n+\t\treturn BISECT_FAILED;\n+\n+\tif (!argc) {\n+\t\tif ((getenv(\"DISPLAY\") || getenv(\"SESSIONNAME\") || getenv(\"MSYSTEM\") ||\n+\t\t     getenv(\"SECURITYSESSIONID\")) && exists_in_PATH(\"gitk\"))\n+\t\t\tstrvec_push(&args, \"gitk\");\n+\t\telse {\n+\t\t\tstrvec_pushl(&args, \"log\", NULL);\n+\t\t\tflags |= RUN_GIT_CMD;\n+\t\t}\n+\t} else {\n+\t\tif (argv[0][0] == '-') {\n+\t\t\tstrvec_pushl(&args, \"log\", NULL);\n+\t\t\tflags |= RUN_GIT_CMD;\n+\t\t} else if (strcmp(argv[0], \"tig\") && !starts_with(argv[0], \"git\"))\n+\t\t\tflags |= RUN_GIT_CMD;\n+\n+\t\tstrvec_pushv(&args, argv);\n+\t}\n+\n+\tstrvec_pushl(&args, \"--bisect\",  NULL);\n+\n+\tstrbuf_read_file(&sb, git_path_bisect_names(), 0);\n+\tstrvec_split(&args, sb.buf);\n+\tstrbuf_release(&sb);\n+\n+\tres = run_command_v_opt(args.v, flags);\n+\tstrvec_clear(&args);\n+\treturn res;\n+}\n+\n int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n {\n \tenum {\n@@ -1046,7 +1085,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n \t\tBISECT_STATE,\n \t\tBISECT_LOG,\n \t\tBISECT_REPLAY,\n-\t\tBISECT_SKIP\n+\t\tBISECT_SKIP,\n+\t\tBISECT_VISUALIZE\n \t} cmdmode = 0;\n \tint res = 0, nolog = 0;\n \tstruct option options[] = {\n@@ -1068,6 +1108,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n \t\t\t N_(\"replay the bisection process from the given file\"), BISECT_REPLAY),\n \t\tOPT_CMDMODE(0, \"bisect-skip\", &cmdmode,\n \t\t\t N_(\"skip some commits for checkout\"), BISECT_SKIP),\n+\t\tOPT_CMDMODE(0, \"bisect-visualize\", &cmdmode,\n+\t\t\t N_(\"visualize the bisection\"), BISECT_VISUALIZE),\n \t\tOPT_BOOL(0, \"no-log\", &nolog,\n \t\t\t N_(\"no log for BISECT_WRITE\")),\n \t\tOPT_END()\n@@ -1128,6 +1170,10 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n \t\tset_terms(&terms, \"bad\", \"good\");\n \t\tres = bisect_skip(&terms, argv, argc);\n \t\tbreak;\n+\tcase BISECT_VISUALIZE:\n+\t\tget_terms(&terms);\n+\t\tres = bisect_visualize(&terms, argv, argc);\n+\t\tbreak;\n \tdefault:\n \t\tBUG(\"unknown subcommand %d\", cmdmode);\n \t}\ndiff --git a/git-bisect.sh b/git-bisect.sh\nindex 6a7afaea8d..95f7f3fb8c 100755\n--- a/git-bisect.sh\n+++ b/git-bisect.sh\n@@ -39,29 +39,6 @@ _x40=\"$_x40$_x40$_x40$_x40$_x40$_x40$_x40$_x40\"\n TERM_BAD=bad\n TERM_GOOD=good\n \n-bisect_visualize() {\n-\tgit bisect--helper --bisect-next-check $TERM_GOOD $TERM_BAD fail || exit\n-\n-\tif test $# = 0\n-\tthen\n-\t\tif test -n \"${DISPLAY+set}${SESSIONNAME+set}${MSYSTEM+set}${SECURITYSESSIONID+set}\" &&\n-\t\t\ttype gitk >/dev/null 2>&1\n-\t\tthen\n-\t\t\tset gitk\n-\t\telse\n-\t\t\tset git log\n-\t\tfi\n-\telse\n-\t\tcase \"$1\" in\n-\t\tgit*|tig) ;;\n-\t\t-*)\tset git log \"$@\" ;;\n-\t\t*)\tset git \"$@\" ;;\n-\t\tesac\n-\tfi\n-\n-\teval '\"$@\"' --bisect -- $(cat \"$GIT_DIR/BISECT_NAMES\")\n-}\n-\n bisect_run () {\n \tgit bisect--helper --bisect-next-check $TERM_GOOD $TERM_BAD fail || exit\n \n@@ -152,7 +129,7 @@ case \"$#\" in\n \t\t# Not sure we want \"next\" at the UI level anymore.\n \t\tgit bisect--helper --bisect-next \"$@\" || exit ;;\n \tvisualize|view)\n-\t\tbisect_visualize \"$@\" ;;\n+\t\tgit bisect--helper --bisect-visualize \"$@\" || exit;;\n \treset)\n \t\tgit bisect--helper --bisect-reset \"$@\" ;;\n \treplay)\n-- \n2.29.2\n\n"},{"id":"421131","messageId":"20210407173334.68222-5-mirucam@gmail.com","threadId":"55448","inReplyTo":"20210407173334.68222-1-mirucam@gmail.com","subject":"[PATCH v2 4/4] bisect--helper: retire `--bisect-next-check` subcommand","fromName":"Miriam Rubio","fromEmail":"mirucam@gmail.com","sentAt":"2021-04-07T17:33:33Z","receivedAt":"2021-04-07T17:34:38Z","isPatch":true,"sender":{"key":"mirucam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56339109?v=4"},"body":"After reimplementation of `git bisect run` in C,\n`--bisect-next-check` subcommand is not needed anymore.\n\nLet's remove it from options list and code.\n\nMentored by: Christian Couder <chriscool@tuxfamily.org>\nSigned-off-by: Miriam Rubio <mirucam@gmail.com>\n---\n builtin/bisect--helper.c | 7 -------\n 1 file changed, 7 deletions(-)\n\ndiff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\nindex 31c5f99660..c20f43c581 100644\n--- a/builtin/bisect--helper.c\n+++ b/builtin/bisect--helper.c\n@@ -21,7 +21,6 @@ static GIT_PATH_FUNC(git_path_bisect_first_parent, \"BISECT_FIRST_PARENT\")\n \n static const char * const git_bisect_helper_usage[] = {\n \tN_(\"git bisect--helper --bisect-reset [<commit>]\"),\n-\tN_(\"git bisect--helper --bisect-next-check <good_term> <bad_term> [<term>]\"),\n \tN_(\"git bisect--helper --bisect-terms [--term-good | --term-old | --term-bad | --term-new]\"),\n \tN_(\"git bisect--helper --bisect-start [--term-{new,bad}=<term> --term-{old,good}=<term>]\"\n \t\t\t\t\t    \" [--no-checkout] [--first-parent] [<bad> [<good>...]] [--] [<paths>...]\"),\n@@ -1192,12 +1191,6 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n \t\t\treturn error(_(\"--bisect-reset requires either no argument or a commit\"));\n \t\tres = bisect_reset(argc ? argv[0] : NULL);\n \t\tbreak;\n-\tcase BISECT_NEXT_CHECK:\n-\t\tif (argc != 2 && argc != 3)\n-\t\t\treturn error(_(\"--bisect-next-check requires 2 or 3 arguments\"));\n-\t\tset_terms(&terms, argv[1], argv[0]);\n-\t\tres = bisect_next_check(&terms, argc == 3 ? argv[2] : NULL);\n-\t\tbreak;\n \tcase BISECT_TERMS:\n \t\tif (argc > 1)\n \t\t\treturn error(_(\"--bisect-terms requires 0 or 1 argument\"));\n-- \n2.29.2\n\n"},{"id":"421157","messageId":"xmqqo8epydwx.fsf@gitster.g","threadId":"55448","inReplyTo":"20210407173334.68222-3-mirucam@gmail.com","subject":"Re: [PATCH v2 2/4] bisect--helper: reimplement `bisect_visualize()`shell function in C","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-04-07T21:37:18Z","receivedAt":"2021-04-07T21:37:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Miriam Rubio <mirucam@gmail.com> writes:\n\nThis bit\n\n> -\t\tcase \"$1\" in\n> -\t\tgit*|tig) ;;\n> -\t\t-*)\tset git log \"$@\" ;;\n> -\t\t*)\tset git \"$@\" ;;\n> -\t\tesac\n\nin the original corresponds to the following.\n\n> +\t} else {\n> +\t\tif (argv[0][0] == '-') {\n> +\t\t\tstrvec_pushl(&args, \"log\", NULL);\n> +\t\t\tflags |= RUN_GIT_CMD;\n\nIf it begins with \"-\", we assume that is an option to \"git log\".  OK.\n\n> +\t\t} else if (strcmp(argv[0], \"tig\") && !starts_with(argv[0], \"git\"))\n> +\t\t\tflags |= RUN_GIT_CMD;\n\nThe condition (if it is not \"tig\" and does not start with \"git\") is\nthe opposite for the first case arm where we just use the rest of\nthe command line as is.  We take it as a git subcommand name and its\narguments.  OK.\n\n> +\t\tstrvec_pushv(&args, argv);\n\nAnd the remainder of the command line is pushed into the arguments.\n\nOK.\n\n> +\t}\n\nAnd this part from the original\n\n> -\teval '\"$@\"' --bisect -- $(cat \"$GIT_DIR/BISECT_NAMES\")\n\ncorresponds to the rest.  What the original calls \"$@\" is already\ncaptured in args.\n\n> +\tstrvec_pushl(&args, \"--bisect\",  NULL);\n\nWe lost the \"--\" (\"end of options and revs, now the pathspec\nfollows\") here.  Not good.\n\n> +\tstrbuf_read_file(&sb, git_path_bisect_names(), 0);\n> +\tstrvec_split(&args, sb.buf);\n\nThis is probably quite wrong.  What we will place in the\nBISECT_NAMES file, as it is a list of pathspec elements (i.e. a\npathspec), can contain spaces.  For that reason, when we write to\nthe file, we use sq_quote_argv() on each pathspec elements.\n\nSee builtin/bisect--helper.c::bisect_start()\n\nIf I understand correctly, strvec_split() is to split a buffer at\nruns of whitespaces, which is a helper for a very different purpose.\n\nPerhaps sq_dequote_to_strvec() is what we want to use here?\n\n> +\tstrbuf_release(&sb);\n> +\tres = run_command_v_opt(args.v, flags);\n> +\tstrvec_clear(&args);\n> +\treturn res;\n> +}\n> +\n>  int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n>  {\n>  \tenum {\n> @@ -1046,7 +1085,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n>  \t\tBISECT_STATE,\n>  \t\tBISECT_LOG,\n>  \t\tBISECT_REPLAY,\n> -\t\tBISECT_SKIP\n> +\t\tBISECT_SKIP,\n> +\t\tBISECT_VISUALIZE\n\nPerhaps it is a good time to add trailing comma after this new\ntoken.  cf. Documentation/CodingGuidelines (look for \"since early\n2012\").  The next patch to add yet another enum won't have to touch\nthe line added here that way.\n\nEverything below looked trivially correct.\n\nThanks.\n"},{"id":"421161","messageId":"xmqqeeflycf9.fsf@gitster.g","threadId":"55448","inReplyTo":"20210407173334.68222-4-mirucam@gmail.com","subject":"Re: [PATCH v2 3/4] bisect--helper: reimplement `bisect_run` shell function in C","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-04-07T22:09:30Z","receivedAt":"2021-04-07T22:09:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Miriam Rubio <mirucam@gmail.com> writes:\n\n> From: Tanushree Tumane <tanushreetumane@gmail.com>\n>\n> Reimplement the `bisect_run()` shell function\n> in C and also add `--bisect-run` subcommand to\n> `git bisect--helper` to call it from git-bisect.sh.\n>\n> Mentored-by: Christian Couder <chriscool@tuxfamily.org>\n> Signed-off-by: Tanushree Tumane <tanushreetumane@gmail.com>\n> Signed-off-by: Miriam Rubio <mirucam@gmail.com>\n\nIf I am reading the patch correctly, this removes the need for the\n$GIT_DIR/BISECT_RUN file that used to be used to keep track of the\nstate?  If that is true, it is worth noting in the proposed log\nmessage.\n\nAs far as I can see, nobody creates $GIT_DIR/BISECT_RUN anymore.\n\n    $ git grep -e path_bisect_run -e BISECT_RUN\n    bisect.c:static GIT_PATH_FUNC(git_path_bisect_run, \"BISECT_RUN\")\n    bisect.c:\tunlink_or_warn(git_path_bisect_run());\n    builtin/bisect--helper.c:\t\tBISECT_RUN,\n    builtin/bisect--helper.c:\t\t\t N_(\"use <cmd>... to automatical...\n    builtin/bisect--helper.c:\tcase BISECT_RUN:\n    t/t6030-bisect-porcelain.sh:\ttest_path_is_missing \".git/BISECT_RUN\" &&\n\nWhat if a run script tried to read from (or checked the presence of)\nthe file for its correct operation (e.g. I would imagine that \"do\nthis operation when run interactively, but do the same operation\nsilently when run from the git-bisect machinery\" may be a reasonable\nthing to do)?\n\nThis change just unintendedly broke such a script, didn't it?  The\nchange makes me a bit worried.\n\n> +\tif (bisect_next_check(terms, NULL))\n> +\t\treturn BISECT_FAILED;\n> +\n> +\tif (argc)\n> +\t\tsq_quote_argv(&command, argv);\n> +\telse\n> +\t\treturn BISECT_FAILED;\n> +\n> +\trun_args.v[0] = xstrdup(command.buf);\n> +\trun_args.nr = 1;\n> +\n> +\twhile (1) {\n> +\t\tstrvec_clear(&args);\n> +\n> +\t\tprintf(_(\"running %s\"), command.buf);\n> +\t\tres = run_command_v_opt(run_args.v, RUN_USING_SHELL);\n\nNicely used sq_quote_argv() with RUN_USING_SHELL here.  Goodl.\n\n> +\t\tif (res < 0 && res >= 128) {\n> +\t\t\terror(_(\"bisect run failed: exit code %d from\"\n> +\t\t\t\t\" '%s' is < 0 or >= 128\"), res, command.buf);\n> +\t\t\tstrbuf_release(&command);\n> +\t\t\treturn res;\n> +\t\t}\n> +\n> +\t\tif (res == 125)\n> +\t\t\tstrvec_push(&args, \"skip\");\n> +\t\telse if (res > 0)\n> +\t\t\tstrvec_push(&args, terms->term_bad);\n> +\t\telse\n> +\t\t\tstrvec_push(&args, terms->term_good);\n> +\n\nbisect_state() does so much that it was a bit hard to follow for me\n(who hasn't been following the bisect-in-C topic very closely), but\nthe code around here roughly corresponds to the following snippet in\nthe original scripted version.\n\n> -\t\tgit bisect--helper --bisect-state $state >\"$GIT_DIR/BISECT_RUN\"\n> -\t\tres=$?\n> -\n> -\t\tcat \"$GIT_DIR/BISECT_RUN\"\n> -\n> -\t\tif sane_grep \"first $TERM_BAD commit could be any of\" \"$GIT_DIR/BISECT_RUN\" \\\n> -\t\t\t>/dev/null\n> -\t\tthen\n> -\t\t\tgettextln \"bisect run cannot continue any more\" >&2\n> -\t\t\texit $res\n> -\t\tfi\n\nI see that the contents of the file BISECT_RUN is shown to the user\nin the original but is that part of what bisect_state() does, or did\nwe lose it during this round of conversion?\n\n> +\t\tres = bisect_state(terms, args.v, args.nr);\n> +\t\tif (res == BISECT_INTERNAL_SUCCESS_MERGE_BASE) {\n> +\t\t\tprintf(_(\"bisect run success\"));\n> +\t\t\tres = BISECT_OK;\n> +\t\t} else if (res == BISECT_ONLY_SKIPPED_LEFT)\n> +\t\t\terror(_(\"bisect run cannot continue any more\"));\n> +\t\telse if (res)\n> +\t\t\terror(_(\"bisect run failed:'git bisect--helper --bisect-state\"\n> +\t\t\t\t\" %s' exited with error code %d\"), args.v[0], res);\n> +\t\telse\n> +\t\t\tcontinue;\n\nIn any case, being able to check the return value from bisect_state()\nand switch is so much nicer than having to sane_grep in BISECT_RUN.\n\n> +\t\tstrbuf_release(&command);\n> +\t\tstrvec_clear(&args);\n> +\t\tstrvec_clear(&run_args);\n> +\n> +\t\treturn res;\n> +\t}\n> +}\n> +\n>  int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n>  {\n>  \tenum {\n> @@ -1086,7 +1146,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n>  \t\tBISECT_LOG,\n>  \t\tBISECT_REPLAY,\n>  \t\tBISECT_SKIP,\n> -\t\tBISECT_VISUALIZE\n> +\t\tBISECT_VISUALIZE,\n> +\t\tBISECT_RUN,\n\nNow this new one has the trailing comma.  I'd suggest doing so in\nthe previous step.\n\nThanks.\n"},{"id":"421202","messageId":"YG7naB1xepTSoeVk@danh.dev","threadId":"55448","inReplyTo":"20210407173334.68222-2-mirucam@gmail.com","subject":"Re: [PATCH v2 1/4] run-command: make `exists_in_PATH()` non-static","fromName":"Đoàn Trần Công Danh","fromEmail":"congdanhqx@gmail.com","sentAt":"2021-04-08T11:22:39Z","receivedAt":"2021-04-08T11:22:45Z","isPatch":true,"sender":{"key":"congdanhqx@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42673067?v=4"},"body":"On 2021-04-07 19:33:30+0200, Miriam Rubio <mirucam@gmail.com> wrote:\n> From: Pranit Bauva <pranit.bauva@gmail.com>\n> \n> Removes the `static` keyword from `exists_in_PATH()` function\n> and declares the function in `run-command.h` file.\n> The function will be used in bisect_visualize() in a later\n> commit.\n> \n> `exists_in_PATH()` and `locate_in_PATH()` functions don't\n> depend on file-local variables.\n\nIsn't this implementation detail? I think we shouldn't include them in\nthe commit message.\n\n> \n> Mentored by: Christian Couder <chriscool@tuxfamily.org>\n> Mentored by: Johannes Schindelin <Johannes.Schindelin@gmx.de>\n> Signed-off-by: Tanushree Tumane <tanushreetumane@gmail.com>\n> Signed-off-by: Miriam Rubio <mirucam@gmail.com>\n> ---\n>  run-command.c |  2 +-\n>  run-command.h | 10 ++++++++++\n>  2 files changed, 11 insertions(+), 1 deletion(-)\n> \n> diff --git a/run-command.c b/run-command.c\n> index be6bc128cd..210b8858f7 100644\n> --- a/run-command.c\n> +++ b/run-command.c\n> @@ -211,7 +211,7 @@ static char *locate_in_PATH(const char *file)\n>  \treturn NULL;\n>  }\n>  \n> -static int exists_in_PATH(const char *file)\n> +int exists_in_PATH(const char *file)\n>  {\n>  \tchar *r = locate_in_PATH(file);\n>  \tint found = r != NULL;\n> diff --git a/run-command.h b/run-command.h\n> index d08414a92e..a520ad1342 100644\n> --- a/run-command.h\n> +++ b/run-command.h\n> @@ -179,6 +179,16 @@ void child_process_clear(struct child_process *);\n>  \n>  int is_executable(const char *name);\n>  \n> +/**\n> + * Returns if a $PATH given by parameter is found or not (it is NULL). This\n> + * function uses locate_in_PATH() function that emulates the path search that\n> + * execvp would perform. Memory used to store the resultant path is freed by\n> + * the function.\n\nI think this documentation focused too much in implementation detail,\nlocate_in_PATH is still an internal linkage symbol at this stage.\nI think its mention here doesn't improve anything.\n\nFurther more, \"a $PATH given by parameter\" is not what this function\ndoes, the function check if a given \"file\" is found in \"$PATH\" or not.\n\nI would copy 2 first paragraphs of locate_in_PATH's documentation, and\nappend the documentation for return values instead:\n\n-- \nDanh\n"},{"id":"421247","messageId":"xmqq4kggvfq1.fsf@gitster.g","threadId":"55448","inReplyTo":"YG7naB1xepTSoeVk@danh.dev","subject":"Re: [PATCH v2 1/4] run-command: make `exists_in_PATH()` non-static","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-04-08T17:38:46Z","receivedAt":"2021-04-08T17:38:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Đoàn Trần Công Danh  <congdanhqx@gmail.com> writes:\n\n> On 2021-04-07 19:33:30+0200, Miriam Rubio <mirucam@gmail.com> wrote:\n>> From: Pranit Bauva <pranit.bauva@gmail.com>\n>> \n>> Removes the `static` keyword from `exists_in_PATH()` function\n>> and declares the function in `run-command.h` file.\n>> The function will be used in bisect_visualize() in a later\n>> commit.\n>> \n>> `exists_in_PATH()` and `locate_in_PATH()` functions don't\n>> depend on file-local variables.\n>\n> Isn't this implementation detail? I think we shouldn't include them in\n> the commit message.\n\nI also was scratching my head about the statement.  What the\nsentence says is not incorrect per-se, but it was not clear what the\nrelevance is to mention it.  I suspect that it may have wanted to\nsay \"because they do not depend on any file scope statics to keep\nstate or base their computation on, it is safe to expose them as a\ngenerally reusable public helper functions\", and if so, \"that's an\nirrelevant implementation detail\" would not be a valid objection\nagainst mentioning it, but as written in the original, the sentence\nas a mere statement of the fact does not seem to help readers.\n\n>> +/**\n>> + * Returns if a $PATH given by parameter is found or not (it is NULL). This\n>> + * function uses locate_in_PATH() function that emulates the path search that\n>> + * execvp would perform. Memory used to store the resultant path is freed by\n>> + * the function.\n>\n> I think this documentation focused too much in implementation detail,\n> locate_in_PATH is still an internal linkage symbol at this stage.\n> I think its mention here doesn't improve anything.\n\nI totally agree with this.  What it does is more important.\n\nIf you have to describe how it does it, it is often because you need\nto warn callers due to a curious implementation glitch (e.g. \"this\nuses 4-slot rotating internal buffer, so do not expect its return\nvalue to stay stable after many calls\").  In such a case, of course\ndescribing how it does it is important to help callers avoid pitfalls,\nbut for this function, I do not see a need for that.\n"},{"id":"421344","messageId":"xmqq8s5snfv8.fsf@gitster.g","threadId":"55448","inReplyTo":"20210407173334.68222-4-mirucam@gmail.com","subject":"Re: [PATCH v2 3/4] bisect--helper: reimplement `bisect_run` shell function in C","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-04-09T06:15:07Z","receivedAt":"2021-04-09T06:15:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Miriam Rubio <mirucam@gmail.com> writes:\n\n> +static int bisect_run(struct bisect_terms *terms, const char **argv, int argc)\n> +{\n> ...\n> +\twhile (1) {\n> +\t\tstrvec_clear(&args);\n> +\n> +\t\tprintf(_(\"running %s\"), command.buf);\n> +\t\tres = run_command_v_opt(run_args.v, RUN_USING_SHELL);\n> +\n> +\t\tif (res < 0 && res >= 128) {\n\nSorry for not noticing this total nonsense during my earlier review,\nbut this condition will never trigger.\n\nI would have noticed it if the style consistently used \"order\nquantities from left to right, as if on a number line\" convention,\ni.e.\n\n\t\tif (res < 0 && 128 <= res) {\n\nBecause there are only two valid range notation around a single\nvariable, which are:\n\n    (res < 0 || 128 <= res) - meaning 'res' is outside a span, or\n    (0 <= res && res < 128) - meaning 'res' is inside a span.\n\nand the above rewriten form is neither, it would have stood out\nas an error immediately.\n\n\n"},{"id":"421582","messageId":"CAN7CjDD02yjB2QX2VQxfVie6OchkVLfH3Hog8s1jXZw5uDDTzg@mail.gmail.com","threadId":"55448","inReplyTo":"xmqqeeflycf9.fsf@gitster.g","subject":"Re: [PATCH v2 3/4] bisect--helper: reimplement `bisect_run` shell function in C","fromName":"Miriam R.","fromEmail":"mirucam@gmail.com","sentAt":"2021-04-11T10:04:25Z","receivedAt":"2021-04-11T10:04:40Z","isPatch":true,"sender":{"key":"mirucam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56339109?v=4"},"body":"El jue, 8 abr 2021 a las 0:09, Junio C Hamano (<gitster@pobox.com>) escribió:\n>\n> Miriam Rubio <mirucam@gmail.com> writes:\n>\n> > From: Tanushree Tumane <tanushreetumane@gmail.com>\n> >\n> > Reimplement the `bisect_run()` shell function\n> > in C and also add `--bisect-run` subcommand to\n> > `git bisect--helper` to call it from git-bisect.sh.\n> >\n> > Mentored-by: Christian Couder <chriscool@tuxfamily.org>\n> > Signed-off-by: Tanushree Tumane <tanushreetumane@gmail.com>\n> > Signed-off-by: Miriam Rubio <mirucam@gmail.com>\n>\n> If I am reading the patch correctly, this removes the need for the\n> $GIT_DIR/BISECT_RUN file that used to be used to keep track of the\n> state?  If that is true, it is worth noting in the proposed log\n> message.\n>\n> As far as I can see, nobody creates $GIT_DIR/BISECT_RUN anymore.\n>\n>     $ git grep -e path_bisect_run -e BISECT_RUN\n>     bisect.c:static GIT_PATH_FUNC(git_path_bisect_run, \"BISECT_RUN\")\n>     bisect.c:   unlink_or_warn(git_path_bisect_run());\n>     builtin/bisect--helper.c:           BISECT_RUN,\n>     builtin/bisect--helper.c:                    N_(\"use <cmd>... to automatical...\n>     builtin/bisect--helper.c:   case BISECT_RUN:\n>     t/t6030-bisect-porcelain.sh:        test_path_is_missing \".git/BISECT_RUN\" &&\n>\n> What if a run script tried to read from (or checked the presence of)\n> the file for its correct operation (e.g. I would imagine that \"do\n> this operation when run interactively, but do the same operation\n> silently when run from the git-bisect machinery\" may be a reasonable\n> thing to do)?\n>\n> This change just unintendedly broke such a script, didn't it?  The\n> change makes me a bit worried.\nHi,\nthank you for reviewing!.\nI don't know why the need for the $GIT_DIR/BISECT_RUN file was\nremoved, so in the last patch series version I have just sent,\nI have added the creation of the file and it contains bisect_state()\noutput as in original shell script version.\nRegards,\nMiriam\n>\n> > +     if (bisect_next_check(terms, NULL))\n> > +             return BISECT_FAILED;\n> > +\n> > +     if (argc)\n> > +             sq_quote_argv(&command, argv);\n> > +     else\n> > +             return BISECT_FAILED;\n> > +\n> > +     run_args.v[0] = xstrdup(command.buf);\n> > +     run_args.nr = 1;\n> > +\n> > +     while (1) {\n> > +             strvec_clear(&args);\n> > +\n> > +             printf(_(\"running %s\"), command.buf);\n> > +             res = run_command_v_opt(run_args.v, RUN_USING_SHELL);\n>\n> Nicely used sq_quote_argv() with RUN_USING_SHELL here.  Goodl.\n>\n> > +             if (res < 0 && res >= 128) {\n> > +                     error(_(\"bisect run failed: exit code %d from\"\n> > +                             \" '%s' is < 0 or >= 128\"), res, command.buf);\n> > +                     strbuf_release(&command);\n> > +                     return res;\n> > +             }\n> > +\n> > +             if (res == 125)\n> > +                     strvec_push(&args, \"skip\");\n> > +             else if (res > 0)\n> > +                     strvec_push(&args, terms->term_bad);\n> > +             else\n> > +                     strvec_push(&args, terms->term_good);\n> > +\n>\n> bisect_state() does so much that it was a bit hard to follow for me\n> (who hasn't been following the bisect-in-C topic very closely), but\n> the code around here roughly corresponds to the following snippet in\n> the original scripted version.\n>\n> > -             git bisect--helper --bisect-state $state >\"$GIT_DIR/BISECT_RUN\"\n> > -             res=$?\n> > -\n> > -             cat \"$GIT_DIR/BISECT_RUN\"\n> > -\n> > -             if sane_grep \"first $TERM_BAD commit could be any of\" \"$GIT_DIR/BISECT_RUN\" \\\n> > -                     >/dev/null\n> > -             then\n> > -                     gettextln \"bisect run cannot continue any more\" >&2\n> > -                     exit $res\n> > -             fi\n>\n> I see that the contents of the file BISECT_RUN is shown to the user\n> in the original but is that part of what bisect_state() does, or did\n> we lose it during this round of conversion?\n>\n> > +             res = bisect_state(terms, args.v, args.nr);\n> > +             if (res == BISECT_INTERNAL_SUCCESS_MERGE_BASE) {\n> > +                     printf(_(\"bisect run success\"));\n> > +                     res = BISECT_OK;\n> > +             } else if (res == BISECT_ONLY_SKIPPED_LEFT)\n> > +                     error(_(\"bisect run cannot continue any more\"));\n> > +             else if (res)\n> > +                     error(_(\"bisect run failed:'git bisect--helper --bisect-state\"\n> > +                             \" %s' exited with error code %d\"), args.v[0], res);\n> > +             else\n> > +                     continue;\n>\n> In any case, being able to check the return value from bisect_state()\n> and switch is so much nicer than having to sane_grep in BISECT_RUN.\n>\n> > +             strbuf_release(&command);\n> > +             strvec_clear(&args);\n> > +             strvec_clear(&run_args);\n> > +\n> > +             return res;\n> > +     }\n> > +}\n> > +\n> >  int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n> >  {\n> >       enum {\n> > @@ -1086,7 +1146,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n> >               BISECT_LOG,\n> >               BISECT_REPLAY,\n> >               BISECT_SKIP,\n> > -             BISECT_VISUALIZE\n> > +             BISECT_VISUALIZE,\n> > +             BISECT_RUN,\n>\n> Now this new one has the trailing comma.  I'd suggest doing so in\n> the previous step.\n>\n> Thanks.\n"}]}