{"thread":{"id":"54167","subject":"[PATCH v7 01/13] bisect--helper: BUG() in cmd_*() on invalid subcommand","startedAt":"2020-08-31T10:51:31Z","lastAt":"2020-09-24T12:56:46Z","messageCount":27,"participants":["Miriam Rubio","Johannes Schindelin","Miriam R.","Christian Couder"],"isPatch":true,"patchVersion":7,"patchTotal":13},"messages":[{"id":"404797","messageId":"20200831105043.97665-2-mirucam@gmail.com","threadId":"54167","inReplyTo":"20200831105043.97665-1-mirucam@gmail.com","subject":"[PATCH v7 01/13] bisect--helper: BUG() in cmd_*() on invalid subcommand","fromName":"Miriam Rubio","fromEmail":"mirucam@gmail.com","sentAt":"2020-08-31T10:50:31Z","receivedAt":"2020-08-31T10:51:31Z","isPatch":true,"sender":{"key":"mirucam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56339109?v=4"},"body":"In cmd_bisect__helper() function, if an invalid or no\nsubcommand is passed there is a BUG.\n\nBUG() out instead of returning an error.\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 cdda279b23..f464e95792 100644\n--- a/builtin/bisect--helper.c\n+++ b/builtin/bisect--helper.c\n@@ -720,7 +720,7 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n \t\tres = bisect_start(&terms, argv, argc);\n \t\tbreak;\n \tdefault:\n-\t\treturn error(\"BUG: unknown subcommand '%d'\", cmdmode);\n+\t\tBUG(\"unknown subcommand %d\", cmdmode);\n \t}\n \tfree_terms(&terms);\n \n-- \n2.25.0\n\n"},{"id":"404798","messageId":"20200831105043.97665-3-mirucam@gmail.com","threadId":"54167","inReplyTo":"20200831105043.97665-1-mirucam@gmail.com","subject":"[PATCH v7 02/13] bisect--helper: use '-res' in 'cmd_bisect__helper' return","fromName":"Miriam Rubio","fromEmail":"mirucam@gmail.com","sentAt":"2020-08-31T10:50:32Z","receivedAt":"2020-08-31T10:51:39Z","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 \nHelped-by: Junio C Hamano \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 f464e95792..b7345be3a5 100644\n--- a/builtin/bisect--helper.c\n+++ b/builtin/bisect--helper.c\n@@ -731,5 +731,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":"404799","messageId":"20200831105043.97665-4-mirucam@gmail.com","threadId":"54167","inReplyTo":"20200831105043.97665-1-mirucam@gmail.com","subject":"[PATCH v7 03/13] bisect--helper: introduce new `write_in_file()` function","fromName":"Miriam Rubio","fromEmail":"mirucam@gmail.com","sentAt":"2020-08-31T10:50:33Z","receivedAt":"2020-08-31T10:51:48Z","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 and a\nwrapper for writing mode.\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 | 43 +++++++++++++++++++++++++++++++++-------\n 1 file changed, 36 insertions(+), 7 deletions(-)\n\ndiff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\nindex b7345be3a5..bae09ce65d 100644\n--- a/builtin/bisect--helper.c\n+++ b/builtin/bisect--helper.c\n@@ -74,6 +74,40 @@ 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, va_list args)\n+{\n+\tFILE *fp = NULL;\n+\tint res = 0;\n+\n+\tif (strcmp(mode, \"w\"))\n+\t\tBUG(\"write-in-file does not support '%s' mode\", mode);\n+\tfp = fopen(path, mode);\n+\tif (!fp)\n+\t\treturn error_errno(_(\"cannot open file '%s' in mode '%s'\"), path, mode);\n+\tres = vfprintf(fp, format, args);\n+\n+\tif (res < 0) {\n+\t\tint saved_errno = errno;\n+\t\tfclose(fp);\n+\t\terrno = saved_errno;\n+\t\treturn error_errno(_(\"could not write to file '%s'\"), path);\n+\t}\n+\n+\treturn fclose(fp);\n+}\n+\n+static int write_to_file(const char *path, const char *format, ...)\n+{\n+\tint res;\n+\tva_list args;\n+\n+\tva_start(args, format);\n+\tres = write_in_file(path, \"w\", format, args);\n+\tva_end(args);\n+\n+\treturn res;\n+}\n+\n static int check_term_format(const char *term, const char *orig_term)\n {\n \tint res;\n@@ -104,7 +138,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,13 +146,9 @@ 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_to_file(git_path_bisect_terms(), \"%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+\treturn res;\n }\n \n static int is_expected_rev(const char *expected_hex)\n-- \n2.25.0\n\n"},{"id":"404800","messageId":"20200831105043.97665-5-mirucam@gmail.com","threadId":"54167","inReplyTo":"20200831105043.97665-1-mirucam@gmail.com","subject":"[PATCH v7 04/13] bisect--helper: reimplement `bisect_autostart` shell function in C","fromName":"Miriam Rubio","fromEmail":"mirucam@gmail.com","sentAt":"2020-08-31T10:50:34Z","receivedAt":"2020-08-31T10:51: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_autostart()` shell function in C and add the\nC implementation from `bisect_next()` which was previously left\nuncovered.\n\nAdd `--bisect-autostart` subcommand to be called from git-bisect.sh.\nUsing `--bisect-autostart` subcommand is a temporary measure to port\nthe shell 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 | 44 +++++++++++++++++++++++++++++++++++++++-\n git-bisect.sh | 25 ++---------------------\n 2 files changed, 45 insertions(+), 24 deletions(-)\n\ndiff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\nindex bae09ce65d..f71e8e54d2 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] [--first-parent] [ [...]] [--] [...]\"),\n+\tN_(\"git bisect--helper --bisect-autostart\"),\n \tNULL\n };\n \n@@ -653,6 +654,38 @@ static int bisect_start(struct bisect_terms *terms, const char **argv, int argc)\n \treturn res;\n }\n \n+static inline int file_is_not_empty(const char *path)\n+{\n+\treturn !is_empty_or_missing_file(path);\n+}\n+\n+static int bisect_autostart(struct bisect_terms *terms)\n+{\n+\tint res;\n+\tconst char *yesno;\n+\n+\tif (file_is_not_empty(git_path_bisect_start()))\n+\t\treturn 0;\n+\n+\tfprintf_ln(stderr, _(\"You need to start by \\\"git bisect \"\n+\t\t\t \"start\\\"\\n\"));\n+\n+\tif (!isatty(STDIN_FILENO))\n+\t\treturn 0;\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+\tres = tolower(*yesno) == 'n' ?\n+\t\t-1 : bisect_start(terms, empty_strvec, 0);\n+\n+\treturn res;\n+}\n+\n int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n {\n \tenum {\n@@ -665,7 +698,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 res = 0, nolog = 0;\n \tstruct option options[] = {\n@@ -689,6 +723,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 it has not yet been started\"), BISECT_AUTOSTART),\n \t\tOPT_BOOL(0, \"no-log\", &nolog,\n \t\t\t N_(\"no log for BISECT_WRITE\")),\n \t\tOPT_END()\n@@ -748,6 +784,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, argv, argc);\n \t\tbreak;\n+\tcase BISECT_AUTOSTART:\n+\t\tif (argc)\n+\t\t\treturn error(_(\"--bisect-autostart does not accept 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\", cmdmode);\n \t}\ndiff --git a/git-bisect.sh b/git-bisect.sh\nindex c7580e51a0..9ca583d964 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":"404801","messageId":"20200831105043.97665-6-mirucam@gmail.com","threadId":"54167","inReplyTo":"20200831105043.97665-1-mirucam@gmail.com","subject":"[PATCH v7 05/13] bisect: call 'clear_commit_marks_all()' in 'bisect_next_all()'","fromName":"Miriam Rubio","fromEmail":"mirucam@gmail.com","sentAt":"2020-08-31T10:50:35Z","receivedAt":"2020-08-31T10:52:07Z","isPatch":true,"sender":{"key":"mirucam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56339109?v=4"},"body":"As there can be other revision walks after bisect_next_all(),\nlet's add a call to a function to clear all the marks at the\nend of bisect_next_all().\n\nMentored-by: Christian Couder \nSigned-off-by: Miriam Rubio \n---\n bisect.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/bisect.c b/bisect.c\nindex d42a3a3767..c6aba2b9f2 100644\n--- a/bisect.c\n+++ b/bisect.c\n@@ -1082,6 +1082,8 @@ enum bisect_error bisect_next_all(struct repository *r, const char *prefix)\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 }\n-- \n2.25.0\n\n"},{"id":"404802","messageId":"20200831105043.97665-9-mirucam@gmail.com","threadId":"54167","inReplyTo":"20200831105043.97665-1-mirucam@gmail.com","subject":"[PATCH v7 08/13] bisect--helper: retire `--bisect-clean-state` subcommand","fromName":"Miriam Rubio","fromEmail":"mirucam@gmail.com","sentAt":"2020-08-31T10:50:38Z","receivedAt":"2020-08-31T10:52:13Z","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 b40369e8aa..b8b8a7473c 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_first_parent, \"BISECT_FIRST_PARENT\")\n static const char * const git_bisect_helper_usage[] = {\n \tN_(\"git bisect--helper --next-all\"),\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@@ -869,7 +868,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@@ -887,8 +885,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@@ -930,10 +926,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":"404803","messageId":"20200831105043.97665-1-mirucam@gmail.com","threadId":"54167","inReplyTo":null,"subject":"[PATCH v7 00/13] Finish converting git bisect to C part 2","fromName":"Miriam Rubio","fromEmail":"mirucam@gmail.com","sentAt":"2020-08-31T10:50:30Z","receivedAt":"2020-08-31T10:52:21Z","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-v7.\n\nI would like to thank Junio Hamano for reviewing this patch series.\n\nGeneral changes\n---------------\n\n* Rebased on e9b77c84a0 (Tenth batch, 2020-08-24), to build on the\n strvec API (instead of argv_array) and adjust to the codebase\n after the \"--first-parent\" feature was added.\n\n\nSpecific changes\n----------------\n\n[1/13] bisect--helper: BUG() in cmd_*() on invalid subcommand\n\n* Amend commit message.\n* Remove casting to int.\n\n---\n\n[2/13] bisect--helper: use '-res' in 'cmd_bisect__helper' return\n\n* Amend commit message.\n\n---\n\n[3/13] bisect--helper: introduce new `write_in_file()` function\n\n* Use saved_errno variable.\n\n---\n\n[5/13] bisect--helper: reimplement `bisect_autostart` shell function in C\n\n* Fix bug using empty_strvec instead of NULL in a `bisect_start()` call.\n\n---\n\n\n[6/13] bisect--helper: reimplement `bisect_next` and `bisect_auto_next` shell\n functions in C\n \n* Remove unused `no-checkout` variable.\n* Move a comment to more appropriate place.\n \n---\n\n.iriam Rubio (4):\n bisect--helper: BUG() in cmd_*() on invalid subcommand\n bisect--helper: use '-res' in 'cmd_bisect__helper' return\n bisect--helper: introduce new `write_in_file()` function\n bisect: call 'clear_commit_marks_all()' in 'bisect_next_all()'\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 | 13 +-\n builtin/bisect--helper.c | 442 ++++++++++++++++++++++++++++++++-------\n git-bisect.sh | 145 +------------\n 3 files changed, 380 insertions(+), 220 deletions(-)\n\n-- \n2.25.0\n\n"},{"id":"404804","messageId":"20200831105043.97665-14-mirucam@gmail.com","threadId":"54167","inReplyTo":"20200831105043.97665-1-mirucam@gmail.com","subject":"[PATCH v7 13/13] bisect--helper: retire `--bisect-autostart` subcommand","fromName":"Miriam Rubio","fromEmail":"mirucam@gmail.com","sentAt":"2020-08-31T10:50:43Z","receivedAt":"2020-08-31T10:52:24Z","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 46ddfd5710..8a48c1ef8e 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] [--first-parent] [ [...]] [--] [...]\"),\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@@ -916,7 +915,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 res = 0, nolog = 0;\n@@ -937,8 +935,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 it has not yet been started\"), 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-log\", &nolog,\n@@ -998,12 +994,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 does not accept 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":"404805","messageId":"20200831105043.97665-12-mirucam@gmail.com","threadId":"54167","inReplyTo":"20200831105043.97665-1-mirucam@gmail.com","subject":"[PATCH v7 11/13] bisect--helper: retire `--check-expected-revs` subcommand","fromName":"Miriam Rubio","fromEmail":"mirucam@gmail.com","sentAt":"2020-08-31T10:50:41Z","receivedAt":"2020-08-31T10:52:26Z","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. Functions `check_expected_revs` and\n`is_expected_revs` are also deleted.\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 | 30 ------------------------------\n 1 file changed, 30 deletions(-)\n\ndiff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\nindex a976f4739c..da203c2091 100644\n--- a/builtin/bisect--helper.c\n+++ b/builtin/bisect--helper.c\n@@ -187,30 +187,6 @@ static int write_terms(const char *bad, const char *good)\n \treturn res;\n }\n \n-static int is_expected_rev(const char *expected_hex)\n-{\n-\tstruct strbuf actual_hex = STRBUF_INIT;\n-\tint res = 0;\n-\tif (strbuf_read_file(&actual_hex, git_path_bisect_expected_rev(), 0) >= 40) {\n-\t\tstrbuf_trim(&actual_hex);\n-\t\tres = !strcmp(actual_hex.buf, expected_hex);\n-\t}\n-\tstrbuf_release(&actual_hex);\n-\treturn res;\n-}\n-\n-static void check_expected_revs(const char **revs, int rev_nr)\n-{\n-\tint i;\n-\n-\tfor (i = 0; i < rev_nr; i++) {\n-\t\tif (!is_expected_rev(revs[i])) {\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}\n-\t}\n-}\n-\n static int bisect_reset(const char *commit)\n {\n \tstruct strbuf branch = STRBUF_INIT;\n@@ -934,7 +910,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@@ -950,8 +925,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@@ -990,9 +963,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":"404806","messageId":"20200831105043.97665-13-mirucam@gmail.com","threadId":"54167","inReplyTo":"20200831105043.97665-1-mirucam@gmail.com","subject":"[PATCH v7 12/13] bisect--helper: retire `--write-terms` subcommand","fromName":"Miriam Rubio","fromEmail":"mirucam@gmail.com","sentAt":"2020-08-31T10:50:42Z","receivedAt":"2020-08-31T10:52:29Z","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 da203c2091..46ddfd5710 100644\n--- a/builtin/bisect--helper.c\n+++ b/builtin/bisect--helper.c\n@@ -20,7 +20,6 @@ static GIT_PATH_FUNC(git_path_bisect_names, \"BISECT_NAMES\")\n static GIT_PATH_FUNC(git_path_bisect_first_parent, \"BISECT_FIRST_PARENT\")\n \n static const char * const git_bisect_helper_usage[] = {\n-\tN_(\"git bisect--helper --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@@ -909,8 +908,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@@ -923,8 +921,6 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n \t} cmdmode = 0;\n \tint 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@@ -959,10 +955,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":"404807","messageId":"20200831105043.97665-11-mirucam@gmail.com","threadId":"54167","inReplyTo":"20200831105043.97665-1-mirucam@gmail.com","subject":"[PATCH v7 10/13] bisect--helper: reimplement `bisect_state` & `bisect_head` shell functions in C","fromName":"Miriam Rubio","fromEmail":"mirucam@gmail.com","sentAt":"2020-08-31T10:50:40Z","receivedAt":"2020-08-31T10:52:30Z","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 | 78 +++++++++++++++++++++++++++++++++++++++-\n git-bisect.sh | 55 +++-------------------------\n 2 files changed, 81 insertions(+), 52 deletions(-)\n\ndiff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\nindex 0796e51672..a976f4739c 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@@ -862,6 +864,72 @@ static int bisect_autostart(struct bisect_terms *terms)\n \treturn res;\n }\n \n+static enum bisect_error bisect_state(struct bisect_terms *terms, const char **argv,\n+\t\t\t\t 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+\tstruct oid_array revs = OID_ARRAY_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+\tif (argc == 0) {\n+\t\tenum get_oid_result res_head = get_oid(\"BISECT_HEAD\", &oid);\n+\t\tif (res_head == MISSING_OBJECT)\n+\t\t\tres_head = get_oid(\"HEAD\", &oid);\n+\t\tif (res_head) {\n+\t\t\terror(_(\"Bad bisect_head rev input\"));\n+\t\t\treturn BISECT_FAILED;\n+\t\t}\n+\t\toid_array_append(&revs, &oid);\n+\t}\n+\n+\t/*\n+\t * All input revs must be checked before executing bisect_write()\n+\t * to discard junk revs.\n+\t */\n+\n+\tfor (; argc; argc--, argv++) {\n+\t\tif (get_oid(*argv, &oid)){\n+\t\t\terror(_(\"Bad rev input: %s\"), *argv);\n+\t\t\treturn BISECT_FAILED;\n+\t\t}\n+\t\toid_array_append(&revs, &oid);\n+\t}\n+\n+\tif (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+\tstrbuf_release(&buf);\n+\n+\tfor (i = 0; i < revs.nr; i++) {\n+\t\tif (bisect_write(state, oid_to_hex(&revs.oid[i]), terms, 0))\n+\t\t\treturn BISECT_FAILED;\n+\n+\t\tif (verify_expected && !oideq(&revs.oid[i], &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+\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@@ -875,7 +943,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 res = 0, nolog = 0;\n \tstruct option options[] = {\n@@ -901,6 +970,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 it has not yet been started\"), 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-log\", &nolog,\n \t\t\t N_(\"no log for BISECT_WRITE\")),\n \t\tOPT_END()\n@@ -971,6 +1042,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\", cmdmode);\n \t}\ndiff --git a/git-bisect.sh b/git-bisect.sh\nindex 356264caf0..7a8f796251 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 git rev-parse --verify -q BISECT_HEAD > /dev/null\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@@ -187,8 +141,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@@ -203,7 +156,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@@ -244,7 +197,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":"404808","messageId":"20200831105043.97665-10-mirucam@gmail.com","threadId":"54167","inReplyTo":"20200831105043.97665-1-mirucam@gmail.com","subject":"[PATCH v7 09/13] bisect--helper: retire `--next-all` subcommand","fromName":"Miriam Rubio","fromEmail":"mirucam@gmail.com","sentAt":"2020-08-31T10:50:39Z","receivedAt":"2020-08-31T10:52:34Z","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 b8b8a7473c..0796e51672 100644\n--- a/builtin/bisect--helper.c\n+++ b/builtin/bisect--helper.c\n@@ -20,7 +20,6 @@ static GIT_PATH_FUNC(git_path_bisect_names, \"BISECT_NAMES\")\n static GIT_PATH_FUNC(git_path_bisect_first_parent, \"BISECT_FIRST_PARENT\")\n \n static const char * const git_bisect_helper_usage[] = {\n-\tN_(\"git bisect--helper --next-all\"),\n \tN_(\"git bisect--helper --write-terms \"),\n \tN_(\"git bisect--helper --bisect-reset []\"),\n \tN_(\"git bisect--helper --bisect-write [--no-log] \"),\n@@ -866,8 +865,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@@ -881,8 +879,6 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n \t} cmdmode = 0;\n \tint 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@@ -919,9 +915,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);\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":"404809","messageId":"20200831105043.97665-8-mirucam@gmail.com","threadId":"54167","inReplyTo":"20200831105043.97665-1-mirucam@gmail.com","subject":"[PATCH v7 07/13] bisect--helper: finish porting `bisect_start()` to C","fromName":"Miriam Rubio","fromEmail":"mirucam@gmail.com","sentAt":"2020-08-31T10:50:37Z","receivedAt":"2020-08-31T10:52:36Z","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\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 | 56 ++++++++++++++++++++++++++++------------\n git-bisect.sh | 26 ++-----------------\n 2 files changed, 41 insertions(+), 41 deletions(-)\n\ndiff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\nindex e29e86142a..b40369e8aa 100644\n--- a/builtin/bisect--helper.c\n+++ b/builtin/bisect--helper.c\n@@ -85,6 +85,19 @@ static int one_of(const char *term, ...)\n \treturn res;\n }\n \n+/*\n+ * return code BISECT_INTERNAL_SUCCESS_MERGE_BASE\n+ * and BISECT_INTERNAL_SUCCESS_1ST_BAD_FOUND are codes\n+ * that indicate special success.\n+ */\n+\n+static int is_bisect_success(enum bisect_error res)\n+{\n+\treturn !res ||\n+\t\tres == BISECT_INTERNAL_SUCCESS_1ST_BAD_FOUND ||\n+\t\tres == BISECT_INTERNAL_SUCCESS_MERGE_BASE;\n+}\n+\n static int write_in_file(const char *path, const char *mode, const char *format, va_list args)\n {\n \tFILE *fp = NULL;\n@@ -607,12 +620,13 @@ 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, const char **argv, int argc)\n+static enum bisect_error bisect_start(struct bisect_terms *terms, const char **argv, int argc)\n {\n \tint no_checkout = 0;\n \tint first_parent_only = 0;\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@@ -672,9 +686,12 @@ static int bisect_start(struct bisect_terms *terms, const char **argv, int argc)\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@@ -752,14 +769,7 @@ static int bisect_start(struct bisect_terms *terms, const char **argv, int argc)\n \t * Get rid of any old bisect state.\n \t */\n \tif (bisect_clean_state())\n-\t\treturn -1;\n-\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 trap this to be able to clean up using\n-\t * \"bisect_clean_state\".\n-\t */\n+\t\treturn BISECT_FAILED;\n \n \t/*\n \t * Write new start state\n@@ -776,7 +786,7 @@ static int bisect_start(struct bisect_terms *terms, const char **argv, int argc)\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@@ -788,25 +798,37 @@ static int bisect_start(struct bisect_terms *terms, const char **argv, int argc)\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,\n+\t * \"bisect_auto_next\" above 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 */\n+\tif (!is_bisect_success(res))\n+\t\tbisect_clean_state();\n \treturn res;\n }\n \ndiff --git a/git-bisect.sh b/git-bisect.sh\nindex 59424f5b37..356264caf0 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@@ -163,8 +142,7 @@ bisect_replay () {\n \t\tget_terms\n \t\tcase \"$command\" in\n \t\tstart)\n-\t\t\tcmd=\"bisect_start $rev $tail\"\n-\t\t\teval \"$cmd\" ;;\n+\t\t\teval \"git bisect--helper --bisect-start $rev $tail\" ;;\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@@ -264,7 +242,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":"404810","messageId":"20200831105043.97665-7-mirucam@gmail.com","threadId":"54167","inReplyTo":"20200831105043.97665-1-mirucam@gmail.com","subject":"[PATCH v7 06/13] bisect--helper: reimplement `bisect_next` and `bisect_auto_next` shell functions in C","fromName":"Miriam Rubio","fromEmail":"mirucam@gmail.com","sentAt":"2020-08-31T10:50:36Z","receivedAt":"2020-08-31T10:52:38Z","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 | 11 ++-\n builtin/bisect--helper.c | 180 ++++++++++++++++++++++++++++++++++++++-\n git-bisect.sh | 47 +---------\n 3 files changed, 190 insertions(+), 48 deletions(-)\n\ndiff --git a/bisect.c b/bisect.c\nindex c6aba2b9f2..f5b1368128 100644\n--- a/bisect.c\n+++ b/bisect.c\n@@ -988,8 +988,11 @@ 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- * If no_checkout is non-zero, the bisection process does not\n- * checkout the trial commit but instead simply updates BISECT_HEAD.\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 enum bisect_error bisect_next_all(struct repository *r, const char *prefix)\n {\n@@ -999,6 +1002,10 @@ enum bisect_error bisect_next_all(struct repository *r, const char *prefix)\n \tenum bisect_error res = BISECT_OK;\n \tstruct object_id *bisect_rev;\n \tchar *steps_msg;\n+\t/*\n+\t * If no_checkout is non-zero, the bisection process does not\n+\t * checkout the trial commit but instead simply updates BISECT_HEAD.\n+\t */\n \tint no_checkout = ref_exists(\"BISECT_HEAD\");\n \tunsigned bisect_flags = 0;\n \ndiff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\nindex f71e8e54d2..e29e86142a 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,10 +30,17 @@ 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] [--first-parent] [ [...]] [--] [...]\"),\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 \n+struct add_bisect_ref_data {\n+\tstruct rev_info *revs;\n+\tunsigned int object_flags;\n+};\n+\n struct bisect_terms {\n \tchar *term_good;\n \tchar *term_bad;\n@@ -56,6 +64,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@@ -80,7 +90,7 @@ static int write_in_file(const char *path, const char *mode, const char *format,\n \tFILE *fp = NULL;\n \tint res = 0;\n \n-\tif (strcmp(mode, \"w\"))\n+\tif (strcmp(mode, \"w\") && strcmp(mode, \"a\"))\n \t\tBUG(\"write-in-file does not support '%s' mode\", mode);\n \tfp = fopen(path, mode);\n \tif (!fp)\n@@ -109,6 +119,18 @@ static int write_to_file(const char *path, const char *format, ...)\n \treturn res;\n }\n \n+static int append_to_file(const char *path, const char *format, ...)\n+{\n+\tint res;\n+\tva_list args;\n+\n+\tva_start(args, format);\n+\tres = write_in_file(path, \"a\", format, args);\n+\tva_end(args);\n+\n+\treturn res;\n+}\n+\n static int check_term_format(const char *term, const char *orig_term)\n {\n \tint res;\n@@ -451,6 +473,140 @@ static int bisect_append_log_quoted(const char **argv)\n \treturn res;\n }\n \n+static int add_bisect_ref(const char *refname, const struct object_id *oid,\n+\t\t\t int flags, void *cb)\n+{\n+\tstruct add_bisect_ref_data *data = cb;\n+\n+\tadd_pending_oid(data->revs, refname, oid, data->object_flags);\n+\n+\treturn 0;\n+}\n+\n+static int prepare_revs(struct bisect_terms *terms, struct rev_info *revs)\n+{\n+\tint res = 0;\n+\tstruct add_bisect_ref_data cb = { revs };\n+\tchar *good = xstrfmt(\"%s-*\", terms->term_good);\n+\n+\t/*\n+\t * We cannot use terms->term_bad directly in\n+\t * for_each_glob_ref_in() and we have to append a '*' to it,\n+\t * otherwise for_each_glob_ref_in() will append '/' and '*'.\n+\t */\n+\tchar *bad = xstrfmt(\"%s*\", terms->term_bad);\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+\tsetup_revisions(0, NULL, revs, NULL);\n+\tfor_each_glob_ref_in(add_bisect_ref, bad, \"refs/bisect/\", &cb);\n+\tcb.object_flags = UNINTERESTING;\n+\tfor_each_glob_ref_in(add_bisect_ref, good, \"refs/bisect/\", &cb);\n+\tif (prepare_revision_walk(revs))\n+\t\tres = error(_(\"revision walk setup failed\\n\"));\n+\n+\tfree(good);\n+\tfree(bad);\n+\treturn res;\n+}\n+\n+static int bisect_skipped_commits(struct bisect_terms *terms)\n+{\n+\tint res;\n+\tFILE *fp = NULL;\n+\tstruct rev_info revs;\n+\tstruct commit *commit;\n+\tstruct pretty_print_context pp = {0};\n+\tstruct strbuf commit_name = STRBUF_INIT;\n+\n+\tres = prepare_revs(terms, &revs);\n+\tif (res)\n+\t\treturn res;\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+\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\tstrbuf_reset(&commit_name);\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}\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+\tstrbuf_release(&commit_name);\n+\tfclose(fp);\n+\treturn 0;\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+\tcommit = lookup_commit_reference_by_name(bad_ref);\n+\tformat_commit_message(commit, \"%s\", &commit_name, &pp);\n+\n+\tres = append_to_file(git_path_bisect_log(), \"# first %s commit: [%s] %s\\n\",\n+\t\t\t terms->term_bad, oid_to_hex(&commit->object.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+\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+\t/* Perform all bisection computation */\n+\tres = bisect_next_all(the_repository, prefix);\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, const char **argv, int argc)\n {\n \tint no_checkout = 0;\n@@ -699,7 +855,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 res = 0, nolog = 0;\n \tstruct option options[] = {\n@@ -723,6 +881,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 it has not yet been started\"), BISECT_AUTOSTART),\n \t\tOPT_BOOL(0, \"no-log\", &nolog,\n@@ -784,6 +946,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, 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 does not accept arguments\"));\n@@ -799,7 +973,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 9ca583d964..59424f5b37 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\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@@ -213,7 +174,7 @@ bisect_replay () {\n \t\tesac\n \tdone <\"$file\"\n \tIFS=\"$oIFS\"\n-\tbisect_auto_next\n+\tgit bisect--helper --bisect-auto-next\n }\n \n bisect_run () {\n@@ -310,7 +271,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":"404968","messageId":"nycvar.QRO.7.76.6.2009030737080.56@tvgsbejvaqbjf.bet","threadId":"54167","inReplyTo":"20200831105043.97665-5-mirucam@gmail.com","subject":"Re: [PATCH v7 04/13] bisect--helper: reimplement `bisect_autostart` shell function in C","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-09-03T05:50:27Z","receivedAt":"2020-09-03T14:51:47Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Miriam,\n\nOn Mon, 31 Aug 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.\n>\n> Add `--bisect-autostart` subcommand to be called from git-bisect.sh.\n> Using `--bisect-autostart` subcommand is a temporary measure to port\n> the shell function to C so as to use the existing test suite. As more\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 | 44 +++++++++++++++++++++++++++++++++++++++-\n> git-bisect.sh | 25 ++---------------------\n> 2 files changed, 45 insertions(+), 24 deletions(-)\n>\n> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\n> index bae09ce65d..f71e8e54d2 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] [--first-parent] [ [...]] [--] [...]\"),\n> +\tN_(\"git bisect--helper --bisect-autostart\"),\n> \tNULL\n> };\n>\n> @@ -653,6 +654,38 @@ static int bisect_start(struct bisect_terms *terms, const char **argv, int argc)\n> \treturn res;\n> }\n>\n> +static inline int file_is_not_empty(const char *path)\n> +{\n> +\treturn !is_empty_or_missing_file(path);\n> +}\n> +\n> +static int bisect_autostart(struct bisect_terms *terms)\n> +{\n> +\tint res;\n> +\tconst char *yesno;\n> +\n> +\tif (file_is_not_empty(git_path_bisect_start()))\n> +\t\treturn 0;\n> +\n> +\tfprintf_ln(stderr, _(\"You need to start by \\\"git bisect \"\n> +\t\t\t \"start\\\"\\n\"));\n> +\n> +\tif (!isatty(STDIN_FILENO))\n\nNot a big deal, but we have only two callers to `isatty()` that use the\n`_FILENO` constants, and neither of them is about stdin. But that is not a\nbig deal.\n\n> +\t\treturn 0;\n\nHowever, when we cannot auto-start, the shell script version calls `exit\n1` to cause a failure. I think we need to do the same here, i.e. `return\n-1;`.\n\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> +\tres = tolower(*yesno) == 'n' ?\n> +\t\t-1 : bisect_start(terms, empty_strvec, 0);\n\nThe corresponding shell script code reads like this:\n\n read yesno\n case \"$yesno\" in\n [Nn]*)\n exit ;;\n esac\n\nThat is, if the user does not want to start, the command exits. With exit\ncode 0, i.e. success.\n\nHowever, we do not do that here.\n\nNow, you could argue (in the commit message) that this actually fixes a\nbug (because if the bisection was aborted, that does not count for\nsuccess), and I'd be fine with that.\n\n> +\n> +\treturn res;\n> +}\n> +\n> int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n> {\n> \tenum {\n> @@ -665,7 +698,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 res = 0, nolog = 0;\n> \tstruct option options[] = {\n> @@ -689,6 +723,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 it has not yet been started\"), BISECT_AUTOSTART),\n> \t\tOPT_BOOL(0, \"no-log\", &nolog,\n> \t\t\t N_(\"no log for BISECT_WRITE\")),\n> \t\tOPT_END()\n> @@ -748,6 +784,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, argv, argc);\n> \t\tbreak;\n> +\tcase BISECT_AUTOSTART:\n> +\t\tif (argc)\n> +\t\t\treturn error(_(\"--bisect-autostart does not accept 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\", cmdmode);\n> \t}\n> diff --git a/git-bisect.sh b/git-bisect.sh\n> index c7580e51a0..9ca583d964 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\nThis (and the change below as well) is insufficient, as it won't `exit` in\ncase of an error (or in case the user aborted). I think you need to append\n`|| exit` as is done two lines below this line.\n\nCiao,\nDscho\n\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":"404973","messageId":"nycvar.QRO.7.76.6.2009030800000.56@tvgsbejvaqbjf.bet","threadId":"54167","inReplyTo":"20200831105043.97665-7-mirucam@gmail.com","subject":"Re: [PATCH v7 06/13] bisect--helper: reimplement `bisect_next` and `bisect_auto_next` shell functions in C","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-09-03T09:48:46Z","receivedAt":"2020-09-03T18:13:21Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Miriam,\n\nOn Mon, 31 Aug 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 | 11 ++-\n> builtin/bisect--helper.c | 180 ++++++++++++++++++++++++++++++++++++++-\n> git-bisect.sh | 47 +---------\n> 3 files changed, 190 insertions(+), 48 deletions(-)\n>\n> diff --git a/bisect.c b/bisect.c\n> index c6aba2b9f2..f5b1368128 100644\n> --- a/bisect.c\n> +++ b/bisect.c\n> @@ -988,8 +988,11 @@ 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> - * If no_checkout is non-zero, the bisection process does not\n> - * checkout the trial commit but instead simply updates BISECT_HEAD.\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> enum bisect_error bisect_next_all(struct repository *r, const char *prefix)\n> {\n> @@ -999,6 +1002,10 @@ enum bisect_error bisect_next_all(struct repository *r, const char *prefix)\n> \tenum bisect_error res = BISECT_OK;\n> \tstruct object_id *bisect_rev;\n> \tchar *steps_msg;\n> +\t/*\n> +\t * If no_checkout is non-zero, the bisection process does not\n> +\t * checkout the trial commit but instead simply updates BISECT_HEAD.\n> +\t */\n> \tint no_checkout = ref_exists(\"BISECT_HEAD\");\n> \tunsigned bisect_flags = 0;\n>\n> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\n> index f71e8e54d2..e29e86142a 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,10 +30,17 @@ 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] [--first-parent] [ [...]] [--] [...]\"),\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>\n> +struct add_bisect_ref_data {\n> +\tstruct rev_info *revs;\n> +\tunsigned int object_flags;\n> +};\n> +\n> struct bisect_terms {\n> \tchar *term_good;\n> \tchar *term_bad;\n> @@ -56,6 +64,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> @@ -80,7 +90,7 @@ static int write_in_file(const char *path, const char *mode, const char *format,\n> \tFILE *fp = NULL;\n> \tint res = 0;\n>\n> -\tif (strcmp(mode, \"w\"))\n> +\tif (strcmp(mode, \"w\") && strcmp(mode, \"a\"))\n> \t\tBUG(\"write-in-file does not support '%s' mode\", mode);\n> \tfp = fopen(path, mode);\n> \tif (!fp)\n> @@ -109,6 +119,18 @@ static int write_to_file(const char *path, const char *format, ...)\n> \treturn res;\n> }\n>\n> +static int append_to_file(const char *path, const char *format, ...)\n> +{\n> +\tint res;\n> +\tva_list args;\n> +\n> +\tva_start(args, format);\n> +\tres = write_in_file(path, \"a\", format, args);\n> +\tva_end(args);\n> +\n> +\treturn res;\n> +}\n> +\n> static int check_term_format(const char *term, const char *orig_term)\n> {\n> \tint res;\n> @@ -451,6 +473,140 @@ static int bisect_append_log_quoted(const char **argv)\n> \treturn res;\n> }\n>\n> +static int add_bisect_ref(const char *refname, const struct object_id *oid,\n> +\t\t\t int flags, void *cb)\n> +{\n> +\tstruct add_bisect_ref_data *data = cb;\n> +\n> +\tadd_pending_oid(data->revs, refname, oid, data->object_flags);\n> +\n> +\treturn 0;\n> +}\n> +\n> +static int prepare_revs(struct bisect_terms *terms, struct rev_info *revs)\n> +{\n> +\tint res = 0;\n> +\tstruct add_bisect_ref_data cb = { revs };\n> +\tchar *good = xstrfmt(\"%s-*\", terms->term_good);\n> +\n> +\t/*\n> +\t * We cannot use terms->term_bad directly in\n> +\t * for_each_glob_ref_in() and we have to append a '*' to it,\n> +\t * otherwise for_each_glob_ref_in() will append '/' and '*'.\n> +\t */\n> +\tchar *bad = xstrfmt(\"%s*\", terms->term_bad);\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> +\tsetup_revisions(0, NULL, revs, NULL);\n> +\tfor_each_glob_ref_in(add_bisect_ref, bad, \"refs/bisect/\", &cb);\n> +\tcb.object_flags = UNINTERESTING;\n> +\tfor_each_glob_ref_in(add_bisect_ref, good, \"refs/bisect/\", &cb);\n> +\tif (prepare_revision_walk(revs))\n> +\t\tres = error(_(\"revision walk setup failed\\n\"));\n> +\n> +\tfree(good);\n> +\tfree(bad);\n> +\treturn res;\n> +}\n> +\n> +static int bisect_skipped_commits(struct bisect_terms *terms)\n> +{\n> +\tint res;\n> +\tFILE *fp = NULL;\n> +\tstruct rev_info revs;\n> +\tstruct commit *commit;\n> +\tstruct pretty_print_context pp = {0};\n> +\tstruct strbuf commit_name = STRBUF_INIT;\n> +\n> +\tres = prepare_revs(terms, &revs);\n> +\tif (res)\n> +\t\treturn res;\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> +\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\tstrbuf_reset(&commit_name);\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}\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> +\tstrbuf_release(&commit_name);\n> +\tfclose(fp);\n> +\treturn 0;\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> +\tcommit = lookup_commit_reference_by_name(bad_ref);\n> +\tformat_commit_message(commit, \"%s\", &commit_name, &pp);\n> +\n> +\tres = append_to_file(git_path_bisect_log(), \"# first %s commit: [%s] %s\\n\",\n> +\t\t\t terms->term_bad, oid_to_hex(&commit->object.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> +\tenum bisect_error res;\n> +\n> +\tbisect_autostart(terms);\n\nWe need to handle a negative value returned by `bisect_autostart()` by\naborting, e.g. `return BISECT_FAILED;`.\n\n> +\tif (bisect_next_check(terms, terms->term_good))\n> +\t\treturn BISECT_FAILED;\n> +\n> +\t/* Perform all bisection computation */\n> +\tres = bisect_next_all(the_repository, prefix);\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\nThis returns an error when `bisect_next()` failed, which is different from\nwhat the shell script version did:\n\n\tgit bisect--helper --bisect-next-check $TERM_GOOD $TERM_BAD && bisect_next || :\n\n(note the trailing `|| :`, which will always return success).\n\nI think this is a bug fix, though, so the change is good. But it probably\ndeserves to be called out in the commit message.\n\n> +}\n> +\n> static int bisect_start(struct bisect_terms *terms, const char **argv, int argc)\n> {\n> \tint no_checkout = 0;\n> @@ -699,7 +855,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\nI am curious: what is the reason not to use a trailing comma, and what is\nthe reason to force AUTOSTART to stay at the end (instead of appending the\nnew modes, which strikes me as more natural)?\n\n> \t} cmdmode = 0;\n> \tint res = 0, nolog = 0;\n> \tstruct option options[] = {\n> @@ -723,6 +881,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 it has not yet been started\"), BISECT_AUTOSTART),\n> \t\tOPT_BOOL(0, \"no-log\", &nolog,\n> @@ -784,6 +946,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, 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 does not accept arguments\"));\n> @@ -799,7 +973,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 9ca583d964..59424f5b37 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\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> @@ -213,7 +174,7 @@ bisect_replay () {\n> \t\tesac\n> \tdone <\"$file\"\n> \tIFS=\"$oIFS\"\n> -\tbisect_auto_next\n> +\tgit bisect--helper --bisect-auto-next\n\nWe probably need to append `|| exit` here.\n\nThank you,\nJohannes\n\n> }\n>\n> bisect_run () {\n> @@ -310,7 +271,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":"404974","messageId":"nycvar.QRO.7.76.6.2009031151500.56@tvgsbejvaqbjf.bet","threadId":"54167","inReplyTo":"20200831105043.97665-8-mirucam@gmail.com","subject":"Re: [PATCH v7 07/13] bisect--helper: finish porting `bisect_start()` to C","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-09-03T09:56:50Z","receivedAt":"2020-09-03T18:21:22Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Miriam,\n\nOn Mon, 31 Aug 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> 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 | 56 ++++++++++++++++++++++++++++------------\n> git-bisect.sh | 26 ++-----------------\n> 2 files changed, 41 insertions(+), 41 deletions(-)\n>\n> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\n> index e29e86142a..b40369e8aa 100644\n> --- a/builtin/bisect--helper.c\n> +++ b/builtin/bisect--helper.c\n> @@ -85,6 +85,19 @@ static int one_of(const char *term, ...)\n> \treturn res;\n> }\n>\n> +/*\n> + * return code BISECT_INTERNAL_SUCCESS_MERGE_BASE\n> + * and BISECT_INTERNAL_SUCCESS_1ST_BAD_FOUND are codes\n> + * that indicate special success.\n> + */\n> +\n> +static int is_bisect_success(enum bisect_error res)\n> +{\n> +\treturn !res ||\n> +\t\tres == BISECT_INTERNAL_SUCCESS_1ST_BAD_FOUND ||\n> +\t\tres == BISECT_INTERNAL_SUCCESS_MERGE_BASE;\n> +}\n> +\n> static int write_in_file(const char *path, const char *mode, const char *format, va_list args)\n> {\n> \tFILE *fp = NULL;\n> @@ -607,12 +620,13 @@ 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, const char **argv, int argc)\n> +static enum bisect_error bisect_start(struct bisect_terms *terms, const char **argv, int argc)\n> {\n> \tint no_checkout = 0;\n> \tint first_parent_only = 0;\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> @@ -672,9 +686,12 @@ static int bisect_start(struct bisect_terms *terms, const char **argv, int argc)\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> @@ -752,14 +769,7 @@ static int bisect_start(struct bisect_terms *terms, const char **argv, int argc)\n> \t * Get rid of any old bisect state.\n> \t */\n> \tif (bisect_clean_state())\n> -\t\treturn -1;\n> -\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 trap this to be able to clean up using\n> -\t * \"bisect_clean_state\".\n> -\t */\n> +\t\treturn BISECT_FAILED;\n>\n> \t/*\n> \t * Write new start state\n> @@ -776,7 +786,7 @@ static int bisect_start(struct bisect_terms *terms, const char **argv, int argc)\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> @@ -788,25 +798,37 @@ static int bisect_start(struct bisect_terms *terms, const char **argv, int argc)\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,\n> +\t * \"bisect_auto_next\" above 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 */\n\nIs this comment still correct in the C version? I thought we had succeeded\nin removing that shell scripting wart by turning all the `exit`s into\n`return`s...\n\n> +\tif (!is_bisect_success(res))\n> +\t\tbisect_clean_state();\n> \treturn res;\n> }\n>\n> diff --git a/git-bisect.sh b/git-bisect.sh\n> index 59424f5b37..356264caf0 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> @@ -163,8 +142,7 @@ bisect_replay () {\n> \t\tget_terms\n> \t\tcase \"$command\" in\n> \t\tstart)\n> -\t\t\tcmd=\"bisect_start $rev $tail\"\n> -\t\t\teval \"$cmd\" ;;\n> +\t\t\teval \"git bisect--helper --bisect-start $rev $tail\" ;;\n\nI think we need to append something like `test 0 = $? || exit $?` here, as\nthe `eval`ed command can no longer exit.\n\nIn fact, I think that the `eval` here no longer makes sense, as the only\nreason we did that before was that `bisect_start` could `exit` and we did\nnot want that here.\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> @@ -264,7 +242,7 @@ case \"$#\" in\n> \thelp)\n> \t\tgit bisect -h ;;\n> \tstart)\n> -\t\tbisect_start \"$@\" ;;\n> +\t\tgit bisect--helper --bisect-start \"$@\" ;;\n\nI guess we do not need to append `|| exit` here because this will be the\nlast command in the script to be executed anyway, therefore its exit\nstatus will become the exit status of the script.\n\nThank you,\nJohannes\n\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":"404983","messageId":"nycvar.QRO.7.76.6.2009031158080.56@tvgsbejvaqbjf.bet","threadId":"54167","inReplyTo":"20200831105043.97665-11-mirucam@gmail.com","subject":"Re: [PATCH v7 10/13] bisect--helper: reimplement `bisect_state` & `bisect_head` shell functions in C","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-09-03T12:03:16Z","receivedAt":"2020-09-03T20:27:51Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Miriam,\n\nOn Mon, 31 Aug 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 | 78 +++++++++++++++++++++++++++++++++++++++-\n> git-bisect.sh | 55 +++-------------------------\n> 2 files changed, 81 insertions(+), 52 deletions(-)\n>\n> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\n> index 0796e51672..a976f4739c 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> @@ -862,6 +864,72 @@ static int bisect_autostart(struct bisect_terms *terms)\n> \treturn res;\n> }\n>\n> +static enum bisect_error bisect_state(struct bisect_terms *terms, const char **argv,\n> +\t\t\t\t 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> +\tstruct oid_array revs = OID_ARRAY_INIT;\n> +\n> +\tif (!argc)\n> +\t\treturn error(_(\"Please call `--bisect-state` with at least one argument\"));\n\nGood. Technically, this is a deviation from the scripted version, but it\nis good not to check and set the terms first, only to then error out\nanyway.\n\nHowever, I think we will need a call to `bisect_autostart()` somewhere\naround here, as that is what the scripted version did first thing.\n\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> +\tif (argc == 0) {\n> +\t\tenum get_oid_result res_head = get_oid(\"BISECT_HEAD\", &oid);\n> +\t\tif (res_head == MISSING_OBJECT)\n> +\t\t\tres_head = get_oid(\"HEAD\", &oid);\n> +\t\tif (res_head) {\n> +\t\t\terror(_(\"Bad bisect_head rev input\"));\n\nIf I am not mistaken, we should convert this shell command more faithfully\nhere:\n\n\t\t\tdie \"$(eval_gettext \"Bad rev input: \\$bisected_head\")\"\n\ni.e. something like this:\n\n\t\tconst char *head = \"BISECT_HEAD\";\n\t\tenum get_oid_result res_head = get_oid(head, &oid);\n\n\t\tif (res_head == MISSING_OBJECT) {\n\t\t\thead = \"HEAD\";\n\t\t\tres_head = get_oid(head, &oid);\n\t\t}\n\n\t\tif (res_head)\n\t\t\terror(_(\"Bad rev input: %s\"), head);\n\n> +\t\t\treturn BISECT_FAILED;\n> +\t\t}\n> +\t\toid_array_append(&revs, &oid);\n> +\t}\n> +\n> +\t/*\n> +\t * All input revs must be checked before executing bisect_write()\n> +\t * to discard junk revs.\n> +\t */\n> +\n> +\tfor (; argc; argc--, argv++) {\n> +\t\tif (get_oid(*argv, &oid)){\n> +\t\t\terror(_(\"Bad rev input: %s\"), *argv);\n> +\t\t\treturn BISECT_FAILED;\n> +\t\t}\n> +\t\toid_array_append(&revs, &oid);\n> +\t}\n> +\n> +\tif (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> +\tstrbuf_release(&buf);\n> +\n> +\tfor (i = 0; i < revs.nr; i++) {\n> +\t\tif (bisect_write(state, oid_to_hex(&revs.oid[i]), terms, 0))\n> +\t\t\treturn BISECT_FAILED;\n> +\n> +\t\tif (verify_expected && !oideq(&revs.oid[i], &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\nNice! This reads so much easier than the shell script version.\n\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> @@ -875,7 +943,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 res = 0, nolog = 0;\n> \tstruct option options[] = {\n> @@ -901,6 +970,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 it has not yet been started\"), 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-log\", &nolog,\n> \t\t\t N_(\"no log for BISECT_WRITE\")),\n> \t\tOPT_END()\n> @@ -971,6 +1042,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\", cmdmode);\n> \t}\n> diff --git a/git-bisect.sh b/git-bisect.sh\n> index 356264caf0..7a8f796251 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 git rev-parse --verify -q BISECT_HEAD > /dev/null\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\nSince `bisect--helper` cannot cause the shell script to `exit`, the `eval`\nshould no longer be necessary.\n\n> }\n>\n> bisect_visualize() {\n> @@ -187,8 +141,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> @@ -203,7 +156,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\nI don't think we should include implementation details such as the use of\n`bisect--helper` in the error message. Personally, I would either keep\n`bisect_state` or paraphrase what failed in English to begin with.\n\nThank you,\nJohannes\n\n> \t\t\texit $res\n> \t\tfi\n>\n> @@ -244,7 +197,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":"404984","messageId":"nycvar.QRO.7.76.6.2009031403510.56@tvgsbejvaqbjf.bet","threadId":"54167","inReplyTo":"20200831105043.97665-1-mirucam@gmail.com","subject":"Re: [PATCH v7 00/13] Finish converting git bisect to C part 2","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-09-03T12:04:39Z","receivedAt":"2020-09-03T20:34:16Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Miriam,\n\n\nOn Mon, 31 Aug 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-v7.\n\nExcellent progress, and thank you for your patience and diligence working\non this patch series. I think we are _really_ close to being ready for\n`next`.\n\nThanks!\nJohannes\n\n>\n> I would like to thank Junio Hamano for reviewing this patch series.\n>\n> General changes\n> ---------------\n>\n> * Rebased on e9b77c84a0 (Tenth batch, 2020-08-24), to build on the\n> strvec API (instead of argv_array) and adjust to the codebase\n> after the \"--first-parent\" feature was added.\n>\n>\n> Specific changes\n> ----------------\n>\n> [1/13] bisect--helper: BUG() in cmd_*() on invalid subcommand\n>\n> * Amend commit message.\n> * Remove casting to int.\n>\n> ---\n>\n> [2/13] bisect--helper: use '-res' in 'cmd_bisect__helper' return\n>\n> * Amend commit message.\n>\n> ---\n>\n> [3/13] bisect--helper: introduce new `write_in_file()` function\n>\n> * Use saved_errno variable.\n>\n> ---\n>\n> [5/13] bisect--helper: reimplement `bisect_autostart` shell function in C\n>\n> * Fix bug using empty_strvec instead of NULL in a `bisect_start()` call.\n>\n> ---\n>\n>\n> [6/13] bisect--helper: reimplement `bisect_next` and `bisect_auto_next` shell\n> functions in C\n>\n> * Remove unused `no-checkout` variable.\n> * Move a comment to more appropriate place.\n>\n> ---\n>\n> .iriam Rubio (4):\n> bisect--helper: BUG() in cmd_*() on invalid subcommand\n> bisect--helper: use '-res' in 'cmd_bisect__helper' return\n> bisect--helper: introduce new `write_in_file()` function\n> bisect: call 'clear_commit_marks_all()' in 'bisect_next_all()'\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 | 13 +-\n> builtin/bisect--helper.c | 442 ++++++++++++++++++++++++++++++++-------\n> git-bisect.sh | 145 +------------\n> 3 files changed, 380 insertions(+), 220 deletions(-)\n>\n> --\n> 2.25.0\n>\n>\n"},{"id":"406169","messageId":"CAN7CjDDVp_i7dhpbAq5zrGW69nE6+SfivJQ-dembmu+WyqKiQQ@mail.gmail.com","threadId":"54167","inReplyTo":"nycvar.QRO.7.76.6.2009031403510.56@tvgsbejvaqbjf.bet","subject":"Re: [PATCH v7 00/13] Finish converting git bisect to C part 2","fromName":"Miriam R.","fromEmail":"mirucam@gmail.com","sentAt":"2020-09-23T07:26:58Z","receivedAt":"2020-09-23T07:27:12Z","isPatch":true,"sender":{"key":"mirucam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56339109?v=4"},"body":"Hi Johannes,\n\nEl jue., 3 sept. 2020 a las 22:29, Johannes Schindelin\n() escribió:\n>\n> Hi Miriam,\n>\n>\n> On Mon, 31 Aug 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-v7.\n>\n> Excellent progress, and thank you for your patience and diligence working\n> on this patch series. I think we are _really_ close to being ready for\n> `next`.\n>\nThank you for your comments and encouragement.\nApplying some of your suggestions related to removing some 'eval' in\ngit-bisect shell script, a bug has appeared. It seems it is related to\na previous code merged before my internship.\nChristian Couder will take over this fix, but in the meantime I will\nsend a subset of this part2, with the first six patches of this patch\nseries (and that are not affected by the problem) in order to move\nthem forward and be accepted, hopefully .\nBest,\nMiriam\n\n\n\n\n> Thanks!\n> Johannes\n>\n> >\n> > I would like to thank Junio Hamano for reviewing this patch series.\n> >\n> > General changes\n> > ---------------\n> >\n> > * Rebased on e9b77c84a0 (Tenth batch, 2020-08-24), to build on the\n> > strvec API (instead of argv_array) and adjust to the codebase\n> > after the \"--first-parent\" feature was added.\n> >\n> >\n> > Specific changes\n> > ----------------\n> >\n> > [1/13] bisect--helper: BUG() in cmd_*() on invalid subcommand\n> >\n> > * Amend commit message.\n> > * Remove casting to int.\n> >\n> > ---\n> >\n> > [2/13] bisect--helper: use '-res' in 'cmd_bisect__helper' return\n> >\n> > * Amend commit message.\n> >\n> > ---\n> >\n> > [3/13] bisect--helper: introduce new `write_in_file()` function\n> >\n> > * Use saved_errno variable.\n> >\n> > ---\n> >\n> > [5/13] bisect--helper: reimplement `bisect_autostart` shell function in C\n> >\n> > * Fix bug using empty_strvec instead of NULL in a `bisect_start()` call.\n> >\n> > ---\n> >\n> >\n> > [6/13] bisect--helper: reimplement `bisect_next` and `bisect_auto_next` shell\n> > functions in C\n> >\n> > * Remove unused `no-checkout` variable.\n> > * Move a comment to more appropriate place.\n> >\n> > ---\n> >\n> > .iriam Rubio (4):\n> > bisect--helper: BUG() in cmd_*() on invalid subcommand\n> > bisect--helper: use '-res' in 'cmd_bisect__helper' return\n> > bisect--helper: introduce new `write_in_file()` function\n> > bisect: call 'clear_commit_marks_all()' in 'bisect_next_all()'\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 | 13 +-\n> > builtin/bisect--helper.c | 442 ++++++++++++++++++++++++++++++++-------\n> > git-bisect.sh | 145 +------------\n> > 3 files changed, 380 insertions(+), 220 deletions(-)\n> >\n> > --\n> > 2.25.0\n> >\n> >\n"},{"id":"406213","messageId":"nycvar.QRO.7.76.6.2009231647370.5061@tvgsbejvaqbjf.bet","threadId":"54167","inReplyTo":"CAN7CjDDVp_i7dhpbAq5zrGW69nE6+SfivJQ-dembmu+WyqKiQQ@mail.gmail.com","subject":"Re: [PATCH v7 00/13] Finish converting git bisect to C part 2","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-09-23T14:48:38Z","receivedAt":"2020-09-23T18:51:00Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Miriam,\n\nOn Wed, 23 Sep 2020, Miriam R. wrote:\n\n> Applying some of your suggestions related to removing some 'eval' in\n> git-bisect shell script, a bug has appeared. It seems it is related to\n> a previous code merged before my internship.\n\nNow you got me curious: what bug did you see?\n\n> Christian Couder will take over this fix, but in the meantime I will\n> send a subset of this part2, with the first six patches of this patch\n> series (and that are not affected by the problem) in order to move\n> them forward and be accepted, hopefully .\n\nOkay, good. Let me know if I can help!\n\nThanks,\nDscho\n"},{"id":"406223","messageId":"nycvar.QRO.7.76.6.2009232316570.5061@tvgsbejvaqbjf.bet","threadId":"54167","inReplyTo":"nycvar.QRO.7.76.6.2009231647370.5061@tvgsbejvaqbjf.bet","subject":"Re: [PATCH v7 00/13] Finish converting git bisect to C part 2","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-09-23T21:23:49Z","receivedAt":"2020-09-23T21:23:54Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Miriam,\n\nOn Wed, 23 Sep 2020, Johannes Schindelin wrote:\n\n> On Wed, 23 Sep 2020, Miriam R. wrote:\n>\n> > Applying some of your suggestions related to removing some 'eval' in\n> > git-bisect shell script, a bug has appeared. It seems it is related to\n> > a previous code merged before my internship.\n>\n> Now you got me curious: what bug did you see?\n\nI found your fork and ran the test, and this is the first symptom:\n\n-- snip --\n[...]\n++ git bisect skip\nBisecting: 1 revision left to test after this (roughly 1 step)\n[32a594a3fdac2d57cf6d02987e30eec68511498c] Add <4: Ciao for now> into .\n++ git bisect good\n++ grep '3082e11d3a0f2edca194c8ce1eb802256e38e75e is the first bad commit' my_bisect_log.txt\n3082e11d3a0f2edca194c8ce1eb802256e38e75e is the first bad commit\n++ git bisect log\n++ git bisect reset\nPrevious HEAD position was 32a594a Add <4: Ciao for now> into .\nSwitched to branch 'other'\nok 22 - bisect skip: add line and then a new test\n\nexpecting success of 6030.23 'bisect skip and bisect replay':\n git bisect replay log_to_replay.txt > my_bisect_log.txt &&\n grep \"$HASH5 is the first bad commit\" my_bisect_log.txt &&\n git bisect reset\n\n++ git bisect replay log_to_replay.txt\nerror: update_ref failed for ref 'refs/bisect/bad': cannot update ref 'refs/bisect/bad': trying to write ref 'refs/bisect/bad' with nonexistent object 10006d020000000068986d020000000054f65f00\nerror: last command exited with $?=1\nnot ok 23 - bisect skip and bisect replay\n#\n# git bisect replay log_to_replay.txt > my_bisect_log.txt &&\n# grep \"$HASH5 is the first bad commit\" my_bisect_log.txt &&\n# git bisect reset\n-- snap --\n\nSo I dug a little bit further (and applied Christian's patch in the\nmeantime), and it turns out that the `eval` has nothing to do with what I\noriginally thought it would be required for: I thought that it wanted to\nprevent `exit` calls from actually exiting the script.\n\nInstead, those `eval` calls are required because the arguments are\nprovided in quoted form. For example, during the execution of t6030.68,\nthe `eval` would expand the call\n\n\teval \"git bisect--helper --bisect-start $rev $tail\"\n\nto\n\n\tgit bisect--helper --bisect-start '--term-old' 'term2' '--term-new' 'term1'\n\nTherefore, the `eval` really needs to stay in place (also the other `eval`\nI had originally suggested to remove, for the same reason).\n\nI would still recommend appending `|| exit`, even if it just so happens\nthat we will eventually abort when the `bisect--helper` command failed\nanyway, because the next command will then fail, and abort. But it's\ncleaner to abort already when this invocation failed rather than relying\non that side effect.\n\nCiao,\nDscho\n\n"},{"id":"406233","messageId":"CAP8UFD1fXBChG3691HrKmbN-4vThCE7=0bL+Ty8-u4FFGTn2tQ@mail.gmail.com","threadId":"54167","inReplyTo":"nycvar.QRO.7.76.6.2009232316570.5061@tvgsbejvaqbjf.bet","subject":"Re: [PATCH v7 00/13] Finish converting git bisect to C part 2","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2020-09-24T05:33:57Z","receivedAt":"2020-09-24T05:34:12Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"HI Dscho,\n\nOn Wed, Sep 23, 2020 at 11:26 PM Johannes Schindelin\n wrote:\n\n> On Wed, 23 Sep 2020, Johannes Schindelin wrote:\n>\n> > On Wed, 23 Sep 2020, Miriam R. wrote:\n> >\n> > > Applying some of your suggestions related to removing some 'eval' in\n> > > git-bisect shell script, a bug has appeared. It seems it is related to\n> > > a previous code merged before my internship.\n> >\n> > Now you got me curious: what bug did you see?\n>\n> I found your fork and ran the test, and this is the first symptom:\n>\n> -- snip --\n> [...]\n> ++ git bisect skip\n> Bisecting: 1 revision left to test after this (roughly 1 step)\n> [32a594a3fdac2d57cf6d02987e30eec68511498c] Add <4: Ciao for now> into .\n> ++ git bisect good\n> ++ grep '3082e11d3a0f2edca194c8ce1eb802256e38e75e is the first bad commit' my_bisect_log.txt\n> 3082e11d3a0f2edca194c8ce1eb802256e38e75e is the first bad commit\n> ++ git bisect log\n> ++ git bisect reset\n> Previous HEAD position was 32a594a Add <4: Ciao for now> into .\n> Switched to branch 'other'\n> ok 22 - bisect skip: add line and then a new test\n>\n> expecting success of 6030.23 'bisect skip and bisect replay':\n> git bisect replay log_to_replay.txt > my_bisect_log.txt &&\n> grep \"$HASH5 is the first bad commit\" my_bisect_log.txt &&\n> git bisect reset\n>\n> ++ git bisect replay log_to_replay.txt\n> error: update_ref failed for ref 'refs/bisect/bad': cannot update ref 'refs/bisect/bad': trying to write ref 'refs/bisect/bad' with nonexistent object 10006d020000000068986d020000000054f65f00\n> error: last command exited with $?=1\n> not ok 23 - bisect skip and bisect replay\n> #\n> # git bisect replay log_to_replay.txt > my_bisect_log.txt &&\n> # grep \"$HASH5 is the first bad commit\" my_bisect_log.txt &&\n> # git bisect reset\n> -- snap --\n>\n> So I dug a little bit further (and applied Christian's patch in the\n> meantime), and it turns out that the `eval` has nothing to do with what I\n> originally thought it would be required for: I thought that it wanted to\n> prevent `exit` calls from actually exiting the script.\n>\n> Instead, those `eval` calls are required because the arguments are\n> provided in quoted form. For example, during the execution of t6030.68,\n> the `eval` would expand the call\n>\n> eval \"git bisect--helper --bisect-start $rev $tail\"\n>\n> to\n>\n> git bisect--helper --bisect-start '--term-old' 'term2' '--term-new' 'term1'\n\nYeah, that was also what I found (along with the bug I sent a patch for).\n\n> Therefore, the `eval` really needs to stay in place (also the other `eval`\n> I had originally suggested to remove, for the same reason).\n>\n> I would still recommend appending `|| exit`, even if it just so happens\n> that we will eventually abort when the `bisect--helper` command failed\n> anyway, because the next command will then fail, and abort. But it's\n> cleaner to abort already when this invocation failed rather than relying\n> on that side effect.\n\nYeah, I think it's a good solution.\n\nThanks for taking a look,\nChristian.\n"},{"id":"406249","messageId":"nycvar.QRO.7.76.6.2009240952300.5061@tvgsbejvaqbjf.bet","threadId":"54167","inReplyTo":"CAP8UFD1fXBChG3691HrKmbN-4vThCE7=0bL+Ty8-u4FFGTn2tQ@mail.gmail.com","subject":"Re: [PATCH v7 00/13] Finish converting git bisect to C part 2","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-09-24T07:56:38Z","receivedAt":"2020-09-24T10:06:49Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Christian,\n\nOn Thu, 24 Sep 2020, Christian Couder wrote:\n\n> On Wed, Sep 23, 2020 at 11:26 PM Johannes Schindelin\n> wrote:\n>\n> > On Wed, 23 Sep 2020, Johannes Schindelin wrote:\n> >\n> > > On Wed, 23 Sep 2020, Miriam R. wrote:\n> > >\n> > > > Applying some of your suggestions related to removing some 'eval' in\n> > > > git-bisect shell script, a bug has appeared. It seems it is related to\n> > > > a previous code merged before my internship.\n> > >\n> > > Now you got me curious: what bug did you see?\n> >\n> > I found your fork and ran the test, and this is the first symptom:\n> >\n> > -- snip --\n> > [...]\n> > ++ git bisect skip\n> > Bisecting: 1 revision left to test after this (roughly 1 step)\n> > [32a594a3fdac2d57cf6d02987e30eec68511498c] Add <4: Ciao for now> into .\n> > ++ git bisect good\n> > ++ grep '3082e11d3a0f2edca194c8ce1eb802256e38e75e is the first bad commit' my_bisect_log.txt\n> > 3082e11d3a0f2edca194c8ce1eb802256e38e75e is the first bad commit\n> > ++ git bisect log\n> > ++ git bisect reset\n> > Previous HEAD position was 32a594a Add <4: Ciao for now> into .\n> > Switched to branch 'other'\n> > ok 22 - bisect skip: add line and then a new test\n> >\n> > expecting success of 6030.23 'bisect skip and bisect replay':\n> > git bisect replay log_to_replay.txt > my_bisect_log.txt &&\n> > grep \"$HASH5 is the first bad commit\" my_bisect_log.txt &&\n> > git bisect reset\n> >\n> > ++ git bisect replay log_to_replay.txt\n> > error: update_ref failed for ref 'refs/bisect/bad': cannot update ref 'refs/bisect/bad': trying to write ref 'refs/bisect/bad' with nonexistent object 10006d020000000068986d020000000054f65f00\n> > error: last command exited with $?=1\n> > not ok 23 - bisect skip and bisect replay\n> > #\n> > # git bisect replay log_to_replay.txt > my_bisect_log.txt &&\n> > # grep \"$HASH5 is the first bad commit\" my_bisect_log.txt &&\n> > # git bisect reset\n> > -- snap --\n> >\n> > So I dug a little bit further (and applied Christian's patch in the\n> > meantime), and it turns out that the `eval` has nothing to do with what I\n> > originally thought it would be required for: I thought that it wanted to\n> > prevent `exit` calls from actually exiting the script.\n> >\n> > Instead, those `eval` calls are required because the arguments are\n> > provided in quoted form. For example, during the execution of t6030.68,\n> > the `eval` would expand the call\n> >\n> > eval \"git bisect--helper --bisect-start $rev $tail\"\n> >\n> > to\n> >\n> > git bisect--helper --bisect-start '--term-old' 'term2' '--term-new' 'term1'\n>\n> Yeah, that was also what I found (along with the bug I sent a patch for).\n\nI suspected that you had found that out, but I really wanted a record on\nthe Git mailing list about our findings.\n\nIt might be a good idea to add a paragraph to the respective patches,\nalong these lines:\n\n\tNote that the `eval` in the changed line of `git-bisect.sh` cannot be\n\tdropped: it is necessary because the `rev` and the `tail`\n\tvariables may contain multiple, quoted arguments that need to be\n\tpassed to `bisect--helper` (without the quotes, naturally).\n\n> > Therefore, the `eval` really needs to stay in place (also the other `eval`\n> > I had originally suggested to remove, for the same reason).\n> >\n> > I would still recommend appending `|| exit`, even if it just so happens\n> > that we will eventually abort when the `bisect--helper` command failed\n> > anyway, because the next command will then fail, and abort. But it's\n> > cleaner to abort already when this invocation failed rather than relying\n> > on that side effect.\n>\n> Yeah, I think it's a good solution.\n\nExcellent. I think we can actually move forward with the entire patch\nseries now, not just the first subset, right?\n\nCiao,\nDscho\n"},{"id":"406252","messageId":"CAP8UFD3rLTLkbq+gYPSD3dmGituWr1g8ZtQDQmPYDdEBtHkU0Q@mail.gmail.com","threadId":"54167","inReplyTo":"nycvar.QRO.7.76.6.2009240952300.5061@tvgsbejvaqbjf.bet","subject":"Re: [PATCH v7 00/13] Finish converting git bisect to C part 2","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2020-09-24T10:46:16Z","receivedAt":"2020-09-24T10:46:30Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Hi Dscho,\n\nOn Thu, Sep 24, 2020 at 12:06 PM Johannes Schindelin\n wrote:\n\n> On Thu, 24 Sep 2020, Christian Couder wrote:\n>\n> > On Wed, Sep 23, 2020 at 11:26 PM Johannes Schindelin\n> > wrote:\n\n> > > Instead, those `eval` calls are required because the arguments are\n> > > provided in quoted form. For example, during the execution of t6030.68,\n> > > the `eval` would expand the call\n> > >\n> > > eval \"git bisect--helper --bisect-start $rev $tail\"\n> > >\n> > > to\n> > >\n> > > git bisect--helper --bisect-start '--term-old' 'term2' '--term-new' 'term1'\n> >\n> > Yeah, that was also what I found (along with the bug I sent a patch for).\n>\n> I suspected that you had found that out, but I really wanted a record on\n> the Git mailing list about our findings.\n>\n> It might be a good idea to add a paragraph to the respective patches,\n> along these lines:\n>\n> Note that the `eval` in the changed line of `git-bisect.sh` cannot be\n> dropped: it is necessary because the `rev` and the `tail`\n> variables may contain multiple, quoted arguments that need to be\n> passed to `bisect--helper` (without the quotes, naturally).\n\nYeah, sure. Hopefully Miriam will send this in the commit message of\nthe right patch which is in the subset of the patch series she hasn't\nsent.\n\n> > > Therefore, the `eval` really needs to stay in place (also the other `eval`\n> > > I had originally suggested to remove, for the same reason).\n> > >\n> > > I would still recommend appending `|| exit`, even if it just so happens\n> > > that we will eventually abort when the `bisect--helper` command failed\n> > > anyway, because the next command will then fail, and abort. But it's\n> > > cleaner to abort already when this invocation failed rather than relying\n> > > on that side effect.\n> >\n> > Yeah, I think it's a good solution.\n>\n> Excellent. I think we can actually move forward with the entire patch\n> series now, not just the first subset, right?\n\nYeah, I think so too.\n\nThanks,\nChristian.\n"},{"id":"406262","messageId":"CAN7CjDA5d4o0RxFmGZkMOiG12AqgxMp5uWf0-qUwYpVNb-oj1Q@mail.gmail.com","threadId":"54167","inReplyTo":"CAP8UFD3rLTLkbq+gYPSD3dmGituWr1g8ZtQDQmPYDdEBtHkU0Q@mail.gmail.com","subject":"Re: [PATCH v7 00/13] Finish converting git bisect to C part 2","fromName":"Miriam R.","fromEmail":"mirucam@gmail.com","sentAt":"2020-09-24T12:53:20Z","receivedAt":"2020-09-24T12:53:33Z","isPatch":true,"sender":{"key":"mirucam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56339109?v=4"},"body":"Hi,\n\nEl jue., 24 sept. 2020 a las 12:46, Christian Couder\n() escribió:\n>\n> Hi Dscho,\n>\n> On Thu, Sep 24, 2020 at 12:06 PM Johannes Schindelin\n> wrote:\n>\n> > On Thu, 24 Sep 2020, Christian Couder wrote:\n> >\n> > > On Wed, Sep 23, 2020 at 11:26 PM Johannes Schindelin\n> > > wrote:\n>\n> > > > Instead, those `eval` calls are required because the arguments are\n> > > > provided in quoted form. For example, during the execution of t6030.68,\n> > > > the `eval` would expand the call\n> > > >\n> > > > eval \"git bisect--helper --bisect-start $rev $tail\"\n> > > >\n> > > > to\n> > > >\n> > > > git bisect--helper --bisect-start '--term-old' 'term2' '--term-new' 'term1'\n> > >\n> > > Yeah, that was also what I found (along with the bug I sent a patch for).\n> >\n> > I suspected that you had found that out, but I really wanted a record on\n> > the Git mailing list about our findings.\n> >\n> > It might be a good idea to add a paragraph to the respective patches,\n> > along these lines:\n> >\n> > Note that the `eval` in the changed line of `git-bisect.sh` cannot be\n> > dropped: it is necessary because the `rev` and the `tail`\n> > variables may contain multiple, quoted arguments that need to be\n> > passed to `bisect--helper` (without the quotes, naturally).\n>\n> Yeah, sure. Hopefully Miriam will send this in the commit message of\n> the right patch which is in the subset of the patch series she hasn't\n> sent.\nOk. Noted!!\n>\n> > > > Therefore, the `eval` really needs to stay in place (also the other `eval`\n> > > > I had originally suggested to remove, for the same reason).\n> > > >\n> > > > I would still recommend appending `|| exit`, even if it just so happens\n> > > > that we will eventually abort when the `bisect--helper` command failed\n> > > > anyway, because the next command will then fail, and abort. But it's\n> > > > cleaner to abort already when this invocation failed rather than relying\n> > > > on that side effect.\n> > >\n> > > Yeah, I think it's a good solution.\n> >\n> > Excellent. I think we can actually move forward with the entire patch\n> > series now, not just the first subset, right?\n>\n> Yeah, I think so too.\nOops, I have already sent the first subset.\n\nBest,\nMiriam\n>\n> Thanks,\n> Christian.\n"},{"id":"406263","messageId":"CAN7CjDBSn1FoTFEeqEaGJuSaiUAvjoWZHd6GkmOO5UW8p9ho_w@mail.gmail.com","threadId":"54167","inReplyTo":"nycvar.QRO.7.76.6.2009232316570.5061@tvgsbejvaqbjf.bet","subject":"Re: [PATCH v7 00/13] Finish converting git bisect to C part 2","fromName":"Miriam R.","fromEmail":"mirucam@gmail.com","sentAt":"2020-09-24T12:56:33Z","receivedAt":"2020-09-24T12:56:46Z","isPatch":true,"sender":{"key":"mirucam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56339109?v=4"},"body":"Hi Johannes,\n\nEl mié., 23 sept. 2020 a las 23:23, Johannes Schindelin\n() escribió:\n>\n> Hi Miriam,\n>\n> On Wed, 23 Sep 2020, Johannes Schindelin wrote:\n>\n> > On Wed, 23 Sep 2020, Miriam R. wrote:\n> >\n> > > Applying some of your suggestions related to removing some 'eval' in\n> > > git-bisect shell script, a bug has appeared. It seems it is related to\n> > > a previous code merged before my internship.\n> >\n> > Now you got me curious: what bug did you see?\n>\n> I found your fork and ran the test, and this is the first symptom:\n>\n> -- snip --\n> [...]\n> ++ git bisect skip\n> Bisecting: 1 revision left to test after this (roughly 1 step)\n> [32a594a3fdac2d57cf6d02987e30eec68511498c] Add <4: Ciao for now> into .\n> ++ git bisect good\n> ++ grep '3082e11d3a0f2edca194c8ce1eb802256e38e75e is the first bad commit' my_bisect_log.txt\n> 3082e11d3a0f2edca194c8ce1eb802256e38e75e is the first bad commit\n> ++ git bisect log\n> ++ git bisect reset\n> Previous HEAD position was 32a594a Add <4: Ciao for now> into .\n> Switched to branch 'other'\n> ok 22 - bisect skip: add line and then a new test\n>\n> expecting success of 6030.23 'bisect skip and bisect replay':\n> git bisect replay log_to_replay.txt > my_bisect_log.txt &&\n> grep \"$HASH5 is the first bad commit\" my_bisect_log.txt &&\n> git bisect reset\n>\n> ++ git bisect replay log_to_replay.txt\n> error: update_ref failed for ref 'refs/bisect/bad': cannot update ref 'refs/bisect/bad': trying to write ref 'refs/bisect/bad' with nonexistent object 10006d020000000068986d020000000054f65f00\n> error: last command exited with $?=1\n> not ok 23 - bisect skip and bisect replay\n> #\n> # git bisect replay log_to_replay.txt > my_bisect_log.txt &&\n> # grep \"$HASH5 is the first bad commit\" my_bisect_log.txt &&\n> # git bisect reset\n> -- snap --\n>\n> So I dug a little bit further (and applied Christian's patch in the\n> meantime), and it turns out that the `eval` has nothing to do with what I\n> originally thought it would be required for: I thought that it wanted to\n> prevent `exit` calls from actually exiting the script.\n>\n> Instead, those `eval` calls are required because the arguments are\n> provided in quoted form. For example, during the execution of t6030.68,\n> the `eval` would expand the call\n>\n> eval \"git bisect--helper --bisect-start $rev $tail\"\n>\n> to\n>\n> git bisect--helper --bisect-start '--term-old' 'term2' '--term-new' 'term1'\n>\n> Therefore, the `eval` really needs to stay in place (also the other `eval`\n> I had originally suggested to remove, for the same reason).\nOk, I will recover your other suggestions too.\n>\n> I would still recommend appending `|| exit`, even if it just so happens\n> that we will eventually abort when the `bisect--helper` command failed\n> anyway, because the next command will then fail, and abort. But it's\n> cleaner to abort already when this invocation failed rather than relying\n> on that side effect.\n\nAha.Ok\nBest,\nMiriam\n>\n> Ciao,\n> Dscho\n>\n"}]}