{"thread":{"id":"52762","subject":"[Outreachy] [PATCH v3 00/13] Finish converting git bisect to C part 1","startedAt":"2020-02-08T09:08:28Z","lastAt":"2020-02-10T16:05:45Z","messageCount":18,"participants":["Miriam Rubio","René Scharfe","Miriam R.","Taylor Blau"],"isPatch":true,"patchVersion":3,"patchTotal":13},"messages":[{"id":"391361","messageId":"20200208090704.26506-1-mirucam@gmail.com","threadId":"52762","inReplyTo":null,"subject":"[Outreachy] [PATCH v3 00/13] Finish converting git bisect to C part 1","fromName":"Miriam Rubio","fromEmail":"mirucam@gmail.com","sentAt":"2020-02-08T09:06:51Z","receivedAt":"2020-02-08T09:08:28Z","isPatch":true,"sender":{"key":"mirucam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56339109?v=4"},"body":"These patches correspond to a first part of patch series \nof Outreachy project \"Finish converting `git bisect` from shell to C\" \nstarted by Pranit Bauva and Tanushree Tumane\n(https://public-inbox.org/git/pull.117.git.gitgitgadget@gmail.com) and\ncontinued by me.\n\nThis first part is formed of preparatory/clean-up patches and all \n`bisect.c` libification work. \n\nI would like to thank Johannes Schindelin and Junio Hamano for reviewing\nthe patch series.\n\nThese patch series emails were generated from:\nhttps://gitlab.com/mirucam/git/commits/git-bisect-work-part1-v3\n\n--- Changes since v2 Finish converting git bisect to C part 1 patch series ---\n\nGeneral changes\n---------------\n\n* Rebase on master branch: de93cc14ab (The third batch for 2.26, 2020-02-05).\n* Amend commit messages.\n* Add an enum to represent bisecting error codes.\n\nSpecific changes\n----------------\n\n[5/13] bisect--helper: introduce new `decide_next()` function\n\n* Change const char* types to char* and fix `free()` calls.\n\n--\n\n[6/13] bisect: add enum to represent bisect returning codes\n\n* New patch that adds an enum to represent bisecting error codes.\n\n--\n\n[7/13] bisect--helper: return error codes from `cmd_bisect__helper()`\n\n* New patch split from `libify `check_good_are_ancestors_of_bad` and its \ndependents`.\n* Change code in return for readability.\n\n--\n\n[8/13] bisect: libify `exit_if_skipped_commits` to `error_if_skipped*` \nand its dependents\n\n* Remove comments.\n\n--\n\n[9/13] bisect: libify `bisect_checkout`\n\n* Fix declaration block.\n* Change code in return for readability.\n* Add comments.\n\n--\n\n[11/13] bisect: libify `check_good_are_ancestors_of_bad` and its \ndependents\n\n* Fix brace location.\n\n--\n\nMiriam Rubio (3):\n  bisect--helper: convert `vocab_*` char pointers to char arrays\n  bisect: use the standard 'if (!var)' way to check for 0\n  bisect: add enum to represent bisect returning codes\n\nPranit Bauva (8):\n  run-command: make `exists_in_PATH()` non-static\n  bisect--helper: return error codes from `cmd_bisect__helper()`\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\nTanushree Tumane (2):\n  bisect--helper: change `retval` to `res`\n  bisect--helper: introduce new `decide_next()` function\n\n bisect.c                 | 135 +++++++++++++++++++++++++++------------\n bisect.h                 |  29 ++++++++-\n builtin/bisect--helper.c | 123 +++++++++++++++++++----------------\n run-command.c            |   2 +-\n run-command.h            |  11 ++++\n 5 files changed, 201 insertions(+), 99 deletions(-)\n\n-- \n2.25.0\n\n"},{"id":"391362","messageId":"20200208090704.26506-2-mirucam@gmail.com","threadId":"52762","inReplyTo":"20200208090704.26506-1-mirucam@gmail.com","subject":"[PATCH v3 01/13] bisect--helper: convert `vocab_*` char pointers to char arrays","fromName":"Miriam Rubio","fromEmail":"mirucam@gmail.com","sentAt":"2020-02-08T09:06:52Z","receivedAt":"2020-02-08T09:08:28Z","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 <chriscool@tuxfamily.org>\nSigned-off-by: Miriam Rubio <mirucam@gmail.com>\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.25.0\n\n"},{"id":"391363","messageId":"20200208090704.26506-3-mirucam@gmail.com","threadId":"52762","inReplyTo":"20200208090704.26506-1-mirucam@gmail.com","subject":"[PATCH v3 02/13] bisect--helper: change `retval` to `res`","fromName":"Miriam Rubio","fromEmail":"mirucam@gmail.com","sentAt":"2020-02-08T09:06:53Z","receivedAt":"2020-02-08T09:08:32Z","isPatch":true,"sender":{"key":"mirucam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56339109?v=4"},"body":"From: Tanushree Tumane <tanushreetumane@gmail.com>\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 <chriscool@tuxfamily.org>\nSigned-off-by: Tanushree Tumane <tanushreetumane@gmail.com>\nSigned-off-by: Miriam Rubio <mirucam@gmail.com>\n---\n builtin/bisect--helper.c | 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 \"<valid-branch>'.\"),\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.25.0\n\n"},{"id":"391365","messageId":"20200208090704.26506-4-mirucam@gmail.com","threadId":"52762","inReplyTo":"20200208090704.26506-1-mirucam@gmail.com","subject":"[PATCH v3 03/13] bisect: use the standard 'if (!var)' way to check for 0","fromName":"Miriam Rubio","fromEmail":"mirucam@gmail.com","sentAt":"2020-02-08T09:06:54Z","receivedAt":"2020-02-08T09:08:32Z","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 <chriscool@tuxfamily.org>\nSigned-off-by: Miriam Rubio <mirucam@gmail.com>\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.25.0\n\n"},{"id":"391364","messageId":"20200208090704.26506-5-mirucam@gmail.com","threadId":"52762","inReplyTo":"20200208090704.26506-1-mirucam@gmail.com","subject":"[PATCH v3 04/13] run-command: make `exists_in_PATH()` non-static","fromName":"Miriam Rubio","fromEmail":"mirucam@gmail.com","sentAt":"2020-02-08T09:06:55Z","receivedAt":"2020-02-08T09:08:33Z","isPatch":true,"sender":{"key":"mirucam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56339109?v=4"},"body":"From: Pranit Bauva <pranit.bauva@gmail.com>\n\nRemoves the `static` keyword from `exists_in_PATH()` function\nand declares the function in `run-command.h` file.\nThe function will be used in bisect_visualize() in a later\ncommit.\n\n`exists_in_PATH()` and `locate_in_PATH()` functions don't\ndepend on file-local variables.\n\nMentored by: Christian Couder <chriscool@tuxfamily.org>\nMentored by: Johannes Schindelin <Johannes.Schindelin@gmx.de>\nSigned-off-by: Tanushree Tumane <tanushreetumane@gmail.com>\nSigned-off-by: Miriam Rubio <mirucam@gmail.com>\n---\n run-command.c |  2 +-\n run-command.h | 11 +++++++++++\n 2 files changed, 12 insertions(+), 1 deletion(-)\n\ndiff --git a/run-command.c b/run-command.c\nindex f5e1149f9b..4df975178d 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 \tint found = r != NULL;\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.25.0\n\n"},{"id":"391366","messageId":"20200208090704.26506-7-mirucam@gmail.com","threadId":"52762","inReplyTo":"20200208090704.26506-1-mirucam@gmail.com","subject":"[PATCH v3 06/13] bisect: add enum to represent bisect returning codes","fromName":"Miriam Rubio","fromEmail":"mirucam@gmail.com","sentAt":"2020-02-08T09:06:57Z","receivedAt":"2020-02-08T09:08:36Z","isPatch":true,"sender":{"key":"mirucam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56339109?v=4"},"body":"Since 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\nCreate an enum called `bisect_error` with the bisecting return codes\nto use in `bisect.c` libification process.\n\nChange bisect_next_all() to make it return this enum.\n\nMentored-by: Christian Couder <chriscool@tuxfamily.org>\nSigned-off-by: Miriam Rubio <mirucam@gmail.com>\n---\n bisect.c |  2 +-\n bisect.h | 14 +++++++++++++-\n 2 files changed, 14 insertions(+), 2 deletions(-)\n\ndiff --git a/bisect.c b/bisect.c\nindex 83cb5b3a98..e4573c7ba1 100644\n--- a/bisect.c\n+++ b/bisect.c\n@@ -945,7 +945,7 @@ void read_bisect_terms(const char **read_bad, const char **read_good)\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-int bisect_next_all(struct repository *r, const char *prefix, int no_checkout)\n+enum bisect_error bisect_next_all(struct repository *r, const char *prefix, int no_checkout)\n {\n \tstruct rev_info revs;\n \tstruct commit_list *tried;\ndiff --git a/bisect.h b/bisect.h\nindex 4e69a11ea8..c921ead02c 100644\n--- a/bisect.h\n+++ b/bisect.h\n@@ -31,7 +31,19 @@ struct rev_list_info {\n \tconst char *header_prefix;\n };\n \n-int bisect_next_all(struct repository *r,\n+/*\n+ * enum bisect_error represents the following return codes:\n+ * BISECT_OK: success code. Internally, it means that next\n+ * commit has been found (and possibly checked out) and it\n+ * should be tested.\n+ * BISECT_FAILED error code: default error code.\n+ */\n+enum bisect_error {\n+\tBISECT_OK = 0,\n+\tBISECT_FAILED = -1\n+};\n+\n+enum bisect_error bisect_next_all(struct repository *r,\n \t\t    const char *prefix,\n \t\t    int no_checkout);\n \n-- \n2.25.0\n\n"},{"id":"391367","messageId":"20200208090704.26506-6-mirucam@gmail.com","threadId":"52762","inReplyTo":"20200208090704.26506-1-mirucam@gmail.com","subject":"[PATCH v3 05/13] bisect--helper: introduce new `decide_next()` function","fromName":"Miriam Rubio","fromEmail":"mirucam@gmail.com","sentAt":"2020-02-08T09:06:56Z","receivedAt":"2020-02-08T09:08:36Z","isPatch":true,"sender":{"key":"mirucam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56339109?v=4"},"body":"From: Tanushree Tumane <tanushreetumane@gmail.com>\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\nWhile at it `bad_ref` and `good_glob` are not const any more\nto void casting them inside `free()`.\n\nMentored-by: Christian Couder <chriscool@tuxfamily.org>\nSigned-off-by: Tanushree Tumane <tanushreetumane@gmail.com>\nSigned-off-by: Miriam Rubio <mirucam@gmail.com>\n---\n builtin/bisect--helper.c | 66 +++++++++++++++++++++-------------------\n 1 file changed, 34 insertions(+), 32 deletions(-)\n\ndiff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\nindex 21de5c096c..e21d3d1a4c 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-\tfree((void *) good_glob);\n-\tfree((void *) bad_ref);\n-\treturn res;\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+\tchar *bad_ref = xstrfmt(\"refs/bisect/%s\", terms->term_bad);\n+\tchar *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(good_glob);\n+\tfree(bad_ref);\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.25.0\n\n"},{"id":"391368","messageId":"20200208090704.26506-8-mirucam@gmail.com","threadId":"52762","inReplyTo":"20200208090704.26506-1-mirucam@gmail.com","subject":"[PATCH v3 07/13] bisect--helper: return error codes from `cmd_bisect__helper()`","fromName":"Miriam Rubio","fromEmail":"mirucam@gmail.com","sentAt":"2020-02-08T09:06:58Z","receivedAt":"2020-02-08T09:08:37Z","isPatch":true,"sender":{"key":"mirucam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56339109?v=4"},"body":"From: Pranit Bauva <pranit.bauva@gmail.com>\n\nSince we want to get rid of git-bisect.sh, it would be necessary\nto convert bisect.c exit() calls to return statements so\nthat errors can be reported. Let's prepare for that by making\nit possible to return different error codes than just 0 or 1.\n\nDifferent error codes might enable a bisecting script calling the\nbisect command that uses this function to do different things\ndepending on the exit status of the bisect command.\n\nMentored-by: Christian Couder <chriscool@tuxfamily.org>\nMentored-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>\nSigned-off-by: Tanushree Tumane <tanushreetumane@gmail.com>\nSigned-off-by: Miriam Rubio <mirucam@gmail.com>\n---\n builtin/bisect--helper.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\nindex e21d3d1a4c..e6bd4d6645 100644\n--- a/builtin/bisect--helper.c\n+++ b/builtin/bisect--helper.c\n@@ -713,5 +713,5 @@ 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+\treturn abs(res);\n }\n-- \n2.25.0\n\n"},{"id":"391369","messageId":"20200208090704.26506-9-mirucam@gmail.com","threadId":"52762","inReplyTo":"20200208090704.26506-1-mirucam@gmail.com","subject":"[PATCH v3 08/13] bisect: libify `exit_if_skipped_commits` to `error_if_skipped*` and its dependents","fromName":"Miriam Rubio","fromEmail":"mirucam@gmail.com","sentAt":"2020-02-08T09:06:59Z","receivedAt":"2020-02-08T09:08:40Z","isPatch":true,"sender":{"key":"mirucam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56339109?v=4"},"body":"From: Pranit Bauva <pranit.bauva@gmail.com>\n\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(<positive-value>)` to\n`return <negative-value>`. Follow POSIX conventions to return\n<negative-value> to indicate error.\n\nUpdate all callers to handle the error returns.\n\nMentored-by: Christian Couder <chriscool@tuxfamily.org>\nMentored-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>\nSigned-off-by: Pranit Bauva <pranit.bauva@gmail.com>\nSigned-off-by: Tanushree Tumane <tanushreetumane@gmail.com>\nSigned-off-by: Miriam Rubio <mirucam@gmail.com>\n---\n bisect.c | 17 +++++++++++------\n bisect.h |  5 ++++-\n 2 files changed, 15 insertions(+), 7 deletions(-)\n\ndiff --git a/bisect.c b/bisect.c\nindex e4573c7ba1..85bda3500b 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 enum bisect_error 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 BISECT_OK;\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,8 @@ 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+\treturn BISECT_ONLY_SKIPPED_LEFT;\n }\n \n static int is_expected_rev(const struct object_id *oid)\n@@ -950,6 +951,7 @@ enum bisect_error bisect_next_all(struct repository *r, const char *prefix, int\n \tstruct rev_info revs;\n \tstruct commit_list *tried;\n \tint reaches = 0, all = 0, nr, steps;\n+\tenum bisect_error res = BISECT_OK;\n \tstruct object_id *bisect_rev;\n \tchar *steps_msg;\n \n@@ -972,8 +974,9 @@ enum bisect_error bisect_next_all(struct repository *r, const char *prefix, int\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 < 0)\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 +993,9 @@ enum bisect_error bisect_next_all(struct repository *r, const char *prefix, int\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);\ndiff --git a/bisect.h b/bisect.h\nindex c921ead02c..19d90e4870 100644\n--- a/bisect.h\n+++ b/bisect.h\n@@ -37,10 +37,13 @@ struct rev_list_info {\n  * commit has been found (and possibly checked out) and it\n  * should be tested.\n  * BISECT_FAILED error code: default error code.\n+ * BISECT_ONLY_SKIPPED_LEFT error code: only skipped\n+ * commits left to be tested.\n  */\n enum bisect_error {\n \tBISECT_OK = 0,\n-\tBISECT_FAILED = -1\n+\tBISECT_FAILED = -1,\n+\tBISECT_ONLY_SKIPPED_LEFT = -2\n };\n \n enum bisect_error bisect_next_all(struct repository *r,\n-- \n2.25.0\n\n"},{"id":"391370","messageId":"20200208090704.26506-10-mirucam@gmail.com","threadId":"52762","inReplyTo":"20200208090704.26506-1-mirucam@gmail.com","subject":"[PATCH v3 09/13] bisect: libify `bisect_checkout`","fromName":"Miriam Rubio","fromEmail":"mirucam@gmail.com","sentAt":"2020-02-08T09:07:00Z","receivedAt":"2020-02-08T09:08:40Z","isPatch":true,"sender":{"key":"mirucam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56339109?v=4"},"body":"From: Pranit Bauva <pranit.bauva@gmail.com>\n\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(<positive-value>)` to\n`return <negative-value>`. Follow POSIX conventions to return\n<negative-value> 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 <chriscool@tuxfamily.org>\nMentored-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>\nSigned-off-by: Pranit Bauva <pranit.bauva@gmail.com>\nSigned-off-by: Tanushree Tumane <tanushreetumane@gmail.com>\nSigned-off-by: Miriam Rubio <mirucam@gmail.com>\n---\n bisect.c | 19 +++++++++++++++----\n 1 file changed, 15 insertions(+), 4 deletions(-)\n\ndiff --git a/bisect.c b/bisect.c\nindex 85bda3500b..f6582ddfed 100644\n--- a/bisect.c\n+++ b/bisect.c\n@@ -704,9 +704,10 @@ static int is_expected_rev(const struct object_id *oid)\n \treturn res;\n }\n \n-static int bisect_checkout(const struct object_id *bisect_rev, int no_checkout)\n+static enum bisect_error bisect_checkout(const struct object_id *bisect_rev, int no_checkout)\n {\n \tchar bisect_rev_hex[GIT_MAX_HEXSZ + 1];\n+\tenum bisect_error res = BISECT_OK;\n \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@@ -716,14 +717,24 @@ 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\t/*\n+\t\t\t * Errors in `run_command()` itself, signaled by res < 0,\n+\t\t\t * and errors in the child process, signaled by res > 0\n+\t\t\t * can both be treated as regular BISECT_FAILURE (-1).\n+\t\t\t */\n+\t\t\treturn -abs(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+\t/*\n+\t * Errors in `run_command()` itself, signaled by res < 0,\n+\t * and errors in the child process, signaled by res > 0\n+\t * can both be treated as regular BISECT_FAILURE (-1).\n+\t */\n+\treturn -abs(res);\n }\n \n static struct commit *get_commit_reference(struct repository *r,\n-- \n2.25.0\n\n"},{"id":"391371","messageId":"20200208090704.26506-11-mirucam@gmail.com","threadId":"52762","inReplyTo":"20200208090704.26506-1-mirucam@gmail.com","subject":"[PATCH v3 10/13] bisect: libify `check_merge_bases` and its dependents","fromName":"Miriam Rubio","fromEmail":"mirucam@gmail.com","sentAt":"2020-02-08T09:07:01Z","receivedAt":"2020-02-08T09:08:41Z","isPatch":true,"sender":{"key":"mirucam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56339109?v=4"},"body":"From: Pranit Bauva <pranit.bauva@gmail.com>\n\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(<positive-value>)` to\n`return <negative-value>`. Follow POSIX conventions to return\n<negative-value> to indicate error.\n\nIn `check_merge_bases()` there is an early success special case,\nso we have introduced special error code\nBISECT_INTERNAL_SUCCESS_MERGE_BASE (-11) which indicates early\nsuccess. This BISECT_INTERNAL_SUCCESS_MERGE_BASE is converted back\nto BISECT_OK (0) in `check_good_are_ancestors_of_bad()`.\n\nUpdate all callers to handle the error returns.\n\nMentored-by: Christian Couder <chriscool@tuxfamily.org>\nMentored by: Johannes Schindelin <Johannes.Schindelin@gmx.de>\nSigned-off-by: Pranit Bauva <pranit.bauva@gmail.com>\nSigned-off-by: Tanushree Tumane <tanushreetumane@gmail.com>\nSigned-off-by: Miriam Rubio <mirucam@gmail.com>\n---\n bisect.c | 22 ++++++++++++++++++----\n bisect.h |  7 ++++++-\n 2 files changed, 24 insertions(+), 5 deletions(-)\n\ndiff --git a/bisect.c b/bisect.c\nindex f6582ddfed..382e0b471f 100644\n--- a/bisect.c\n+++ b/bisect.c\n@@ -811,13 +811,18 @@ 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\n+ * BISECT_INTERNAL_SUCCESS_MERGE_BASE (-11) a special condition\n+ * for early success, this will be converted back to 0 in\n+ * check_good_are_ancestors_of_bad().\n  */\n-static void check_merge_bases(int rev_nr, struct commit **rev, int no_checkout)\n+static enum bisect_error check_merge_bases(int rev_nr, struct commit **rev, int no_checkout)\n {\n+\tenum bisect_error res = BISECT_OK;\n \tstruct commit_list *result;\n \n \tresult = get_merge_bases_many(rev[0], rev_nr - 1, rev + 1);\n@@ -832,11 +837,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 = BISECT_INTERNAL_SUCCESS_MERGE_BASE;\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@@ -871,6 +881,7 @@ static void check_good_are_ancestors_of_bad(struct repository *r,\n \tchar *filename = git_pathdup(\"BISECT_ANCESTORS_OK\");\n \tstruct stat st;\n \tint fd, rev_nr;\n+\tenum bisect_error res = BISECT_OK;\n \tstruct commit **rev;\n \n \tif (!current_bad_oid)\n@@ -885,10 +896,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 == BISECT_INTERNAL_SUCCESS_MERGE_BASE ? BISECT_OK : -res);\n \n \t/* Create file BISECT_ANCESTORS_OK. */\n \tfd = open(filename, O_CREAT | O_TRUNC | O_WRONLY, 0600);\ndiff --git a/bisect.h b/bisect.h\nindex 19d90e4870..f68ae85376 100644\n--- a/bisect.h\n+++ b/bisect.h\n@@ -39,11 +39,16 @@ struct rev_list_info {\n  * BISECT_FAILED error code: default error code.\n  * BISECT_ONLY_SKIPPED_LEFT error code: only skipped\n  * commits left to be tested.\n+ * BISECT_INTERNAL_SUCCESS_MERGE_BASE early success\n+ * code: found merge base that should be tested.\n+ * Early success code BISECT_INTERNAL_SUCCESS_MERGE_BASE\n+ * should be only an internal code.\n  */\n enum bisect_error {\n \tBISECT_OK = 0,\n \tBISECT_FAILED = -1,\n-\tBISECT_ONLY_SKIPPED_LEFT = -2\n+\tBISECT_ONLY_SKIPPED_LEFT = -2,\n+\tBISECT_INTERNAL_SUCCESS_MERGE_BASE = -11\n };\n \n enum bisect_error bisect_next_all(struct repository *r,\n-- \n2.25.0\n\n"},{"id":"391372","messageId":"20200208090704.26506-13-mirucam@gmail.com","threadId":"52762","inReplyTo":"20200208090704.26506-1-mirucam@gmail.com","subject":"[PATCH v3 12/13] bisect: libify `handle_bad_merge_base` and its dependents","fromName":"Miriam Rubio","fromEmail":"mirucam@gmail.com","sentAt":"2020-02-08T09:07:03Z","receivedAt":"2020-02-08T09:08:42Z","isPatch":true,"sender":{"key":"mirucam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56339109?v=4"},"body":"From: Pranit Bauva <pranit.bauva@gmail.com>\n\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(<positive-value>)` to\n`return <negative-value>`. Follow POSIX conventions to return\n<negative-value> to indicate error.\n\nUpdate all callers to handle the error returns.\n\nMentored-by: Christian Couder <chriscool@tuxfamily.org>\nMentored-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>\nSigned-off-by: Pranit Bauva <pranit.bauva@gmail.com>\nSigned-off-by: Tanushree Tumane <tanushreetumane@gmail.com>\nSigned-off-by: Miriam Rubio <mirucam@gmail.com>\n---\n bisect.c | 9 +++++----\n bisect.h | 1 +\n 2 files changed, 6 insertions(+), 4 deletions(-)\n\ndiff --git a/bisect.c b/bisect.c\nindex f5ce3a4b70..837332a428 100644\n--- a/bisect.c\n+++ b/bisect.c\n@@ -761,7 +761,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 enum bisect_error 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@@ -782,14 +782,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 BISECT_MERGE_BASE_CHECK;\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 BISECT_FAILED;\n }\n \n static void handle_skipped_merge_base(const struct object_id *mb)\n@@ -830,7 +830,8 @@ static enum bisect_error check_merge_bases(int rev_nr, struct commit **rev, int\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)) {\ndiff --git a/bisect.h b/bisect.h\nindex f68ae85376..0d9758179f 100644\n--- a/bisect.h\n+++ b/bisect.h\n@@ -48,6 +48,7 @@ enum bisect_error {\n \tBISECT_OK = 0,\n \tBISECT_FAILED = -1,\n \tBISECT_ONLY_SKIPPED_LEFT = -2,\n+\tBISECT_MERGE_BASE_CHECK = -3,\n \tBISECT_INTERNAL_SUCCESS_MERGE_BASE = -11\n };\n \n-- \n2.25.0\n\n"},{"id":"391373","messageId":"20200208090704.26506-12-mirucam@gmail.com","threadId":"52762","inReplyTo":"20200208090704.26506-1-mirucam@gmail.com","subject":"[PATCH v3 11/13] bisect: libify `check_good_are_ancestors_of_bad` and its dependents","fromName":"Miriam Rubio","fromEmail":"mirucam@gmail.com","sentAt":"2020-02-08T09:07:02Z","receivedAt":"2020-02-08T09:08:43Z","isPatch":true,"sender":{"key":"mirucam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56339109?v=4"},"body":"From: Pranit Bauva <pranit.bauva@gmail.com>\n\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(<positive-value>)` to\n`return <negative-value>`. Follow POSIX conventions to return\n<negative-value> to indicate error.\n\nCode that turns BISECT_INTERNAL_SUCCESS_MERGE_BASE (-11)\n to BISECT_OK (0) from `check_good_are_ancestors_of_bad()` has been moved to\n`cmd_bisect__helper()`.\n\nUpdate all callers to handle the error returns.\n\nMentored-by: Christian Couder <chriscool@tuxfamily.org>\nMentored by: Johannes Schindelin <Johannes.Schindelin@gmx.de>\nSigned-off-by: Pranit Bauva <pranit.bauva@gmail.com>\nSigned-off-by: Tanushree Tumane <tanushreetumane@gmail.com>\nSigned-off-by: Miriam Rubio <mirucam@gmail.com>\n---\n bisect.c                 | 41 ++++++++++++++++++++++++++--------------\n builtin/bisect--helper.c | 11 ++++++++++-\n 2 files changed, 37 insertions(+), 15 deletions(-)\n\ndiff --git a/bisect.c b/bisect.c\nindex 382e0b471f..f5ce3a4b70 100644\n--- a/bisect.c\n+++ b/bisect.c\n@@ -872,20 +872,23 @@ 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 enum bisect_error 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-\tchar *filename = git_pathdup(\"BISECT_ANCESTORS_OK\");\n+\tchar *filename;\n \tstruct stat st;\n \tint fd, rev_nr;\n \tenum bisect_error res = BISECT_OK;\n \tstruct commit **rev;\n \n \tif (!current_bad_oid)\n-\t\tdie(_(\"a %s revision is needed\"), term_bad);\n+\t\treturn error(_(\"a %s revision is needed\"), term_bad);\n+\n+\tfilename = git_pathdup(\"BISECT_ANCESTORS_OK\");\n \n \t/* Check if file BISECT_ANCESTORS_OK exists. */\n \tif (!stat(filename, &st) && S_ISREG(st.st_mode))\n@@ -901,18 +904,26 @@ 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 == BISECT_INTERNAL_SUCCESS_MERGE_BASE ? BISECT_OK : -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+\tif (!res) {\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\t/*\n+\t\t\t * BISECT_ANCESTORS_OK file is not absolutely necessary,\n+\t\t\t * the bisection process will continue at the next\n+\t\t\t * bisection step.\n+\t\t\t * So, just signal with a warning that something\n+\t\t\t * might be wrong.\n+\t\t\t */\n+\t\t\twarning_errno(_(\"could not create file '%s'\"),\n+\t\t\t\tfilename);\n+\t\telse\n+\t\t\tclose(fd);\n+\t}\n  done:\n \tfree(filename);\n+\treturn res;\n }\n \n /*\n@@ -984,7 +995,9 @@ enum bisect_error bisect_next_all(struct repository *r, const char *prefix, int\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 e6bd4d6645..c1c40b516d 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,13 @@ 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+\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 == BISECT_INTERNAL_SUCCESS_MERGE_BASE)\n+\t\tres = BISECT_OK;\n+\n \treturn abs(res);\n }\n-- \n2.25.0\n\n"},{"id":"391374","messageId":"20200208090704.26506-14-mirucam@gmail.com","threadId":"52762","inReplyTo":"20200208090704.26506-1-mirucam@gmail.com","subject":"[PATCH v3 13/13] bisect: libify `bisect_next_all`","fromName":"Miriam Rubio","fromEmail":"mirucam@gmail.com","sentAt":"2020-02-08T09:07:04Z","receivedAt":"2020-02-08T09:08:48Z","isPatch":true,"sender":{"key":"mirucam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56339109?v=4"},"body":"From: Pranit Bauva <pranit.bauva@gmail.com>\n\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(<positive-value>)` to\n`return <negative-value>`. Follow POSIX conventions to return\n<negative-value> to indicate error.\n\nAll the functions calling `bisect_next_all()` are already able to\nhandle return values from it.\n\nMentored-by: Christian Couder <chriscool@tuxfamily.org>\nMentored-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>\nSigned-off-by: Pranit Bauva <pranit.bauva@gmail.com>\nSigned-off-by: Tanushree Tumane <tanushreetumane@gmail.com>\nSigned-off-by: Miriam Rubio <mirucam@gmail.com>\n---\n bisect.c | 29 +++++++++++++++++++----------\n bisect.h | 10 ++++++++--\n 2 files changed, 27 insertions(+), 12 deletions(-)\n\ndiff --git a/bisect.c b/bisect.c\nindex 837332a428..9154f810f7 100644\n--- a/bisect.c\n+++ b/bisect.c\n@@ -976,10 +976,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+ * We use the convention that return BISECT_INTERNAL_SUCCESS_1ST_BAD_FOUND (-10) means\n  * the bisection process finished successfully.\n- * In this case the calling shell script should exit 0.\n- *\n+ * In this case the calling function or command should not turn a\n+ * BISECT_INTERNAL_SUCCESS_1ST_BAD_FOUND return code into an error or a non zero exit code.\n  * If no_checkout is non-zero, the bisection process does not\n  * checkout the trial commit but instead simply updates BISECT_HEAD.\n  */\n@@ -1010,23 +1010,25 @@ enum bisect_error bisect_next_all(struct repository *r, const char *prefix, int\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 < 0)\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\treturn BISECT_FAILED;\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\treturn BISECT_NO_TESTABLE_COMMIT;\n \t}\n \n \tbisect_rev = &revs.commits->item->object.oid;\n@@ -1034,12 +1036,19 @@ enum bisect_error bisect_next_all(struct repository *r, const char *prefix, int\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 BISECT_INTERNAL_SUCCESS_1ST_BAD_FOUND (-10)\n+\t\t * 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 BISECT_INTERNAL_SUCCESS_1ST_BAD_FOUND;\n \t}\n \n \tnr = all - reaches - 1;\ndiff --git a/bisect.h b/bisect.h\nindex 0d9758179f..8bad8d8391 100644\n--- a/bisect.h\n+++ b/bisect.h\n@@ -39,16 +39,22 @@ struct rev_list_info {\n  * BISECT_FAILED error code: default error code.\n  * BISECT_ONLY_SKIPPED_LEFT error code: only skipped\n  * commits left to be tested.\n+ * BISECT_MERGE_BASE_CHECK error code: merge base check failed.\n+ * BISECT_NO_TESTABLE_COMMIT error code: no testable commit found.\n+ * BISECT_INTERNAL_SUCCESS_1ST_BAD_FOUND early success code:\n+ * first term_bad commit found.\n  * BISECT_INTERNAL_SUCCESS_MERGE_BASE early success\n  * code: found merge base that should be tested.\n- * Early success code BISECT_INTERNAL_SUCCESS_MERGE_BASE\n- * should be only an internal code.\n+ * Early success codes BISECT_INTERNAL_SUCCESS_1ST_BAD_FOUND and\n+ * BISECT_INTERNAL_SUCCESS_MERGE_BASE should be only internal codes.\n  */\n enum bisect_error {\n \tBISECT_OK = 0,\n \tBISECT_FAILED = -1,\n \tBISECT_ONLY_SKIPPED_LEFT = -2,\n \tBISECT_MERGE_BASE_CHECK = -3,\n+\tBISECT_NO_TESTABLE_COMMIT = -4,\n+\tBISECT_INTERNAL_SUCCESS_1ST_BAD_FOUND = -10,\n \tBISECT_INTERNAL_SUCCESS_MERGE_BASE = -11\n };\n \n-- \n2.25.0\n\n"},{"id":"391375","messageId":"4550b9a5-74ef-2739-990b-6f41f3319129@web.de","threadId":"52762","inReplyTo":"20200208090704.26506-5-mirucam@gmail.com","subject":"Re: [PATCH v3 04/13] run-command: make `exists_in_PATH()` non-static","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2020-02-08T10:55:00Z","receivedAt":"2020-02-08T10:55:41Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 08.02.20 um 10:06 schrieb Miriam Rubio:\n> From: Pranit Bauva <pranit.bauva@gmail.com>\n>\n> Removes the `static` keyword from `exists_in_PATH()` function\n> and declares the function in `run-command.h` file.\n> The function will be used in bisect_visualize() in a later\n> commit.\n\nI couldn't find the mentioned later commit in this series.  Do you\nactually still need to export exists_in_PATH()?\n\nAnd if yes: locate_in_PATH() splits PATH by colon.  That means it\ndoesn't work on Windows, where the paths are separated by semicolons.\nexists_in_PATH() wraps it, so it shares that limitation.  Wouldn't that\ncause issues for your use?\n\nRené\n"},{"id":"391386","messageId":"CAN7CjDB8NKQg279WjT6kWNaENiBRSEqztL0g9HbnS6RB_ZV7+A@mail.gmail.com","threadId":"52762","inReplyTo":"4550b9a5-74ef-2739-990b-6f41f3319129@web.de","subject":"Re: [PATCH v3 04/13] run-command: make `exists_in_PATH()` non-static","fromName":"Miriam R.","fromEmail":"mirucam@gmail.com","sentAt":"2020-02-09T09:59:47Z","receivedAt":"2020-02-09T10:00:00Z","isPatch":true,"sender":{"key":"mirucam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56339109?v=4"},"body":"Hi,\n\nEl sáb., 8 feb. 2020 a las 11:55, René Scharfe (<l.s.r@web.de>) escribió:\n>\n> Am 08.02.20 um 10:06 schrieb Miriam Rubio:\n> > From: Pranit Bauva <pranit.bauva@gmail.com>\n> >\n> > Removes the `static` keyword from `exists_in_PATH()` function\n> > and declares the function in `run-command.h` file.\n> > The function will be used in bisect_visualize() in a later\n> > commit.\n>\n> I couldn't find the mentioned later commit in this series.  Do you\n> actually still need to export exists_in_PATH()?\n\nThis series is part of a bigger patch series\n(https://public-inbox.org/git/20200120143800.900-1-mirucam@gmail.com/)\nthat has been split as a suggestion of a reviewer in order to be\neasily reviewed.\nThis first part is formed of preparatory/clean-up patches and all\n`bisect.c` libification work. The actual patch is one of the\npreparatory patches and the function will be used in the upcoming\npatch series.\n\n>\n> And if yes: locate_in_PATH() splits PATH by colon.  That means it\n> doesn't work on Windows, where the paths are separated by semicolons.\n> exists_in_PATH() wraps it, so it shares that limitation.  Wouldn't that\n> cause issues for your use?\n\nThank you, for point that out.  I will check this.\nBest,\nMiriam\n>\n> René\n"},{"id":"391423","messageId":"20200209231602.GC4530@syl.local","threadId":"52762","inReplyTo":"20200208090704.26506-5-mirucam@gmail.com","subject":"Re: [PATCH v3 04/13] run-command: make `exists_in_PATH()` non-static","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-02-09T23:16:02Z","receivedAt":"2020-02-09T23:16:05Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Sat, Feb 08, 2020 at 10:06:55AM +0100, Miriam Rubio wrote:\n> From: Pranit Bauva <pranit.bauva@gmail.com>\n>\n> Removes the `static` keyword from `exists_in_PATH()` function\n> and declares the function in `run-command.h` file.\n> The function will be used in bisect_visualize() in a later\n> commit.\n\nThere may be some odd line-wrapping going on here.\n\n> `exists_in_PATH()` and `locate_in_PATH()` functions don't\n> depend on file-local variables.\n>\n> Mentored by: Christian Couder <chriscool@tuxfamily.org>\n> Mentored by: Johannes Schindelin <Johannes.Schindelin@gmx.de>\n> Signed-off-by: Tanushree Tumane <tanushreetumane@gmail.com>\n> Signed-off-by: Miriam Rubio <mirucam@gmail.com>\n> ---\n>  run-command.c |  2 +-\n>  run-command.h | 11 +++++++++++\n>  2 files changed, 12 insertions(+), 1 deletion(-)\n>\n> diff --git a/run-command.c b/run-command.c\n> index f5e1149f9b..4df975178d 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>  \tint found = r != NULL;\n> diff --git a/run-command.h b/run-command.h\n> index 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\nThis line-wrapping caught my eye: it looks like 'separators' would fit\non the line before, or at least that this paragraph and the one above it\nare wrapped at two different widths.\n\nI'm not sure that it really matters, since it looks like the wrapping in\nthis file isn't entirely consistent, but I figured that I'd point it out\nnonetheless.\n\n> + */\n> +int exists_in_PATH(const char *);\n\nHave you considered naming this argument? It's somewhat clear from the\ndocumentation, but I don't see a reason not to name it (especially since\nother declarations in this file *do* name their parameters). What about:\n\n  int exists_in_PATH(const char *file);\n\nTo match its definition in 'run-command.c'? (Admittedly, if I had\nwritten this I'd probably call it 'entry', but they should at least be\nconsistent).\n\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> --\n> 2.25.0\n>\n\nThanks,\nTaylor\n"},{"id":"391469","messageId":"CAN7CjDB=n=wfkEb=9kR7RKRm9oUAOsyxYJeU1w5MW=QH40kJVw@mail.gmail.com","threadId":"52762","inReplyTo":"47b51655-6373-0d5f-1397-8cbbb73d6661@web.de","subject":"Re: [PATCH v3 04/13] run-command: make `exists_in_PATH()` non-static","fromName":"Miriam R.","fromEmail":"mirucam@gmail.com","sentAt":"2020-02-10T16:05:31Z","receivedAt":"2020-02-10T16:05:45Z","isPatch":true,"sender":{"key":"mirucam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56339109?v=4"},"body":"Hi René,\n\nEl lun., 10 feb. 2020 a las 16:43, René Scharfe (<l.s.r@web.de>) escribió:\n>\n> Hello Miriam,\n>\n> did you remove the Git mailing list from cc: intentionally?\nNo, no.\nMaybe I clicked reply, instead reply all. Sorry.\nIt is included in CC now.\n\n>\n> Am 10.02.20 um 12:16 schrieb Miriam R.:\n> > El dom., 9 feb. 2020 a las 10:59, Miriam R. (<mirucam@gmail.com>) escribió:\n> >> El sáb., 8 feb. 2020 a las 11:55, René Scharfe (<l.s.r@web.de>) escribió:\n> >>> And if yes: locate_in_PATH() splits PATH by colon.  That means it\n> >>> doesn't work on Windows, where the paths are separated by semicolons.\n> >>> exists_in_PATH() wraps it, so it shares that limitation.  Wouldn't that\n> >>> cause issues for your use?\n> >>\n> >> Thank you, for point that out.  I will check this.\n> >\n> > This function is used only to test if gitk exists. But as gitk does not work\n> > on Windows, then I think it is all ok because the evaluation is going to\n> > return false and nothing has to be changed.\n>\n> Now I'm confused, because I do use gitk on Windows.  The third\n> screenshot on https://gitforwindows.org/ shows it as well, so you don't\n> have to just take my word on it.\n\nOhh, as I'm not a Windows user, and after some google searches it\nseems to me it doesn't. It is clear that I was not asking the correct\nthings, hehe. Thank you for the clarification.\n\n>\n> > Also, as this patch is not really needed for this part1, I will move\n> > it to the upcoming patch series.\n>\n> That makes sense.\n>\n> I guess it's to used in the C equivalent of the Shell function\n> bisect_visualize(), which resolves gitk using type, calls it if it's\n> found and falls back to git log if it isn't.\nExactly.\n\n>Why not try to exec gitk\n> and fall back to git log if that fails?\n>\nIt seems a good solution for me :)\n\nThank you for reviewing.\nBest,\nMiriam.\n\n> René\n"}]}