{"thread":{"id":"53293","subject":"[PATCH v3 00/12] Finish converting git bisect to C part 2","startedAt":"2020-04-23T07:09:38Z","lastAt":"2020-06-22T12:18:02Z","messageCount":28,"participants":["Miriam Rubio","Junio C Hamano","Miriam R.","Johannes Schindelin","Đoàn Trần Công Danh"],"isPatch":true,"patchVersion":3,"patchTotal":12},"messages":[{"id":"395982","messageId":"20200423070704.39872-1-mirucam@gmail.com","threadId":"53293","inReplyTo":null,"subject":"[PATCH v3 00/12] Finish converting git bisect to C part 2","fromName":"Miriam Rubio","fromEmail":"mirucam@gmail.com","sentAt":"2020-04-23T07:06:52Z","receivedAt":"2020-04-23T07:09:38Z","isPatch":true,"sender":{"key":"mirucam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56339109?v=4"},"body":"These patches correspond to a second 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\nThese patch series emails were generated from:\nhttps://gitlab.com/mirucam/git/commits/git-bisect-work-part2-v3.\n\nI would like to thank Junio Hamano for reviewing this patch series and\nChristian Couder for his help.\n\n--- Changes since v2 Finish converting git bisect to C part 2 patch series ---\n\nGeneral changes\n---------------\n\n* Rebase on master branch: efe3874640 (Sync with v2.26.1, 2020-04-13)\n\nSpecific changes\n----------------\n\n[1/12] bisect--helper: fix `cmd_*()` function switch default return\n\n* Use `BUG()` instead of `return error()` in default switch.\n\n--\n\n[2/12] bisect--helper: use '-res' in 'cmd_bisect__helper' return\n\n* New patch: use '-res' instead of 'abs(res)'.\n\n--\n\n[3/12] bisect--helper: introduce new `write_in_file()` function\n\n* Rename input parameter `filepath` to `path`.\n* Change `error_errno()` to `error()` in mode checking.\n* Change error message when file cannot be opened.\n* Add `fclose()` before error return.\n\n--\n\n[4/12] bisect--helper: reimplement `bisect_autostart` shell function in C\n\n* Reorder patch before `reimplement `bisect_next` and `bisect_auto_next`\nshell functions in C` to use `bisect_autostart()` function in \n`bisect_append_log_quoted()`.\n\n--\n`\n[5/12] bisect--helper: reimplement `bisect_next` and `bisect_auto_next` shell\nfunctions in C\n\n* Amend commit message.\n* Add `clear_commit_marks()` at the end of bisect_next_all() and remove it from \n`bisect_rev_setup()`.\n* Fix if condition.\n* Add `bisect_autostart()` in `bisect_append_log_quoted()`.\n* Check `git_path_bisect_head()` with `file_exist()` instead of \n`is_empty_or_missing_file()`.\n* Fix return with BISECT_INTERNAL_SUCCESS_1ST_BAD_FOUND.\n* Add conversion in `cmd_*()` to BISECT_OK when return is \nBISECT_INTERNAL_SUCCESS_1ST_BAD_FOUND.\n\n* Note to previous reviewer: suggestion to change `bisect_auto_next()` function\nto return void cannot be done because errors from `bisect_next()` are not \nignored.\n\n--\n\n[5/12] bisect--helper: reimplement `bisect_state` & `bisect_head` shell\nfunctions in C\n\n* Check `git_path_bisect_head()` with `file_exist()` instead of \n`is_empty_or_missing_file()`.\n\n--\n\nMiriam Rubio (3):\n bisect--helper: fix `cmd_*()` function switch default return\n bisect--helper: use '-res' in 'cmd_bisect__helper' return\n bisect--helper: introduce new `write_in_file()` function\n\nPranit Bauva (9):\n bisect--helper: reimplement `bisect_autostart` shell function in C\n bisect--helper: reimplement `bisect_next` and `bisect_auto_next` shell\n functions in C\n bisect--helper: finish porting `bisect_start()` to C\n bisect--helper: retire `--bisect-clean-state` subcommand\n bisect--helper: retire `--next-all` subcommand\n bisect--helper: reimplement `bisect_state` & `bisect_head` shell\n functions in C\n bisect--helper: retire `--check-expected-revs` subcommand\n bisect--helper: retire `--write-terms` subcommand\n bisect--helper: retire `--bisect-autostart` subcommand\n\n bisect.c | 8 +\n builtin/bisect--helper.c | 383 +++++++++++++++++++++++++++++++++------\n git-bisect.sh | 145 +--------------\n 3 files changed, 347 insertions(+), 189 deletions(-)\n\n-- \n2.25.0\n\n"},{"id":"395983","messageId":"20200423070704.39872-2-mirucam@gmail.com","threadId":"53293","inReplyTo":"20200423070704.39872-1-mirucam@gmail.com","subject":"[PATCH v3 01/12] bisect--helper: fix `cmd_*()` function switch default return","fromName":"Miriam Rubio","fromEmail":"mirucam@gmail.com","sentAt":"2020-04-23T07:06:53Z","receivedAt":"2020-04-23T07:09:39Z","isPatch":true,"sender":{"key":"mirucam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56339109?v=4"},"body":"In a `cmd_*()` function, return `error()` cannot be used\nbecause that translates to `-1` and `cmd_*()` functions need\nto return exit codes.\n\nLet's fix switch default return.\n\nMentored-by: Christian Couder \nMentored-by: Johannes Schindelin \nSigned-off-by: Miriam Rubio \n---\n builtin/bisect--helper.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\nindex c1c40b516d..a81a2c76ff 100644\n--- a/builtin/bisect--helper.c\n+++ b/builtin/bisect--helper.c\n@@ -711,7 +711,7 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n \t\tres = bisect_start(&terms, no_checkout, argv, argc);\n \t\tbreak;\n \tdefault:\n-\t\treturn error(\"BUG: unknown subcommand '%d'\", cmdmode);\n+\t\tBUG(\"unknown subcommand %d\", (int)cmdmode);\n \t}\n \tfree_terms(&terms);\n \n-- \n2.25.0\n\n"},{"id":"395984","messageId":"20200423070704.39872-4-mirucam@gmail.com","threadId":"53293","inReplyTo":"20200423070704.39872-1-mirucam@gmail.com","subject":"[PATCH v3 03/12] bisect--helper: introduce new `write_in_file()` function","fromName":"Miriam Rubio","fromEmail":"mirucam@gmail.com","sentAt":"2020-04-23T07:06:55Z","receivedAt":"2020-04-23T07:09:41Z","isPatch":true,"sender":{"key":"mirucam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56339109?v=4"},"body":"Let's refactor code adding a new `write_in_file()` function\nthat opens a file for writing a message and closes it.\n\nThis helper will be used in later steps and makes the code\nsimpler and easier to understand.\n\nMentored-by: Christian Couder \nMentored-by: Johannes Schindelin \nSigned-off-by: Miriam Rubio \n---\n builtin/bisect--helper.c | 29 +++++++++++++++++++++++------\n 1 file changed, 23 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\nindex 0fbd924aac..d3b2b33df0 100644\n--- a/builtin/bisect--helper.c\n+++ b/builtin/bisect--helper.c\n@@ -74,6 +74,28 @@ static int one_of(const char *term, ...)\n \treturn res;\n }\n \n+static int write_in_file(const char *path, const char *mode, const char *format,...)\n+{\n+\tFILE *fp = NULL;\n+\tva_list args;\n+\tint res = 0;\n+\n+\tif (!strcmp(mode, \"a\") && !strcmp(mode, \"w\"))\n+\t\treturn error(_(\"wrong writing mode '%s'\"), mode);\n+\tfp = fopen(path, mode);\n+\tif (!fp)\n+\t\treturn error_errno(_(\"cannot open file '%s' in mode '%s'\"), path, mode);\n+\tva_start(args, format);\n+\tres = vfprintf(fp, format, args);\n+\tva_end(args);\n+\tif (!res) {\n+\t\tfclose(fp);\n+\t\treturn error_errno(_(\"could not write to file '%s'\"), path);\n+\t}\n+\n+\treturn fclose(fp);\n+}\n+\n static int check_term_format(const char *term, const char *orig_term)\n {\n \tint res;\n@@ -104,7 +126,6 @@ static int check_term_format(const char *term, const char *orig_term)\n \n static int write_terms(const char *bad, const char *good)\n {\n-\tFILE *fp = NULL;\n \tint res;\n \n \tif (!strcmp(bad, good))\n@@ -113,12 +134,8 @@ static int write_terms(const char *bad, const char *good)\n \tif (check_term_format(bad, \"bad\") || check_term_format(good, \"good\"))\n \t\treturn -1;\n \n-\tfp = fopen(git_path_bisect_terms(), \"w\");\n-\tif (!fp)\n-\t\treturn error_errno(_(\"could not open the file BISECT_TERMS\"));\n+\tres = write_in_file(git_path_bisect_terms(), \"w\", \"%s\\n%s\\n\", bad, good);\n \n-\tres = fprintf(fp, \"%s\\n%s\\n\", bad, good);\n-\tres |= fclose(fp);\n \treturn (res < 0) ? -1 : 0;\n }\n \n-- \n2.25.0\n\n"},{"id":"395985","messageId":"20200423070704.39872-3-mirucam@gmail.com","threadId":"53293","inReplyTo":"20200423070704.39872-1-mirucam@gmail.com","subject":"[PATCH v3 02/12] bisect--helper: use '-res' in 'cmd_bisect__helper' return","fromName":"Miriam Rubio","fromEmail":"mirucam@gmail.com","sentAt":"2020-04-23T07:06:54Z","receivedAt":"2020-04-23T07:09:42Z","isPatch":true,"sender":{"key":"mirucam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56339109?v=4"},"body":"Following 'enum bisect_error' vocabulary, return variable 'res' is\nalways non-positive.\nLet's use '-res' instead of 'abs(res)' to make the code clearer.\n\nMentored-by: Christian Couder \nSigned-off-by: Miriam Rubio \nHelped-by: Junio C Hamano \n---\n builtin/bisect--helper.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\nindex a81a2c76ff..0fbd924aac 100644\n--- a/builtin/bisect--helper.c\n+++ b/builtin/bisect--helper.c\n@@ -722,5 +722,5 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n \tif (res == BISECT_INTERNAL_SUCCESS_MERGE_BASE)\n \t\tres = BISECT_OK;\n \n-\treturn abs(res);\n+\treturn -res;\n }\n-- \n2.25.0\n\n"},{"id":"395986","messageId":"20200423070704.39872-5-mirucam@gmail.com","threadId":"53293","inReplyTo":"20200423070704.39872-1-mirucam@gmail.com","subject":"[PATCH v3 04/12] bisect--helper: reimplement `bisect_autostart` shell function in C","fromName":"Miriam Rubio","fromEmail":"mirucam@gmail.com","sentAt":"2020-04-23T07:06:56Z","receivedAt":"2020-04-23T07:09:44Z","isPatch":true,"sender":{"key":"mirucam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56339109?v=4"},"body":"From: Pranit Bauva \n\nReimplement the `bisect_autostart()` shell function in C and add the\nC implementation from `bisect_next()` which was previously left\nuncovered. Also add a subcommand `--bisect-autostart` to\n`git bisect--helper` be called from `bisect_state()` from\ngit-bisect.sh .\n\nUsing `--bisect-autostart` subcommand is a temporary measure to port\nshell function to C so as to use the existing test suite. As more\nfunctions are ported, this subcommand will be retired and\nbisect_autostart() will be called directly by `bisect_state()`.\n\nMentored-by: Lars Schneider \nMentored-by: Christian Couder \nMentored-by: Johannes Schindelin \nSigned-off-by: Pranit Bauva \nSigned-off-by: Tanushree Tumane \nSigned-off-by: Miriam Rubio \n---\n builtin/bisect--helper.c | 40 +++++++++++++++++++++++++++++++++++++++-\n git-bisect.sh | 25 ++-----------------------\n 2 files changed, 41 insertions(+), 24 deletions(-)\n\ndiff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\nindex d3b2b33df0..9df69800e3 100644\n--- a/builtin/bisect--helper.c\n+++ b/builtin/bisect--helper.c\n@@ -29,6 +29,7 @@ static const char * const git_bisect_helper_usage[] = {\n \tN_(\"git bisect--helper --bisect-terms [--term-good | --term-old | --term-bad | --term-new]\"),\n \tN_(\"git bisect--helper --bisect-start [--term-{old,good}= --term-{new,bad}=]\"\n \t\t\t\t\t \"[--no-checkout] [ [...]] [--] [...]\"),\n+\tN_(\"git bisect--helper --bisect-autostart\"),\n \tNULL\n };\n \n@@ -55,6 +56,8 @@ static void set_terms(struct bisect_terms *terms, const char *bad,\n static const char vocab_bad[] = \"bad|new\";\n static const char vocab_good[] = \"good|old\";\n \n+static int bisect_autostart(struct bisect_terms *terms);\n+\n /*\n * Check whether the string `term` belongs to the set of strings\n * included in the variable arguments.\n@@ -630,6 +633,32 @@ static int bisect_start(struct bisect_terms *terms, int no_checkout,\n \treturn res;\n }\n \n+static int bisect_autostart(struct bisect_terms *terms)\n+{\n+\tconst char *yesno;\n+\n+\tif (!is_empty_or_missing_file(git_path_bisect_start()))\n+\t\treturn 0;\n+\n+\tfprintf(stderr, _(\"You need to start by \\\"git bisect \"\n+\t\t\t \"start\\\"\\n\"));\n+\n+\tif (!isatty(STDIN_FILENO))\n+\t\treturn 1;\n+\n+\t/*\n+\t * TRANSLATORS: Make sure to include [Y] and [n] in your\n+\t * translation. The program will only accept English input\n+\t * at this point.\n+\t */\n+\tyesno = git_prompt(_(\"Do you want me to do it for you \"\n+\t\t\t \"[Y/n]? \"), PROMPT_ECHO);\n+\tif (starts_with(yesno, _(\"n\")) || starts_with(yesno, _(\"N\")))\n+\t\treturn 1;\n+\n+\treturn bisect_start(terms, 0, NULL, 0);\n+}\n+\n int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n {\n \tenum {\n@@ -642,7 +671,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n \t\tCHECK_AND_SET_TERMS,\n \t\tBISECT_NEXT_CHECK,\n \t\tBISECT_TERMS,\n-\t\tBISECT_START\n+\t\tBISECT_START,\n+\t\tBISECT_AUTOSTART,\n \t} cmdmode = 0;\n \tint no_checkout = 0, res = 0, nolog = 0;\n \tstruct option options[] = {\n@@ -666,6 +696,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n \t\t\t N_(\"print out the bisect terms\"), BISECT_TERMS),\n \t\tOPT_CMDMODE(0, \"bisect-start\", &cmdmode,\n \t\t\t N_(\"start the bisect session\"), BISECT_START),\n+\t\tOPT_CMDMODE(0, \"bisect-autostart\", &cmdmode,\n+\t\t\t N_(\"start the bisection if BISECT_START is empty or missing\"), BISECT_AUTOSTART),\n \t\tOPT_BOOL(0, \"no-checkout\", &no_checkout,\n \t\t\t N_(\"update BISECT_HEAD instead of checking out the current commit\")),\n \t\tOPT_BOOL(0, \"no-log\", &nolog,\n@@ -727,6 +759,12 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n \t\tset_terms(&terms, \"bad\", \"good\");\n \t\tres = bisect_start(&terms, no_checkout, argv, argc);\n \t\tbreak;\n+\tcase BISECT_AUTOSTART:\n+\t\tif (argc)\n+\t\t\treturn error(_(\"--bisect-autostart requires 0 arguments\"));\n+\t\tset_terms(&terms, \"bad\", \"good\");\n+\t\tres = bisect_autostart(&terms);\n+\t\tbreak;\n \tdefault:\n \t\tBUG(\"unknown subcommand %d\", (int)cmdmode);\n \t}\ndiff --git a/git-bisect.sh b/git-bisect.sh\nindex efee12b8b1..426d443e7e 100755\n--- a/git-bisect.sh\n+++ b/git-bisect.sh\n@@ -49,27 +49,6 @@ bisect_head()\n \tfi\n }\n \n-bisect_autostart() {\n-\ttest -s \"$GIT_DIR/BISECT_START\" || {\n-\t\tgettextln \"You need to start by \\\"git bisect start\\\"\" >&2\n-\t\tif test -t 0\n-\t\tthen\n-\t\t\t# TRANSLATORS: Make sure to include [Y] and [n] in your\n-\t\t\t# translation. The program will only accept English input\n-\t\t\t# at this point.\n-\t\t\tgettext \"Do you want me to do it for you [Y/n]? \" >&2\n-\t\t\tread yesno\n-\t\t\tcase \"$yesno\" in\n-\t\t\t[Nn]*)\n-\t\t\t\texit ;;\n-\t\t\tesac\n-\t\t\tbisect_start\n-\t\telse\n-\t\t\texit 1\n-\t\tfi\n-\t}\n-}\n-\n bisect_start() {\n \tgit bisect--helper --bisect-start $@ || exit\n \n@@ -108,7 +87,7 @@ bisect_skip() {\n }\n \n bisect_state() {\n-\tbisect_autostart\n+\tgit bisect--helper --bisect-autostart\n \tstate=$1\n \tgit bisect--helper --check-and-set-terms $state $TERM_GOOD $TERM_BAD || exit\n \tget_terms\n@@ -149,7 +128,7 @@ bisect_auto_next() {\n \n bisect_next() {\n \tcase \"$#\" in 0) ;; *) usage ;; esac\n-\tbisect_autostart\n+\tgit bisect--helper --bisect-autostart\n \tgit bisect--helper --bisect-next-check $TERM_GOOD $TERM_BAD $TERM_GOOD|| exit\n \n \t# Perform all bisection computation, display and checkout\n-- \n2.25.0\n\n"},{"id":"395987","messageId":"20200423070704.39872-6-mirucam@gmail.com","threadId":"53293","inReplyTo":"20200423070704.39872-1-mirucam@gmail.com","subject":"[PATCH v3 05/12] bisect--helper: reimplement `bisect_next` and `bisect_auto_next` shell functions in C","fromName":"Miriam Rubio","fromEmail":"mirucam@gmail.com","sentAt":"2020-04-23T07:06:57Z","receivedAt":"2020-04-23T07:09:45Z","isPatch":true,"sender":{"key":"mirucam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56339109?v=4"},"body":"From: Pranit Bauva \n\nReimplement the `bisect_next()` and the `bisect_auto_next()` shell functions\nin C and add the subcommands to `git bisect--helper` to call them from\ngit-bisect.sh .\n\nbisect_auto_next() function returns an enum bisect_error type as whole\n`git bisect` can exit with an error code when bisect_next() does.\n\nUsing `--bisect-next` and `--bisect-auto-next` subcommands is a\ntemporary measure to port shell function to C so as to use the existing\ntest suite. As more functions are ported, `--bisect-auto-next`\nsubcommand will be retired and will be called by some other methods.\n\nMentored-by: Lars Schneider \nMentored-by: Christian Couder \nMentored-by: Johannes Schindelin \nSigned-off-by: Pranit Bauva \nSigned-off-by: Tanushree Tumane \nSigned-off-by: Miriam Rubio \n---\n bisect.c | 8 ++\n builtin/bisect--helper.c | 169 ++++++++++++++++++++++++++++++++++++++-\n git-bisect.sh | 47 +----------\n 3 files changed, 179 insertions(+), 45 deletions(-)\n\ndiff --git a/bisect.c b/bisect.c\nindex 9154f810f7..31cab3f0b0 100644\n--- a/bisect.c\n+++ b/bisect.c\n@@ -980,6 +980,12 @@ void read_bisect_terms(const char **read_bad, const char **read_good)\n * the bisection process finished successfully.\n * In this case the calling function or command should not turn a\n * BISECT_INTERNAL_SUCCESS_1ST_BAD_FOUND return code into an error or a non zero exit code.\n+ *\n+ * Checking BISECT_INTERNAL_SUCCESS_1ST_BAD_FOUND\n+ * in bisect_helper::bisect_next() and only transforming it to 0 at\n+ * the end of bisect_helper::cmd_bisect__helper() helps bypassing\n+ * all the code related to finding a commit to test.\n+ *\n * If no_checkout is non-zero, the bisection process does not\n * checkout the trial commit but instead simply updates BISECT_HEAD.\n */\n@@ -1064,6 +1070,8 @@ enum bisect_error bisect_next_all(struct repository *r, const char *prefix, int\n \t\t \"Bisecting: %d revisions left to test after this %s\\n\",\n \t\t nr), nr, steps_msg);\n \tfree(steps_msg);\n+\t/* Clean up objects used, as they will be reused. */\n+\tclear_commit_marks_all(ALL_REV_FLAGS);\n \n \treturn bisect_checkout(bisect_rev, no_checkout);\n }\ndiff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\nindex 9df69800e3..c6aaa8eb15 100644\n--- a/builtin/bisect--helper.c\n+++ b/builtin/bisect--helper.c\n@@ -8,6 +8,7 @@\n #include \"run-command.h\"\n #include \"prompt.h\"\n #include \"quote.h\"\n+#include \"revision.h\"\n \n static GIT_PATH_FUNC(git_path_bisect_terms, \"BISECT_TERMS\")\n static GIT_PATH_FUNC(git_path_bisect_expected_rev, \"BISECT_EXPECTED_REV\")\n@@ -29,6 +30,8 @@ static const char * const git_bisect_helper_usage[] = {\n \tN_(\"git bisect--helper --bisect-terms [--term-good | --term-old | --term-bad | --term-new]\"),\n \tN_(\"git bisect--helper --bisect-start [--term-{old,good}= --term-{new,bad}=]\"\n \t\t\t\t\t \"[--no-checkout] [ [...]] [--] [...]\"),\n+\tN_(\"git bisect--helper --bisect-next\"),\n+\tN_(\"git bisect--helper --bisect-auto-next\"),\n \tN_(\"git bisect--helper --bisect-autostart\"),\n \tNULL\n };\n@@ -441,6 +444,150 @@ static int bisect_append_log_quoted(const char **argv)\n \treturn res;\n }\n \n+static int register_good_ref(const char *refname,\n+\t\t\t const struct object_id *oid, int flags,\n+\t\t\t void *cb_data)\n+{\n+\tstruct argv_array *rev_argv = cb_data;\n+\n+\targv_array_push(rev_argv, oid_to_hex(oid));\n+\treturn 0;\n+}\n+\n+static void prepare_rev_argv(struct bisect_terms *terms, struct argv_array *rev_argv)\n+{\n+\tchar *term_good = xstrfmt(\"%s-*\", terms->term_good);\n+\n+\targv_array_pushl(rev_argv, \"skipped_commits\", \"refs/bisect/bad\", \"--not\", NULL);\n+\tfor_each_glob_ref_in(register_good_ref, term_good, \"refs/bisect/\", rev_argv);\n+\n+\tfree(term_good);\n+}\n+\n+static int prepare_revs(struct bisect_terms *terms, struct rev_info *revs)\n+{\n+\tint res = 0;\n+\tstruct argv_array rev_argv = ARGV_ARRAY_INIT;\n+\n+\tprepare_rev_argv(terms, &rev_argv);\n+\n+\t/*\n+\t * It is important to reset the flags used by revision walks\n+\t * as the previous call to bisect_next_all() in turn\n+\t * sets up a revision walk.\n+\t */\n+\treset_revision_walk();\n+\tinit_revisions(revs, NULL);\n+\trev_argv.argc = setup_revisions(rev_argv.argc, rev_argv.argv, revs, NULL);\n+\tif (prepare_revision_walk(revs))\n+\t\tres = error(_(\"revision walk setup failed\\n\"));\n+\n+\targv_array_clear(&rev_argv);\n+\treturn res;\n+}\n+\n+static int process_skipped_commits(FILE *fp, struct bisect_terms *terms, struct rev_info *revs)\n+{\n+\tstruct commit *commit;\n+\tstruct pretty_print_context pp = {0};\n+\n+\tif (fprintf(fp, \"# only skipped commits left to test\\n\") < 0)\n+\t\treturn error_errno(_(\"failed to write to '%s'\"), git_path_bisect_log());\n+\n+\twhile ((commit = get_revision(revs)) != NULL) {\n+\t\tstruct strbuf commit_name = STRBUF_INIT;\n+\t\tformat_commit_message(commit, \"%s\",\n+\t\t\t\t &commit_name, &pp);\n+\t\tfprintf(fp, \"# possible first %s commit: [%s] %s\\n\",\n+\t\t\tterms->term_bad, oid_to_hex(&commit->object.oid),\n+\t\t\tcommit_name.buf);\n+\t\tstrbuf_release(&commit_name);\n+\t}\n+\n+\t/*\n+\t * Reset the flags used by revision walks in case\n+\t * there is another revision walk after this one.\n+\t */\n+\treset_revision_walk();\n+\n+\treturn 0;\n+}\n+\n+static int bisect_skipped_commits(struct bisect_terms *terms)\n+{\n+\tint res = 0;\n+\tFILE *fp = NULL;\n+\tstruct rev_info revs;\n+\n+\tfp = fopen(git_path_bisect_log(), \"a\");\n+\tif (!fp)\n+\t\treturn error_errno(_(\"could not open '%s' for appending\"),\n+\t\t\t\t git_path_bisect_log());\n+\n+\tres = prepare_revs(terms, &revs);\n+\n+\tif (!res)\n+\t\tres = process_skipped_commits(fp, terms, &revs);\n+\n+\tfclose(fp);\n+\treturn res;\n+}\n+\n+static int bisect_successful(struct bisect_terms *terms)\n+{\n+\tstruct object_id oid;\n+\tstruct commit *commit;\n+\tstruct pretty_print_context pp = {0};\n+\tstruct strbuf commit_name = STRBUF_INIT;\n+\tchar *bad_ref = xstrfmt(\"refs/bisect/%s\",terms->term_bad);\n+\tint res;\n+\n+\tread_ref(bad_ref, &oid);\n+\tprintf(\"%s\\n\", bad_ref);\n+\tcommit = lookup_commit_reference(the_repository, &oid);\n+\tformat_commit_message(commit, \"%s\", &commit_name, &pp);\n+\n+\tres = write_in_file(git_path_bisect_log(), \"a\", \"# first %s commit: [%s] %s\\n\",\n+\t\t\t terms->term_bad, oid_to_hex(&oid),\n+\t\t\t commit_name.buf);\n+\n+\tstrbuf_release(&commit_name);\n+\tfree(bad_ref);\n+\treturn res;\n+}\n+\n+static enum bisect_error bisect_next(struct bisect_terms *terms, const char *prefix)\n+{\n+\tint no_checkout;\n+\tenum bisect_error res;\n+\n+\tbisect_autostart(terms);\n+\tif (bisect_next_check(terms, terms->term_good))\n+\t\treturn BISECT_FAILED;\n+\n+\tno_checkout = file_exists(git_path_bisect_head());\n+\n+\t/* Perform all bisection computation, display and checkout */\n+\tres = bisect_next_all(the_repository, prefix, no_checkout);\n+\n+\tif (res == BISECT_INTERNAL_SUCCESS_1ST_BAD_FOUND) {\n+\t\tres = bisect_successful(terms);\n+\t\treturn res ? res : BISECT_INTERNAL_SUCCESS_1ST_BAD_FOUND;\n+\t} else if (res == BISECT_ONLY_SKIPPED_LEFT) {\n+\t\tres = bisect_skipped_commits(terms);\n+\t\treturn res ? res : BISECT_ONLY_SKIPPED_LEFT;\n+\t}\n+\treturn res;\n+}\n+\n+static enum bisect_error bisect_auto_next(struct bisect_terms *terms, const char *prefix)\n+{\n+\tif (bisect_next_check(terms, NULL))\n+\t\treturn BISECT_OK;\n+\n+\treturn bisect_next(terms, prefix);\n+}\n+\n static int bisect_start(struct bisect_terms *terms, int no_checkout,\n \t\t\tconst char **argv, int argc)\n {\n@@ -672,7 +819,9 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n \t\tBISECT_NEXT_CHECK,\n \t\tBISECT_TERMS,\n \t\tBISECT_START,\n-\t\tBISECT_AUTOSTART,\n+\t\tBISECT_NEXT,\n+\t\tBISECT_AUTO_NEXT,\n+\t\tBISECT_AUTOSTART\n \t} cmdmode = 0;\n \tint no_checkout = 0, res = 0, nolog = 0;\n \tstruct option options[] = {\n@@ -696,6 +845,10 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n \t\t\t N_(\"print out the bisect terms\"), BISECT_TERMS),\n \t\tOPT_CMDMODE(0, \"bisect-start\", &cmdmode,\n \t\t\t N_(\"start the bisect session\"), BISECT_START),\n+\t\tOPT_CMDMODE(0, \"bisect-next\", &cmdmode,\n+\t\t\t N_(\"find the next bisection commit\"), BISECT_NEXT),\n+\t\tOPT_CMDMODE(0, \"bisect-auto-next\", &cmdmode,\n+\t\t\t N_(\"verify the next bisection state then checkout the next bisection commit\"), BISECT_AUTO_NEXT),\n \t\tOPT_CMDMODE(0, \"bisect-autostart\", &cmdmode,\n \t\t\t N_(\"start the bisection if BISECT_START is empty or missing\"), BISECT_AUTOSTART),\n \t\tOPT_BOOL(0, \"no-checkout\", &no_checkout,\n@@ -759,6 +912,18 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n \t\tset_terms(&terms, \"bad\", \"good\");\n \t\tres = bisect_start(&terms, no_checkout, argv, argc);\n \t\tbreak;\n+\tcase BISECT_NEXT:\n+\t\tif (argc)\n+\t\t\treturn error(_(\"--bisect-next requires 0 arguments\"));\n+\t\tget_terms(&terms);\n+\t\tres = bisect_next(&terms, prefix);\n+\t\tbreak;\n+\tcase BISECT_AUTO_NEXT:\n+\t\tif (argc)\n+\t\t\treturn error(_(\"--bisect-auto-next requires 0 arguments\"));\n+\t\tget_terms(&terms);\n+\t\tres = bisect_auto_next(&terms, prefix);\n+\t\tbreak;\n \tcase BISECT_AUTOSTART:\n \t\tif (argc)\n \t\t\treturn error(_(\"--bisect-autostart requires 0 arguments\"));\n@@ -774,7 +939,7 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n \t * Handle early success\n \t * From check_merge_bases > check_good_are_ancestors_of_bad > bisect_next_all\n \t */\n-\tif (res == BISECT_INTERNAL_SUCCESS_MERGE_BASE)\n+\tif ((res == BISECT_INTERNAL_SUCCESS_MERGE_BASE) || (res == BISECT_INTERNAL_SUCCESS_1ST_BAD_FOUND))\n \t\tres = BISECT_OK;\n \n \treturn -res;\ndiff --git a/git-bisect.sh b/git-bisect.sh\nindex 426d443e7e..897825b675 100755\n--- a/git-bisect.sh\n+++ b/git-bisect.sh\n@@ -65,8 +65,7 @@ bisect_start() {\n \t#\n \t# Check if we can proceed to the next bisect state.\n \t#\n-\tget_terms\n-\tbisect_auto_next\n+\tgit bisect--helper --bisect-auto-next || exit\n \n \ttrap '-' 0\n }\n@@ -119,45 +118,7 @@ bisect_state() {\n \t*)\n \t\tusage ;;\n \tesac\n-\tbisect_auto_next\n-}\n-\n-bisect_auto_next() {\n-\tgit bisect--helper --bisect-next-check $TERM_GOOD $TERM_BAD && bisect_next || :\n-}\n-\n-bisect_next() {\n-\tcase \"$#\" in 0) ;; *) usage ;; esac\n-\tgit bisect--helper --bisect-autostart\n-\tgit bisect--helper --bisect-next-check $TERM_GOOD $TERM_BAD $TERM_GOOD|| exit\n-\n-\t# Perform all bisection computation, display and checkout\n-\tgit bisect--helper --next-all $(test -f \"$GIT_DIR/BISECT_HEAD\" && echo --no-checkout)\n-\tres=$?\n-\n-\t# Check if we should exit because bisection is finished\n-\tif test $res -eq 10\n-\tthen\n-\t\tbad_rev=$(git show-ref --hash --verify refs/bisect/$TERM_BAD)\n-\t\tbad_commit=$(git show-branch $bad_rev)\n-\t\techo \"# first $TERM_BAD commit: $bad_commit\" >>\"$GIT_DIR/BISECT_LOG\"\n-\t\texit 0\n-\telif test $res -eq 2\n-\tthen\n-\t\techo \"# only skipped commits left to test\" >>\"$GIT_DIR/BISECT_LOG\"\n-\t\tgood_revs=$(git for-each-ref --format=\"%(objectname)\" \"refs/bisect/$TERM_GOOD-*\")\n-\t\tfor skipped in $(git rev-list refs/bisect/$TERM_BAD --not $good_revs)\n-\t\tdo\n-\t\t\tskipped_commit=$(git show-branch $skipped)\n-\t\t\techo \"# possible first $TERM_BAD commit: $skipped_commit\" >>\"$GIT_DIR/BISECT_LOG\"\n-\t\tdone\n-\t\texit $res\n-\tfi\n-\n-\t# Check for an error in the bisection process\n-\ttest $res -ne 0 && exit $res\n-\n-\treturn 0\n+\tgit bisect--helper --bisect-auto-next\n }\n \n bisect_visualize() {\n@@ -211,7 +172,7 @@ bisect_replay () {\n \t\t\tdie \"$(gettext \"?? what are you talking about?\")\" ;;\n \t\tesac\n \tdone <\"$file\"\n-\tbisect_auto_next\n+\tgit bisect--helper --bisect-auto-next\n }\n \n bisect_run () {\n@@ -308,7 +269,7 @@ case \"$#\" in\n \t\tbisect_skip \"$@\" ;;\n \tnext)\n \t\t# Not sure we want \"next\" at the UI level anymore.\n-\t\tbisect_next \"$@\" ;;\n+\t\tgit bisect--helper --bisect-next \"$@\" || exit ;;\n \tvisualize|view)\n \t\tbisect_visualize \"$@\" ;;\n \treset)\n-- \n2.25.0\n\n"},{"id":"395988","messageId":"20200423070704.39872-8-mirucam@gmail.com","threadId":"53293","inReplyTo":"20200423070704.39872-1-mirucam@gmail.com","subject":"[PATCH v3 07/12] bisect--helper: retire `--bisect-clean-state` subcommand","fromName":"Miriam Rubio","fromEmail":"mirucam@gmail.com","sentAt":"2020-04-23T07:06:59Z","receivedAt":"2020-04-23T07:09:47Z","isPatch":true,"sender":{"key":"mirucam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56339109?v=4"},"body":"From: Pranit Bauva \n\nThe `--bisect-clean-state` subcommand is no longer used from the\ngit-bisect.sh shell script. Instead the function\n`bisect_clean_state()` is directly called from the C\nimplementation.\n\nMentored-by: Lars Schneider \nMentored-by: Christian Couder \nSigned-off-by: Pranit Bauva \nSigned-off-by: Tanushree Tumane \nSigned-off-by: Miriam Rubio \n---\n builtin/bisect--helper.c | 8 --------\n 1 file changed, 8 deletions(-)\n\ndiff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\nindex f2ec7f89e4..1b4d0a0f87 100644\n--- a/builtin/bisect--helper.c\n+++ b/builtin/bisect--helper.c\n@@ -22,7 +22,6 @@ static GIT_PATH_FUNC(git_path_bisect_names, \"BISECT_NAMES\")\n static const char * const git_bisect_helper_usage[] = {\n \tN_(\"git bisect--helper --next-all [--no-checkout]\"),\n \tN_(\"git bisect--helper --write-terms \"),\n-\tN_(\"git bisect--helper --bisect-clean-state\"),\n \tN_(\"git bisect--helper --bisect-reset []\"),\n \tN_(\"git bisect--helper --bisect-write [--no-log] \"),\n \tN_(\"git bisect--helper --bisect-check-and-set-terms \"),\n@@ -841,7 +840,6 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n \tenum {\n \t\tNEXT_ALL = 1,\n \t\tWRITE_TERMS,\n-\t\tBISECT_CLEAN_STATE,\n \t\tCHECK_EXPECTED_REVS,\n \t\tBISECT_RESET,\n \t\tBISECT_WRITE,\n@@ -859,8 +857,6 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n \t\t\t N_(\"perform 'git bisect next'\"), NEXT_ALL),\n \t\tOPT_CMDMODE(0, \"write-terms\", &cmdmode,\n \t\t\t N_(\"write the terms to .git/BISECT_TERMS\"), WRITE_TERMS),\n-\t\tOPT_CMDMODE(0, \"bisect-clean-state\", &cmdmode,\n-\t\t\t N_(\"cleanup the bisection state\"), BISECT_CLEAN_STATE),\n \t\tOPT_CMDMODE(0, \"check-expected-revs\", &cmdmode,\n \t\t\t N_(\"check for expected revs\"), CHECK_EXPECTED_REVS),\n \t\tOPT_CMDMODE(0, \"bisect-reset\", &cmdmode,\n@@ -904,10 +900,6 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n \t\tif (argc != 2)\n \t\t\treturn error(_(\"--write-terms requires two arguments\"));\n \t\treturn write_terms(argv[0], argv[1]);\n-\tcase BISECT_CLEAN_STATE:\n-\t\tif (argc != 0)\n-\t\t\treturn error(_(\"--bisect-clean-state requires no arguments\"));\n-\t\treturn bisect_clean_state();\n \tcase CHECK_EXPECTED_REVS:\n \t\tcheck_expected_revs(argv, argc);\n \t\treturn 0;\n-- \n2.25.0\n\n"},{"id":"395989","messageId":"20200423070704.39872-7-mirucam@gmail.com","threadId":"53293","inReplyTo":"20200423070704.39872-1-mirucam@gmail.com","subject":"[PATCH v3 06/12] bisect--helper: finish porting `bisect_start()` to C","fromName":"Miriam Rubio","fromEmail":"mirucam@gmail.com","sentAt":"2020-04-23T07:06:58Z","receivedAt":"2020-04-23T07:09:48Z","isPatch":true,"sender":{"key":"mirucam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56339109?v=4"},"body":"From: Pranit Bauva \n\nAdd the subcommand to `git bisect--helper` and call it from\ngit-bisect.sh.\n\nWith the conversion of `bisect_auto_next()` from shell to C in a\nprevious commit, `bisect_start()` can now be fully ported to C.\n\nSo let's complete the `--bisect-start` subcommand of\n`git bisect--helper` so that it fully implements `bisect_start()`,\nand let's use this subcommand in `git-bisect.sh` instead of\n`bisect_start()`.\n\nThis removes the signal handling we had in `bisect_start()` as we\ndon't really need it. While at it, \"trap\" is changed to \"handle\".\nAs \"trap\" is a reference to the shell \"trap\" builtin, which isn't\nused anymore.\n\nMentored-by: Christian Couder \nMentored-by: Johannes Schindelin \nSigned-off-by: Pranit Bauva \nSigned-off-by: Tanushree Tumane \nSigned-off-by: Miriam Rubio \n---\n builtin/bisect--helper.c | 54 +++++++++++++++++++++++++++++++---------\n git-bisect.sh | 26 ++-----------------\n 2 files changed, 44 insertions(+), 36 deletions(-)\n\ndiff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\nindex c6aaa8eb15..f2ec7f89e4 100644\n--- a/builtin/bisect--helper.c\n+++ b/builtin/bisect--helper.c\n@@ -588,11 +588,12 @@ static enum bisect_error bisect_auto_next(struct bisect_terms *terms, const char\n \treturn bisect_next(terms, prefix);\n }\n \n-static int bisect_start(struct bisect_terms *terms, int no_checkout,\n+static enum bisect_error bisect_start(struct bisect_terms *terms, int no_checkout,\n \t\t\tconst char **argv, int argc)\n {\n \tint i, has_double_dash = 0, must_write_terms = 0, bad_seen = 0;\n-\tint flags, pathspec_pos, res = 0;\n+\tint flags, pathspec_pos;\n+\tenum bisect_error res = BISECT_OK;\n \tstruct string_list revs = STRING_LIST_INIT_DUP;\n \tstruct string_list states = STRING_LIST_INIT_DUP;\n \tstruct strbuf start_head = STRBUF_INIT;\n@@ -645,9 +646,12 @@ static int bisect_start(struct bisect_terms *terms, int no_checkout,\n \t\t\treturn error(_(\"unrecognized option: '%s'\"), arg);\n \t\t} else {\n \t\t\tchar *commit_id = xstrfmt(\"%s^{commit}\", arg);\n-\t\t\tif (get_oid(commit_id, &oid) && has_double_dash)\n-\t\t\t\tdie(_(\"'%s' does not appear to be a valid \"\n-\t\t\t\t \"revision\"), arg);\n+\t\t\tif (get_oid(commit_id, &oid) && has_double_dash) {\n+\t\t\t\terror(_(\"'%s' does not appear to be a valid \"\n+\t\t\t\t\t\"revision\"), arg);\n+\t\t\t\tfree(commit_id);\n+\t\t\t\treturn BISECT_FAILED;\n+\t\t\t}\n \n \t\t\tstring_list_append(&revs, oid_to_hex(&oid));\n \t\t\tfree(commit_id);\n@@ -725,12 +729,12 @@ static int bisect_start(struct bisect_terms *terms, int no_checkout,\n \t * Get rid of any old bisect state.\n \t */\n \tif (bisect_clean_state())\n-\t\treturn -1;\n+\t\treturn BISECT_FAILED;\n \n \t/*\n-\t * In case of mistaken revs or checkout error, or signals received,\n+\t * In case of mistaken revs or checkout error,\n \t * \"bisect_auto_next\" below may exit or misbehave.\n-\t * We have to trap this to be able to clean up using\n+\t * We have to handle this to be able to clean up using\n \t * \"bisect_clean_state\".\n \t */\n \n@@ -746,7 +750,7 @@ static int bisect_start(struct bisect_terms *terms, int no_checkout,\n \t\t}\n \t\tif (update_ref(NULL, \"BISECT_HEAD\", &oid, NULL, 0,\n \t\t\t UPDATE_REFS_MSG_ON_ERR)) {\n-\t\t\tres = -1;\n+\t\t\tres = BISECT_FAILED;\n \t\t\tgoto finish;\n \t\t}\n \t}\n@@ -758,25 +762,51 @@ static int bisect_start(struct bisect_terms *terms, int no_checkout,\n \tfor (i = 0; i < states.nr; i++)\n \t\tif (bisect_write(states.items[i].string,\n \t\t\t\t revs.items[i].string, terms, 1)) {\n-\t\t\tres = -1;\n+\t\t\tres = BISECT_FAILED;\n \t\t\tgoto finish;\n \t\t}\n \n \tif (must_write_terms && write_terms(terms->term_bad,\n \t\t\t\t\t terms->term_good)) {\n-\t\tres = -1;\n+\t\tres = BISECT_FAILED;\n \t\tgoto finish;\n \t}\n \n \tres = bisect_append_log_quoted(argv);\n \tif (res)\n-\t\tres = -1;\n+\t\tres = BISECT_FAILED;\n \n finish:\n \tstring_list_clear(&revs, 0);\n \tstring_list_clear(&states, 0);\n \tstrbuf_release(&start_head);\n \tstrbuf_release(&bisect_names);\n+\tif (res)\n+\t\treturn res;\n+\n+\tres = bisect_auto_next(terms, NULL);\n+\t/*\n+\t * In case of mistaken revs or checkout error, or signals received,\n+\t * \"bisect_auto_next\" below may exit or misbehave.\n+\t * We have to handle this to be able to clean up using\n+\t * \"bisect_clean_state\".\n+\t * return code BISECT_INTERNAL_SUCCESS_MERGE_BASE is special code\n+\t * that indicates special success.\n+\t *\t-> bisect_start()\n+\t *\t . res = bisect_auto_next()\n+\t *\t -> bisect_auto_next()\n+\t *\t . return bisect_next()\n+\t *\t -> bisect_next()\n+\t *\t\t . res = bisect_next_all()\n+\t *\t\t -> bisect_next_all()\n+\t *\t\t . res = check_good_are_ancestors_of_bad()\n+\t *\t\t -> check_good_are_ancestors_of_bad()\n+\t *\t\t\t. res = check_merge_bases()\n+\t *\t\t\t-> check_merge_bases()\n+\t *\t\t\t . res = BISECT_INTERNAL_SUCCESS_MERGE_BASE\n+\t */\n+\tif (res && res != BISECT_INTERNAL_SUCCESS_MERGE_BASE)\n+\t\tbisect_clean_state();\n \treturn res;\n }\n \ndiff --git a/git-bisect.sh b/git-bisect.sh\nindex 897825b675..049ffacdff 100755\n--- a/git-bisect.sh\n+++ b/git-bisect.sh\n@@ -49,27 +49,6 @@ bisect_head()\n \tfi\n }\n \n-bisect_start() {\n-\tgit bisect--helper --bisect-start $@ || exit\n-\n-\t#\n-\t# Change state.\n-\t# In case of mistaken revs or checkout error, or signals received,\n-\t# \"bisect_auto_next\" below may exit or misbehave.\n-\t# We have to trap this to be able to clean up using\n-\t# \"bisect_clean_state\".\n-\t#\n-\ttrap 'git bisect--helper --bisect-clean-state' 0\n-\ttrap 'exit 255' 1 2 3 15\n-\n-\t#\n-\t# Check if we can proceed to the next bisect state.\n-\t#\n-\tgit bisect--helper --bisect-auto-next || exit\n-\n-\ttrap '-' 0\n-}\n-\n bisect_skip() {\n \tall=''\n \tfor arg in \"$@\"\n@@ -162,8 +141,7 @@ bisect_replay () {\n \t\tget_terms\n \t\tcase \"$command\" in\n \t\tstart)\n-\t\t\tcmd=\"bisect_start $rev\"\n-\t\t\teval \"$cmd\" ;;\n+\t\t\teval \"git bisect--helper --bisect-start $rev\" ;;\n \t\t\"$TERM_GOOD\"|\"$TERM_BAD\"|skip)\n \t\t\tgit bisect--helper --bisect-write \"$command\" \"$rev\" \"$TERM_GOOD\" \"$TERM_BAD\" || exit;;\n \t\tterms)\n@@ -262,7 +240,7 @@ case \"$#\" in\n \thelp)\n \t\tgit bisect -h ;;\n \tstart)\n-\t\tbisect_start \"$@\" ;;\n+\t\tgit bisect--helper --bisect-start \"$@\" ;;\n \tbad|good|new|old|\"$TERM_BAD\"|\"$TERM_GOOD\")\n \t\tbisect_state \"$cmd\" \"$@\" ;;\n \tskip)\n-- \n2.25.0\n\n"},{"id":"395990","messageId":"20200423070704.39872-9-mirucam@gmail.com","threadId":"53293","inReplyTo":"20200423070704.39872-1-mirucam@gmail.com","subject":"[PATCH v3 08/12] bisect--helper: retire `--next-all` subcommand","fromName":"Miriam Rubio","fromEmail":"mirucam@gmail.com","sentAt":"2020-04-23T07:07:00Z","receivedAt":"2020-04-23T07:09:49Z","isPatch":true,"sender":{"key":"mirucam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56339109?v=4"},"body":"From: Pranit Bauva \n\nThe `--next-all` subcommand is no longer used from the git-bisect.sh\nshell script. Instead the function `bisect_next_all()` is called from\nthe C implementation of `bisect_next()`.\n\nMentored-by: Lars Schneider \nMentored-by: Christian Couder \nSigned-off-by: Pranit Bauva \nSigned-off-by: Tanushree Tumane \nSigned-off-by: Miriam Rubio \n---\n builtin/bisect--helper.c | 9 +--------\n 1 file changed, 1 insertion(+), 8 deletions(-)\n\ndiff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\nindex 1b4d0a0f87..2d8660c79f 100644\n--- a/builtin/bisect--helper.c\n+++ b/builtin/bisect--helper.c\n@@ -20,7 +20,6 @@ static GIT_PATH_FUNC(git_path_head_name, \"head-name\")\n static GIT_PATH_FUNC(git_path_bisect_names, \"BISECT_NAMES\")\n \n static const char * const git_bisect_helper_usage[] = {\n-\tN_(\"git bisect--helper --next-all [--no-checkout]\"),\n \tN_(\"git bisect--helper --write-terms \"),\n \tN_(\"git bisect--helper --bisect-reset []\"),\n \tN_(\"git bisect--helper --bisect-write [--no-log] \"),\n@@ -838,8 +837,7 @@ static int bisect_autostart(struct bisect_terms *terms)\n int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n {\n \tenum {\n-\t\tNEXT_ALL = 1,\n-\t\tWRITE_TERMS,\n+\t\tWRITE_TERMS = 1,\n \t\tCHECK_EXPECTED_REVS,\n \t\tBISECT_RESET,\n \t\tBISECT_WRITE,\n@@ -853,8 +851,6 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n \t} cmdmode = 0;\n \tint no_checkout = 0, res = 0, nolog = 0;\n \tstruct option options[] = {\n-\t\tOPT_CMDMODE(0, \"next-all\", &cmdmode,\n-\t\t\t N_(\"perform 'git bisect next'\"), NEXT_ALL),\n \t\tOPT_CMDMODE(0, \"write-terms\", &cmdmode,\n \t\t\t N_(\"write the terms to .git/BISECT_TERMS\"), WRITE_TERMS),\n \t\tOPT_CMDMODE(0, \"check-expected-revs\", &cmdmode,\n@@ -893,9 +889,6 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n \t\tusage_with_options(git_bisect_helper_usage, options);\n \n \tswitch (cmdmode) {\n-\tcase NEXT_ALL:\n-\t\tres = bisect_next_all(the_repository, prefix, no_checkout);\n-\t\tbreak;\n \tcase WRITE_TERMS:\n \t\tif (argc != 2)\n \t\t\treturn error(_(\"--write-terms requires two arguments\"));\n-- \n2.25.0\n\n"},{"id":"395991","messageId":"20200423070704.39872-11-mirucam@gmail.com","threadId":"53293","inReplyTo":"20200423070704.39872-1-mirucam@gmail.com","subject":"[PATCH v3 10/12] bisect--helper: retire `--check-expected-revs` subcommand","fromName":"Miriam Rubio","fromEmail":"mirucam@gmail.com","sentAt":"2020-04-23T07:07:02Z","receivedAt":"2020-04-23T07:09:51Z","isPatch":true,"sender":{"key":"mirucam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56339109?v=4"},"body":"From: Pranit Bauva \n\nThe `--check-expected-revs` subcommand is no longer used from the\ngit-bisect.sh shell script. Instead the function\n`check_expected_revs()` is called from the C implementation of\n`bisect-next()`.\n\nMentored-by: Lars Schneider \nMentored-by: Christian Couder \nMentored-by: Johannes Schindelin \nSigned-off-by: Pranit Bauva \nSigned-off-by: Tanushree Tumane \nSigned-off-by: Miriam Rubio \n---\n builtin/bisect--helper.c | 6 ------\n 1 file changed, 6 deletions(-)\n\ndiff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\nindex 9db72f5891..7db58ada6f 100644\n--- a/builtin/bisect--helper.c\n+++ b/builtin/bisect--helper.c\n@@ -898,7 +898,6 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n {\n \tenum {\n \t\tWRITE_TERMS = 1,\n-\t\tCHECK_EXPECTED_REVS,\n \t\tBISECT_RESET,\n \t\tBISECT_WRITE,\n \t\tCHECK_AND_SET_TERMS,\n@@ -914,8 +913,6 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n \tstruct option options[] = {\n \t\tOPT_CMDMODE(0, \"write-terms\", &cmdmode,\n \t\t\t N_(\"write the terms to .git/BISECT_TERMS\"), WRITE_TERMS),\n-\t\tOPT_CMDMODE(0, \"check-expected-revs\", &cmdmode,\n-\t\t\t N_(\"check for expected revs\"), CHECK_EXPECTED_REVS),\n \t\tOPT_CMDMODE(0, \"bisect-reset\", &cmdmode,\n \t\t\t N_(\"reset the bisection state\"), BISECT_RESET),\n \t\tOPT_CMDMODE(0, \"bisect-write\", &cmdmode,\n@@ -956,9 +953,6 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n \t\tif (argc != 2)\n \t\t\treturn error(_(\"--write-terms requires two arguments\"));\n \t\treturn write_terms(argv[0], argv[1]);\n-\tcase CHECK_EXPECTED_REVS:\n-\t\tcheck_expected_revs(argv, argc);\n-\t\treturn 0;\n \tcase BISECT_RESET:\n \t\tif (argc > 1)\n \t\t\treturn error(_(\"--bisect-reset requires either no argument or a commit\"));\n-- \n2.25.0\n\n"},{"id":"395992","messageId":"20200423070704.39872-10-mirucam@gmail.com","threadId":"53293","inReplyTo":"20200423070704.39872-1-mirucam@gmail.com","subject":"[PATCH v3 09/12] bisect--helper: reimplement `bisect_state` & `bisect_head` shell functions in C","fromName":"Miriam Rubio","fromEmail":"mirucam@gmail.com","sentAt":"2020-04-23T07:07:01Z","receivedAt":"2020-04-23T07:09:51Z","isPatch":true,"sender":{"key":"mirucam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56339109?v=4"},"body":"From: Pranit Bauva \n\nReimplement the `bisect_state()` shell functions in C and also add a\nsubcommand `--bisect-state` to `git-bisect--helper` to call them from\ngit-bisect.sh .\n\nUsing `--bisect-state` subcommand is a temporary measure to port shell\nfunction to C so as to use the existing test suite. As more functions\nare ported, this subcommand will be retired and will be called by some\nother methods.\n\n`bisect_head()` is only called from `bisect_state()`, thus it is not\nrequired to introduce another subcommand.\n\nMentored-by: Lars Schneider \nMentored-by: Christian Couder \nMentored-by: Johannes Schindelin \nSigned-off-by: Pranit Bauva \nSigned-off-by: Tanushree Tumane \nSigned-off-by: Miriam Rubio \n---\n builtin/bisect--helper.c | 70 +++++++++++++++++++++++++++++++++++++++-\n git-bisect.sh | 55 +++----------------------------\n 2 files changed, 73 insertions(+), 52 deletions(-)\n\ndiff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\nindex 2d8660c79f..9db72f5891 100644\n--- a/builtin/bisect--helper.c\n+++ b/builtin/bisect--helper.c\n@@ -31,6 +31,8 @@ static const char * const git_bisect_helper_usage[] = {\n \tN_(\"git bisect--helper --bisect-next\"),\n \tN_(\"git bisect--helper --bisect-auto-next\"),\n \tN_(\"git bisect--helper --bisect-autostart\"),\n+\tN_(\"git bisect--helper --bisect-state (bad|new) []\"),\n+\tN_(\"git bisect--helper --bisect-state (good|old) [...]\"),\n \tNULL\n };\n \n@@ -834,6 +836,64 @@ static int bisect_autostart(struct bisect_terms *terms)\n \treturn bisect_start(terms, 0, NULL, 0);\n }\n \n+static int bisect_head(struct object_id *oid)\n+{\n+\tif (!file_exists(git_path_bisect_head()))\n+\t\treturn get_oid(\"HEAD\", oid);\n+\n+\treturn get_oid(\"BISECT_HEAD\", oid);\n+}\n+\n+static enum bisect_error bisect_state(struct bisect_terms *terms, const char **argv,\n+\t\t\tint argc)\n+{\n+\tconst char *state;\n+\tconst char *hex;\n+\tint i;\n+\tstruct oid_array revs = OID_ARRAY_INIT;\n+\tstruct object_id oid;\n+\n+\tif (!argc)\n+\t\treturn error(_(\"Please call `--bisect-state` with at least one argument\"));\n+\tstate = argv[0];\n+\tif (check_and_set_terms(terms, state) ||\n+\t !one_of(state, terms->term_good,terms->term_bad, \"skip\", NULL))\n+\t\treturn BISECT_FAILED;\n+\targv++;\n+\targc--;\n+\tif (!strcmp(state, terms->term_bad) && (argc > 1))\n+\t\treturn error(_(\"'git bisect %s' can take only one argument.\"),terms->term_bad);\n+\tif (argc == 0) {\n+\t\tif (bisect_head(&oid))\n+\t\t\treturn error(_(\"Bad bisect_head rev input\"));\n+\t\thex = oid_to_hex(&oid);\n+\t\tif (bisect_write(state, hex, terms, 0))\n+\t\t\treturn BISECT_FAILED;\n+\t\tcheck_expected_revs(&hex, 1);\n+\t\treturn bisect_auto_next(terms, NULL);\n+\t}\n+\n+\t/* Here argc > 0 */\n+\tfor (; argc; argc--, argv++) {\n+\t\tstruct object_id oid;\n+\t\tif (get_oid(*argv, &oid))\n+\t\t\treturn error(_(\"Bad rev input: %s\"), *argv);\n+\t\toid_array_append(&revs, &oid);\n+\t}\n+\n+\tfor (i = 0; i < revs.nr; i++) {\n+\t\thex = oid_to_hex(&revs.oid[i]);\n+\t\tif (bisect_write(state, hex, terms, 0)) {\n+\t\t\toid_array_clear(&revs);\n+\t\t\treturn BISECT_FAILED;\n+\t\t}\n+\t\tcheck_expected_revs(&hex, 1);\n+\t}\n+\n+\toid_array_clear(&revs);\n+\treturn bisect_auto_next(terms, NULL);\n+}\n+\n int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n {\n \tenum {\n@@ -847,7 +907,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n \t\tBISECT_START,\n \t\tBISECT_NEXT,\n \t\tBISECT_AUTO_NEXT,\n-\t\tBISECT_AUTOSTART\n+\t\tBISECT_AUTOSTART,\n+\t\tBISECT_STATE\n \t} cmdmode = 0;\n \tint no_checkout = 0, res = 0, nolog = 0;\n \tstruct option options[] = {\n@@ -873,6 +934,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n \t\t\t N_(\"verify the next bisection state then checkout the next bisection commit\"), BISECT_AUTO_NEXT),\n \t\tOPT_CMDMODE(0, \"bisect-autostart\", &cmdmode,\n \t\t\t N_(\"start the bisection if BISECT_START is empty or missing\"), BISECT_AUTOSTART),\n+\t\tOPT_CMDMODE(0, \"bisect-state\", &cmdmode,\n+\t\t\t N_(\"mark the state of ref (or refs)\"), BISECT_STATE),\n \t\tOPT_BOOL(0, \"no-checkout\", &no_checkout,\n \t\t\t N_(\"update BISECT_HEAD instead of checking out the current commit\")),\n \t\tOPT_BOOL(0, \"no-log\", &nolog,\n@@ -945,6 +1008,11 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n \t\tset_terms(&terms, \"bad\", \"good\");\n \t\tres = bisect_autostart(&terms);\n \t\tbreak;\n+\tcase BISECT_STATE:\n+\t\tset_terms(&terms, \"bad\", \"good\");\n+\t\tget_terms(&terms);\n+\t\tres = bisect_state(&terms, argv, argc);\n+\t\tbreak;\n \tdefault:\n \t\tBUG(\"unknown subcommand %d\", (int)cmdmode);\n \t}\ndiff --git a/git-bisect.sh b/git-bisect.sh\nindex 049ffacdff..2da0810b1a 100755\n--- a/git-bisect.sh\n+++ b/git-bisect.sh\n@@ -39,16 +39,6 @@ _x40=\"$_x40$_x40$_x40$_x40$_x40$_x40$_x40$_x40\"\n TERM_BAD=bad\n TERM_GOOD=good\n \n-bisect_head()\n-{\n-\tif test -f \"$GIT_DIR/BISECT_HEAD\"\n-\tthen\n-\t\techo BISECT_HEAD\n-\telse\n-\t\techo HEAD\n-\tfi\n-}\n-\n bisect_skip() {\n \tall=''\n \tfor arg in \"$@\"\n@@ -61,43 +51,7 @@ bisect_skip() {\n \t\tesac\n \t\tall=\"$all $revs\"\n \tdone\n-\teval bisect_state 'skip' $all\n-}\n-\n-bisect_state() {\n-\tgit bisect--helper --bisect-autostart\n-\tstate=$1\n-\tgit bisect--helper --check-and-set-terms $state $TERM_GOOD $TERM_BAD || exit\n-\tget_terms\n-\tcase \"$#,$state\" in\n-\t0,*)\n-\t\tdie \"Please call 'bisect_state' with at least one argument.\" ;;\n-\t1,\"$TERM_BAD\"|1,\"$TERM_GOOD\"|1,skip)\n-\t\tbisected_head=$(bisect_head)\n-\t\trev=$(git rev-parse --verify \"$bisected_head\") ||\n-\t\t\tdie \"$(eval_gettext \"Bad rev input: \\$bisected_head\")\"\n-\t\tgit bisect--helper --bisect-write \"$state\" \"$rev\" \"$TERM_GOOD\" \"$TERM_BAD\" || exit\n-\t\tgit bisect--helper --check-expected-revs \"$rev\" ;;\n-\t2,\"$TERM_BAD\"|*,\"$TERM_GOOD\"|*,skip)\n-\t\tshift\n-\t\thash_list=''\n-\t\tfor rev in \"$@\"\n-\t\tdo\n-\t\t\tsha=$(git rev-parse --verify \"$rev^{commit}\") ||\n-\t\t\t\tdie \"$(eval_gettext \"Bad rev input: \\$rev\")\"\n-\t\t\thash_list=\"$hash_list $sha\"\n-\t\tdone\n-\t\tfor rev in $hash_list\n-\t\tdo\n-\t\t\tgit bisect--helper --bisect-write \"$state\" \"$rev\" \"$TERM_GOOD\" \"$TERM_BAD\" || exit\n-\t\tdone\n-\t\tgit bisect--helper --check-expected-revs $hash_list ;;\n-\t*,\"$TERM_BAD\")\n-\t\tdie \"$(eval_gettext \"'git bisect \\$TERM_BAD' can take only one argument.\")\" ;;\n-\t*)\n-\t\tusage ;;\n-\tesac\n-\tgit bisect--helper --bisect-auto-next\n+\teval git bisect--helper --bisect-state 'skip' $all\n }\n \n bisect_visualize() {\n@@ -185,8 +139,7 @@ exit code \\$res from '\\$command' is < 0 or >= 128\" >&2\n \t\t\tstate=\"$TERM_GOOD\"\n \t\tfi\n \n-\t\t# We have to use a subshell because \"bisect_state\" can exit.\n-\t\t( bisect_state $state >\"$GIT_DIR/BISECT_RUN\" )\n+\t\tgit bisect--helper --bisect-state $state >\"$GIT_DIR/BISECT_RUN\"\n \t\tres=$?\n \n \t\tcat \"$GIT_DIR/BISECT_RUN\"\n@@ -201,7 +154,7 @@ exit code \\$res from '\\$command' is < 0 or >= 128\" >&2\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+'git bisect--helper --bisect-state \\$state' exited with error code \\$res\" >&2\n \t\t\texit $res\n \t\tfi\n \n@@ -242,7 +195,7 @@ case \"$#\" in\n \tstart)\n \t\tgit bisect--helper --bisect-start \"$@\" ;;\n \tbad|good|new|old|\"$TERM_BAD\"|\"$TERM_GOOD\")\n-\t\tbisect_state \"$cmd\" \"$@\" ;;\n+\t\tgit bisect--helper --bisect-state \"$cmd\" \"$@\" ;;\n \tskip)\n \t\tbisect_skip \"$@\" ;;\n \tnext)\n-- \n2.25.0\n\n"},{"id":"395993","messageId":"20200423070704.39872-12-mirucam@gmail.com","threadId":"53293","inReplyTo":"20200423070704.39872-1-mirucam@gmail.com","subject":"[PATCH v3 11/12] bisect--helper: retire `--write-terms` subcommand","fromName":"Miriam Rubio","fromEmail":"mirucam@gmail.com","sentAt":"2020-04-23T07:07:03Z","receivedAt":"2020-04-23T07:09:54Z","isPatch":true,"sender":{"key":"mirucam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56339109?v=4"},"body":"From: Pranit Bauva \n\nThe `--write-terms` subcommand is no longer used from the\ngit-bisect.sh shell script. Instead the function `write_terms()`\nis called from the C implementation of `set_terms()` and\n`bisect_start()`.\n\nMentored-by: Lars Schneider \nMentored-by: Christian Couder \nMentored-by: Johannes Schindelin \nSigned-off-by: Pranit Bauva \nSigned-off-by: Tanushree Tumane \nSigned-off-by: Miriam Rubio \n---\n builtin/bisect--helper.c | 10 +---------\n 1 file changed, 1 insertion(+), 9 deletions(-)\n\ndiff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\nindex 7db58ada6f..e7467dc1c7 100644\n--- a/builtin/bisect--helper.c\n+++ b/builtin/bisect--helper.c\n@@ -20,7 +20,6 @@ static GIT_PATH_FUNC(git_path_head_name, \"head-name\")\n static GIT_PATH_FUNC(git_path_bisect_names, \"BISECT_NAMES\")\n \n static const char * const git_bisect_helper_usage[] = {\n-\tN_(\"git bisect--helper --write-terms \"),\n \tN_(\"git bisect--helper --bisect-reset []\"),\n \tN_(\"git bisect--helper --bisect-write [--no-log] \"),\n \tN_(\"git bisect--helper --bisect-check-and-set-terms \"),\n@@ -897,8 +896,7 @@ static enum bisect_error bisect_state(struct bisect_terms *terms, const char **a\n int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n {\n \tenum {\n-\t\tWRITE_TERMS = 1,\n-\t\tBISECT_RESET,\n+\t\tBISECT_RESET = 1,\n \t\tBISECT_WRITE,\n \t\tCHECK_AND_SET_TERMS,\n \t\tBISECT_NEXT_CHECK,\n@@ -911,8 +909,6 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n \t} cmdmode = 0;\n \tint no_checkout = 0, res = 0, nolog = 0;\n \tstruct option options[] = {\n-\t\tOPT_CMDMODE(0, \"write-terms\", &cmdmode,\n-\t\t\t N_(\"write the terms to .git/BISECT_TERMS\"), WRITE_TERMS),\n \t\tOPT_CMDMODE(0, \"bisect-reset\", &cmdmode,\n \t\t\t N_(\"reset the bisection state\"), BISECT_RESET),\n \t\tOPT_CMDMODE(0, \"bisect-write\", &cmdmode,\n@@ -949,10 +945,6 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n \t\tusage_with_options(git_bisect_helper_usage, options);\n \n \tswitch (cmdmode) {\n-\tcase WRITE_TERMS:\n-\t\tif (argc != 2)\n-\t\t\treturn error(_(\"--write-terms requires two arguments\"));\n-\t\treturn write_terms(argv[0], argv[1]);\n \tcase BISECT_RESET:\n \t\tif (argc > 1)\n \t\t\treturn error(_(\"--bisect-reset requires either no argument or a commit\"));\n-- \n2.25.0\n\n"},{"id":"395994","messageId":"20200423070704.39872-13-mirucam@gmail.com","threadId":"53293","inReplyTo":"20200423070704.39872-1-mirucam@gmail.com","subject":"[PATCH v3 12/12] bisect--helper: retire `--bisect-autostart` subcommand","fromName":"Miriam Rubio","fromEmail":"mirucam@gmail.com","sentAt":"2020-04-23T07:07:04Z","receivedAt":"2020-04-23T07:09:55Z","isPatch":true,"sender":{"key":"mirucam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56339109?v=4"},"body":"From: Pranit Bauva \n\nThe `--bisect-autostart` subcommand is no longer used from the\ngit-bisect.sh shell script. Instead the function\n`bisect_autostart()` is directly called from the C implementation.\n\nMentored-by: Lars Schneider \nMentored-by: Christian Couder \nMentored-by: Johannes Schindelin \nSigned-off-by: Pranit Bauva \nSigned-off-by: Tanushree Tumane \nSigned-off-by: Miriam Rubio \n---\n builtin/bisect--helper.c | 10 ----------\n 1 file changed, 10 deletions(-)\n\ndiff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\nindex e7467dc1c7..66f9b8eab6 100644\n--- a/builtin/bisect--helper.c\n+++ b/builtin/bisect--helper.c\n@@ -29,7 +29,6 @@ static const char * const git_bisect_helper_usage[] = {\n \t\t\t\t\t \"[--no-checkout] [ [...]] [--] [...]\"),\n \tN_(\"git bisect--helper --bisect-next\"),\n \tN_(\"git bisect--helper --bisect-auto-next\"),\n-\tN_(\"git bisect--helper --bisect-autostart\"),\n \tN_(\"git bisect--helper --bisect-state (bad|new) []\"),\n \tN_(\"git bisect--helper --bisect-state (good|old) [...]\"),\n \tNULL\n@@ -904,7 +903,6 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n \t\tBISECT_START,\n \t\tBISECT_NEXT,\n \t\tBISECT_AUTO_NEXT,\n-\t\tBISECT_AUTOSTART,\n \t\tBISECT_STATE\n \t} cmdmode = 0;\n \tint no_checkout = 0, res = 0, nolog = 0;\n@@ -925,8 +923,6 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n \t\t\t N_(\"find the next bisection commit\"), BISECT_NEXT),\n \t\tOPT_CMDMODE(0, \"bisect-auto-next\", &cmdmode,\n \t\t\t N_(\"verify the next bisection state then checkout the next bisection commit\"), BISECT_AUTO_NEXT),\n-\t\tOPT_CMDMODE(0, \"bisect-autostart\", &cmdmode,\n-\t\t\t N_(\"start the bisection if BISECT_START is empty or missing\"), BISECT_AUTOSTART),\n \t\tOPT_CMDMODE(0, \"bisect-state\", &cmdmode,\n \t\t\t N_(\"mark the state of ref (or refs)\"), BISECT_STATE),\n \t\tOPT_BOOL(0, \"no-checkout\", &no_checkout,\n@@ -988,12 +984,6 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n \t\tget_terms(&terms);\n \t\tres = bisect_auto_next(&terms, prefix);\n \t\tbreak;\n-\tcase BISECT_AUTOSTART:\n-\t\tif (argc)\n-\t\t\treturn error(_(\"--bisect-autostart requires 0 arguments\"));\n-\t\tset_terms(&terms, \"bad\", \"good\");\n-\t\tres = bisect_autostart(&terms);\n-\t\tbreak;\n \tcase BISECT_STATE:\n \t\tset_terms(&terms, \"bad\", \"good\");\n \t\tget_terms(&terms);\n-- \n2.25.0\n\n"},{"id":"396027","messageId":"xmqqa732gee8.fsf@gitster.c.googlers.com","threadId":"53293","inReplyTo":"20200423070704.39872-1-mirucam@gmail.com","subject":"Re: [PATCH v3 00/12] Finish converting git bisect to C part 2","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-04-23T20:01:35Z","receivedAt":"2020-04-23T20:01:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Miriam Rubio writes:\n\n> --- Changes since v2 Finish converting git bisect to C part 2 patch series ---\n>\n> General changes\n> ---------------\n>\n> * Rebase on master branch: efe3874640 (Sync with v2.26.1, 2020-04-13)\n\nWas there a particular reason why you needed to do this rebase? It\nseems that the patches apply cleanly on the same base as v2 has been\nqueued? Does the result cause hard-to-resolve conflicts when merged\nto the 'master', 'next' or 'pu' branches unless you do that rebase?\n\n> Specific changes\n> ----------------\n>\n> [1/12] bisect--helper: fix `cmd_*()` function switch default return\n>\n> * Use `BUG()` instead of `return error()` in default switch.\n\nThis rings a bell. range-diff looks good.\n\n> [2/12] bisect--helper: use '-res' in 'cmd_bisect__helper' return\n>\n> * New patch: use '-res' instead of 'abs(res)'.\n\nAlright.\n\n> [3/12] bisect--helper: introduce new `write_in_file()` function\n>\n> * Rename input parameter `filepath` to `path`.\n> * Change `error_errno()` to `error()` in mode checking.\n> * Change error message when file cannot be opened.\n> * Add `fclose()` before error return.\n\nOK. I am not sure if the error behaviour when fclose() fails is\nideal, but I do not think it is worth further polishing.\n\n> [4/12] bisect--helper: reimplement `bisect_autostart` shell function in C\n>\n> * Reorder patch before `reimplement `bisect_next` and `bisect_auto_next`\n> shell functions in C` to use `bisect_autostart()` function in \n> `bisect_append_log_quoted()`.\n\nOK.\n\nWill take a look at individual patches later.\n\nThanks.\n"},{"id":"396242","messageId":"CAN7CjDA04+9UOmzu-XtwPTnLRmwWakpVeGQuRMkuh4ruGAaZug@mail.gmail.com","threadId":"53293","inReplyTo":"xmqqa732gee8.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v3 00/12] Finish converting git bisect to C part 2","fromName":"Miriam R.","fromEmail":"mirucam@gmail.com","sentAt":"2020-04-25T10:57:32Z","receivedAt":"2020-04-25T10:57:46Z","isPatch":true,"sender":{"key":"mirucam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56339109?v=4"},"body":"Hi Junio,\n\nEl jue., 23 abr. 2020 a las 22:01, Junio C Hamano\n() escribió:\n>\n> Miriam Rubio writes:\n>\n> > --- Changes since v2 Finish converting git bisect to C part 2 patch series ---\n> >\n> > General changes\n> > ---------------\n> >\n> > * Rebase on master branch: efe3874640 (Sync with v2.26.1, 2020-04-13)\n>\n> Was there a particular reason why you needed to do this rebase? It\n> seems that the patches apply cleanly on the same base as v2 has been\n> queued? Does the result cause hard-to-resolve conflicts when merged\n> to the 'master', 'next' or 'pu' branches unless you do that rebase?\n>\nNo particular reason. I use to do a rebase on master before applying\nreviewers suggestions if I see there are new commits on master.\n\n> > Specific changes\n> > ----------------\n> >\n> > [1/12] bisect--helper: fix `cmd_*()` function switch default return\n> >\n> > * Use `BUG()` instead of `return error()` in default switch.\n>\n> This rings a bell. range-diff looks good.\n>\n> > [2/12] bisect--helper: use '-res' in 'cmd_bisect__helper' return\n> >\n> > * New patch: use '-res' instead of 'abs(res)'.\n>\n> Alright.\n>\n> > [3/12] bisect--helper: introduce new `write_in_file()` function\n> >\n> > * Rename input parameter `filepath` to `path`.\n> > * Change `error_errno()` to `error()` in mode checking.\n> > * Change error message when file cannot be opened.\n> > * Add `fclose()` before error return.\n>\n> OK. I am not sure if the error behaviour when fclose() fails is\n> ideal, but I do not think it is worth further polishing.\n>\n> > [4/12] bisect--helper: reimplement `bisect_autostart` shell function in C\n> >\n> > * Reorder patch before `reimplement `bisect_next` and `bisect_auto_next`\n> > shell functions in C` to use `bisect_autostart()` function in\n> > `bisect_append_log_quoted()`.\n>\n> OK.\n>\n> Will take a look at individual patches later.\nGreat, thank you!\n>\n> Thanks.\n"},{"id":"398420","messageId":"nycvar.QRO.7.76.6.2005221513060.56@tvgsbejvaqbjf.bet","threadId":"53293","inReplyTo":"20200423070704.39872-2-mirucam@gmail.com","subject":"Re: [PATCH v3 01/12] bisect--helper: fix `cmd_*()` function switch default return","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-05-22T13:14:06Z","receivedAt":"2020-05-22T13:14:13Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Miriam,\n\nOn Thu, 23 Apr 2020, Miriam Rubio wrote:\n\n> In a `cmd_*()` function, return `error()` cannot be used\n> because that translates to `-1` and `cmd_*()` functions need\n> to return exit codes.\n>\n> Let's fix switch default return.\n\nI think there is an even stronger argument for not returning here: it\nactually _would_ be a bug, not a user error, if the `default` branch was\nreached.\n\nThe patch looks good.\n\nCiao,\nDscho\n\n>\n> Mentored-by: Christian Couder \n> Mentored-by: Johannes Schindelin \n> Signed-off-by: Miriam Rubio \n> ---\n> builtin/bisect--helper.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\n> index c1c40b516d..a81a2c76ff 100644\n> --- a/builtin/bisect--helper.c\n> +++ b/builtin/bisect--helper.c\n> @@ -711,7 +711,7 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n> \t\tres = bisect_start(&terms, no_checkout, argv, argc);\n> \t\tbreak;\n> \tdefault:\n> -\t\treturn error(\"BUG: unknown subcommand '%d'\", cmdmode);\n> +\t\tBUG(\"unknown subcommand %d\", (int)cmdmode);\n> \t}\n> \tfree_terms(&terms);\n>\n> --\n> 2.25.0\n>\n>\n"},{"id":"398421","messageId":"nycvar.QRO.7.76.6.2005221515430.56@tvgsbejvaqbjf.bet","threadId":"53293","inReplyTo":"20200423070704.39872-3-mirucam@gmail.com","subject":"Re: [PATCH v3 02/12] bisect--helper: use '-res' in 'cmd_bisect__helper' return","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-05-22T13:16:01Z","receivedAt":"2020-05-22T13:16:18Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Miriam,\n\nOn Thu, 23 Apr 2020, Miriam Rubio wrote:\n\n> Following 'enum bisect_error' vocabulary, return variable 'res' is\n> always non-positive.\n> Let's use '-res' instead of 'abs(res)' to make the code clearer.\n\nI agree with this reasoning, and with the patch.\n\nThank you,\nDscho\n\n>\n> Mentored-by: Christian Couder \n> Signed-off-by: Miriam Rubio \n> Helped-by: Junio C Hamano \n> ---\n> builtin/bisect--helper.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\n> index a81a2c76ff..0fbd924aac 100644\n> --- a/builtin/bisect--helper.c\n> +++ b/builtin/bisect--helper.c\n> @@ -722,5 +722,5 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n> \tif (res == BISECT_INTERNAL_SUCCESS_MERGE_BASE)\n> \t\tres = BISECT_OK;\n>\n> -\treturn abs(res);\n> +\treturn -res;\n> }\n> --\n> 2.25.0\n>\n>\n"},{"id":"398422","messageId":"nycvar.QRO.7.76.6.2005221516540.56@tvgsbejvaqbjf.bet","threadId":"53293","inReplyTo":"20200423070704.39872-4-mirucam@gmail.com","subject":"Re: [PATCH v3 03/12] bisect--helper: introduce new `write_in_file()` function","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-05-22T13:25:05Z","receivedAt":"2020-05-22T13:25:14Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Miriam,\n\nOn Thu, 23 Apr 2020, Miriam Rubio wrote:\n\n> Let's refactor code adding a new `write_in_file()` function\n> that opens a file for writing a message and closes it.\n\nDon't we have that already?\nhttps://github.com/git/git/blob/v2.27.0-rc1/wrapper.c#L625-L638\n\nIf that function does not meet the bill, it would make more sense to\nextend it (or refactor it, providing a new global function that does meet\nthe bill).\n\n> This helper will be used in later steps and makes the code\n> simpler and easier to understand.\n\nMakes a lot of sense to me!\n\n> Mentored-by: Christian Couder \n> Mentored-by: Johannes Schindelin \n> Signed-off-by: Miriam Rubio \n> ---\n> builtin/bisect--helper.c | 29 +++++++++++++++++++++++------\n> 1 file changed, 23 insertions(+), 6 deletions(-)\n>\n> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\n> index 0fbd924aac..d3b2b33df0 100644\n> --- a/builtin/bisect--helper.c\n> +++ b/builtin/bisect--helper.c\n> @@ -74,6 +74,28 @@ static int one_of(const char *term, ...)\n> \treturn res;\n> }\n>\n> +static int write_in_file(const char *path, const char *mode, const char *format,...)\n\nI wonder whether there is _ever_ a case where we want to set `mode` to\nanything else than `\"w\"`? Ah...\n\n> +{\n> +\tFILE *fp = NULL;\n> +\tva_list args;\n> +\tint res = 0;\n> +\n> +\tif (!strcmp(mode, \"a\") && !strcmp(mode, \"w\"))\n> +\t\treturn error(_(\"wrong writing mode '%s'\"), mode);\n\n... rather than doing this, I would prefer a pair of functions (that can\ncall the same helper function doing the actual work): `write_to_file()`\nand `append_to_file()`.\n\nApart from that, the patch looks good to me,\nDscho\n\n> +\tfp = fopen(path, mode);\n> +\tif (!fp)\n> +\t\treturn error_errno(_(\"cannot open file '%s' in mode '%s'\"), path, mode);\n> +\tva_start(args, format);\n> +\tres = vfprintf(fp, format, args);\n> +\tva_end(args);\n> +\tif (!res) {\n> +\t\tfclose(fp);\n> +\t\treturn error_errno(_(\"could not write to file '%s'\"), path);\n> +\t}\n> +\n> +\treturn fclose(fp);\n> +}\n> +\n> static int check_term_format(const char *term, const char *orig_term)\n> {\n> \tint res;\n> @@ -104,7 +126,6 @@ static int check_term_format(const char *term, const char *orig_term)\n>\n> static int write_terms(const char *bad, const char *good)\n> {\n> -\tFILE *fp = NULL;\n> \tint res;\n>\n> \tif (!strcmp(bad, good))\n> @@ -113,12 +134,8 @@ static int write_terms(const char *bad, const char *good)\n> \tif (check_term_format(bad, \"bad\") || check_term_format(good, \"good\"))\n> \t\treturn -1;\n>\n> -\tfp = fopen(git_path_bisect_terms(), \"w\");\n> -\tif (!fp)\n> -\t\treturn error_errno(_(\"could not open the file BISECT_TERMS\"));\n> +\tres = write_in_file(git_path_bisect_terms(), \"w\", \"%s\\n%s\\n\", bad, good);\n>\n> -\tres = fprintf(fp, \"%s\\n%s\\n\", bad, good);\n> -\tres |= fclose(fp);\n> \treturn (res < 0) ? -1 : 0;\n> }\n>\n> --\n> 2.25.0\n>\n>\n"},{"id":"398434","messageId":"nycvar.QRO.7.76.6.2005222058380.56@tvgsbejvaqbjf.bet","threadId":"53293","inReplyTo":"20200423070704.39872-5-mirucam@gmail.com","subject":"Re: [PATCH v3 04/12] bisect--helper: reimplement `bisect_autostart` shell function in C","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-05-22T19:27:52Z","receivedAt":"2020-05-22T19:28:01Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Miriam,\n\nOn Thu, 23 Apr 2020, Miriam Rubio wrote:\n\n> From: Pranit Bauva \n>\n> Reimplement the `bisect_autostart()` shell function in C and add the\n> C implementation from `bisect_next()` which was previously left\n> uncovered. Also add a subcommand `--bisect-autostart` to\n> `git bisect--helper` be called from `bisect_state()` from\n> git-bisect.sh .\n\nI know you did not author this, but maybe I can still ask you to address a\ncouple of concerns I have? I kind of stumble when I read \"Also add a\nsubcommand ... be called from ... from ...\".\n\nAlso, why the funny space before the full stop?\n\n> Using `--bisect-autostart` subcommand is a temporary measure to port\n> shell function to C so as to use the existing test suite. As more\n\nLikewise, \"... to port shell function...\" might benefit from inserting\n\"the\" in the middle?\n\n> functions are ported, this subcommand will be retired and\n> bisect_autostart() will be called directly by `bisect_state()`.\n>\n> Mentored-by: Lars Schneider \n> Mentored-by: Christian Couder \n> Mentored-by: Johannes Schindelin \n> Signed-off-by: Pranit Bauva \n> Signed-off-by: Tanushree Tumane \n> Signed-off-by: Miriam Rubio \n> ---\n> builtin/bisect--helper.c | 40 +++++++++++++++++++++++++++++++++++++++-\n> git-bisect.sh | 25 ++-----------------------\n> 2 files changed, 41 insertions(+), 24 deletions(-)\n>\n> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\n> index d3b2b33df0..9df69800e3 100644\n> --- a/builtin/bisect--helper.c\n> +++ b/builtin/bisect--helper.c\n> @@ -29,6 +29,7 @@ static const char * const git_bisect_helper_usage[] = {\n> \tN_(\"git bisect--helper --bisect-terms [--term-good | --term-old | --term-bad | --term-new]\"),\n> \tN_(\"git bisect--helper --bisect-start [--term-{old,good}= --term-{new,bad}=]\"\n> \t\t\t\t\t \"[--no-checkout] [ [...]] [--] [...]\"),\n> +\tN_(\"git bisect--helper --bisect-autostart\"),\n> \tNULL\n> };\n>\n> @@ -55,6 +56,8 @@ static void set_terms(struct bisect_terms *terms, const char *bad,\n> static const char vocab_bad[] = \"bad|new\";\n> static const char vocab_good[] = \"good|old\";\n>\n> +static int bisect_autostart(struct bisect_terms *terms);\n> +\n\nWhy forward-declare it? There is no reference before it is defined below,\nso I think this forward-declaration can be removed without any problem.\n\n> /*\n> * Check whether the string `term` belongs to the set of strings\n> * included in the variable arguments.\n> @@ -630,6 +633,32 @@ static int bisect_start(struct bisect_terms *terms, int no_checkout,\n> \treturn res;\n> }\n>\n> +static int bisect_autostart(struct bisect_terms *terms)\n> +{\n> +\tconst char *yesno;\n> +\n> +\tif (!is_empty_or_missing_file(git_path_bisect_start()))\n> +\t\treturn 0;\n\nThis is a bit more convoluted to read than the shell script:\n\n\tbisect_autostart() {\n\t\ttest -s \"$GIT_DIR/BISECT_START\" || {\n\t\t\tgettextln \"You need to start by \\\"git bisect start\\\"\" >&2\n\t\t\t[...]\n\t\t}\n\t}\n\nThe `test -s ` command succeeds if the file exists and is non-empty.\nWhich makes sense: either the file exists and is non-empty, or we need to\ndo something.\n\nWhen I read the C version, it takes me a bit more time to figure out what\nis going on here. Maybe we could introduce a helper function like this and\nuse it here?\n\n\tstatic inline int file_is_not_empty(const char *path)\n\t{\n\t\treturn !is_empty_or_missing_file(path);\n\t}\n\n> +\n> +\tfprintf(stderr, _(\"You need to start by \\\"git bisect \"\n> +\t\t\t \"start\\\"\\n\"));\n\nIf you use `fprintf_ln()` here, you can keep the string identical to the\nshell script version, i.e. without the trailing newline. That will allow\nthe translations to be re-used, lessening the burden of the i18n team.\n\n> +\n> +\tif (!isatty(STDIN_FILENO))\n> +\t\treturn 1;\n\nI would prefer it if only `cmd_*()` functions return positive values for\nthe error case.\n> +\n> +\t/*\n> +\t * TRANSLATORS: Make sure to include [Y] and [n] in your\n> +\t * translation. The program will only accept English input\n> +\t * at this point.\n> +\t */\n> +\tyesno = git_prompt(_(\"Do you want me to do it for you \"\n> +\t\t\t \"[Y/n]? \"), PROMPT_ECHO);\n\nTechnically, `PROMPT_ECHO` is not required here as `git_prompt()` adds\nthat flag implicitly unless you pass in `PASS_ASKPASS`, but I guess it\ndoes not hurt here.\n\nHowever, I cannot fail to note that this constitutes a change of behavior,\nas `git_prompt()` does _not_ read from `stdin`.\n\nMaybe we should call `git_read_line_interactively()` instead, after\nwriting the prompt to `stderr` explicitly?\n\n> +\tif (starts_with(yesno, _(\"n\")) || starts_with(yesno, _(\"N\")))\n\nThis is wrong: \"n\" and \"N\" should not be translated, as they are not\ntranslated in the shell script version, either:\n\n\t\t\tcase \"$yesno\" in\n\t\t\t[Nn]*)\n\t\t\t\texit ;;\n\t\t\tesac\n\nIn light of this, a much, much shorter version would be:\n\n\tif (tolower(*yesno) == 'n')\n\t\treturn -1;\n\n(I use `-1` here because this is not within a `cmd_*()` function.)\n\nHowever, I still think that we need to call `free(yesno);` or we leak\nmemory. My preferred way would be:\n\n\t\tint ret;\n\t\t[...]\n\n\t\tret = tolower(*yesno) == 'n' ?\n\t\t\t-1 : bisect_start(terms, 0, NULL, 0);\n\n\t\tfree(yesno);\n\t\treturn ret;\n\n> +\t\treturn 1;\n> +\n> +\treturn bisect_start(terms, 0, NULL, 0);\n> +}\n> +\n> int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n> {\n> \tenum {\n> @@ -642,7 +671,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n> \t\tCHECK_AND_SET_TERMS,\n> \t\tBISECT_NEXT_CHECK,\n> \t\tBISECT_TERMS,\n> -\t\tBISECT_START\n> +\t\tBISECT_START,\n> +\t\tBISECT_AUTOSTART,\n> \t} cmdmode = 0;\n> \tint no_checkout = 0, res = 0, nolog = 0;\n> \tstruct option options[] = {\n> @@ -666,6 +696,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n> \t\t\t N_(\"print out the bisect terms\"), BISECT_TERMS),\n> \t\tOPT_CMDMODE(0, \"bisect-start\", &cmdmode,\n> \t\t\t N_(\"start the bisect session\"), BISECT_START),\n> +\t\tOPT_CMDMODE(0, \"bisect-autostart\", &cmdmode,\n> +\t\t\t N_(\"start the bisection if BISECT_START is empty or missing\"), BISECT_AUTOSTART),\n\nNot that it matters a lot because this command mode will go away soon\nthereafter, but this describes an implementation detail rather than the\npurpose of the function. I would much rather read \"start the bisection if\nit has not yet been started\" instead.\n\n> \t\tOPT_BOOL(0, \"no-checkout\", &no_checkout,\n> \t\t\t N_(\"update BISECT_HEAD instead of checking out the current commit\")),\n> \t\tOPT_BOOL(0, \"no-log\", &nolog,\n> @@ -727,6 +759,12 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n> \t\tset_terms(&terms, \"bad\", \"good\");\n> \t\tres = bisect_start(&terms, no_checkout, argv, argc);\n> \t\tbreak;\n> +\tcase BISECT_AUTOSTART:\n> +\t\tif (argc)\n> +\t\t\treturn error(_(\"--bisect-autostart requires 0 arguments\"));\n\nMaybe \"--bisect-autostart does not accept arguments\" instead?\n\nCiao,\nDscho\n\n> +\t\tset_terms(&terms, \"bad\", \"good\");\n> +\t\tres = bisect_autostart(&terms);\n> +\t\tbreak;\n> \tdefault:\n> \t\tBUG(\"unknown subcommand %d\", (int)cmdmode);\n> \t}\n> diff --git a/git-bisect.sh b/git-bisect.sh\n> index efee12b8b1..426d443e7e 100755\n> --- a/git-bisect.sh\n> +++ b/git-bisect.sh\n> @@ -49,27 +49,6 @@ bisect_head()\n> \tfi\n> }\n>\n> -bisect_autostart() {\n> -\ttest -s \"$GIT_DIR/BISECT_START\" || {\n> -\t\tgettextln \"You need to start by \\\"git bisect start\\\"\" >&2\n> -\t\tif test -t 0\n> -\t\tthen\n> -\t\t\t# TRANSLATORS: Make sure to include [Y] and [n] in your\n> -\t\t\t# translation. The program will only accept English input\n> -\t\t\t# at this point.\n> -\t\t\tgettext \"Do you want me to do it for you [Y/n]? \" >&2\n> -\t\t\tread yesno\n> -\t\t\tcase \"$yesno\" in\n> -\t\t\t[Nn]*)\n> -\t\t\t\texit ;;\n> -\t\t\tesac\n> -\t\t\tbisect_start\n> -\t\telse\n> -\t\t\texit 1\n> -\t\tfi\n> -\t}\n> -}\n> -\n> bisect_start() {\n> \tgit bisect--helper --bisect-start $@ || exit\n>\n> @@ -108,7 +87,7 @@ bisect_skip() {\n> }\n>\n> bisect_state() {\n> -\tbisect_autostart\n> +\tgit bisect--helper --bisect-autostart\n> \tstate=$1\n> \tgit bisect--helper --check-and-set-terms $state $TERM_GOOD $TERM_BAD || exit\n> \tget_terms\n> @@ -149,7 +128,7 @@ bisect_auto_next() {\n>\n> bisect_next() {\n> \tcase \"$#\" in 0) ;; *) usage ;; esac\n> -\tbisect_autostart\n> +\tgit bisect--helper --bisect-autostart\n> \tgit bisect--helper --bisect-next-check $TERM_GOOD $TERM_BAD $TERM_GOOD|| exit\n>\n> \t# Perform all bisection computation, display and checkout\n> --\n> 2.25.0\n>\n>\n"},{"id":"398437","messageId":"nycvar.QRO.7.76.6.2005222129130.56@tvgsbejvaqbjf.bet","threadId":"53293","inReplyTo":"20200423070704.39872-6-mirucam@gmail.com","subject":"Re: [PATCH v3 05/12] bisect--helper: reimplement `bisect_next` and `bisect_auto_next` shell functions in C","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-05-22T20:47:04Z","receivedAt":"2020-05-22T20:47:15Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Miriam,\n\nOn Thu, 23 Apr 2020, Miriam Rubio wrote:\n\n> From: Pranit Bauva \n>\n> Reimplement the `bisect_next()` and the `bisect_auto_next()` shell functions\n> in C and add the subcommands to `git bisect--helper` to call them from\n> git-bisect.sh .\n>\n> bisect_auto_next() function returns an enum bisect_error type as whole\n> `git bisect` can exit with an error code when bisect_next() does.\n>\n> Using `--bisect-next` and `--bisect-auto-next` subcommands is a\n> temporary measure to port shell function to C so as to use the existing\n> test suite. As more functions are ported, `--bisect-auto-next`\n> subcommand will be retired and will be called by some other methods.\n>\n> Mentored-by: Lars Schneider \n> Mentored-by: Christian Couder \n> Mentored-by: Johannes Schindelin \n> Signed-off-by: Pranit Bauva \n> Signed-off-by: Tanushree Tumane \n> Signed-off-by: Miriam Rubio \n> ---\n> bisect.c | 8 ++\n> builtin/bisect--helper.c | 169 ++++++++++++++++++++++++++++++++++++++-\n> git-bisect.sh | 47 +----------\n> 3 files changed, 179 insertions(+), 45 deletions(-)\n>\n> diff --git a/bisect.c b/bisect.c\n> index 9154f810f7..31cab3f0b0 100644\n> --- a/bisect.c\n> +++ b/bisect.c\n> @@ -980,6 +980,12 @@ void read_bisect_terms(const char **read_bad, const char **read_good)\n> * the bisection process finished successfully.\n> * In this case the calling function or command should not turn a\n> * BISECT_INTERNAL_SUCCESS_1ST_BAD_FOUND return code into an error or a non zero exit code.\n> + *\n> + * Checking BISECT_INTERNAL_SUCCESS_1ST_BAD_FOUND\n> + * in bisect_helper::bisect_next() and only transforming it to 0 at\n> + * the end of bisect_helper::cmd_bisect__helper() helps bypassing\n> + * all the code related to finding a commit to test.\n\nNice!\n\n> + *\n> * If no_checkout is non-zero, the bisection process does not\n> * checkout the trial commit but instead simply updates BISECT_HEAD.\n> */\n> @@ -1064,6 +1070,8 @@ enum bisect_error bisect_next_all(struct repository *r, const char *prefix, int\n> \t\t \"Bisecting: %d revisions left to test after this %s\\n\",\n> \t\t nr), nr, steps_msg);\n> \tfree(steps_msg);\n> +\t/* Clean up objects used, as they will be reused. */\n> +\tclear_commit_marks_all(ALL_REV_FLAGS);\n\nThat strikes me as a change that is orthogonal to the purpose claimed by\nthe commit message. As such, I think it wants to live in its own commit.\n\n>\n> \treturn bisect_checkout(bisect_rev, no_checkout);\n> }\n> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\n> index 9df69800e3..c6aaa8eb15 100644\n> --- a/builtin/bisect--helper.c\n> +++ b/builtin/bisect--helper.c\n> @@ -8,6 +8,7 @@\n> #include \"run-command.h\"\n> #include \"prompt.h\"\n> #include \"quote.h\"\n> +#include \"revision.h\"\n>\n> static GIT_PATH_FUNC(git_path_bisect_terms, \"BISECT_TERMS\")\n> static GIT_PATH_FUNC(git_path_bisect_expected_rev, \"BISECT_EXPECTED_REV\")\n> @@ -29,6 +30,8 @@ static const char * const git_bisect_helper_usage[] = {\n> \tN_(\"git bisect--helper --bisect-terms [--term-good | --term-old | --term-bad | --term-new]\"),\n> \tN_(\"git bisect--helper --bisect-start [--term-{old,good}= --term-{new,bad}=]\"\n> \t\t\t\t\t \"[--no-checkout] [ [...]] [--] [...]\"),\n> +\tN_(\"git bisect--helper --bisect-next\"),\n> +\tN_(\"git bisect--helper --bisect-auto-next\"),\n> \tN_(\"git bisect--helper --bisect-autostart\"),\n> \tNULL\n> };\n> @@ -441,6 +444,150 @@ static int bisect_append_log_quoted(const char **argv)\n> \treturn res;\n> }\n>\n> +static int register_good_ref(const char *refname,\n> +\t\t\t const struct object_id *oid, int flags,\n> +\t\t\t void *cb_data)\n> +{\n> +\tstruct argv_array *rev_argv = cb_data;\n> +\n> +\targv_array_push(rev_argv, oid_to_hex(oid));\n> +\treturn 0;\n> +}\n> +\n> +static void prepare_rev_argv(struct bisect_terms *terms, struct argv_array *rev_argv)\n> +{\n> +\tchar *term_good = xstrfmt(\"%s-*\", terms->term_good);\n\nThis could be `xstrfmt(\"refs/bisect/%s-*\", terms->term_good)` instead, and\nthat would have avoided some head-scratching on my side. The\n\"refs/bisect/\" prefix passed to `for_each_glob_ref_in()` makes this\ncorrect, of course.\n\n> +\n> +\targv_array_pushl(rev_argv, \"skipped_commits\", \"refs/bisect/bad\", \"--not\", NULL);\n\nThis hard-codes `bad` where the shell script version uses\n`refs/bisect/$TERM_BAD`.\n\n> +\tfor_each_glob_ref_in(register_good_ref, term_good, \"refs/bisect/\", rev_argv);\n> +\n> +\tfree(term_good);\n> +}\n> +\n> +static int prepare_revs(struct bisect_terms *terms, struct rev_info *revs)\n> +{\n> +\tint res = 0;\n> +\tstruct argv_array rev_argv = ARGV_ARRAY_INIT;\n> +\n> +\tprepare_rev_argv(terms, &rev_argv);\n> +\n> +\t/*\n> +\t * It is important to reset the flags used by revision walks\n> +\t * as the previous call to bisect_next_all() in turn\n> +\t * sets up a revision walk.\n> +\t */\n> +\treset_revision_walk();\n> +\tinit_revisions(revs, NULL);\n> +\trev_argv.argc = setup_revisions(rev_argv.argc, rev_argv.argv, revs, NULL);\n\nI have not heard any valid argument against my suggestion in\nhttps://lore.kernel.org/git/nycvar.QRO.7.76.6.2001301619340.46@tvgsbejvaqbjf.bet/\nto render into the pending objects directly. Yes, there was a hand-waving\n\"but the shell script version does not do that\" but that's just not a\nvalid argument because if the shell script version could, it would, but it\ncannot: it's shell script, for crying out loud. It has no way to use the\nappropriate data structures. C code does. And should. We're not in the\nbusiness of converting OIDs into strings that are appended to string lists\nso that they can be re-parsed into OIDs.\n\nTherefore, I would suggest (and now that almost four months have passed\nsince my initial suggestion, rather strongly) to do something like this:\n\n\tstruct add_bisect_ref_data {\n\t\tstruct rev_info *revs;\n\t\tint object_flags;\n\t};\n\n\tstatic int add_bisect_ref(const char *refname, const struct object_id *oid,\n\t\t\t\t int flags, void *cb)\n\t{\n\t\tstruct add_bisect_ref_data *data = cb;\n\n\t\tadd_pending_oid(data->revs, refname, oid, data->object_flags);\n\n\t\treturn 0;\n\t}\n\n\t[...]\n\t\tstruct add_bisect_ref_data cb = { &revs };\n\t\tchar *good = xstrfmt(\"%s-*\", terms->term_good);\n\n\t\t[...]\n\t\tsetup_revisions(0, NULL, revs, NULL);\n\t\tfor_each_glob_ref_in(add_bisect_ref, terms->term_bad, \"refs/bisect/\", &cb);\n\t\tcb.object_flags = UNINTERESTING;\n\t\tfor_each_glob_ref_in(add_bisect_ref, good, \"refs/bisect/\", &cb);\n\nThis would remove one (unnecessary) level of indirection, and converting\nOIDs from/to/from strings.\n\n> +\tif (prepare_revision_walk(revs))\n> +\t\tres = error(_(\"revision walk setup failed\\n\"));\n> +\n> +\targv_array_clear(&rev_argv);\n> +\treturn res;\n> +}\n> +\n> +static int process_skipped_commits(FILE *fp, struct bisect_terms *terms, struct rev_info *revs)\n> +{\n> +\tstruct commit *commit;\n> +\tstruct pretty_print_context pp = {0};\n> +\n> +\tif (fprintf(fp, \"# only skipped commits left to test\\n\") < 0)\n> +\t\treturn error_errno(_(\"failed to write to '%s'\"), git_path_bisect_log());\n> +\n> +\twhile ((commit = get_revision(revs)) != NULL) {\n> +\t\tstruct strbuf commit_name = STRBUF_INIT;\n\nI missed this in my review end of January: it is an anti-pattern to create\na new `strbuf` and releasing it right away in the same loop iteration.\nLet's instead move the declaration of `commit_name` to the top of the\nfunction, then call `strbuf_reset()` at the beginning of the loop, and\nfinally call `strbuf_release()` only _once_, at the end of the function.\n\n> +\t\tformat_commit_message(commit, \"%s\",\n> +\t\t\t\t &commit_name, &pp);\n> +\t\tfprintf(fp, \"# possible first %s commit: [%s] %s\\n\",\n> +\t\t\tterms->term_bad, oid_to_hex(&commit->object.oid),\n> +\t\t\tcommit_name.buf);\n> +\t\tstrbuf_release(&commit_name);\n> +\t}\n> +\n> +\t/*\n> +\t * Reset the flags used by revision walks in case\n> +\t * there is another revision walk after this one.\n> +\t */\n> +\treset_revision_walk();\n> +\n> +\treturn 0;\n> +}\n> +\n> +static int bisect_skipped_commits(struct bisect_terms *terms)\n> +{\n> +\tint res = 0;\n> +\tFILE *fp = NULL;\n> +\tstruct rev_info revs;\n> +\n> +\tfp = fopen(git_path_bisect_log(), \"a\");\n> +\tif (!fp)\n> +\t\treturn error_errno(_(\"could not open '%s' for appending\"),\n> +\t\t\t\t git_path_bisect_log());\n> +\n> +\tres = prepare_revs(terms, &revs);\n> +\n> +\tif (!res)\n> +\t\tres = process_skipped_commits(fp, terms, &revs);\n> +\n> +\tfclose(fp);\n> +\treturn res;\n\nIt would make the code easier to follow if `prepare_revs()` was called\nbefore opening the file and returning early in case of an error.\n\nFurther, I do not see the point of having the code in\n`process_skipped_commits()` there is only this caller, in a tiny function.\nIt would be better to fold the code into a single function.\n\nI did point that out already in my review at the end of this past January.\n\n> +}\n> +\n> +static int bisect_successful(struct bisect_terms *terms)\n> +{\n> +\tstruct object_id oid;\n> +\tstruct commit *commit;\n> +\tstruct pretty_print_context pp = {0};\n> +\tstruct strbuf commit_name = STRBUF_INIT;\n> +\tchar *bad_ref = xstrfmt(\"refs/bisect/%s\",terms->term_bad);\n> +\tint res;\n> +\n> +\tread_ref(bad_ref, &oid);\n> +\tprintf(\"%s\\n\", bad_ref);\n\nI missed this in January: is this a left-over debug printf? I cannot find\nthe equivalent in the shell script version.\n\n> +\tcommit = lookup_commit_reference(the_repository, &oid);\n\nThere is a relatively recent convention to initialize a local variable\n`struct repository *r = the_repository;` at the beginning of the function\nand then using `r` throughout the function. I think I'd like to use that\nhere, too, even if it is only used once.\n\nAny reason not to use `lookup_commit_reference_by_name(r, bad_ref)`\ndirectly instead? You can still reference `commit->object.oid` instead of\n`oid` later on.\n\n> +\tformat_commit_message(commit, \"%s\", &commit_name, &pp);\n> +\n> +\tres = write_in_file(git_path_bisect_log(), \"a\", \"# first %s commit: [%s] %s\\n\",\n> +\t\t\t terms->term_bad, oid_to_hex(&oid),\n> +\t\t\t commit_name.buf);\n> +\n> +\tstrbuf_release(&commit_name);\n> +\tfree(bad_ref);\n> +\treturn res;\n> +}\n> +\n> +static enum bisect_error bisect_next(struct bisect_terms *terms, const char *prefix)\n> +{\n> +\tint no_checkout;\n> +\tenum bisect_error res;\n> +\n> +\tbisect_autostart(terms);\n> +\tif (bisect_next_check(terms, terms->term_good))\n> +\t\treturn BISECT_FAILED;\n> +\n> +\tno_checkout = file_exists(git_path_bisect_head());\n> +\n> +\t/* Perform all bisection computation, display and checkout */\n> +\tres = bisect_next_all(the_repository, prefix, no_checkout);\n> +\n> +\tif (res == BISECT_INTERNAL_SUCCESS_1ST_BAD_FOUND) {\n> +\t\tres = bisect_successful(terms);\n> +\t\treturn res ? res : BISECT_INTERNAL_SUCCESS_1ST_BAD_FOUND;\n> +\t} else if (res == BISECT_ONLY_SKIPPED_LEFT) {\n> +\t\tres = bisect_skipped_commits(terms);\n> +\t\treturn res ? res : BISECT_ONLY_SKIPPED_LEFT;\n> +\t}\n\nThis looks much, much nicer than the equivalent code I reviewed almost\nfour months ago.\n\nPersonally, I would have used the idiom\n\n\t\tif (!res)\n\t\t\treturn BISECT_INTERNAL_SUCCESS_1ST_BAD_FOUND;\n\nbut I can live with the current version.\n\n> +\treturn res;\n> +}\n> +\n> +static enum bisect_error bisect_auto_next(struct bisect_terms *terms, const char *prefix)\n> +{\n> +\tif (bisect_next_check(terms, NULL))\n> +\t\treturn BISECT_OK;\n\nThis is a straight-forward translation from the shell code.\n\nI would like to take a step back, though: the only two differences between the\n`bisect_next_check()` call here and the one in `bisect_next()` are:\n\n- the return value if `bisect_next_check()` failed: the \"auto\" version\n seems to report success, the non-\"auto\" version failure\n\n- the \"auto\" version seems not to accept missing \"good\" commits\n\nSeeing this, it does strike me odd that `bisect_next()` is called from\n`bisect_auto_next()`, and I would have rather expected an `int auto`\nparameter to the `bisect_next()` function, where the `bisect_autostart()`\ncall is skipped in case `auto != 0`.\n\nThe rest of the patch looks fine to me.\n\nCiao,\nDscho\n\n> +\n> +\treturn bisect_next(terms, prefix);\n> +}\n> +\n> static int bisect_start(struct bisect_terms *terms, int no_checkout,\n> \t\t\tconst char **argv, int argc)\n> {\n> @@ -672,7 +819,9 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n> \t\tBISECT_NEXT_CHECK,\n> \t\tBISECT_TERMS,\n> \t\tBISECT_START,\n> -\t\tBISECT_AUTOSTART,\n> +\t\tBISECT_NEXT,\n> +\t\tBISECT_AUTO_NEXT,\n> +\t\tBISECT_AUTOSTART\n> \t} cmdmode = 0;\n> \tint no_checkout = 0, res = 0, nolog = 0;\n> \tstruct option options[] = {\n> @@ -696,6 +845,10 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n> \t\t\t N_(\"print out the bisect terms\"), BISECT_TERMS),\n> \t\tOPT_CMDMODE(0, \"bisect-start\", &cmdmode,\n> \t\t\t N_(\"start the bisect session\"), BISECT_START),\n> +\t\tOPT_CMDMODE(0, \"bisect-next\", &cmdmode,\n> +\t\t\t N_(\"find the next bisection commit\"), BISECT_NEXT),\n> +\t\tOPT_CMDMODE(0, \"bisect-auto-next\", &cmdmode,\n> +\t\t\t N_(\"verify the next bisection state then checkout the next bisection commit\"), BISECT_AUTO_NEXT),\n> \t\tOPT_CMDMODE(0, \"bisect-autostart\", &cmdmode,\n> \t\t\t N_(\"start the bisection if BISECT_START is empty or missing\"), BISECT_AUTOSTART),\n> \t\tOPT_BOOL(0, \"no-checkout\", &no_checkout,\n> @@ -759,6 +912,18 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n> \t\tset_terms(&terms, \"bad\", \"good\");\n> \t\tres = bisect_start(&terms, no_checkout, argv, argc);\n> \t\tbreak;\n> +\tcase BISECT_NEXT:\n> +\t\tif (argc)\n> +\t\t\treturn error(_(\"--bisect-next requires 0 arguments\"));\n> +\t\tget_terms(&terms);\n> +\t\tres = bisect_next(&terms, prefix);\n> +\t\tbreak;\n> +\tcase BISECT_AUTO_NEXT:\n> +\t\tif (argc)\n> +\t\t\treturn error(_(\"--bisect-auto-next requires 0 arguments\"));\n> +\t\tget_terms(&terms);\n> +\t\tres = bisect_auto_next(&terms, prefix);\n> +\t\tbreak;\n> \tcase BISECT_AUTOSTART:\n> \t\tif (argc)\n> \t\t\treturn error(_(\"--bisect-autostart requires 0 arguments\"));\n> @@ -774,7 +939,7 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n> \t * Handle early success\n> \t * From check_merge_bases > check_good_are_ancestors_of_bad > bisect_next_all\n> \t */\n> -\tif (res == BISECT_INTERNAL_SUCCESS_MERGE_BASE)\n> +\tif ((res == BISECT_INTERNAL_SUCCESS_MERGE_BASE) || (res == BISECT_INTERNAL_SUCCESS_1ST_BAD_FOUND))\n> \t\tres = BISECT_OK;\n>\n> \treturn -res;\n> diff --git a/git-bisect.sh b/git-bisect.sh\n> index 426d443e7e..897825b675 100755\n> --- a/git-bisect.sh\n> +++ b/git-bisect.sh\n> @@ -65,8 +65,7 @@ bisect_start() {\n> \t#\n> \t# Check if we can proceed to the next bisect state.\n> \t#\n> -\tget_terms\n> -\tbisect_auto_next\n> +\tgit bisect--helper --bisect-auto-next || exit\n>\n> \ttrap '-' 0\n> }\n> @@ -119,45 +118,7 @@ bisect_state() {\n> \t*)\n> \t\tusage ;;\n> \tesac\n> -\tbisect_auto_next\n> -}\n> -\n> -bisect_auto_next() {\n> -\tgit bisect--helper --bisect-next-check $TERM_GOOD $TERM_BAD && bisect_next || :\n> -}\n> -\n> -bisect_next() {\n> -\tcase \"$#\" in 0) ;; *) usage ;; esac\n> -\tgit bisect--helper --bisect-autostart\n> -\tgit bisect--helper --bisect-next-check $TERM_GOOD $TERM_BAD $TERM_GOOD|| exit\n> -\n> -\t# Perform all bisection computation, display and checkout\n> -\tgit bisect--helper --next-all $(test -f \"$GIT_DIR/BISECT_HEAD\" && echo --no-checkout)\n> -\tres=$?\n> -\n> -\t# Check if we should exit because bisection is finished\n> -\tif test $res -eq 10\n> -\tthen\n> -\t\tbad_rev=$(git show-ref --hash --verify refs/bisect/$TERM_BAD)\n> -\t\tbad_commit=$(git show-branch $bad_rev)\n> -\t\techo \"# first $TERM_BAD commit: $bad_commit\" >>\"$GIT_DIR/BISECT_LOG\"\n> -\t\texit 0\n> -\telif test $res -eq 2\n> -\tthen\n> -\t\techo \"# only skipped commits left to test\" >>\"$GIT_DIR/BISECT_LOG\"\n> -\t\tgood_revs=$(git for-each-ref --format=\"%(objectname)\" \"refs/bisect/$TERM_GOOD-*\")\n> -\t\tfor skipped in $(git rev-list refs/bisect/$TERM_BAD --not $good_revs)\n> -\t\tdo\n> -\t\t\tskipped_commit=$(git show-branch $skipped)\n> -\t\t\techo \"# possible first $TERM_BAD commit: $skipped_commit\" >>\"$GIT_DIR/BISECT_LOG\"\n> -\t\tdone\n> -\t\texit $res\n> -\tfi\n> -\n> -\t# Check for an error in the bisection process\n> -\ttest $res -ne 0 && exit $res\n> -\n> -\treturn 0\n> +\tgit bisect--helper --bisect-auto-next\n> }\n>\n> bisect_visualize() {\n> @@ -211,7 +172,7 @@ bisect_replay () {\n> \t\t\tdie \"$(gettext \"?? what are you talking about?\")\" ;;\n> \t\tesac\n> \tdone <\"$file\"\n> -\tbisect_auto_next\n> +\tgit bisect--helper --bisect-auto-next\n> }\n>\n> bisect_run () {\n> @@ -308,7 +269,7 @@ case \"$#\" in\n> \t\tbisect_skip \"$@\" ;;\n> \tnext)\n> \t\t# Not sure we want \"next\" at the UI level anymore.\n> -\t\tbisect_next \"$@\" ;;\n> +\t\tgit bisect--helper --bisect-next \"$@\" || exit ;;\n> \tvisualize|view)\n> \t\tbisect_visualize \"$@\" ;;\n> \treset)\n> --\n> 2.25.0\n>\n>\n"},{"id":"398438","messageId":"nycvar.QRO.7.76.6.2005222247500.56@tvgsbejvaqbjf.bet","threadId":"53293","inReplyTo":"nycvar.QRO.7.76.6.2005222058380.56@tvgsbejvaqbjf.bet","subject":"Re: [PATCH v3 04/12] bisect--helper: reimplement `bisect_autostart` shell function in C","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-05-22T20:50:37Z","receivedAt":"2020-05-22T20:50:43Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Miriam,\n\nOn Fri, 22 May 2020, Johannes Schindelin wrote:\n\n> On Thu, 23 Apr 2020, Miriam Rubio wrote:\n>\n> > +\t/*\n> > +\t * TRANSLATORS: Make sure to include [Y] and [n] in your\n> > +\t * translation. The program will only accept English input\n> > +\t * at this point.\n> > +\t */\n> > +\tyesno = git_prompt(_(\"Do you want me to do it for you \"\n> > +\t\t\t \"[Y/n]? \"), PROMPT_ECHO);\n>\n> [...]\n>\n> I still think that we need to call `free(yesno);` or we leak memory.\n\nNever mind. I got fooled by the fact that `git_prompt()` returns `char *`.\nIt actually returns the pointer `buf.buf` of a `static `strbuf buf`, so\nthere is nothing to `free()` here.\n\nSorry for the noise,\nDscho\n"},{"id":"398442","messageId":"nycvar.QRO.7.76.6.2005222256130.56@tvgsbejvaqbjf.bet","threadId":"53293","inReplyTo":"20200423070704.39872-7-mirucam@gmail.com","subject":"Re: [PATCH v3 06/12] bisect--helper: finish porting `bisect_start()` to C","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-05-22T21:08:12Z","receivedAt":"2020-05-22T21:08:20Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Miriam,\n\nOn Thu, 23 Apr 2020, Miriam Rubio wrote:\n\n> From: Pranit Bauva \n>\n> Add the subcommand to `git bisect--helper` and call it from\n> git-bisect.sh.\n>\n> With the conversion of `bisect_auto_next()` from shell to C in a\n> previous commit, `bisect_start()` can now be fully ported to C.\n>\n> So let's complete the `--bisect-start` subcommand of\n> `git bisect--helper` so that it fully implements `bisect_start()`,\n> and let's use this subcommand in `git-bisect.sh` instead of\n> `bisect_start()`.\n>\n> This removes the signal handling we had in `bisect_start()` as we\n> don't really need it. While at it, \"trap\" is changed to \"handle\".\n\nI do not see any \"handle\" in the diff...\n\n> As \"trap\" is a reference to the shell \"trap\" builtin, which isn't\n> used anymore.\n\nOtherwise, the commit message looks good to me!\n\n>\n> Mentored-by: Christian Couder \n> Mentored-by: Johannes Schindelin \n> Signed-off-by: Pranit Bauva \n> Signed-off-by: Tanushree Tumane \n> Signed-off-by: Miriam Rubio \n> ---\n> builtin/bisect--helper.c | 54 +++++++++++++++++++++++++++++++---------\n> git-bisect.sh | 26 ++-----------------\n> 2 files changed, 44 insertions(+), 36 deletions(-)\n>\n> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\n> index c6aaa8eb15..f2ec7f89e4 100644\n> --- a/builtin/bisect--helper.c\n> +++ b/builtin/bisect--helper.c\n> @@ -588,11 +588,12 @@ static enum bisect_error bisect_auto_next(struct bisect_terms *terms, const char\n> \treturn bisect_next(terms, prefix);\n> }\n>\n> -static int bisect_start(struct bisect_terms *terms, int no_checkout,\n> +static enum bisect_error bisect_start(struct bisect_terms *terms, int no_checkout,\n> \t\t\tconst char **argv, int argc)\n> {\n> \tint i, has_double_dash = 0, must_write_terms = 0, bad_seen = 0;\n> -\tint flags, pathspec_pos, res = 0;\n> +\tint flags, pathspec_pos;\n> +\tenum bisect_error res = BISECT_OK;\n> \tstruct string_list revs = STRING_LIST_INIT_DUP;\n> \tstruct string_list states = STRING_LIST_INIT_DUP;\n> \tstruct strbuf start_head = STRBUF_INIT;\n> @@ -645,9 +646,12 @@ static int bisect_start(struct bisect_terms *terms, int no_checkout,\n> \t\t\treturn error(_(\"unrecognized option: '%s'\"), arg);\n> \t\t} else {\n> \t\t\tchar *commit_id = xstrfmt(\"%s^{commit}\", arg);\n> -\t\t\tif (get_oid(commit_id, &oid) && has_double_dash)\n> -\t\t\t\tdie(_(\"'%s' does not appear to be a valid \"\n> -\t\t\t\t \"revision\"), arg);\n> +\t\t\tif (get_oid(commit_id, &oid) && has_double_dash) {\n> +\t\t\t\terror(_(\"'%s' does not appear to be a valid \"\n> +\t\t\t\t\t\"revision\"), arg);\n> +\t\t\t\tfree(commit_id);\n> +\t\t\t\treturn BISECT_FAILED;\n> +\t\t\t}\n\nMakes sense.\n\n>\n> \t\t\tstring_list_append(&revs, oid_to_hex(&oid));\n> \t\t\tfree(commit_id);\n> @@ -725,12 +729,12 @@ static int bisect_start(struct bisect_terms *terms, int no_checkout,\n> \t * Get rid of any old bisect state.\n> \t */\n> \tif (bisect_clean_state())\n> -\t\treturn -1;\n> +\t\treturn BISECT_FAILED;\n>\n> \t/*\n> -\t * In case of mistaken revs or checkout error, or signals received,\n> +\t * In case of mistaken revs or checkout error,\n> \t * \"bisect_auto_next\" below may exit or misbehave.\n> -\t * We have to trap this to be able to clean up using\n> +\t * We have to handle this to be able to clean up using\n> \t * \"bisect_clean_state\".\n> \t */\n>\n> @@ -746,7 +750,7 @@ static int bisect_start(struct bisect_terms *terms, int no_checkout,\n> \t\t}\n> \t\tif (update_ref(NULL, \"BISECT_HEAD\", &oid, NULL, 0,\n> \t\t\t UPDATE_REFS_MSG_ON_ERR)) {\n> -\t\t\tres = -1;\n> +\t\t\tres = BISECT_FAILED;\n> \t\t\tgoto finish;\n> \t\t}\n> \t}\n> @@ -758,25 +762,51 @@ static int bisect_start(struct bisect_terms *terms, int no_checkout,\n> \tfor (i = 0; i < states.nr; i++)\n> \t\tif (bisect_write(states.items[i].string,\n> \t\t\t\t revs.items[i].string, terms, 1)) {\n> -\t\t\tres = -1;\n> +\t\t\tres = BISECT_FAILED;\n> \t\t\tgoto finish;\n> \t\t}\n>\n> \tif (must_write_terms && write_terms(terms->term_bad,\n> \t\t\t\t\t terms->term_good)) {\n> -\t\tres = -1;\n> +\t\tres = BISECT_FAILED;\n> \t\tgoto finish;\n> \t}\n>\n> \tres = bisect_append_log_quoted(argv);\n> \tif (res)\n> -\t\tres = -1;\n> +\t\tres = BISECT_FAILED;\n>\n> finish:\n> \tstring_list_clear(&revs, 0);\n> \tstring_list_clear(&states, 0);\n> \tstrbuf_release(&start_head);\n> \tstrbuf_release(&bisect_names);\n> +\tif (res)\n> +\t\treturn res;\n> +\n> +\tres = bisect_auto_next(terms, NULL);\n> +\t/*\n> +\t * In case of mistaken revs or checkout error, or signals received,\n> +\t * \"bisect_auto_next\" below may exit or misbehave.\n\nThis \"below\" was true in the shell script version, but now it should be\n\"above\".\n\n> +\t * We have to handle this to be able to clean up using\n> +\t * \"bisect_clean_state\".\n> +\t * return code BISECT_INTERNAL_SUCCESS_MERGE_BASE is special code\n> +\t * that indicates special success.\n\nMaybe we should have a helper function and use it here:\n\n\tstatic int is_bisect_success(enum bisect_error res)\n\t{\n\t\treturn !res ||\n\t\t\tres == BISECT_INTERNAL_1ST_BAD_FOUND ||\n\t\t\tres == BISECT_INTERNAL_SUCCESS_MERGE_BASE;\n\t}\n\n(Note that I added the `_1ST_BAD_FOUND` case here.)\n\n> +\t *\t-> bisect_start()\n> +\t *\t . res = bisect_auto_next()\n> +\t *\t -> bisect_auto_next()\n> +\t *\t . return bisect_next()\n> +\t *\t -> bisect_next()\n> +\t *\t\t . res = bisect_next_all()\n> +\t *\t\t -> bisect_next_all()\n> +\t *\t\t . res = check_good_are_ancestors_of_bad()\n> +\t *\t\t -> check_good_are_ancestors_of_bad()\n> +\t *\t\t\t. res = check_merge_bases()\n> +\t *\t\t\t-> check_merge_bases()\n> +\t *\t\t\t . res = BISECT_INTERNAL_SUCCESS_MERGE_BASE\n\nWhile this might be an interesting tidbit for the commit message, I do not\nthink that it is a good idea to keep this as a code comment; It is prone\nto become stale/incorrect over time.\n\n> +\t */\n> +\tif (res && res != BISECT_INTERNAL_SUCCESS_MERGE_BASE)\n> +\t\tbisect_clean_state();\n> \treturn res;\n> }\n>\n> diff --git a/git-bisect.sh b/git-bisect.sh\n> index 897825b675..049ffacdff 100755\n> --- a/git-bisect.sh\n> +++ b/git-bisect.sh\n> @@ -49,27 +49,6 @@ bisect_head()\n> \tfi\n> }\n>\n> -bisect_start() {\n> -\tgit bisect--helper --bisect-start $@ || exit\n> -\n> -\t#\n> -\t# Change state.\n> -\t# In case of mistaken revs or checkout error, or signals received,\n> -\t# \"bisect_auto_next\" below may exit or misbehave.\n> -\t# We have to trap this to be able to clean up using\n> -\t# \"bisect_clean_state\".\n> -\t#\n> -\ttrap 'git bisect--helper --bisect-clean-state' 0\n> -\ttrap 'exit 255' 1 2 3 15\n> -\n> -\t#\n> -\t# Check if we can proceed to the next bisect state.\n> -\t#\n> -\tgit bisect--helper --bisect-auto-next || exit\n> -\n> -\ttrap '-' 0\n> -}\n> -\n> bisect_skip() {\n> \tall=''\n> \tfor arg in \"$@\"\n> @@ -162,8 +141,7 @@ bisect_replay () {\n> \t\tget_terms\n> \t\tcase \"$command\" in\n> \t\tstart)\n> -\t\t\tcmd=\"bisect_start $rev\"\n> -\t\t\teval \"$cmd\" ;;\n> +\t\t\teval \"git bisect--helper --bisect-start $rev\" ;;\n\nThe original code _had_ to use `eval` because `bisect_start` could cause\nthe code to `exit` and we did _not_ want to let that exit the `replay`\nprocess.\n\nHowever, we are calling `git bisect--helper` here, which runs in its own\nprocess anyway, so the `eval` should be dropped now.\n\nOtherwise, the patch looks fine to me,\nDscho\n\n> \t\t\"$TERM_GOOD\"|\"$TERM_BAD\"|skip)\n> \t\t\tgit bisect--helper --bisect-write \"$command\" \"$rev\" \"$TERM_GOOD\" \"$TERM_BAD\" || exit;;\n> \t\tterms)\n> @@ -262,7 +240,7 @@ case \"$#\" in\n> \thelp)\n> \t\tgit bisect -h ;;\n> \tstart)\n> -\t\tbisect_start \"$@\" ;;\n> +\t\tgit bisect--helper --bisect-start \"$@\" ;;\n> \tbad|good|new|old|\"$TERM_BAD\"|\"$TERM_GOOD\")\n> \t\tbisect_state \"$cmd\" \"$@\" ;;\n> \tskip)\n> --\n> 2.25.0\n>\n>\n"},{"id":"398444","messageId":"nycvar.QRO.7.76.6.2005222311120.56@tvgsbejvaqbjf.bet","threadId":"53293","inReplyTo":"20200423070704.39872-10-mirucam@gmail.com","subject":"Re: [PATCH v3 09/12] bisect--helper: reimplement `bisect_state` & `bisect_head` shell functions in C","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-05-22T22:06:54Z","receivedAt":"2020-05-22T22:07:04Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Miriam,\n\nOn Thu, 23 Apr 2020, Miriam Rubio wrote:\n\n> From: Pranit Bauva \n>\n> Reimplement the `bisect_state()` shell functions in C and also add a\n> subcommand `--bisect-state` to `git-bisect--helper` to call them from\n> git-bisect.sh .\n>\n> Using `--bisect-state` subcommand is a temporary measure to port shell\n> function to C so as to use the existing test suite. As more functions\n> are ported, this subcommand will be retired and will be called by some\n> other methods.\n>\n> `bisect_head()` is only called from `bisect_state()`, thus it is not\n> required to introduce another subcommand.\n>\n> Mentored-by: Lars Schneider \n> Mentored-by: Christian Couder \n> Mentored-by: Johannes Schindelin \n> Signed-off-by: Pranit Bauva \n> Signed-off-by: Tanushree Tumane \n> Signed-off-by: Miriam Rubio \n> ---\n> builtin/bisect--helper.c | 70 +++++++++++++++++++++++++++++++++++++++-\n> git-bisect.sh | 55 +++----------------------------\n> 2 files changed, 73 insertions(+), 52 deletions(-)\n>\n> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\n> index 2d8660c79f..9db72f5891 100644\n> --- a/builtin/bisect--helper.c\n> +++ b/builtin/bisect--helper.c\n> @@ -31,6 +31,8 @@ static const char * const git_bisect_helper_usage[] = {\n> \tN_(\"git bisect--helper --bisect-next\"),\n> \tN_(\"git bisect--helper --bisect-auto-next\"),\n> \tN_(\"git bisect--helper --bisect-autostart\"),\n> +\tN_(\"git bisect--helper --bisect-state (bad|new) []\"),\n> +\tN_(\"git bisect--helper --bisect-state (good|old) [...]\"),\n> \tNULL\n> };\n>\n> @@ -834,6 +836,64 @@ static int bisect_autostart(struct bisect_terms *terms)\n> \treturn bisect_start(terms, 0, NULL, 0);\n> }\n>\n> +static int bisect_head(struct object_id *oid)\n> +{\n> +\tif (!file_exists(git_path_bisect_head()))\n> +\t\treturn get_oid(\"HEAD\", oid);\n> +\n> +\treturn get_oid(\"BISECT_HEAD\", oid);\n\nThis can be easily reduced to\n\n\treturn get_oid(file_exists(git_path_bisect_head()) ?\n\t\t \"BISECT_HEAD\" : \"HEAD\", oid);\n\nAt the same time, it is wrong, just like the shell script version was\nwrong: in particular in light of the `hn/reftable` effort, we do _not_\nwant to assume that all refs are backed by files!\n\nSo really, what this should do instead is this:\n\n\tenum get_oid_result res = get_oid(\"BISECT_HEAD\", oid);\n\n\tif (res == MISSING_OBJECT)\n\t\tres = get_oid(\"HEAD\", oid);\n\nGiven that this is still only three lines long, the overhead of having it\nin its own function for just a _single_ call seems excessive. I'd prefer\nit to be inlined in `bisect_state()`.\n\n> +}\n> +\n> +static enum bisect_error bisect_state(struct bisect_terms *terms, const char **argv,\n> +\t\t\tint argc)\n> +{\n\nI offered a lengthy discussion about this function in\nhttps://lore.kernel.org/git/nycvar.QRO.7.76.6.2002272244150.9783@tvgsbejvaqbjf.bet/\n\nIt does not look, however, as if v3 benefitted from the entirety of my\nanalysis: All the `check_expected_revs()` function does is to verify that\nthe passed list of revs matches exactly the contents of the\n`BISECT_EXPECTED_REV` file.\n\nThat can be done in a much simpler way, though, by first reading the file\nand parsing the contents into an OID, and then comparing to that parsed\nOID instead.\n\nBesides, `check_expected_revs()` is only used to check one rev at a time.\n\nIn other words, it could be simplified to something like this:\n\nstatic void check_expected_rev(struct object_id *oid)\n{\n\tstruct object_id expected;\n\tstruct strbuf buf = STRBUF_INIT;\n\n if (strbuf_read_file(&buf, git_path_bisect_expected_rev(), 0)\n\t < the_hash_algo->hexsz ||\n\t get_oid_hex(buf.buf, &expected) < 0)\n\t\treturn; /* Ignore invalid file contents */\n\n\tif (!oideq(oid, &expected)) {\n\t\t... unlink ...\n\t\treturn;\n\t}\n}\n\nBut even that would be wasteful, as we would read the file over and over\nand over again.\n\nThe good news is that we do not even _need_ `check_expected_rev()`.\nBecause we do not need to have two call sites, we can simplify the code\nmuch further. See below:\n\n> +\tconst char *state;\n> +\tconst char *hex;\n> +\tint i;\n> +\tstruct oid_array revs = OID_ARRAY_INIT;\n> +\tstruct object_id oid;\n> +\n> +\tif (!argc)\n> +\t\treturn error(_(\"Please call `--bisect-state` with at least one argument\"));\n> +\tstate = argv[0];\n> +\tif (check_and_set_terms(terms, state) ||\n> +\t !one_of(state, terms->term_good,terms->term_bad, \"skip\", NULL))\n> +\t\treturn BISECT_FAILED;\n> +\targv++;\n> +\targc--;\n> +\tif (!strcmp(state, terms->term_bad) && (argc > 1))\n> +\t\treturn error(_(\"'git bisect %s' can take only one argument.\"),terms->term_bad);\n> +\tif (argc == 0) {\n> +\t\tif (bisect_head(&oid))\n> +\t\t\treturn error(_(\"Bad bisect_head rev input\"));\n> +\t\thex = oid_to_hex(&oid);\n> +\t\tif (bisect_write(state, hex, terms, 0))\n> +\t\t\treturn BISECT_FAILED;\n> +\t\tcheck_expected_revs(&hex, 1);\n> +\t\treturn bisect_auto_next(terms, NULL);\n> +\t}\n> +\n> +\t/* Here argc > 0 */\n> +\tfor (; argc; argc--, argv++) {\n> +\t\tstruct object_id oid;\n> +\t\tif (get_oid(*argv, &oid))\n> +\t\t\treturn error(_(\"Bad rev input: %s\"), *argv);\n> +\t\toid_array_append(&revs, &oid);\n> +\t}\n\nIt really does not make sense to parse the arguments into an OID array,\n_then_ iterate over the array once, and then immediately releasing it.\nThat OID array is not needed at all.\n\nSo we'll end up with this loop in case `argc > 0` (where we now call\n`get_oid()`, too), and note how the loop body looks _eerily_ similar to\nthe conditional `argc == 0` code block above?\n\n> +\n> +\tfor (i = 0; i < revs.nr; i++) {\n> +\t\thex = oid_to_hex(&revs.oid[i]);\n> +\t\tif (bisect_write(state, hex, terms, 0)) {\n> +\t\t\toid_array_clear(&revs);\n> +\t\t\treturn BISECT_FAILED;\n> +\t\t}\n> +\t\tcheck_expected_revs(&hex, 1);\n> +\t}\n> +\n> +\toid_array_clear(&revs);\n> +\treturn bisect_auto_next(terms, NULL);\n> +}\n\nSo really, this function pretty much _wants_ to look this way (modulo\nbugs, as I did not even test-compile the code):\n\nstatic enum bisect_error bisect_state(struct bisect_terms *terms,\n\t\t\t\t const char **argv, int argc)\n{\n\tconst char *state;\n\tint i, verify_expected = 1;\n\tstruct object_id oid, expected;\n\tstruct strbuf buf = STRBUF_INIT;\n\n\tif (!argc)\n\t\treturn error(_(\"Please call `--bisect-state` with at least one argument\"));\n\n\tstate = argv[0];\n\tif (check_and_set_terms(terms, state) ||\n\t !one_of(state, terms->term_good, terms->term_bad, \"skip\", NULL))\n\t\treturn BISECT_FAILED;\n\n\targv++;\n\targc--;\n\tif (argc > 1 && !strcmp(state, terms->term_bad))\n\t\treturn error(_(\"'git bisect %s' can take only one argument.\"), terms->term_bad);\n\n if (strbuf_read_file(&buf, git_path_bisect_expected_rev(), 0) < the_hash_algo->hexsz ||\n\t get_oid_hex(buf.buf, &expected) < 0)\n\t\tverify_expected = 0; /* Ignore invalid file contents */\n\n\n\tfor (i = 0; i < argc + !argc; i++) {\n\t\tif (argc) {\n\t\t\tif (get_oid(argv[i], &oid)) {\n\t\t\t\terror(_(\"Bad rev input: %s\"), *argv);\n\t\t\t\treturn BISECT_FAILED;\n\t\t\t}\n\t\t} else {\n\t\t\tenum get_oid_result res = get_oid(\"BISECT_HEAD\", &oid);\n\n\t\t\tif (res == MISSING_OBJECT)\n\t\t\t\tres = get_oid(\"HEAD\", &oid);\n\t\t\tif (res) {\n\t\t\t\terror(_(\"Bad bisect_head rev input\"));\n\t\t\t\treturn BISECT_FAILED;\n\t\t\t}\n\t\t}\n\n\t\tif (bisect_write(state, oid_to_hex(&oid), terms, 0))\n\t\t\treturn BISECT_FAILED;\n\n\t\tif (verify_expected && !oideq(&oid, &expected)) {\n\t\t\tunlink_or_warn(git_path_bisect_ancestors_ok());\n\t\t\tunlink_or_warn(git_path_bisect_expected_rev());\n\t\t\tverify_expected = 0;\n\t\t}\n\t}\n\n\treturn bisect_auto_next(terms, NULL);\n}\n\nThere, not bad, is it?\n\n> +\n> int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n> {\n> \tenum {\n> @@ -847,7 +907,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n> \t\tBISECT_START,\n> \t\tBISECT_NEXT,\n> \t\tBISECT_AUTO_NEXT,\n> -\t\tBISECT_AUTOSTART\n> +\t\tBISECT_AUTOSTART,\n> +\t\tBISECT_STATE\n> \t} cmdmode = 0;\n> \tint no_checkout = 0, res = 0, nolog = 0;\n> \tstruct option options[] = {\n> @@ -873,6 +934,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n> \t\t\t N_(\"verify the next bisection state then checkout the next bisection commit\"), BISECT_AUTO_NEXT),\n> \t\tOPT_CMDMODE(0, \"bisect-autostart\", &cmdmode,\n> \t\t\t N_(\"start the bisection if BISECT_START is empty or missing\"), BISECT_AUTOSTART),\n> +\t\tOPT_CMDMODE(0, \"bisect-state\", &cmdmode,\n> +\t\t\t N_(\"mark the state of ref (or refs)\"), BISECT_STATE),\n> \t\tOPT_BOOL(0, \"no-checkout\", &no_checkout,\n> \t\t\t N_(\"update BISECT_HEAD instead of checking out the current commit\")),\n> \t\tOPT_BOOL(0, \"no-log\", &nolog,\n> @@ -945,6 +1008,11 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n> \t\tset_terms(&terms, \"bad\", \"good\");\n> \t\tres = bisect_autostart(&terms);\n> \t\tbreak;\n> +\tcase BISECT_STATE:\n> +\t\tset_terms(&terms, \"bad\", \"good\");\n> +\t\tget_terms(&terms);\n> +\t\tres = bisect_state(&terms, argv, argc);\n> +\t\tbreak;\n> \tdefault:\n> \t\tBUG(\"unknown subcommand %d\", (int)cmdmode);\n> \t}\n> diff --git a/git-bisect.sh b/git-bisect.sh\n> index 049ffacdff..2da0810b1a 100755\n> --- a/git-bisect.sh\n> +++ b/git-bisect.sh\n> @@ -39,16 +39,6 @@ _x40=\"$_x40$_x40$_x40$_x40$_x40$_x40$_x40$_x40\"\n> TERM_BAD=bad\n> TERM_GOOD=good\n>\n> -bisect_head()\n> -{\n> -\tif test -f \"$GIT_DIR/BISECT_HEAD\"\n> -\tthen\n> -\t\techo BISECT_HEAD\n> -\telse\n> -\t\techo HEAD\n> -\tfi\n> -}\n> -\n> bisect_skip() {\n> \tall=''\n> \tfor arg in \"$@\"\n> @@ -61,43 +51,7 @@ bisect_skip() {\n> \t\tesac\n> \t\tall=\"$all $revs\"\n> \tdone\n> -\teval bisect_state 'skip' $all\n> -}\n> -\n> -bisect_state() {\n> -\tgit bisect--helper --bisect-autostart\n> -\tstate=$1\n> -\tgit bisect--helper --check-and-set-terms $state $TERM_GOOD $TERM_BAD || exit\n> -\tget_terms\n> -\tcase \"$#,$state\" in\n> -\t0,*)\n> -\t\tdie \"Please call 'bisect_state' with at least one argument.\" ;;\n> -\t1,\"$TERM_BAD\"|1,\"$TERM_GOOD\"|1,skip)\n> -\t\tbisected_head=$(bisect_head)\n> -\t\trev=$(git rev-parse --verify \"$bisected_head\") ||\n> -\t\t\tdie \"$(eval_gettext \"Bad rev input: \\$bisected_head\")\"\n> -\t\tgit bisect--helper --bisect-write \"$state\" \"$rev\" \"$TERM_GOOD\" \"$TERM_BAD\" || exit\n> -\t\tgit bisect--helper --check-expected-revs \"$rev\" ;;\n> -\t2,\"$TERM_BAD\"|*,\"$TERM_GOOD\"|*,skip)\n> -\t\tshift\n> -\t\thash_list=''\n> -\t\tfor rev in \"$@\"\n> -\t\tdo\n> -\t\t\tsha=$(git rev-parse --verify \"$rev^{commit}\") ||\n> -\t\t\t\tdie \"$(eval_gettext \"Bad rev input: \\$rev\")\"\n> -\t\t\thash_list=\"$hash_list $sha\"\n> -\t\tdone\n> -\t\tfor rev in $hash_list\n> -\t\tdo\n> -\t\t\tgit bisect--helper --bisect-write \"$state\" \"$rev\" \"$TERM_GOOD\" \"$TERM_BAD\" || exit\n> -\t\tdone\n> -\t\tgit bisect--helper --check-expected-revs $hash_list ;;\n> -\t*,\"$TERM_BAD\")\n> -\t\tdie \"$(eval_gettext \"'git bisect \\$TERM_BAD' can take only one argument.\")\" ;;\n> -\t*)\n> -\t\tusage ;;\n> -\tesac\n> -\tgit bisect--helper --bisect-auto-next\n> +\teval git bisect--helper --bisect-state 'skip' $all\n> }\n>\n> bisect_visualize() {\n> @@ -185,8 +139,7 @@ exit code \\$res from '\\$command' is < 0 or >= 128\" >&2\n> \t\t\tstate=\"$TERM_GOOD\"\n> \t\tfi\n>\n> -\t\t# We have to use a subshell because \"bisect_state\" can exit.\n> -\t\t( bisect_state $state >\"$GIT_DIR/BISECT_RUN\" )\n> +\t\tgit bisect--helper --bisect-state $state >\"$GIT_DIR/BISECT_RUN\"\n> \t\tres=$?\n>\n> \t\tcat \"$GIT_DIR/BISECT_RUN\"\n> @@ -201,7 +154,7 @@ exit code \\$res from '\\$command' is < 0 or >= 128\" >&2\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> +'git bisect--helper --bisect-state \\$state' exited with error code \\$res\" >&2\n\nThis is not your fault, of course, but it does make me shudder to see such\nan obvious implementation detail in a user-facing error message.\n\nMaybe something to fix up in a follow-up?\n\nCiao,\nDscho\n\n> \t\t\texit $res\n> \t\tfi\n>\n> @@ -242,7 +195,7 @@ case \"$#\" in\n> \tstart)\n> \t\tgit bisect--helper --bisect-start \"$@\" ;;\n> \tbad|good|new|old|\"$TERM_BAD\"|\"$TERM_GOOD\")\n> -\t\tbisect_state \"$cmd\" \"$@\" ;;\n> +\t\tgit bisect--helper --bisect-state \"$cmd\" \"$@\" ;;\n> \tskip)\n> \t\tbisect_skip \"$@\" ;;\n> \tnext)\n> --\n> 2.25.0\n>\n>\n"},{"id":"398445","messageId":"nycvar.QRO.7.76.6.2005230007260.56@tvgsbejvaqbjf.bet","threadId":"53293","inReplyTo":"20200423070704.39872-1-mirucam@gmail.com","subject":"Re: [PATCH v3 00/12] Finish converting git bisect to C part 2","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-05-22T22:09:03Z","receivedAt":"2020-05-22T22:09:10Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Miriam,\n\nOn Thu, 23 Apr 2020, Miriam Rubio wrote:\n\n> These patches correspond to a second part of patch series\n> of Outreachy project \"Finish converting `git bisect` from shell to C\"\n> started by Pranit Bauva and Tanushree Tumane\n> (https://public-inbox.org/git/pull.117.git.gitgitgadget@gmail.com) and\n> continued by me.\n>\n> These patch series emails were generated from:\n> https://gitlab.com/mirucam/git/commits/git-bisect-work-part2-v3.\n>\n> I would like to thank Junio Hamano for reviewing this patch series and\n> Christian Couder for his help.\n>\n> --- Changes since v2 Finish converting git bisect to C part 2 patch series ---\n\nThank you for this detailed summary!\n\nI reviewed the patches in their entirety, and left a couple of\nsuggestions, some of them minor.\n\nHopefully you find them helpful!\n\nCiao,\nDscho\n\n>\n> General changes\n> ---------------\n>\n> * Rebase on master branch: efe3874640 (Sync with v2.26.1, 2020-04-13)\n>\n> Specific changes\n> ----------------\n>\n> [1/12] bisect--helper: fix `cmd_*()` function switch default return\n>\n> * Use `BUG()` instead of `return error()` in default switch.\n>\n> --\n>\n> [2/12] bisect--helper: use '-res' in 'cmd_bisect__helper' return\n>\n> * New patch: use '-res' instead of 'abs(res)'.\n>\n> --\n>\n> [3/12] bisect--helper: introduce new `write_in_file()` function\n>\n> * Rename input parameter `filepath` to `path`.\n> * Change `error_errno()` to `error()` in mode checking.\n> * Change error message when file cannot be opened.\n> * Add `fclose()` before error return.\n>\n> --\n>\n> [4/12] bisect--helper: reimplement `bisect_autostart` shell function in C\n>\n> * Reorder patch before `reimplement `bisect_next` and `bisect_auto_next`\n> shell functions in C` to use `bisect_autostart()` function in\n> `bisect_append_log_quoted()`.\n>\n> --\n> `\n> [5/12] bisect--helper: reimplement `bisect_next` and `bisect_auto_next` shell\n> functions in C\n>\n> * Amend commit message.\n> * Add `clear_commit_marks()` at the end of bisect_next_all() and remove it from\n> `bisect_rev_setup()`.\n> * Fix if condition.\n> * Add `bisect_autostart()` in `bisect_append_log_quoted()`.\n> * Check `git_path_bisect_head()` with `file_exist()` instead of\n> `is_empty_or_missing_file()`.\n> * Fix return with BISECT_INTERNAL_SUCCESS_1ST_BAD_FOUND.\n> * Add conversion in `cmd_*()` to BISECT_OK when return is\n> BISECT_INTERNAL_SUCCESS_1ST_BAD_FOUND.\n>\n> * Note to previous reviewer: suggestion to change `bisect_auto_next()` function\n> to return void cannot be done because errors from `bisect_next()` are not\n> ignored.\n>\n> --\n>\n> [5/12] bisect--helper: reimplement `bisect_state` & `bisect_head` shell\n> functions in C\n>\n> * Check `git_path_bisect_head()` with `file_exist()` instead of\n> `is_empty_or_missing_file()`.\n>\n> --\n>\n> Miriam Rubio (3):\n> bisect--helper: fix `cmd_*()` function switch default return\n> bisect--helper: use '-res' in 'cmd_bisect__helper' return\n> bisect--helper: introduce new `write_in_file()` function\n>\n> Pranit Bauva (9):\n> bisect--helper: reimplement `bisect_autostart` shell function in C\n> bisect--helper: reimplement `bisect_next` and `bisect_auto_next` shell\n> functions in C\n> bisect--helper: finish porting `bisect_start()` to C\n> bisect--helper: retire `--bisect-clean-state` subcommand\n> bisect--helper: retire `--next-all` subcommand\n> bisect--helper: reimplement `bisect_state` & `bisect_head` shell\n> functions in C\n> bisect--helper: retire `--check-expected-revs` subcommand\n> bisect--helper: retire `--write-terms` subcommand\n> bisect--helper: retire `--bisect-autostart` subcommand\n>\n> bisect.c | 8 +\n> builtin/bisect--helper.c | 383 +++++++++++++++++++++++++++++++++------\n> git-bisect.sh | 145 +--------------\n> 3 files changed, 347 insertions(+), 189 deletions(-)\n>\n> --\n> 2.25.0\n>\n>\n"},{"id":"398447","messageId":"20200523015331.GD3283@danh.dev","threadId":"53293","inReplyTo":"20200423070704.39872-4-mirucam@gmail.com","subject":"Re: [PATCH v3 03/12] bisect--helper: introduce new `write_in_file()` function","fromName":"Đoàn Trần Công Danh","fromEmail":"congdanhqx@gmail.com","sentAt":"2020-05-23T01:53:31Z","receivedAt":"2020-05-23T01:53:37Z","isPatch":true,"sender":{"key":"congdanhqx@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42673067?v=4"},"body":"On 2020-04-23 09:06:55+0200, Miriam Rubio wrote:\n> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\n> index 0fbd924aac..d3b2b33df0 100644\n> --- a/builtin/bisect--helper.c\n> +++ b/builtin/bisect--helper.c\n> @@ -74,6 +74,28 @@ static int one_of(const char *term, ...)\n> \treturn res;\n> }\n> \n> +static int write_in_file(const char *path, const char *mode, const char *format,...)\n\nNit: It looks like we want to add a space in the end \", ...)\"\nSince \"one_of\" above also has that space.\n\nOtherwise, look good.\n\n-- \nDanh\n"},{"id":"398476","messageId":"CAN7CjDD1v+0Rxc-+r=8zusknTgkfR=5ese8cXmWVsoXxt3A9Fw@mail.gmail.com","threadId":"53293","inReplyTo":"nycvar.QRO.7.76.6.2005230007260.56@tvgsbejvaqbjf.bet","subject":"Re: [PATCH v3 00/12] Finish converting git bisect to C part 2","fromName":"Miriam R.","fromEmail":"mirucam@gmail.com","sentAt":"2020-05-24T21:19:42Z","receivedAt":"2020-05-24T21:19:56Z","isPatch":true,"sender":{"key":"mirucam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56339109?v=4"},"body":"Hi Johannes,\n\nEl sáb., 23 may. 2020 a las 0:09, Johannes Schindelin\n() escribió:\n>\n> Hi Miriam,\n>\n> On Thu, 23 Apr 2020, Miriam Rubio wrote:\n>\n> > These patches correspond to a second part of patch series\n> > of Outreachy project \"Finish converting `git bisect` from shell to C\"\n> > started by Pranit Bauva and Tanushree Tumane\n> > (https://public-inbox.org/git/pull.117.git.gitgitgadget@gmail.com) and\n> > continued by me.\n> >\n> > These patch series emails were generated from:\n> > https://gitlab.com/mirucam/git/commits/git-bisect-work-part2-v3.\n> >\n> > I would like to thank Junio Hamano for reviewing this patch series and\n> > Christian Couder for his help.\n> >\n> > --- Changes since v2 Finish converting git bisect to C part 2 patch series ---\n>\n> Thank you for this detailed summary!\n>\n> I reviewed the patches in their entirety, and left a couple of\n> suggestions, some of them minor.\n>\nThank you very much for reviewing.\n\n> Hopefully you find them helpful!\nOf course! :)\n\nI hope I can send the next version soon.\n\nBest,\nMiriam\n>\n> Ciao,\n> Dscho\n>\n> >\n> > General changes\n> > ---------------\n> >\n> > * Rebase on master branch: efe3874640 (Sync with v2.26.1, 2020-04-13)\n> >\n> > Specific changes\n> > ----------------\n> >\n> > [1/12] bisect--helper: fix `cmd_*()` function switch default return\n> >\n> > * Use `BUG()` instead of `return error()` in default switch.\n> >\n> > --\n> >\n> > [2/12] bisect--helper: use '-res' in 'cmd_bisect__helper' return\n> >\n> > * New patch: use '-res' instead of 'abs(res)'.\n> >\n> > --\n> >\n> > [3/12] bisect--helper: introduce new `write_in_file()` function\n> >\n> > * Rename input parameter `filepath` to `path`.\n> > * Change `error_errno()` to `error()` in mode checking.\n> > * Change error message when file cannot be opened.\n> > * Add `fclose()` before error return.\n> >\n> > --\n> >\n> > [4/12] bisect--helper: reimplement `bisect_autostart` shell function in C\n> >\n> > * Reorder patch before `reimplement `bisect_next` and `bisect_auto_next`\n> > shell functions in C` to use `bisect_autostart()` function in\n> > `bisect_append_log_quoted()`.\n> >\n> > --\n> > `\n> > [5/12] bisect--helper: reimplement `bisect_next` and `bisect_auto_next` shell\n> > functions in C\n> >\n> > * Amend commit message.\n> > * Add `clear_commit_marks()` at the end of bisect_next_all() and remove it from\n> > `bisect_rev_setup()`.\n> > * Fix if condition.\n> > * Add `bisect_autostart()` in `bisect_append_log_quoted()`.\n> > * Check `git_path_bisect_head()` with `file_exist()` instead of\n> > `is_empty_or_missing_file()`.\n> > * Fix return with BISECT_INTERNAL_SUCCESS_1ST_BAD_FOUND.\n> > * Add conversion in `cmd_*()` to BISECT_OK when return is\n> > BISECT_INTERNAL_SUCCESS_1ST_BAD_FOUND.\n> >\n> > * Note to previous reviewer: suggestion to change `bisect_auto_next()` function\n> > to return void cannot be done because errors from `bisect_next()` are not\n> > ignored.\n> >\n> > --\n> >\n> > [5/12] bisect--helper: reimplement `bisect_state` & `bisect_head` shell\n> > functions in C\n> >\n> > * Check `git_path_bisect_head()` with `file_exist()` instead of\n> > `is_empty_or_missing_file()`.\n> >\n> > --\n> >\n> > Miriam Rubio (3):\n> > bisect--helper: fix `cmd_*()` function switch default return\n> > bisect--helper: use '-res' in 'cmd_bisect__helper' return\n> > bisect--helper: introduce new `write_in_file()` function\n> >\n> > Pranit Bauva (9):\n> > bisect--helper: reimplement `bisect_autostart` shell function in C\n> > bisect--helper: reimplement `bisect_next` and `bisect_auto_next` shell\n> > functions in C\n> > bisect--helper: finish porting `bisect_start()` to C\n> > bisect--helper: retire `--bisect-clean-state` subcommand\n> > bisect--helper: retire `--next-all` subcommand\n> > bisect--helper: reimplement `bisect_state` & `bisect_head` shell\n> > functions in C\n> > bisect--helper: retire `--check-expected-revs` subcommand\n> > bisect--helper: retire `--write-terms` subcommand\n> > bisect--helper: retire `--bisect-autostart` subcommand\n> >\n> > bisect.c | 8 +\n> > builtin/bisect--helper.c | 383 +++++++++++++++++++++++++++++++++------\n> > git-bisect.sh | 145 +--------------\n> > 3 files changed, 347 insertions(+), 189 deletions(-)\n> >\n> > --\n> > 2.25.0\n> >\n> >\n"},{"id":"400251","messageId":"CAN7CjDAnksyerW_sEDLFS=GT0g7rLt53dUA09a8jAqnZb9P_8w@mail.gmail.com","threadId":"53293","inReplyTo":"nycvar.QRO.7.76.6.2005222311120.56@tvgsbejvaqbjf.bet","subject":"Re: [PATCH v3 09/12] bisect--helper: reimplement `bisect_state` & `bisect_head` shell functions in C","fromName":"Miriam R.","fromEmail":"mirucam@gmail.com","sentAt":"2020-06-20T08:04:46Z","receivedAt":"2020-06-20T08:05:03Z","isPatch":true,"sender":{"key":"mirucam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56339109?v=4"},"body":"Hi Johannes,\nI'm finishing the next patch series version and I have an issue about\none of your suggestions:\n\nEl sáb., 23 may. 2020 a las 0:06, Johannes Schindelin\n() escribió:\n>\n> Hi Miriam,\n>\n> On Thu, 23 Apr 2020, Miriam Rubio wrote:\n>\n> > From: Pranit Bauva \n> >\n> > Reimplement the `bisect_state()` shell functions in C and also add a\n> > subcommand `--bisect-state` to `git-bisect--helper` to call them from\n> > git-bisect.sh .\n> >\n> > Using `--bisect-state` subcommand is a temporary measure to port shell\n> > function to C so as to use the existing test suite. As more functions\n> > are ported, this subcommand will be retired and will be called by some\n> > other methods.\n> >\n> > `bisect_head()` is only called from `bisect_state()`, thus it is not\n> > required to introduce another subcommand.\n> >\n> > Mentored-by: Lars Schneider \n> > Mentored-by: Christian Couder \n> > Mentored-by: Johannes Schindelin \n> > Signed-off-by: Pranit Bauva \n> > Signed-off-by: Tanushree Tumane \n> > Signed-off-by: Miriam Rubio \n> > ---\n> > builtin/bisect--helper.c | 70 +++++++++++++++++++++++++++++++++++++++-\n> > git-bisect.sh | 55 +++----------------------------\n> > 2 files changed, 73 insertions(+), 52 deletions(-)\n> >\n> > diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\n> > index 2d8660c79f..9db72f5891 100644\n> > --- a/builtin/bisect--helper.c\n> > +++ b/builtin/bisect--helper.c\n> > @@ -31,6 +31,8 @@ static const char * const git_bisect_helper_usage[] = {\n> > N_(\"git bisect--helper --bisect-next\"),\n> > N_(\"git bisect--helper --bisect-auto-next\"),\n> > N_(\"git bisect--helper --bisect-autostart\"),\n> > + N_(\"git bisect--helper --bisect-state (bad|new) []\"),\n> > + N_(\"git bisect--helper --bisect-state (good|old) [...]\"),\n> > NULL\n> > };\n> >\n> > @@ -834,6 +836,64 @@ static int bisect_autostart(struct bisect_terms *terms)\n> > return bisect_start(terms, 0, NULL, 0);\n> > }\n> >\n> > +static int bisect_head(struct object_id *oid)\n> > +{\n> > + if (!file_exists(git_path_bisect_head()))\n> > + return get_oid(\"HEAD\", oid);\n> > +\n> > + return get_oid(\"BISECT_HEAD\", oid);\n>\n> This can be easily reduced to\n>\n> return get_oid(file_exists(git_path_bisect_head()) ?\n> \"BISECT_HEAD\" : \"HEAD\", oid);\n>\n> At the same time, it is wrong, just like the shell script version was\n> wrong: in particular in light of the `hn/reftable` effort, we do _not_\n> want to assume that all refs are backed by files!\n>\n> So really, what this should do instead is this:\n>\n> enum get_oid_result res = get_oid(\"BISECT_HEAD\", oid);\n>\n> if (res == MISSING_OBJECT)\n> res = get_oid(\"HEAD\", oid);\n>\n> Given that this is still only three lines long, the overhead of having it\n> in its own function for just a _single_ call seems excessive. I'd prefer\n> it to be inlined in `bisect_state()`.\n>\n> > +}\n> > +\n> > +static enum bisect_error bisect_state(struct bisect_terms *terms, const char **argv,\n> > + int argc)\n> > +{\n>\n> I offered a lengthy discussion about this function in\n> https://lore.kernel.org/git/nycvar.QRO.7.76.6.2002272244150.9783@tvgsbejvaqbjf.bet/\n>\n> It does not look, however, as if v3 benefitted from the entirety of my\n> analysis: All the `check_expected_revs()` function does is to verify that\n> the passed list of revs matches exactly the contents of the\n> `BISECT_EXPECTED_REV` file.\n>\n> That can be done in a much simpler way, though, by first reading the file\n> and parsing the contents into an OID, and then comparing to that parsed\n> OID instead.\n>\n> Besides, `check_expected_revs()` is only used to check one rev at a time.\n>\n> In other words, it could be simplified to something like this:\n>\n> static void check_expected_rev(struct object_id *oid)\n> {\n> struct object_id expected;\n> struct strbuf buf = STRBUF_INIT;\n>\n> if (strbuf_read_file(&buf, git_path_bisect_expected_rev(), 0)\n> < the_hash_algo->hexsz ||\n> get_oid_hex(buf.buf, &expected) < 0)\n> return; /* Ignore invalid file contents */\n>\n> if (!oideq(oid, &expected)) {\n> ... unlink ...\n> return;\n> }\n> }\n>\n> But even that would be wasteful, as we would read the file over and over\n> and over again.\n>\n> The good news is that we do not even _need_ `check_expected_rev()`.\n> Because we do not need to have two call sites, we can simplify the code\n> much further. See below:\n>\n> > + const char *state;\n> > + const char *hex;\n> > + int i;\n> > + struct oid_array revs = OID_ARRAY_INIT;\n> > + struct object_id oid;\n> > +\n> > + if (!argc)\n> > + return error(_(\"Please call `--bisect-state` with at least one argument\"));\n> > + state = argv[0];\n> > + if (check_and_set_terms(terms, state) ||\n> > + !one_of(state, terms->term_good,terms->term_bad, \"skip\", NULL))\n> > + return BISECT_FAILED;\n> > + argv++;\n> > + argc--;\n> > + if (!strcmp(state, terms->term_bad) && (argc > 1))\n> > + return error(_(\"'git bisect %s' can take only one argument.\"),terms->term_bad);\n> > + if (argc == 0) {\n> > + if (bisect_head(&oid))\n> > + return error(_(\"Bad bisect_head rev input\"));\n> > + hex = oid_to_hex(&oid);\n> > + if (bisect_write(state, hex, terms, 0))\n> > + return BISECT_FAILED;\n> > + check_expected_revs(&hex, 1);\n> > + return bisect_auto_next(terms, NULL);\n> > + }\n> > +\n> > + /* Here argc > 0 */\n> > + for (; argc; argc--, argv++) {\n> > + struct object_id oid;\n> > + if (get_oid(*argv, &oid))\n> > + return error(_(\"Bad rev input: %s\"), *argv);\n> > + oid_array_append(&revs, &oid);\n> > + }\n>\n> It really does not make sense to parse the arguments into an OID array,\n> _then_ iterate over the array once, and then immediately releasing it.\n> That OID array is not needed at all.\n>\n> So we'll end up with this loop in case `argc > 0` (where we now call\n> `get_oid()`, too), and note how the loop body looks _eerily_ similar to\n> the conditional `argc == 0` code block above?\n>\n> > +\n> > + for (i = 0; i < revs.nr; i++) {\n> > + hex = oid_to_hex(&revs.oid[i]);\n> > + if (bisect_write(state, hex, terms, 0)) {\n> > + oid_array_clear(&revs);\n> > + return BISECT_FAILED;\n> > + }\n> > + check_expected_revs(&hex, 1);\n> > + }\n> > +\n> > + oid_array_clear(&revs);\n> > + return bisect_auto_next(terms, NULL);\n> > +}\n>\n> So really, this function pretty much _wants_ to look this way (modulo\n> bugs, as I did not even test-compile the code):\n>\n> static enum bisect_error bisect_state(struct bisect_terms *terms,\n> const char **argv, int argc)\n> {\n> const char *state;\n> int i, verify_expected = 1;\n> struct object_id oid, expected;\n> struct strbuf buf = STRBUF_INIT;\n>\n> if (!argc)\n> return error(_(\"Please call `--bisect-state` with at least one argument\"));\n>\n> state = argv[0];\n> if (check_and_set_terms(terms, state) ||\n> !one_of(state, terms->term_good, terms->term_bad, \"skip\", NULL))\n> return BISECT_FAILED;\n>\n> argv++;\n> argc--;\n> if (argc > 1 && !strcmp(state, terms->term_bad))\n> return error(_(\"'git bisect %s' can take only one argument.\"), terms->term_bad);\n>\n> if (strbuf_read_file(&buf, git_path_bisect_expected_rev(), 0) < the_hash_algo->hexsz ||\n> get_oid_hex(buf.buf, &expected) < 0)\n> verify_expected = 0; /* Ignore invalid file contents */\n>\n>\n> for (i = 0; i < argc + !argc; i++) {\n> if (argc) {\n> if (get_oid(argv[i], &oid)) {\n> error(_(\"Bad rev input: %s\"), *argv);\n> return BISECT_FAILED;\n> }\n> } else {\n> enum get_oid_result res = get_oid(\"BISECT_HEAD\", &oid);\n>\n> if (res == MISSING_OBJECT)\n> res = get_oid(\"HEAD\", &oid);\n> if (res) {\n> error(_(\"Bad bisect_head rev input\"));\n> return BISECT_FAILED;\n> }\n> }\n>\n> if (bisect_write(state, oid_to_hex(&oid), terms, 0))\n> return BISECT_FAILED;\n>\n> if (verify_expected && !oideq(&oid, &expected)) {\n> unlink_or_warn(git_path_bisect_ancestors_ok());\n> unlink_or_warn(git_path_bisect_expected_rev());\n> verify_expected = 0;\n> }\n> }\n>\n> return bisect_auto_next(terms, NULL);\n> }\n>\n> There, not bad, is it?\n>\n After implementing this solution some tests failed. After debugging\nthem, I found that with Pranit's solution, that arguments were parsed\ninto an OID array, if bisect received some junk rev the function\nreturned and bisect_write() was not executed.\nWith the new solution, if junk rev is received after a valid rev,\nbisect_write() was executed for the valid and the function returned with\nthe junk rev.\nSo, there is garbage in the file and when for example bisect-porcelain\ntest number 5 - 'bisect fails if given any junk instead of revs'\nexecutes 'test -z' fails.\n\nShould I keep the original patch and add a comment in the code that\nexplains why we use an oid array?.\n\n(I also have implemented an alternative solution that when some junk\nrev is found, I delete all refs written, but maybe is too complicated\nor not totally correct:\nhttps://gitlab.com/mirucam/git/-/commit/93f669877b87d09a30a5d07f0967667b22026511\n)\n\n\n> > +\n> > int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n> > {\n> > enum {\n> > @@ -847,7 +907,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n> > BISECT_START,\n> > BISECT_NEXT,\n> > BISECT_AUTO_NEXT,\n> > - BISECT_AUTOSTART\n> > + BISECT_AUTOSTART,\n> > + BISECT_STATE\n> > } cmdmode = 0;\n> > int no_checkout = 0, res = 0, nolog = 0;\n> > struct option options[] = {\n> > @@ -873,6 +934,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n> > N_(\"verify the next bisection state then checkout the next bisection commit\"), BISECT_AUTO_NEXT),\n> > OPT_CMDMODE(0, \"bisect-autostart\", &cmdmode,\n> > N_(\"start the bisection if BISECT_START is empty or missing\"), BISECT_AUTOSTART),\n> > + OPT_CMDMODE(0, \"bisect-state\", &cmdmode,\n> > + N_(\"mark the state of ref (or refs)\"), BISECT_STATE),\n> > OPT_BOOL(0, \"no-checkout\", &no_checkout,\n> > N_(\"update BISECT_HEAD instead of checking out the current commit\")),\n> > OPT_BOOL(0, \"no-log\", &nolog,\n> > @@ -945,6 +1008,11 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n> > set_terms(&terms, \"bad\", \"good\");\n> > res = bisect_autostart(&terms);\n> > break;\n> > + case BISECT_STATE:\n> > + set_terms(&terms, \"bad\", \"good\");\n> > + get_terms(&terms);\n> > + res = bisect_state(&terms, argv, argc);\n> > + break;\n> > default:\n> > BUG(\"unknown subcommand %d\", (int)cmdmode);\n> > }\n> > diff --git a/git-bisect.sh b/git-bisect.sh\n> > index 049ffacdff..2da0810b1a 100755\n> > --- a/git-bisect.sh\n> > +++ b/git-bisect.sh\n> > @@ -39,16 +39,6 @@ _x40=\"$_x40$_x40$_x40$_x40$_x40$_x40$_x40$_x40\"\n> > TERM_BAD=bad\n> > TERM_GOOD=good\n> >\n> > -bisect_head()\n> > -{\n> > - if test -f \"$GIT_DIR/BISECT_HEAD\"\n> > - then\n> > - echo BISECT_HEAD\n> > - else\n> > - echo HEAD\n> > - fi\n> > -}\n> > -\n> > bisect_skip() {\n> > all=''\n> > for arg in \"$@\"\n> > @@ -61,43 +51,7 @@ bisect_skip() {\n> > esac\n> > all=\"$all $revs\"\n> > done\n> > - eval bisect_state 'skip' $all\n> > -}\n> > -\n> > -bisect_state() {\n> > - git bisect--helper --bisect-autostart\n> > - state=$1\n> > - git bisect--helper --check-and-set-terms $state $TERM_GOOD $TERM_BAD || exit\n> > - get_terms\n> > - case \"$#,$state\" in\n> > - 0,*)\n> > - die \"Please call 'bisect_state' with at least one argument.\" ;;\n> > - 1,\"$TERM_BAD\"|1,\"$TERM_GOOD\"|1,skip)\n> > - bisected_head=$(bisect_head)\n> > - rev=$(git rev-parse --verify \"$bisected_head\") ||\n> > - die \"$(eval_gettext \"Bad rev input: \\$bisected_head\")\"\n> > - git bisect--helper --bisect-write \"$state\" \"$rev\" \"$TERM_GOOD\" \"$TERM_BAD\" || exit\n> > - git bisect--helper --check-expected-revs \"$rev\" ;;\n> > - 2,\"$TERM_BAD\"|*,\"$TERM_GOOD\"|*,skip)\n> > - shift\n> > - hash_list=''\n> > - for rev in \"$@\"\n> > - do\n> > - sha=$(git rev-parse --verify \"$rev^{commit}\") ||\n> > - die \"$(eval_gettext \"Bad rev input: \\$rev\")\"\n> > - hash_list=\"$hash_list $sha\"\n> > - done\n> > - for rev in $hash_list\n> > - do\n> > - git bisect--helper --bisect-write \"$state\" \"$rev\" \"$TERM_GOOD\" \"$TERM_BAD\" || exit\n> > - done\n> > - git bisect--helper --check-expected-revs $hash_list ;;\n> > - *,\"$TERM_BAD\")\n> > - die \"$(eval_gettext \"'git bisect \\$TERM_BAD' can take only one argument.\")\" ;;\n> > - *)\n> > - usage ;;\n> > - esac\n> > - git bisect--helper --bisect-auto-next\n> > + eval git bisect--helper --bisect-state 'skip' $all\n> > }\n> >\n> > bisect_visualize() {\n> > @@ -185,8 +139,7 @@ exit code \\$res from '\\$command' is < 0 or >= 128\" >&2\n> > state=\"$TERM_GOOD\"\n> > fi\n> >\n> > - # We have to use a subshell because \"bisect_state\" can exit.\n> > - ( bisect_state $state >\"$GIT_DIR/BISECT_RUN\" )\n> > + git bisect--helper --bisect-state $state >\"$GIT_DIR/BISECT_RUN\"\n> > res=$?\n> >\n> > cat \"$GIT_DIR/BISECT_RUN\"\n> > @@ -201,7 +154,7 @@ exit code \\$res from '\\$command' is < 0 or >= 128\" >&2\n> > if [ $res -ne 0 ]\n> > then\n> > eval_gettextln \"bisect run failed:\n> > -'bisect_state \\$state' exited with error code \\$res\" >&2\n> > +'git bisect--helper --bisect-state \\$state' exited with error code \\$res\" >&2\n>\n> This is not your fault, of course, but it does make me shudder to see such\n> an obvious implementation detail in a user-facing error message.\n>\n> Maybe something to fix up in a follow-up?\n>\n> Ciao,\n> Dscho\n>\n> > exit $res\n> > fi\n> >\n> > @@ -242,7 +195,7 @@ case \"$#\" in\n> > start)\n> > git bisect--helper --bisect-start \"$@\" ;;\n> > bad|good|new|old|\"$TERM_BAD\"|\"$TERM_GOOD\")\n> > - bisect_state \"$cmd\" \"$@\" ;;\n> > + git bisect--helper --bisect-state \"$cmd\" \"$@\" ;;\n> > skip)\n> > bisect_skip \"$@\" ;;\n> > next)\n> > --\n> > 2.25.0\n> >\n> >\nBest,\nMiriam.\n"},{"id":"400278","messageId":"nycvar.QRO.7.76.6.2006191555170.54@tvgsbejvaqbjf.bet","threadId":"53293","inReplyTo":"CAN7CjDAnksyerW_sEDLFS=GT0g7rLt53dUA09a8jAqnZb9P_8w@mail.gmail.com","subject":"Re: [PATCH v3 09/12] bisect--helper: reimplement `bisect_state` & `bisect_head` shell functions in C","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-06-19T13:57:40Z","receivedAt":"2020-06-22T12:18:02Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Miriam,\n\nOn Sat, 20 Jun 2020, Miriam R. wrote:\n\n> Hi Johannes,\n> I'm finishing the next patch series version and I have an issue about\n> one of your suggestions:\n>\n> El sáb., 23 may. 2020 a las 0:06, Johannes Schindelin\n> () escribió:\n> >\n> > Hi Miriam,\n> >\n> > On Thu, 23 Apr 2020, Miriam Rubio wrote:\n> >\n> > > From: Pranit Bauva \n> > >\n> > > Reimplement the `bisect_state()` shell functions in C and also add a\n> > > subcommand `--bisect-state` to `git-bisect--helper` to call them from\n> > > git-bisect.sh .\n> > >\n> > > Using `--bisect-state` subcommand is a temporary measure to port shell\n> > > function to C so as to use the existing test suite. As more functions\n> > > are ported, this subcommand will be retired and will be called by some\n> > > other methods.\n> > >\n> > > `bisect_head()` is only called from `bisect_state()`, thus it is not\n> > > required to introduce another subcommand.\n> > >\n> > > Mentored-by: Lars Schneider \n> > > Mentored-by: Christian Couder \n> > > Mentored-by: Johannes Schindelin \n> > > Signed-off-by: Pranit Bauva \n> > > Signed-off-by: Tanushree Tumane \n> > > Signed-off-by: Miriam Rubio \n> > > ---\n> > > builtin/bisect--helper.c | 70 +++++++++++++++++++++++++++++++++++++++-\n> > > git-bisect.sh | 55 +++----------------------------\n> > > 2 files changed, 73 insertions(+), 52 deletions(-)\n> > >\n> > > diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\n> > > index 2d8660c79f..9db72f5891 100644\n> > > --- a/builtin/bisect--helper.c\n> > > +++ b/builtin/bisect--helper.c\n> > > @@ -31,6 +31,8 @@ static const char * const git_bisect_helper_usage[] = {\n> > > N_(\"git bisect--helper --bisect-next\"),\n> > > N_(\"git bisect--helper --bisect-auto-next\"),\n> > > N_(\"git bisect--helper --bisect-autostart\"),\n> > > + N_(\"git bisect--helper --bisect-state (bad|new) []\"),\n> > > + N_(\"git bisect--helper --bisect-state (good|old) [...]\"),\n> > > NULL\n> > > };\n> > >\n> > > @@ -834,6 +836,64 @@ static int bisect_autostart(struct bisect_terms *terms)\n> > > return bisect_start(terms, 0, NULL, 0);\n> > > }\n> > >\n> > > +static int bisect_head(struct object_id *oid)\n> > > +{\n> > > + if (!file_exists(git_path_bisect_head()))\n> > > + return get_oid(\"HEAD\", oid);\n> > > +\n> > > + return get_oid(\"BISECT_HEAD\", oid);\n> >\n> > This can be easily reduced to\n> >\n> > return get_oid(file_exists(git_path_bisect_head()) ?\n> > \"BISECT_HEAD\" : \"HEAD\", oid);\n> >\n> > At the same time, it is wrong, just like the shell script version was\n> > wrong: in particular in light of the `hn/reftable` effort, we do _not_\n> > want to assume that all refs are backed by files!\n> >\n> > So really, what this should do instead is this:\n> >\n> > enum get_oid_result res = get_oid(\"BISECT_HEAD\", oid);\n> >\n> > if (res == MISSING_OBJECT)\n> > res = get_oid(\"HEAD\", oid);\n> >\n> > Given that this is still only three lines long, the overhead of having it\n> > in its own function for just a _single_ call seems excessive. I'd prefer\n> > it to be inlined in `bisect_state()`.\n> >\n> > > +}\n> > > +\n> > > +static enum bisect_error bisect_state(struct bisect_terms *terms, const char **argv,\n> > > + int argc)\n> > > +{\n> >\n> > I offered a lengthy discussion about this function in\n> > https://lore.kernel.org/git/nycvar.QRO.7.76.6.2002272244150.9783@tvgsbejvaqbjf.bet/\n> >\n> > It does not look, however, as if v3 benefitted from the entirety of my\n> > analysis: All the `check_expected_revs()` function does is to verify that\n> > the passed list of revs matches exactly the contents of the\n> > `BISECT_EXPECTED_REV` file.\n> >\n> > That can be done in a much simpler way, though, by first reading the file\n> > and parsing the contents into an OID, and then comparing to that parsed\n> > OID instead.\n> >\n> > Besides, `check_expected_revs()` is only used to check one rev at a time.\n> >\n> > In other words, it could be simplified to something like this:\n> >\n> > static void check_expected_rev(struct object_id *oid)\n> > {\n> > struct object_id expected;\n> > struct strbuf buf = STRBUF_INIT;\n> >\n> > if (strbuf_read_file(&buf, git_path_bisect_expected_rev(), 0)\n> > < the_hash_algo->hexsz ||\n> > get_oid_hex(buf.buf, &expected) < 0)\n> > return; /* Ignore invalid file contents */\n> >\n> > if (!oideq(oid, &expected)) {\n> > ... unlink ...\n> > return;\n> > }\n> > }\n> >\n> > But even that would be wasteful, as we would read the file over and over\n> > and over again.\n> >\n> > The good news is that we do not even _need_ `check_expected_rev()`.\n> > Because we do not need to have two call sites, we can simplify the code\n> > much further. See below:\n> >\n> > > + const char *state;\n> > > + const char *hex;\n> > > + int i;\n> > > + struct oid_array revs = OID_ARRAY_INIT;\n> > > + struct object_id oid;\n> > > +\n> > > + if (!argc)\n> > > + return error(_(\"Please call `--bisect-state` with at least one argument\"));\n> > > + state = argv[0];\n> > > + if (check_and_set_terms(terms, state) ||\n> > > + !one_of(state, terms->term_good,terms->term_bad, \"skip\", NULL))\n> > > + return BISECT_FAILED;\n> > > + argv++;\n> > > + argc--;\n> > > + if (!strcmp(state, terms->term_bad) && (argc > 1))\n> > > + return error(_(\"'git bisect %s' can take only one argument.\"),terms->term_bad);\n> > > + if (argc == 0) {\n> > > + if (bisect_head(&oid))\n> > > + return error(_(\"Bad bisect_head rev input\"));\n> > > + hex = oid_to_hex(&oid);\n> > > + if (bisect_write(state, hex, terms, 0))\n> > > + return BISECT_FAILED;\n> > > + check_expected_revs(&hex, 1);\n> > > + return bisect_auto_next(terms, NULL);\n> > > + }\n> > > +\n> > > + /* Here argc > 0 */\n> > > + for (; argc; argc--, argv++) {\n> > > + struct object_id oid;\n> > > + if (get_oid(*argv, &oid))\n> > > + return error(_(\"Bad rev input: %s\"), *argv);\n> > > + oid_array_append(&revs, &oid);\n> > > + }\n> >\n> > It really does not make sense to parse the arguments into an OID array,\n> > _then_ iterate over the array once, and then immediately releasing it.\n> > That OID array is not needed at all.\n> >\n> > So we'll end up with this loop in case `argc > 0` (where we now call\n> > `get_oid()`, too), and note how the loop body looks _eerily_ similar to\n> > the conditional `argc == 0` code block above?\n> >\n> > > +\n> > > + for (i = 0; i < revs.nr; i++) {\n> > > + hex = oid_to_hex(&revs.oid[i]);\n> > > + if (bisect_write(state, hex, terms, 0)) {\n> > > + oid_array_clear(&revs);\n> > > + return BISECT_FAILED;\n> > > + }\n> > > + check_expected_revs(&hex, 1);\n> > > + }\n> > > +\n> > > + oid_array_clear(&revs);\n> > > + return bisect_auto_next(terms, NULL);\n> > > +}\n> >\n> > So really, this function pretty much _wants_ to look this way (modulo\n> > bugs, as I did not even test-compile the code):\n> >\n> > static enum bisect_error bisect_state(struct bisect_terms *terms,\n> > const char **argv, int argc)\n> > {\n> > const char *state;\n> > int i, verify_expected = 1;\n> > struct object_id oid, expected;\n> > struct strbuf buf = STRBUF_INIT;\n> >\n> > if (!argc)\n> > return error(_(\"Please call `--bisect-state` with at least one argument\"));\n> >\n> > state = argv[0];\n> > if (check_and_set_terms(terms, state) ||\n> > !one_of(state, terms->term_good, terms->term_bad, \"skip\", NULL))\n> > return BISECT_FAILED;\n> >\n> > argv++;\n> > argc--;\n> > if (argc > 1 && !strcmp(state, terms->term_bad))\n> > return error(_(\"'git bisect %s' can take only one argument.\"), terms->term_bad);\n> >\n> > if (strbuf_read_file(&buf, git_path_bisect_expected_rev(), 0) < the_hash_algo->hexsz ||\n> > get_oid_hex(buf.buf, &expected) < 0)\n> > verify_expected = 0; /* Ignore invalid file contents */\n> >\n> >\n> > for (i = 0; i < argc + !argc; i++) {\n> > if (argc) {\n> > if (get_oid(argv[i], &oid)) {\n> > error(_(\"Bad rev input: %s\"), *argv);\n> > return BISECT_FAILED;\n> > }\n> > } else {\n> > enum get_oid_result res = get_oid(\"BISECT_HEAD\", &oid);\n> >\n> > if (res == MISSING_OBJECT)\n> > res = get_oid(\"HEAD\", &oid);\n> > if (res) {\n> > error(_(\"Bad bisect_head rev input\"));\n> > return BISECT_FAILED;\n> > }\n> > }\n> >\n> > if (bisect_write(state, oid_to_hex(&oid), terms, 0))\n> > return BISECT_FAILED;\n> >\n> > if (verify_expected && !oideq(&oid, &expected)) {\n> > unlink_or_warn(git_path_bisect_ancestors_ok());\n> > unlink_or_warn(git_path_bisect_expected_rev());\n> > verify_expected = 0;\n> > }\n> > }\n> >\n> > return bisect_auto_next(terms, NULL);\n> > }\n> >\n> > There, not bad, is it?\n> >\n> After implementing this solution some tests failed. After debugging\n> them, I found that with Pranit's solution, that arguments were parsed\n> into an OID array, if bisect received some junk rev the function\n> returned and bisect_write() was not executed.\n\nThat's a good point, and should be turned into a code comment lest\noverzealous developers would attempt the simplification I proposed in the\nfuture.\n\n> With the new solution, if junk rev is received after a valid rev,\n> bisect_write() was executed for the valid and the function returned with\n> the junk rev.\n> So, there is garbage in the file and when for example bisect-porcelain\n> test number 5 - 'bisect fails if given any junk instead of revs'\n> executes 'test -z' fails.\n>\n> Should I keep the original patch and add a comment in the code that\n> explains why we use an oid array?.\n\nI still think that the `check_expected_revs()` function is not needed, and\nunnecessarily complex.\n\nIf you _want_ to keep Pranit's original implementation, then I would _at\nleast_ want you to forward-port the `verify_expected` changes I suggested.\n\n>\n> (I also have implemented an alternative solution that when some junk\n> rev is found, I delete all refs written, but maybe is too complicated\n> or not totally correct:\n> https://gitlab.com/mirucam/git/-/commit/93f669877b87d09a30a5d07f0967667b22026511\n> )\n\nYou seem to share my preference for not writing out bogus revs in the\nfirst place.\n\nThanks,\nJohannes\n\n>\n>\n> > > +\n> > > int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n> > > {\n> > > enum {\n> > > @@ -847,7 +907,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n> > > BISECT_START,\n> > > BISECT_NEXT,\n> > > BISECT_AUTO_NEXT,\n> > > - BISECT_AUTOSTART\n> > > + BISECT_AUTOSTART,\n> > > + BISECT_STATE\n> > > } cmdmode = 0;\n> > > int no_checkout = 0, res = 0, nolog = 0;\n> > > struct option options[] = {\n> > > @@ -873,6 +934,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n> > > N_(\"verify the next bisection state then checkout the next bisection commit\"), BISECT_AUTO_NEXT),\n> > > OPT_CMDMODE(0, \"bisect-autostart\", &cmdmode,\n> > > N_(\"start the bisection if BISECT_START is empty or missing\"), BISECT_AUTOSTART),\n> > > + OPT_CMDMODE(0, \"bisect-state\", &cmdmode,\n> > > + N_(\"mark the state of ref (or refs)\"), BISECT_STATE),\n> > > OPT_BOOL(0, \"no-checkout\", &no_checkout,\n> > > N_(\"update BISECT_HEAD instead of checking out the current commit\")),\n> > > OPT_BOOL(0, \"no-log\", &nolog,\n> > > @@ -945,6 +1008,11 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n> > > set_terms(&terms, \"bad\", \"good\");\n> > > res = bisect_autostart(&terms);\n> > > break;\n> > > + case BISECT_STATE:\n> > > + set_terms(&terms, \"bad\", \"good\");\n> > > + get_terms(&terms);\n> > > + res = bisect_state(&terms, argv, argc);\n> > > + break;\n> > > default:\n> > > BUG(\"unknown subcommand %d\", (int)cmdmode);\n> > > }\n> > > diff --git a/git-bisect.sh b/git-bisect.sh\n> > > index 049ffacdff..2da0810b1a 100755\n> > > --- a/git-bisect.sh\n> > > +++ b/git-bisect.sh\n> > > @@ -39,16 +39,6 @@ _x40=\"$_x40$_x40$_x40$_x40$_x40$_x40$_x40$_x40\"\n> > > TERM_BAD=bad\n> > > TERM_GOOD=good\n> > >\n> > > -bisect_head()\n> > > -{\n> > > - if test -f \"$GIT_DIR/BISECT_HEAD\"\n> > > - then\n> > > - echo BISECT_HEAD\n> > > - else\n> > > - echo HEAD\n> > > - fi\n> > > -}\n> > > -\n> > > bisect_skip() {\n> > > all=''\n> > > for arg in \"$@\"\n> > > @@ -61,43 +51,7 @@ bisect_skip() {\n> > > esac\n> > > all=\"$all $revs\"\n> > > done\n> > > - eval bisect_state 'skip' $all\n> > > -}\n> > > -\n> > > -bisect_state() {\n> > > - git bisect--helper --bisect-autostart\n> > > - state=$1\n> > > - git bisect--helper --check-and-set-terms $state $TERM_GOOD $TERM_BAD || exit\n> > > - get_terms\n> > > - case \"$#,$state\" in\n> > > - 0,*)\n> > > - die \"Please call 'bisect_state' with at least one argument.\" ;;\n> > > - 1,\"$TERM_BAD\"|1,\"$TERM_GOOD\"|1,skip)\n> > > - bisected_head=$(bisect_head)\n> > > - rev=$(git rev-parse --verify \"$bisected_head\") ||\n> > > - die \"$(eval_gettext \"Bad rev input: \\$bisected_head\")\"\n> > > - git bisect--helper --bisect-write \"$state\" \"$rev\" \"$TERM_GOOD\" \"$TERM_BAD\" || exit\n> > > - git bisect--helper --check-expected-revs \"$rev\" ;;\n> > > - 2,\"$TERM_BAD\"|*,\"$TERM_GOOD\"|*,skip)\n> > > - shift\n> > > - hash_list=''\n> > > - for rev in \"$@\"\n> > > - do\n> > > - sha=$(git rev-parse --verify \"$rev^{commit}\") ||\n> > > - die \"$(eval_gettext \"Bad rev input: \\$rev\")\"\n> > > - hash_list=\"$hash_list $sha\"\n> > > - done\n> > > - for rev in $hash_list\n> > > - do\n> > > - git bisect--helper --bisect-write \"$state\" \"$rev\" \"$TERM_GOOD\" \"$TERM_BAD\" || exit\n> > > - done\n> > > - git bisect--helper --check-expected-revs $hash_list ;;\n> > > - *,\"$TERM_BAD\")\n> > > - die \"$(eval_gettext \"'git bisect \\$TERM_BAD' can take only one argument.\")\" ;;\n> > > - *)\n> > > - usage ;;\n> > > - esac\n> > > - git bisect--helper --bisect-auto-next\n> > > + eval git bisect--helper --bisect-state 'skip' $all\n> > > }\n> > >\n> > > bisect_visualize() {\n> > > @@ -185,8 +139,7 @@ exit code \\$res from '\\$command' is < 0 or >= 128\" >&2\n> > > state=\"$TERM_GOOD\"\n> > > fi\n> > >\n> > > - # We have to use a subshell because \"bisect_state\" can exit.\n> > > - ( bisect_state $state >\"$GIT_DIR/BISECT_RUN\" )\n> > > + git bisect--helper --bisect-state $state >\"$GIT_DIR/BISECT_RUN\"\n> > > res=$?\n> > >\n> > > cat \"$GIT_DIR/BISECT_RUN\"\n> > > @@ -201,7 +154,7 @@ exit code \\$res from '\\$command' is < 0 or >= 128\" >&2\n> > > if [ $res -ne 0 ]\n> > > then\n> > > eval_gettextln \"bisect run failed:\n> > > -'bisect_state \\$state' exited with error code \\$res\" >&2\n> > > +'git bisect--helper --bisect-state \\$state' exited with error code \\$res\" >&2\n> >\n> > This is not your fault, of course, but it does make me shudder to see such\n> > an obvious implementation detail in a user-facing error message.\n> >\n> > Maybe something to fix up in a follow-up?\n> >\n> > Ciao,\n> > Dscho\n> >\n> > > exit $res\n> > > fi\n> > >\n> > > @@ -242,7 +195,7 @@ case \"$#\" in\n> > > start)\n> > > git bisect--helper --bisect-start \"$@\" ;;\n> > > bad|good|new|old|\"$TERM_BAD\"|\"$TERM_GOOD\")\n> > > - bisect_state \"$cmd\" \"$@\" ;;\n> > > + git bisect--helper --bisect-state \"$cmd\" \"$@\" ;;\n> > > skip)\n> > > bisect_skip \"$@\" ;;\n> > > next)\n> > > --\n> > > 2.25.0\n> > >\n> > >\n> Best,\n> Miriam.\n>\n"}]}