{"thread":{"id":"52666","subject":"[Outreachy][PATCH 00/29] Finish converting git bisect to C part 1","startedAt":"2020-01-20T14:38:33Z","lastAt":"2020-02-17T22:01:16Z","messageCount":54,"participants":["Miriam Rubio","Johannes Schindelin","Christian Couder","Miriam R.","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":29},"messages":[{"id":"390077","messageId":"20200120143800.900-1-mirucam@gmail.com","threadId":"52666","inReplyTo":null,"subject":"[Outreachy][PATCH 00/29] Finish converting git bisect to C part 1","fromName":"Miriam Rubio","fromEmail":"mirucam@gmail.com","sentAt":"2020-01-20T14:37:31Z","receivedAt":"2020-01-20T14:38:33Z","isPatch":true,"sender":{"key":"mirucam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56339109?v=4"},"body":"--- Changes since Tanushree’s pr117 sent patch series:\nhttps://public-inbox.org/git/pull.117.git.gitgitgadget@gmail.com) ---\n\nGeneral changes\n---------------\n\n* Rebase on master branch.\n* Improve commit messages.\n* Amend patch series titles.\n* Reorder commits: first clean-up/preparatory commits, squash or split \ncommits.\n\nSpecific changes\n----------------\n\n[1/29] bisect--helper: convert `vocab_*` char pointers to char arrays\n\n* New patch to convert `vocab_bad` and `vocab_good` char pointers \nto char arrays\n \n--\n\n[2/29] bisect--helper: change `retval` to `res`\n\n* Replace one last variable `retval` to `res`.\n\n--\n\n[3/29] bisect: use the standard 'if (!var)' way to check for 0\n\n* New patch to use '!var' and make 'bisect.c' more consistent with the\nrest of the code\n\n--\n\n[4/29] run-command: make `exists_in_PATH()` non-static\n\n* Add comment before function declaration.\n* Move function declaration in `run-command.h`.\n\n--\n\n[6/29] bisect: libify `exit_if_skipped_commits` to `error_if_skipped*` \nand its dependents\n \n* Fix `mark_edges_uninteresting()` and `show_diff_tree()` calls after \nrebase on master.\n\n--\n\n[7/29] bisect: libify `bisect_checkout`\n \n* Fix `memcpy()` call after rebase on master.\n* Introduce `res` variable to return `bisect_checkout()` output. \n* Fix `get_commit_reference()` declaration after rebase on master.\n\n--\n\n[8/29] bisect: libify `check_merge_bases` and its dependents\n\nState: Previously sent\n\n* Fix `check_ancestors()` declaration after rebase on master.\n* Fix `get_bad_and_good_commits()` call after rebase on master.\n\n--\n\n[9/29] bisect: libify `check_good_are_ancestors_of_bad` and its \ndependents\n\nState: Previously sent\n\n* Fix `check_good_are_ancestors_of_bad()` declaration after rebase on \nmaster.\n* Fix `check_good_are_ancestors_of_bad()`, `bisect_next_all()`\nand `bisect_rev_setup()` calls after rebase on master.\n\n--\n\n[11/29] bisect: libify `bisect_next_all`\n\nState: Previously sent\n\n* Fix `show_diff_tree()` call after rebase on master.\n\n--\n\n[12/29] bisect--helper: reimplement `bisect_next` and `bisect_auto_next`\nshell functions in C\n\n* Fix `repo_init_revisions()` and `bisect_next_all()` calls after rebase\non master.\n* Remove `goto` statement in `bisect_skipped_commits()`\n\n--\n\n[13/29] bisect--helper: finish porting `bisect_start()` to C\n\n* Change `return` statement instead of `die` in error handling.\n* Remove `goto` statements in `bisect_skipped_commits()`.\n\n--\n\n[21/29] bisect--helper: reimplement `bisect_replay` shell function in C\n\n* Add blank line in `get_next_word()`.\n* Remove `goto` statements in `bisect_replay()`.\n\n--\n\n[23/29] bisect--helper: use `res` instead of return in BISECT_RESET case\noption\n\n* New patch to split previous commit in two.\n\n--\n\n[26/29] bisect--helper: reimplement `bisect_skip` shell function in C\n\nState: Previously sent\n\n* Add blank line.\n\n--\n\n[28/29] bisect--helper: reimplement `bisect_visualize()`shell function \nin C\n\nNew patch:\n\n* Reimplement the `bisect_visualize()` shell function in C.\n* Add `--bisect-visualize` subcommand.\n* Fix long code line.\n\n--\n\n[29/29] bisect--helper: reimplement `bisect_run` shell function in C\n\nNew patch:\n\n* Reimplement the `bisect_run()` shell function in C.\n* Add `--bisect-run` subcommand.\n* Remove blank line.\n\n--\n\nMiriam Rubio (2):\n bisect--helper: convert `vocab_*` char pointers to char arrays\n bisect: use the standard 'if (!var)' way to check for 0\n\nPranit Bauva (24):\n run-command: make `exists_in_PATH()` non-static\n bisect: libify `exit_if_skipped_commits` to `error_if_skipped*` and\n its dependents\n bisect: libify `bisect_checkout`\n bisect: libify `check_merge_bases` and its dependents\n bisect: libify `check_good_are_ancestors_of_bad` and its dependents\n bisect: libify `handle_bad_merge_base` and its dependents\n bisect: libify `bisect_next_all`\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_autostart` shell function in C\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: reimplement `bisect_log` shell function in C\n bisect--helper: reimplement `bisect_replay` shell function in C\n bisect--helper: retire `--bisect-write` subcommand\n bisect--helper: use `res` instead of return in BISECT_RESET case\n option\n bisect--helper: retire `--bisect-autostart` subcommand\n bisect--helper: retire `--bisect-auto-next` subcommand\n bisect--helper: reimplement `bisect_skip` shell function in C\n bisect--helper: retire `--check-and-set-terms` subcommand\n bisect--helper: reimplement `bisect_visualize()`shell function in C\n\nTanushree Tumane (3):\n bisect--helper: change `retval` to `res`\n bisect--helper: introduce new `decide_next()` function\n bisect--helper: reimplement `bisect_run` shell function in C\n\n bisect.c | 146 +++++---\n builtin/bisect--helper.c | 776 +++++++++++++++++++++++++++++++++------\n git-bisect.sh | 279 +-------------\n run-command.c | 2 +-\n run-command.h | 11 +\n 5 files changed, 793 insertions(+), 421 deletions(-)\n\n-- \n2.21.1 (Apple Git-122.3)\n\n"},{"id":"390078","messageId":"20200120143800.900-4-mirucam@gmail.com","threadId":"52666","inReplyTo":"20200120143800.900-1-mirucam@gmail.com","subject":"[PATCH 03/29] bisect: use the standard 'if (!var)' way to check for 0","fromName":"Miriam Rubio","fromEmail":"mirucam@gmail.com","sentAt":"2020-01-20T14:37:34Z","receivedAt":"2020-01-20T14:38:34Z","isPatch":true,"sender":{"key":"mirucam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56339109?v=4"},"body":"Instead of using 'var == 0' in an if condition, let's use '!var' and\nmake 'bisect.c' more consistent with the rest of the code.\n\nMentored-by: Christian Couder \nSigned-off-by: Miriam Rubio \n---\n bisect.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/bisect.c b/bisect.c\nindex e81c91d02c..83cb5b3a98 100644\n--- a/bisect.c\n+++ b/bisect.c\n@@ -572,7 +572,7 @@ static int sqrti(int val)\n {\n \tfloat d, x = val;\n \n-\tif (val == 0)\n+\tif (!val)\n \t\treturn 0;\n \n \tdo {\n@@ -869,7 +869,7 @@ static void check_good_are_ancestors_of_bad(struct repository *r,\n \t\tgoto done;\n \n \t/* Bisecting with no good rev is ok. */\n-\tif (good_revs.nr == 0)\n+\tif (!good_revs.nr)\n \t\tgoto done;\n \n \t/* Check if all good revs are ancestor of the bad rev. */\n-- \n2.21.1 (Apple Git-122.3)\n\n"},{"id":"390079","messageId":"20200120143800.900-2-mirucam@gmail.com","threadId":"52666","inReplyTo":"20200120143800.900-1-mirucam@gmail.com","subject":"[PATCH 01/29] bisect--helper: convert `vocab_*` char pointers to char arrays","fromName":"Miriam Rubio","fromEmail":"mirucam@gmail.com","sentAt":"2020-01-20T14:37:32Z","receivedAt":"2020-01-20T14:38:35Z","isPatch":true,"sender":{"key":"mirucam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56339109?v=4"},"body":"Instead of using a pointer that points at a constant string,\njust give name directly to the constant string; this way, we\ndo not have to allocate a pointer variable in addition to\nthe string we want to use.\n\nLet's convert `vocab_bad` and `vocab_good` char pointers to char arrays.\n\nMentored-by: Christian Couder \nSigned-off-by: Miriam Rubio \n---\n builtin/bisect--helper.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\nindex 1718df7f09..36c09b7238 100644\n--- a/builtin/bisect--helper.c\n+++ b/builtin/bisect--helper.c\n@@ -52,8 +52,8 @@ static void set_terms(struct bisect_terms *terms, const char *bad,\n \tterms->term_bad = xstrdup(bad);\n }\n \n-static const char *vocab_bad = \"bad|new\";\n-static const char *vocab_good = \"good|old\";\n+static const char vocab_bad[] = \"bad|new\";\n+static const char vocab_good[] = \"good|old\";\n \n /*\n * Check whether the string `term` belongs to the set of strings\n-- \n2.21.1 (Apple Git-122.3)\n\n"},{"id":"390080","messageId":"20200120143800.900-3-mirucam@gmail.com","threadId":"52666","inReplyTo":"20200120143800.900-1-mirucam@gmail.com","subject":"[PATCH 02/29] bisect--helper: change `retval` to `res`","fromName":"Miriam Rubio","fromEmail":"mirucam@gmail.com","sentAt":"2020-01-20T14:37:33Z","receivedAt":"2020-01-20T14:38:37Z","isPatch":true,"sender":{"key":"mirucam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56339109?v=4"},"body":"From: Tanushree Tumane \n\nLet's rename variable retval to res, so that variable names\nin bisect--helper.c are more consistent.\n\nAfter this change, there are 110 occurrences of res in the file\nand zero of retval, while there were 26 instances of retval before.\n\nMentored-by: Christian Couder \nSigned-off-by: Tanushree Tumane \nSigned-off-by: Miriam Rubio \n---\n builtin/bisect--helper.c | 52 ++++++++++++++++++++--------------------\n 1 file changed, 26 insertions(+), 26 deletions(-)\n\ndiff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\nindex 36c09b7238..21de5c096c 100644\n--- a/builtin/bisect--helper.c\n+++ b/builtin/bisect--helper.c\n@@ -206,31 +206,31 @@ static int bisect_write(const char *state, const char *rev,\n \tstruct object_id oid;\n \tstruct commit *commit;\n \tFILE *fp = NULL;\n-\tint retval = 0;\n+\tint res = 0;\n \n \tif (!strcmp(state, terms->term_bad)) {\n \t\tstrbuf_addf(&tag, \"refs/bisect/%s\", state);\n \t} else if (one_of(state, terms->term_good, \"skip\", NULL)) {\n \t\tstrbuf_addf(&tag, \"refs/bisect/%s-%s\", state, rev);\n \t} else {\n-\t\tretval = error(_(\"Bad bisect_write argument: %s\"), state);\n+\t\tres = error(_(\"Bad bisect_write argument: %s\"), state);\n \t\tgoto finish;\n \t}\n \n \tif (get_oid(rev, &oid)) {\n-\t\tretval = error(_(\"couldn't get the oid of the rev '%s'\"), rev);\n+\t\tres = error(_(\"couldn't get the oid of the rev '%s'\"), rev);\n \t\tgoto finish;\n \t}\n \n \tif (update_ref(NULL, tag.buf, &oid, NULL, 0,\n \t\t UPDATE_REFS_MSG_ON_ERR)) {\n-\t\tretval = -1;\n+\t\tres = -1;\n \t\tgoto finish;\n \t}\n \n \tfp = fopen(git_path_bisect_log(), \"a\");\n \tif (!fp) {\n-\t\tretval = error_errno(_(\"couldn't open the file '%s'\"), git_path_bisect_log());\n+\t\tres = error_errno(_(\"couldn't open the file '%s'\"), git_path_bisect_log());\n \t\tgoto finish;\n \t}\n \n@@ -244,7 +244,7 @@ static int bisect_write(const char *state, const char *rev,\n \tif (fp)\n \t\tfclose(fp);\n \tstrbuf_release(&tag);\n-\treturn retval;\n+\treturn res;\n }\n \n static int check_and_set_terms(struct bisect_terms *terms, const char *cmd)\n@@ -294,7 +294,7 @@ static const char need_bisect_start_warning[] =\n static int bisect_next_check(const struct bisect_terms *terms,\n \t\t\t const char *current_term)\n {\n-\tint missing_good = 1, missing_bad = 1, retval = 0;\n+\tint missing_good = 1, missing_bad = 1, res = 0;\n \tconst char *bad_ref = xstrfmt(\"refs/bisect/%s\", terms->term_bad);\n \tconst char *good_glob = xstrfmt(\"%s-*\", terms->term_good);\n \n@@ -308,7 +308,7 @@ static int bisect_next_check(const struct bisect_terms *terms,\n \t\tgoto finish;\n \n \tif (!current_term) {\n-\t\tretval = -1;\n+\t\tres = -1;\n \t\tgoto finish;\n \t}\n \n@@ -329,21 +329,21 @@ static int bisect_next_check(const struct bisect_terms *terms,\n \t\t */\n \t\tyesno = git_prompt(_(\"Are you sure [Y/n]? \"), PROMPT_ECHO);\n \t\tif (starts_with(yesno, \"N\") || starts_with(yesno, \"n\"))\n-\t\t\tretval = -1;\n+\t\t\tres = -1;\n \t\tgoto finish;\n \t}\n \tif (!is_empty_or_missing_file(git_path_bisect_start())) {\n-\t\tretval = error(_(need_bad_and_good_revision_warning),\n+\t\tres = error(_(need_bad_and_good_revision_warning),\n \t\t\t vocab_bad, vocab_good, vocab_bad, vocab_good);\n \t} else {\n-\t\tretval = error(_(need_bisect_start_warning),\n+\t\tres = error(_(need_bisect_start_warning),\n \t\t\t vocab_good, vocab_bad, vocab_good, vocab_bad);\n \t}\n \n finish:\n \tfree((void *) good_glob);\n \tfree((void *) bad_ref);\n-\treturn retval;\n+\treturn res;\n }\n \n static int get_terms(struct bisect_terms *terms)\n@@ -397,7 +397,7 @@ static int bisect_terms(struct bisect_terms *terms, const char *option)\n \n static int bisect_append_log_quoted(const char **argv)\n {\n-\tint retval = 0;\n+\tint res = 0;\n \tFILE *fp = fopen(git_path_bisect_log(), \"a\");\n \tstruct strbuf orig_args = STRBUF_INIT;\n \n@@ -405,25 +405,25 @@ static int bisect_append_log_quoted(const char **argv)\n \t\treturn -1;\n \n \tif (fprintf(fp, \"git bisect start\") < 1) {\n-\t\tretval = -1;\n+\t\tres = -1;\n \t\tgoto finish;\n \t}\n \n \tsq_quote_argv(&orig_args, argv);\n \tif (fprintf(fp, \"%s\\n\", orig_args.buf) < 1)\n-\t\tretval = -1;\n+\t\tres = -1;\n \n finish:\n \tfclose(fp);\n \tstrbuf_release(&orig_args);\n-\treturn retval;\n+\treturn res;\n }\n \n static int bisect_start(struct bisect_terms *terms, int no_checkout,\n \t\t\tconst char **argv, int argc)\n {\n \tint i, has_double_dash = 0, must_write_terms = 0, bad_seen = 0;\n-\tint flags, pathspec_pos, retval = 0;\n+\tint flags, pathspec_pos, res = 0;\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@@ -524,7 +524,7 @@ static int bisect_start(struct bisect_terms *terms, int no_checkout,\n \t\t\targv_array_pushl(&argv, \"checkout\", start_head.buf,\n \t\t\t\t\t \"--\", NULL);\n \t\t\tif (run_command_v_opt(argv.argv, RUN_GIT_CMD)) {\n-\t\t\t\tretval = error(_(\"checking out '%s' failed.\"\n+\t\t\t\tres = error(_(\"checking out '%s' failed.\"\n \t\t\t\t\t\t \" Try 'git bisect start \"\n \t\t\t\t\t\t \"'.\"),\n \t\t\t\t\t start_head.buf);\n@@ -572,12 +572,12 @@ static int bisect_start(struct bisect_terms *terms, int no_checkout,\n \n \tif (no_checkout) {\n \t\tif (get_oid(start_head.buf, &oid) < 0) {\n-\t\t\tretval = error(_(\"invalid ref: '%s'\"), start_head.buf);\n+\t\t\tres = error(_(\"invalid ref: '%s'\"), start_head.buf);\n \t\t\tgoto finish;\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\tretval = -1;\n+\t\t\tres = -1;\n \t\t\tgoto finish;\n \t\t}\n \t}\n@@ -589,26 +589,26 @@ static int bisect_start(struct bisect_terms *terms, int no_checkout,\n \tfor (i = 0; i < states.nr; i++)\n \t\tif (bisect_write(states.items[i].string,\n \t\t\t\t revs.items[i].string, terms, 1)) {\n-\t\t\tretval = -1;\n+\t\t\tres = -1;\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\tretval = -1;\n+\t\tres = -1;\n \t\tgoto finish;\n \t}\n \n-\tretval = bisect_append_log_quoted(argv);\n-\tif (retval)\n-\t\tretval = -1;\n+\tres = bisect_append_log_quoted(argv);\n+\tif (res)\n+\t\tres = -1;\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-\treturn retval;\n+\treturn res;\n }\n \n int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n-- \n2.21.1 (Apple Git-122.3)\n\n"},{"id":"390081","messageId":"20200120143800.900-8-mirucam@gmail.com","threadId":"52666","inReplyTo":"20200120143800.900-1-mirucam@gmail.com","subject":"[PATCH 07/29] bisect: libify `bisect_checkout`","fromName":"Miriam Rubio","fromEmail":"mirucam@gmail.com","sentAt":"2020-01-20T14:37:38Z","receivedAt":"2020-01-20T14:38:40Z","isPatch":true,"sender":{"key":"mirucam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56339109?v=4"},"body":"From: Pranit Bauva \n\nSince we want to get rid of git-bisect.sh it would be necessary to\nconvert those exit() calls to return statements so that errors can be\nreported.\n\nEmulate try catch in C by converting `exit()` to\n`return `. Follow POSIX conventions to return\n to indicate error.\n\nTurn `exit()` to `return` calls in `bisect_checkout()`.\nChanges related to return values have no bad side effects on the\ncode that calls `bisect_checkout()`.\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 bisect.c | 7 ++++---\n 1 file changed, 4 insertions(+), 3 deletions(-)\n\ndiff --git a/bisect.c b/bisect.c\nindex 2ac0463327..385afaf875 100644\n--- a/bisect.c\n+++ b/bisect.c\n@@ -713,6 +713,7 @@ static int bisect_checkout(const struct object_id *bisect_rev, int no_checkout)\n {\n \tchar bisect_rev_hex[GIT_MAX_HEXSZ + 1];\n \n+\tint res = 0;\n \tmemcpy(bisect_rev_hex, oid_to_hex(bisect_rev), the_hash_algo->hexsz + 1);\n \tupdate_ref(NULL, \"BISECT_EXPECTED_REV\", bisect_rev, NULL, 0, UPDATE_REFS_DIE_ON_ERR);\n \n@@ -721,14 +722,14 @@ static int bisect_checkout(const struct object_id *bisect_rev, int no_checkout)\n \t\tupdate_ref(NULL, \"BISECT_HEAD\", bisect_rev, NULL, 0,\n \t\t\t UPDATE_REFS_DIE_ON_ERR);\n \t} else {\n-\t\tint res;\n \t\tres = run_command_v_opt(argv_checkout, RUN_GIT_CMD);\n \t\tif (res)\n-\t\t\texit(res);\n+\t\t\treturn res > 0 ? -res : res;\n \t}\n \n \targv_show_branch[1] = bisect_rev_hex;\n-\treturn run_command_v_opt(argv_show_branch, RUN_GIT_CMD);\n+\tres = run_command_v_opt(argv_show_branch, RUN_GIT_CMD);\n+\treturn res > 0 ? -res : res;\n }\n \n static struct commit *get_commit_reference(struct repository *r,\n-- \n2.21.1 (Apple Git-122.3)\n\n"},{"id":"390082","messageId":"20200120143800.900-9-mirucam@gmail.com","threadId":"52666","inReplyTo":"20200120143800.900-1-mirucam@gmail.com","subject":"[PATCH 08/29] bisect: libify `check_merge_bases` and its dependents","fromName":"Miriam Rubio","fromEmail":"mirucam@gmail.com","sentAt":"2020-01-20T14:37:39Z","receivedAt":"2020-01-20T14:38:41Z","isPatch":true,"sender":{"key":"mirucam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56339109?v=4"},"body":"From: Pranit Bauva \n\nSince we want to get rid of git-bisect.sh it would be necessary to\nconvert those exit() calls to return statements so that errors can be\nreported.\n\nEmulate try catch in C by converting `exit()` to\n`return `. Follow POSIX conventions to return\n to indicate error.\n\nTurn `exit()` to `return` calls in `check_merge_bases()`.\n\nIn `check_merge_bases()` there is an early success special case,\nso we have introduced special error code `-11` which indicates early\nsuccess. This `-11` is converted back to `0` in `check_good_are_ancestors_of_bad()`.\n\nHandle the return value in dependent function `check_good_are_ancestors_of_bad()`.\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 bisect.c | 21 ++++++++++++++++-----\n 1 file changed, 16 insertions(+), 5 deletions(-)\n\ndiff --git a/bisect.c b/bisect.c\nindex 385afaf875..367258b0dd 100644\n--- a/bisect.c\n+++ b/bisect.c\n@@ -806,13 +806,16 @@ static void handle_skipped_merge_base(const struct object_id *mb)\n * \"check_merge_bases\" checks that merge bases are not \"bad\" (or \"new\").\n *\n * - If one is \"bad\" (or \"new\"), it means the user assumed something wrong\n- * and we must exit with a non 0 error code.\n+ * and we must return error with a non 0 error code.\n * - If one is \"good\" (or \"old\"), that's good, we have nothing to do.\n * - If one is \"skipped\", we can't know but we should warn.\n * - If we don't know, we should check it out and ask the user to test.\n+ * - If a merge base must be tested, on success return -11 a special condition\n+ * for early success, this will be converted back to 0 in check_good_are_ancestors_of_bad().\n */\n-static void check_merge_bases(int rev_nr, struct commit **rev, int no_checkout)\n+static int check_merge_bases(int rev_nr, struct commit **rev, int no_checkout)\n {\n+\tint res = 0;\n \tstruct commit_list *result;\n \n \tresult = get_merge_bases_many(rev[0], rev_nr - 1, rev + 1);\n@@ -827,11 +830,16 @@ static void check_merge_bases(int rev_nr, struct commit **rev, int no_checkout)\n \t\t\thandle_skipped_merge_base(mb);\n \t\t} else {\n \t\t\tprintf(_(\"Bisecting: a merge base must be tested\\n\"));\n-\t\t\texit(bisect_checkout(mb, no_checkout));\n+\t\t\tres = bisect_checkout(mb, no_checkout);\n+\t\t\tif (!res)\n+\t\t\t\t/* indicate early success */\n+\t\t\t\tres = -11;\n+\t\t\tbreak;\n \t\t}\n \t}\n \n \tfree_commit_list(result);\n+\treturn res;\n }\n \n static int check_ancestors(struct repository *r, int rev_nr,\n@@ -865,7 +873,7 @@ static void check_good_are_ancestors_of_bad(struct repository *r,\n {\n \tchar *filename = git_pathdup(\"BISECT_ANCESTORS_OK\");\n \tstruct stat st;\n-\tint fd, rev_nr;\n+\tint fd, rev_nr, res = 0;\n \tstruct commit **rev;\n \n \tif (!current_bad_oid)\n@@ -880,10 +888,13 @@ static void check_good_are_ancestors_of_bad(struct repository *r,\n \t\tgoto done;\n \n \t/* Check if all good revs are ancestor of the bad rev. */\n+\n \trev = get_bad_and_good_commits(r, &rev_nr);\n \tif (check_ancestors(r, rev_nr, rev, prefix))\n-\t\tcheck_merge_bases(rev_nr, rev, no_checkout);\n+\t\tres = check_merge_bases(rev_nr, rev, no_checkout);\n \tfree(rev);\n+\tif(res)\n+\t\texit(res == -11 ? 0 : -res);\n \n \t/* Create file BISECT_ANCESTORS_OK. */\n \tfd = open(filename, O_CREAT | O_TRUNC | O_WRONLY, 0600);\n-- \n2.21.1 (Apple Git-122.3)\n\n"},{"id":"390084","messageId":"20200120143800.900-5-mirucam@gmail.com","threadId":"52666","inReplyTo":"20200120143800.900-1-mirucam@gmail.com","subject":"[PATCH 04/29] run-command: make `exists_in_PATH()` non-static","fromName":"Miriam Rubio","fromEmail":"mirucam@gmail.com","sentAt":"2020-01-20T14:37:35Z","receivedAt":"2020-01-20T14:38:41Z","isPatch":true,"sender":{"key":"mirucam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56339109?v=4"},"body":"From: Pranit Bauva \n\nRemoves the `static` keyword from `exists_in_PATH()` function\nand declares the function in `run-command.h` file.\nThe function will be used in bisect_visualize() in a later\ncommit.\n\nMentored by: Christian Couder \nMentored by: Johannes Schindelin \nSigned-off-by: Tanushree Tumane \nSigned-off-by: Miriam Rubio \n---\n run-command.c | 2 +-\n run-command.h | 11 +++++++++++\n 2 files changed, 12 insertions(+), 1 deletion(-)\n\ndiff --git a/run-command.c b/run-command.c\nindex 9942f120a9..096faea5b9 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -210,7 +210,7 @@ static char *locate_in_PATH(const char *file)\n \treturn NULL;\n }\n \n-static int exists_in_PATH(const char *file)\n+int exists_in_PATH(const char *file)\n {\n \tchar *r = locate_in_PATH(file);\n \tfree(r);\ndiff --git a/run-command.h b/run-command.h\nindex 592d9dc035..7c8e206d97 100644\n--- a/run-command.h\n+++ b/run-command.h\n@@ -172,6 +172,17 @@ void child_process_clear(struct child_process *);\n \n int is_executable(const char *name);\n \n+/**\n+ * Returns if a $PATH given by parameter is found or not (it is NULL). This\n+ * function uses locate_in_PATH() function that emulates the path search that\n+ * execvp would perform. Memory used to store the resultant path is freed by\n+ * the function.\n+ *\n+ * The caller should ensure that $PATH contains no directory\n+ * separators.\n+ */\n+int exists_in_PATH(const char *);\n+\n /**\n * Start a sub-process. Takes a pointer to a `struct child_process`\n * that specifies the details and returns pipe FDs (if requested).\n-- \n2.21.1 (Apple Git-122.3)\n\n"},{"id":"390086","messageId":"20200120143800.900-10-mirucam@gmail.com","threadId":"52666","inReplyTo":"20200120143800.900-1-mirucam@gmail.com","subject":"[PATCH 09/29] bisect: libify `check_good_are_ancestors_of_bad` and its dependents","fromName":"Miriam Rubio","fromEmail":"mirucam@gmail.com","sentAt":"2020-01-20T14:37:40Z","receivedAt":"2020-01-20T14:38:42Z","isPatch":true,"sender":{"key":"mirucam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56339109?v=4"},"body":"From: Pranit Bauva \n\nSince we want to get rid of git-bisect.sh it would be necessary to\nconvert those exit() calls to return statements so that errors can be\nreported.\n\nEmulate try catch in C by converting `exit()` to\n`return `. Follow POSIX conventions to return\n to indicate error.\n\nTurn `exit()` to `return` calls in `check_good_are_ancestors_of_bad()`.\n\nCode that turns -11(early success code) to 0 from\n`check_good_are_ancestors_of_bad()` has been moved to\n`cmd_bisect__helper()`.\n\nHandle the return value in dependent function `bisect_next_all()`.\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 bisect.c | 42 ++++++++++++++++++++++++++--------------\n builtin/bisect--helper.c | 12 ++++++++++--\n 2 files changed, 37 insertions(+), 17 deletions(-)\n\ndiff --git a/bisect.c b/bisect.c\nindex 367258b0dd..2b80597a1d 100644\n--- a/bisect.c\n+++ b/bisect.c\n@@ -865,9 +865,10 @@ static int check_ancestors(struct repository *r, int rev_nr,\n *\n * If that's not the case, we need to check the merge bases.\n * If a merge base must be tested by the user, its source code will be\n- * checked out to be tested by the user and we will exit.\n+ * checked out to be tested by the user and we will return.\n */\n-static void check_good_are_ancestors_of_bad(struct repository *r,\n+\n+static int check_good_are_ancestors_of_bad(struct repository *r,\n \t\t\t\t\t const char *prefix,\n \t\t\t\t\t int no_checkout)\n {\n@@ -876,8 +877,15 @@ static void check_good_are_ancestors_of_bad(struct repository *r,\n \tint fd, rev_nr, res = 0;\n \tstruct commit **rev;\n \n-\tif (!current_bad_oid)\n-\t\tdie(_(\"a %s revision is needed\"), term_bad);\n+\t/*\n+\t * We don't want to clean the bisection state\n+\t * as we need to get back to where we started\n+\t * by using `git bisect reset`.\n+\t */\n+\tif (!current_bad_oid) {\n+\t\tres = error(_(\"a %s revision is needed\"), term_bad);\n+\t\tgoto done;\n+\t}\n \n \t/* Check if file BISECT_ANCESTORS_OK exists. */\n \tif (!stat(filename, &st) && S_ISREG(st.st_mode))\n@@ -893,18 +901,20 @@ static void check_good_are_ancestors_of_bad(struct repository *r,\n \tif (check_ancestors(r, rev_nr, rev, prefix))\n \t\tres = check_merge_bases(rev_nr, rev, no_checkout);\n \tfree(rev);\n-\tif(res)\n-\t\texit(res == -11 ? 0 : -res);\n-\n-\t/* Create file BISECT_ANCESTORS_OK. */\n-\tfd = open(filename, O_CREAT | O_TRUNC | O_WRONLY, 0600);\n-\tif (fd < 0)\n-\t\twarning_errno(_(\"could not create file '%s'\"),\n-\t\t\t filename);\n-\telse\n-\t\tclose(fd);\n+\t\n+\tif (!res)\n+\t{\n+\t\t/* Create file BISECT_ANCESTORS_OK. */\n+\t\tfd = open(filename, O_CREAT | O_TRUNC | O_WRONLY, 0600);\n+\t\tif (fd < 0)\n+\t\t\twarning_errno(_(\"could not create file '%s'\"),\n+\t\t\t\t filename);\n+\t\telse\n+\t\t\tclose(fd);\n+\t}\n done:\n \tfree(filename);\n+\treturn res;\n }\n \n /*\n@@ -975,7 +985,9 @@ int bisect_next_all(struct repository *r, const char *prefix, int no_checkout)\n \tif (read_bisect_refs())\n \t\tdie(_(\"reading bisect refs failed\"));\n \n-\tcheck_good_are_ancestors_of_bad(r, prefix, no_checkout);\n+\tres = check_good_are_ancestors_of_bad(r, prefix, no_checkout);\n+\tif (res)\n+\t\treturn res;\n \n \tbisect_rev_setup(r, &revs, prefix, \"%s\", \"^%s\", 1);\n \trevs.limited = 1;\ndiff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\nindex 826fcba2ed..5e0f759d50 100644\n--- a/builtin/bisect--helper.c\n+++ b/builtin/bisect--helper.c\n@@ -666,7 +666,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n \n \tswitch (cmdmode) {\n \tcase NEXT_ALL:\n-\t\treturn bisect_next_all(the_repository, prefix, no_checkout);\n+\t\tres = bisect_next_all(the_repository, prefix, no_checkout);\n+\t\tbreak;\n \tcase WRITE_TERMS:\n \t\tif (argc != 2)\n \t\t\treturn error(_(\"--write-terms requires two arguments\"));\n@@ -713,5 +714,12 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n \t\treturn error(\"BUG: unknown subcommand '%d'\", cmdmode);\n \t}\n \tfree_terms(&terms);\n-\treturn !!res;\n+\t/* \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 == -11)\n+\t\tres = 0;\n+\n+\treturn res < 0 ? -res : res;\n }\n-- \n2.21.1 (Apple Git-122.3)\n\n"},{"id":"390083","messageId":"20200120143800.900-6-mirucam@gmail.com","threadId":"52666","inReplyTo":"20200120143800.900-1-mirucam@gmail.com","subject":"[PATCH 05/29] bisect--helper: introduce new `decide_next()` function","fromName":"Miriam Rubio","fromEmail":"mirucam@gmail.com","sentAt":"2020-01-20T14:37:36Z","receivedAt":"2020-01-20T14:38:43Z","isPatch":true,"sender":{"key":"mirucam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56339109?v=4"},"body":"From: Tanushree Tumane \n\nLet's refactor code from bisect_next_check() into a new\ndecide_next() helper function.\n\nThis removes some goto statements and makes the code simpler,\nclearer and easier to understand.\n\nMentored-by: Christian Couder \nSigned-off-by: Tanushree Tumane \nSigned-off-by: Miriam Rubio \n---\n builtin/bisect--helper.c | 62 +++++++++++++++++++++-------------------\n 1 file changed, 32 insertions(+), 30 deletions(-)\n\ndiff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\nindex 21de5c096c..826fcba2ed 100644\n--- a/builtin/bisect--helper.c\n+++ b/builtin/bisect--helper.c\n@@ -291,26 +291,14 @@ static const char need_bisect_start_warning[] =\n \t \"You then need to give me at least one %s and %s revision.\\n\"\n \t \"You can use \\\"git bisect %s\\\" and \\\"git bisect %s\\\" for that.\");\n \n-static int bisect_next_check(const struct bisect_terms *terms,\n-\t\t\t const char *current_term)\n+static int decide_next(const struct bisect_terms *terms,\n+\t\t const char *current_term, int missing_good,\n+\t\t int missing_bad)\n {\n-\tint missing_good = 1, missing_bad = 1, res = 0;\n-\tconst char *bad_ref = xstrfmt(\"refs/bisect/%s\", terms->term_bad);\n-\tconst char *good_glob = xstrfmt(\"%s-*\", terms->term_good);\n-\n-\tif (ref_exists(bad_ref))\n-\t\tmissing_bad = 0;\n-\n-\tfor_each_glob_ref_in(mark_good, good_glob, \"refs/bisect/\",\n-\t\t\t (void *) &missing_good);\n-\n \tif (!missing_good && !missing_bad)\n-\t\tgoto finish;\n-\n-\tif (!current_term) {\n-\t\tres = -1;\n-\t\tgoto finish;\n-\t}\n+\t\treturn 0;\n+\tif (!current_term)\n+\t\treturn -1;\n \n \tif (missing_good && !missing_bad &&\n \t !strcmp(current_term, terms->term_good)) {\n@@ -321,7 +309,7 @@ static int bisect_next_check(const struct bisect_terms *terms,\n \t\t */\n \t\twarning(_(\"bisecting only with a %s commit\"), terms->term_bad);\n \t\tif (!isatty(0))\n-\t\t\tgoto finish;\n+\t\t\treturn 0;\n \t\t/*\n \t\t * TRANSLATORS: Make sure to include [Y] and [n] in your\n \t\t * translation. The program will only accept English input\n@@ -329,21 +317,35 @@ static int bisect_next_check(const struct bisect_terms *terms,\n \t\t */\n \t\tyesno = git_prompt(_(\"Are you sure [Y/n]? \"), PROMPT_ECHO);\n \t\tif (starts_with(yesno, \"N\") || starts_with(yesno, \"n\"))\n-\t\t\tres = -1;\n-\t\tgoto finish;\n-\t}\n-\tif (!is_empty_or_missing_file(git_path_bisect_start())) {\n-\t\tres = error(_(need_bad_and_good_revision_warning),\n-\t\t\t vocab_bad, vocab_good, vocab_bad, vocab_good);\n-\t} else {\n-\t\tres = error(_(need_bisect_start_warning),\n-\t\t\t vocab_good, vocab_bad, vocab_good, vocab_bad);\n+\t\t\treturn -1;\n+\t\treturn 0;\n \t}\n \n-finish:\n+\tif (!is_empty_or_missing_file(git_path_bisect_start()))\n+\t\treturn error(_(need_bad_and_good_revision_warning),\n+\t\t\t vocab_bad, vocab_good, vocab_bad, vocab_good);\n+\telse\n+\t\treturn error(_(need_bisect_start_warning),\n+\t\t\t vocab_good, vocab_bad, vocab_good, vocab_bad);\n+}\n+\n+static int bisect_next_check(const struct bisect_terms *terms,\n+\t\t\t const char *current_term)\n+{\n+\tint missing_good = 1, missing_bad = 1;\n+\tconst char *bad_ref = xstrfmt(\"refs/bisect/%s\", terms->term_bad);\n+\tconst char *good_glob = xstrfmt(\"%s-*\", terms->term_good);\n+\n+\tif (ref_exists(bad_ref))\n+\t\tmissing_bad = 0;\n+\n+\tfor_each_glob_ref_in(mark_good, good_glob, \"refs/bisect/\",\n+\t\t\t (void *) &missing_good);\n+\n \tfree((void *) good_glob);\n \tfree((void *) bad_ref);\n-\treturn res;\n+\n+\treturn decide_next(terms, current_term, missing_good, missing_bad);\n }\n \n static int get_terms(struct bisect_terms *terms)\n-- \n2.21.1 (Apple Git-122.3)\n\n"},{"id":"390085","messageId":"20200120143800.900-11-mirucam@gmail.com","threadId":"52666","inReplyTo":"20200120143800.900-1-mirucam@gmail.com","subject":"[PATCH 10/29] bisect: libify `handle_bad_merge_base` and its dependents","fromName":"Miriam Rubio","fromEmail":"mirucam@gmail.com","sentAt":"2020-01-20T14:37:41Z","receivedAt":"2020-01-20T14:38:45Z","isPatch":true,"sender":{"key":"mirucam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56339109?v=4"},"body":"From: Pranit Bauva \n\nSince we want to get rid of git-bisect.sh it would be necessary to\nconvert those exit() calls to return statements so that errors can be\nreported.\n\nEmulate try catch in C by converting `exit()` to\n`return `. Follow POSIX conventions to return\n to indicate error.\n\nTurn `exit()` to `return` calls in `handle_bad_merge_base()`.\n\nHandle the return value in dependent function check_merge_bases().\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 bisect.c | 9 +++++----\n 1 file changed, 5 insertions(+), 4 deletions(-)\n\ndiff --git a/bisect.c b/bisect.c\nindex 2b80597a1d..acb5a13911 100644\n--- a/bisect.c\n+++ b/bisect.c\n@@ -756,7 +756,7 @@ static struct commit **get_bad_and_good_commits(struct repository *r,\n \treturn rev;\n }\n \n-static void handle_bad_merge_base(void)\n+static int handle_bad_merge_base(void)\n {\n \tif (is_expected_rev(current_bad_oid)) {\n \t\tchar *bad_hex = oid_to_hex(current_bad_oid);\n@@ -777,14 +777,14 @@ static void handle_bad_merge_base(void)\n \t\t\t\t\"between %s and [%s].\\n\"),\n \t\t\t\tbad_hex, term_bad, term_good, bad_hex, good_hex);\n \t\t}\n-\t\texit(3);\n+\t\treturn -3;\n \t}\n \n \tfprintf(stderr, _(\"Some %s revs are not ancestors of the %s rev.\\n\"\n \t\t\"git bisect cannot work properly in this case.\\n\"\n \t\t\"Maybe you mistook %s and %s revs?\\n\"),\n \t\tterm_good, term_bad, term_good, term_bad);\n-\texit(1);\n+\treturn -1;\n }\n \n static void handle_skipped_merge_base(const struct object_id *mb)\n@@ -823,7 +823,8 @@ static int check_merge_bases(int rev_nr, struct commit **rev, int no_checkout)\n \tfor (; result; result = result->next) {\n \t\tconst struct object_id *mb = &result->item->object.oid;\n \t\tif (oideq(mb, current_bad_oid)) {\n-\t\t\thandle_bad_merge_base();\n+\t\t\tres = handle_bad_merge_base();\n+\t\t\tbreak;\n \t\t} else if (0 <= oid_array_lookup(&good_revs, mb)) {\n \t\t\tcontinue;\n \t\t} else if (0 <= oid_array_lookup(&skipped_revs, mb)) {\n-- \n2.21.1 (Apple Git-122.3)\n\n"},{"id":"390087","messageId":"20200120143800.900-12-mirucam@gmail.com","threadId":"52666","inReplyTo":"20200120143800.900-1-mirucam@gmail.com","subject":"[PATCH 11/29] bisect: libify `bisect_next_all`","fromName":"Miriam Rubio","fromEmail":"mirucam@gmail.com","sentAt":"2020-01-20T14:37:42Z","receivedAt":"2020-01-20T14:38:46Z","isPatch":true,"sender":{"key":"mirucam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56339109?v=4"},"body":"From: Pranit Bauva \n\nSince we want to get rid of git-bisect.sh it would be necessary to\nconvert those exit() calls to return statements so that errors can be\nreported.\n\nEmulate try catch in C by converting `exit()` to\n`return `. Follow POSIX conventions to return\n to indicate error.\n\nTurn `exit()` to `return` calls in `bisect_next_all()`.\n\nAll the functions calling `bisect_next_all()` are already able to\nhandle return values from it.\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 bisect.c | 40 +++++++++++++++++++++++++++++-----------\n 1 file changed, 29 insertions(+), 11 deletions(-)\n\ndiff --git a/bisect.c b/bisect.c\nindex acb5a13911..33f2829c19 100644\n--- a/bisect.c\n+++ b/bisect.c\n@@ -967,10 +967,10 @@ void read_bisect_terms(const char **read_bad, const char **read_good)\n }\n \n /*\n- * We use the convention that exiting with an exit code 10 means that\n- * the bisection process finished successfully.\n- * In this case the calling shell script should exit 0.\n- *\n+ * We use the convention that return -10 means the bisection process\n+ * finished successfully.\n+ * In this case the calling function or command should not turn a -10\n+ * 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@@ -1000,23 +1000,35 @@ int bisect_next_all(struct repository *r, const char *prefix, int no_checkout)\n \n \tif (!revs.commits) {\n \t\t/*\n-\t\t * We should exit here only if the \"bad\"\n+\t\t * We should return error here only if the \"bad\"\n \t\t * commit is also a \"skip\" commit.\n \t\t */\n \t\tres = error_if_skipped_commits(tried, NULL);\n \t\tif (res)\n-\t\t\texit(-res);\n+\t\t\treturn res;\n \t\tprintf(_(\"%s was both %s and %s\\n\"),\n \t\t oid_to_hex(current_bad_oid),\n \t\t term_good,\n \t\t term_bad);\n-\t\texit(1);\n+\n+\t\t/*\n+\t\t * We don't want to clean the bisection state\n+\t\t * as we need to get back to where we started\n+\t\t * by using `git bisect reset`.\n+\t\t */\n+\t\treturn -1;\n \t}\n \n \tif (!all) {\n \t\tfprintf(stderr, _(\"No testable commit found.\\n\"\n \t\t\t\"Maybe you started with bad path parameters?\\n\"));\n-\t\texit(4);\n+\n+\t\t/*\n+\t\t * We don't want to clean the bisection state\n+\t\t * as we need to get back to where we started\n+\t\t * by using `git bisect reset`.\n+\t\t */\n+\t\treturn -4;\n \t}\n \n \tbisect_rev = &revs.commits->item->object.oid;\n@@ -1024,12 +1036,18 @@ int bisect_next_all(struct repository *r, const char *prefix, int no_checkout)\n \tif (oideq(bisect_rev, current_bad_oid)) {\n \t\tres = error_if_skipped_commits(tried, current_bad_oid);\n \t\tif (res)\n-\t\t\texit(-res);\n+\t\t\treturn res;\n \t\tprintf(\"%s is the first %s commit\\n\", oid_to_hex(bisect_rev),\n \t\t\tterm_bad);\n+\n \t\tshow_diff_tree(r, prefix, revs.commits->item);\n-\t\t/* This means the bisection process succeeded. */\n-\t\texit(10);\n+\t\t/*\n+\t\t * This means the bisection process succeeded.\n+\t\t * Using -10 so that the call chain can simply check\n+\t\t * for negative return values for early returns up\n+\t\t * until the cmd_bisect__helper() caller.\n+\t\t */\n+\t\treturn -10;\n \t}\n \n \tnr = all - reaches - 1;\n-- \n2.21.1 (Apple Git-122.3)\n\n"},{"id":"390088","messageId":"20200120143800.900-13-mirucam@gmail.com","threadId":"52666","inReplyTo":"20200120143800.900-1-mirucam@gmail.com","subject":"[PATCH 12/29] bisect--helper: reimplement `bisect_next` and `bisect_auto_next` shell functions in C","fromName":"Miriam Rubio","fromEmail":"mirucam@gmail.com","sentAt":"2020-01-20T14:37:43Z","receivedAt":"2020-01-20T14:38:48Z","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\nUsing `--bisect-next` and `--bisect-auto-start` 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-start`\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 | 10 +++\n builtin/bisect--helper.c | 174 ++++++++++++++++++++++++++++++++++++++-\n git-bisect.sh | 47 ++---------\n 3 files changed, 188 insertions(+), 43 deletions(-)\n\ndiff --git a/bisect.c b/bisect.c\nindex 33f2829c19..1c13da8a28 100644\n--- a/bisect.c\n+++ b/bisect.c\n@@ -635,6 +635,12 @@ static void bisect_rev_setup(struct repository *r, struct rev_info *revs,\n \tstruct argv_array rev_argv = ARGV_ARRAY_INIT;\n \tint i;\n \n+\t/*\n+\t * Since the code is slowly being converted to C, there might be\n+\t * instances where the revisions were initialized before. Thus\n+\t * we first need to reset it.\n+\t */\n+\treset_revision_walk();\n \trepo_init_revisions(r, revs, prefix);\n \trevs->abbrev = 0;\n \trevs->commit_format = CMIT_FMT_UNSPECIFIED;\n@@ -971,6 +977,10 @@ void read_bisect_terms(const char **read_bad, const char **read_good)\n * finished successfully.\n * In this case the calling function or command should not turn a -10\n * return code into an error or a non zero exit code.\n+ * This returned -10 is checked in bisect_helper::bisect_next() and\n+ * eventually transformed to 0 at the end of\n+ * bisect_helper::cmd_bisect__helper().\n+ *\n * If no_checkout is non-zero, the bisection process does not\n * checkout the trial commit but instead simply updates BISECT_HEAD.\n */\ndiff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\nindex 5e0f759d50..29bbc1573b 100644\n--- a/builtin/bisect--helper.c\n+++ b/builtin/bisect--helper.c\n@@ -8,6 +8,7 @@\n #include \"run-command.h\"\n #include \"prompt.h\"\n #include \"quote.h\"\n+#include \"revision.h\"\n \n static GIT_PATH_FUNC(git_path_bisect_terms, \"BISECT_TERMS\")\n static GIT_PATH_FUNC(git_path_bisect_expected_rev, \"BISECT_EXPECTED_REV\")\n@@ -29,6 +30,8 @@ static const char * const git_bisect_helper_usage[] = {\n \tN_(\"git bisect--helper --bisect-terms [--term-good | --term-old | --term-bad | --term-new]\"),\n \tN_(\"git bisect--helper --bisect-start [--term-{old,good}= --term-{new,bad}=]\"\n \t\t\t\t\t \"[--no-checkout] [ [...]] [--] [...]\"),\n+\tN_(\"git bisect--helper --bisect-next\"),\n+\tN_(\"git bisect--helper --bisect-auto-next\"),\n \tNULL\n };\n \n@@ -421,6 +424,157 @@ static int bisect_append_log_quoted(const char **argv)\n \treturn res;\n }\n \n+static int register_good_ref(const char *refname,\n+\t\t\t const struct object_id *oid, int flags,\n+\t\t\t void *cb_data)\n+{\n+\tstruct string_list *good_refs = cb_data;\n+\tstring_list_append(good_refs, oid_to_hex(oid));\n+\treturn 0;\n+}\n+\n+static void prepare_rev_argv(struct bisect_terms *terms, struct argv_array *rev_argv)\n+{\n+\tstruct string_list good_revs = STRING_LIST_INIT_DUP;\n+\tchar *term_good = xstrfmt(\"%s-*\", terms->term_good);\n+\n+\tfor_each_glob_ref_in(register_good_ref, term_good,\n+\t\t\t \"refs/bisect/\", &good_revs);\n+\n+\targv_array_pushl(rev_argv, \"skipped_commits\", \"refs/bisect/bad\", \"--not\", NULL);\n+\tfor (int i = 0; i < good_revs.nr; i++)\n+\t\targv_array_push(rev_argv, good_revs.items[i].string);\n+\n+\tstring_list_clear(&good_revs, 0);\n+\tfree(term_good);\n+}\n+\n+static int prepare_revs(struct bisect_terms *terms, struct rev_info *revs)\n+{\n+\tint res = 0;\n+\tstruct argv_array rev_argv = ARGV_ARRAY_INIT;\n+\n+\tprepare_rev_argv(terms, &rev_argv);\n+\n+\t/*\n+\t * It is important to reset the flags used by revision walks\n+\t * as the previous call to bisect_next_all() in turn\n+\t * setups a revision walk.\n+\t */\n+\treset_revision_walk();\n+\tinit_revisions(revs, NULL);\n+\trev_argv.argc = setup_revisions(rev_argv.argc, rev_argv.argv, revs, NULL);\n+\tif (prepare_revision_walk(revs))\n+\t\tres = error(_(\"revision walk setup failed\\n\"));\n+\n+\targv_array_clear(&rev_argv);\n+\treturn res;\n+}\n+\n+static int process_skipped_commits(FILE *fp, struct bisect_terms *terms, struct rev_info *revs)\n+{\n+\tstruct commit *commit;\n+\tstruct pretty_print_context pp = {0};\n+\n+\tif (fprintf(fp, \"# only skipped commits left to test\\n\") < 1)\n+\t\treturn -1;\n+\n+\twhile ((commit = get_revision(revs)) != NULL) {\n+\t\tstruct strbuf commit_name = STRBUF_INIT;\n+\t\tformat_commit_message(commit, \"%s\",\n+\t\t\t\t &commit_name, &pp);\n+\t\tfprintf(fp, \"# possible first %s commit: [%s] %s\\n\",\n+\t\t\tterms->term_bad, oid_to_hex(&commit->object.oid),\n+\t\t\tcommit_name.buf);\n+\t\tstrbuf_release(&commit_name);\n+\t}\n+\n+\treturn 0;\n+}\n+\n+static int bisect_skipped_commits(struct bisect_terms *terms)\n+{\n+\tint res = 0;\n+\tFILE *fp = NULL;\n+\tstruct rev_info revs;\n+\n+\tfp = fopen(git_path_bisect_log(), \"a\");\n+\tif (!fp)\n+\t\treturn error_errno(_(\"could not open '%s' for appending\"),\n+\t\t\t\t git_path_bisect_log());\n+\n+\tres = prepare_revs(terms, &revs);\n+\n+\tif (!res)\n+\t\tres = process_skipped_commits(fp, terms, &revs);\n+\n+\tfclose(fp);\n+\treturn res;\n+}\n+\n+static int bisect_successful(struct bisect_terms *terms)\n+{\n+\tFILE *fp = NULL;\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\",\n+\t\t\t\tterms->term_bad);\n+\tint res = 0;\n+\n+\tread_ref(bad_ref, &oid);\n+\tprintf(\"%s\\n\", bad_ref);\n+\tcommit = lookup_commit_reference(the_repository, &oid);\n+\tformat_commit_message(commit, \"%s\", &commit_name, &pp);\n+\t\n+\tfp = fopen(git_path_bisect_log(), \"a\");\n+\tif (fp) {\n+\t\tif (fprintf(fp, \"# first %s commit: [%s] %s\\n\",\n+\t\t\t terms->term_bad, oid_to_hex(&oid),\n+\t\t\t commit_name.buf) < 1)\n+\t\t\tres = -1;\n+\t\tfclose(fp);\n+\t} else {\n+\t\tres = error_errno(_(\"could not open '%s' for \"\n+\t\t\t\t \"appending\"),\n+\t\t\t\t git_path_bisect_log());\n+\t}\n+\tstrbuf_release(&commit_name);\n+\tfree(bad_ref);\n+\treturn res;\n+}\n+\n+static int bisect_next(struct bisect_terms *terms, const char *prefix)\n+{\n+\tint res, no_checkout;\n+\n+\tif (bisect_next_check(terms, terms->term_good))\n+\t\treturn -1;\n+\n+\tno_checkout = !is_empty_or_missing_file(git_path_bisect_head());\n+\n+\t/* Perform all bisection computation, display and checkout */\n+\tres = bisect_next_all(the_repository, prefix, no_checkout);\n+\n+\tif (res == -10) {\n+\t\tres = bisect_successful(terms);\n+\t\treturn res ? res : -11;\n+\t} else if (res == -2) {\n+\t\tres = bisect_skipped_commits(terms);\n+\t\treturn res ? res : -2;\n+\t}\n+\treturn res;\n+}\n+\n+static int bisect_auto_next(struct bisect_terms *terms, const char *prefix)\n+{\n+\tif (!bisect_next_check(terms, NULL))\n+\t\treturn bisect_next(terms, prefix);\n+\n+\treturn 0;\n+}\n+\n static int bisect_start(struct bisect_terms *terms, int no_checkout,\n \t\t\tconst char **argv, int argc)\n {\n@@ -625,7 +779,9 @@ 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_NEXT,\n+\t\tBISECT_AUTO_NEXT,\n \t} cmdmode = 0;\n \tint no_checkout = 0, res = 0, nolog = 0;\n \tstruct option options[] = {\n@@ -649,6 +805,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_BOOL(0, \"no-checkout\", &no_checkout,\n \t\t\t N_(\"update BISECT_HEAD instead of checking out the current commit\")),\n \t\tOPT_BOOL(0, \"no-log\", &nolog,\n@@ -710,6 +870,18 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n \t\tset_terms(&terms, \"bad\", \"good\");\n \t\tres = bisect_start(&terms, no_checkout, argv, argc);\n \t\tbreak;\n+\tcase BISECT_NEXT:\n+\t\tif (argc)\n+\t\t\treturn error(_(\"--bisect-next requires 0 arguments\"));\n+\t\tget_terms(&terms);\n+\t\tres = bisect_next(&terms, prefix);\n+\t\tbreak;\n+\tcase BISECT_AUTO_NEXT:\n+\t\tif (argc)\n+\t\t\treturn error(_(\"--bisect-auto-next requires 0 arguments\"));\n+\t\tget_terms(&terms);\n+\t\tres = bisect_auto_next(&terms, prefix);\n+\t\tbreak;\n \tdefault:\n \t\treturn error(\"BUG: unknown subcommand '%d'\", cmdmode);\n \t}\ndiff --git a/git-bisect.sh b/git-bisect.sh\nindex efee12b8b1..7531b74708 100755\n--- a/git-bisect.sh\n+++ b/git-bisect.sh\n@@ -87,7 +87,7 @@ bisect_start() {\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@@ -140,45 +140,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-\tbisect_autostart\n-\tgit bisect--helper --bisect-next-check $TERM_GOOD $TERM_BAD $TERM_GOOD|| exit\n-\n-\t# Perform all bisection computation, display and checkout\n-\tgit bisect--helper --next-all $(test -f \"$GIT_DIR/BISECT_HEAD\" && echo --no-checkout)\n-\tres=$?\n-\n-\t# Check if we should exit because bisection is finished\n-\tif test $res -eq 10\n-\tthen\n-\t\tbad_rev=$(git show-ref --hash --verify refs/bisect/$TERM_BAD)\n-\t\tbad_commit=$(git show-branch $bad_rev)\n-\t\techo \"# first $TERM_BAD commit: $bad_commit\" >>\"$GIT_DIR/BISECT_LOG\"\n-\t\texit 0\n-\telif test $res -eq 2\n-\tthen\n-\t\techo \"# only skipped commits left to test\" >>\"$GIT_DIR/BISECT_LOG\"\n-\t\tgood_revs=$(git for-each-ref --format=\"%(objectname)\" \"refs/bisect/$TERM_GOOD-*\")\n-\t\tfor skipped in $(git rev-list refs/bisect/$TERM_BAD --not $good_revs)\n-\t\tdo\n-\t\t\tskipped_commit=$(git show-branch $skipped)\n-\t\t\techo \"# possible first $TERM_BAD commit: $skipped_commit\" >>\"$GIT_DIR/BISECT_LOG\"\n-\t\tdone\n-\t\texit $res\n-\tfi\n-\n-\t# Check for an error in the bisection process\n-\ttest $res -ne 0 && exit $res\n-\n-\treturn 0\n+\tgit bisect--helper --bisect-auto-next\n }\n \n bisect_visualize() {\n@@ -232,7 +194,7 @@ bisect_replay () {\n \t\t\tdie \"$(gettext \"?? what are you talking about?\")\" ;;\n \t\tesac\n \tdone <\"$file\"\n-\tbisect_auto_next\n+\tgit bisect--helper --bisect-auto-next\n }\n \n bisect_run () {\n@@ -329,7 +291,8 @@ 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\tget_terms\n+\t\tgit bisect--helper --bisect-next \"$@\" || exit ;;\n \tvisualize|view)\n \t\tbisect_visualize \"$@\" ;;\n \treset)\n-- \n2.21.1 (Apple Git-122.3)\n\n"},{"id":"390090","messageId":"20200120143800.900-14-mirucam@gmail.com","threadId":"52666","inReplyTo":"20200120143800.900-1-mirucam@gmail.com","subject":"[PATCH 13/29] bisect--helper: finish porting `bisect_start()` to C","fromName":"Miriam Rubio","fromEmail":"mirucam@gmail.com","sentAt":"2020-01-20T14:37:44Z","receivedAt":"2020-01-20T14:38:48Z","isPatch":true,"sender":{"key":"mirucam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56339109?v=4"},"body":"From: Pranit Bauva \n\nAdd the subcommand to `git bisect--helper` and call it from\ngit-bisect.sh.\n\nWith the conversion of `bisect_auto_next()` from shell to C in a\nprevious commit, `bisect_start()` can now be fully ported to C.\n\nSo let's complete the `--bisect-start` subcommand of\n`git bisect--helper` so that it fully implements `bisect_start()`,\nand let's use this subcommand in `git-bisect.sh` instead of\n`bisect_start()`.\n\nThis removes the signal handling we had in `bisect_start()` as we\ndon't really need it. While at it, \"trap\" is changed to \"handle\".\nAs \"trap\" is a reference to the shell \"trap\" builtin, which isn't\nused anymore.\n\nMentored-by: Christian Couder \nMentored-by: Johannes Schindelin \nSigned-off-by: Pranit Bauva \nSigned-off-by: Tanushree Tumane \nSigned-off-by: Miriam Rubio \n---\n builtin/bisect--helper.c | 38 +++++++++++++++++++++++++++++++++-----\n git-bisect.sh | 29 +++--------------------------\n 2 files changed, 36 insertions(+), 31 deletions(-)\n\ndiff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\nindex 29bbc1573b..e604334c91 100644\n--- a/builtin/bisect--helper.c\n+++ b/builtin/bisect--helper.c\n@@ -632,9 +632,12 @@ static int bisect_start(struct bisect_terms *terms, int no_checkout,\n \t\t\treturn error(_(\"unrecognized option: '%s'\"), arg);\n \t\t} else {\n \t\t\tchar *commit_id = xstrfmt(\"%s^{commit}\", arg);\n-\t\t\tif (get_oid(commit_id, &oid) && has_double_dash)\n-\t\t\t\tdie(_(\"'%s' does not appear to be a valid \"\n-\t\t\t\t \"revision\"), arg);\n+\t\t\tif (get_oid(commit_id, &oid) && has_double_dash) {\n+\t\t\t\terror(_(\"'%s' does not appear to be a valid \"\n+\t\t\t\t\t\"revision\"), arg);\n+\t\t\t\tfree(commit_id);\n+\t\t\t\treturn -1;\n+\t\t\t}\n \n \t\t\tstring_list_append(&revs, oid_to_hex(&oid));\n \t\t\tfree(commit_id);\n@@ -715,9 +718,9 @@ static int bisect_start(struct bisect_terms *terms, int no_checkout,\n \t\treturn -1;\n \n \t/*\n-\t * In case of mistaken revs or checkout error, or signals received,\n+\t * In case of mistaken revs or checkout error,\n \t * \"bisect_auto_next\" below may exit or misbehave.\n-\t * We have to trap this to be able to clean up using\n+\t * We have to handle this to be able to clean up using\n \t * \"bisect_clean_state\".\n \t */\n \n@@ -764,6 +767,31 @@ static int bisect_start(struct bisect_terms *terms, int no_checkout,\n \tstring_list_clear(&states, 0);\n \tstrbuf_release(&start_head);\n \tstrbuf_release(&bisect_names);\n+\tif (res)\n+\t\treturn res;\n+\n+\tres = bisect_auto_next(terms, NULL);\n+\t/*\n+\t * In case of mistaken revs or checkout error, or signals received,\n+\t * \"bisect_auto_next\" below may exit or misbehave.\n+\t * We have to handle this to be able to clean up using\n+\t * \"bisect_clean_state\".\n+\t * return code -11 is special code that indicates special success.\n+\t *\t-> bisect_start()\n+\t *\t . res = bisect_auto_next()\n+\t *\t -> bisect_auto_next()\n+\t *\t . return bisect_next()\n+\t *\t -> bisect_next()\n+\t *\t\t . res = bisect_next_all()\n+\t *\t\t -> bisect_next_all()\n+\t *\t\t . res = check_good_are_ancestors_of_bad()\n+\t *\t\t -> check_good_are_ancestors_of_bad()\n+\t *\t\t\t. res = check_merge_bases()\n+\t *\t\t\t-> check_merge_bases()\n+\t *\t\t\t . res = -11\n+\t */\n+\tif (res && res != -11)\n+\t\tbisect_clean_state();\n \treturn res;\n }\n \ndiff --git a/git-bisect.sh b/git-bisect.sh\nindex 7531b74708..fec527e1ef 100755\n--- a/git-bisect.sh\n+++ b/git-bisect.sh\n@@ -63,35 +63,13 @@ bisect_autostart() {\n \t\t\t[Nn]*)\n \t\t\t\texit ;;\n \t\t\tesac\n-\t\t\tbisect_start\n+\t\t\tgit bisect--helper --bisect-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-\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-\tget_terms\n-\tgit bisect--helper --bisect-auto-next || exit\n-\n-\ttrap '-' 0\n-}\n-\n bisect_skip() {\n \tall=''\n \tfor arg in \"$@\"\n@@ -184,8 +162,7 @@ bisect_replay () {\n \t\tget_terms\n \t\tcase \"$command\" in\n \t\tstart)\n-\t\t\tcmd=\"bisect_start $rev\"\n-\t\t\teval \"$cmd\" ;;\n+\t\t\teval \"git bisect--helper --bisect-start $rev\" ;;\n \t\t\"$TERM_GOOD\"|\"$TERM_BAD\"|skip)\n \t\t\tgit bisect--helper --bisect-write \"$command\" \"$rev\" \"$TERM_GOOD\" \"$TERM_BAD\" || exit;;\n \t\tterms)\n@@ -284,7 +261,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.21.1 (Apple Git-122.3)\n\n"},{"id":"390089","messageId":"20200120143800.900-7-mirucam@gmail.com","threadId":"52666","inReplyTo":"20200120143800.900-1-mirucam@gmail.com","subject":"[PATCH 06/29] bisect: libify `exit_if_skipped_commits` to `error_if_skipped*` and its dependents","fromName":"Miriam Rubio","fromEmail":"mirucam@gmail.com","sentAt":"2020-01-20T14:37:37Z","receivedAt":"2020-01-20T14:38:49Z","isPatch":true,"sender":{"key":"mirucam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56339109?v=4"},"body":"From: Pranit Bauva \n\nSince we want to get rid of git-bisect.sh it would be necessary to\nconvert those exit() calls to return statements so that errors can be\nreported.\n\nEmulate try catch in C by converting `exit()` to\n`return `. Follow POSIX conventions to return\n to indicate error.\nModify `cmd_bisect_helper()` to handle these negative returns.\n\nTurn `exit()` to `return` calls in `exit_if_skipped_commits()` and rename\nthe method to `error_if_skipped_commits()`.\n\nHandle this return in dependant function `bisect_next_all()`.\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 bisect.c | 23 ++++++++++++++++-------\n 1 file changed, 16 insertions(+), 7 deletions(-)\n\ndiff --git a/bisect.c b/bisect.c\nindex 83cb5b3a98..2ac0463327 100644\n--- a/bisect.c\n+++ b/bisect.c\n@@ -661,11 +661,11 @@ static void bisect_common(struct rev_info *revs)\n \t\tmark_edges_uninteresting(revs, NULL, 0);\n }\n \n-static void exit_if_skipped_commits(struct commit_list *tried,\n+static int error_if_skipped_commits(struct commit_list *tried,\n \t\t\t\t const struct object_id *bad)\n {\n \tif (!tried)\n-\t\treturn;\n+\t\treturn 0;\n \n \tprintf(\"There are only 'skip'ped commits left to test.\\n\"\n \t \"The first %s commit could be any of:\\n\", term_bad);\n@@ -676,7 +676,13 @@ static void exit_if_skipped_commits(struct commit_list *tried,\n \tif (bad)\n \t\tprintf(\"%s\\n\", oid_to_hex(bad));\n \tprintf(_(\"We cannot bisect more!\\n\"));\n-\texit(2);\n+\n+\t/*\n+\t * We don't want to clean the bisection state\n+\t * as we need to get back to where we started\n+\t * by using `git bisect reset`.\n+\t */\n+\treturn -2;\n }\n \n static int is_expected_rev(const struct object_id *oid)\n@@ -949,7 +955,7 @@ int bisect_next_all(struct repository *r, const char *prefix, int no_checkout)\n {\n \tstruct rev_info revs;\n \tstruct commit_list *tried;\n-\tint reaches = 0, all = 0, nr, steps;\n+\tint reaches = 0, all = 0, nr, steps, res;\n \tstruct object_id *bisect_rev;\n \tchar *steps_msg;\n \n@@ -972,8 +978,9 @@ int bisect_next_all(struct repository *r, const char *prefix, int no_checkout)\n \t\t * We should exit here only if the \"bad\"\n \t\t * commit is also a \"skip\" commit.\n \t\t */\n-\t\texit_if_skipped_commits(tried, NULL);\n-\n+\t\tres = error_if_skipped_commits(tried, NULL);\n+\t\tif (res)\n+\t\t\texit(-res);\n \t\tprintf(_(\"%s was both %s and %s\\n\"),\n \t\t oid_to_hex(current_bad_oid),\n \t\t term_good,\n@@ -990,7 +997,9 @@ int bisect_next_all(struct repository *r, const char *prefix, int no_checkout)\n \tbisect_rev = &revs.commits->item->object.oid;\n \n \tif (oideq(bisect_rev, current_bad_oid)) {\n-\t\texit_if_skipped_commits(tried, current_bad_oid);\n+\t\tres = error_if_skipped_commits(tried, current_bad_oid);\n+\t\tif (res)\n+\t\t\texit(-res);\n \t\tprintf(\"%s is the first %s commit\\n\", oid_to_hex(bisect_rev),\n \t\t\tterm_bad);\n \t\tshow_diff_tree(r, prefix, revs.commits->item);\n-- \n2.21.1 (Apple Git-122.3)\n\n"},{"id":"390092","messageId":"20200120143800.900-15-mirucam@gmail.com","threadId":"52666","inReplyTo":"20200120143800.900-1-mirucam@gmail.com","subject":"[PATCH 14/29] bisect--helper: retire `--bisect-clean-state` subcommand","fromName":"Miriam Rubio","fromEmail":"mirucam@gmail.com","sentAt":"2020-01-20T14:37:45Z","receivedAt":"2020-01-20T14:38:50Z","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 e604334c91..7190304a98 100644\n--- a/builtin/bisect--helper.c\n+++ b/builtin/bisect--helper.c\n@@ -22,7 +22,6 @@ static GIT_PATH_FUNC(git_path_bisect_names, \"BISECT_NAMES\")\n static const char * const git_bisect_helper_usage[] = {\n \tN_(\"git bisect--helper --next-all [--no-checkout]\"),\n \tN_(\"git bisect--helper --write-terms \"),\n-\tN_(\"git bisect--helper --bisect-clean-state\"),\n \tN_(\"git bisect--helper --bisect-reset []\"),\n \tN_(\"git bisect--helper --bisect-write [--no-log] \"),\n \tN_(\"git bisect--helper --bisect-check-and-set-terms \"),\n@@ -800,7 +799,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@@ -817,8 +815,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@@ -860,10 +856,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.21.1 (Apple Git-122.3)\n\n"},{"id":"390091","messageId":"20200120143800.900-16-mirucam@gmail.com","threadId":"52666","inReplyTo":"20200120143800.900-1-mirucam@gmail.com","subject":"[PATCH 15/29] bisect--helper: retire `--next-all` subcommand","fromName":"Miriam Rubio","fromEmail":"mirucam@gmail.com","sentAt":"2020-01-20T14:37:46Z","receivedAt":"2020-01-20T14:38:53Z","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 7190304a98..6953c68f93 100644\n--- a/builtin/bisect--helper.c\n+++ b/builtin/bisect--helper.c\n@@ -20,7 +20,6 @@ static GIT_PATH_FUNC(git_path_head_name, \"head-name\")\n static GIT_PATH_FUNC(git_path_bisect_names, \"BISECT_NAMES\")\n \n static const char * const git_bisect_helper_usage[] = {\n-\tN_(\"git bisect--helper --next-all [--no-checkout]\"),\n \tN_(\"git bisect--helper --write-terms \"),\n \tN_(\"git bisect--helper --bisect-reset []\"),\n \tN_(\"git bisect--helper --bisect-write [--no-log] \"),\n@@ -797,8 +796,7 @@ static int bisect_start(struct bisect_terms *terms, int no_checkout,\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@@ -811,8 +809,6 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n \t} cmdmode = 0;\n \tint no_checkout = 0, res = 0, nolog = 0;\n \tstruct option options[] = {\n-\t\tOPT_CMDMODE(0, \"next-all\", &cmdmode,\n-\t\t\t N_(\"perform 'git bisect next'\"), NEXT_ALL),\n \t\tOPT_CMDMODE(0, \"write-terms\", &cmdmode,\n \t\t\t N_(\"write the terms to .git/BISECT_TERMS\"), WRITE_TERMS),\n \t\tOPT_CMDMODE(0, \"check-expected-revs\", &cmdmode,\n@@ -849,9 +845,6 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n \t\tusage_with_options(git_bisect_helper_usage, options);\n \n \tswitch (cmdmode) {\n-\tcase NEXT_ALL:\n-\t\tres = bisect_next_all(the_repository, prefix, no_checkout);\n-\t\tbreak;\n \tcase WRITE_TERMS:\n \t\tif (argc != 2)\n \t\t\treturn error(_(\"--write-terms requires two arguments\"));\n-- \n2.21.1 (Apple Git-122.3)\n\n"},{"id":"390093","messageId":"20200120143800.900-18-mirucam@gmail.com","threadId":"52666","inReplyTo":"20200120143800.900-1-mirucam@gmail.com","subject":"[PATCH 17/29] bisect--helper: reimplement `bisect_state` & `bisect_head` shell functions in C","fromName":"Miriam Rubio","fromEmail":"mirucam@gmail.com","sentAt":"2020-01-20T14:37:48Z","receivedAt":"2020-01-20T14:38:54Z","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 | 80 ++++++++++++++++++++++++++++++++++++++++\n git-bisect.sh | 55 ++-------------------------\n 2 files changed, 84 insertions(+), 51 deletions(-)\n\ndiff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\nindex f03ed23431..fdcc5f47ec 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@@ -823,6 +825,74 @@ static int bisect_autostart(struct bisect_terms *terms)\n \treturn 0;\n }\n \n+static char *bisect_head(void)\n+{\n+\tif (is_empty_or_missing_file(git_path_bisect_head()))\n+\t\treturn \"HEAD\";\n+\telse\n+\t\treturn \"BISECT_HEAD\";\n+}\n+\n+static int bisect_state(struct bisect_terms *terms, const char **argv,\n+\t\t\tint argc)\n+{\n+\tconst char *state = argv[0];\n+\n+\tif (check_and_set_terms(terms, state))\n+\t\treturn -1;\n+\n+\tif (!argc)\n+\t\treturn error(_(\"Please call `--bisect-state` with at least one argument\"));\n+\n+\tif (argc == 1 && one_of(state, terms->term_good,\n+\t terms->term_bad, \"skip\", NULL)) {\n+\t\tconst char *bisected_head = xstrdup(bisect_head());\n+\t\tconst char *hex[1];\n+\t\tstruct object_id oid;\n+\n+\t\tif (get_oid(bisected_head, &oid))\n+\t\t\treturn error(_(\"Bad rev input: %s\"), bisected_head);\n+\t\tif (bisect_write(state, oid_to_hex(&oid), terms, 0))\n+\t\t\treturn -1;\n+\n+\t\t*hex = xstrdup(oid_to_hex(&oid));\n+\t\tcheck_expected_revs(hex, 1);\n+\t\treturn bisect_auto_next(terms, NULL);\n+\t}\n+\n+\tif ((argc == 2 && !strcmp(state, terms->term_bad)) ||\n+\t\t\tone_of(state, terms->term_good, \"skip\", NULL)) {\n+\t\tint i;\n+\t\tstruct string_list hex = STRING_LIST_INIT_DUP;\n+\n+\t\tfor (i = 1; i < argc; i++) {\n+\t\t\tstruct object_id oid;\n+\n+\t\t\tif (get_oid(argv[i], &oid)) {\n+\t\t\t\tstring_list_clear(&hex, 0);\n+\t\t\t\treturn error(_(\"Bad rev input: %s\"), argv[i]);\n+\t\t\t}\n+\t\t\tstring_list_append(&hex, oid_to_hex(&oid));\n+\t\t}\n+\t\tfor (i = 0; i < hex.nr; i++) {\n+\t\t\tconst char **hex_string = (const char **) &hex.items[i].string;\n+\t\t\tif (bisect_write(state, *hex_string, terms, 0)) {\n+\t\t\t\tstring_list_clear(&hex, 0);\n+\t\t\t\treturn -1;\n+\t\t\t}\n+\t\t\tcheck_expected_revs(hex_string, 1);\n+\t\t}\n+\t\tstring_list_clear(&hex, 0);\n+\t\treturn bisect_auto_next(terms, NULL);\n+\t}\n+\n+\tif (!strcmp(state, terms->term_bad))\n+\t\treturn error(_(\"'git bisect %s' can take only one argument.\"),\n+\t\t terms->term_bad);\n+\n+\treturn -1;\n+}\n+\n int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n {\n \tenum {\n@@ -837,6 +907,7 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n \t\tBISECT_NEXT,\n \t\tBISECT_AUTO_NEXT,\n \t\tBISECT_AUTOSTART,\n+\t\tBISECT_STATE\n \t} cmdmode = 0;\n \tint no_checkout = 0, res = 0, nolog = 0;\n \tstruct option options[] = {\n@@ -862,6 +933,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n \t\t\t N_(\"verify the next bisection state then checkout the next bisection commit\"), BISECT_AUTO_NEXT),\n \t\tOPT_CMDMODE(0, \"bisect-autostart\", &cmdmode,\n \t\t\t N_(\"start the bisection if BISECT_START is empty or missing\"), BISECT_AUTOSTART),\n+\t\tOPT_CMDMODE(0, \"bisect-state\", &cmdmode,\n+\t\t\t N_(\"mark the state of ref (or refs)\"), BISECT_STATE),\n \t\tOPT_BOOL(0, \"no-checkout\", &no_checkout,\n \t\t\t N_(\"update BISECT_HEAD instead of checking out the current commit\")),\n \t\tOPT_BOOL(0, \"no-log\", &nolog,\n@@ -934,6 +1007,13 @@ 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\tif (!argc)\n+\t\t\treturn error(_(\"--bisect-state requires at least one revision\"));\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\treturn error(\"BUG: unknown subcommand '%d'\", cmdmode);\n \t}\ndiff --git a/git-bisect.sh b/git-bisect.sh\nindex 97bb15b38f..4a5afc7a93 100755\n--- a/git-bisect.sh\n+++ b/git-bisect.sh\n@@ -39,16 +39,6 @@ _x40=\"$_x40$_x40$_x40$_x40$_x40$_x40$_x40$_x40\"\n TERM_BAD=bad\n TERM_GOOD=good\n \n-bisect_head()\n-{\n-\tif test -f \"$GIT_DIR/BISECT_HEAD\"\n-\tthen\n-\t\techo BISECT_HEAD\n-\telse\n-\t\techo HEAD\n-\tfi\n-}\n-\n bisect_skip() {\n \tall=''\n \tfor arg in \"$@\"\n@@ -61,43 +51,7 @@ bisect_skip() {\n \t\tesac\n \t\tall=\"$all $revs\"\n \tdone\n-\teval bisect_state 'skip' $all\n-}\n-\n-bisect_state() {\n-\tgit bisect--helper --bisect-autostart\n-\tstate=$1\n-\tgit bisect--helper --check-and-set-terms $state $TERM_GOOD $TERM_BAD || exit\n-\tget_terms\n-\tcase \"$#,$state\" in\n-\t0,*)\n-\t\tdie \"Please call 'bisect_state' with at least one argument.\" ;;\n-\t1,\"$TERM_BAD\"|1,\"$TERM_GOOD\"|1,skip)\n-\t\tbisected_head=$(bisect_head)\n-\t\trev=$(git rev-parse --verify \"$bisected_head\") ||\n-\t\t\tdie \"$(eval_gettext \"Bad rev input: \\$bisected_head\")\"\n-\t\tgit bisect--helper --bisect-write \"$state\" \"$rev\" \"$TERM_GOOD\" \"$TERM_BAD\" || exit\n-\t\tgit bisect--helper --check-expected-revs \"$rev\" ;;\n-\t2,\"$TERM_BAD\"|*,\"$TERM_GOOD\"|*,skip)\n-\t\tshift\n-\t\thash_list=''\n-\t\tfor rev in \"$@\"\n-\t\tdo\n-\t\t\tsha=$(git rev-parse --verify \"$rev^{commit}\") ||\n-\t\t\t\tdie \"$(eval_gettext \"Bad rev input: \\$rev\")\"\n-\t\t\thash_list=\"$hash_list $sha\"\n-\t\tdone\n-\t\tfor rev in $hash_list\n-\t\tdo\n-\t\t\tgit bisect--helper --bisect-write \"$state\" \"$rev\" \"$TERM_GOOD\" \"$TERM_BAD\" || exit\n-\t\tdone\n-\t\tgit bisect--helper --check-expected-revs $hash_list ;;\n-\t*,\"$TERM_BAD\")\n-\t\tdie \"$(eval_gettext \"'git bisect \\$TERM_BAD' can take only one argument.\")\" ;;\n-\t*)\n-\t\tusage ;;\n-\tesac\n-\tgit bisect--helper --bisect-auto-next\n+\teval git bisect--helper --bisect-state 'skip' $all\n }\n \n bisect_visualize() {\n@@ -185,8 +139,7 @@ exit code \\$res from '\\$command' is < 0 or >= 128\" >&2\n \t\t\tstate=\"$TERM_GOOD\"\n \t\tfi\n \n-\t\t# We have to use a subshell because \"bisect_state\" can exit.\n-\t\t( bisect_state $state >\"$GIT_DIR/BISECT_RUN\" )\n+\t\t( git bisect--helper --bisect-state $state >\"$GIT_DIR/BISECT_RUN\" )\n \t\tres=$?\n \n \t\tcat \"$GIT_DIR/BISECT_RUN\"\n@@ -201,7 +154,7 @@ exit code \\$res from '\\$command' is < 0 or >= 128\" >&2\n \t\tif [ $res -ne 0 ]\n \t\tthen\n \t\t\teval_gettextln \"bisect run failed:\n-'bisect_state \\$state' exited with error code \\$res\" >&2\n+'git bisect--helper --bisect-state \\$state' exited with error code \\$res\" >&2\n \t\t\texit $res\n \t\tfi\n \n@@ -242,7 +195,7 @@ case \"$#\" in\n \tstart)\n \t\tgit bisect--helper --bisect-start \"$@\" ;;\n \tbad|good|new|old|\"$TERM_BAD\"|\"$TERM_GOOD\")\n-\t\tbisect_state \"$cmd\" \"$@\" ;;\n+\t\tgit bisect--helper --bisect-state \"$cmd\" \"$@\" ;;\n \tskip)\n \t\tbisect_skip \"$@\" ;;\n \tnext)\n-- \n2.21.1 (Apple Git-122.3)\n\n"},{"id":"390094","messageId":"20200120143800.900-17-mirucam@gmail.com","threadId":"52666","inReplyTo":"20200120143800.900-1-mirucam@gmail.com","subject":"[PATCH 16/29] bisect--helper: reimplement `bisect_autostart` shell function in C","fromName":"Miriam Rubio","fromEmail":"mirucam@gmail.com","sentAt":"2020-01-20T14:37:47Z","receivedAt":"2020-01-20T14:38:55Z","isPatch":true,"sender":{"key":"mirucam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56339109?v=4"},"body":"From: Pranit Bauva \n\nReimplement the `bisect_autostart()` shell function in C and add the\nC implementation from `bisect_next()` which was previously left\nuncovered. Also add a subcommand `--bisect-autostart` to\n`git bisect--helper` be called from `bisect_state()` from\ngit-bisect.sh .\n\nUsing `--bisect-autostart` subcommand is a temporary measure to port\nshell function to C so as to use the existing test suite. As more\nfunctions are ported, this subcommand will be retired and\nbisect_autostart() will be called directly by `bisect_state()`.\n\nMentored-by: Lars Schneider \nMentored-by: Christian Couder \nMentored-by: Johannes Schindelin \nSigned-off-by: Pranit Bauva \nSigned-off-by: Tanushree Tumane \nSigned-off-by: Miriam Rubio \n---\n builtin/bisect--helper.c | 39 +++++++++++++++++++++++++++++++++++++++\n git-bisect.sh | 23 +----------------------\n 2 files changed, 40 insertions(+), 22 deletions(-)\n\ndiff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\nindex 6953c68f93..f03ed23431 100644\n--- a/builtin/bisect--helper.c\n+++ b/builtin/bisect--helper.c\n@@ -30,6 +30,7 @@ static const char * const git_bisect_helper_usage[] = {\n \t\t\t\t\t \"[--no-checkout] [ [...]] [--] [...]\"),\n \tN_(\"git bisect--helper --bisect-next\"),\n \tN_(\"git bisect--helper --bisect-auto-next\"),\n+\tN_(\"git bisect--helper --bisect-autostart\"),\n \tNULL\n };\n \n@@ -56,6 +57,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@@ -547,6 +550,7 @@ static int bisect_next(struct bisect_terms *terms, const char *prefix)\n {\n \tint res, no_checkout;\n \n+\tbisect_autostart(terms);\n \tif (bisect_next_check(terms, terms->term_good))\n \t\treturn -1;\n \n@@ -793,6 +797,32 @@ static int bisect_start(struct bisect_terms *terms, int no_checkout,\n \treturn res;\n }\n \n+static int bisect_autostart(struct bisect_terms *terms)\n+{\n+\tif (is_empty_or_missing_file(git_path_bisect_start())) {\n+\t\tconst char *yesno;\n+\t\tconst char *argv[] = {NULL};\n+\t\tfprintf(stderr, _(\"You need to start by \\\"git bisect \"\n+\t\t\t\t \"start\\\"\\n\"));\n+\n+\t\tif (!isatty(STDIN_FILENO))\n+\t\t\treturn 1;\n+\n+\t\t/*\n+\t\t * TRANSLATORS: Make sure to include [Y] and [n] in your\n+\t\t * translation. The program will only accept English input\n+\t\t * at this point.\n+\t\t */\n+\t\tyesno = git_prompt(_(\"Do you want me to do it for you \"\n+\t\t\t\t \"[Y/n]? \"), PROMPT_ECHO);\n+\t\tif (starts_with(yesno, _(\"n\")) || starts_with(yesno, _(\"N\")))\n+\t\t\treturn 1;\n+\n+\t\treturn bisect_start(terms, 0, argv, 0);\n+\t}\n+\treturn 0;\n+}\n+\n int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n {\n \tenum {\n@@ -806,6 +836,7 @@ 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} cmdmode = 0;\n \tint no_checkout = 0, res = 0, nolog = 0;\n \tstruct option options[] = {\n@@ -829,6 +860,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n \t\t\t N_(\"find the next bisection commit\"), BISECT_NEXT),\n \t\tOPT_CMDMODE(0, \"bisect-auto-next\", &cmdmode,\n \t\t\t N_(\"verify the next bisection state then checkout the next bisection commit\"), BISECT_AUTO_NEXT),\n+\t\tOPT_CMDMODE(0, \"bisect-autostart\", &cmdmode,\n+\t\t\t N_(\"start the bisection if BISECT_START is empty or missing\"), BISECT_AUTOSTART),\n \t\tOPT_BOOL(0, \"no-checkout\", &no_checkout,\n \t\t\t N_(\"update BISECT_HEAD instead of checking out the current commit\")),\n \t\tOPT_BOOL(0, \"no-log\", &nolog,\n@@ -895,6 +928,12 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n \t\tget_terms(&terms);\n \t\tres = bisect_auto_next(&terms, prefix);\n \t\tbreak;\n+\tcase BISECT_AUTOSTART:\n+\t\tif (argc)\n+\t\t\treturn error(_(\"--bisect-autostart requires 0 arguments\"));\n+\t\tset_terms(&terms, \"bad\", \"good\");\n+\t\tres = bisect_autostart(&terms);\n+\t\tbreak;\n \tdefault:\n \t\treturn error(\"BUG: unknown subcommand '%d'\", cmdmode);\n \t}\ndiff --git a/git-bisect.sh b/git-bisect.sh\nindex fec527e1ef..97bb15b38f 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\tgit bisect--helper --bisect-start\n-\t\telse\n-\t\t\texit 1\n-\t\tfi\n-\t}\n-}\n-\n bisect_skip() {\n \tall=''\n \tfor arg in \"$@\"\n@@ -86,7 +65,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-- \n2.21.1 (Apple Git-122.3)\n\n"},{"id":"390095","messageId":"20200120143800.900-20-mirucam@gmail.com","threadId":"52666","inReplyTo":"20200120143800.900-1-mirucam@gmail.com","subject":"[PATCH 19/29] bisect--helper: retire `--write-terms` subcommand","fromName":"Miriam Rubio","fromEmail":"mirucam@gmail.com","sentAt":"2020-01-20T14:37:50Z","receivedAt":"2020-01-20T14:38:56Z","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 3868d34a29..4ff8035cd8 100644\n--- a/builtin/bisect--helper.c\n+++ b/builtin/bisect--helper.c\n@@ -20,7 +20,6 @@ static GIT_PATH_FUNC(git_path_head_name, \"head-name\")\n static GIT_PATH_FUNC(git_path_bisect_names, \"BISECT_NAMES\")\n \n static const char * const git_bisect_helper_usage[] = {\n-\tN_(\"git bisect--helper --write-terms \"),\n \tN_(\"git bisect--helper --bisect-reset []\"),\n \tN_(\"git bisect--helper --bisect-write [--no-log] \"),\n \tN_(\"git bisect--helper --bisect-check-and-set-terms \"),\n@@ -896,8 +895,7 @@ static int bisect_state(struct bisect_terms *terms, const char **argv,\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@@ -910,8 +908,6 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n \t} cmdmode = 0;\n \tint no_checkout = 0, res = 0, nolog = 0;\n \tstruct option options[] = {\n-\t\tOPT_CMDMODE(0, \"write-terms\", &cmdmode,\n-\t\t\t N_(\"write the terms to .git/BISECT_TERMS\"), WRITE_TERMS),\n \t\tOPT_CMDMODE(0, \"bisect-reset\", &cmdmode,\n \t\t\t N_(\"reset the bisection state\"), BISECT_RESET),\n \t\tOPT_CMDMODE(0, \"bisect-write\", &cmdmode,\n@@ -948,10 +944,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.21.1 (Apple Git-122.3)\n\n"},{"id":"390096","messageId":"20200120143800.900-19-mirucam@gmail.com","threadId":"52666","inReplyTo":"20200120143800.900-1-mirucam@gmail.com","subject":"[PATCH 18/29] bisect--helper: retire `--check-expected-revs` subcommand","fromName":"Miriam Rubio","fromEmail":"mirucam@gmail.com","sentAt":"2020-01-20T14:37:49Z","receivedAt":"2020-01-20T14:38:57Z","isPatch":true,"sender":{"key":"mirucam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56339109?v=4"},"body":"From: Pranit Bauva \n\nThe `--check-expected-revs` subcommand is no longer used from the\ngit-bisect.sh shell script. Instead the function\n`check_expected_revs()` is called from the C implementation of\n`bisect-next()`.\n\nMentored-by: Lars Schneider \nMentored-by: Christian Couder \nMentored-by: Johannes Schindelin \nSigned-off-by: Pranit Bauva \nSigned-off-by: Tanushree Tumane \nSigned-off-by: Miriam Rubio \n---\n builtin/bisect--helper.c | 6 ------\n 1 file changed, 6 deletions(-)\n\ndiff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\nindex fdcc5f47ec..3868d34a29 100644\n--- a/builtin/bisect--helper.c\n+++ b/builtin/bisect--helper.c\n@@ -897,7 +897,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@@ -913,8 +912,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@@ -955,9 +952,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.21.1 (Apple Git-122.3)\n\n"},{"id":"390097","messageId":"20200120143800.900-21-mirucam@gmail.com","threadId":"52666","inReplyTo":"20200120143800.900-1-mirucam@gmail.com","subject":"[PATCH 20/29] bisect--helper: reimplement `bisect_log` shell function in C","fromName":"Miriam Rubio","fromEmail":"mirucam@gmail.com","sentAt":"2020-01-20T14:37:51Z","receivedAt":"2020-01-20T14:38:59Z","isPatch":true,"sender":{"key":"mirucam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56339109?v=4"},"body":"From: Pranit Bauva \n\nReimplement the `bisect_log()` shell function in C and also add\n`--bisect-log` subcommand to `git bisect--helper` to call it from\ngit-bisect.sh .\n\nUsing `--bisect-log` subcommand is a temporary measure to port shell\nfunction to C so as to use the existing test suite.\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 | 22 +++++++++++++++++++++-\n git-bisect.sh | 7 +------\n 2 files changed, 22 insertions(+), 7 deletions(-)\n\ndiff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\nindex 4ff8035cd8..4795b7880c 100644\n--- a/builtin/bisect--helper.c\n+++ b/builtin/bisect--helper.c\n@@ -892,6 +892,18 @@ static int bisect_state(struct bisect_terms *terms, const char **argv,\n \treturn -1;\n }\n \n+static int bisect_log(void)\n+{\n+\tint fd, status;\n+\tfd = open(git_path_bisect_log(), O_RDONLY);\n+\tif (fd < 0)\n+\t\treturn -1;\n+\n+\tstatus = copy_fd(fd, STDOUT_FILENO);\n+\tclose(fd);\n+ return status ? -1 : 0;\n+}\n+\n int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n {\n \tenum {\n@@ -904,7 +916,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n \t\tBISECT_NEXT,\n \t\tBISECT_AUTO_NEXT,\n \t\tBISECT_AUTOSTART,\n-\t\tBISECT_STATE\n+\t\tBISECT_STATE,\n+\t\tBISECT_LOG\n \t} cmdmode = 0;\n \tint no_checkout = 0, res = 0, nolog = 0;\n \tstruct option options[] = {\n@@ -928,6 +941,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n \t\t\t N_(\"start the bisection if BISECT_START is empty or missing\"), BISECT_AUTOSTART),\n \t\tOPT_CMDMODE(0, \"bisect-state\", &cmdmode,\n \t\t\t N_(\"mark the state of ref (or refs)\"), BISECT_STATE),\n+\t\tOPT_CMDMODE(0, \"bisect-log\", &cmdmode,\n+\t\t\t N_(\"output the contents of BISECT_LOG\"), BISECT_LOG),\n \t\tOPT_BOOL(0, \"no-checkout\", &no_checkout,\n \t\t\t N_(\"update BISECT_HEAD instead of checking out the current commit\")),\n \t\tOPT_BOOL(0, \"no-log\", &nolog,\n@@ -1000,6 +1015,11 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n \t\tget_terms(&terms);\n \t\tres = bisect_state(&terms, argv, argc);\n \t\tbreak;\n+\tcase BISECT_LOG:\n+\t\tif (argc > 1)\n+\t\t\treturn error(_(\"--bisect-log requires 0 arguments\"));\n+\t\tres = bisect_log();\n+\t\tbreak;\n \tdefault:\n \t\treturn error(\"BUG: unknown subcommand '%d'\", cmdmode);\n \t}\ndiff --git a/git-bisect.sh b/git-bisect.sh\nindex 4a5afc7a93..151358aeda 100755\n--- a/git-bisect.sh\n+++ b/git-bisect.sh\n@@ -167,11 +167,6 @@ exit code \\$res from '\\$command' is < 0 or >= 128\" >&2\n \tdone\n }\n \n-bisect_log () {\n-\ttest -s \"$GIT_DIR/BISECT_LOG\" || die \"$(gettext \"We are not bisecting.\")\"\n-\tcat \"$GIT_DIR/BISECT_LOG\"\n-}\n-\n get_terms () {\n \tif test -s \"$GIT_DIR/BISECT_TERMS\"\n \tthen\n@@ -209,7 +204,7 @@ case \"$#\" in\n \treplay)\n \t\tbisect_replay \"$@\" ;;\n \tlog)\n-\t\tbisect_log ;;\n+\t\tgit bisect--helper --bisect-log ;;\n \trun)\n \t\tbisect_run \"$@\" ;;\n \tterms)\n-- \n2.21.1 (Apple Git-122.3)\n\n"},{"id":"390098","messageId":"20200120143800.900-22-mirucam@gmail.com","threadId":"52666","inReplyTo":"20200120143800.900-1-mirucam@gmail.com","subject":"[PATCH 21/29] bisect--helper: reimplement `bisect_replay` shell function in C","fromName":"Miriam Rubio","fromEmail":"mirucam@gmail.com","sentAt":"2020-01-20T14:37:52Z","receivedAt":"2020-01-20T14:39:00Z","isPatch":true,"sender":{"key":"mirucam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56339109?v=4"},"body":"From: Pranit Bauva \n\nReimplement the `bisect_replay` shell function in C and also add\n`--bisect-replay` subcommand to `git bisect--helper` to call it from\ngit-bisect.sh\n\nUsing `--bisect-replay` subcommand is a temporary measure to port shell\nfunction to C so as to use the existing test suite.\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 | 127 ++++++++++++++++++++++++++++++++++++++-\n git-bisect.sh | 32 +---------\n 2 files changed, 127 insertions(+), 32 deletions(-)\n\ndiff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\nindex 4795b7880c..4b4ce13b50 100644\n--- a/builtin/bisect--helper.c\n+++ b/builtin/bisect--helper.c\n@@ -32,6 +32,7 @@ static const char * const git_bisect_helper_usage[] = {\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+\tN_(\"git bisect--helper --bisect-replay \"),\n \tNULL\n };\n \n@@ -904,6 +905,121 @@ static int bisect_log(void)\n return status ? -1 : 0;\n }\n \n+static int get_next_word(const char *line, int pos, struct strbuf *word)\n+{\n+\tint i, len = strlen(line), begin = 0;\n+\t\n+\tstrbuf_reset(word);\n+\tfor (i = pos; i < len; i++) {\n+\t\tif (line[i] == ' ' && begin)\n+\t\t\treturn i + 1;\n+\n+\t\tif (!begin)\n+\t\t\tbegin = 1;\n+\t\tstrbuf_addch(word, line[i]);\n+\t}\n+\n+\treturn i;\n+}\n+\n+static int process_line(struct bisect_terms *terms, struct strbuf *line, struct strbuf *word)\n+{\n+\tint res = 0;\n+\tint pos = 0;\n+\n+\twhile (pos < line->len) {\n+\t\tpos = get_next_word(line->buf, pos, word);\n+\n+\t\tif (!strcmp(word->buf, \"git\"))\n+\t\t\tcontinue;\n+\t\telse if (!strcmp(word->buf, \"git-bisect\"))\n+\t\t\tcontinue;\n+\t\telse if (!strcmp(word->buf, \"bisect\"))\n+\t\t\tcontinue;\n+\t\telse if (starts_with(word->buf, \"#\"))\n+\t\t\tbreak;\n+\n+\t\tget_terms(terms);\n+\t\tif (check_and_set_terms(terms, word->buf))\n+\t\t\treturn -1;\n+\n+\t\tif (!strcmp(word->buf, \"start\")) {\n+\t\t\tstruct argv_array argv = ARGV_ARRAY_INIT;\n+\t\t\tint res;\n+\t\t\tsq_dequote_to_argv_array(line->buf+pos, &argv);\n+\t\t\tres = bisect_start(terms, 0, argv.argv, argv.argc);\n+\t\t\targv_array_clear(&argv);\n+\t\t\tif (res)\n+\t\t\t\treturn -1;\n+\t\t\tbreak;\n+\t\t}\n+\n+\t\tif (one_of(word->buf, terms->term_good,\n+\t\t\t terms->term_bad, \"skip\", NULL)) {\n+\t\t\tif (bisect_write(word->buf, line->buf+pos, terms, 0))\n+\t\t\t\treturn -1;\n+\t\t\tbreak;\n+\t\t}\n+\n+\t\tif (!strcmp(word->buf, \"terms\")) {\n+\t\t\tstruct argv_array argv = ARGV_ARRAY_INIT;\n+\t\t\tint res;\n+\t\t\tsq_dequote_to_argv_array(line->buf+pos, &argv);\n+\t\t\tres = bisect_terms(terms, argv.argc == 1 ? argv.argv[0] : NULL);\n+\t\t\targv_array_clear(&argv);\n+\t\t\tif (res)\n+\t\t\t\treturn -1;\n+\t\t\tbreak;\n+\t\t}\n+\n+\t\terror(_(\"Replay file contains rubbish (\\\"%s\\\")\"),\n+\t\t word->buf);\n+\t\tres = -1;\n+\t}\n+\treturn res;\n+}\n+\n+static int process_replay_file(FILE *fp, struct bisect_terms *terms)\n+{\n+\tstruct strbuf line = STRBUF_INIT;\n+\tstruct strbuf word = STRBUF_INIT;\n+\tint res = 0;\n+\t \n+\twhile (strbuf_getline(&line, fp) != EOF) {\n+\t\tres = process_line(terms, &line, &word);\n+\t\tif (res)\n+\t\t\tbreak;\n+\t}\n+\n+\tstrbuf_release(&line);\n+\tstrbuf_release(&word);\n+\treturn res;\n+}\n+\n+static int bisect_replay(struct bisect_terms *terms, const char *filename)\n+{\n+\tFILE *fp = NULL;\n+\tint res = 0;\n+\n+\tif (is_empty_or_missing_file(filename))\n+\t\treturn error(_(\"cannot read file '%s' for replaying\"), filename);\n+\n+\tif (bisect_reset(NULL))\n+\t\treturn -1;\n+\n+\tfp = fopen(filename, \"r\");\n+\tif (!fp)\n+\t\treturn -1;\n+\n+\tres = process_replay_file(fp, terms);\n+\tfclose(fp);\n+\t\n+\tif (res)\n+\t\treturn -1;\n+\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@@ -917,7 +1033,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n \t\tBISECT_AUTO_NEXT,\n \t\tBISECT_AUTOSTART,\n \t\tBISECT_STATE,\n-\t\tBISECT_LOG\n+\t\tBISECT_LOG,\n+\t\tBISECT_REPLAY\n \t} cmdmode = 0;\n \tint no_checkout = 0, res = 0, nolog = 0;\n \tstruct option options[] = {\n@@ -943,6 +1060,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n \t\t\t N_(\"mark the state of ref (or refs)\"), BISECT_STATE),\n \t\tOPT_CMDMODE(0, \"bisect-log\", &cmdmode,\n \t\t\t N_(\"output the contents of BISECT_LOG\"), BISECT_LOG),\n+\t\tOPT_CMDMODE(0, \"bisect-replay\", &cmdmode,\n+\t\t\t N_(\"replay the bisection process from the given file\"), BISECT_REPLAY),\n \t\tOPT_BOOL(0, \"no-checkout\", &no_checkout,\n \t\t\t N_(\"update BISECT_HEAD instead of checking out the current commit\")),\n \t\tOPT_BOOL(0, \"no-log\", &nolog,\n@@ -1020,6 +1139,12 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n \t\t\treturn error(_(\"--bisect-log requires 0 arguments\"));\n \t\tres = bisect_log();\n \t\tbreak;\n+\tcase BISECT_REPLAY:\n+\t\tif (argc != 1)\n+\t\t\treturn error(_(\"no logfile given\"));\n+\t\tset_terms(&terms, \"bad\", \"good\");\n+\t\tres = bisect_replay(&terms, argv[0]);\n+\t\tbreak;\n \tdefault:\n \t\treturn error(\"BUG: unknown subcommand '%d'\", cmdmode);\n \t}\ndiff --git a/git-bisect.sh b/git-bisect.sh\nindex 151358aeda..0555191c41 100755\n--- a/git-bisect.sh\n+++ b/git-bisect.sh\n@@ -77,36 +77,6 @@ bisect_visualize() {\n \teval '\"$@\"' --bisect -- $(cat \"$GIT_DIR/BISECT_NAMES\")\n }\n \n-bisect_replay () {\n-\tfile=\"$1\"\n-\ttest \"$#\" -eq 1 || die \"$(gettext \"No logfile given\")\"\n-\ttest -r \"$file\" || die \"$(eval_gettext \"cannot read \\$file for replaying\")\"\n-\tgit bisect--helper --bisect-reset || exit\n-\twhile read git bisect command rev\n-\tdo\n-\t\ttest \"$git $bisect\" = \"git bisect\" || test \"$git\" = \"git-bisect\" || continue\n-\t\tif test \"$git\" = \"git-bisect\"\n-\t\tthen\n-\t\t\trev=\"$command\"\n-\t\t\tcommand=\"$bisect\"\n-\t\tfi\n-\t\tget_terms\n-\t\tgit bisect--helper --check-and-set-terms \"$command\" \"$TERM_GOOD\" \"$TERM_BAD\" || exit\n-\t\tget_terms\n-\t\tcase \"$command\" in\n-\t\tstart)\n-\t\t\teval \"git bisect--helper --bisect-start $rev\" ;;\n-\t\t\"$TERM_GOOD\"|\"$TERM_BAD\"|skip)\n-\t\t\tgit bisect--helper --bisect-write \"$command\" \"$rev\" \"$TERM_GOOD\" \"$TERM_BAD\" || exit;;\n-\t\tterms)\n-\t\t\tgit bisect--helper --bisect-terms $rev || exit;;\n-\t\t*)\n-\t\t\tdie \"$(gettext \"?? what are you talking about?\")\" ;;\n-\t\tesac\n-\tdone <\"$file\"\n-\tgit bisect--helper --bisect-auto-next\n-}\n-\n bisect_run () {\n \tgit bisect--helper --bisect-next-check $TERM_GOOD $TERM_BAD fail || exit\n \n@@ -202,7 +172,7 @@ case \"$#\" in\n \treset)\n \t\tgit bisect--helper --bisect-reset \"$@\" ;;\n \treplay)\n-\t\tbisect_replay \"$@\" ;;\n+\t\tgit bisect--helper --bisect-replay \"$@\" ;;\n \tlog)\n \t\tgit bisect--helper --bisect-log ;;\n \trun)\n-- \n2.21.1 (Apple Git-122.3)\n\n"},{"id":"390099","messageId":"20200120143800.900-23-mirucam@gmail.com","threadId":"52666","inReplyTo":"20200120143800.900-1-mirucam@gmail.com","subject":"[PATCH 22/29] bisect--helper: retire `--bisect-write` subcommand","fromName":"Miriam Rubio","fromEmail":"mirucam@gmail.com","sentAt":"2020-01-20T14:37:53Z","receivedAt":"2020-01-20T14:39:01Z","isPatch":true,"sender":{"key":"mirucam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56339109?v=4"},"body":"From: Pranit Bauva \n\nThe `--bisect-write` subcommand is no longer used from the\ngit-bisect.sh shell script. Instead the function `bisect_write()`\nis 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 | 9 ---------\n 1 file changed, 9 deletions(-)\n\ndiff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\nindex 4b4ce13b50..95ac1a4558 100644\n--- a/builtin/bisect--helper.c\n+++ b/builtin/bisect--helper.c\n@@ -21,7 +21,6 @@ static GIT_PATH_FUNC(git_path_bisect_names, \"BISECT_NAMES\")\n \n static const char * const git_bisect_helper_usage[] = {\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 \tN_(\"git bisect--helper --bisect-next-check []\"),\n \tN_(\"git bisect--helper --bisect-terms [--term-good | --term-old | --term-bad | --term-new]\"),\n@@ -1024,7 +1023,6 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n {\n \tenum {\n \t\tBISECT_RESET = 1,\n-\t\tBISECT_WRITE,\n \t\tCHECK_AND_SET_TERMS,\n \t\tBISECT_NEXT_CHECK,\n \t\tBISECT_TERMS,\n@@ -1040,8 +1038,6 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n \tstruct option options[] = {\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-\t\t\t N_(\"write out the bisection state in BISECT_LOG\"), BISECT_WRITE),\n \t\tOPT_CMDMODE(0, \"check-and-set-terms\", &cmdmode,\n \t\t\t N_(\"check and set terms in a bisection state\"), CHECK_AND_SET_TERMS),\n \t\tOPT_CMDMODE(0, \"bisect-next-check\", &cmdmode,\n@@ -1082,11 +1078,6 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n \t\tif (argc > 1)\n \t\t\treturn error(_(\"--bisect-reset requires either no argument or a commit\"));\n \t\treturn !!bisect_reset(argc ? argv[0] : NULL);\n-\tcase BISECT_WRITE:\n-\t\tif (argc != 4 && argc != 5)\n-\t\t\treturn error(_(\"--bisect-write requires either 4 or 5 arguments\"));\n-\t\tset_terms(&terms, argv[3], argv[2]);\n-\t\tres = bisect_write(argv[0], argv[1], &terms, nolog);\n \t\tbreak;\n \tcase CHECK_AND_SET_TERMS:\n \t\tif (argc != 3)\n-- \n2.21.1 (Apple Git-122.3)\n\n"},{"id":"390102","messageId":"20200120143800.900-25-mirucam@gmail.com","threadId":"52666","inReplyTo":"20200120143800.900-1-mirucam@gmail.com","subject":"[PATCH 24/29] bisect--helper: retire `--bisect-autostart` subcommand","fromName":"Miriam Rubio","fromEmail":"mirucam@gmail.com","sentAt":"2020-01-20T14:37:55Z","receivedAt":"2020-01-20T14:39:01Z","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 378b41cf70..9becc1d514 100644\n--- a/builtin/bisect--helper.c\n+++ b/builtin/bisect--helper.c\n@@ -28,7 +28,6 @@ static const char * const git_bisect_helper_usage[] = {\n \t\t\t\t\t \"[--no-checkout] [ [...]] [--] [...]\"),\n \tN_(\"git bisect--helper --bisect-next\"),\n \tN_(\"git bisect--helper --bisect-auto-next\"),\n-\tN_(\"git bisect--helper --bisect-autostart\"),\n \tN_(\"git bisect--helper --bisect-state (bad|new) []\"),\n \tN_(\"git bisect--helper --bisect-state (good|old) [...]\"),\n \tN_(\"git bisect--helper --bisect-replay \"),\n@@ -1029,7 +1028,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\tBISECT_LOG,\n \t\tBISECT_REPLAY\n@@ -1050,8 +1048,6 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n \t\t\t N_(\"find the next bisection commit\"), BISECT_NEXT),\n \t\tOPT_CMDMODE(0, \"bisect-auto-next\", &cmdmode,\n \t\t\t N_(\"verify the next bisection state then checkout the next bisection commit\"), BISECT_AUTO_NEXT),\n-\t\tOPT_CMDMODE(0, \"bisect-autostart\", &cmdmode,\n-\t\t\t N_(\"start the bisection if BISECT_START is empty or missing\"), BISECT_AUTOSTART),\n \t\tOPT_CMDMODE(0, \"bisect-state\", &cmdmode,\n \t\t\t N_(\"mark the state of ref (or refs)\"), BISECT_STATE),\n \t\tOPT_CMDMODE(0, \"bisect-log\", &cmdmode,\n@@ -1112,12 +1108,6 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n \t\tget_terms(&terms);\n \t\tres = bisect_auto_next(&terms, prefix);\n \t\tbreak;\n-\tcase BISECT_AUTOSTART:\n-\t\tif (argc)\n-\t\t\treturn error(_(\"--bisect-autostart requires 0 arguments\"));\n-\t\tset_terms(&terms, \"bad\", \"good\");\n-\t\tres = bisect_autostart(&terms);\n-\t\tbreak;\n \tcase BISECT_STATE:\n \t\tif (!argc)\n \t\t\treturn error(_(\"--bisect-state requires at least one revision\"));\n-- \n2.21.1 (Apple Git-122.3)\n\n"},{"id":"390100","messageId":"20200120143800.900-26-mirucam@gmail.com","threadId":"52666","inReplyTo":"20200120143800.900-1-mirucam@gmail.com","subject":"[PATCH 25/29] bisect--helper: retire `--bisect-auto-next` subcommand","fromName":"Miriam Rubio","fromEmail":"mirucam@gmail.com","sentAt":"2020-01-20T14:37:56Z","receivedAt":"2020-01-20T14:39:03Z","isPatch":true,"sender":{"key":"mirucam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56339109?v=4"},"body":"From: Pranit Bauva \n\nThe --bisect-auto-next subcommand is no longer used from the\ngit-bisect.sh shell script. Instead the function bisect_auto_next()\nis 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 9becc1d514..a40477811e 100644\n--- a/builtin/bisect--helper.c\n+++ b/builtin/bisect--helper.c\n@@ -27,7 +27,6 @@ static const char * const git_bisect_helper_usage[] = {\n \tN_(\"git bisect--helper --bisect-start [--term-{old,good}= --term-{new,bad}=]\"\n \t\t\t\t\t \"[--no-checkout] [ [...]] [--] [...]\"),\n \tN_(\"git bisect--helper --bisect-next\"),\n-\tN_(\"git bisect--helper --bisect-auto-next\"),\n \tN_(\"git bisect--helper --bisect-state (bad|new) []\"),\n \tN_(\"git bisect--helper --bisect-state (good|old) [...]\"),\n \tN_(\"git bisect--helper --bisect-replay \"),\n@@ -1027,7 +1026,6 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n \t\tBISECT_TERMS,\n \t\tBISECT_START,\n \t\tBISECT_NEXT,\n-\t\tBISECT_AUTO_NEXT,\n \t\tBISECT_STATE,\n \t\tBISECT_LOG,\n \t\tBISECT_REPLAY\n@@ -1046,8 +1044,6 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\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-state\", &cmdmode,\n \t\t\t N_(\"mark the state of ref (or refs)\"), BISECT_STATE),\n \t\tOPT_CMDMODE(0, \"bisect-log\", &cmdmode,\n@@ -1102,12 +1098,6 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\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_STATE:\n \t\tif (!argc)\n \t\t\treturn error(_(\"--bisect-state requires at least one revision\"));\n-- \n2.21.1 (Apple Git-122.3)\n\n"},{"id":"390101","messageId":"20200120143800.900-24-mirucam@gmail.com","threadId":"52666","inReplyTo":"20200120143800.900-1-mirucam@gmail.com","subject":"[PATCH 23/29] bisect--helper: use `res` instead of return in BISECT_RESET case option","fromName":"Miriam Rubio","fromEmail":"mirucam@gmail.com","sentAt":"2020-01-20T14:37:54Z","receivedAt":"2020-01-20T14:39:03Z","isPatch":true,"sender":{"key":"mirucam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56339109?v=4"},"body":"From: Pranit Bauva \n\nUse `res` variable to store `bisect_reset()` output in BISECT_RESET\ncase option to make bisect--helper.c more consistent.\n\nMentored-by: Christian Couder \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 95ac1a4558..378b41cf70 100644\n--- a/builtin/bisect--helper.c\n+++ b/builtin/bisect--helper.c\n@@ -1077,7 +1077,7 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n \tcase BISECT_RESET:\n \t\tif (argc > 1)\n \t\t\treturn error(_(\"--bisect-reset requires either no argument or a commit\"));\n-\t\treturn !!bisect_reset(argc ? argv[0] : NULL);\n+\t\tres = bisect_reset(argc ? argv[0] : NULL);\n \t\tbreak;\n \tcase CHECK_AND_SET_TERMS:\n \t\tif (argc != 3)\n-- \n2.21.1 (Apple Git-122.3)\n\n"},{"id":"390103","messageId":"20200120143800.900-27-mirucam@gmail.com","threadId":"52666","inReplyTo":"20200120143800.900-1-mirucam@gmail.com","subject":"[PATCH 26/29] bisect--helper: reimplement `bisect_skip` shell function in C","fromName":"Miriam Rubio","fromEmail":"mirucam@gmail.com","sentAt":"2020-01-20T14:37:57Z","receivedAt":"2020-01-20T14:39:05Z","isPatch":true,"sender":{"key":"mirucam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56339109?v=4"},"body":"From: Pranit Bauva \n\nReimplement the `bisect_skip()` shell function in C and also add\n`bisect-skip` subcommand to `git bisect--helper` to call it from\ngit-bisect.sh\n\nUsing `--bisect-skip` subcommand is a temporary measure to port shell\nfunction to C so as to use the existing test suite.\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 | 46 +++++++++++++++++++++++++++++++++++++++-\n git-bisect.sh | 17 +--------------\n 2 files changed, 46 insertions(+), 17 deletions(-)\n\ndiff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\nindex a40477811e..841a76fc7c 100644\n--- a/builtin/bisect--helper.c\n+++ b/builtin/bisect--helper.c\n@@ -30,6 +30,7 @@ static const char * const git_bisect_helper_usage[] = {\n \tN_(\"git bisect--helper --bisect-state (bad|new) []\"),\n \tN_(\"git bisect--helper --bisect-state (good|old) [...]\"),\n \tN_(\"git bisect--helper --bisect-replay \"),\n+\tN_(\"git bisect--helper --bisect-skip [(|)...]\"),\n \tNULL\n };\n \n@@ -1017,6 +1018,42 @@ static int bisect_replay(struct bisect_terms *terms, const char *filename)\n \treturn bisect_auto_next(terms, NULL);\n }\n \n+static int bisect_skip(struct bisect_terms *terms, const char **argv, int argc)\n+{\n+\tint i, res;\n+\tconst char *pattern = \"*..*\";\n+\tstruct argv_array argv_state = ARGV_ARRAY_INIT;\n+\n+\targv_array_push(&argv_state, \"skip\");\n+\n+\tfor (i = 0; i < argc; i++) {\n+\t\tif (!wildmatch(pattern, argv[i], 0)) {\n+\t\t\tstruct rev_info revs;\n+\t\t\tstruct commit *commit;\n+\t\t\tstruct argv_array rev_argv = ARGV_ARRAY_INIT;\n+\n+\t\t\targv_array_pushl(&rev_argv, \"skipped_commits\", argv[i], NULL);\n+\t\t\tinit_revisions(&revs, NULL);\n+\t\t\tsetup_revisions(rev_argv.argc, rev_argv.argv, &revs, NULL);\n+\t\t\targv_array_clear(&rev_argv);\n+\n+\t\t\tif (prepare_revision_walk(&revs))\n+\t\t\t\tdie(_(\"revision walk setup failed\\n\"));\n+\t\t\twhile ((commit = get_revision(&revs)) != NULL)\n+\t\t\t\targv_array_push(&argv_state,\n+\t\t\t\t\t\toid_to_hex(&commit->object.oid));\n+\n+\t\t\treset_revision_walk();\n+\t\t} else {\n+\t\t\targv_array_push(&argv_state, argv[i]);\n+\t\t}\n+\t}\n+\tres = bisect_state(terms, argv_state.argv, argv_state.argc);\n+\t\n+\targv_array_clear(&argv_state);\n+\treturn res;\n+}\n+\n int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n {\n \tenum {\n@@ -1028,7 +1065,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n \t\tBISECT_NEXT,\n \t\tBISECT_STATE,\n \t\tBISECT_LOG,\n-\t\tBISECT_REPLAY\n+\t\tBISECT_REPLAY,\n+\t\tBISECT_SKIP\n \t} cmdmode = 0;\n \tint no_checkout = 0, res = 0, nolog = 0;\n \tstruct option options[] = {\n@@ -1050,6 +1088,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n \t\t\t N_(\"output the contents of BISECT_LOG\"), BISECT_LOG),\n \t\tOPT_CMDMODE(0, \"bisect-replay\", &cmdmode,\n \t\t\t N_(\"replay the bisection process from the given file\"), BISECT_REPLAY),\n+\t\tOPT_CMDMODE(0, \"bisect-skip\", &cmdmode,\n+\t\t\t N_(\"skip some commits for checkout\"), BISECT_SKIP),\n \t\tOPT_BOOL(0, \"no-checkout\", &no_checkout,\n \t\t\t N_(\"update BISECT_HEAD instead of checking out the current commit\")),\n \t\tOPT_BOOL(0, \"no-log\", &nolog,\n@@ -1116,6 +1156,10 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n \t\tset_terms(&terms, \"bad\", \"good\");\n \t\tres = bisect_replay(&terms, argv[0]);\n \t\tbreak;\n+\tcase BISECT_SKIP:\n+\t\tset_terms(&terms, \"bad\", \"good\");\n+\t\tres = bisect_skip(&terms, argv, argc);\n+\t\tbreak;\n \tdefault:\n \t\treturn error(\"BUG: unknown subcommand '%d'\", cmdmode);\n \t}\ndiff --git a/git-bisect.sh b/git-bisect.sh\nindex 0555191c41..edfd3f8b3d 100755\n--- a/git-bisect.sh\n+++ b/git-bisect.sh\n@@ -39,21 +39,6 @@ _x40=\"$_x40$_x40$_x40$_x40$_x40$_x40$_x40$_x40\"\n TERM_BAD=bad\n TERM_GOOD=good\n \n-bisect_skip() {\n-\tall=''\n-\tfor arg in \"$@\"\n-\tdo\n-\t\tcase \"$arg\" in\n-\t\t*..*)\n-\t\t\trevs=$(git rev-list \"$arg\") || die \"$(eval_gettext \"Bad rev input: \\$arg\")\" ;;\n-\t\t*)\n-\t\t\trevs=$(git rev-parse --sq-quote \"$arg\") ;;\n-\t\tesac\n-\t\tall=\"$all $revs\"\n-\tdone\n-\teval git bisect--helper --bisect-state 'skip' $all\n-}\n-\n bisect_visualize() {\n \tgit bisect--helper --bisect-next-check $TERM_GOOD $TERM_BAD fail || exit\n \n@@ -162,7 +147,7 @@ case \"$#\" in\n \tbad|good|new|old|\"$TERM_BAD\"|\"$TERM_GOOD\")\n \t\tgit bisect--helper --bisect-state \"$cmd\" \"$@\" ;;\n \tskip)\n-\t\tbisect_skip \"$@\" ;;\n+\t\tgit bisect--helper --bisect-skip \"$@\" ;;\n \tnext)\n \t\t# Not sure we want \"next\" at the UI level anymore.\n \t\tget_terms\n-- \n2.21.1 (Apple Git-122.3)\n\n"},{"id":"390104","messageId":"20200120143800.900-28-mirucam@gmail.com","threadId":"52666","inReplyTo":"20200120143800.900-1-mirucam@gmail.com","subject":"[PATCH 27/29] bisect--helper: retire `--check-and-set-terms` subcommand","fromName":"Miriam Rubio","fromEmail":"mirucam@gmail.com","sentAt":"2020-01-20T14:37:58Z","receivedAt":"2020-01-20T14:39:06Z","isPatch":true,"sender":{"key":"mirucam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56339109?v=4"},"body":"From: Pranit Bauva \n\nThe `--check-and-set-terms` subcommand is no longer from the\ngit-bisect.sh shell script. Instead the function\n`check_and_set_terms()` is 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 841a76fc7c..7bc28d728e 100644\n--- a/builtin/bisect--helper.c\n+++ b/builtin/bisect--helper.c\n@@ -21,7 +21,6 @@ static GIT_PATH_FUNC(git_path_bisect_names, \"BISECT_NAMES\")\n \n static const char * const git_bisect_helper_usage[] = {\n \tN_(\"git bisect--helper --bisect-reset []\"),\n-\tN_(\"git bisect--helper --bisect-check-and-set-terms \"),\n \tN_(\"git bisect--helper --bisect-next-check []\"),\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@@ -1058,7 +1057,6 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n {\n \tenum {\n \t\tBISECT_RESET = 1,\n-\t\tCHECK_AND_SET_TERMS,\n \t\tBISECT_NEXT_CHECK,\n \t\tBISECT_TERMS,\n \t\tBISECT_START,\n@@ -1072,8 +1070,6 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n \tstruct option options[] = {\n \t\tOPT_CMDMODE(0, \"bisect-reset\", &cmdmode,\n \t\t\t N_(\"reset the bisection state\"), BISECT_RESET),\n-\t\tOPT_CMDMODE(0, \"check-and-set-terms\", &cmdmode,\n-\t\t\t N_(\"check and set terms in a bisection state\"), CHECK_AND_SET_TERMS),\n \t\tOPT_CMDMODE(0, \"bisect-next-check\", &cmdmode,\n \t\t\t N_(\"check whether bad or good terms exist\"), BISECT_NEXT_CHECK),\n \t\tOPT_CMDMODE(0, \"bisect-terms\", &cmdmode,\n@@ -1111,12 +1107,6 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n \t\t\treturn error(_(\"--bisect-reset requires either no argument or a commit\"));\n \t\tres = bisect_reset(argc ? argv[0] : NULL);\n \t\tbreak;\n-\tcase CHECK_AND_SET_TERMS:\n-\t\tif (argc != 3)\n-\t\t\treturn error(_(\"--check-and-set-terms requires 3 arguments\"));\n-\t\tset_terms(&terms, argv[2], argv[1]);\n-\t\tres = check_and_set_terms(&terms, argv[0]);\n-\t\tbreak;\n \tcase BISECT_NEXT_CHECK:\n \t\tif (argc != 2 && argc != 3)\n \t\t\treturn error(_(\"--bisect-next-check requires 2 or 3 arguments\"));\n-- \n2.21.1 (Apple Git-122.3)\n\n"},{"id":"390105","messageId":"20200120143800.900-29-mirucam@gmail.com","threadId":"52666","inReplyTo":"20200120143800.900-1-mirucam@gmail.com","subject":"[PATCH 28/29] bisect--helper: reimplement `bisect_visualize()`shell function in C","fromName":"Miriam Rubio","fromEmail":"mirucam@gmail.com","sentAt":"2020-01-20T14:37:59Z","receivedAt":"2020-01-20T14:39:07Z","isPatch":true,"sender":{"key":"mirucam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56339109?v=4"},"body":"From: Pranit Bauva \n\nReimplement the `bisect_visualize()` shell function\nin C and also add `--bisect-visualize` subcommand to\n`git bisect--helper` to call it from git-bisect.sh.\n\nMentored-by: Christian Couder \nMentored-by: Johannes Schindelin \nSigned-off-by: Tanushree Tumane \nSigned-off-by: Miriam Rubio \n---\n builtin/bisect--helper.c | 48 +++++++++++++++++++++++++++++++++++++++-\n git-bisect.sh | 25 +--------------------\n 2 files changed, 48 insertions(+), 25 deletions(-)\n\ndiff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\nindex 7bc28d728e..4b41cc7749 100644\n--- a/builtin/bisect--helper.c\n+++ b/builtin/bisect--helper.c\n@@ -30,6 +30,7 @@ static const char * const git_bisect_helper_usage[] = {\n \tN_(\"git bisect--helper --bisect-state (good|old) [...]\"),\n \tN_(\"git bisect--helper --bisect-replay \"),\n \tN_(\"git bisect--helper --bisect-skip [(|)...]\"),\n+\tN_(\"git bisect--helper --bisect-visualize\"),\n \tNULL\n };\n \n@@ -1053,6 +1054,44 @@ static int bisect_skip(struct bisect_terms *terms, const char **argv, int argc)\n \treturn res;\n }\n \n+static int bisect_visualize(struct bisect_terms *terms, const char **argv, int argc)\n+{\n+\tstruct argv_array args = ARGV_ARRAY_INIT;\n+\tint flags = RUN_COMMAND_NO_STDIN, res = 0;\n+\tstruct strbuf sb;\n+\n+\tif (!bisect_next_check(terms, NULL))\n+\t\treturn -1;\n+\n+\tif (!argc) {\n+\t\tif ((getenv(\"DISPLAY\") || getenv(\"SESSIONNAME\") || getenv(\"MSYSTEM\") ||\n+\t\t getenv(\"SECURITYSESSIONID\")) && exists_in_PATH(\"gitk\"))\n+\t\t\targv_array_push(&args, \"gitk\");\n+\t\telse {\n+\t\t\targv_array_pushl(&args, \"log\", NULL);\n+\t\t\tflags |= RUN_GIT_CMD;\n+\t\t}\n+\t} else {\n+\t\tif (argv[0][0] == '-') {\n+\t\t\targv_array_pushl(&args, \"log\", NULL);\n+\t\t\tflags |= RUN_GIT_CMD;\n+\t\t} else if (strcmp(argv[0], \"tig\") && !starts_with(argv[0], \"git\"))\n+\t\t\tflags |= RUN_GIT_CMD;\n+\n+\t\targv_array_pushv(&args, argv);\n+\t}\n+\n+\targv_array_pushl(&args, \"--bisect\", NULL);\n+\n+\tstrbuf_read_file(&sb, git_path_bisect_names(), 0);\n+\targv_array_split(&args, sb.buf);\n+\tstrbuf_release(&sb);\n+\n+\tres = run_command_v_opt(args.argv, flags);\n+\targv_array_clear(&args);\n+\treturn res;\n+}\n+\n int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n {\n \tenum {\n@@ -1064,7 +1103,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n \t\tBISECT_STATE,\n \t\tBISECT_LOG,\n \t\tBISECT_REPLAY,\n-\t\tBISECT_SKIP\n+\t\tBISECT_SKIP,\n+\t\tBISECT_VISUALIZE\n \t} cmdmode = 0;\n \tint no_checkout = 0, res = 0, nolog = 0;\n \tstruct option options[] = {\n@@ -1086,6 +1126,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n \t\t\t N_(\"replay the bisection process from the given file\"), BISECT_REPLAY),\n \t\tOPT_CMDMODE(0, \"bisect-skip\", &cmdmode,\n \t\t\t N_(\"skip some commits for checkout\"), BISECT_SKIP),\n+\t\tOPT_CMDMODE(0, \"bisect-visualize\", &cmdmode,\n+\t\t\t N_(\"visualize the bisection\"), BISECT_VISUALIZE),\n \t\tOPT_BOOL(0, \"no-checkout\", &no_checkout,\n \t\t\t N_(\"update BISECT_HEAD instead of checking out the current commit\")),\n \t\tOPT_BOOL(0, \"no-log\", &nolog,\n@@ -1150,6 +1192,10 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n \t\tset_terms(&terms, \"bad\", \"good\");\n \t\tres = bisect_skip(&terms, argv, argc);\n \t\tbreak;\n+\tcase BISECT_VISUALIZE:\n+\t\tget_terms(&terms);\n+\t\tres = bisect_visualize(&terms, argv, argc);\n+\t\tbreak;\n \tdefault:\n \t\treturn error(\"BUG: unknown subcommand '%d'\", cmdmode);\n \t}\ndiff --git a/git-bisect.sh b/git-bisect.sh\nindex edfd3f8b3d..084766636d 100755\n--- a/git-bisect.sh\n+++ b/git-bisect.sh\n@@ -39,29 +39,6 @@ _x40=\"$_x40$_x40$_x40$_x40$_x40$_x40$_x40$_x40\"\n TERM_BAD=bad\n TERM_GOOD=good\n \n-bisect_visualize() {\n-\tgit bisect--helper --bisect-next-check $TERM_GOOD $TERM_BAD fail || exit\n-\n-\tif test $# = 0\n-\tthen\n-\t\tif test -n \"${DISPLAY+set}${SESSIONNAME+set}${MSYSTEM+set}${SECURITYSESSIONID+set}\" &&\n-\t\t\ttype gitk >/dev/null 2>&1\n-\t\tthen\n-\t\t\tset gitk\n-\t\telse\n-\t\t\tset git log\n-\t\tfi\n-\telse\n-\t\tcase \"$1\" in\n-\t\tgit*|tig) ;;\n-\t\t-*)\tset git log \"$@\" ;;\n-\t\t*)\tset git \"$@\" ;;\n-\t\tesac\n-\tfi\n-\n-\teval '\"$@\"' --bisect -- $(cat \"$GIT_DIR/BISECT_NAMES\")\n-}\n-\n bisect_run () {\n \tgit bisect--helper --bisect-next-check $TERM_GOOD $TERM_BAD fail || exit\n \n@@ -153,7 +130,7 @@ case \"$#\" in\n \t\tget_terms\n \t\tgit bisect--helper --bisect-next \"$@\" || exit ;;\n \tvisualize|view)\n-\t\tbisect_visualize \"$@\" ;;\n+\t\tgit bisect--helper --bisect-visualize \"$@\" ;;\n \treset)\n \t\tgit bisect--helper --bisect-reset \"$@\" ;;\n \treplay)\n-- \n2.21.1 (Apple Git-122.3)\n\n"},{"id":"390106","messageId":"20200120143800.900-30-mirucam@gmail.com","threadId":"52666","inReplyTo":"20200120143800.900-1-mirucam@gmail.com","subject":"[PATCH 29/29] bisect--helper: reimplement `bisect_run` shell function in C","fromName":"Miriam Rubio","fromEmail":"mirucam@gmail.com","sentAt":"2020-01-20T14:38:00Z","receivedAt":"2020-01-20T14:39:08Z","isPatch":true,"sender":{"key":"mirucam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56339109?v=4"},"body":"From: Tanushree Tumane \n\nReimplement the `bisect_run()` shell function\nin C and also add `--bisect-run` subcommand to\n`git bisect--helper` to call it from git-bisect.sh.\n\nMentored-by: Christian Couder \nSigned-off-by: Tanushree Tumane \nSigned-off-by: Miriam Rubio \n---\n builtin/bisect--helper.c | 66 +++++++++++++++++++++++++++++++++++++++-\n git-bisect.sh | 62 +------------------------------------\n 2 files changed, 66 insertions(+), 62 deletions(-)\n\ndiff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\nindex 4b41cc7749..1899cc8114 100644\n--- a/builtin/bisect--helper.c\n+++ b/builtin/bisect--helper.c\n@@ -31,6 +31,7 @@ static const char * const git_bisect_helper_usage[] = {\n \tN_(\"git bisect--helper --bisect-replay \"),\n \tN_(\"git bisect--helper --bisect-skip [(|)...]\"),\n \tN_(\"git bisect--helper --bisect-visualize\"),\n+\tN_(\"git bisect--helper --bisect-run ...\"),\n \tNULL\n };\n \n@@ -1092,6 +1093,60 @@ static int bisect_visualize(struct bisect_terms *terms, const char **argv, int a\n \treturn res;\n }\n \n+static int bisect_run(struct bisect_terms *terms, const char **argv, int argc)\n+{\n+ \tint i, res = 0;\n+\tstruct strbuf command = STRBUF_INIT;\n+\tstruct argv_array args = ARGV_ARRAY_INIT;\n+\n+\tif (bisect_next_check(terms, NULL))\n+ \treturn -1;\n+\n+\tfor (i = 0; i < argc; i++) {\n+ \tstrbuf_addstr(&command, argv[i]);\n+\t\tstrbuf_addstr(&command, \" \");\n+ }\n+\n+\twhile (1) {\n+\t\targv_array_clear(&args);\n+\n+\t\tprintf(_(\"running %s\"), command.buf);\n+\t\tres = run_command_v_opt(argv, RUN_USING_SHELL);\n+\n+\t\tif (res < 0 && res >= 128) {\n+\t\t\terror(_(\"bisect run failed: exit code %d from\"\n+\t\t\t\t\" '%s' is < 0 or >= 128\"), res, command.buf);\n+\t\t\tstrbuf_release(&command);\n+\t\t\treturn res;\n+\t\t}\n+\n+\t\tif (res == 125)\n+\t\t\targv_array_push(&args, \"skip\");\n+\t\telse if (res > 0)\n+\t\t\targv_array_push(&args, terms->term_bad);\n+\t\telse\n+\t\t\targv_array_push(&args, terms->term_good);\n+\t\t\n+\t\tres = bisect_state(terms, args.argv, args.argc);\n+\t\t\n+\t\tif (res == -11) {\n+ \t\tprintf(_(\"bisect run success\"));\n+ \t\tres = 0;\n+ \t}\n+\t\telse if (res == -2)\n+ \t\terror(_(\"bisect run cannot continue any more\"));\n+ \telse if (res)\n+ \t\terror(_(\"bisect run failed:'git bisect--helper --bisect-state\"\n+ \t\t\t\" %s' exited with error code %d\"), args.argv[0], res);\n+ \telse\n+ \t\tcontinue;\n+\n+\t\tstrbuf_release(&command);\n+\t\targv_array_clear(&args);\n+\t\treturn res;\n+\t}\n+}\n+\n int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n {\n \tenum {\n@@ -1104,7 +1159,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n \t\tBISECT_LOG,\n \t\tBISECT_REPLAY,\n \t\tBISECT_SKIP,\n-\t\tBISECT_VISUALIZE\n+\t\tBISECT_VISUALIZE,\n+\t\tBISECT_RUN,\n \t} cmdmode = 0;\n \tint no_checkout = 0, res = 0, nolog = 0;\n \tstruct option options[] = {\n@@ -1128,6 +1184,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n \t\t\t N_(\"skip some commits for checkout\"), BISECT_SKIP),\n \t\tOPT_CMDMODE(0, \"bisect-visualize\", &cmdmode,\n \t\t\t N_(\"visualize the bisection\"), BISECT_VISUALIZE),\n+\t\tOPT_CMDMODE(0, \"bisect-run\", &cmdmode,\n+\t\t\t N_(\"use ... to automatically bisect.\"), BISECT_RUN),\n \t\tOPT_BOOL(0, \"no-checkout\", &no_checkout,\n \t\t\t N_(\"update BISECT_HEAD instead of checking out the current commit\")),\n \t\tOPT_BOOL(0, \"no-log\", &nolog,\n@@ -1196,6 +1254,12 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n \t\tget_terms(&terms);\n \t\tres = bisect_visualize(&terms, argv, argc);\n \t\tbreak;\n+\tcase BISECT_RUN:\n+\t\tif (!argc)\n+\t\t\treturn error(_(\"bisect run failed: no command provided.\"));\n+\t\tget_terms(&terms);\n+\t\tres = bisect_run(&terms, argv, argc);\n+\t\tbreak;\n \tdefault:\n \t\treturn error(\"BUG: unknown subcommand '%d'\", cmdmode);\n \t}\ndiff --git a/git-bisect.sh b/git-bisect.sh\nindex 084766636d..d4f64674fe 100755\n--- a/git-bisect.sh\n+++ b/git-bisect.sh\n@@ -39,66 +39,6 @@ _x40=\"$_x40$_x40$_x40$_x40$_x40$_x40$_x40$_x40\"\n TERM_BAD=bad\n TERM_GOOD=good\n \n-bisect_run () {\n-\tgit bisect--helper --bisect-next-check $TERM_GOOD $TERM_BAD fail || exit\n-\n-\ttest -n \"$*\" || die \"$(gettext \"bisect run failed: no command provided.\")\"\n-\n-\twhile true\n-\tdo\n-\t\tcommand=\"$@\"\n-\t\teval_gettextln \"running \\$command\"\n-\t\t\"$@\"\n-\t\tres=$?\n-\n-\t\t# Check for really bad run error.\n-\t\tif [ $res -lt 0 -o $res -ge 128 ]\n-\t\tthen\n-\t\t\teval_gettextln \"bisect run failed:\n-exit code \\$res from '\\$command' is < 0 or >= 128\" >&2\n-\t\t\texit $res\n-\t\tfi\n-\n-\t\t# Find current state depending on run success or failure.\n-\t\t# A special exit code of 125 means cannot test.\n-\t\tif [ $res -eq 125 ]\n-\t\tthen\n-\t\t\tstate='skip'\n-\t\telif [ $res -gt 0 ]\n-\t\tthen\n-\t\t\tstate=\"$TERM_BAD\"\n-\t\telse\n-\t\t\tstate=\"$TERM_GOOD\"\n-\t\tfi\n-\n-\t\t( git bisect--helper --bisect-state $state >\"$GIT_DIR/BISECT_RUN\" )\n-\t\tres=$?\n-\n-\t\tcat \"$GIT_DIR/BISECT_RUN\"\n-\n-\t\tif sane_grep \"first $TERM_BAD commit could be any of\" \"$GIT_DIR/BISECT_RUN\" \\\n-\t\t\t>/dev/null\n-\t\tthen\n-\t\t\tgettextln \"bisect run cannot continue any more\" >&2\n-\t\t\texit $res\n-\t\tfi\n-\n-\t\tif [ $res -ne 0 ]\n-\t\tthen\n-\t\t\teval_gettextln \"bisect run failed:\n-'git bisect--helper --bisect-state \\$state' exited with error code \\$res\" >&2\n-\t\t\texit $res\n-\t\tfi\n-\n-\t\tif sane_grep \"is the first $TERM_BAD commit\" \"$GIT_DIR/BISECT_RUN\" >/dev/null\n-\t\tthen\n-\t\t\tgettextln \"bisect run success\"\n-\t\t\texit 0;\n-\t\tfi\n-\n-\tdone\n-}\n-\n get_terms () {\n \tif test -s \"$GIT_DIR/BISECT_TERMS\"\n \tthen\n@@ -138,7 +78,7 @@ case \"$#\" in\n \tlog)\n \t\tgit bisect--helper --bisect-log ;;\n \trun)\n-\t\tbisect_run \"$@\" ;;\n+\t\tgit bisect--helper --bisect-run \"$(git rev-parse --sq-quote \"$@\")\" ;;\n \tterms)\n \t\tgit bisect--helper --bisect-terms \"$@\" || exit;;\n \t*)\n-- \n2.21.1 (Apple Git-122.3)\n\n"},{"id":"390132","messageId":"nycvar.QRO.7.76.6.2001202237140.46@tvgsbejvaqbjf.bet","threadId":"52666","inReplyTo":"20200120143800.900-1-mirucam@gmail.com","subject":"Re: [Outreachy][PATCH 00/29] Finish converting git bisect to C part 1","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-01-20T21:41:23Z","receivedAt":"2020-01-20T21:41:28Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Miriam,\n\nOn Mon, 20 Jan 2020, Miriam Rubio wrote:\n\n> --- Changes since Tanushree’s pr117 sent patch series:\n> https://public-inbox.org/git/pull.117.git.gitgitgadget@gmail.com) ---\n>\n> General changes\n> ---------------\n>\n> * Rebase on master branch.\n> * Improve commit messages.\n> * Amend patch series titles.\n> * Reorder commits: first clean-up/preparatory commits, squash or split\n> commits.\n\nGreat!\n\n> Specific changes\n> ----------------\n>\n> [1/29] bisect--helper: convert `vocab_*` char pointers to char arrays\n>\n> * New patch to convert `vocab_bad` and `vocab_good` char pointers\n> to char arrays\n\n29 patches is _a lot_ to review. I would have preferred a series of\nsmaller patch series.\n\nFor example, the first three patches would have made for a fine \"some\ncleanups\" patch series, from my point of view.\n\nAlso, as the mail's subject says \"part 1\", it would be good to have an\noverview how this part fits into the overall story of converting `git\nbisect` into a built-in.\n\nFinally, it would be nice to have a link to a public repository with the\nbranch from which these mails were generated.\n\nI will try to review this patch series in its entirety, but it will take\nme a while.\n\nCiao,\nJohannes\n\n>\n> --\n>\n> [2/29] bisect--helper: change `retval` to `res`\n>\n> * Replace one last variable `retval` to `res`.\n>\n> --\n>\n> [3/29] bisect: use the standard 'if (!var)' way to check for 0\n>\n> * New patch to use '!var' and make 'bisect.c' more consistent with the\n> rest of the code\n>\n> --\n>\n> [4/29] run-command: make `exists_in_PATH()` non-static\n>\n> * Add comment before function declaration.\n> * Move function declaration in `run-command.h`.\n>\n> --\n>\n> [6/29] bisect: libify `exit_if_skipped_commits` to `error_if_skipped*`\n> and its dependents\n>\n> * Fix `mark_edges_uninteresting()` and `show_diff_tree()` calls after\n> rebase on master.\n>\n> --\n>\n> [7/29] bisect: libify `bisect_checkout`\n>\n> * Fix `memcpy()` call after rebase on master.\n> * Introduce `res` variable to return `bisect_checkout()` output.\n> * Fix `get_commit_reference()` declaration after rebase on master.\n>\n> --\n>\n> [8/29] bisect: libify `check_merge_bases` and its dependents\n>\n> State: Previously sent\n>\n> * Fix `check_ancestors()` declaration after rebase on master.\n> * Fix `get_bad_and_good_commits()` call after rebase on master.\n>\n> --\n>\n> [9/29] bisect: libify `check_good_are_ancestors_of_bad` and its\n> dependents\n>\n> State: Previously sent\n>\n> * Fix `check_good_are_ancestors_of_bad()` declaration after rebase on\n> master.\n> * Fix `check_good_are_ancestors_of_bad()`, `bisect_next_all()`\n> and `bisect_rev_setup()` calls after rebase on master.\n>\n> --\n>\n> [11/29] bisect: libify `bisect_next_all`\n>\n> State: Previously sent\n>\n> * Fix `show_diff_tree()` call after rebase on master.\n>\n> --\n>\n> [12/29] bisect--helper: reimplement `bisect_next` and `bisect_auto_next`\n> shell functions in C\n>\n> * Fix `repo_init_revisions()` and `bisect_next_all()` calls after rebase\n> on master.\n> * Remove `goto` statement in `bisect_skipped_commits()`\n>\n> --\n>\n> [13/29] bisect--helper: finish porting `bisect_start()` to C\n>\n> * Change `return` statement instead of `die` in error handling.\n> * Remove `goto` statements in `bisect_skipped_commits()`.\n>\n> --\n>\n> [21/29] bisect--helper: reimplement `bisect_replay` shell function in C\n>\n> * Add blank line in `get_next_word()`.\n> * Remove `goto` statements in `bisect_replay()`.\n>\n> --\n>\n> [23/29] bisect--helper: use `res` instead of return in BISECT_RESET case\n> option\n>\n> * New patch to split previous commit in two.\n>\n> --\n>\n> [26/29] bisect--helper: reimplement `bisect_skip` shell function in C\n>\n> State: Previously sent\n>\n> * Add blank line.\n>\n> --\n>\n> [28/29] bisect--helper: reimplement `bisect_visualize()`shell function\n> in C\n>\n> New patch:\n>\n> * Reimplement the `bisect_visualize()` shell function in C.\n> * Add `--bisect-visualize` subcommand.\n> * Fix long code line.\n>\n> --\n>\n> [29/29] bisect--helper: reimplement `bisect_run` shell function in C\n>\n> New patch:\n>\n> * Reimplement the `bisect_run()` shell function in C.\n> * Add `--bisect-run` subcommand.\n> * Remove blank line.\n>\n> --\n>\n> Miriam Rubio (2):\n> bisect--helper: convert `vocab_*` char pointers to char arrays\n> bisect: use the standard 'if (!var)' way to check for 0\n>\n> Pranit Bauva (24):\n> run-command: make `exists_in_PATH()` non-static\n> bisect: libify `exit_if_skipped_commits` to `error_if_skipped*` and\n> its dependents\n> bisect: libify `bisect_checkout`\n> bisect: libify `check_merge_bases` and its dependents\n> bisect: libify `check_good_are_ancestors_of_bad` and its dependents\n> bisect: libify `handle_bad_merge_base` and its dependents\n> bisect: libify `bisect_next_all`\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_autostart` shell function in C\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: reimplement `bisect_log` shell function in C\n> bisect--helper: reimplement `bisect_replay` shell function in C\n> bisect--helper: retire `--bisect-write` subcommand\n> bisect--helper: use `res` instead of return in BISECT_RESET case\n> option\n> bisect--helper: retire `--bisect-autostart` subcommand\n> bisect--helper: retire `--bisect-auto-next` subcommand\n> bisect--helper: reimplement `bisect_skip` shell function in C\n> bisect--helper: retire `--check-and-set-terms` subcommand\n> bisect--helper: reimplement `bisect_visualize()`shell function in C\n>\n> Tanushree Tumane (3):\n> bisect--helper: change `retval` to `res`\n> bisect--helper: introduce new `decide_next()` function\n> bisect--helper: reimplement `bisect_run` shell function in C\n>\n> bisect.c | 146 +++++---\n> builtin/bisect--helper.c | 776 +++++++++++++++++++++++++++++++++------\n> git-bisect.sh | 279 +-------------\n> run-command.c | 2 +-\n> run-command.h | 11 +\n> 5 files changed, 793 insertions(+), 421 deletions(-)\n>\n> --\n> 2.21.1 (Apple Git-122.3)\n>\n>\n"},{"id":"390134","messageId":"nycvar.QRO.7.76.6.2001202253450.46@tvgsbejvaqbjf.bet","threadId":"52666","inReplyTo":"20200120143800.900-7-mirucam@gmail.com","subject":"Re: [PATCH 06/29] bisect: libify `exit_if_skipped_commits` to `error_if_skipped*` and its dependents","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-01-20T21:57:40Z","receivedAt":"2020-01-20T21:57:46Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Miriam,\n\nOn Mon, 20 Jan 2020, Miriam Rubio wrote:\n\n> From: Pranit Bauva \n>\n> Since we want to get rid of git-bisect.sh it would be necessary to\n> convert those exit() calls to return statements so that errors can be\n> reported.\n>\n> Emulate try catch in C by converting `exit()` to\n> `return `. Follow POSIX conventions to return\n> to indicate error.\n\nGood.\n\n> Modify `cmd_bisect_helper()` to handle these negative returns.\n>\n> Turn `exit()` to `return` calls in `exit_if_skipped_commits()` and rename\n> the method to `error_if_skipped_commits()`.\n\nI would remove these two sentences, as I deem it obvious from the\nrationale above that this needs to be done, and the patch repeats it.\n\n>\n> Handle this return in dependant function `bisect_next_all()`.\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> bisect.c | 23 ++++++++++++++++-------\n> 1 file changed, 16 insertions(+), 7 deletions(-)\n>\n> diff --git a/bisect.c b/bisect.c\n> index 83cb5b3a98..2ac0463327 100644\n> --- a/bisect.c\n> +++ b/bisect.c\n> @@ -661,11 +661,11 @@ static void bisect_common(struct rev_info *revs)\n> \t\tmark_edges_uninteresting(revs, NULL, 0);\n> }\n>\n> -static void exit_if_skipped_commits(struct commit_list *tried,\n> +static int error_if_skipped_commits(struct commit_list *tried,\n> \t\t\t\t const struct object_id *bad)\n> {\n> \tif (!tried)\n> -\t\treturn;\n> +\t\treturn 0;\n>\n> \tprintf(\"There are only 'skip'ped commits left to test.\\n\"\n> \t \"The first %s commit could be any of:\\n\", term_bad);\n> @@ -676,7 +676,13 @@ static void exit_if_skipped_commits(struct commit_list *tried,\n> \tif (bad)\n> \t\tprintf(\"%s\\n\", oid_to_hex(bad));\n> \tprintf(_(\"We cannot bisect more!\\n\"));\n> -\texit(2);\n> +\n> +\t/*\n> +\t * We don't want to clean the bisection state\n> +\t * as we need to get back to where we started\n> +\t * by using `git bisect reset`.\n> +\t */\n> +\treturn -2;\n\nThis comment is a good indicator that the constant `-2` here is a \"magic\"\nnumber and it would most likely make sense to turn the return type from an\n`int` into an `enum` instead.\n\n> }\n>\n> static int is_expected_rev(const struct object_id *oid)\n> @@ -949,7 +955,7 @@ int bisect_next_all(struct repository *r, const char *prefix, int no_checkout)\n> {\n> \tstruct rev_info revs;\n> \tstruct commit_list *tried;\n> -\tint reaches = 0, all = 0, nr, steps;\n> +\tint reaches = 0, all = 0, nr, steps, res;\n> \tstruct object_id *bisect_rev;\n> \tchar *steps_msg;\n>\n> @@ -972,8 +978,9 @@ int bisect_next_all(struct repository *r, const char *prefix, int no_checkout)\n> \t\t * We should exit here only if the \"bad\"\n> \t\t * commit is also a \"skip\" commit.\n> \t\t */\n> -\t\texit_if_skipped_commits(tried, NULL);\n> -\n> +\t\tres = error_if_skipped_commits(tried, NULL);\n> +\t\tif (res)\n> +\t\t\texit(-res);\n\nSo we still `exit()` in `libgit.a`? I hoped for a more thorough\nlibification.\n\nBesides, the `if (res)` probably wants to be an `if (res < 0)`, right?\n\nCiao,\nJohannes\n\n> \t\tprintf(_(\"%s was both %s and %s\\n\"),\n> \t\t oid_to_hex(current_bad_oid),\n> \t\t term_good,\n> @@ -990,7 +997,9 @@ int bisect_next_all(struct repository *r, const char *prefix, int no_checkout)\n> \tbisect_rev = &revs.commits->item->object.oid;\n>\n> \tif (oideq(bisect_rev, current_bad_oid)) {\n> -\t\texit_if_skipped_commits(tried, current_bad_oid);\n> +\t\tres = error_if_skipped_commits(tried, current_bad_oid);\n> +\t\tif (res)\n> +\t\t\texit(-res);\n> \t\tprintf(\"%s is the first %s commit\\n\", oid_to_hex(bisect_rev),\n> \t\t\tterm_bad);\n> \t\tshow_diff_tree(r, prefix, revs.commits->item);\n> --\n> 2.21.1 (Apple Git-122.3)\n>\n>\n"},{"id":"390136","messageId":"nycvar.QRO.7.76.6.2001202308050.46@tvgsbejvaqbjf.bet","threadId":"52666","inReplyTo":"20200120143800.900-9-mirucam@gmail.com","subject":"Re: [PATCH 08/29] bisect: libify `check_merge_bases` and its dependents","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-01-20T22:09:51Z","receivedAt":"2020-01-20T22:09:58Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Mon, 20 Jan 2020, Miriam Rubio wrote:\n\n> From: Pranit Bauva \n>\n> Since we want to get rid of git-bisect.sh it would be necessary to\n> convert those exit() calls to return statements so that errors can be\n> reported.\n>\n> Emulate try catch in C by converting `exit()` to\n> `return `. Follow POSIX conventions to return\n> to indicate error.\n>\n> Turn `exit()` to `return` calls in `check_merge_bases()`.\n>\n> In `check_merge_bases()` there is an early success special case,\n> so we have introduced special error code `-11` which indicates early\n> success. This `-11` is converted back to `0` in `check_good_are_ancestors_of_bad()`.\n>\n> Handle the return value in dependent function `check_good_are_ancestors_of_bad()`.\n\nThis is a lot of repeated text from earlier commit messages. It might make\nsense to condense it a bit, and while at it, to remove sentences that\nessentially repeat what the diff says.\n\n>\n> Mentored-by: Christian Couder \n> Mentored by: Johannes Schindelin \n> Signed-off-by: Pranit Bauva \n> Signed-off-by: Tanushree Tumane \n> Signed-off-by: Miriam Rubio \n> ---\n> bisect.c | 21 ++++++++++++++++-----\n> 1 file changed, 16 insertions(+), 5 deletions(-)\n>\n> diff --git a/bisect.c b/bisect.c\n> index 385afaf875..367258b0dd 100644\n> --- a/bisect.c\n> +++ b/bisect.c\n> @@ -806,13 +806,16 @@ static void handle_skipped_merge_base(const struct object_id *mb)\n> * \"check_merge_bases\" checks that merge bases are not \"bad\" (or \"new\").\n> *\n> * - If one is \"bad\" (or \"new\"), it means the user assumed something wrong\n> - * and we must exit with a non 0 error code.\n> + * and we must return error with a non 0 error code.\n> * - If one is \"good\" (or \"old\"), that's good, we have nothing to do.\n> * - If one is \"skipped\", we can't know but we should warn.\n> * - If we don't know, we should check it out and ask the user to test.\n> + * - If a merge base must be tested, on success return -11 a special condition\n> + * for early success, this will be converted back to 0 in check_good_are_ancestors_of_bad().\n> */\n> -static void check_merge_bases(int rev_nr, struct commit **rev, int no_checkout)\n> +static int check_merge_bases(int rev_nr, struct commit **rev, int no_checkout)\n> {\n> +\tint res = 0;\n> \tstruct commit_list *result;\n>\n> \tresult = get_merge_bases_many(rev[0], rev_nr - 1, rev + 1);\n> @@ -827,11 +830,16 @@ static void check_merge_bases(int rev_nr, struct commit **rev, int no_checkout)\n> \t\t\thandle_skipped_merge_base(mb);\n> \t\t} else {\n> \t\t\tprintf(_(\"Bisecting: a merge base must be tested\\n\"));\n> -\t\t\texit(bisect_checkout(mb, no_checkout));\n> +\t\t\tres = bisect_checkout(mb, no_checkout);\n> +\t\t\tif (!res)\n> +\t\t\t\t/* indicate early success */\n> +\t\t\t\tres = -11;\n\nThis is yet another good candidate for an `enum`.\n\n> +\t\t\tbreak;\n> \t\t}\n> \t}\n>\n> \tfree_commit_list(result);\n> +\treturn res;\n> }\n>\n> static int check_ancestors(struct repository *r, int rev_nr,\n> @@ -865,7 +873,7 @@ static void check_good_are_ancestors_of_bad(struct repository *r,\n> {\n> \tchar *filename = git_pathdup(\"BISECT_ANCESTORS_OK\");\n> \tstruct stat st;\n> -\tint fd, rev_nr;\n> +\tint fd, rev_nr, res = 0;\n> \tstruct commit **rev;\n>\n> \tif (!current_bad_oid)\n> @@ -880,10 +888,13 @@ static void check_good_are_ancestors_of_bad(struct repository *r,\n> \t\tgoto done;\n>\n> \t/* Check if all good revs are ancestor of the bad rev. */\n> +\n> \trev = get_bad_and_good_commits(r, &rev_nr);\n> \tif (check_ancestors(r, rev_nr, rev, prefix))\n> -\t\tcheck_merge_bases(rev_nr, rev, no_checkout);\n> +\t\tres = check_merge_bases(rev_nr, rev, no_checkout);\n> \tfree(rev);\n> +\tif(res)\n\nPlease put a space between the `if` keyword and the `(` following it.\n\nThanks,\nJohannes\n\n> +\t\texit(res == -11 ? 0 : -res);\n>\n> \t/* Create file BISECT_ANCESTORS_OK. */\n> \tfd = open(filename, O_CREAT | O_TRUNC | O_WRONLY, 0600);\n> --\n> 2.21.1 (Apple Git-122.3)\n>\n>\n"},{"id":"390137","messageId":"nycvar.QRO.7.76.6.2001202313130.46@tvgsbejvaqbjf.bet","threadId":"52666","inReplyTo":"20200120143800.900-10-mirucam@gmail.com","subject":"Re: [PATCH 09/29] bisect: libify `check_good_are_ancestors_of_bad` and its dependents","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-01-20T22:20:25Z","receivedAt":"2020-01-20T22:20:31Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Mon, 20 Jan 2020, Miriam Rubio wrote:\n\n> From: Pranit Bauva \n>\n> Since we want to get rid of git-bisect.sh it would be necessary to\n> convert those exit() calls to return statements so that errors can be\n> reported.\n>\n> Emulate try catch in C by converting `exit()` to\n> `return `. Follow POSIX conventions to return\n> to indicate error.\n>\n> Turn `exit()` to `return` calls in `check_good_are_ancestors_of_bad()`.\n>\n> Code that turns -11(early success code) to 0 from\n> `check_good_are_ancestors_of_bad()` has been moved to\n> `cmd_bisect__helper()`.\n>\n> Handle the return value in dependent function `bisect_next_all()`.\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> bisect.c | 42 ++++++++++++++++++++++++++--------------\n> builtin/bisect--helper.c | 12 ++++++++++--\n> 2 files changed, 37 insertions(+), 17 deletions(-)\n>\n> diff --git a/bisect.c b/bisect.c\n> index 367258b0dd..2b80597a1d 100644\n> --- a/bisect.c\n> +++ b/bisect.c\n> @@ -865,9 +865,10 @@ static int check_ancestors(struct repository *r, int rev_nr,\n> *\n> * If that's not the case, we need to check the merge bases.\n> * If a merge base must be tested by the user, its source code will be\n> - * checked out to be tested by the user and we will exit.\n> + * checked out to be tested by the user and we will return.\n> */\n> -static void check_good_are_ancestors_of_bad(struct repository *r,\n> +\n> +static int check_good_are_ancestors_of_bad(struct repository *r,\n> \t\t\t\t\t const char *prefix,\n> \t\t\t\t\t int no_checkout)\n> {\n> @@ -876,8 +877,15 @@ static void check_good_are_ancestors_of_bad(struct repository *r,\n> \tint fd, rev_nr, res = 0;\n> \tstruct commit **rev;\n>\n> -\tif (!current_bad_oid)\n> -\t\tdie(_(\"a %s revision is needed\"), term_bad);\n> +\t/*\n> +\t * We don't want to clean the bisection state\n> +\t * as we need to get back to where we started\n> +\t * by using `git bisect reset`.\n> +\t */\n> +\tif (!current_bad_oid) {\n> +\t\tres = error(_(\"a %s revision is needed\"), term_bad);\n> +\t\tgoto done;\n> +\t}\n\nWhy not just return here? Ah, there is a `filename` that was allocated...\nit is too bad that we have a mailing-list based review, as the hunk\ncontext simply cannot be extended in a mail.\n\nPersonally, I think it would be nicer to split the definition of\n`filename` from its declaration and move it _after_ this conditional code,\nso that we can `return` right away.\n\nHowever, there is a more pressing issue than that: `die()` exits with exit\ncode 128, so in keeping with the idea to hand down negative exit codes as\nreturn values, should we not assign `res = -128` here?\n\n>\n> \t/* Check if file BISECT_ANCESTORS_OK exists. */\n> \tif (!stat(filename, &st) && S_ISREG(st.st_mode))\n> @@ -893,18 +901,20 @@ static void check_good_are_ancestors_of_bad(struct repository *r,\n> \tif (check_ancestors(r, rev_nr, rev, prefix))\n> \t\tres = check_merge_bases(rev_nr, rev, no_checkout);\n> \tfree(rev);\n> -\tif(res)\n> -\t\texit(res == -11 ? 0 : -res);\n> -\n> -\t/* Create file BISECT_ANCESTORS_OK. */\n> -\tfd = open(filename, O_CREAT | O_TRUNC | O_WRONLY, 0600);\n> -\tif (fd < 0)\n> -\t\twarning_errno(_(\"could not create file '%s'\"),\n> -\t\t\t filename);\n> -\telse\n> -\t\tclose(fd);\n> +\n> +\tif (!res)\n> +\t{\n\nWe usually put the `{` on the same line as the `if` condition (like you\ndid in the `if (!current_bad_oid)` line above.\n\nThe rest looks reasonable. Thank you,\nJohannes\n\n> +\t\t/* Create file BISECT_ANCESTORS_OK. */\n> +\t\tfd = open(filename, O_CREAT | O_TRUNC | O_WRONLY, 0600);\n> +\t\tif (fd < 0)\n> +\t\t\twarning_errno(_(\"could not create file '%s'\"),\n> +\t\t\t\t filename);\n> +\t\telse\n> +\t\t\tclose(fd);\n> +\t}\n> done:\n> \tfree(filename);\n> +\treturn res;\n> }\n>\n> /*\n> @@ -975,7 +985,9 @@ int bisect_next_all(struct repository *r, const char *prefix, int no_checkout)\n> \tif (read_bisect_refs())\n> \t\tdie(_(\"reading bisect refs failed\"));\n>\n> -\tcheck_good_are_ancestors_of_bad(r, prefix, no_checkout);\n> +\tres = check_good_are_ancestors_of_bad(r, prefix, no_checkout);\n> +\tif (res)\n> +\t\treturn res;\n>\n> \tbisect_rev_setup(r, &revs, prefix, \"%s\", \"^%s\", 1);\n> \trevs.limited = 1;\n> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\n> index 826fcba2ed..5e0f759d50 100644\n> --- a/builtin/bisect--helper.c\n> +++ b/builtin/bisect--helper.c\n> @@ -666,7 +666,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n>\n> \tswitch (cmdmode) {\n> \tcase NEXT_ALL:\n> -\t\treturn bisect_next_all(the_repository, prefix, no_checkout);\n> +\t\tres = bisect_next_all(the_repository, prefix, no_checkout);\n> +\t\tbreak;\n> \tcase WRITE_TERMS:\n> \t\tif (argc != 2)\n> \t\t\treturn error(_(\"--write-terms requires two arguments\"));\n> @@ -713,5 +714,12 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n> \t\treturn error(\"BUG: unknown subcommand '%d'\", cmdmode);\n> \t}\n> \tfree_terms(&terms);\n> -\treturn !!res;\n> +\t/*\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 == -11)\n> +\t\tres = 0;\n> +\n> +\treturn res < 0 ? -res : res;\n> }\n> --\n> 2.21.1 (Apple Git-122.3)\n>\n>\n"},{"id":"390138","messageId":"nycvar.QRO.7.76.6.2001202321210.46@tvgsbejvaqbjf.bet","threadId":"52666","inReplyTo":"20200120143800.900-11-mirucam@gmail.com","subject":"Re: [PATCH 10/29] bisect: libify `handle_bad_merge_base` and its dependents","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-01-20T22:23:03Z","receivedAt":"2020-01-20T22:23:07Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Miriam,\n\nOn Mon, 20 Jan 2020, Miriam Rubio wrote:\n\n> From: Pranit Bauva \n>\n> Since we want to get rid of git-bisect.sh it would be necessary to\n> convert those exit() calls to return statements so that errors can be\n> reported.\n>\n> Emulate try catch in C by converting `exit()` to\n> `return `. Follow POSIX conventions to return\n> to indicate error.\n>\n> Turn `exit()` to `return` calls in `handle_bad_merge_base()`.\n>\n> Handle the return value in dependent function check_merge_bases().\n\nThis is again a lot of essentially repeated text from earlier commit\nmessages, but the most pressing question is not addressed...\n\n>\n> Mentored-by: Christian Couder \n> Mentored-by: Johannes Schindelin \n> Signed-off-by: Pranit Bauva \n> Signed-off-by: Tanushree Tumane \n> Signed-off-by: Miriam Rubio \n> ---\n> bisect.c | 9 +++++----\n> 1 file changed, 5 insertions(+), 4 deletions(-)\n>\n> diff --git a/bisect.c b/bisect.c\n> index 2b80597a1d..acb5a13911 100644\n> --- a/bisect.c\n> +++ b/bisect.c\n> @@ -756,7 +756,7 @@ static struct commit **get_bad_and_good_commits(struct repository *r,\n> \treturn rev;\n> }\n>\n> -static void handle_bad_merge_base(void)\n> +static int handle_bad_merge_base(void)\n> {\n> \tif (is_expected_rev(current_bad_oid)) {\n> \t\tchar *bad_hex = oid_to_hex(current_bad_oid);\n> @@ -777,14 +777,14 @@ static void handle_bad_merge_base(void)\n> \t\t\t\t\"between %s and [%s].\\n\"),\n> \t\t\t\tbad_hex, term_bad, term_good, bad_hex, good_hex);\n> \t\t}\n> -\t\texit(3);\n> +\t\treturn -3;\n\n... which is: what does `3` stand for?\n\nThanks,\nJohannes\n\n> \t}\n>\n> \tfprintf(stderr, _(\"Some %s revs are not ancestors of the %s rev.\\n\"\n> \t\t\"git bisect cannot work properly in this case.\\n\"\n> \t\t\"Maybe you mistook %s and %s revs?\\n\"),\n> \t\tterm_good, term_bad, term_good, term_bad);\n> -\texit(1);\n> +\treturn -1;\n> }\n>\n> static void handle_skipped_merge_base(const struct object_id *mb)\n> @@ -823,7 +823,8 @@ static int check_merge_bases(int rev_nr, struct commit **rev, int no_checkout)\n> \tfor (; result; result = result->next) {\n> \t\tconst struct object_id *mb = &result->item->object.oid;\n> \t\tif (oideq(mb, current_bad_oid)) {\n> -\t\t\thandle_bad_merge_base();\n> +\t\t\tres = handle_bad_merge_base();\n> +\t\t\tbreak;\n> \t\t} else if (0 <= oid_array_lookup(&good_revs, mb)) {\n> \t\t\tcontinue;\n> \t\t} else if (0 <= oid_array_lookup(&skipped_revs, mb)) {\n> --\n> 2.21.1 (Apple Git-122.3)\n>\n>\n"},{"id":"390139","messageId":"nycvar.QRO.7.76.6.2001202325000.46@tvgsbejvaqbjf.bet","threadId":"52666","inReplyTo":"20200120143800.900-12-mirucam@gmail.com","subject":"Re: [PATCH 11/29] bisect: libify `bisect_next_all`","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-01-20T22:29:14Z","receivedAt":"2020-01-20T22:29:20Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Miriam,\n\nOn Mon, 20 Jan 2020, Miriam Rubio wrote:\n\n> From: Pranit Bauva \n>\n> Since we want to get rid of git-bisect.sh it would be necessary to\n> convert those exit() calls to return statements so that errors can be\n> reported.\n>\n> Emulate try catch in C by converting `exit()` to\n> `return `. Follow POSIX conventions to return\n> to indicate error.\n>\n> Turn `exit()` to `return` calls in `bisect_next_all()`.\n>\n> All the functions calling `bisect_next_all()` are already able to\n> handle return values from it.\n\nSo this patch brings more magic values that really would like to become\n`enum` values. At this point, we're talking about `bisect_next_all()`\nwhich is _not_ a file-local (`static`) function. Therefore, we now really\nwade into API territory where an `enum` is no longer just a nice thing,\nbut a necessary thing: `bisect_next_all()` is a function that can be\ncalled from other source files.\n\nAs far as I can see, this patch concludes the \"libify\" part of the patch\nseries (read: it would have been the second patch series I would have\nsuggested to split out from an otherwise too-long-to-be-reviewed set of\npatches, if I had been your mentor), and I'll leave it at that for\ntonight; Hopefully I will find some time to review more of these patches\ntomorrow.\n\nCiao,\nJohannes\n\n>\n> Mentored-by: Christian Couder \n> Mentored-by: Johannes Schindelin \n> Signed-off-by: Pranit Bauva \n> Signed-off-by: Tanushree Tumane \n> Signed-off-by: Miriam Rubio \n> ---\n> bisect.c | 40 +++++++++++++++++++++++++++++-----------\n> 1 file changed, 29 insertions(+), 11 deletions(-)\n>\n> diff --git a/bisect.c b/bisect.c\n> index acb5a13911..33f2829c19 100644\n> --- a/bisect.c\n> +++ b/bisect.c\n> @@ -967,10 +967,10 @@ void read_bisect_terms(const char **read_bad, const char **read_good)\n> }\n>\n> /*\n> - * We use the convention that exiting with an exit code 10 means that\n> - * the bisection process finished successfully.\n> - * In this case the calling shell script should exit 0.\n> - *\n> + * We use the convention that return -10 means the bisection process\n> + * finished successfully.\n> + * In this case the calling function or command should not turn a -10\n> + * 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> @@ -1000,23 +1000,35 @@ int bisect_next_all(struct repository *r, const char *prefix, int no_checkout)\n>\n> \tif (!revs.commits) {\n> \t\t/*\n> -\t\t * We should exit here only if the \"bad\"\n> +\t\t * We should return error here only if the \"bad\"\n> \t\t * commit is also a \"skip\" commit.\n> \t\t */\n> \t\tres = error_if_skipped_commits(tried, NULL);\n> \t\tif (res)\n> -\t\t\texit(-res);\n> +\t\t\treturn res;\n> \t\tprintf(_(\"%s was both %s and %s\\n\"),\n> \t\t oid_to_hex(current_bad_oid),\n> \t\t term_good,\n> \t\t term_bad);\n> -\t\texit(1);\n> +\n> +\t\t/*\n> +\t\t * We don't want to clean the bisection state\n> +\t\t * as we need to get back to where we started\n> +\t\t * by using `git bisect reset`.\n> +\t\t */\n> +\t\treturn -1;\n> \t}\n>\n> \tif (!all) {\n> \t\tfprintf(stderr, _(\"No testable commit found.\\n\"\n> \t\t\t\"Maybe you started with bad path parameters?\\n\"));\n> -\t\texit(4);\n> +\n> +\t\t/*\n> +\t\t * We don't want to clean the bisection state\n> +\t\t * as we need to get back to where we started\n> +\t\t * by using `git bisect reset`.\n> +\t\t */\n> +\t\treturn -4;\n> \t}\n>\n> \tbisect_rev = &revs.commits->item->object.oid;\n> @@ -1024,12 +1036,18 @@ int bisect_next_all(struct repository *r, const char *prefix, int no_checkout)\n> \tif (oideq(bisect_rev, current_bad_oid)) {\n> \t\tres = error_if_skipped_commits(tried, current_bad_oid);\n> \t\tif (res)\n> -\t\t\texit(-res);\n> +\t\t\treturn res;\n> \t\tprintf(\"%s is the first %s commit\\n\", oid_to_hex(bisect_rev),\n> \t\t\tterm_bad);\n> +\n> \t\tshow_diff_tree(r, prefix, revs.commits->item);\n> -\t\t/* This means the bisection process succeeded. */\n> -\t\texit(10);\n> +\t\t/*\n> +\t\t * This means the bisection process succeeded.\n> +\t\t * Using -10 so that the call chain can simply check\n> +\t\t * for negative return values for early returns up\n> +\t\t * until the cmd_bisect__helper() caller.\n> +\t\t */\n> +\t\treturn -10;\n> \t}\n>\n> \tnr = all - reaches - 1;\n> --\n> 2.21.1 (Apple Git-122.3)\n>\n>\n"},{"id":"390140","messageId":"CAP8UFD0W=kBs1zne56OjPbFzeX7BtEazPh2MW1My3qAsByS=nA@mail.gmail.com","threadId":"52666","inReplyTo":"nycvar.QRO.7.76.6.2001202237140.46@tvgsbejvaqbjf.bet","subject":"Re: [Outreachy][PATCH 00/29] Finish converting git bisect to C part 1","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2020-01-20T23:24:03Z","receivedAt":"2020-01-20T23:24:23Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Hi Dscho,\n\nOn Mon, Jan 20, 2020 at 10:43 PM Johannes Schindelin\n wrote:\n>\n> On Mon, 20 Jan 2020, Miriam Rubio wrote:\n\n> > [1/29] bisect--helper: convert `vocab_*` char pointers to char arrays\n> >\n> > * New patch to convert `vocab_bad` and `vocab_good` char pointers\n> > to char arrays\n>\n> 29 patches is _a lot_ to review. I would have preferred a series of\n> smaller patch series.\n\nYeah, it's possible to split it into smaller patch series. There are a\nmany similar patches in the series so it was easier to work on\neverything together to make similar and consistent changes to all the\npatches at once.\n\n> For example, the first three patches would have made for a fine \"some\n> cleanups\" patch series, from my point of view.\n\nYeah, but this might then be rejected by Junio as it would be only \"code churn\".\n\n> Also, as the mail's subject says \"part 1\", it would be good to have an\n> overview how this part fits into the overall story of converting `git\n> bisect` into a built-in.\n\nWe don't know how the rest will be split yet. Hopefully there will be\nonly one other smaller patch series after this one.\n\n> Finally, it would be nice to have a link to a public repository with the\n> branch from which these mails were generated.\n\nYeah, I agree that would be nice.\n\n> I will try to review this patch series in its entirety, but it will take\n> me a while.\n\nGreat, thanks!\n\nChristian.\n"},{"id":"390146","messageId":"CAP8UFD1T3_1t0Qf3jP1GttNHvaLEWoaeAVeZid6AMH_MAKPjSw@mail.gmail.com","threadId":"52666","inReplyTo":"nycvar.QRO.7.76.6.2001202253450.46@tvgsbejvaqbjf.bet","subject":"Re: [PATCH 06/29] bisect: libify `exit_if_skipped_commits` to `error_if_skipped*` and its dependents","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2020-01-21T06:40:29Z","receivedAt":"2020-01-21T06:40:44Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Hi Dscho,\n\nOn Mon, Jan 20, 2020 at 10:57 PM Johannes Schindelin\n wrote:\n>\n> On Mon, 20 Jan 2020, Miriam Rubio wrote:\n\n> > printf(\"There are only 'skip'ped commits left to test.\\n\"\n> > \"The first %s commit could be any of:\\n\", term_bad);\n> > @@ -676,7 +676,13 @@ static void exit_if_skipped_commits(struct commit_list *tried,\n> > if (bad)\n> > printf(\"%s\\n\", oid_to_hex(bad));\n> > printf(_(\"We cannot bisect more!\\n\"));\n> > - exit(2);\n> > +\n> > + /*\n> > + * We don't want to clean the bisection state\n> > + * as we need to get back to where we started\n> > + * by using `git bisect reset`.\n> > + */\n> > + return -2;\n>\n> This comment is a good indicator that the constant `-2` here is a \"magic\"\n> number and it would most likely make sense to turn the return type from an\n> `int` into an `enum` instead.\n\nMany functions use `return error(...)` and error codes from these\nfunctions and from exit_if_skipped_commits() are going to get mixed.\nSo I am not sure that using an enum for only some of the error codes\nwill make things clearer.\n\n> > static int is_expected_rev(const struct object_id *oid)\n> > @@ -949,7 +955,7 @@ int bisect_next_all(struct repository *r, const char *prefix, int no_checkout)\n> > {\n> > struct rev_info revs;\n> > struct commit_list *tried;\n> > - int reaches = 0, all = 0, nr, steps;\n> > + int reaches = 0, all = 0, nr, steps, res;\n> > struct object_id *bisect_rev;\n> > char *steps_msg;\n> >\n> > @@ -972,8 +978,9 @@ int bisect_next_all(struct repository *r, const char *prefix, int no_checkout)\n> > * We should exit here only if the \"bad\"\n> > * commit is also a \"skip\" commit.\n> > */\n> > - exit_if_skipped_commits(tried, NULL);\n> > -\n> > + res = error_if_skipped_commits(tried, NULL);\n> > + if (res)\n> > + exit(-res);\n>\n> So we still `exit()` in `libgit.a`? I hoped for a more thorough\n> libification.\n\nThe exit() calls are removed in later patches.\n\n> Besides, the `if (res)` probably wants to be an `if (res < 0)`, right?\n\nYeah, I agree.\n\nThanks for your review,\nChristian.\n"},{"id":"390147","messageId":"CAP8UFD0MO9wEc=ErEAFm4kSabLKUTp1a5VTPxgDS2yqt+rxXag@mail.gmail.com","threadId":"52666","inReplyTo":"nycvar.QRO.7.76.6.2001202313130.46@tvgsbejvaqbjf.bet","subject":"Re: [PATCH 09/29] bisect: libify `check_good_are_ancestors_of_bad` and its dependents","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2020-01-21T06:59:02Z","receivedAt":"2020-01-21T06:59:17Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Mon, Jan 20, 2020 at 11:20 PM Johannes Schindelin\n wrote:\n>\n> On Mon, 20 Jan 2020, Miriam Rubio wrote:\n\n> > @@ -876,8 +877,15 @@ static void check_good_are_ancestors_of_bad(struct repository *r,\n> > int fd, rev_nr, res = 0;\n> > struct commit **rev;\n> >\n> > - if (!current_bad_oid)\n> > - die(_(\"a %s revision is needed\"), term_bad);\n> > + /*\n> > + * We don't want to clean the bisection state\n> > + * as we need to get back to where we started\n> > + * by using `git bisect reset`.\n> > + */\n> > + if (!current_bad_oid) {\n> > + res = error(_(\"a %s revision is needed\"), term_bad);\n> > + goto done;\n> > + }\n>\n> Why not just return here? Ah, there is a `filename` that was allocated...\n> it is too bad that we have a mailing-list based review, as the hunk\n> context simply cannot be extended in a mail.\n>\n> Personally, I think it would be nicer to split the definition of\n> `filename` from its declaration and move it _after_ this conditional code,\n> so that we can `return` right away.\n\nYeah, I agree.\n\n> However, there is a more pressing issue than that: `die()` exits with exit\n> code 128, so in keeping with the idea to hand down negative exit codes as\n> return values, should we not assign `res = -128` here?\n\nI think it has been ok when converting git-bisect.sh to C to just\nconvert `die(...)` into `return error(...)`.\n\n> > /* Check if file BISECT_ANCESTORS_OK exists. */\n> > if (!stat(filename, &st) && S_ISREG(st.st_mode))\n> > @@ -893,18 +901,20 @@ static void check_good_are_ancestors_of_bad(struct repository *r,\n> > if (check_ancestors(r, rev_nr, rev, prefix))\n> > res = check_merge_bases(rev_nr, rev, no_checkout);\n> > free(rev);\n> > - if(res)\n> > - exit(res == -11 ? 0 : -res);\n> > -\n> > - /* Create file BISECT_ANCESTORS_OK. */\n> > - fd = open(filename, O_CREAT | O_TRUNC | O_WRONLY, 0600);\n> > - if (fd < 0)\n> > - warning_errno(_(\"could not create file '%s'\"),\n> > - filename);\n> > - else\n> > - close(fd);\n> > +\n> > + if (!res)\n> > + {\n>\n> We usually put the `{` on the same line as the `if` condition (like you\n> did in the `if (!current_bad_oid)` line above.\n>\n> The rest looks reasonable. Thank you,\n\nThank you for your review,\nChristian.\n"},{"id":"390148","messageId":"CAP8UFD0TU9FQQfk6SgDtOi+_eDni0ZJf+Bw-uN_LzAdeVh3HvQ@mail.gmail.com","threadId":"52666","inReplyTo":"nycvar.QRO.7.76.6.2001202321210.46@tvgsbejvaqbjf.bet","subject":"Re: [PATCH 10/29] bisect: libify `handle_bad_merge_base` and its dependents","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2020-01-21T07:05:34Z","receivedAt":"2020-01-21T07:05:48Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Mon, Jan 20, 2020 at 11:23 PM Johannes Schindelin\n wrote:\n\n> On Mon, 20 Jan 2020, Miriam Rubio wrote:\n\n> > @@ -777,14 +777,14 @@ static void handle_bad_merge_base(void)\n> > \"between %s and [%s].\\n\"),\n> > bad_hex, term_bad, term_good, bad_hex, good_hex);\n> > }\n> > - exit(3);\n> > + return -3;\n>\n> ... which is: what does `3` stand for?\n\nMaybe the question should have been answered by adding a comment to\nthe previous patch that added the `exit(3)` statement.\n\nSo yeah we could here add a separate patch that just adds such a comment.\n\nOr maybe we can add such a comment in this patch and say something\nlike \"while at it let's explain a bit the '3' error code\" in the\ncommit message.\n"},{"id":"390149","messageId":"CAP8UFD3QP3QXe32cdK+q=P+v3dK1Bfws5KgNnm-JfDnPkpBZfw@mail.gmail.com","threadId":"52666","inReplyTo":"nycvar.QRO.7.76.6.2001202325000.46@tvgsbejvaqbjf.bet","subject":"Re: [PATCH 11/29] bisect: libify `bisect_next_all`","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2020-01-21T07:15:59Z","receivedAt":"2020-01-21T07:16:14Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Mon, Jan 20, 2020 at 11:29 PM Johannes Schindelin\n wrote:\n\n> On Mon, 20 Jan 2020, Miriam Rubio wrote:\n\n> > Since we want to get rid of git-bisect.sh it would be necessary to\n> > convert those exit() calls to return statements so that errors can be\n> > reported.\n> >\n> > Emulate try catch in C by converting `exit()` to\n> > `return `. Follow POSIX conventions to return\n> > to indicate error.\n> >\n> > Turn `exit()` to `return` calls in `bisect_next_all()`.\n> >\n> > All the functions calling `bisect_next_all()` are already able to\n> > handle return values from it.\n>\n> So this patch brings more magic values that really would like to become\n> `enum` values. At this point, we're talking about `bisect_next_all()`\n> which is _not_ a file-local (`static`) function. Therefore, we now really\n> wade into API territory where an `enum` is no longer just a nice thing,\n> but a necessary thing: `bisect_next_all()` is a function that can be\n> called from other source files.\n\nI agree that return values could be better documented. About enum\nthough, I am perhaps wrong but I don't think that we use them often\nfor error codes. Do you have examples in the code base where they are\nused for such a purpose along with regular `error(...)` calls?\n\n> As far as I can see, this patch concludes the \"libify\" part of the patch\n> series (read: it would have been the second patch series I would have\n> suggested to split out from an otherwise too-long-to-be-reviewed set of\n> patches, if I had been your mentor),\n\nI am ok with saying that \"part 1\" stops after this patch and that the\nrest will be \"part 2\" and will not be resent in the version 2 of the\n\"part 1\" patch series, but will rather be resent later, when \"part 1\"\nwill be in next for example.\n\n> and I'll leave it at that for\n> tonight; Hopefully I will find some time to review more of these patches\n> tomorrow.\n\nThat would be great!\n\nThanks,\nChristian.\n"},{"id":"390150","messageId":"CAN7CjDC-UrD0sTE=jjNm8=tYsS5-X8p_u_8LYuvRix=_QvnxfA@mail.gmail.com","threadId":"52666","inReplyTo":"nycvar.QRO.7.76.6.2001202237140.46@tvgsbejvaqbjf.bet","subject":"Re: [Outreachy][PATCH 00/29] Finish converting git bisect to C part 1","fromName":"Miriam R.","fromEmail":"mirucam@gmail.com","sentAt":"2020-01-21T08:44:13Z","receivedAt":"2020-01-21T08:44:26Z","isPatch":true,"sender":{"key":"mirucam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56339109?v=4"},"body":"Hi Johannes,\n\nEl lun., 20 ene. 2020 a las 22:41, Johannes Schindelin\n() escribió:\n>\n> Hi Miriam,\n>\n> On Mon, 20 Jan 2020, Miriam Rubio wrote:\n>\n> > --- Changes since Tanushree’s pr117 sent patch series:\n> > https://public-inbox.org/git/pull.117.git.gitgitgadget@gmail.com) ---\n> >\n> > General changes\n> > ---------------\n> >\n> > * Rebase on master branch.\n> > * Improve commit messages.\n> > * Amend patch series titles.\n> > * Reorder commits: first clean-up/preparatory commits, squash or split\n> > commits.\n>\n> Great!\n>\n> > Specific changes\n> > ----------------\n> >\n> > [1/29] bisect--helper: convert `vocab_*` char pointers to char arrays\n> >\n> > * New patch to convert `vocab_bad` and `vocab_good` char pointers\n> > to char arrays\n>\n> 29 patches is _a lot_ to review. I would have preferred a series of\n> smaller patch series.\n\nOk, I will discuss with my mentor how is the best way to split all this work.\n\n>\n> For example, the first three patches would have made for a fine \"some\n> cleanups\" patch series, from my point of view.\n>\n> Also, as the mail's subject says \"part 1\", it would be good to have an\n> overview how this part fits into the overall story of converting `git\n> bisect` into a built-in.\n>\n> Finally, it would be nice to have a link to a public repository with the\n> branch from which these mails were generated.\n>\n\nThis is the link of the current branch:\nhttps://gitlab.com/mirucam/git/commits/git-bisect-work2.8.2\nI will add the public repository link on next cover-letters.\nThank you for the suggestion.\n\n> I will try to review this patch series in its entirety, but it will take\n> me a while.\n\nThank you very much for reviewing.\nBest,\nMiriam\n>\n> Ciao,\n> Johannes\n>\n> >\n> > --\n> >\n> > [2/29] bisect--helper: change `retval` to `res`\n> >\n> > * Replace one last variable `retval` to `res`.\n> >\n> > --\n> >\n> > [3/29] bisect: use the standard 'if (!var)' way to check for 0\n> >\n> > * New patch to use '!var' and make 'bisect.c' more consistent with the\n> > rest of the code\n> >\n> > --\n> >\n> > [4/29] run-command: make `exists_in_PATH()` non-static\n> >\n> > * Add comment before function declaration.\n> > * Move function declaration in `run-command.h`.\n> >\n> > --\n> >\n> > [6/29] bisect: libify `exit_if_skipped_commits` to `error_if_skipped*`\n> > and its dependents\n> >\n> > * Fix `mark_edges_uninteresting()` and `show_diff_tree()` calls after\n> > rebase on master.\n> >\n> > --\n> >\n> > [7/29] bisect: libify `bisect_checkout`\n> >\n> > * Fix `memcpy()` call after rebase on master.\n> > * Introduce `res` variable to return `bisect_checkout()` output.\n> > * Fix `get_commit_reference()` declaration after rebase on master.\n> >\n> > --\n> >\n> > [8/29] bisect: libify `check_merge_bases` and its dependents\n> >\n> > State: Previously sent\n> >\n> > * Fix `check_ancestors()` declaration after rebase on master.\n> > * Fix `get_bad_and_good_commits()` call after rebase on master.\n> >\n> > --\n> >\n> > [9/29] bisect: libify `check_good_are_ancestors_of_bad` and its\n> > dependents\n> >\n> > State: Previously sent\n> >\n> > * Fix `check_good_are_ancestors_of_bad()` declaration after rebase on\n> > master.\n> > * Fix `check_good_are_ancestors_of_bad()`, `bisect_next_all()`\n> > and `bisect_rev_setup()` calls after rebase on master.\n> >\n> > --\n> >\n> > [11/29] bisect: libify `bisect_next_all`\n> >\n> > State: Previously sent\n> >\n> > * Fix `show_diff_tree()` call after rebase on master.\n> >\n> > --\n> >\n> > [12/29] bisect--helper: reimplement `bisect_next` and `bisect_auto_next`\n> > shell functions in C\n> >\n> > * Fix `repo_init_revisions()` and `bisect_next_all()` calls after rebase\n> > on master.\n> > * Remove `goto` statement in `bisect_skipped_commits()`\n> >\n> > --\n> >\n> > [13/29] bisect--helper: finish porting `bisect_start()` to C\n> >\n> > * Change `return` statement instead of `die` in error handling.\n> > * Remove `goto` statements in `bisect_skipped_commits()`.\n> >\n> > --\n> >\n> > [21/29] bisect--helper: reimplement `bisect_replay` shell function in C\n> >\n> > * Add blank line in `get_next_word()`.\n> > * Remove `goto` statements in `bisect_replay()`.\n> >\n> > --\n> >\n> > [23/29] bisect--helper: use `res` instead of return in BISECT_RESET case\n> > option\n> >\n> > * New patch to split previous commit in two.\n> >\n> > --\n> >\n> > [26/29] bisect--helper: reimplement `bisect_skip` shell function in C\n> >\n> > State: Previously sent\n> >\n> > * Add blank line.\n> >\n> > --\n> >\n> > [28/29] bisect--helper: reimplement `bisect_visualize()`shell function\n> > in C\n> >\n> > New patch:\n> >\n> > * Reimplement the `bisect_visualize()` shell function in C.\n> > * Add `--bisect-visualize` subcommand.\n> > * Fix long code line.\n> >\n> > --\n> >\n> > [29/29] bisect--helper: reimplement `bisect_run` shell function in C\n> >\n> > New patch:\n> >\n> > * Reimplement the `bisect_run()` shell function in C.\n> > * Add `--bisect-run` subcommand.\n> > * Remove blank line.\n> >\n> > --\n> >\n> > Miriam Rubio (2):\n> > bisect--helper: convert `vocab_*` char pointers to char arrays\n> > bisect: use the standard 'if (!var)' way to check for 0\n> >\n> > Pranit Bauva (24):\n> > run-command: make `exists_in_PATH()` non-static\n> > bisect: libify `exit_if_skipped_commits` to `error_if_skipped*` and\n> > its dependents\n> > bisect: libify `bisect_checkout`\n> > bisect: libify `check_merge_bases` and its dependents\n> > bisect: libify `check_good_are_ancestors_of_bad` and its dependents\n> > bisect: libify `handle_bad_merge_base` and its dependents\n> > bisect: libify `bisect_next_all`\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_autostart` shell function in C\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: reimplement `bisect_log` shell function in C\n> > bisect--helper: reimplement `bisect_replay` shell function in C\n> > bisect--helper: retire `--bisect-write` subcommand\n> > bisect--helper: use `res` instead of return in BISECT_RESET case\n> > option\n> > bisect--helper: retire `--bisect-autostart` subcommand\n> > bisect--helper: retire `--bisect-auto-next` subcommand\n> > bisect--helper: reimplement `bisect_skip` shell function in C\n> > bisect--helper: retire `--check-and-set-terms` subcommand\n> > bisect--helper: reimplement `bisect_visualize()`shell function in C\n> >\n> > Tanushree Tumane (3):\n> > bisect--helper: change `retval` to `res`\n> > bisect--helper: introduce new `decide_next()` function\n> > bisect--helper: reimplement `bisect_run` shell function in C\n> >\n> > bisect.c | 146 +++++---\n> > builtin/bisect--helper.c | 776 +++++++++++++++++++++++++++++++++------\n> > git-bisect.sh | 279 +-------------\n> > run-command.c | 2 +-\n> > run-command.h | 11 +\n> > 5 files changed, 793 insertions(+), 421 deletions(-)\n> >\n> > --\n> > 2.21.1 (Apple Git-122.3)\n> >\n> >\n"},{"id":"390159","messageId":"CAN7CjDCmqvAkbeHnqLM6SUUyGWQu23hE=nbab6bYxZKZsXB1Qw@mail.gmail.com","threadId":"52666","inReplyTo":"nycvar.QRO.7.76.6.2001202308050.46@tvgsbejvaqbjf.bet","subject":"Re: [PATCH 08/29] bisect: libify `check_merge_bases` and its dependents","fromName":"Miriam R.","fromEmail":"mirucam@gmail.com","sentAt":"2020-01-21T09:59:53Z","receivedAt":"2020-01-21T10:00:06Z","isPatch":true,"sender":{"key":"mirucam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56339109?v=4"},"body":"Hi,\n\nEl lun., 20 ene. 2020 a las 23:09, Johannes Schindelin\n() escribió:\n>\n> Hi,\n>\n> On Mon, 20 Jan 2020, Miriam Rubio wrote:\n>\n> > From: Pranit Bauva \n> >\n> > Since we want to get rid of git-bisect.sh it would be necessary to\n> > convert those exit() calls to return statements so that errors can be\n> > reported.\n> >\n> > Emulate try catch in C by converting `exit()` to\n> > `return `. Follow POSIX conventions to return\n> > to indicate error.\n> >\n> > Turn `exit()` to `return` calls in `check_merge_bases()`.\n> >\n> > In `check_merge_bases()` there is an early success special case,\n> > so we have introduced special error code `-11` which indicates early\n> > success. This `-11` is converted back to `0` in `check_good_are_ancestors_of_bad()`.\n> >\n> > Handle the return value in dependent function `check_good_are_ancestors_of_bad()`.\n>\n> This is a lot of repeated text from earlier commit messages. It might make\n> sense to condense it a bit, and while at it, to remove sentences that\n> essentially repeat what the diff says.\n>\nOk. Noted!\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> > bisect.c | 21 ++++++++++++++++-----\n> > 1 file changed, 16 insertions(+), 5 deletions(-)\n> >\n> > diff --git a/bisect.c b/bisect.c\n> > index 385afaf875..367258b0dd 100644\n> > --- a/bisect.c\n> > +++ b/bisect.c\n> > @@ -806,13 +806,16 @@ static void handle_skipped_merge_base(const struct object_id *mb)\n> > * \"check_merge_bases\" checks that merge bases are not \"bad\" (or \"new\").\n> > *\n> > * - If one is \"bad\" (or \"new\"), it means the user assumed something wrong\n> > - * and we must exit with a non 0 error code.\n> > + * and we must return error with a non 0 error code.\n> > * - If one is \"good\" (or \"old\"), that's good, we have nothing to do.\n> > * - If one is \"skipped\", we can't know but we should warn.\n> > * - If we don't know, we should check it out and ask the user to test.\n> > + * - If a merge base must be tested, on success return -11 a special condition\n> > + * for early success, this will be converted back to 0 in check_good_are_ancestors_of_bad().\n> > */\n> > -static void check_merge_bases(int rev_nr, struct commit **rev, int no_checkout)\n> > +static int check_merge_bases(int rev_nr, struct commit **rev, int no_checkout)\n> > {\n> > + int res = 0;\n> > struct commit_list *result;\n> >\n> > result = get_merge_bases_many(rev[0], rev_nr - 1, rev + 1);\n> > @@ -827,11 +830,16 @@ static void check_merge_bases(int rev_nr, struct commit **rev, int no_checkout)\n> > handle_skipped_merge_base(mb);\n> > } else {\n> > printf(_(\"Bisecting: a merge base must be tested\\n\"));\n> > - exit(bisect_checkout(mb, no_checkout));\n> > + res = bisect_checkout(mb, no_checkout);\n> > + if (!res)\n> > + /* indicate early success */\n> > + res = -11;\n>\n> This is yet another good candidate for an `enum`.\n>\n> > + break;\n> > }\n> > }\n> >\n> > free_commit_list(result);\n> > + return res;\n> > }\n> >\n> > static int check_ancestors(struct repository *r, int rev_nr,\n> > @@ -865,7 +873,7 @@ static void check_good_are_ancestors_of_bad(struct repository *r,\n> > {\n> > char *filename = git_pathdup(\"BISECT_ANCESTORS_OK\");\n> > struct stat st;\n> > - int fd, rev_nr;\n> > + int fd, rev_nr, res = 0;\n> > struct commit **rev;\n> >\n> > if (!current_bad_oid)\n> > @@ -880,10 +888,13 @@ static void check_good_are_ancestors_of_bad(struct repository *r,\n> > goto done;\n> >\n> > /* Check if all good revs are ancestor of the bad rev. */\n> > +\n> > rev = get_bad_and_good_commits(r, &rev_nr);\n> > if (check_ancestors(r, rev_nr, rev, prefix))\n> > - check_merge_bases(rev_nr, rev, no_checkout);\n> > + res = check_merge_bases(rev_nr, rev, no_checkout);\n> > free(rev);\n> > + if(res)\n>\n> Please put a space between the `if` keyword and the `(` following it.\n\nOk.\n\n\n>\n> Thanks,\n> Johannes\n>\n> > + exit(res == -11 ? 0 : -res);\n> >\n> > /* Create file BISECT_ANCESTORS_OK. */\n> > fd = open(filename, O_CREAT | O_TRUNC | O_WRONLY, 0600);\n> > --\n> > 2.21.1 (Apple Git-122.3)\n> >\n> >\n"},{"id":"390160","messageId":"CAN7CjDDZw4e13bJzH3gpitb__=H5VocrEZRY6cBTrCpAJ1q13Q@mail.gmail.com","threadId":"52666","inReplyTo":"CAP8UFD1T3_1t0Qf3jP1GttNHvaLEWoaeAVeZid6AMH_MAKPjSw@mail.gmail.com","subject":"Re: [PATCH 06/29] bisect: libify `exit_if_skipped_commits` to `error_if_skipped*` and its dependents","fromName":"Miriam R.","fromEmail":"mirucam@gmail.com","sentAt":"2020-01-21T10:00:05Z","receivedAt":"2020-01-21T10:00:18Z","isPatch":true,"sender":{"key":"mirucam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56339109?v=4"},"body":"Hi,\n\nEl mar., 21 ene. 2020 a las 7:40, Christian Couder\n() escribió:\n>\n> Hi Dscho,\n>\n> On Mon, Jan 20, 2020 at 10:57 PM Johannes Schindelin\n> wrote:\n> >\n> > On Mon, 20 Jan 2020, Miriam Rubio wrote:\n>\n> > > printf(\"There are only 'skip'ped commits left to test.\\n\"\n> > > \"The first %s commit could be any of:\\n\", term_bad);\n> > > @@ -676,7 +676,13 @@ static void exit_if_skipped_commits(struct commit_list *tried,\n> > > if (bad)\n> > > printf(\"%s\\n\", oid_to_hex(bad));\n> > > printf(_(\"We cannot bisect more!\\n\"));\n> > > - exit(2);\n> > > +\n> > > + /*\n> > > + * We don't want to clean the bisection state\n> > > + * as we need to get back to where we started\n> > > + * by using `git bisect reset`.\n> > > + */\n> > > + return -2;\n> >\n> > This comment is a good indicator that the constant `-2` here is a \"magic\"\n> > number and it would most likely make sense to turn the return type from an\n> > `int` into an `enum` instead.\n>\n> Many functions use `return error(...)` and error codes from these\n> functions and from exit_if_skipped_commits() are going to get mixed.\n> So I am not sure that using an enum for only some of the error codes\n> will make things clearer.\n>\n> > > static int is_expected_rev(const struct object_id *oid)\n> > > @@ -949,7 +955,7 @@ int bisect_next_all(struct repository *r, const char *prefix, int no_checkout)\n> > > {\n> > > struct rev_info revs;\n> > > struct commit_list *tried;\n> > > - int reaches = 0, all = 0, nr, steps;\n> > > + int reaches = 0, all = 0, nr, steps, res;\n> > > struct object_id *bisect_rev;\n> > > char *steps_msg;\n> > >\n> > > @@ -972,8 +978,9 @@ int bisect_next_all(struct repository *r, const char *prefix, int no_checkout)\n> > > * We should exit here only if the \"bad\"\n> > > * commit is also a \"skip\" commit.\n> > > */\n> > > - exit_if_skipped_commits(tried, NULL);\n> > > -\n> > > + res = error_if_skipped_commits(tried, NULL);\n> > > + if (res)\n> > > + exit(-res);\n> >\n> > So we still `exit()` in `libgit.a`? I hoped for a more thorough\n> > libification.\n>\n> The exit() calls are removed in later patches.\n>\n> > Besides, the `if (res)` probably wants to be an `if (res < 0)`, right?\n>\n> Yeah, I agree.\n>\nNoted!\nThank you Johannes and Christian.\n\n> Thanks for your review,\n> Christian.\n"},{"id":"390161","messageId":"CAN7CjDAb4AxZQ-dU0GBibXhWCwxRDqbbkVaKuNfVJ+8LASr8fQ@mail.gmail.com","threadId":"52666","inReplyTo":"CAP8UFD0MO9wEc=ErEAFm4kSabLKUTp1a5VTPxgDS2yqt+rxXag@mail.gmail.com","subject":"Re: [PATCH 09/29] bisect: libify `check_good_are_ancestors_of_bad` and its dependents","fromName":"Miriam R.","fromEmail":"mirucam@gmail.com","sentAt":"2020-01-21T10:00:21Z","receivedAt":"2020-01-21T10:00:34Z","isPatch":true,"sender":{"key":"mirucam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56339109?v=4"},"body":"Hi,\n\nEl mar., 21 ene. 2020 a las 7:59, Christian Couder\n() escribió:\n>\n> On Mon, Jan 20, 2020 at 11:20 PM Johannes Schindelin\n> wrote:\n> >\n> > On Mon, 20 Jan 2020, Miriam Rubio wrote:\n>\n> > > @@ -876,8 +877,15 @@ static void check_good_are_ancestors_of_bad(struct repository *r,\n> > > int fd, rev_nr, res = 0;\n> > > struct commit **rev;\n> > >\n> > > - if (!current_bad_oid)\n> > > - die(_(\"a %s revision is needed\"), term_bad);\n> > > + /*\n> > > + * We don't want to clean the bisection state\n> > > + * as we need to get back to where we started\n> > > + * by using `git bisect reset`.\n> > > + */\n> > > + if (!current_bad_oid) {\n> > > + res = error(_(\"a %s revision is needed\"), term_bad);\n> > > + goto done;\n> > > + }\n> >\n> > Why not just return here? Ah, there is a `filename` that was allocated...\n> > it is too bad that we have a mailing-list based review, as the hunk\n> > context simply cannot be extended in a mail.\n> >\n> > Personally, I think it would be nicer to split the definition of\n> > `filename` from its declaration and move it _after_ this conditional code,\n> > so that we can `return` right away.\n>\n> Yeah, I agree.\nOk. Noted.\n>\n> > However, there is a more pressing issue than that: `die()` exits with exit\n> > code 128, so in keeping with the idea to hand down negative exit codes as\n> > return values, should we not assign `res = -128` here?\n>\n> I think it has been ok when converting git-bisect.sh to C to just\n> convert `die(...)` into `return error(...)`.\n>\n> > > /* Check if file BISECT_ANCESTORS_OK exists. */\n> > > if (!stat(filename, &st) && S_ISREG(st.st_mode))\n> > > @@ -893,18 +901,20 @@ static void check_good_are_ancestors_of_bad(struct repository *r,\n> > > if (check_ancestors(r, rev_nr, rev, prefix))\n> > > res = check_merge_bases(rev_nr, rev, no_checkout);\n> > > free(rev);\n> > > - if(res)\n> > > - exit(res == -11 ? 0 : -res);\n> > > -\n> > > - /* Create file BISECT_ANCESTORS_OK. */\n> > > - fd = open(filename, O_CREAT | O_TRUNC | O_WRONLY, 0600);\n> > > - if (fd < 0)\n> > > - warning_errno(_(\"could not create file '%s'\"),\n> > > - filename);\n> > > - else\n> > > - close(fd);\n> > > +\n> > > + if (!res)\n> > > + {\n> >\n> > We usually put the `{` on the same line as the `if` condition (like you\n> > did in the `if (!current_bad_oid)` line above.\nOk. I will change that.\n> >\n> > The rest looks reasonable. Thank you,\nGreat! Thank you for your review!\n\n>\n> Thank you for your review,\n> Christian.\n"},{"id":"390162","messageId":"CAN7CjDCXm9f9PV61uK_6aVrLSTGvxAiS_t607-JMLnDY5i9AxQ@mail.gmail.com","threadId":"52666","inReplyTo":"CAP8UFD0TU9FQQfk6SgDtOi+_eDni0ZJf+Bw-uN_LzAdeVh3HvQ@mail.gmail.com","subject":"Re: [PATCH 10/29] bisect: libify `handle_bad_merge_base` and its dependents","fromName":"Miriam R.","fromEmail":"mirucam@gmail.com","sentAt":"2020-01-21T10:00:29Z","receivedAt":"2020-01-21T10:00:42Z","isPatch":true,"sender":{"key":"mirucam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56339109?v=4"},"body":"Hi,\n\nEl mar., 21 ene. 2020 a las 8:05, Christian Couder\n() escribió:\n>\n> On Mon, Jan 20, 2020 at 11:23 PM Johannes Schindelin\n> wrote:\n>\n> > On Mon, 20 Jan 2020, Miriam Rubio wrote:\n>\n> > > @@ -777,14 +777,14 @@ static void handle_bad_merge_base(void)\n> > > \"between %s and [%s].\\n\"),\n> > > bad_hex, term_bad, term_good, bad_hex, good_hex);\n> > > }\n> > > - exit(3);\n> > > + return -3;\n> >\n> > ... which is: what does `3` stand for?\n>\n> Maybe the question should have been answered by adding a comment to\n> the previous patch that added the `exit(3)` statement.\n>\n> So yeah we could here add a separate patch that just adds such a comment.\n>\n> Or maybe we can add such a comment in this patch and say something\n> like \"while at it let's explain a bit the '3' error code\" in the\n> commit message.\n\nI like most your first option because we explain the code in the patch\nwhere it is added, but the second one is also ok for me :).\nThanks.\nBest,\nMiriam\n"},{"id":"390806","messageId":"nycvar.QRO.7.76.6.2001301605110.46@tvgsbejvaqbjf.bet","threadId":"52666","inReplyTo":"CAP8UFD0W=kBs1zne56OjPbFzeX7BtEazPh2MW1My3qAsByS=nA@mail.gmail.com","subject":"Re: [Outreachy][PATCH 00/29] Finish converting git bisect to C part 1","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-01-30T15:12:22Z","receivedAt":"2020-01-30T15:12:25Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Chris,\n\nOn Tue, 21 Jan 2020, Christian Couder wrote:\n\n> On Mon, Jan 20, 2020 at 10:43 PM Johannes Schindelin\n> wrote:\n> >\n> > On Mon, 20 Jan 2020, Miriam Rubio wrote:\n>\n> > > [1/29] bisect--helper: convert `vocab_*` char pointers to char arrays\n> > >\n> > > * New patch to convert `vocab_bad` and `vocab_good` char pointers\n> > > to char arrays\n> >\n> > 29 patches is _a lot_ to review. I would have preferred a series of\n> > smaller patch series.\n>\n> Yeah, it's possible to split it into smaller patch series. There are a\n> many similar patches in the series so it was easier to work on\n> everything together to make similar and consistent changes to all the\n> patches at once.\n>\n> > For example, the first three patches would have made for a fine \"some\n> > cleanups\" patch series, from my point of view.\n>\n> Yeah, but this might then be rejected by Junio as it would be only \"code\n> churn\".\n\nI don't think so. We have ample \"prior art\" where a patch series came with\na wonderful cover letter that explained why such a cleanup prepares for a\nlater patch series rather than being code churn.\n\nIn general, it appears to me that cleanups are much appreciated over here\nrather than discouraged.\n\nI also have to admit that I can justify _a lot better_ to set aside time\nfor reviewing a tiny patch series consisting of three or so pure cleanups\non one day, and then a now-smaller patch series to libify `bisect.c` on\nanother day. Much easier to justify than trying to find the time to read\nthrough one honking 29-strong patch series, where realistically I have to\nre-read all of the patches upon the next iteration because I filled my\nbrain with so many other things in the meantime.\n\nI think that it would be good mentor advice to pass along: not only the\ncommits are ideally so small as to be _trivial_ to review, also the patch\nseries should be structured in such a way. The scarce resource is the\nreviewer time, after all.\n\n> > Also, as the mail's subject says \"part 1\", it would be good to have an\n> > overview how this part fits into the overall story of converting `git\n> > bisect` into a built-in.\n>\n> We don't know how the rest will be split yet. Hopefully there will be\n> only one other smaller patch series after this one.\n\nThat is a wonderful contradiction: first you say that you cannot know, and\nthen you state that there will be only one patch series after this ;-)\n\nSeriously again, nobody will hold you to a statement like \"this is the\nfirst of three patch series\" when you later explain \"originally, I\nintended this patch series to be the final one, but decided to split it\neven further, to make it easier on reviewers\" or some such.\n\nBut done is done, v2 was a nice read.\n\nCiao,\nDscho\n"},{"id":"390807","messageId":"nycvar.QRO.7.76.6.2001301612460.46@tvgsbejvaqbjf.bet","threadId":"52666","inReplyTo":"CAN7CjDC-UrD0sTE=jjNm8=tYsS5-X8p_u_8LYuvRix=_QvnxfA@mail.gmail.com","subject":"Re: [Outreachy][PATCH 00/29] Finish converting git bisect to C part 1","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-01-30T15:13:20Z","receivedAt":"2020-01-30T15:13:23Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Miriam,\n\nOn Tue, 21 Jan 2020, Miriam R. wrote:\n\n> El lun., 20 ene. 2020 a las 22:41, Johannes Schindelin\n> () escribió:\n> >\n> > Finally, it would be nice to have a link to a public repository with the\n> > branch from which these mails were generated.\n> >\n>\n> This is the link of the current branch:\n> https://gitlab.com/mirucam/git/commits/git-bisect-work2.8.2\n> I will add the public repository link on next cover-letters.\n> Thank you for the suggestion.\n\nExcellent, thank you very much!\nDscho\n"},{"id":"390809","messageId":"nycvar.QRO.7.76.6.2001301614410.46@tvgsbejvaqbjf.bet","threadId":"52666","inReplyTo":"CAP8UFD3QP3QXe32cdK+q=P+v3dK1Bfws5KgNnm-JfDnPkpBZfw@mail.gmail.com","subject":"Re: [PATCH 11/29] bisect: libify `bisect_next_all`","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-01-30T15:18:23Z","receivedAt":"2020-01-30T15:18:28Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Chris,\n\nOn Tue, 21 Jan 2020, Christian Couder wrote:\n\n> On Mon, Jan 20, 2020 at 11:29 PM Johannes Schindelin\n> wrote:\n>\n> > On Mon, 20 Jan 2020, Miriam Rubio wrote:\n>\n> > > Since we want to get rid of git-bisect.sh it would be necessary to\n> > > convert those exit() calls to return statements so that errors can be\n> > > reported.\n> > >\n> > > Emulate try catch in C by converting `exit()` to\n> > > `return `. Follow POSIX conventions to return\n> > > to indicate error.\n> > >\n> > > Turn `exit()` to `return` calls in `bisect_next_all()`.\n> > >\n> > > All the functions calling `bisect_next_all()` are already able to\n> > > handle return values from it.\n> >\n> > So this patch brings more magic values that really would like to become\n> > `enum` values. At this point, we're talking about `bisect_next_all()`\n> > which is _not_ a file-local (`static`) function. Therefore, we now really\n> > wade into API territory where an `enum` is no longer just a nice thing,\n> > but a necessary thing: `bisect_next_all()` is a function that can be\n> > called from other source files.\n>\n> I agree that return values could be better documented. About enum\n> though, I am perhaps wrong but I don't think that we use them often\n> for error codes.\n\nIt does not matter how often we actually use them, it matters more whether\nwe _want_ to use them. And in the recent years, we have definitely made\nmore use of enums than before, to allow the compiler to catch mistakes\nearlier. We even started to convert existing constants to enums, for\nexample.\n\nSo I don't think that it is sound advice here to point to the code base.\n\nIf you _must_ value the existing practice over a clearly communicated\nreview, then please look no further than at the declaration of\n`safe_create_leading_directories()`.\n\n> Do you have examples in the code base where they are used for such a\n> purpose along with regular `error(...)` calls?\n\nIt's all in the code. You don't need me to read it aloud for you.\n\n:-)\n\nCiao,\nDscho\n"},{"id":"390844","messageId":"xmqqr1zgbqdn.fsf@gitster-ct.c.googlers.com","threadId":"52666","inReplyTo":"nycvar.QRO.7.76.6.2001301605110.46@tvgsbejvaqbjf.bet","subject":"Re: [Outreachy][PATCH 00/29] Finish converting git bisect to C part 1","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-01-30T21:12:20Z","receivedAt":"2020-01-30T21:12:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> In general, it appears to me that cleanups are much appreciated over here\n> rather than discouraged.\n\nYup.\n"},{"id":"390863","messageId":"nycvar.QRO.7.76.6.2001301619340.46@tvgsbejvaqbjf.bet","threadId":"52666","inReplyTo":"20200120143800.900-13-mirucam@gmail.com","subject":"Re: [PATCH 12/29] bisect--helper: reimplement `bisect_next` and `bisect_auto_next` shell functions in C","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-01-30T22:47:09Z","receivedAt":"2020-01-30T22:47:18Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Miriam,\n\nI started looking at this patch, and will just send the comments, but\nplease note that I would not mind at all leaving the review for later,\nwhen the libifying patches that you kept in v2 (and probably will send out\na v3 for) made it into `next` and you send the remainder as a new patch\nseries.\n\nOn Mon, 20 Jan 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> Using `--bisect-next` and `--bisect-auto-start` 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-start`\n> subcommand will be retired and will be called by some other methods.\n\nThis still sounds clear enough.\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 | 10 +++\n> builtin/bisect--helper.c | 174 ++++++++++++++++++++++++++++++++++++++-\n> git-bisect.sh | 47 ++---------\n> 3 files changed, 188 insertions(+), 43 deletions(-)\n>\n> diff --git a/bisect.c b/bisect.c\n> index 33f2829c19..1c13da8a28 100644\n> --- a/bisect.c\n> +++ b/bisect.c\n> @@ -635,6 +635,12 @@ static void bisect_rev_setup(struct repository *r, struct rev_info *revs,\n> \tstruct argv_array rev_argv = ARGV_ARRAY_INIT;\n> \tint i;\n>\n> +\t/*\n> +\t * Since the code is slowly being converted to C, there might be\n> +\t * instances where the revisions were initialized before. Thus\n> +\t * we first need to reset it.\n> +\t */\n\nThis comment sounds good right now, but is prone to get stale rather\nquickly.\n\nBut that's actually remedied rather easily: if the comment is reworded\nonly slightly, to avoid talking about the conversion process, and to\nmention instead that `revs` could have been used before, then we're\ngolden.\n\n> +\treset_revision_walk();\n> \trepo_init_revisions(r, revs, prefix);\n> \trevs->abbrev = 0;\n> \trevs->commit_format = CMIT_FMT_UNSPECIFIED;\n> @@ -971,6 +977,10 @@ void read_bisect_terms(const char **read_bad, const char **read_good)\n> * finished successfully.\n> * In this case the calling function or command should not turn a -10\n> * return code into an error or a non zero exit code.\n\nI'd like to have an empty line here (well, a line that only contains an\nindented `*`).\n\n> + * This returned -10 is checked in bisect_helper::bisect_next() and\n> + * eventually transformed to 0 at the end of\n> + * bisect_helper::cmd_bisect__helper().\n\nThis says _what_ it does. But not why. I would contend that it is much\nmore important to know what role the `-10` serves than explaining where\nthe role is acted out.\n\n> + *\n> * If no_checkout is non-zero, the bisection process does not\n> * checkout the trial commit but instead simply updates BISECT_HEAD.\n> */\n> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\n> index 5e0f759d50..29bbc1573b 100644\n> --- a/builtin/bisect--helper.c\n> +++ b/builtin/bisect--helper.c\n> @@ -8,6 +8,7 @@\n> #include \"run-command.h\"\n> #include \"prompt.h\"\n> #include \"quote.h\"\n> +#include \"revision.h\"\n>\n> static GIT_PATH_FUNC(git_path_bisect_terms, \"BISECT_TERMS\")\n> static GIT_PATH_FUNC(git_path_bisect_expected_rev, \"BISECT_EXPECTED_REV\")\n> @@ -29,6 +30,8 @@ static const char * const git_bisect_helper_usage[] = {\n> \tN_(\"git bisect--helper --bisect-terms [--term-good | --term-old | --term-bad | --term-new]\"),\n> \tN_(\"git bisect--helper --bisect-start [--term-{old,good}= --term-{new,bad}=]\"\n> \t\t\t\t\t \"[--no-checkout] [ [...]] [--] [...]\"),\n> +\tN_(\"git bisect--helper --bisect-next\"),\n> +\tN_(\"git bisect--helper --bisect-auto-next\"),\n> \tNULL\n> };\n>\n> @@ -421,6 +424,157 @@ static int bisect_append_log_quoted(const char **argv)\n> \treturn res;\n> }\n>\n> +static int register_good_ref(const char *refname,\n> +\t\t\t const struct object_id *oid, int flags,\n> +\t\t\t void *cb_data)\n> +{\n> +\tstruct string_list *good_refs = cb_data;\n> +\tstring_list_append(good_refs, oid_to_hex(oid));\n> +\treturn 0;\n> +}\n> +\n> +static void prepare_rev_argv(struct bisect_terms *terms, struct argv_array *rev_argv)\n> +{\n> +\tstruct string_list good_revs = STRING_LIST_INIT_DUP;\n> +\tchar *term_good = xstrfmt(\"%s-*\", terms->term_good);\n> +\n> +\tfor_each_glob_ref_in(register_good_ref, term_good,\n> +\t\t\t \"refs/bisect/\", &good_revs);\n> +\n> +\targv_array_pushl(rev_argv, \"skipped_commits\", \"refs/bisect/bad\", \"--not\", NULL);\n> +\tfor (int i = 0; i < good_revs.nr; i++)\n> +\t\targv_array_push(rev_argv, good_revs.items[i].string);\n> +\n> +\tstring_list_clear(&good_revs, 0);\n> +\tfree(term_good);\n> +}\n\nMaybe we should fold that into `prepare_revs()`? We could then render the\narguments directly into `revs` (via `add_pending_object()`, after setting\nobj->flags |= UNINTERESTING`) rather than formatting them into a string\nlist, then deep-copy them into an `argv_array` only to parse them back\ninto OIDs that we already had in the first place.\n\n> +\n> +static int prepare_revs(struct bisect_terms *terms, struct rev_info *revs)\n> +{\n> +\tint res = 0;\n> +\tstruct argv_array rev_argv = ARGV_ARRAY_INIT;\n> +\n> +\tprepare_rev_argv(terms, &rev_argv);\n> +\n> +\t/*\n> +\t * It is important to reset the flags used by revision walks\n> +\t * as the previous call to bisect_next_all() in turn\n> +\t * setups a revision walk.\n> +\t */\n> +\treset_revision_walk();\n> +\tinit_revisions(revs, NULL);\n> +\trev_argv.argc = setup_revisions(rev_argv.argc, rev_argv.argv, revs, NULL);\n> +\tif (prepare_revision_walk(revs))\n> +\t\tres = error(_(\"revision walk setup failed\\n\"));\n> +\n> +\targv_array_clear(&rev_argv);\n> +\treturn res;\n> +}\n> +\n> +static int process_skipped_commits(FILE *fp, struct bisect_terms *terms, struct rev_info *revs)\n> +{\n> +\tstruct commit *commit;\n> +\tstruct pretty_print_context pp = {0};\n> +\n> +\tif (fprintf(fp, \"# only skipped commits left to test\\n\") < 1)\n> +\t\treturn -1;\n> +\n> +\twhile ((commit = get_revision(revs)) != NULL) {\n> +\t\tstruct strbuf commit_name = STRBUF_INIT;\n> +\t\tformat_commit_message(commit, \"%s\",\n> +\t\t\t\t &commit_name, &pp);\n> +\t\tfprintf(fp, \"# possible first %s commit: [%s] %s\\n\",\n> +\t\t\tterms->term_bad, oid_to_hex(&commit->object.oid),\n> +\t\t\tcommit_name.buf);\n> +\t\tstrbuf_release(&commit_name);\n> +\t}\n\nIn the interest of allowing further revision walks, we will probably need\nto re-set the flags via `clear_commit_marks()`, just like\n`check_ancestors()` does.\n\n> +\n> +\treturn 0;\n> +}\n> +\n> +static int bisect_skipped_commits(struct bisect_terms *terms)\n> +{\n> +\tint res = 0;\n> +\tFILE *fp = NULL;\n> +\tstruct rev_info revs;\n> +\n> +\tfp = fopen(git_path_bisect_log(), \"a\");\n> +\tif (!fp)\n> +\t\treturn error_errno(_(\"could not open '%s' for appending\"),\n> +\t\t\t\t git_path_bisect_log());\n> +\n> +\tres = prepare_revs(terms, &revs);\n> +\n> +\tif (!res)\n> +\t\tres = process_skipped_commits(fp, terms, &revs);\n> +\n> +\tfclose(fp);\n> +\treturn res;\n> +}\n\nThis is again a very short wrapper around another function, so it will\nprobably make sense to merge the two, otherwise the boilerplate might very\nwell outweigh the actual code doing actual work.\n\n> +\n> +static int bisect_successful(struct bisect_terms *terms)\n> +{\n> +\tFILE *fp = NULL;\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\",\n> +\t\t\t\tterms->term_bad);\n> +\tint res = 0;\n> +\n> +\tread_ref(bad_ref, &oid);\n> +\tprintf(\"%s\\n\", bad_ref);\n> +\tcommit = lookup_commit_reference(the_repository, &oid);\n> +\tformat_commit_message(commit, \"%s\", &commit_name, &pp);\n> +\n\nThere is a trailing tab here. Maybe it would make sense to check the\npatches via `git log --check`?\n\n> +\tfp = fopen(git_path_bisect_log(), \"a\");\n> +\tif (fp) {\n> +\t\tif (fprintf(fp, \"# first %s commit: [%s] %s\\n\",\n> +\t\t\t terms->term_bad, oid_to_hex(&oid),\n> +\t\t\t commit_name.buf) < 1)\n> +\t\t\tres = -1;\n\nThis would probably do with an error message, i.e. `res =\nerror_errno(...);`\n\n> +\t\tfclose(fp);\n> +\t} else {\n> +\t\tres = error_errno(_(\"could not open '%s' for \"\n> +\t\t\t\t \"appending\"),\n> +\t\t\t\t git_path_bisect_log());\n> +\t}\n\nThis pattern of opening a file, writing something into it, and then return\nsuccess, otherwise failure, seems like a repeated pattern. In other words,\nit would be a good candidate for factoring out into its own function.\n\n> +\tstrbuf_release(&commit_name);\n> +\tfree(bad_ref);\n> +\treturn res;\n> +}\n> +\n> +static int bisect_next(struct bisect_terms *terms, const char *prefix)\n> +{\n> +\tint res, no_checkout;\n> +\n> +\tif (bisect_next_check(terms, terms->term_good))\n> +\t\treturn -1;\n> +\n> +\tno_checkout = !is_empty_or_missing_file(git_path_bisect_head());\n> +\n> +\t/* Perform all bisection computation, display and checkout */\n> +\tres = bisect_next_all(the_repository, prefix, no_checkout);\n> +\n> +\tif (res == -10) {\n> +\t\tres = bisect_successful(terms);\n> +\t\treturn res ? res : -11;\n> +\t} else if (res == -2) {\n> +\t\tres = bisect_skipped_commits(terms);\n> +\t\treturn res ? res : -2;\n> +\t}\n\nI know exactly what I'll think if I see those constants six months from\nnow, when I forgot most of the details of our conversation over here. A\n-10 means.. wait, what?\n\nSeriously, it is quite bad to keep those constants unexplained.\n\nIn contrast, look at this here code:\n\n\tenum scld_error {\n\t\tSCLD_OK = 0,\n\t\tSCLD_FAILED = -1,\n\t\tSCLD_PERMS = -2,\n\t\tSCLD_EXISTS = -3,\n\t\tSCLD_VANISHED = -4\n\t};\n\tenum scld_error safe_create_leading_directories(char *path);\n\nWhat do you think? Will any reader stumble over this and say \"what the\nheck is going on? What are these return values even _supposed_ to mean?\"?\n\nEven better, it seems as if modern debuggers can figure out that a value\n-4 returned from `safe_create_leading_directories()` actually mean\n`SCLD_VANISHED` and display that to the user.\n\nSo armed with this example, you could of course go back to your mentor and\nask for permission to change the bisect code accordingly.\n\nYou could also just decide on your own that this is what you want to do\nbecause it is so much more elegant, anyway.\n\n> +\treturn res;\n> +}\n> +\n> +static int bisect_auto_next(struct bisect_terms *terms, const char *prefix)\n> +{\n> +\tif (!bisect_next_check(terms, NULL))\n> +\t\treturn bisect_next(terms, prefix);\n> +\n> +\treturn 0;\n\nA common pattern in Git's source code is to present the early return\nfirst, i.e.\n\n\tif (bisect_next_check(terms, NULL))\n\t\treturn 0;\n\n\treturn bisect_next(terms, prefix);\n\nI do find it easier to read that way, too.\n\n> +\n> static int bisect_start(struct bisect_terms *terms, int no_checkout,\n> \t\t\tconst char **argv, int argc)\n> {\n> @@ -625,7 +779,9 @@ 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_NEXT,\n> +\t\tBISECT_AUTO_NEXT,\n> \t} cmdmode = 0;\n> \tint no_checkout = 0, res = 0, nolog = 0;\n> \tstruct option options[] = {\n> @@ -649,6 +805,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_BOOL(0, \"no-checkout\", &no_checkout,\n> \t\t\t N_(\"update BISECT_HEAD instead of checking out the current commit\")),\n> \t\tOPT_BOOL(0, \"no-log\", &nolog,\n> @@ -710,6 +870,18 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n> \t\tset_terms(&terms, \"bad\", \"good\");\n> \t\tres = bisect_start(&terms, no_checkout, argv, argc);\n> \t\tbreak;\n> +\tcase BISECT_NEXT:\n> +\t\tif (argc)\n> +\t\t\treturn error(_(\"--bisect-next requires 0 arguments\"));\n> +\t\tget_terms(&terms);\n> +\t\tres = bisect_next(&terms, prefix);\n> +\t\tbreak;\n> +\tcase BISECT_AUTO_NEXT:\n> +\t\tif (argc)\n> +\t\t\treturn error(_(\"--bisect-auto-next requires 0 arguments\"));\n> +\t\tget_terms(&terms);\n> +\t\tres = bisect_auto_next(&terms, prefix);\n> +\t\tbreak;\n> \tdefault:\n> \t\treturn error(\"BUG: unknown subcommand '%d'\", cmdmode);\n> \t}\n> diff --git a/git-bisect.sh b/git-bisect.sh\n> index efee12b8b1..7531b74708 100755\n> --- a/git-bisect.sh\n> +++ b/git-bisect.sh\n> @@ -87,7 +87,7 @@ bisect_start() {\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> @@ -140,45 +140,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> -\tbisect_autostart\n> -\tgit bisect--helper --bisect-next-check $TERM_GOOD $TERM_BAD $TERM_GOOD|| exit\n> -\n> -\t# Perform all bisection computation, display and checkout\n> -\tgit bisect--helper --next-all $(test -f \"$GIT_DIR/BISECT_HEAD\" && echo --no-checkout)\n> -\tres=$?\n> -\n> -\t# Check if we should exit because bisection is finished\n> -\tif test $res -eq 10\n> -\tthen\n> -\t\tbad_rev=$(git show-ref --hash --verify refs/bisect/$TERM_BAD)\n> -\t\tbad_commit=$(git show-branch $bad_rev)\n> -\t\techo \"# first $TERM_BAD commit: $bad_commit\" >>\"$GIT_DIR/BISECT_LOG\"\n> -\t\texit 0\n> -\telif test $res -eq 2\n> -\tthen\n> -\t\techo \"# only skipped commits left to test\" >>\"$GIT_DIR/BISECT_LOG\"\n> -\t\tgood_revs=$(git for-each-ref --format=\"%(objectname)\" \"refs/bisect/$TERM_GOOD-*\")\n> -\t\tfor skipped in $(git rev-list refs/bisect/$TERM_BAD --not $good_revs)\n> -\t\tdo\n> -\t\t\tskipped_commit=$(git show-branch $skipped)\n> -\t\t\techo \"# possible first $TERM_BAD commit: $skipped_commit\" >>\"$GIT_DIR/BISECT_LOG\"\n> -\t\tdone\n> -\t\texit $res\n> -\tfi\n> -\n> -\t# Check for an error in the bisection process\n> -\ttest $res -ne 0 && exit $res\n> -\n> -\treturn 0\n> +\tgit bisect--helper --bisect-auto-next\n\nBeautiful.\n\n> }\n>\n> bisect_visualize() {\n> @@ -232,7 +194,7 @@ bisect_replay () {\n> \t\t\tdie \"$(gettext \"?? what are you talking about?\")\" ;;\n> \t\tesac\n> \tdone <\"$file\"\n> -\tbisect_auto_next\n> +\tgit bisect--helper --bisect-auto-next\n> }\n>\n> bisect_run () {\n> @@ -329,7 +291,8 @@ 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\tget_terms\n\nI vaguely remember that we talked about this, or at least about a similar\nscenario. It needs to be explained in the commit message why we need to\ncall `get_terms` here when previously, we did not.\n\nOf course, after thinking about this and looking around for a couple of\nminutes, I know why. My point is that I, or for that matter, any reader of\nthis commit, should not need to repeat that analysis.\n\nOther than that, the patch looks good.\n\nAs I said, I will stop reviewing the remainder of this patch series, as it\nhas been removed from v2 and will probably be presented as a follow-up\npatch series soon.\n\nThanks,\nDscho\n\n> +\t\tgit bisect--helper --bisect-next \"$@\" || exit ;;\n> \tvisualize|view)\n> \t\tbisect_visualize \"$@\" ;;\n> \treset)\n> --\n> 2.21.1 (Apple Git-122.3)\n>\n>\n"},{"id":"390898","messageId":"CAN7CjDCX6ohY4HHGe2EOFWf-U8wpM=xxRsPG8N7z4RjiQT0Qzg@mail.gmail.com","threadId":"52666","inReplyTo":"nycvar.QRO.7.76.6.2001301619340.46@tvgsbejvaqbjf.bet","subject":"Re: [PATCH 12/29] bisect--helper: reimplement `bisect_next` and `bisect_auto_next` shell functions in C","fromName":"Miriam R.","fromEmail":"mirucam@gmail.com","sentAt":"2020-01-31T10:53:30Z","receivedAt":"2020-01-31T10:53:45Z","isPatch":true,"sender":{"key":"mirucam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56339109?v=4"},"body":"Hi,\n\n\nEl jue., 30 ene. 2020 a las 23:47, Johannes Schindelin\n() escribió:\n>\n> Hi Miriam,\n>\n> I started looking at this patch, and will just send the comments, but\n> please note that I would not mind at all leaving the review for later,\n> when the libifying patches that you kept in v2 (and probably will send out\n> a v3 for) made it into `next` and you send the remainder as a new patch\n> series.\n\nYes, sure. Thank you, Johannes.\n\n>\n> On Mon, 20 Jan 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> > Using `--bisect-next` and `--bisect-auto-start` 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-start`\n> > subcommand will be retired and will be called by some other methods.\n>\n> This still sounds clear enough.\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 | 10 +++\n> > builtin/bisect--helper.c | 174 ++++++++++++++++++++++++++++++++++++++-\n> > git-bisect.sh | 47 ++---------\n> > 3 files changed, 188 insertions(+), 43 deletions(-)\n> >\n> > diff --git a/bisect.c b/bisect.c\n> > index 33f2829c19..1c13da8a28 100644\n> > --- a/bisect.c\n> > +++ b/bisect.c\n> > @@ -635,6 +635,12 @@ static void bisect_rev_setup(struct repository *r, struct rev_info *revs,\n> > struct argv_array rev_argv = ARGV_ARRAY_INIT;\n> > int i;\n> >\n> > + /*\n> > + * Since the code is slowly being converted to C, there might be\n> > + * instances where the revisions were initialized before. Thus\n> > + * we first need to reset it.\n> > + */\n>\n> This comment sounds good right now, but is prone to get stale rather\n> quickly.\n>\n> But that's actually remedied rather easily: if the comment is reworded\n> only slightly, to avoid talking about the conversion process, and to\n> mention instead that `revs` could have been used before, then we're\n> golden.\nOk\n>\n> > + reset_revision_walk();\n> > repo_init_revisions(r, revs, prefix);\n> > revs->abbrev = 0;\n> > revs->commit_format = CMIT_FMT_UNSPECIFIED;\n> > @@ -971,6 +977,10 @@ void read_bisect_terms(const char **read_bad, const char **read_good)\n> > * finished successfully.\n> > * In this case the calling function or command should not turn a -10\n> > * return code into an error or a non zero exit code.\n>\n> I'd like to have an empty line here (well, a line that only contains an\n> indented `*`).\nNoted.\n>\n> > + * This returned -10 is checked in bisect_helper::bisect_next() and\n> > + * eventually transformed to 0 at the end of\n> > + * bisect_helper::cmd_bisect__helper().\n>\n> This says _what_ it does. But not why. I would contend that it is much\n> more important to know what role the `-10` serves than explaining where\n> the role is acted out.\nAha.\n>\n> > + *\n> > * If no_checkout is non-zero, the bisection process does not\n> > * checkout the trial commit but instead simply updates BISECT_HEAD.\n> > */\n> > diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\n> > index 5e0f759d50..29bbc1573b 100644\n> > --- a/builtin/bisect--helper.c\n> > +++ b/builtin/bisect--helper.c\n> > @@ -8,6 +8,7 @@\n> > #include \"run-command.h\"\n> > #include \"prompt.h\"\n> > #include \"quote.h\"\n> > +#include \"revision.h\"\n> >\n> > static GIT_PATH_FUNC(git_path_bisect_terms, \"BISECT_TERMS\")\n> > static GIT_PATH_FUNC(git_path_bisect_expected_rev, \"BISECT_EXPECTED_REV\")\n> > @@ -29,6 +30,8 @@ static const char * const git_bisect_helper_usage[] = {\n> > N_(\"git bisect--helper --bisect-terms [--term-good | --term-old | --term-bad | --term-new]\"),\n> > N_(\"git bisect--helper --bisect-start [--term-{old,good}= --term-{new,bad}=]\"\n> > \"[--no-checkout] [ [...]] [--] [...]\"),\n> > + N_(\"git bisect--helper --bisect-next\"),\n> > + N_(\"git bisect--helper --bisect-auto-next\"),\n> > NULL\n> > };\n> >\n> > @@ -421,6 +424,157 @@ static int bisect_append_log_quoted(const char **argv)\n> > return res;\n> > }\n> >\n> > +static int register_good_ref(const char *refname,\n> > + const struct object_id *oid, int flags,\n> > + void *cb_data)\n> > +{\n> > + struct string_list *good_refs = cb_data;\n> > + string_list_append(good_refs, oid_to_hex(oid));\n> > + return 0;\n> > +}\n> > +\n> > +static void prepare_rev_argv(struct bisect_terms *terms, struct argv_array *rev_argv)\n> > +{\n> > + struct string_list good_revs = STRING_LIST_INIT_DUP;\n> > + char *term_good = xstrfmt(\"%s-*\", terms->term_good);\n> > +\n> > + for_each_glob_ref_in(register_good_ref, term_good,\n> > + \"refs/bisect/\", &good_revs);\n> > +\n> > + argv_array_pushl(rev_argv, \"skipped_commits\", \"refs/bisect/bad\", \"--not\", NULL);\n> > + for (int i = 0; i < good_revs.nr; i++)\n> > + argv_array_push(rev_argv, good_revs.items[i].string);\n> > +\n> > + string_list_clear(&good_revs, 0);\n> > + free(term_good);\n> > +}\n>\n> Maybe we should fold that into `prepare_revs()`? We could then render the\n> arguments directly into `revs` (via `add_pending_object()`, after setting\n> obj->flags |= UNINTERESTING`) rather than formatting them into a string\n> list, then deep-copy them into an `argv_array` only to parse them back\n> into OIDs that we already had in the first place.\n>\n> > +\n> > +static int prepare_revs(struct bisect_terms *terms, struct rev_info *revs)\n> > +{\n> > + int res = 0;\n> > + struct argv_array rev_argv = ARGV_ARRAY_INIT;\n> > +\n> > + prepare_rev_argv(terms, &rev_argv);\n> > +\n> > + /*\n> > + * It is important to reset the flags used by revision walks\n> > + * as the previous call to bisect_next_all() in turn\n> > + * setups a revision walk.\n> > + */\n> > + reset_revision_walk();\n> > + init_revisions(revs, NULL);\n> > + rev_argv.argc = setup_revisions(rev_argv.argc, rev_argv.argv, revs, NULL);\n> > + if (prepare_revision_walk(revs))\n> > + res = error(_(\"revision walk setup failed\\n\"));\n> > +\n> > + argv_array_clear(&rev_argv);\n> > + return res;\n> > +}\n> > +\n> > +static int process_skipped_commits(FILE *fp, struct bisect_terms *terms, struct rev_info *revs)\n> > +{\n> > + struct commit *commit;\n> > + struct pretty_print_context pp = {0};\n> > +\n> > + if (fprintf(fp, \"# only skipped commits left to test\\n\") < 1)\n> > + return -1;\n> > +\n> > + while ((commit = get_revision(revs)) != NULL) {\n> > + struct strbuf commit_name = STRBUF_INIT;\n> > + format_commit_message(commit, \"%s\",\n> > + &commit_name, &pp);\n> > + fprintf(fp, \"# possible first %s commit: [%s] %s\\n\",\n> > + terms->term_bad, oid_to_hex(&commit->object.oid),\n> > + commit_name.buf);\n> > + strbuf_release(&commit_name);\n> > + }\n>\n> In the interest of allowing further revision walks, we will probably need\n> to re-set the flags via `clear_commit_marks()`, just like\n> `check_ancestors()` does.\n>\n> > +\n> > + return 0;\n> > +}\n> > +\n> > +static int bisect_skipped_commits(struct bisect_terms *terms)\n> > +{\n> > + int res = 0;\n> > + FILE *fp = NULL;\n> > + struct rev_info revs;\n> > +\n> > + fp = fopen(git_path_bisect_log(), \"a\");\n> > + if (!fp)\n> > + return error_errno(_(\"could not open '%s' for appending\"),\n> > + git_path_bisect_log());\n> > +\n> > + res = prepare_revs(terms, &revs);\n> > +\n> > + if (!res)\n> > + res = process_skipped_commits(fp, terms, &revs);\n> > +\n> > + fclose(fp);\n> > + return res;\n> > +}\n>\n> This is again a very short wrapper around another function, so it will\n> probably make sense to merge the two, otherwise the boilerplate might very\n> well outweigh the actual code doing actual work.\n>\n> > +\n> > +static int bisect_successful(struct bisect_terms *terms)\n> > +{\n> > + FILE *fp = NULL;\n> > + struct object_id oid;\n> > + struct commit *commit;\n> > + struct pretty_print_context pp = {0};\n> > + struct strbuf commit_name = STRBUF_INIT;\n> > + char *bad_ref = xstrfmt(\"refs/bisect/%s\",\n> > + terms->term_bad);\n> > + int res = 0;\n> > +\n> > + read_ref(bad_ref, &oid);\n> > + printf(\"%s\\n\", bad_ref);\n> > + commit = lookup_commit_reference(the_repository, &oid);\n> > + format_commit_message(commit, \"%s\", &commit_name, &pp);\n> > +\n>\n> There is a trailing tab here. Maybe it would make sense to check the\n> patches via `git log --check`?\nThese extra trailing tabs are removed in my working branch :)\nThis is my last branch:\nhttps://gitlab.com/mirucam/git/commits/git-bisect-work3.3.1.\n>\n> > + fp = fopen(git_path_bisect_log(), \"a\");\n> > + if (fp) {\n> > + if (fprintf(fp, \"# first %s commit: [%s] %s\\n\",\n> > + terms->term_bad, oid_to_hex(&oid),\n> > + commit_name.buf) < 1)\n> > + res = -1;\n>\n> This would probably do with an error message, i.e. `res =\n> error_errno(...);`\n>\n> > + fclose(fp);\n> > + } else {\n> > + res = error_errno(_(\"could not open '%s' for \"\n> > + \"appending\"),\n> > + git_path_bisect_log());\n> > + }\n>\n> This pattern of opening a file, writing something into it, and then return\n> success, otherwise failure, seems like a repeated pattern. In other words,\n> it would be a good candidate for factoring out into its own function.\n>\n> > + strbuf_release(&commit_name);\n> > + free(bad_ref);\n> > + return res;\n> > +}\n> > +\n> > +static int bisect_next(struct bisect_terms *terms, const char *prefix)\n> > +{\n> > + int res, no_checkout;\n> > +\n> > + if (bisect_next_check(terms, terms->term_good))\n> > + return -1;\n> > +\n> > + no_checkout = !is_empty_or_missing_file(git_path_bisect_head());\n> > +\n> > + /* Perform all bisection computation, display and checkout */\n> > + res = bisect_next_all(the_repository, prefix, no_checkout);\n> > +\n> > + if (res == -10) {\n> > + res = bisect_successful(terms);\n> > + return res ? res : -11;\n> > + } else if (res == -2) {\n> > + res = bisect_skipped_commits(terms);\n> > + return res ? res : -2;\n> > + }\n>\n> I know exactly what I'll think if I see those constants six months from\n> now, when I forgot most of the details of our conversation over here. A\n> -10 means.. wait, what?\n>\n> Seriously, it is quite bad to keep those constants unexplained.\n>\n> In contrast, look at this here code:\n>\n> enum scld_error {\n> SCLD_OK = 0,\n> SCLD_FAILED = -1,\n> SCLD_PERMS = -2,\n> SCLD_EXISTS = -3,\n> SCLD_VANISHED = -4\n> };\n> enum scld_error safe_create_leading_directories(char *path);\n>\n> What do you think? Will any reader stumble over this and say \"what the\n> heck is going on? What are these return values even _supposed_ to mean?\"?\n>\n> Even better, it seems as if modern debuggers can figure out that a value\n> -4 returned from `safe_create_leading_directories()` actually mean\n> `SCLD_VANISHED` and display that to the user.\n>\n> So armed with this example, you could of course go back to your mentor and\n> ask for permission to change the bisect code accordingly.\n>\n> You could also just decide on your own that this is what you want to do\n> because it is so much more elegant, anyway.\n>\n> > + return res;\n> > +}\n> > +\n> > +static int bisect_auto_next(struct bisect_terms *terms, const char *prefix)\n> > +{\n> > + if (!bisect_next_check(terms, NULL))\n> > + return bisect_next(terms, prefix);\n> > +\n> > + return 0;\n>\n> A common pattern in Git's source code is to present the early return\n> first, i.e.\n>\n> if (bisect_next_check(terms, NULL))\n> return 0;\n>\n> return bisect_next(terms, prefix);\n>\n> I do find it easier to read that way, too.\n>\n> > +\n> > static int bisect_start(struct bisect_terms *terms, int no_checkout,\n> > const char **argv, int argc)\n> > {\n> > @@ -625,7 +779,9 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n> > CHECK_AND_SET_TERMS,\n> > BISECT_NEXT_CHECK,\n> > BISECT_TERMS,\n> > - BISECT_START\n> > + BISECT_START,\n> > + BISECT_NEXT,\n> > + BISECT_AUTO_NEXT,\n> > } cmdmode = 0;\n> > int no_checkout = 0, res = 0, nolog = 0;\n> > struct option options[] = {\n> > @@ -649,6 +805,10 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n> > N_(\"print out the bisect terms\"), BISECT_TERMS),\n> > OPT_CMDMODE(0, \"bisect-start\", &cmdmode,\n> > N_(\"start the bisect session\"), BISECT_START),\n> > + OPT_CMDMODE(0, \"bisect-next\", &cmdmode,\n> > + N_(\"find the next bisection commit\"), BISECT_NEXT),\n> > + OPT_CMDMODE(0, \"bisect-auto-next\", &cmdmode,\n> > + N_(\"verify the next bisection state then checkout the next bisection commit\"), BISECT_AUTO_NEXT),\n> > OPT_BOOL(0, \"no-checkout\", &no_checkout,\n> > N_(\"update BISECT_HEAD instead of checking out the current commit\")),\n> > OPT_BOOL(0, \"no-log\", &nolog,\n> > @@ -710,6 +870,18 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n> > set_terms(&terms, \"bad\", \"good\");\n> > res = bisect_start(&terms, no_checkout, argv, argc);\n> > break;\n> > + case BISECT_NEXT:\n> > + if (argc)\n> > + return error(_(\"--bisect-next requires 0 arguments\"));\n> > + get_terms(&terms);\n> > + res = bisect_next(&terms, prefix);\n> > + break;\n> > + case BISECT_AUTO_NEXT:\n> > + if (argc)\n> > + return error(_(\"--bisect-auto-next requires 0 arguments\"));\n> > + get_terms(&terms);\n> > + res = bisect_auto_next(&terms, prefix);\n> > + break;\n> > default:\n> > return error(\"BUG: unknown subcommand '%d'\", cmdmode);\n> > }\n> > diff --git a/git-bisect.sh b/git-bisect.sh\n> > index efee12b8b1..7531b74708 100755\n> > --- a/git-bisect.sh\n> > +++ b/git-bisect.sh\n> > @@ -87,7 +87,7 @@ bisect_start() {\n> > # Check if we can proceed to the next bisect state.\n> > #\n> > get_terms\n> > - bisect_auto_next\n> > + git bisect--helper --bisect-auto-next || exit\n> >\n> > trap '-' 0\n> > }\n> > @@ -140,45 +140,7 @@ bisect_state() {\n> > *)\n> > usage ;;\n> > esac\n> > - bisect_auto_next\n> > -}\n> > -\n> > -bisect_auto_next() {\n> > - git bisect--helper --bisect-next-check $TERM_GOOD $TERM_BAD && bisect_next || :\n> > -}\n> > -\n> > -bisect_next() {\n> > - case \"$#\" in 0) ;; *) usage ;; esac\n> > - bisect_autostart\n> > - git bisect--helper --bisect-next-check $TERM_GOOD $TERM_BAD $TERM_GOOD|| exit\n> > -\n> > - # Perform all bisection computation, display and checkout\n> > - git bisect--helper --next-all $(test -f \"$GIT_DIR/BISECT_HEAD\" && echo --no-checkout)\n> > - res=$?\n> > -\n> > - # Check if we should exit because bisection is finished\n> > - if test $res -eq 10\n> > - then\n> > - bad_rev=$(git show-ref --hash --verify refs/bisect/$TERM_BAD)\n> > - bad_commit=$(git show-branch $bad_rev)\n> > - echo \"# first $TERM_BAD commit: $bad_commit\" >>\"$GIT_DIR/BISECT_LOG\"\n> > - exit 0\n> > - elif test $res -eq 2\n> > - then\n> > - echo \"# only skipped commits left to test\" >>\"$GIT_DIR/BISECT_LOG\"\n> > - good_revs=$(git for-each-ref --format=\"%(objectname)\" \"refs/bisect/$TERM_GOOD-*\")\n> > - for skipped in $(git rev-list refs/bisect/$TERM_BAD --not $good_revs)\n> > - do\n> > - skipped_commit=$(git show-branch $skipped)\n> > - echo \"# possible first $TERM_BAD commit: $skipped_commit\" >>\"$GIT_DIR/BISECT_LOG\"\n> > - done\n> > - exit $res\n> > - fi\n> > -\n> > - # Check for an error in the bisection process\n> > - test $res -ne 0 && exit $res\n> > -\n> > - return 0\n> > + git bisect--helper --bisect-auto-next\n>\n> Beautiful.\n>\n> > }\n> >\n> > bisect_visualize() {\n> > @@ -232,7 +194,7 @@ bisect_replay () {\n> > die \"$(gettext \"?? what are you talking about?\")\" ;;\n> > esac\n> > done <\"$file\"\n> > - bisect_auto_next\n> > + git bisect--helper --bisect-auto-next\n> > }\n> >\n> > bisect_run () {\n> > @@ -329,7 +291,8 @@ case \"$#\" in\n> > bisect_skip \"$@\" ;;\n> > next)\n> > # Not sure we want \"next\" at the UI level anymore.\n> > - bisect_next \"$@\" ;;\n> > + get_terms\n>\n> I vaguely remember that we talked about this, or at least about a similar\n> scenario. It needs to be explained in the commit message why we need to\n> call `get_terms` here when previously, we did not.\n>\n> Of course, after thinking about this and looking around for a couple of\n> minutes, I know why. My point is that I, or for that matter, any reader of\n> this commit, should not need to repeat that analysis.\n>\n> Other than that, the patch looks good.\n>\nFor the rest of your suggestions I haven't answered, I will wait to my\nmentor opinion first. :)\nThank you.\n\n> As I said, I will stop reviewing the remainder of this patch series, as it\n> has been removed from v2 and will probably be presented as a follow-up\n> patch series soon.\n>\nYes, they will be sent as soon as the part1 is in `next`:).\n\nBest,\nMiriam\n> Thanks,\n> Dscho\n>\n> > + git bisect--helper --bisect-next \"$@\" || exit ;;\n> > visualize|view)\n> > bisect_visualize \"$@\" ;;\n> > reset)\n> > --\n> > 2.21.1 (Apple Git-122.3)\n> >\n> >\n"},{"id":"391896","messageId":"CAP8UFD1UBPwYLUEdUJj3k1Yj0Y9dD6RyGh1dFL=wsSNihErqZQ@mail.gmail.com","threadId":"52666","inReplyTo":"nycvar.QRO.7.76.6.2001301619340.46@tvgsbejvaqbjf.bet","subject":"Re: [PATCH 12/29] bisect--helper: reimplement `bisect_next` and `bisect_auto_next` shell functions in C","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2020-02-17T07:20:39Z","receivedAt":"2020-02-17T07:20:58Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Hi Dscho,\n\nThanks for your review! I agree with everything you said except a few\nthings below.\n\nOn Thu, Jan 30, 2020 at 11:47 PM Johannes Schindelin\n wrote:\n>\n> On Mon, 20 Jan 2020, Miriam Rubio wrote:\n\n> > +static void prepare_rev_argv(struct bisect_terms *terms, struct argv_array *rev_argv)> > +{\n> > + struct string_list good_revs = STRING_LIST_INIT_DUP;\n> > + char *term_good = xstrfmt(\"%s-*\", terms->term_good);\n> > +\n> > + for_each_glob_ref_in(register_good_ref, term_good,\n> > + \"refs/bisect/\", &good_revs);\n> > +\n> > + argv_array_pushl(rev_argv, \"skipped_commits\", \"refs/bisect/bad\", \"--not\", NULL);\n> > + for (int i = 0; i < good_revs.nr; i++)\n> > + argv_array_push(rev_argv, good_revs.items[i].string);\n> > +\n> > + string_list_clear(&good_revs, 0);\n> > + free(term_good);\n> > +}\n>\n> Maybe we should fold that into `prepare_revs()`? We could then render the\n> arguments directly into `revs` (via `add_pending_object()`, after setting\n> obj->flags |= UNINTERESTING`) rather than formatting them into a string\n> list, then deep-copy them into an `argv_array` only to parse them back\n> into OIDs that we already had in the first place.\n\nThe current code is a straightforward port from shell. If we do what\nyou suggest, yeah, it will be less wasteful, but on the other hand it\nwill be less easy to see that we are doing a good job of properly\nporting code from shell. I suggest we try to focus on the later rather\nthan the former right now, especially as performance is not very\nimportant here. Further improvements on top could be left for another\npatch series or perhaps as microprojects for GSoC or Outreachy\napplicants.\n\nUsing small functions also makes it easy to see that we are properly\nreleasing memory. A previous version of this code had everything into\na big function that used goto statements and it was less clear that we\nreleased everything.\n\n> > +static int bisect_skipped_commits(struct bisect_terms *terms)\n> > +{\n> > + int res = 0;\n> > + FILE *fp = NULL;\n> > + struct rev_info revs;\n> > +\n> > + fp = fopen(git_path_bisect_log(), \"a\");\n> > + if (!fp)\n> > + return error_errno(_(\"could not open '%s' for appending\"),\n> > + git_path_bisect_log());\n> > +\n> > + res = prepare_revs(terms, &revs);\n> > +\n> > + if (!res)\n> > + res = process_skipped_commits(fp, terms, &revs);\n> > +\n> > + fclose(fp);\n> > + return res;\n> > +}\n>\n> This is again a very short wrapper around another function, so it will\n> probably make sense to merge the two, otherwise the boilerplate might very\n> well outweigh the actual code doing actual work.\n\nYeah, there is perhaps a significant amount of boiler plate, but the\ncode is much easier to check for leaks than when everything was in the\nsame big function and there were goto statements, so I think it's a\nreasonable trade-off\n\n> > + fclose(fp);\n> > + } else {\n> > + res = error_errno(_(\"could not open '%s' for \"\n> > + \"appending\"),\n> > + git_path_bisect_log());\n> > + }\n>\n> This pattern of opening a file, writing something into it, and then return\n> success, otherwise failure, seems like a repeated pattern. In other words,\n> it would be a good candidate for factoring out into its own function.\n\nYeah, but it seems that in this patch series we use the pattern only\nonce. So I think it's fair to leave that for another patch series with\ncleanups and performance improvements or perhaps for microprojects.\n\nThanks,\nChristian.\n"},{"id":"391956","messageId":"nycvar.QRO.7.76.6.2002172245430.46@tvgsbejvaqbjf.bet","threadId":"52666","inReplyTo":"CAP8UFD1UBPwYLUEdUJj3k1Yj0Y9dD6RyGh1dFL=wsSNihErqZQ@mail.gmail.com","subject":"Re: [PATCH 12/29] bisect--helper: reimplement `bisect_next` and `bisect_auto_next` shell functions in C","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-02-17T22:00:55Z","receivedAt":"2020-02-17T22:01:16Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Chris,\n\nOn Mon, 17 Feb 2020, Christian Couder wrote:\n\n> On Thu, Jan 30, 2020 at 11:47 PM Johannes Schindelin\n> wrote:\n> >\n> > On Mon, 20 Jan 2020, Miriam Rubio wrote:\n>\n> > > +static void prepare_rev_argv(struct bisect_terms *terms, struct argv_array *rev_argv)> > +{\n> > > + struct string_list good_revs = STRING_LIST_INIT_DUP;\n> > > + char *term_good = xstrfmt(\"%s-*\", terms->term_good);\n> > > +\n> > > + for_each_glob_ref_in(register_good_ref, term_good,\n> > > + \"refs/bisect/\", &good_revs);\n> > > +\n> > > + argv_array_pushl(rev_argv, \"skipped_commits\", \"refs/bisect/bad\", \"--not\", NULL);\n> > > + for (int i = 0; i < good_revs.nr; i++)\n> > > + argv_array_push(rev_argv, good_revs.items[i].string);\n> > > +\n> > > + string_list_clear(&good_revs, 0);\n> > > + free(term_good);\n> > > +}\n> >\n> > Maybe we should fold that into `prepare_revs()`? We could then render the\n> > arguments directly into `revs` (via `add_pending_object()`, after setting\n> > obj->flags |= UNINTERESTING`) rather than formatting them into a string\n> > list, then deep-copy them into an `argv_array` only to parse them back\n> > into OIDs that we already had in the first place.\n>\n> The current code is a straightforward port from shell. If we do what\n> you suggest, yeah, it will be less wasteful, but on the other hand it\n> will be less easy to see that we are doing a good job of properly\n> porting code from shell.\n\nIf you reason that way, you will have to use tons of `run_command()` calls\nto translate the shell code as verbatim as possible.\n\nHowever, as you can see from our commit history, we do not do that.\nInstead, we use the more powerful expressiveness of C to come up with more\nelegant code than to slavishly convert shell code to inelegant C code.\n\n> I suggest we try to focus on the later rather than the former right now,\n> especially as performance is not very important here.\n\nOh, but my comment was totally not about performance, and pretty much\nexclusively about readability.\n\nIf Miriam goes with my suggestion, it will result in more readable code\nthat is easier to review and therefore much more likely to be free of\nunintentional bugs.\n\n> Using small functions also makes it easy to see that we are properly\n> releasing memory. A previous version of this code had everything into\n> a big function that used goto statements and it was less clear that we\n> released everything.\n\nIf you want to make it easier to avoid double-free()s and memory leaks, I\nam a bit puzzled how you want to claim that the current \"we're copying the\nstrings so often that pretty much everybody loses track of them\" approach\nshould be superior to adding the strings once, and once only, to a string\narray.\n\n> > > +static int bisect_skipped_commits(struct bisect_terms *terms)\n> > > +{\n> > > + int res = 0;\n> > > + FILE *fp = NULL;\n> > > + struct rev_info revs;\n> > > +\n> > > + fp = fopen(git_path_bisect_log(), \"a\");\n> > > + if (!fp)\n> > > + return error_errno(_(\"could not open '%s' for appending\"),\n> > > + git_path_bisect_log());\n> > > +\n> > > + res = prepare_revs(terms, &revs);\n> > > +\n> > > + if (!res)\n> > > + res = process_skipped_commits(fp, terms, &revs);\n> > > +\n> > > + fclose(fp);\n> > > + return res;\n> > > +}\n> >\n> > This is again a very short wrapper around another function, so it will\n> > probably make sense to merge the two, otherwise the boilerplate might very\n> > well outweigh the actual code doing actual work.\n>\n> Yeah, there is perhaps a significant amount of boiler plate, but the\n> code is much easier to check for leaks than when everything was in the\n> same big function and there were goto statements, so I think it's a\n> reasonable trade-off\n\nGiven this snippet, I would strongly disagree with this assessment:\n\n fp = fopen(git_path_bisect_log(), \"a\");\n if (!fp)\n res = error_errno(_(\"could not open '%s' for appending\"),\n git_path_bisect_log());\n else\n res = prepare_revs(terms, &revs);\n\n if (!res)\n res = process_skipped_commits(fp, terms, &revs);\n\n if (fp)\n fclose(fp);\n\nThere is positively no need for a `goto` whatsoever.\n\n> > > + fclose(fp);\n> > > + } else {\n> > > + res = error_errno(_(\"could not open '%s' for \"\n> > > + \"appending\"),\n> > > + git_path_bisect_log());\n> > > + }\n> >\n> > This pattern of opening a file, writing something into it, and then return\n> > success, otherwise failure, seems like a repeated pattern. In other words,\n> > it would be a good candidate for factoring out into its own function.\n>\n> Yeah, but it seems that in this patch series we use the pattern only\n> once. So I think it's fair to leave that for another patch series with\n> cleanups and performance improvements or perhaps for microprojects.\n\nSure, we could repeat past mistakes in this patch series, too.\n\nIf, on the other hand, we use this patch series as \"an excuse\" to\nintroduce such a helper, no future patch series will have to use the same\nkind of argument as you just offered. Instead, we will have an improved\nAPI that will help not only this patch series, but many more to come.\n\nThere is tons of precedent for this kind of thing, where we add an\nintroductory patch at the beginning of a patch series, factoring out\nalready-existing code into a more reusable shape, and then use it.\n\nSo why not repeat that pattern and do the same thing here?\n\nCiao,\nJohannes\n"}]}