{"thread":{"id":"52709","subject":"[Outreachy][PATCH v2 00/11] Finish converting git bisect to C part 1","startedAt":"2020-01-28T14:41:12Z","lastAt":"2020-01-31T18:40:09Z","messageCount":29,"participants":["Miriam Rubio","Johannes Schindelin","Miriam R.","Christian Couder","Junio C Hamano"],"isPatch":true,"patchVersion":2,"patchTotal":11},"messages":[{"id":"390624","messageId":"20200128144026.53128-1-mirucam@gmail.com","threadId":"52709","inReplyTo":null,"subject":"[Outreachy][PATCH v2 00/11] Finish converting git bisect to C part 1","fromName":"Miriam Rubio","fromEmail":"mirucam@gmail.com","sentAt":"2020-01-28T14:40:15Z","receivedAt":"2020-01-28T14:41:12Z","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 the interns Pranit Bauva and Tanushree Tumane\n(https://public-inbox.org/git/pull.117.git.gitgitgadget@gmail.com) and\ncontinued by Miriam Rubio.\n\nThis first part are formed of preparatory/clean-up patches and all \n`bisect.c` libification work. \n\nThese patch series emails were generated from:\nhttps://gitlab.com/mirucam/git/commits/git-bisect-work-part1\n\nIt has to be noted that in this version 2 nothing has been done about a \nreviewer suggestion of using enums for error codes, because there was no\nconsensus about using them by the reviewers.\n\n--- Changes since v1 Finish converting git bisect to C part 1 patch series ---\n\nGeneral changes\n---------------\n\n* Previous patch series version has been split in smaller groups \nin order to facilitate revision and integration.\n* Rebase on master branch: c7a6207591 (Sync with maint, 2020-01-27).\n* Improve commit messages.\n\nSpecific changes\n----------------\n\n[6/11] bisect: libify `exit_if_skipped_commits` to `error_if_skipped*` \nand its dependents\n \n* Remove redundant sentences in commit message.\n* Use `if (res < 0)` instead of `if (res)`.\n\n[8/11] bisect: libify `check_merge_bases` and its dependents\n\n* Remove redundant sentence in commit message.\n\n--\n\n[9/11] bisect: libify `check_good_are_ancestors_of_bad` and its \ndependents\n\n* Remove redundant sentences in commit message.\n* Return in `if (!current_bad_oid)` condition.\n\n--\n\n[10/11] bisect: libify `handle_bad_merge_base` and its dependents\n\n* Remove redundant sentence in commit message.\n\n--\n\n[11/11] bisect: libify `bisect_next_all`\n\n* Remove redundant sentence in commit message.\n* Add return codes explanations in `bisect.h`.\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 (7):\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\nTanushree Tumane (2):\n bisect--helper: change `retval` to `res`\n bisect--helper: introduce new `decide_next()` function\n\n bisect.c | 136 +++++++++++++++++++++++++++------------\n bisect.h | 23 +++++++\n builtin/bisect--helper.c | 118 +++++++++++++++++----------------\n run-command.c | 2 +-\n run-command.h | 11 ++++\n 5 files changed, 193 insertions(+), 97 deletions(-)\n\n-- \n2.21.1 (Apple Git-122.3)\n\n"},{"id":"390625","messageId":"20200128144026.53128-2-mirucam@gmail.com","threadId":"52709","inReplyTo":"20200128144026.53128-1-mirucam@gmail.com","subject":"[PATCH v2 01/11] bisect--helper: convert `vocab_*` char pointers to char arrays","fromName":"Miriam Rubio","fromEmail":"mirucam@gmail.com","sentAt":"2020-01-28T14:40:16Z","receivedAt":"2020-01-28T14:41:17Z","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":"390626","messageId":"20200128144026.53128-3-mirucam@gmail.com","threadId":"52709","inReplyTo":"20200128144026.53128-1-mirucam@gmail.com","subject":"[PATCH v2 02/11] bisect--helper: change `retval` to `res`","fromName":"Miriam Rubio","fromEmail":"mirucam@gmail.com","sentAt":"2020-01-28T14:40:17Z","receivedAt":"2020-01-28T14:41:18Z","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":"390627","messageId":"20200128144026.53128-4-mirucam@gmail.com","threadId":"52709","inReplyTo":"20200128144026.53128-1-mirucam@gmail.com","subject":"[PATCH v2 03/11] bisect: use the standard 'if (!var)' way to check for 0","fromName":"Miriam Rubio","fromEmail":"mirucam@gmail.com","sentAt":"2020-01-28T14:40:18Z","receivedAt":"2020-01-28T14:41:21Z","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":"390628","messageId":"20200128144026.53128-5-mirucam@gmail.com","threadId":"52709","inReplyTo":"20200128144026.53128-1-mirucam@gmail.com","subject":"[PATCH v2 04/11] run-command: make `exists_in_PATH()` non-static","fromName":"Miriam Rubio","fromEmail":"mirucam@gmail.com","sentAt":"2020-01-28T14:40:19Z","receivedAt":"2020-01-28T14:41:21Z","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 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.21.1 (Apple Git-122.3)\n\n"},{"id":"390629","messageId":"20200128144026.53128-6-mirucam@gmail.com","threadId":"52709","inReplyTo":"20200128144026.53128-1-mirucam@gmail.com","subject":"[PATCH v2 05/11] bisect--helper: introduce new `decide_next()` function","fromName":"Miriam Rubio","fromEmail":"mirucam@gmail.com","sentAt":"2020-01-28T14:40:20Z","receivedAt":"2020-01-28T14:41:24Z","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":"390630","messageId":"20200128144026.53128-7-mirucam@gmail.com","threadId":"52709","inReplyTo":"20200128144026.53128-1-mirucam@gmail.com","subject":"[PATCH v2 06/11] bisect: libify `exit_if_skipped_commits` to `error_if_skipped*` and its dependents","fromName":"Miriam Rubio","fromEmail":"mirucam@gmail.com","sentAt":"2020-01-28T14:40:21Z","receivedAt":"2020-01-28T14:41:26Z","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\nHandle this return 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 | 23 ++++++++++++++++-------\n 1 file changed, 16 insertions(+), 7 deletions(-)\n\ndiff --git a/bisect.c b/bisect.c\nindex 83cb5b3a98..a7a5d158e6 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 < 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 +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":"390631","messageId":"20200128144026.53128-8-mirucam@gmail.com","threadId":"52709","inReplyTo":"20200128144026.53128-1-mirucam@gmail.com","subject":"[PATCH v2 07/11] bisect: libify `bisect_checkout`","fromName":"Miriam Rubio","fromEmail":"mirucam@gmail.com","sentAt":"2020-01-28T14:40:22Z","receivedAt":"2020-01-28T14:41:27Z","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 a7a5d158e6..dee8318d9b 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":"390632","messageId":"20200128144026.53128-9-mirucam@gmail.com","threadId":"52709","inReplyTo":"20200128144026.53128-1-mirucam@gmail.com","subject":"[PATCH v2 08/11] bisect: libify `check_merge_bases` and its dependents","fromName":"Miriam Rubio","fromEmail":"mirucam@gmail.com","sentAt":"2020-01-28T14:40:23Z","receivedAt":"2020-01-28T14:41:30Z","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\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 dee8318d9b..2a6566d066 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":"390633","messageId":"20200128144026.53128-12-mirucam@gmail.com","threadId":"52709","inReplyTo":"20200128144026.53128-1-mirucam@gmail.com","subject":"[PATCH v2 11/11] bisect: libify `bisect_next_all`","fromName":"Miriam Rubio","fromEmail":"mirucam@gmail.com","sentAt":"2020-01-28T14:40:26Z","receivedAt":"2020-01-28T14:41:31Z","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\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 bisect.h | 23 +++++++++++++++++++++++\n 2 files changed, 52 insertions(+), 11 deletions(-)\n\ndiff --git a/bisect.c b/bisect.c\nindex 43baa3df28..d4b883f67d 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 < 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\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;\ndiff --git a/bisect.h b/bisect.h\nindex 4e69a11ea8..f640c4f963 100644\n--- a/bisect.h\n+++ b/bisect.h\n@@ -31,6 +31,29 @@ struct rev_list_info {\n \tconst char *header_prefix;\n };\n \n+/*\n+ * bisect_next_all() could return the following codes:\n+ * 0 success code (from bisect_checkout()): a new commit to test\n+ * has been found (and possibly checked out too).\n+ * -1 error code (from bisect_next_all()): default error code.\n+ * -2 error code (from error_if_skipped_commits()): only skipped\n+ * commits left to be tested.\n+ * -3 error code (from handle_bad_merge_base() through\n+ * check_merge_bases() and check_good_are_ancestors_of_bad()): merge\n+ * base check failed.\n+ * -4 error code (from bisect_next_all()): no testable commit found.\n+ * -10 early success code (from bisect_next_all()): first term_bad\n+ * commit found.\n+ * This value is checked in builtin/bisect_helper::bisect_next().\n+ * -11 early success code (from check_merge_bases() through\n+ * check_good_are_ancestors_of_bad()): successfully tested\n+ * merge base.\n+ * Early success codes -10 and -11 should be only internal codes and\n+ * converted to a 0 status code when the bisecting command exits.\n+ * Different error codes might enable a bisecting script calling the\n+ * bisect command that uses this function to do different things\n+ * depending on the exit status of the bisect command.\n+ */\n int bisect_next_all(struct repository *r,\n \t\t const char *prefix,\n \t\t int no_checkout);\n-- \n2.21.1 (Apple Git-122.3)\n\n"},{"id":"390634","messageId":"20200128144026.53128-11-mirucam@gmail.com","threadId":"52709","inReplyTo":"20200128144026.53128-1-mirucam@gmail.com","subject":"[PATCH v2 10/11] bisect: libify `handle_bad_merge_base` and its dependents","fromName":"Miriam Rubio","fromEmail":"mirucam@gmail.com","sentAt":"2020-01-28T14:40:25Z","receivedAt":"2020-01-28T14:41:33Z","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\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 d519e10827..43baa3df28 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":"390635","messageId":"20200128144026.53128-10-mirucam@gmail.com","threadId":"52709","inReplyTo":"20200128144026.53128-1-mirucam@gmail.com","subject":"[PATCH v2 09/11] bisect: libify `check_good_are_ancestors_of_bad` and its dependents","fromName":"Miriam Rubio","fromEmail":"mirucam@gmail.com","sentAt":"2020-01-28T14:40:24Z","receivedAt":"2020-01-28T14:41:34Z","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\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 | 40 ++++++++++++++++++++++++++--------------\n builtin/bisect--helper.c | 12 ++++++++++--\n 2 files changed, 36 insertions(+), 16 deletions(-)\n\ndiff --git a/bisect.c b/bisect.c\nindex 2a6566d066..d519e10827 100644\n--- a/bisect.c\n+++ b/bisect.c\n@@ -865,19 +865,27 @@ 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-\tchar *filename = git_pathdup(\"BISECT_ANCESTORS_OK\");\n+\tchar *filename;\n \tstruct stat st;\n \tint fd, rev_nr, res = 0;\n \tstruct commit **rev;\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 \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@@ -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+\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..3442bfe2cb 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":"390788","messageId":"nycvar.QRO.7.76.6.2001301330070.46@tvgsbejvaqbjf.bet","threadId":"52709","inReplyTo":"20200128144026.53128-6-mirucam@gmail.com","subject":"Re: [PATCH v2 05/11] bisect--helper: introduce new `decide_next()` function","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-01-30T12:31:44Z","receivedAt":"2020-01-30T12:31:50Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Miriam,\n\nOn Tue, 28 Jan 2020, Miriam Rubio wrote:\n\n> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\n> index 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> +\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\nI know this is not something you introduced, but while you are already in\nthe neighborhood, why not fix the types of `bad_ref` and `good_glob`? The\n`xstrfmt()` function returns `char *` for a reason: so that you do not\nhave to cast it when `free()`ing the memory.\n\nThanks,\nDscho\n\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> --\n> 2.21.1 (Apple Git-122.3)\n>\n>\n"},{"id":"390789","messageId":"nycvar.QRO.7.76.6.2001301332450.46@tvgsbejvaqbjf.bet","threadId":"52709","inReplyTo":"20200128144026.53128-5-mirucam@gmail.com","subject":"Re: [PATCH v2 04/11] run-command: make `exists_in_PATH()` non-static","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-01-30T12:36:05Z","receivedAt":"2020-01-30T12:36:10Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Miriam,\n\nOn Tue, 28 Jan 2020, Miriam Rubio wrote:\n\n> From: Pranit Bauva \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 inspected the code in `exists_in_PATH()` and in `locate_in_PATH()` and\nit looks as if neither of them depended on file-local variables (which\nwould otherwise need to be addressed when exporting the function).\n\nIf you contribute another iteration of this patch series, it might make\nsense to mention this in the commit message explicitly.\n\nThanks,\nDscho\n\n>\n> Mentored by: Christian Couder \n> Mentored by: Johannes Schindelin \n> Signed-off-by: Tanushree Tumane \n> Signed-off-by: Miriam Rubio \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> + */\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> --\n> 2.21.1 (Apple Git-122.3)\n>\n>\n"},{"id":"390794","messageId":"nycvar.QRO.7.76.6.2001301341100.46@tvgsbejvaqbjf.bet","threadId":"52709","inReplyTo":"20200128144026.53128-10-mirucam@gmail.com","subject":"Re: [PATCH v2 09/11] bisect: libify `check_good_are_ancestors_of_bad` and its dependents","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-01-30T13:46:19Z","receivedAt":"2020-01-30T13:46:24Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Miriam,\n\nOn Tue, 28 Jan 2020, Miriam Rubio wrote:\n\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> +\tif (!res)\n> +\t{\n\nPlease move the opening `{` to the same line as the `if (!res)`.\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\nI wonder whether this would be easier to read:\n\n\tif (res == -11)\n\t\tres = 0;\n\telse if (res)\n\t\t; /* error out */\n\telse if ((fd = open(filename, O_CREAT | O_TRUNC | O_WRONLY, 0600)) < 0)\n\t\tres = warning_errno(_(\"could not create file '%s'\"), filename);\n\telse\n\t\tclose(fd);\n\nNote: my code explicitly assigns `res = -1` if the file could not be\ncreated, which is technically a change in behavior, but I think it is\nactually a bug fix.\n\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\nI see that there is still a `die()` here, and you left it alone in this\npatch because at this point, only the callers of\n`check_good_are_ancestors_of_bad()` need to be addressed. Good.\n\nAt a later point, this will have to be dealt with, of course.\n\nAnother thing will need to be handled, too: while I was looking at the\ncode whether any resources need to be released (returning a negative\ninteger does not release memory or close file handles, unlike `die()`), I\nstumbled across the fact that `term_bad` and `term_good` are file-local\nvariables. They will need to be made attributes of a `struct` and will\nneed to be released properly, i.e. the ownership will have to be clarified\n(is a failed `bisect_next_all()` responsible for releasing the memory it\nallocated via `read_bisect_terms()`, or its caller?).\n\nJust something to keep in mind. Or better: to jot down in a TODO list.\n\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..3442bfe2cb 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\nHmm. Is this the correct place, though? Should `bisect_next_all()` not be\nthe function that already turns `-11` into `0`? In other words, _which_\ncode are we supposed to skip over when `check_good_are_ancestors_of_bad()`\nreturns `-11`? In other words, where would the `catch` of the\n`try`/`catch` be, if we had portable exceptions in C?\n\n> +\n> +\treturn res < 0 ? -res : res;\n\nThis is a change in behavior, though: previously we guaranteed that the\nexit code is either 0 on success or 1 upon failure. I am not quite sure\nthat we want to change that behavior.\n\nCiao,\nDscho\n\n> }\n> --\n> 2.21.1 (Apple Git-122.3)\n>\n>\n"},{"id":"390795","messageId":"CAN7CjDBjJT30BrpWUnPOe-T7e3FkBohxxSP16PJvOF68quvw8A@mail.gmail.com","threadId":"52709","inReplyTo":"nycvar.QRO.7.76.6.2001301330070.46@tvgsbejvaqbjf.bet","subject":"Re: [PATCH v2 05/11] bisect--helper: introduce new `decide_next()` function","fromName":"Miriam R.","fromEmail":"mirucam@gmail.com","sentAt":"2020-01-30T14:05:06Z","receivedAt":"2020-01-30T14:05:19Z","isPatch":true,"sender":{"key":"mirucam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56339109?v=4"},"body":"Hi,\n\nEl jue., 30 ene. 2020 a las 13:31, Johannes Schindelin\n() escribió:\n>\n> Hi Miriam,\n>\n> On Tue, 28 Jan 2020, Miriam Rubio wrote:\n>\n> > diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\n> > index 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> > \"You then need to give me at least one %s and %s revision.\\n\"\n> > \"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> > - const char *current_term)\n> > +static int decide_next(const struct bisect_terms *terms,\n> > + const char *current_term, int missing_good,\n> > + int missing_bad)\n> > {\n> > - int missing_good = 1, missing_bad = 1, res = 0;\n> > - const char *bad_ref = xstrfmt(\"refs/bisect/%s\", terms->term_bad);\n> > - const char *good_glob = xstrfmt(\"%s-*\", terms->term_good);\n> > -\n> > - if (ref_exists(bad_ref))\n> > - missing_bad = 0;\n> > -\n> > - for_each_glob_ref_in(mark_good, good_glob, \"refs/bisect/\",\n> > - (void *) &missing_good);\n> > -\n> > if (!missing_good && !missing_bad)\n> > - goto finish;\n> > -\n> > - if (!current_term) {\n> > - res = -1;\n> > - goto finish;\n> > - }\n> > + return 0;\n> > + if (!current_term)\n> > + return -1;\n> > [...]\n> > +\n> > +static int bisect_next_check(const struct bisect_terms *terms,\n> > + const char *current_term)\n> > +{\n> > + int missing_good = 1, missing_bad = 1;\n> > + const char *bad_ref = xstrfmt(\"refs/bisect/%s\", terms->term_bad);\n> > + const char *good_glob = xstrfmt(\"%s-*\", terms->term_good);\n> > +\n> > + if (ref_exists(bad_ref))\n> > + missing_bad = 0;\n> > +\n> > + for_each_glob_ref_in(mark_good, good_glob, \"refs/bisect/\",\n> > + (void *) &missing_good);\n> > +\n> > free((void *) good_glob);\n> > free((void *) bad_ref);\n>\n> I know this is not something you introduced, but while you are already in\n> the neighborhood, why not fix the types of `bad_ref` and `good_glob`? The\n> `xstrfmt()` function returns `char *` for a reason: so that you do not\n> have to cast it when `free()`ing the memory.\n\nSure! I will fix this.\nThank you for reviewing.\nBest,\nMiriam\n\n>\n> Thanks,\n> Dscho\n>\n> > - return res;\n> > +\n> > + return decide_next(terms, current_term, missing_good, missing_bad);\n> > }\n> >\n> > static int get_terms(struct bisect_terms *terms)\n> > --\n> > 2.21.1 (Apple Git-122.3)\n> >\n> >\n"},{"id":"390797","messageId":"CAN7CjDCiP_KVC5Ey6cMuPN17vRNp_s4vy94qsaAKRPG=k3Rj3g@mail.gmail.com","threadId":"52709","inReplyTo":"nycvar.QRO.7.76.6.2001301341100.46@tvgsbejvaqbjf.bet","subject":"Re: [PATCH v2 09/11] bisect: libify `check_good_are_ancestors_of_bad` and its dependents","fromName":"Miriam R.","fromEmail":"mirucam@gmail.com","sentAt":"2020-01-30T14:40:27Z","receivedAt":"2020-01-30T14:40:41Z","isPatch":true,"sender":{"key":"mirucam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56339109?v=4"},"body":"Hi,\n\nEl jue., 30 ene. 2020 a las 14:46, Johannes Schindelin\n() escribió:\n>\n> Hi Miriam,\n>\n> On Tue, 28 Jan 2020, Miriam Rubio wrote:\n>\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> > + if (!res)\n> > + {\n>\n> Please move the opening `{` to the same line as the `if (!res)`.\nNoted.\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>\n> I wonder whether this would be easier to read:\n>\n> if (res == -11)\n> res = 0;\n> else if (res)\n> ; /* error out */\n> else if ((fd = open(filename, O_CREAT | O_TRUNC | O_WRONLY, 0600)) < 0)\n> res = warning_errno(_(\"could not create file '%s'\"), filename);\n> else\n> close(fd);\n>\nYes, I think it is a good improvement.\n\n> Note: my code explicitly assigns `res = -1` if the file could not be\n> created, which is technically a change in behavior, but I think it is\n> actually a bug fix.\n\nAha.\n\nIf my mentor is ok with this change, I will apply the improvement you\nsuggested :).\n\n>\n> > done:\n> > free(filename);\n> > + return res;\n> > }\n> >\n> > /*\n> > @@ -975,7 +985,9 @@ int bisect_next_all(struct repository *r, const char *prefix, int no_checkout)\n> > if (read_bisect_refs())\n> > die(_(\"reading bisect refs failed\"));\n>\n> I see that there is still a `die()` here, and you left it alone in this\n> patch because at this point, only the callers of\n> `check_good_are_ancestors_of_bad()` need to be addressed. Good.\n>\n> At a later point, this will have to be dealt with, of course.\n>\n> Another thing will need to be handled, too: while I was looking at the\n> code whether any resources need to be released (returning a negative\n> integer does not release memory or close file handles, unlike `die()`), I\n> stumbled across the fact that `term_bad` and `term_good` are file-local\n> variables. They will need to be made attributes of a `struct` and will\n> need to be released properly, i.e. the ownership will have to be clarified\n> (is a failed `bisect_next_all()` responsible for releasing the memory it\n> allocated via `read_bisect_terms()`, or its caller?).\n>\n> Just something to keep in mind. Or better: to jot down in a TODO list.\n\nOk. I will write this down for future improvements. Thank you!\n>\n> >\n> > - check_good_are_ancestors_of_bad(r, prefix, no_checkout);\n> > + res = check_good_are_ancestors_of_bad(r, prefix, no_checkout);\n> > + if (res)\n> > + return res;\n> >\n> > bisect_rev_setup(r, &revs, prefix, \"%s\", \"^%s\", 1);\n> > revs.limited = 1;\n> > diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\n> > index 826fcba2ed..3442bfe2cb 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> > switch (cmdmode) {\n> > case NEXT_ALL:\n> > - return bisect_next_all(the_repository, prefix, no_checkout);\n> > + res = bisect_next_all(the_repository, prefix, no_checkout);\n> > + break;\n> > case WRITE_TERMS:\n> > if (argc != 2)\n> > return error(_(\"--write-terms requires two arguments\"));\n> > @@ -713,5 +714,12 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n> > return error(\"BUG: unknown subcommand '%d'\", cmdmode);\n> > }\n> > free_terms(&terms);\n> > - return !!res;\n> > + /*\n> > + * Handle early success\n> > + * From check_merge_bases > check_good_are_ancestors_of_bad > bisect_next_all\n> > + */\n> > + if (res == -11)\n> > + res = 0;\n>\n> Hmm. Is this the correct place, though? Should `bisect_next_all()` not be\n> the function that already turns `-11` into `0`? In other words, _which_\n> code are we supposed to skip over when `check_good_are_ancestors_of_bad()`\n> returns `-11`? In other words, where would the `catch` of the\n> `try`/`catch` be, if we had portable exceptions in C?\n\nI think there must be a reason to do it there (but I don't know\nexactly), because there are some comments in code that say explicitly\nthat this -11 to 0 is done in cmd_bisect_helper(), when the bisecting\ncommand exits.\n\n>\n> > +\n> > + return res < 0 ? -res : res;\n>\n> This is a change in behavior, though: previously we guaranteed that the\n> exit code is either 0 on success or 1 upon failure. I am not quite sure\n> that we want to change that behavior.\n\nI think this is because different error codes might enable a bisecting\nscript calling the bisect command that uses this function to do\ndifferent things depending on the exit status of the bisect command.\n\nThank you for reviewing.\nBest,\nMiriam.\n>\n> Ciao,\n> Dscho\n>\n> > }\n> > --\n> > 2.21.1 (Apple Git-122.3)\n> >\n> >\n"},{"id":"390798","messageId":"CAN7CjDC+LaZmNUoW=MKHy=08KnCgVtxA3_CXUpNQvqo3_cPcWQ@mail.gmail.com","threadId":"52709","inReplyTo":"CAN7CjDCiG6KZU+yHGxQ26TESb1yfvc7aWh0EKhE=owSV7D-C0Q@mail.gmail.com","subject":"Fwd: [PATCH v2 04/11] run-command: make `exists_in_PATH()` non-static","fromName":"Miriam R.","fromEmail":"mirucam@gmail.com","sentAt":"2020-01-30T14:41:27Z","receivedAt":"2020-01-30T14:41:42Z","isPatch":true,"sender":{"key":"mirucam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56339109?v=4"},"body":"Hi,\n\nEl jue., 30 ene. 2020 a las 13:36, Johannes Schindelin\n() escribió:\n>\n> Hi Miriam,\n>\n> On Tue, 28 Jan 2020, Miriam Rubio wrote:\n>\n> > From: Pranit Bauva \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 inspected the code in `exists_in_PATH()` and in `locate_in_PATH()` and\n> it looks as if neither of them depended on file-local variables (which\n> would otherwise need to be addressed when exporting the function).\n>\n> If you contribute another iteration of this patch series, it might make\n> sense to mention this in the commit message explicitly.\n\nOk, I will add the comment in the commit message.\nThank you.\n\nBest,\nMiriam\n\n>\n> Thanks,\n> Dscho\n>\n> >\n> > Mentored by: Christian Couder \n> > Mentored by: Johannes Schindelin \n> > Signed-off-by: Tanushree Tumane \n> > Signed-off-by: Miriam Rubio \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> > return NULL;\n> > }\n> >\n> > -static int exists_in_PATH(const char *file)\n> > +int exists_in_PATH(const char *file)\n> > {\n> > char *r = locate_in_PATH(file);\n> > int 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> > + */\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> > --\n> > 2.21.1 (Apple Git-122.3)\n> >\n> >\n"},{"id":"390802","messageId":"nycvar.QRO.7.76.6.2001301557300.46@tvgsbejvaqbjf.bet","threadId":"52709","inReplyTo":"CAN7CjDCiP_KVC5Ey6cMuPN17vRNp_s4vy94qsaAKRPG=k3Rj3g@mail.gmail.com","subject":"Re: [PATCH v2 09/11] bisect: libify `check_good_are_ancestors_of_bad` and its dependents","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-01-30T15:01:41Z","receivedAt":"2020-01-30T15:01:44Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Miriam,\n\nOn Thu, 30 Jan 2020, Miriam R. wrote:\n\n> El jue., 30 ene. 2020 a las 14:46, Johannes Schindelin\n> () escribió:\n> >\n> > On Tue, 28 Jan 2020, Miriam Rubio wrote:\n> >\n> > > +\n> > > + return res < 0 ? -res : res;\n> >\n> > This is a change in behavior, though: previously we guaranteed that the\n> > exit code is either 0 on success or 1 upon failure. I am not quite sure\n> > that we want to change that behavior.\n>\n> I think this is because different error codes might enable a bisecting\n> script calling the bisect command that uses this function to do\n> different things depending on the exit status of the bisect command.\n\nOops. I am _totally_ wrong on this.\n\nAs you are changing a lot of `exit()` to `return -` with the\nintention to turn the value into an exit code only at the\n`cmd_bisect__helper()` level, this is actually required a change.\n\nI am a bit uneasy about this, but I could not see any return values in\n`bisect.c` other than 0 and -1, prior to this patch series.\n\nHowever, I would love to see this refactored into its own commit, more\ntoward the beginning of the patch series, with a very clean commit message\nthat describes that intention of being _the_ exit point from `bisect.c`.\n\nWithout this change, you simply cannot change the `exit();` calls in\n`bisect.c` for any `` other than 0 or 1.\n\nSorry that it took me so long to wrap my head around this rather trivial\nidea.\n\nCiao,\nDscho\n"},{"id":"390804","messageId":"nycvar.QRO.7.76.6.2001301601580.46@tvgsbejvaqbjf.bet","threadId":"52709","inReplyTo":"20200128144026.53128-1-mirucam@gmail.com","subject":"Re: [Outreachy][PATCH v2 00/11] Finish converting git bisect to C part 1","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-01-30T15:04:11Z","receivedAt":"2020-01-30T15:04:14Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Miriam,\n\nOn Tue, 28 Jan 2020, Miriam Rubio wrote:\n\n> These patches correspond to a first part of patch series\n> of Outreachy project \"Finish converting `git bisect` from shell to C\"\n> started by the interns Pranit Bauva and Tanushree Tumane\n> (https://public-inbox.org/git/pull.117.git.gitgitgadget@gmail.com) and\n> continued by Miriam Rubio.\n>\n> This first part are formed of preparatory/clean-up patches and all\n> `bisect.c` libification work.\n>\n> These patch series emails were generated from:\n> https://gitlab.com/mirucam/git/commits/git-bisect-work-part1\n>\n> It has to be noted that in this version 2 nothing has been done about a\n> reviewer suggestion of using enums for error codes, because there was no\n> consensus about using them by the reviewers.\n\nIt is a pity, as \"magic\" constants tend to remain \"magic\" (and eventually\n\"tragic\" when they have not been addressed properly).\n\nHowever, this does not need to hold up your patch series.\n\nI noted a couple of rather minor things; It looks pretty good to me\nalready, though, and I thank you very much for splitting this patch series\noff, so that only the libification remains. It made for a pleasant read.\n\nThanks,\nDscho\n\n>\n> --- Changes since v1 Finish converting git bisect to C part 1 patch series ---\n>\n> General changes\n> ---------------\n>\n> * Previous patch series version has been split in smaller groups\n> in order to facilitate revision and integration.\n> * Rebase on master branch: c7a6207591 (Sync with maint, 2020-01-27).\n> * Improve commit messages.\n>\n> Specific changes\n> ----------------\n>\n> [6/11] bisect: libify `exit_if_skipped_commits` to `error_if_skipped*`\n> and its dependents\n>\n> * Remove redundant sentences in commit message.\n> * Use `if (res < 0)` instead of `if (res)`.\n>\n> [8/11] bisect: libify `check_merge_bases` and its dependents\n>\n> * Remove redundant sentence in commit message.\n>\n> --\n>\n> [9/11] bisect: libify `check_good_are_ancestors_of_bad` and its\n> dependents\n>\n> * Remove redundant sentences in commit message.\n> * Return in `if (!current_bad_oid)` condition.\n>\n> --\n>\n> [10/11] bisect: libify `handle_bad_merge_base` and its dependents\n>\n> * Remove redundant sentence in commit message.\n>\n> --\n>\n> [11/11] bisect: libify `bisect_next_all`\n>\n> * Remove redundant sentence in commit message.\n> * Add return codes explanations in `bisect.h`.\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 (7):\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>\n> Tanushree Tumane (2):\n> bisect--helper: change `retval` to `res`\n> bisect--helper: introduce new `decide_next()` function\n>\n> bisect.c | 136 +++++++++++++++++++++++++++------------\n> bisect.h | 23 +++++++\n> builtin/bisect--helper.c | 118 +++++++++++++++++----------------\n> run-command.c | 2 +-\n> run-command.h | 11 ++++\n> 5 files changed, 193 insertions(+), 97 deletions(-)\n>\n> --\n> 2.21.1 (Apple Git-122.3)\n>\n>\n"},{"id":"390810","messageId":"CAN7CjDCrrk9gdd-6LMHTYB-A+cCndb-xdbGF4sjz7gZz-_prDw@mail.gmail.com","threadId":"52709","inReplyTo":"nycvar.QRO.7.76.6.2001301601580.46@tvgsbejvaqbjf.bet","subject":"Re: [Outreachy][PATCH v2 00/11] Finish converting git bisect to C part 1","fromName":"Miriam R.","fromEmail":"mirucam@gmail.com","sentAt":"2020-01-30T15:18:28Z","receivedAt":"2020-01-30T15:18:41Z","isPatch":true,"sender":{"key":"mirucam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56339109?v=4"},"body":"El jue., 30 ene. 2020 a las 16:04, Johannes Schindelin\n() escribió:\n>\n> Hi Miriam,\n>\n> On Tue, 28 Jan 2020, Miriam Rubio wrote:\n>\n> > These patches correspond to a first part of patch series\n> > of Outreachy project \"Finish converting `git bisect` from shell to C\"\n> > started by the interns Pranit Bauva and Tanushree Tumane\n> > (https://public-inbox.org/git/pull.117.git.gitgitgadget@gmail.com) and\n> > continued by Miriam Rubio.\n> >\n> > This first part are formed of preparatory/clean-up patches and all\n> > `bisect.c` libification work.\n> >\n> > These patch series emails were generated from:\n> > https://gitlab.com/mirucam/git/commits/git-bisect-work-part1\n> >\n> > It has to be noted that in this version 2 nothing has been done about a\n> > reviewer suggestion of using enums for error codes, because there was no\n> > consensus about using them by the reviewers.\n>\n> It is a pity, as \"magic\" constants tend to remain \"magic\" (and eventually\n> \"tragic\" when they have not been addressed properly).\n>\n> However, this does not need to hold up your patch series.\n>\n> I noted a couple of rather minor things; It looks pretty good to me\n> already, though, and I thank you very much for splitting this patch series\n> off, so that only the libification remains. It made for a pleasant read.\n>\nThank you very much for the review.\nI hope to send another patch series version soon with the new changes\nand suggestions :)\n\nBest,\nMiriam\n\n> Thanks,\n> Dscho\n>\n> >\n> > --- Changes since v1 Finish converting git bisect to C part 1 patch series ---\n> >\n> > General changes\n> > ---------------\n> >\n> > * Previous patch series version has been split in smaller groups\n> > in order to facilitate revision and integration.\n> > * Rebase on master branch: c7a6207591 (Sync with maint, 2020-01-27).\n> > * Improve commit messages.\n> >\n> > Specific changes\n> > ----------------\n> >\n> > [6/11] bisect: libify `exit_if_skipped_commits` to `error_if_skipped*`\n> > and its dependents\n> >\n> > * Remove redundant sentences in commit message.\n> > * Use `if (res < 0)` instead of `if (res)`.\n> >\n> > [8/11] bisect: libify `check_merge_bases` and its dependents\n> >\n> > * Remove redundant sentence in commit message.\n> >\n> > --\n> >\n> > [9/11] bisect: libify `check_good_are_ancestors_of_bad` and its\n> > dependents\n> >\n> > * Remove redundant sentences in commit message.\n> > * Return in `if (!current_bad_oid)` condition.\n> >\n> > --\n> >\n> > [10/11] bisect: libify `handle_bad_merge_base` and its dependents\n> >\n> > * Remove redundant sentence in commit message.\n> >\n> > --\n> >\n> > [11/11] bisect: libify `bisect_next_all`\n> >\n> > * Remove redundant sentence in commit message.\n> > * Add return codes explanations in `bisect.h`.\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 (7):\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> >\n> > Tanushree Tumane (2):\n> > bisect--helper: change `retval` to `res`\n> > bisect--helper: introduce new `decide_next()` function\n> >\n> > bisect.c | 136 +++++++++++++++++++++++++++------------\n> > bisect.h | 23 +++++++\n> > builtin/bisect--helper.c | 118 +++++++++++++++++----------------\n> > run-command.c | 2 +-\n> > run-command.h | 11 ++++\n> > 5 files changed, 193 insertions(+), 97 deletions(-)\n> >\n> > --\n> > 2.21.1 (Apple Git-122.3)\n> >\n> >\n"},{"id":"390813","messageId":"CAN7CjDBrWUFiY+mh3n0xBMQMsJ1YejRb1+N5caG+Of==uxQR8w@mail.gmail.com","threadId":"52709","inReplyTo":"nycvar.QRO.7.76.6.2001301557300.46@tvgsbejvaqbjf.bet","subject":"Re: [PATCH v2 09/11] bisect: libify `check_good_are_ancestors_of_bad` and its dependents","fromName":"Miriam R.","fromEmail":"mirucam@gmail.com","sentAt":"2020-01-30T15:26:32Z","receivedAt":"2020-01-30T15:26:45Z","isPatch":true,"sender":{"key":"mirucam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56339109?v=4"},"body":"Hi,\n\nEl jue., 30 ene. 2020 a las 16:01, Johannes Schindelin\n() escribió:\n>\n> Hi Miriam,\n>\n> On Thu, 30 Jan 2020, Miriam R. wrote:\n>\n> > El jue., 30 ene. 2020 a las 14:46, Johannes Schindelin\n> > () escribió:\n> > >\n> > > On Tue, 28 Jan 2020, Miriam Rubio wrote:\n> > >\n> > > > +\n> > > > + return res < 0 ? -res : res;\n> > >\n> > > This is a change in behavior, though: previously we guaranteed that the\n> > > exit code is either 0 on success or 1 upon failure. I am not quite sure\n> > > that we want to change that behavior.\n> >\n> > I think this is because different error codes might enable a bisecting\n> > script calling the bisect command that uses this function to do\n> > different things depending on the exit status of the bisect command.\n>\n> Oops. I am _totally_ wrong on this.\n>\n> As you are changing a lot of `exit()` to `return -` with the\n> intention to turn the value into an exit code only at the\n> `cmd_bisect__helper()` level, this is actually required a change.\n>\n> I am a bit uneasy about this, but I could not see any return values in\n> `bisect.c` other than 0 and -1, prior to this patch series.\n>\n> However, I would love to see this refactored into its own commit, more\n> toward the beginning of the patch series, with a very clean commit message\n> that describes that intention of being _the_ exit point from `bisect.c`.\n\nOk. Noted\n>\n> Without this change, you simply cannot change the `exit();` calls in\n> `bisect.c` for any `` other than 0 or 1.\n>\n> Sorry that it took me so long to wrap my head around this rather trivial\n> idea.\n\nPlease, don't worry :)\nThank you again!\n\nBest,\nMiriam.\n\n>\n> Ciao,\n> Dscho\n"},{"id":"390849","messageId":"CAP8UFD3mhirdjHnT+XRq1mPzii3O+mtAMrYCy7mf4HKQZo8Acw@mail.gmail.com","threadId":"52709","inReplyTo":"nycvar.QRO.7.76.6.2001301341100.46@tvgsbejvaqbjf.bet","subject":"Re: [PATCH v2 09/11] bisect: libify `check_good_are_ancestors_of_bad` and its dependents","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2020-01-30T21:59:41Z","receivedAt":"2020-01-30T21:59:56Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Hi Dscho,\n\nOn Thu, Jan 30, 2020 at 2:46 PM Johannes Schindelin\n wrote:\n>\n> On Tue, 28 Jan 2020, Miriam Rubio wrote:\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>\n> I wonder whether this would be easier to read:\n>\n> if (res == -11)\n> res = 0;\n> else if (res)\n> ; /* error out */\n> else if ((fd = open(filename, O_CREAT | O_TRUNC | O_WRONLY, 0600)) < 0)\n> res = warning_errno(_(\"could not create file '%s'\"), filename);\n> else\n> close(fd);\n>\n> Note: my code explicitly assigns `res = -1` if the file could not be\n> created, which is technically a change in behavior, but I think it is\n> actually a bug fix.\n\nI don't think so. I think creating the BISECT_ANCESTORS_OK file is not\nabsolutely necessary. If it doesn't exist we will just check if\nancestors are ok again at the next bisection step, which will take a\nbit of time, but which will not make us give any false result, or\nprevent us from continuing the bisection process.\n\nI think that it's also the reason why warning_errno(...) is used in\ncase we could not create the file instead of error_errno(...). We just\nwant to signal with a warning that something might be wrong because we\ncould not create the file, but not stop everything because of that.\n\nBest,\nChristian.\n"},{"id":"390887","messageId":"nycvar.QRO.7.76.6.2001311006360.46@tvgsbejvaqbjf.bet","threadId":"52709","inReplyTo":"CAP8UFD3mhirdjHnT+XRq1mPzii3O+mtAMrYCy7mf4HKQZo8Acw@mail.gmail.com","subject":"Re: [PATCH v2 09/11] bisect: libify `check_good_are_ancestors_of_bad` and its dependents","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-01-31T09:07:03Z","receivedAt":"2020-01-31T09:07:09Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Chris,\n\nOn Thu, 30 Jan 2020, Christian Couder wrote:\n\n> Hi Dscho,\n>\n> On Thu, Jan 30, 2020 at 2:46 PM Johannes Schindelin\n> wrote:\n> >\n> > On Tue, 28 Jan 2020, Miriam Rubio wrote:\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> >\n> > I wonder whether this would be easier to read:\n> >\n> > if (res == -11)\n> > res = 0;\n> > else if (res)\n> > ; /* error out */\n> > else if ((fd = open(filename, O_CREAT | O_TRUNC | O_WRONLY, 0600)) < 0)\n> > res = warning_errno(_(\"could not create file '%s'\"), filename);\n> > else\n> > close(fd);\n> >\n> > Note: my code explicitly assigns `res = -1` if the file could not be\n> > created, which is technically a change in behavior, but I think it is\n> > actually a bug fix.\n>\n> I don't think so. I think creating the BISECT_ANCESTORS_OK file is not\n> absolutely necessary. If it doesn't exist we will just check if\n> ancestors are ok again at the next bisection step, which will take a\n> bit of time, but which will not make us give any false result, or\n> prevent us from continuing the bisection process.\n>\n> I think that it's also the reason why warning_errno(...) is used in\n> case we could not create the file instead of error_errno(...). We just\n> want to signal with a warning that something might be wrong because we\n> could not create the file, but not stop everything because of that.\n\nThank you for this explanation, it makes sense to me.\n\nMaybe a code comment would be in order?\n\nCiao,\nDscho\n"},{"id":"390888","messageId":"CAP8UFD2Crc2k7vdqxAaAvyLUqz5VdSfL6cfY110LHE2RpPRBKQ@mail.gmail.com","threadId":"52709","inReplyTo":"nycvar.QRO.7.76.6.2001311006360.46@tvgsbejvaqbjf.bet","subject":"Re: [PATCH v2 09/11] bisect: libify `check_good_are_ancestors_of_bad` and its dependents","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2020-01-31T09:15:25Z","receivedAt":"2020-01-31T09:16:14Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Hi Dscho,\n\nOn Fri, Jan 31, 2020 at 10:07 AM Johannes Schindelin\n wrote:\n>\n> On Thu, 30 Jan 2020, Christian Couder wrote:\n>\n> > On Thu, Jan 30, 2020 at 2:46 PM Johannes Schindelin\n> > wrote:\n> > >\n> > > On Tue, 28 Jan 2020, Miriam Rubio wrote:\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> > >\n> > > I wonder whether this would be easier to read:\n> > >\n> > > if (res == -11)\n> > > res = 0;\n> > > else if (res)\n> > > ; /* error out */\n> > > else if ((fd = open(filename, O_CREAT | O_TRUNC | O_WRONLY, 0600)) < 0)\n> > > res = warning_errno(_(\"could not create file '%s'\"), filename);\n> > > else\n> > > close(fd);\n> > >\n> > > Note: my code explicitly assigns `res = -1` if the file could not be\n> > > created, which is technically a change in behavior, but I think it is\n> > > actually a bug fix.\n> >\n> > I don't think so. I think creating the BISECT_ANCESTORS_OK file is not\n> > absolutely necessary. If it doesn't exist we will just check if\n> > ancestors are ok again at the next bisection step, which will take a\n> > bit of time, but which will not make us give any false result, or\n> > prevent us from continuing the bisection process.\n> >\n> > I think that it's also the reason why warning_errno(...) is used in\n> > case we could not create the file instead of error_errno(...). We just\n> > want to signal with a warning that something might be wrong because we\n> > could not create the file, but not stop everything because of that.\n>\n> Thank you for this explanation, it makes sense to me.\n>\n> Maybe a code comment would be in order?\n\nYeah, I agree it would help.\n\nThanks for your review,\nChristian.\n"},{"id":"390895","messageId":"CAN7CjDD2rDKT6Ngg7Mfo4oDB5vxCGqhx-w5aGfC-f_thNMBKmQ@mail.gmail.com","threadId":"52709","inReplyTo":"CAN7CjDC7ijMDtJdShRB+P0d0GRYYrQXktdH2Og9XGDqJ-OZxzw@mail.gmail.com","subject":"Fwd: [PATCH v2 09/11] bisect: libify `check_good_are_ancestors_of_bad` and its dependents","fromName":"Miriam R.","fromEmail":"mirucam@gmail.com","sentAt":"2020-01-31T10:21:10Z","receivedAt":"2020-01-31T10:21:24Z","isPatch":true,"sender":{"key":"mirucam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56339109?v=4"},"body":"Hi,\n\nEl vie., 31 ene. 2020 a las 10:15, Christian Couder\n() escribió:\n>\n> Hi Dscho,\n>\n> On Fri, Jan 31, 2020 at 10:07 AM Johannes Schindelin\n> wrote:\n> >\n> > On Thu, 30 Jan 2020, Christian Couder wrote:\n> >\n> > > On Thu, Jan 30, 2020 at 2:46 PM Johannes Schindelin\n> > > wrote:\n> > > >\n> > > > On Tue, 28 Jan 2020, Miriam Rubio wrote:\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> > > >\n> > > > I wonder whether this would be easier to read:\n> > > >\n> > > > if (res == -11)\n> > > > res = 0;\n> > > > else if (res)\n> > > > ; /* error out */\n> > > > else if ((fd = open(filename, O_CREAT | O_TRUNC | O_WRONLY, 0600)) < 0)\n> > > > res = warning_errno(_(\"could not create file '%s'\"), filename);\n> > > > else\n> > > > close(fd);\n> > > >\n> > > > Note: my code explicitly assigns `res = -1` if the file could not be\n> > > > created, which is technically a change in behavior, but I think it is\n> > > > actually a bug fix.\n> > >\n> > > I don't think so. I think creating the BISECT_ANCESTORS_OK file is not\n> > > absolutely necessary. If it doesn't exist we will just check if\n> > > ancestors are ok again at the next bisection step, which will take a\n> > > bit of time, but which will not make us give any false result, or\n> > > prevent us from continuing the bisection process.\n> > >\n> > > I think that it's also the reason why warning_errno(...) is used in\n> > > case we could not create the file instead of error_errno(...). We just\n> > > want to signal with a warning that something might be wrong because we\n> > > could not create the file, but not stop everything because of that.\n> >\n> > Thank you for this explanation, it makes sense to me.\n> >\n> > Maybe a code comment would be in order?\n>\n> Yeah, I agree it would help.\n>\nNoted.\nThank you both for the review!\n\nBest,\nMiriam\n> Thanks for your review,\n> Christian.\n"},{"id":"390922","messageId":"xmqq5zgrbi5e.fsf@gitster-ct.c.googlers.com","threadId":"52709","inReplyTo":"20200128144026.53128-7-mirucam@gmail.com","subject":"Re: [PATCH v2 06/11] bisect: libify `exit_if_skipped_commits` to `error_if_skipped*` and its dependents","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-01-31T18:22:21Z","receivedAt":"2020-01-31T18:22:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Miriam Rubio writes:\n\n> From: Pranit Bauva \n>\n> Since we want to get rid of git-bisect.sh it would be necessary to\n\nPlease add back the missing comma after \".sh\" for readability.\n\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> Handle this return in dependent function `bisect_next_all()`.\n\nIt makes it sound as if there were multiple callers and this change\nconverted only one of them---\"Update all callers to handle the error\nreturns\" would avoid such a misunderstanding.\n\n> diff --git a/bisect.c b/bisect.c\n> index 83cb5b3a98..a7a5d158e6 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\nWhat this comment says may be true, but does it belong here? After\nreturning, the caller will exit(2) without cleaning anyway with or\nwithout this patch, so I am a bit puzzled about the comment.\n\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 < 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 +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"},{"id":"390930","messageId":"xmqq1rrfbhpu.fsf@gitster-ct.c.googlers.com","threadId":"52709","inReplyTo":"20200128144026.53128-8-mirucam@gmail.com","subject":"Re: [PATCH v2 07/11] bisect: libify `bisect_checkout`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-01-31T18:31:41Z","receivedAt":"2020-01-31T18:31:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Miriam Rubio writes:\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_checkout()`.\n> Changes related to return values have no bad side effects on the\n> code that calls `bisect_checkout()`.\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 | 7 ++++---\n> 1 file changed, 4 insertions(+), 3 deletions(-)\n>\n> diff --git a/bisect.c b/bisect.c\n> index a7a5d158e6..dee8318d9b 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\nWrong placement of a new decl. Have a block of decls and then have\na blank line before the first statement, i.e.\n\n\t\tchar bisect_rev_hex[...];\n\t+\tint res = 0;\n\n\t\tmemcpy(...);\n\nThis comment probably applies to other hunks in the entire series.\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\nHmph. \n\nThis means that res == -1 and res == 1 from run_command_v_opt()\ncannot be distinguished by our callers. Is that what we want here?\n\nIf that is really what we want, it probably is easier to read if\nthis were written like so:\n\n\t\treturn -abs(res);\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\nLikewise.\n\n"},{"id":"390931","messageId":"xmqqwo97a2rj.fsf@gitster-ct.c.googlers.com","threadId":"52709","inReplyTo":"20200128144026.53128-9-mirucam@gmail.com","subject":"Re: [PATCH v2 08/11] bisect: libify `check_merge_bases` and its dependents","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-01-31T18:40:00Z","receivedAt":"2020-01-31T18:40:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Miriam Rubio writes:\n\n> + * - If a merge base must be tested, on success return -11 a special condition\n\nIs this eleven something the original (scripted version) used, or\nwhat this series invented? If the latter, what was the reason the\nvalue was chosen, and what were the constraints when choosing the\nvalue (e.g. it cannot be used an exit code by something)?\n\nIn any case, these values (if there other special codes defined)\nshould be given symbolic names (either #define or enum).\n"}]}