{"thread":{"id":"52890","subject":"[PATCH 01/10] bisect--helper: introduce new `write_in_file()` function","startedAt":"2020-02-26T10:16:11Z","lastAt":"2020-03-11T18:58:56Z","messageCount":20,"participants":["Miriam Rubio","Junio C Hamano","Miriam R.","Johannes Schindelin","Christian Couder"],"isPatch":true,"patchVersion":1,"patchTotal":10},"messages":[{"id":"392523","messageId":"20200226101429.81327-2-mirucam@gmail.com","threadId":"52890","inReplyTo":"20200226101429.81327-1-mirucam@gmail.com","subject":"[PATCH 01/10] bisect--helper: introduce new `write_in_file()` function","fromName":"Miriam Rubio","fromEmail":"mirucam@gmail.com","sentAt":"2020-02-26T10:14:20Z","receivedAt":"2020-02-26T10:16:11Z","isPatch":true,"sender":{"key":"mirucam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56339109?v=4"},"body":"Let's refactor code adding a new `write_in_file()` function\nthat opens a file for writing a message and closes it.\n\nThis removes some duplicated code and makes the code simpler,\nclearer and easier to understand.\n\nMentored-by: Christian Couder \nMentored-by: Johannes Schindelin \nSigned-off-by: Miriam Rubio \n---\n builtin/bisect--helper.c | 22 ++++++++++++++++------\n 1 file changed, 16 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\nindex c1c40b516d..ee1be630da 100644\n--- a/builtin/bisect--helper.c\n+++ b/builtin/bisect--helper.c\n@@ -74,6 +74,19 @@ static int one_of(const char *term, ...)\n \treturn res;\n }\n \n+static int write_in_file(const char *filepath, const char *content, int append)\n+{\n+\tFILE *fp = NULL;\n+\tconst char *mode = append ? \"a\" : \"w\";\n+\n+\tfp = fopen(filepath, mode);\n+\tif (!fp)\n+\t\treturn error_errno(_(\"could not open the file '%s'\"), filepath);\n+\tif (!fprintf(fp, \"%s\\n\", content))\n+\t\treturn error_errno(_(\"could not write in file '%s'\"), filepath);\n+\treturn fclose(fp);\n+}\n+\n static int check_term_format(const char *term, const char *orig_term)\n {\n \tint res;\n@@ -104,7 +117,7 @@ static int check_term_format(const char *term, const char *orig_term)\n \n static int write_terms(const char *bad, const char *good)\n {\n-\tFILE *fp = NULL;\n+\tchar *content = xstrfmt(\"%s\\n%s\", bad, good);\n \tint res;\n \n \tif (!strcmp(bad, good))\n@@ -113,12 +126,9 @@ static int write_terms(const char *bad, const char *good)\n \tif (check_term_format(bad, \"bad\") || check_term_format(good, \"good\"))\n \t\treturn -1;\n \n-\tfp = fopen(git_path_bisect_terms(), \"w\");\n-\tif (!fp)\n-\t\treturn error_errno(_(\"could not open the file BISECT_TERMS\"));\n+\tres = write_in_file(git_path_bisect_terms(), content, 0);\n+\tfree(content);\n \n-\tres = fprintf(fp, \"%s\\n%s\\n\", bad, good);\n-\tres |= fclose(fp);\n \treturn (res < 0) ? -1 : 0;\n }\n \n-- \n2.25.0\n\n"},{"id":"392524","messageId":"20200226101429.81327-1-mirucam@gmail.com","threadId":"52890","inReplyTo":null,"subject":"[Outreachy][PATCH 00/10] Finish converting git bisect to C part 2","fromName":"Miriam Rubio","fromEmail":"mirucam@gmail.com","sentAt":"2020-02-26T10:14:19Z","receivedAt":"2020-02-26T10:16:12Z","isPatch":true,"sender":{"key":"mirucam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56339109?v=4"},"body":"These patches correspond to a second part of patch series \nof Outreachy project \"Finish converting `git bisect` from shell to C\" \nstarted by Pranit Bauva and Tanushree Tumane\n(https://public-inbox.org/git/pull.117.git.gitgitgadget@gmail.com) and\ncontinued by me.\n\nThis second part depends on mr/bisect-in-c-1 and it is formed by \nreimplementations of some `git bisect` subcommands and removal of some\ntemporary subcommands.\n\nThese patch series emails were generated from:\nhttps://gitlab.com/mirucam/git/commits/git-bisect-work-part2.\n\nGeneral changes\n---------------\n\n* Rebase on next branch: 5900a2a8f9 (Sync with master, 2020-02-25).\n* Improve commit messages.\n* Amend patch series titles.\n* Reorder commits.\n\nSpecific changes\n----------------\n\n[1/10] bisect--helper: introduce new `write_in_file()` function\n\n* New patch to refactor code in `write_in_file()` function and use it in\n`write_terms()`.\n\n--\n\n[2/10] bisect--helper: reimplement `bisect_next` and `bisect_auto_next` shell functions in C\n\n* Fix `repo_init_revisions()` and bisect_next_all` calls after \nrebase on master.\n* Add blank line.\n* Remove goto statements in `bisect_skipped_commits()`.\n* Add `clear_commit_marks()` in `process_skipped_commits()`.\n* Use `write_in_file()` in `bisect_successful()`.\n* Change header return type and internal returns to enum in \n`bisect_next()` and `bisect_auto_next()`.\n* Improve code comments.\n\n--\n\n[3/10] bisect--helper: finish porting `bisect_start()` to C\n\n* Change header return type and internal returns to enum in `bisect_start()`.\n* Improve code comments. \n\n--\n\n[6/10] bisect--helper: reimplement `bisect_autostart` shell function in C\n\n* Change an internal return to enum in `bisect_autostart()`.\n\n--\n\n[7/10] bisect--helper: reimplement `bisect_state` & `bisect_head` shell functions in C\n\n* Change header return type and internal returns to enum in `bisect_state()`.\n\n--\n\nMiriam Rubio (1):\n bisect--helper: introduce new `write_in_file()` function\n\nPranit Bauva (9):\n bisect--helper: reimplement `bisect_next` and `bisect_auto_next` shell\n functions in C\n bisect--helper: finish porting `bisect_start()` to C\n bisect--helper: retire `--bisect-clean-state` subcommand\n bisect--helper: retire `--next-all` subcommand\n bisect--helper: reimplement `bisect_autostart` shell function in C\n bisect--helper: reimplement `bisect_state` & `bisect_head` shell\n functions in C\n bisect--helper: retire `--check-expected-revs` subcommand\n bisect--helper: retire `--write-terms` subcommand\n bisect--helper: retire `--bisect-autostart` subcommand\n\n bisect.c | 11 ++\n builtin/bisect--helper.c | 388 ++++++++++++++++++++++++++++++++++-----\n git-bisect.sh | 145 +--------------\n 3 files changed, 358 insertions(+), 186 deletions(-)\n\n-- \n2.25.0\n\n"},{"id":"392525","messageId":"20200226101429.81327-4-mirucam@gmail.com","threadId":"52890","inReplyTo":"20200226101429.81327-1-mirucam@gmail.com","subject":"[PATCH 03/10] bisect--helper: finish porting `bisect_start()` to C","fromName":"Miriam Rubio","fromEmail":"mirucam@gmail.com","sentAt":"2020-02-26T10:14:22Z","receivedAt":"2020-02-26T10:16:14Z","isPatch":true,"sender":{"key":"mirucam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56339109?v=4"},"body":"From: Pranit Bauva \n\nAdd the subcommand to `git bisect--helper` and call it from\ngit-bisect.sh.\n\nWith the conversion of `bisect_auto_next()` from shell to C in a\nprevious commit, `bisect_start()` can now be fully ported to C.\n\nSo let's complete the `--bisect-start` subcommand of\n`git bisect--helper` so that it fully implements `bisect_start()`,\nand let's use this subcommand in `git-bisect.sh` instead of\n`bisect_start()`.\n\nThis removes the signal handling we had in `bisect_start()` as we\ndon't really need it. While at it, \"trap\" is changed to \"handle\".\nAs \"trap\" is a reference to the shell \"trap\" builtin, which isn't\nused anymore.\n\nMentored-by: Christian Couder \nMentored-by: Johannes Schindelin \nSigned-off-by: Pranit Bauva \nSigned-off-by: Tanushree Tumane \nSigned-off-by: Miriam Rubio \n---\n builtin/bisect--helper.c | 54 +++++++++++++++++++++++++++++++---------\n git-bisect.sh | 28 +++------------------\n 2 files changed, 45 insertions(+), 37 deletions(-)\n\ndiff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\nindex c82ffe9c1c..e75e7a0837 100644\n--- a/builtin/bisect--helper.c\n+++ b/builtin/bisect--helper.c\n@@ -583,11 +583,12 @@ static enum bisect_error bisect_auto_next(struct bisect_terms *terms, const char\n \treturn bisect_next(terms, prefix);\n }\n \n-static int bisect_start(struct bisect_terms *terms, int no_checkout,\n+static enum bisect_error bisect_start(struct bisect_terms *terms, int no_checkout,\n \t\t\tconst char **argv, int argc)\n {\n \tint i, has_double_dash = 0, must_write_terms = 0, bad_seen = 0;\n-\tint flags, pathspec_pos, res = 0;\n+\tint flags, pathspec_pos;\n+\tenum bisect_error res = BISECT_OK;\n \tstruct string_list revs = STRING_LIST_INIT_DUP;\n \tstruct string_list states = STRING_LIST_INIT_DUP;\n \tstruct strbuf start_head = STRBUF_INIT;\n@@ -640,9 +641,12 @@ static int bisect_start(struct bisect_terms *terms, int no_checkout,\n \t\t\treturn error(_(\"unrecognized option: '%s'\"), arg);\n \t\t} else {\n \t\t\tchar *commit_id = xstrfmt(\"%s^{commit}\", arg);\n-\t\t\tif (get_oid(commit_id, &oid) && has_double_dash)\n-\t\t\t\tdie(_(\"'%s' does not appear to be a valid \"\n-\t\t\t\t \"revision\"), arg);\n+\t\t\tif (get_oid(commit_id, &oid) && has_double_dash) {\n+\t\t\t\terror(_(\"'%s' does not appear to be a valid \"\n+\t\t\t\t\t\"revision\"), arg);\n+\t\t\t\tfree(commit_id);\n+\t\t\t\treturn BISECT_FAILED;\n+\t\t\t}\n \n \t\t\tstring_list_append(&revs, oid_to_hex(&oid));\n \t\t\tfree(commit_id);\n@@ -720,12 +724,12 @@ static int bisect_start(struct bisect_terms *terms, int no_checkout,\n \t * Get rid of any old bisect state.\n \t */\n \tif (bisect_clean_state())\n-\t\treturn -1;\n+\t\treturn BISECT_FAILED;\n \n \t/*\n-\t * In case of mistaken revs or checkout error, or signals received,\n+\t * In case of mistaken revs or checkout error,\n \t * \"bisect_auto_next\" below may exit or misbehave.\n-\t * We have to trap this to be able to clean up using\n+\t * We have to handle this to be able to clean up using\n \t * \"bisect_clean_state\".\n \t */\n \n@@ -741,7 +745,7 @@ static int bisect_start(struct bisect_terms *terms, int no_checkout,\n \t\t}\n \t\tif (update_ref(NULL, \"BISECT_HEAD\", &oid, NULL, 0,\n \t\t\t UPDATE_REFS_MSG_ON_ERR)) {\n-\t\t\tres = -1;\n+\t\t\tres = BISECT_FAILED;\n \t\t\tgoto finish;\n \t\t}\n \t}\n@@ -753,25 +757,51 @@ static int bisect_start(struct bisect_terms *terms, int no_checkout,\n \tfor (i = 0; i < states.nr; i++)\n \t\tif (bisect_write(states.items[i].string,\n \t\t\t\t revs.items[i].string, terms, 1)) {\n-\t\t\tres = -1;\n+\t\t\tres = BISECT_FAILED;\n \t\t\tgoto finish;\n \t\t}\n \n \tif (must_write_terms && write_terms(terms->term_bad,\n \t\t\t\t\t terms->term_good)) {\n-\t\tres = -1;\n+\t\tres = BISECT_FAILED;\n \t\tgoto finish;\n \t}\n \n \tres = bisect_append_log_quoted(argv);\n \tif (res)\n-\t\tres = -1;\n+\t\tres = BISECT_FAILED;\n \n finish:\n \tstring_list_clear(&revs, 0);\n \tstring_list_clear(&states, 0);\n \tstrbuf_release(&start_head);\n \tstrbuf_release(&bisect_names);\n+\tif (res)\n+\t\treturn res;\n+\n+\tres = bisect_auto_next(terms, NULL);\n+\t/*\n+\t * In case of mistaken revs or checkout error, or signals received,\n+\t * \"bisect_auto_next\" below may exit or misbehave.\n+\t * We have to handle this to be able to clean up using\n+\t * \"bisect_clean_state\".\n+\t * return code BISECT_INTERNAL_SUCCESS_MERGE_BASE is special code\n+\t * that indicates special success.\n+\t *\t-> bisect_start()\n+\t *\t . res = bisect_auto_next()\n+\t *\t -> bisect_auto_next()\n+\t *\t . return bisect_next()\n+\t *\t -> bisect_next()\n+\t *\t\t . res = bisect_next_all()\n+\t *\t\t -> bisect_next_all()\n+\t *\t\t . res = check_good_are_ancestors_of_bad()\n+\t *\t\t -> check_good_are_ancestors_of_bad()\n+\t *\t\t\t. res = check_merge_bases()\n+\t *\t\t\t-> check_merge_bases()\n+\t *\t\t\t . res = BISECT_INTERNAL_SUCCESS_MERGE_BASE\n+\t */\n+\tif (res && res != BISECT_INTERNAL_SUCCESS_MERGE_BASE)\n+\t\tbisect_clean_state();\n \treturn res;\n }\n \ndiff --git a/git-bisect.sh b/git-bisect.sh\nindex e03f210d55..166f6a64dd 100755\n--- a/git-bisect.sh\n+++ b/git-bisect.sh\n@@ -63,34 +63,13 @@ bisect_autostart() {\n \t\t\t[Nn]*)\n \t\t\t\texit ;;\n \t\t\tesac\n-\t\t\tbisect_start\n+\t\t\tgit bisect--helper --bisect-start\n \t\telse\n \t\t\texit 1\n \t\tfi\n \t}\n }\n \n-bisect_start() {\n-\tgit bisect--helper --bisect-start $@ || exit\n-\n-\t#\n-\t# Change state.\n-\t# In case of mistaken revs or checkout error, or signals received,\n-\t# \"bisect_auto_next\" below may exit or misbehave.\n-\t# We have to trap this to be able to clean up using\n-\t# \"bisect_clean_state\".\n-\t#\n-\ttrap 'git bisect--helper --bisect-clean-state' 0\n-\ttrap 'exit 255' 1 2 3 15\n-\n-\t#\n-\t# Check if we can proceed to the next bisect state.\n-\t#\n-\tgit bisect--helper --bisect-auto-next || exit\n-\n-\ttrap '-' 0\n-}\n-\n bisect_skip() {\n \tall=''\n \tfor arg in \"$@\"\n@@ -183,8 +162,7 @@ bisect_replay () {\n \t\tget_terms\n \t\tcase \"$command\" in\n \t\tstart)\n-\t\t\tcmd=\"bisect_start $rev\"\n-\t\t\teval \"$cmd\" ;;\n+\t\t\teval \"git bisect--helper --bisect-start $rev\" ;;\n \t\t\"$TERM_GOOD\"|\"$TERM_BAD\"|skip)\n \t\t\tgit bisect--helper --bisect-write \"$command\" \"$rev\" \"$TERM_GOOD\" \"$TERM_BAD\" || exit;;\n \t\tterms)\n@@ -283,7 +261,7 @@ case \"$#\" in\n \thelp)\n \t\tgit bisect -h ;;\n \tstart)\n-\t\tbisect_start \"$@\" ;;\n+\t\tgit bisect--helper --bisect-start \"$@\" ;;\n \tbad|good|new|old|\"$TERM_BAD\"|\"$TERM_GOOD\")\n \t\tbisect_state \"$cmd\" \"$@\" ;;\n \tskip)\n-- \n2.25.0\n\n"},{"id":"392526","messageId":"20200226101429.81327-3-mirucam@gmail.com","threadId":"52890","inReplyTo":"20200226101429.81327-1-mirucam@gmail.com","subject":"[PATCH 02/10] bisect--helper: reimplement `bisect_next` and `bisect_auto_next` shell functions in C","fromName":"Miriam Rubio","fromEmail":"mirucam@gmail.com","sentAt":"2020-02-26T10:14:21Z","receivedAt":"2020-02-26T10:16:14Z","isPatch":true,"sender":{"key":"mirucam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56339109?v=4"},"body":"From: Pranit Bauva \n\nReimplement the `bisect_next()` and the `bisect_auto_next()` shell functions\nin C and add the subcommands to `git bisect--helper` to call them from\ngit-bisect.sh .\n\nUsing `--bisect-next` and `--bisect-auto-start` subcommands is a\ntemporary measure to port shell function to C so as to use the existing\ntest suite. As more functions are ported, `--bisect-auto-start`\nsubcommand will be retired and will be called by some other methods.\n\nMentored-by: Lars Schneider \nMentored-by: Christian Couder \nMentored-by: Johannes Schindelin \nSigned-off-by: Pranit Bauva \nSigned-off-by: Tanushree Tumane \nSigned-off-by: Miriam Rubio \n---\n bisect.c | 11 +++\n builtin/bisect--helper.c | 172 ++++++++++++++++++++++++++++++++++++++-\n git-bisect.sh | 47 +----------\n 3 files changed, 186 insertions(+), 44 deletions(-)\n\ndiff --git a/bisect.c b/bisect.c\nindex 9154f810f7..a50278ea7e 100644\n--- a/bisect.c\n+++ b/bisect.c\n@@ -635,6 +635,11 @@ static void bisect_rev_setup(struct repository *r, struct rev_info *revs,\n \tstruct argv_array rev_argv = ARGV_ARRAY_INIT;\n \tint i;\n \n+\t/*\n+\t * `revs` could has been used before.\n+\t * Thus we first need to reset it.\n+\t */\n+\treset_revision_walk();\n \trepo_init_revisions(r, revs, prefix);\n \trevs->abbrev = 0;\n \trevs->commit_format = CMIT_FMT_UNSPECIFIED;\n@@ -980,6 +985,12 @@ void read_bisect_terms(const char **read_bad, const char **read_good)\n * the bisection process finished successfully.\n * In this case the calling function or command should not turn a\n * BISECT_INTERNAL_SUCCESS_1ST_BAD_FOUND return code into an error or a non zero exit code.\n+ *\n+ * Checking BISECT_INTERNAL_SUCCESS_1ST_BAD_FOUND\n+ * in bisect_helper::bisect_next() and only transforming it to 0 at\n+ * the end of bisect_helper::cmd_bisect__helper() helps bypassing\n+ * all the code related to finding a commit to test.\n+ *\n * If no_checkout is non-zero, the bisection process does not\n * checkout the trial commit but instead simply updates BISECT_HEAD.\n */\ndiff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\nindex ee1be630da..c82ffe9c1c 100644\n--- a/builtin/bisect--helper.c\n+++ b/builtin/bisect--helper.c\n@@ -8,6 +8,7 @@\n #include \"run-command.h\"\n #include \"prompt.h\"\n #include \"quote.h\"\n+#include \"revision.h\"\n \n static GIT_PATH_FUNC(git_path_bisect_terms, \"BISECT_TERMS\")\n static GIT_PATH_FUNC(git_path_bisect_expected_rev, \"BISECT_EXPECTED_REV\")\n@@ -29,6 +30,8 @@ static const char * const git_bisect_helper_usage[] = {\n \tN_(\"git bisect--helper --bisect-terms [--term-good | --term-old | --term-bad | --term-new]\"),\n \tN_(\"git bisect--helper --bisect-start [--term-{old,good}= --term-{new,bad}=]\"\n \t\t\t\t\t \"[--no-checkout] [ [...]] [--] [...]\"),\n+\tN_(\"git bisect--helper --bisect-next\"),\n+\tN_(\"git bisect--helper --bisect-auto-next\"),\n \tNULL\n };\n \n@@ -431,6 +434,155 @@ static int bisect_append_log_quoted(const char **argv)\n \treturn res;\n }\n \n+static int register_good_ref(const char *refname,\n+\t\t\t const struct object_id *oid, int flags,\n+\t\t\t void *cb_data)\n+{\n+\tstruct string_list *good_refs = cb_data;\n+\n+\tstring_list_append(good_refs, oid_to_hex(oid));\n+\treturn 0;\n+}\n+\n+static void prepare_rev_argv(struct bisect_terms *terms, struct argv_array *rev_argv)\n+{\n+\tstruct string_list good_revs = STRING_LIST_INIT_DUP;\n+\tchar *term_good = xstrfmt(\"%s-*\", terms->term_good);\n+\n+\tfor_each_glob_ref_in(register_good_ref, term_good,\n+\t\t\t \"refs/bisect/\", &good_revs);\n+\n+\targv_array_pushl(rev_argv, \"skipped_commits\", \"refs/bisect/bad\", \"--not\", NULL);\n+\tfor (int i = 0; i < good_revs.nr; i++)\n+\t\targv_array_push(rev_argv, good_revs.items[i].string);\n+\n+\tstring_list_clear(&good_revs, 0);\n+\tfree(term_good);\n+}\n+\n+static int prepare_revs(struct bisect_terms *terms, struct rev_info *revs)\n+{\n+\tint res = 0;\n+\tstruct argv_array rev_argv = ARGV_ARRAY_INIT;\n+\n+\tprepare_rev_argv(terms, &rev_argv);\n+\n+\t/*\n+\t * It is important to reset the flags used by revision walks\n+\t * as the previous call to bisect_next_all() in turn\n+\t * setups a revision walk.\n+\t */\n+\treset_revision_walk();\n+\tinit_revisions(revs, NULL);\n+\trev_argv.argc = setup_revisions(rev_argv.argc, rev_argv.argv, revs, NULL);\n+\tif (prepare_revision_walk(revs))\n+\t\tres = error(_(\"revision walk setup failed\\n\"));\n+\n+\targv_array_clear(&rev_argv);\n+\treturn res;\n+}\n+\n+static int process_skipped_commits(FILE *fp, struct bisect_terms *terms, struct rev_info *revs)\n+{\n+\tstruct commit *commit;\n+\tstruct pretty_print_context pp = {0};\n+\n+\tif (fprintf(fp, \"# only skipped commits left to test\\n\") < 1)\n+\t\treturn -1;\n+\n+\twhile ((commit = get_revision(revs)) != NULL) {\n+\t\tstruct strbuf commit_name = STRBUF_INIT;\n+\t\tformat_commit_message(commit, \"%s\",\n+\t\t\t\t &commit_name, &pp);\n+\t\tfprintf(fp, \"# possible first %s commit: [%s] %s\\n\",\n+\t\t\tterms->term_bad, oid_to_hex(&commit->object.oid),\n+\t\t\tcommit_name.buf);\n+\t\tstrbuf_release(&commit_name);\n+\t\tclear_commit_marks(commit, ALL_REV_FLAGS);\n+\t}\n+\n+\treturn 0;\n+}\n+\n+static int bisect_skipped_commits(struct bisect_terms *terms)\n+{\n+\tint res = 0;\n+\tFILE *fp = NULL;\n+\tstruct rev_info revs;\n+\n+\tfp = fopen(git_path_bisect_log(), \"a\");\n+\tif (!fp)\n+\t\treturn error_errno(_(\"could not open '%s' for appending\"),\n+\t\t\t\t git_path_bisect_log());\n+\n+\tres = prepare_revs(terms, &revs);\n+\n+\tif (!res)\n+\t\tres = process_skipped_commits(fp, terms, &revs);\n+\n+\tfclose(fp);\n+\treturn res;\n+}\n+\n+static int bisect_successful(struct bisect_terms *terms)\n+{\n+\tstruct object_id oid;\n+\tstruct commit *commit;\n+\tstruct pretty_print_context pp = {0};\n+\tstruct strbuf commit_name = STRBUF_INIT;\n+\tchar *bad_ref = xstrfmt(\"refs/bisect/%s\",\n+\t\t\t\tterms->term_bad);\n+\tchar *content;\n+\tint res;\n+\n+\tread_ref(bad_ref, &oid);\n+\tprintf(\"%s\\n\", bad_ref);\n+\tcommit = lookup_commit_reference(the_repository, &oid);\n+\tformat_commit_message(commit, \"%s\", &commit_name, &pp);\n+\n+\tcontent = xstrfmt(\"# first %s commit: [%s] %s\",\n+\tterms->term_bad, oid_to_hex(&oid),\n+\tcommit_name.buf);\n+\n+\tres = write_in_file(git_path_bisect_log(), content, 1);\n+\n+\tstrbuf_release(&commit_name);\n+\tfree(bad_ref);\n+\tfree(content);\n+\treturn res;\n+}\n+\n+static enum bisect_error bisect_next(struct bisect_terms *terms, const char *prefix)\n+{\n+\tint no_checkout;\n+\tenum bisect_error res;\n+\n+\tif (bisect_next_check(terms, terms->term_good))\n+\t\treturn BISECT_FAILED;\n+\n+\tno_checkout = !is_empty_or_missing_file(git_path_bisect_head());\n+\n+\t/* Perform all bisection computation, display and checkout */\n+\tres = bisect_next_all(the_repository, prefix, no_checkout);\n+\n+\tif (res == BISECT_INTERNAL_SUCCESS_1ST_BAD_FOUND) {\n+\t\tres = bisect_successful(terms);\n+\t\treturn res ? res : BISECT_INTERNAL_SUCCESS_MERGE_BASE;\n+\t} else if (res == BISECT_ONLY_SKIPPED_LEFT) {\n+\t\tres = bisect_skipped_commits(terms);\n+\t\treturn res ? res : BISECT_ONLY_SKIPPED_LEFT;\n+\t}\n+\treturn res;\n+}\n+\n+static enum bisect_error bisect_auto_next(struct bisect_terms *terms, const char *prefix)\n+{\n+\tif (bisect_next_check(terms, NULL))\n+\t\treturn BISECT_OK;\n+\n+\treturn bisect_next(terms, prefix);\n+}\n+\n static int bisect_start(struct bisect_terms *terms, int no_checkout,\n \t\t\tconst char **argv, int argc)\n {\n@@ -635,7 +787,9 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n \t\tCHECK_AND_SET_TERMS,\n \t\tBISECT_NEXT_CHECK,\n \t\tBISECT_TERMS,\n-\t\tBISECT_START\n+\t\tBISECT_START,\n+\t\tBISECT_NEXT,\n+\t\tBISECT_AUTO_NEXT,\n \t} cmdmode = 0;\n \tint no_checkout = 0, res = 0, nolog = 0;\n \tstruct option options[] = {\n@@ -659,6 +813,10 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n \t\t\t N_(\"print out the bisect terms\"), BISECT_TERMS),\n \t\tOPT_CMDMODE(0, \"bisect-start\", &cmdmode,\n \t\t\t N_(\"start the bisect session\"), BISECT_START),\n+\t\tOPT_CMDMODE(0, \"bisect-next\", &cmdmode,\n+\t\t\t N_(\"find the next bisection commit\"), BISECT_NEXT),\n+\t\tOPT_CMDMODE(0, \"bisect-auto-next\", &cmdmode,\n+\t\t\t N_(\"verify the next bisection state then checkout the next bisection commit\"), BISECT_AUTO_NEXT),\n \t\tOPT_BOOL(0, \"no-checkout\", &no_checkout,\n \t\t\t N_(\"update BISECT_HEAD instead of checking out the current commit\")),\n \t\tOPT_BOOL(0, \"no-log\", &nolog,\n@@ -720,6 +878,18 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n \t\tset_terms(&terms, \"bad\", \"good\");\n \t\tres = bisect_start(&terms, no_checkout, argv, argc);\n \t\tbreak;\n+\tcase BISECT_NEXT:\n+\t\tif (argc)\n+\t\t\treturn error(_(\"--bisect-next requires 0 arguments\"));\n+\t\tget_terms(&terms);\n+\t\tres = bisect_next(&terms, prefix);\n+\t\tbreak;\n+\tcase BISECT_AUTO_NEXT:\n+\t\tif (argc)\n+\t\t\treturn error(_(\"--bisect-auto-next requires 0 arguments\"));\n+\t\tget_terms(&terms);\n+\t\tres = bisect_auto_next(&terms, prefix);\n+\t\tbreak;\n \tdefault:\n \t\treturn error(\"BUG: unknown subcommand '%d'\", cmdmode);\n \t}\ndiff --git a/git-bisect.sh b/git-bisect.sh\nindex efee12b8b1..e03f210d55 100755\n--- a/git-bisect.sh\n+++ b/git-bisect.sh\n@@ -86,8 +86,7 @@ bisect_start() {\n \t#\n \t# Check if we can proceed to the next bisect state.\n \t#\n-\tget_terms\n-\tbisect_auto_next\n+\tgit bisect--helper --bisect-auto-next || exit\n \n \ttrap '-' 0\n }\n@@ -140,45 +139,7 @@ bisect_state() {\n \t*)\n \t\tusage ;;\n \tesac\n-\tbisect_auto_next\n-}\n-\n-bisect_auto_next() {\n-\tgit bisect--helper --bisect-next-check $TERM_GOOD $TERM_BAD && bisect_next || :\n-}\n-\n-bisect_next() {\n-\tcase \"$#\" in 0) ;; *) usage ;; esac\n-\tbisect_autostart\n-\tgit bisect--helper --bisect-next-check $TERM_GOOD $TERM_BAD $TERM_GOOD|| exit\n-\n-\t# Perform all bisection computation, display and checkout\n-\tgit bisect--helper --next-all $(test -f \"$GIT_DIR/BISECT_HEAD\" && echo --no-checkout)\n-\tres=$?\n-\n-\t# Check if we should exit because bisection is finished\n-\tif test $res -eq 10\n-\tthen\n-\t\tbad_rev=$(git show-ref --hash --verify refs/bisect/$TERM_BAD)\n-\t\tbad_commit=$(git show-branch $bad_rev)\n-\t\techo \"# first $TERM_BAD commit: $bad_commit\" >>\"$GIT_DIR/BISECT_LOG\"\n-\t\texit 0\n-\telif test $res -eq 2\n-\tthen\n-\t\techo \"# only skipped commits left to test\" >>\"$GIT_DIR/BISECT_LOG\"\n-\t\tgood_revs=$(git for-each-ref --format=\"%(objectname)\" \"refs/bisect/$TERM_GOOD-*\")\n-\t\tfor skipped in $(git rev-list refs/bisect/$TERM_BAD --not $good_revs)\n-\t\tdo\n-\t\t\tskipped_commit=$(git show-branch $skipped)\n-\t\t\techo \"# possible first $TERM_BAD commit: $skipped_commit\" >>\"$GIT_DIR/BISECT_LOG\"\n-\t\tdone\n-\t\texit $res\n-\tfi\n-\n-\t# Check for an error in the bisection process\n-\ttest $res -ne 0 && exit $res\n-\n-\treturn 0\n+\tgit bisect--helper --bisect-auto-next\n }\n \n bisect_visualize() {\n@@ -232,7 +193,7 @@ bisect_replay () {\n \t\t\tdie \"$(gettext \"?? what are you talking about?\")\" ;;\n \t\tesac\n \tdone <\"$file\"\n-\tbisect_auto_next\n+\tgit bisect--helper --bisect-auto-next\n }\n \n bisect_run () {\n@@ -329,7 +290,7 @@ case \"$#\" in\n \t\tbisect_skip \"$@\" ;;\n \tnext)\n \t\t# Not sure we want \"next\" at the UI level anymore.\n-\t\tbisect_next \"$@\" ;;\n+\t\tgit bisect--helper --bisect-next \"$@\" || exit ;;\n \tvisualize|view)\n \t\tbisect_visualize \"$@\" ;;\n \treset)\n-- \n2.25.0\n\n"},{"id":"392527","messageId":"20200226101429.81327-5-mirucam@gmail.com","threadId":"52890","inReplyTo":"20200226101429.81327-1-mirucam@gmail.com","subject":"[PATCH 04/10] bisect--helper: retire `--bisect-clean-state` subcommand","fromName":"Miriam Rubio","fromEmail":"mirucam@gmail.com","sentAt":"2020-02-26T10:14:23Z","receivedAt":"2020-02-26T10:16:16Z","isPatch":true,"sender":{"key":"mirucam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56339109?v=4"},"body":"From: Pranit Bauva \n\nThe `--bisect-clean-state` subcommand is no longer used from the\ngit-bisect.sh shell script. Instead the function\n`bisect_clean_state()` is directly called from the C\nimplementation.\n\nMentored-by: Lars Schneider \nMentored-by: Christian Couder \nSigned-off-by: Pranit Bauva \nSigned-off-by: Tanushree Tumane \nSigned-off-by: Miriam Rubio \n---\n builtin/bisect--helper.c | 8 --------\n 1 file changed, 8 deletions(-)\n\ndiff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\nindex e75e7a0837..1792751179 100644\n--- a/builtin/bisect--helper.c\n+++ b/builtin/bisect--helper.c\n@@ -22,7 +22,6 @@ static GIT_PATH_FUNC(git_path_bisect_names, \"BISECT_NAMES\")\n static const char * const git_bisect_helper_usage[] = {\n \tN_(\"git bisect--helper --next-all [--no-checkout]\"),\n \tN_(\"git bisect--helper --write-terms \"),\n-\tN_(\"git bisect--helper --bisect-clean-state\"),\n \tN_(\"git bisect--helper --bisect-reset []\"),\n \tN_(\"git bisect--helper --bisect-write [--no-log] \"),\n \tN_(\"git bisect--helper --bisect-check-and-set-terms \"),\n@@ -810,7 +809,6 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n \tenum {\n \t\tNEXT_ALL = 1,\n \t\tWRITE_TERMS,\n-\t\tBISECT_CLEAN_STATE,\n \t\tCHECK_EXPECTED_REVS,\n \t\tBISECT_RESET,\n \t\tBISECT_WRITE,\n@@ -827,8 +825,6 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n \t\t\t N_(\"perform 'git bisect next'\"), NEXT_ALL),\n \t\tOPT_CMDMODE(0, \"write-terms\", &cmdmode,\n \t\t\t N_(\"write the terms to .git/BISECT_TERMS\"), WRITE_TERMS),\n-\t\tOPT_CMDMODE(0, \"bisect-clean-state\", &cmdmode,\n-\t\t\t N_(\"cleanup the bisection state\"), BISECT_CLEAN_STATE),\n \t\tOPT_CMDMODE(0, \"check-expected-revs\", &cmdmode,\n \t\t\t N_(\"check for expected revs\"), CHECK_EXPECTED_REVS),\n \t\tOPT_CMDMODE(0, \"bisect-reset\", &cmdmode,\n@@ -870,10 +866,6 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n \t\tif (argc != 2)\n \t\t\treturn error(_(\"--write-terms requires two arguments\"));\n \t\treturn write_terms(argv[0], argv[1]);\n-\tcase BISECT_CLEAN_STATE:\n-\t\tif (argc != 0)\n-\t\t\treturn error(_(\"--bisect-clean-state requires no arguments\"));\n-\t\treturn bisect_clean_state();\n \tcase CHECK_EXPECTED_REVS:\n \t\tcheck_expected_revs(argv, argc);\n \t\treturn 0;\n-- \n2.25.0\n\n"},{"id":"392528","messageId":"20200226101429.81327-6-mirucam@gmail.com","threadId":"52890","inReplyTo":"20200226101429.81327-1-mirucam@gmail.com","subject":"[PATCH 05/10] bisect--helper: retire `--next-all` subcommand","fromName":"Miriam Rubio","fromEmail":"mirucam@gmail.com","sentAt":"2020-02-26T10:14:24Z","receivedAt":"2020-02-26T10:16:18Z","isPatch":true,"sender":{"key":"mirucam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56339109?v=4"},"body":"From: Pranit Bauva \n\nThe `--next-all` subcommand is no longer used from the git-bisect.sh\nshell script. Instead the function `bisect_next_all()` is called from\nthe C implementation of `bisect_next()`.\n\nMentored-by: Lars Schneider \nMentored-by: Christian Couder \nSigned-off-by: Pranit Bauva \nSigned-off-by: Tanushree Tumane \nSigned-off-by: Miriam Rubio \n---\n builtin/bisect--helper.c | 9 +--------\n 1 file changed, 1 insertion(+), 8 deletions(-)\n\ndiff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\nindex 1792751179..c3bb936a40 100644\n--- a/builtin/bisect--helper.c\n+++ b/builtin/bisect--helper.c\n@@ -20,7 +20,6 @@ static GIT_PATH_FUNC(git_path_head_name, \"head-name\")\n static GIT_PATH_FUNC(git_path_bisect_names, \"BISECT_NAMES\")\n \n static const char * const git_bisect_helper_usage[] = {\n-\tN_(\"git bisect--helper --next-all [--no-checkout]\"),\n \tN_(\"git bisect--helper --write-terms \"),\n \tN_(\"git bisect--helper --bisect-reset []\"),\n \tN_(\"git bisect--helper --bisect-write [--no-log] \"),\n@@ -807,8 +806,7 @@ static enum bisect_error bisect_start(struct bisect_terms *terms, int no_checkou\n int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n {\n \tenum {\n-\t\tNEXT_ALL = 1,\n-\t\tWRITE_TERMS,\n+\t\tWRITE_TERMS = 1,\n \t\tCHECK_EXPECTED_REVS,\n \t\tBISECT_RESET,\n \t\tBISECT_WRITE,\n@@ -821,8 +819,6 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n \t} cmdmode = 0;\n \tint no_checkout = 0, res = 0, nolog = 0;\n \tstruct option options[] = {\n-\t\tOPT_CMDMODE(0, \"next-all\", &cmdmode,\n-\t\t\t N_(\"perform 'git bisect next'\"), NEXT_ALL),\n \t\tOPT_CMDMODE(0, \"write-terms\", &cmdmode,\n \t\t\t N_(\"write the terms to .git/BISECT_TERMS\"), WRITE_TERMS),\n \t\tOPT_CMDMODE(0, \"check-expected-revs\", &cmdmode,\n@@ -859,9 +855,6 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n \t\tusage_with_options(git_bisect_helper_usage, options);\n \n \tswitch (cmdmode) {\n-\tcase NEXT_ALL:\n-\t\tres = bisect_next_all(the_repository, prefix, no_checkout);\n-\t\tbreak;\n \tcase WRITE_TERMS:\n \t\tif (argc != 2)\n \t\t\treturn error(_(\"--write-terms requires two arguments\"));\n-- \n2.25.0\n\n"},{"id":"392529","messageId":"20200226101429.81327-7-mirucam@gmail.com","threadId":"52890","inReplyTo":"20200226101429.81327-1-mirucam@gmail.com","subject":"[PATCH 06/10] bisect--helper: reimplement `bisect_autostart` shell function in C","fromName":"Miriam Rubio","fromEmail":"mirucam@gmail.com","sentAt":"2020-02-26T10:14:25Z","receivedAt":"2020-02-26T10:16:18Z","isPatch":true,"sender":{"key":"mirucam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56339109?v=4"},"body":"From: Pranit Bauva \n\nReimplement the `bisect_autostart()` shell function in C and add the\nC implementation from `bisect_next()` which was previously left\nuncovered. Also add a subcommand `--bisect-autostart` to\n`git bisect--helper` be called from `bisect_state()` from\ngit-bisect.sh .\n\nUsing `--bisect-autostart` subcommand is a temporary measure to port\nshell function to C so as to use the existing test suite. As more\nfunctions are ported, this subcommand will be retired and\nbisect_autostart() will be called directly by `bisect_state()`.\n\nMentored-by: Lars Schneider \nMentored-by: Christian Couder \nMentored-by: Johannes Schindelin \nSigned-off-by: Pranit Bauva \nSigned-off-by: Tanushree Tumane \nSigned-off-by: Miriam Rubio \n---\n builtin/bisect--helper.c | 39 +++++++++++++++++++++++++++++++++++++++\n git-bisect.sh | 23 +----------------------\n 2 files changed, 40 insertions(+), 22 deletions(-)\n\ndiff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\nindex c3bb936a40..f9b04bee23 100644\n--- a/builtin/bisect--helper.c\n+++ b/builtin/bisect--helper.c\n@@ -30,6 +30,7 @@ static const char * const git_bisect_helper_usage[] = {\n \t\t\t\t\t \"[--no-checkout] [ [...]] [--] [...]\"),\n \tN_(\"git bisect--helper --bisect-next\"),\n \tN_(\"git bisect--helper --bisect-auto-next\"),\n+\tN_(\"git bisect--helper --bisect-autostart\"),\n \tNULL\n };\n \n@@ -56,6 +57,8 @@ static void set_terms(struct bisect_terms *terms, const char *bad,\n static const char vocab_bad[] = \"bad|new\";\n static const char vocab_good[] = \"good|old\";\n \n+static int bisect_autostart(struct bisect_terms *terms);\n+\n /*\n * Check whether the string `term` belongs to the set of strings\n * included in the variable arguments.\n@@ -555,6 +558,7 @@ static enum bisect_error bisect_next(struct bisect_terms *terms, const char *pre\n \tint no_checkout;\n \tenum bisect_error res;\n \n+\tbisect_autostart(terms);\n \tif (bisect_next_check(terms, terms->term_good))\n \t\treturn BISECT_FAILED;\n \n@@ -803,6 +807,32 @@ static enum bisect_error bisect_start(struct bisect_terms *terms, int no_checkou\n \treturn res;\n }\n \n+static int bisect_autostart(struct bisect_terms *terms)\n+{\n+\tif (is_empty_or_missing_file(git_path_bisect_start())) {\n+\t\tconst char *yesno;\n+\t\tconst char *argv[] = {NULL};\n+\t\tfprintf(stderr, _(\"You need to start by \\\"git bisect \"\n+\t\t\t\t \"start\\\"\\n\"));\n+\n+\t\tif (!isatty(STDIN_FILENO))\n+\t\t\treturn 1;\n+\n+\t\t/*\n+\t\t * TRANSLATORS: Make sure to include [Y] and [n] in your\n+\t\t * translation. The program will only accept English input\n+\t\t * at this point.\n+\t\t */\n+\t\tyesno = git_prompt(_(\"Do you want me to do it for you \"\n+\t\t\t\t \"[Y/n]? \"), PROMPT_ECHO);\n+\t\tif (starts_with(yesno, _(\"n\")) || starts_with(yesno, _(\"N\")))\n+\t\t\treturn 1;\n+\n+\t\treturn bisect_start(terms, 0, argv, 0);\n+\t}\n+\treturn BISECT_OK;\n+}\n+\n int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n {\n \tenum {\n@@ -816,6 +846,7 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n \t\tBISECT_START,\n \t\tBISECT_NEXT,\n \t\tBISECT_AUTO_NEXT,\n+\t\tBISECT_AUTOSTART,\n \t} cmdmode = 0;\n \tint no_checkout = 0, res = 0, nolog = 0;\n \tstruct option options[] = {\n@@ -839,6 +870,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n \t\t\t N_(\"find the next bisection commit\"), BISECT_NEXT),\n \t\tOPT_CMDMODE(0, \"bisect-auto-next\", &cmdmode,\n \t\t\t N_(\"verify the next bisection state then checkout the next bisection commit\"), BISECT_AUTO_NEXT),\n+\t\tOPT_CMDMODE(0, \"bisect-autostart\", &cmdmode,\n+\t\t\t N_(\"start the bisection if BISECT_START is empty or missing\"), BISECT_AUTOSTART),\n \t\tOPT_BOOL(0, \"no-checkout\", &no_checkout,\n \t\t\t N_(\"update BISECT_HEAD instead of checking out the current commit\")),\n \t\tOPT_BOOL(0, \"no-log\", &nolog,\n@@ -905,6 +938,12 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n \t\tget_terms(&terms);\n \t\tres = bisect_auto_next(&terms, prefix);\n \t\tbreak;\n+\tcase BISECT_AUTOSTART:\n+\t\tif (argc)\n+\t\t\treturn error(_(\"--bisect-autostart requires 0 arguments\"));\n+\t\tset_terms(&terms, \"bad\", \"good\");\n+\t\tres = bisect_autostart(&terms);\n+\t\tbreak;\n \tdefault:\n \t\treturn error(\"BUG: unknown subcommand '%d'\", cmdmode);\n \t}\ndiff --git a/git-bisect.sh b/git-bisect.sh\nindex 166f6a64dd..049ffacdff 100755\n--- a/git-bisect.sh\n+++ b/git-bisect.sh\n@@ -49,27 +49,6 @@ bisect_head()\n \tfi\n }\n \n-bisect_autostart() {\n-\ttest -s \"$GIT_DIR/BISECT_START\" || {\n-\t\tgettextln \"You need to start by \\\"git bisect start\\\"\" >&2\n-\t\tif test -t 0\n-\t\tthen\n-\t\t\t# TRANSLATORS: Make sure to include [Y] and [n] in your\n-\t\t\t# translation. The program will only accept English input\n-\t\t\t# at this point.\n-\t\t\tgettext \"Do you want me to do it for you [Y/n]? \" >&2\n-\t\t\tread yesno\n-\t\t\tcase \"$yesno\" in\n-\t\t\t[Nn]*)\n-\t\t\t\texit ;;\n-\t\t\tesac\n-\t\t\tgit bisect--helper --bisect-start\n-\t\telse\n-\t\t\texit 1\n-\t\tfi\n-\t}\n-}\n-\n bisect_skip() {\n \tall=''\n \tfor arg in \"$@\"\n@@ -86,7 +65,7 @@ bisect_skip() {\n }\n \n bisect_state() {\n-\tbisect_autostart\n+\tgit bisect--helper --bisect-autostart\n \tstate=$1\n \tgit bisect--helper --check-and-set-terms $state $TERM_GOOD $TERM_BAD || exit\n \tget_terms\n-- \n2.25.0\n\n"},{"id":"392530","messageId":"20200226101429.81327-8-mirucam@gmail.com","threadId":"52890","inReplyTo":"20200226101429.81327-1-mirucam@gmail.com","subject":"[PATCH 07/10] bisect--helper: reimplement `bisect_state` & `bisect_head` shell functions in C","fromName":"Miriam Rubio","fromEmail":"mirucam@gmail.com","sentAt":"2020-02-26T10:14:26Z","receivedAt":"2020-02-26T10:16:20Z","isPatch":true,"sender":{"key":"mirucam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56339109?v=4"},"body":"From: Pranit Bauva \n\nReimplement the `bisect_state()` shell functions in C and also add a\nsubcommand `--bisect-state` to `git-bisect--helper` to call them from\ngit-bisect.sh .\n\nUsing `--bisect-state` subcommand is a temporary measure to port shell\nfunction to C so as to use the existing test suite. As more functions\nare ported, this subcommand will be retired and will be called by some\nother methods.\n\n`bisect_head()` is only called from `bisect_state()`, thus it is not\nrequired to introduce another subcommand.\n\nMentored-by: Lars Schneider \nMentored-by: Christian Couder \nMentored-by: Johannes Schindelin \nSigned-off-by: Pranit Bauva \nSigned-off-by: Tanushree Tumane \nSigned-off-by: Miriam Rubio \n---\n builtin/bisect--helper.c | 80 ++++++++++++++++++++++++++++++++++++++++\n git-bisect.sh | 55 ++-------------------------\n 2 files changed, 84 insertions(+), 51 deletions(-)\n\ndiff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\nindex f9b04bee23..fedcad4d9b 100644\n--- a/builtin/bisect--helper.c\n+++ b/builtin/bisect--helper.c\n@@ -31,6 +31,8 @@ static const char * const git_bisect_helper_usage[] = {\n \tN_(\"git bisect--helper --bisect-next\"),\n \tN_(\"git bisect--helper --bisect-auto-next\"),\n \tN_(\"git bisect--helper --bisect-autostart\"),\n+\tN_(\"git bisect--helper --bisect-state (bad|new) []\"),\n+\tN_(\"git bisect--helper --bisect-state (good|old) [...]\"),\n \tNULL\n };\n \n@@ -833,6 +835,74 @@ static int bisect_autostart(struct bisect_terms *terms)\n \treturn BISECT_OK;\n }\n \n+static char *bisect_head(void)\n+{\n+\tif (is_empty_or_missing_file(git_path_bisect_head()))\n+\t\treturn \"HEAD\";\n+\telse\n+\t\treturn \"BISECT_HEAD\";\n+}\n+\n+static enum bisect_error bisect_state(struct bisect_terms *terms, const char **argv,\n+\t\t\tint argc)\n+{\n+\tconst char *state = argv[0];\n+\n+\tif (check_and_set_terms(terms, state))\n+\t\treturn BISECT_FAILED;\n+\n+\tif (!argc)\n+\t\treturn error(_(\"Please call `--bisect-state` with at least one argument\"));\n+\n+\tif (argc == 1 && one_of(state, terms->term_good,\n+\t terms->term_bad, \"skip\", NULL)) {\n+\t\tconst char *bisected_head = xstrdup(bisect_head());\n+\t\tconst char *hex[1];\n+\t\tstruct object_id oid;\n+\n+\t\tif (get_oid(bisected_head, &oid))\n+\t\t\treturn error(_(\"Bad rev input: %s\"), bisected_head);\n+\t\tif (bisect_write(state, oid_to_hex(&oid), terms, 0))\n+\t\t\treturn BISECT_FAILED;\n+\n+\t\t*hex = xstrdup(oid_to_hex(&oid));\n+\t\tcheck_expected_revs(hex, 1);\n+\t\treturn bisect_auto_next(terms, NULL);\n+\t}\n+\n+\tif ((argc == 2 && !strcmp(state, terms->term_bad)) ||\n+\t\t\tone_of(state, terms->term_good, \"skip\", NULL)) {\n+\t\tint i;\n+\t\tstruct string_list hex = STRING_LIST_INIT_DUP;\n+\n+\t\tfor (i = 1; i < argc; i++) {\n+\t\t\tstruct object_id oid;\n+\n+\t\t\tif (get_oid(argv[i], &oid)) {\n+\t\t\t\tstring_list_clear(&hex, 0);\n+\t\t\t\treturn error(_(\"Bad rev input: %s\"), argv[i]);\n+\t\t\t}\n+\t\t\tstring_list_append(&hex, oid_to_hex(&oid));\n+\t\t}\n+\t\tfor (i = 0; i < hex.nr; i++) {\n+\t\t\tconst char **hex_string = (const char **) &hex.items[i].string;\n+\t\t\tif (bisect_write(state, *hex_string, terms, 0)) {\n+\t\t\t\tstring_list_clear(&hex, 0);\n+\t\t\t\treturn BISECT_FAILED;\n+\t\t\t}\n+\t\t\tcheck_expected_revs(hex_string, 1);\n+\t\t}\n+\t\tstring_list_clear(&hex, 0);\n+\t\treturn bisect_auto_next(terms, NULL);\n+\t}\n+\n+\tif (!strcmp(state, terms->term_bad))\n+\t\treturn error(_(\"'git bisect %s' can take only one argument.\"),\n+\t\t terms->term_bad);\n+\n+\treturn BISECT_FAILED;\n+}\n+\n int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n {\n \tenum {\n@@ -847,6 +917,7 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n \t\tBISECT_NEXT,\n \t\tBISECT_AUTO_NEXT,\n \t\tBISECT_AUTOSTART,\n+\t\tBISECT_STATE\n \t} cmdmode = 0;\n \tint no_checkout = 0, res = 0, nolog = 0;\n \tstruct option options[] = {\n@@ -872,6 +943,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n \t\t\t N_(\"verify the next bisection state then checkout the next bisection commit\"), BISECT_AUTO_NEXT),\n \t\tOPT_CMDMODE(0, \"bisect-autostart\", &cmdmode,\n \t\t\t N_(\"start the bisection if BISECT_START is empty or missing\"), BISECT_AUTOSTART),\n+\t\tOPT_CMDMODE(0, \"bisect-state\", &cmdmode,\n+\t\t\t N_(\"mark the state of ref (or refs)\"), BISECT_STATE),\n \t\tOPT_BOOL(0, \"no-checkout\", &no_checkout,\n \t\t\t N_(\"update BISECT_HEAD instead of checking out the current commit\")),\n \t\tOPT_BOOL(0, \"no-log\", &nolog,\n@@ -944,6 +1017,13 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n \t\tset_terms(&terms, \"bad\", \"good\");\n \t\tres = bisect_autostart(&terms);\n \t\tbreak;\n+\tcase BISECT_STATE:\n+\t\tif (!argc)\n+\t\t\treturn error(_(\"--bisect-state requires at least one revision\"));\n+\t\tset_terms(&terms, \"bad\", \"good\");\n+\t\tget_terms(&terms);\n+\t\tres = bisect_state(&terms, argv, argc);\n+\t\tbreak;\n \tdefault:\n \t\treturn error(\"BUG: unknown subcommand '%d'\", cmdmode);\n \t}\ndiff --git a/git-bisect.sh b/git-bisect.sh\nindex 049ffacdff..7f10187055 100755\n--- a/git-bisect.sh\n+++ b/git-bisect.sh\n@@ -39,16 +39,6 @@ _x40=\"$_x40$_x40$_x40$_x40$_x40$_x40$_x40$_x40\"\n TERM_BAD=bad\n TERM_GOOD=good\n \n-bisect_head()\n-{\n-\tif test -f \"$GIT_DIR/BISECT_HEAD\"\n-\tthen\n-\t\techo BISECT_HEAD\n-\telse\n-\t\techo HEAD\n-\tfi\n-}\n-\n bisect_skip() {\n \tall=''\n \tfor arg in \"$@\"\n@@ -61,43 +51,7 @@ bisect_skip() {\n \t\tesac\n \t\tall=\"$all $revs\"\n \tdone\n-\teval bisect_state 'skip' $all\n-}\n-\n-bisect_state() {\n-\tgit bisect--helper --bisect-autostart\n-\tstate=$1\n-\tgit bisect--helper --check-and-set-terms $state $TERM_GOOD $TERM_BAD || exit\n-\tget_terms\n-\tcase \"$#,$state\" in\n-\t0,*)\n-\t\tdie \"Please call 'bisect_state' with at least one argument.\" ;;\n-\t1,\"$TERM_BAD\"|1,\"$TERM_GOOD\"|1,skip)\n-\t\tbisected_head=$(bisect_head)\n-\t\trev=$(git rev-parse --verify \"$bisected_head\") ||\n-\t\t\tdie \"$(eval_gettext \"Bad rev input: \\$bisected_head\")\"\n-\t\tgit bisect--helper --bisect-write \"$state\" \"$rev\" \"$TERM_GOOD\" \"$TERM_BAD\" || exit\n-\t\tgit bisect--helper --check-expected-revs \"$rev\" ;;\n-\t2,\"$TERM_BAD\"|*,\"$TERM_GOOD\"|*,skip)\n-\t\tshift\n-\t\thash_list=''\n-\t\tfor rev in \"$@\"\n-\t\tdo\n-\t\t\tsha=$(git rev-parse --verify \"$rev^{commit}\") ||\n-\t\t\t\tdie \"$(eval_gettext \"Bad rev input: \\$rev\")\"\n-\t\t\thash_list=\"$hash_list $sha\"\n-\t\tdone\n-\t\tfor rev in $hash_list\n-\t\tdo\n-\t\t\tgit bisect--helper --bisect-write \"$state\" \"$rev\" \"$TERM_GOOD\" \"$TERM_BAD\" || exit\n-\t\tdone\n-\t\tgit bisect--helper --check-expected-revs $hash_list ;;\n-\t*,\"$TERM_BAD\")\n-\t\tdie \"$(eval_gettext \"'git bisect \\$TERM_BAD' can take only one argument.\")\" ;;\n-\t*)\n-\t\tusage ;;\n-\tesac\n-\tgit bisect--helper --bisect-auto-next\n+\teval git bisect--helper --bisect-state 'skip' $all\n }\n \n bisect_visualize() {\n@@ -185,8 +139,7 @@ exit code \\$res from '\\$command' is < 0 or >= 128\" >&2\n \t\t\tstate=\"$TERM_GOOD\"\n \t\tfi\n \n-\t\t# We have to use a subshell because \"bisect_state\" can exit.\n-\t\t( bisect_state $state >\"$GIT_DIR/BISECT_RUN\" )\n+\t\t( git bisect--helper --bisect-state $state >\"$GIT_DIR/BISECT_RUN\" )\n \t\tres=$?\n \n \t\tcat \"$GIT_DIR/BISECT_RUN\"\n@@ -201,7 +154,7 @@ exit code \\$res from '\\$command' is < 0 or >= 128\" >&2\n \t\tif [ $res -ne 0 ]\n \t\tthen\n \t\t\teval_gettextln \"bisect run failed:\n-'bisect_state \\$state' exited with error code \\$res\" >&2\n+'git bisect--helper --bisect-state \\$state' exited with error code \\$res\" >&2\n \t\t\texit $res\n \t\tfi\n \n@@ -242,7 +195,7 @@ case \"$#\" in\n \tstart)\n \t\tgit bisect--helper --bisect-start \"$@\" ;;\n \tbad|good|new|old|\"$TERM_BAD\"|\"$TERM_GOOD\")\n-\t\tbisect_state \"$cmd\" \"$@\" ;;\n+\t\tgit bisect--helper --bisect-state \"$cmd\" \"$@\" ;;\n \tskip)\n \t\tbisect_skip \"$@\" ;;\n \tnext)\n-- \n2.25.0\n\n"},{"id":"392531","messageId":"20200226101429.81327-9-mirucam@gmail.com","threadId":"52890","inReplyTo":"20200226101429.81327-1-mirucam@gmail.com","subject":"[PATCH 08/10] bisect--helper: retire `--check-expected-revs` subcommand","fromName":"Miriam Rubio","fromEmail":"mirucam@gmail.com","sentAt":"2020-02-26T10:14:27Z","receivedAt":"2020-02-26T10:16:21Z","isPatch":true,"sender":{"key":"mirucam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56339109?v=4"},"body":"From: Pranit Bauva \n\nThe `--check-expected-revs` subcommand is no longer used from the\ngit-bisect.sh shell script. Instead the function\n`check_expected_revs()` is called from the C implementation of\n`bisect-next()`.\n\nMentored-by: Lars Schneider \nMentored-by: Christian Couder \nMentored-by: Johannes Schindelin \nSigned-off-by: Pranit Bauva \nSigned-off-by: Tanushree Tumane \nSigned-off-by: Miriam Rubio \n---\n builtin/bisect--helper.c | 6 ------\n 1 file changed, 6 deletions(-)\n\ndiff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\nindex fedcad4d9b..c2fd3ee1b3 100644\n--- a/builtin/bisect--helper.c\n+++ b/builtin/bisect--helper.c\n@@ -907,7 +907,6 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n {\n \tenum {\n \t\tWRITE_TERMS = 1,\n-\t\tCHECK_EXPECTED_REVS,\n \t\tBISECT_RESET,\n \t\tBISECT_WRITE,\n \t\tCHECK_AND_SET_TERMS,\n@@ -923,8 +922,6 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n \tstruct option options[] = {\n \t\tOPT_CMDMODE(0, \"write-terms\", &cmdmode,\n \t\t\t N_(\"write the terms to .git/BISECT_TERMS\"), WRITE_TERMS),\n-\t\tOPT_CMDMODE(0, \"check-expected-revs\", &cmdmode,\n-\t\t\t N_(\"check for expected revs\"), CHECK_EXPECTED_REVS),\n \t\tOPT_CMDMODE(0, \"bisect-reset\", &cmdmode,\n \t\t\t N_(\"reset the bisection state\"), BISECT_RESET),\n \t\tOPT_CMDMODE(0, \"bisect-write\", &cmdmode,\n@@ -965,9 +962,6 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n \t\tif (argc != 2)\n \t\t\treturn error(_(\"--write-terms requires two arguments\"));\n \t\treturn write_terms(argv[0], argv[1]);\n-\tcase CHECK_EXPECTED_REVS:\n-\t\tcheck_expected_revs(argv, argc);\n-\t\treturn 0;\n \tcase BISECT_RESET:\n \t\tif (argc > 1)\n \t\t\treturn error(_(\"--bisect-reset requires either no argument or a commit\"));\n-- \n2.25.0\n\n"},{"id":"392532","messageId":"20200226101429.81327-10-mirucam@gmail.com","threadId":"52890","inReplyTo":"20200226101429.81327-1-mirucam@gmail.com","subject":"[PATCH 09/10] bisect--helper: retire `--write-terms` subcommand","fromName":"Miriam Rubio","fromEmail":"mirucam@gmail.com","sentAt":"2020-02-26T10:14:28Z","receivedAt":"2020-02-26T10:16:22Z","isPatch":true,"sender":{"key":"mirucam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56339109?v=4"},"body":"From: Pranit Bauva \n\nThe `--write-terms` subcommand is no longer used from the\ngit-bisect.sh shell script. Instead the function `write_terms()`\nis called from the C implementation of `set_terms()` and\n`bisect_start()`.\n\nMentored-by: Lars Schneider \nMentored-by: Christian Couder \nMentored-by: Johannes Schindelin \nSigned-off-by: Pranit Bauva \nSigned-off-by: Tanushree Tumane \nSigned-off-by: Miriam Rubio \n---\n builtin/bisect--helper.c | 10 +---------\n 1 file changed, 1 insertion(+), 9 deletions(-)\n\ndiff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\nindex c2fd3ee1b3..650ceefcd7 100644\n--- a/builtin/bisect--helper.c\n+++ b/builtin/bisect--helper.c\n@@ -20,7 +20,6 @@ static GIT_PATH_FUNC(git_path_head_name, \"head-name\")\n static GIT_PATH_FUNC(git_path_bisect_names, \"BISECT_NAMES\")\n \n static const char * const git_bisect_helper_usage[] = {\n-\tN_(\"git bisect--helper --write-terms \"),\n \tN_(\"git bisect--helper --bisect-reset []\"),\n \tN_(\"git bisect--helper --bisect-write [--no-log] \"),\n \tN_(\"git bisect--helper --bisect-check-and-set-terms \"),\n@@ -906,8 +905,7 @@ static enum bisect_error bisect_state(struct bisect_terms *terms, const char **a\n int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n {\n \tenum {\n-\t\tWRITE_TERMS = 1,\n-\t\tBISECT_RESET,\n+\t\tBISECT_RESET = 1,\n \t\tBISECT_WRITE,\n \t\tCHECK_AND_SET_TERMS,\n \t\tBISECT_NEXT_CHECK,\n@@ -920,8 +918,6 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n \t} cmdmode = 0;\n \tint no_checkout = 0, res = 0, nolog = 0;\n \tstruct option options[] = {\n-\t\tOPT_CMDMODE(0, \"write-terms\", &cmdmode,\n-\t\t\t N_(\"write the terms to .git/BISECT_TERMS\"), WRITE_TERMS),\n \t\tOPT_CMDMODE(0, \"bisect-reset\", &cmdmode,\n \t\t\t N_(\"reset the bisection state\"), BISECT_RESET),\n \t\tOPT_CMDMODE(0, \"bisect-write\", &cmdmode,\n@@ -958,10 +954,6 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n \t\tusage_with_options(git_bisect_helper_usage, options);\n \n \tswitch (cmdmode) {\n-\tcase WRITE_TERMS:\n-\t\tif (argc != 2)\n-\t\t\treturn error(_(\"--write-terms requires two arguments\"));\n-\t\treturn write_terms(argv[0], argv[1]);\n \tcase BISECT_RESET:\n \t\tif (argc > 1)\n \t\t\treturn error(_(\"--bisect-reset requires either no argument or a commit\"));\n-- \n2.25.0\n\n"},{"id":"392533","messageId":"20200226101429.81327-11-mirucam@gmail.com","threadId":"52890","inReplyTo":"20200226101429.81327-1-mirucam@gmail.com","subject":"[PATCH 10/10] bisect--helper: retire `--bisect-autostart` subcommand","fromName":"Miriam Rubio","fromEmail":"mirucam@gmail.com","sentAt":"2020-02-26T10:14:29Z","receivedAt":"2020-02-26T10:16:25Z","isPatch":true,"sender":{"key":"mirucam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56339109?v=4"},"body":"From: Pranit Bauva \n\nThe `--bisect-autostart` subcommand is no longer used from the\ngit-bisect.sh shell script. Instead the function\n`bisect_autostart()` is directly called from the C implementation.\n\nMentored-by: Lars Schneider \nMentored-by: Christian Couder \nMentored-by: Johannes Schindelin \nSigned-off-by: Pranit Bauva \nSigned-off-by: Tanushree Tumane \nSigned-off-by: Miriam Rubio \n---\n builtin/bisect--helper.c | 10 ----------\n 1 file changed, 10 deletions(-)\n\ndiff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\nindex 650ceefcd7..c692c660a4 100644\n--- a/builtin/bisect--helper.c\n+++ b/builtin/bisect--helper.c\n@@ -29,7 +29,6 @@ static const char * const git_bisect_helper_usage[] = {\n \t\t\t\t\t \"[--no-checkout] [ [...]] [--] [...]\"),\n \tN_(\"git bisect--helper --bisect-next\"),\n \tN_(\"git bisect--helper --bisect-auto-next\"),\n-\tN_(\"git bisect--helper --bisect-autostart\"),\n \tN_(\"git bisect--helper --bisect-state (bad|new) []\"),\n \tN_(\"git bisect--helper --bisect-state (good|old) [...]\"),\n \tNULL\n@@ -913,7 +912,6 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n \t\tBISECT_START,\n \t\tBISECT_NEXT,\n \t\tBISECT_AUTO_NEXT,\n-\t\tBISECT_AUTOSTART,\n \t\tBISECT_STATE\n \t} cmdmode = 0;\n \tint no_checkout = 0, res = 0, nolog = 0;\n@@ -934,8 +932,6 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n \t\t\t N_(\"find the next bisection commit\"), BISECT_NEXT),\n \t\tOPT_CMDMODE(0, \"bisect-auto-next\", &cmdmode,\n \t\t\t N_(\"verify the next bisection state then checkout the next bisection commit\"), BISECT_AUTO_NEXT),\n-\t\tOPT_CMDMODE(0, \"bisect-autostart\", &cmdmode,\n-\t\t\t N_(\"start the bisection if BISECT_START is empty or missing\"), BISECT_AUTOSTART),\n \t\tOPT_CMDMODE(0, \"bisect-state\", &cmdmode,\n \t\t\t N_(\"mark the state of ref (or refs)\"), BISECT_STATE),\n \t\tOPT_BOOL(0, \"no-checkout\", &no_checkout,\n@@ -997,12 +993,6 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n \t\tget_terms(&terms);\n \t\tres = bisect_auto_next(&terms, prefix);\n \t\tbreak;\n-\tcase BISECT_AUTOSTART:\n-\t\tif (argc)\n-\t\t\treturn error(_(\"--bisect-autostart requires 0 arguments\"));\n-\t\tset_terms(&terms, \"bad\", \"good\");\n-\t\tres = bisect_autostart(&terms);\n-\t\tbreak;\n \tcase BISECT_STATE:\n \t\tif (!argc)\n \t\t\treturn error(_(\"--bisect-state requires at least one revision\"));\n-- \n2.25.0\n\n"},{"id":"392554","messageId":"xmqq4kvdjhhc.fsf@gitster-ct.c.googlers.com","threadId":"52890","inReplyTo":"20200226101429.81327-2-mirucam@gmail.com","subject":"Re: [PATCH 01/10] bisect--helper: introduce new `write_in_file()` function","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-02-26T19:06:39Z","receivedAt":"2020-02-26T19:06:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Miriam Rubio writes:\n\n> Let's refactor code adding a new `write_in_file()` function\n> that opens a file for writing a message and closes it.\n>\n> This removes some duplicated code and makes the code simpler,\n> clearer and easier to understand.\n\nI find it a bit too much self promotion ;-) With what we see only in\nthis step, the code is not so much simpler nor easier to understand.\nPerhaps when we see more callers in the remainder of the series, we\nmay start to appreciate it, but if that is the case, we should\nclarify it here, e.g.\n\n This helper will be used in later steps and makes the code\n simpler, ...\n\n> +static int write_in_file(const char *filepath, const char *content, int append)\n> +{\n> +\tFILE *fp = NULL;\n> +\tconst char *mode = append ? \"a\" : \"w\";\n> +\n> +\tfp = fopen(filepath, mode);\n> +\tif (!fp)\n> +\t\treturn error_errno(_(\"could not open the file '%s'\"), filepath);\n> +\tif (!fprintf(fp, \"%s\\n\", content))\n> +\t\treturn error_errno(_(\"could not write in file '%s'\"), filepath);\n\nUse and non-use of definite article being inconstent between \"open\nthe file X\" vs \"write in file X\" bothers me as a reader here.\n\nWouldn't it be more common to say \"write TO\" here, not \"write IN\"?\n\n> +\treturn fclose(fp);\n\nWe didn't use to say error_errno(_(\"could not close ...\")) in the\noriginal, so we stay silent and return an error (EOF, which is a\nnegative integer) from here.\n\n> +}\n> +\n> static int check_term_format(const char *term, const char *orig_term)\n> {\n> \tint res;\n> @@ -104,7 +117,7 @@ static int check_term_format(const char *term, const char *orig_term)\n> \n> static int write_terms(const char *bad, const char *good)\n> {\n> -\tFILE *fp = NULL;\n> +\tchar *content = xstrfmt(\"%s\\n%s\", bad, good);\n> \tint res;\n> \n> \tif (!strcmp(bad, good))\n> @@ -113,12 +126,9 @@ static int write_terms(const char *bad, const char *good)\n> \tif (check_term_format(bad, \"bad\") || check_term_format(good, \"good\"))\n> \t\treturn -1;\n> \n> -\tfp = fopen(git_path_bisect_terms(), \"w\");\n> -\tif (!fp)\n> -\t\treturn error_errno(_(\"could not open the file BISECT_TERMS\"));\n> +\tres = write_in_file(git_path_bisect_terms(), content, 0);\n> +\tfree(content);\n> \n> -\tres = fprintf(fp, \"%s\\n%s\\n\", bad, good);\n\nWith just this one callsite, I do not think it is a good idea to\nlose the \"write a formatted output without an extra allcoation\" \nby introducing this wrapper. The equation may become different if\nwe see more (a lot more) callsites that already have strings ready\nto be written that can use this helper, though.\n\n> -\tres |= fclose(fp);\n> \treturn (res < 0) ? -1 : 0;\n\nThe return value is a bit strange here, but it is inherited from the\noriginal, so I'll let it pass.\n\n> }\n"},{"id":"392555","messageId":"xmqqzhd5i1na.fsf@gitster-ct.c.googlers.com","threadId":"52890","inReplyTo":"20200226101429.81327-3-mirucam@gmail.com","subject":"Re: [PATCH 02/10] bisect--helper: reimplement `bisect_next` and `bisect_auto_next` shell functions in C","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-02-26T19:34:01Z","receivedAt":"2020-02-26T19:34:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Miriam Rubio writes:\n\n> +static int register_good_ref(const char *refname,\n> +\t\t\t const struct object_id *oid, int flags,\n> +\t\t\t void *cb_data)\n> +{\n> +\tstruct string_list *good_refs = cb_data;\n> +\n> +\tstring_list_append(good_refs, oid_to_hex(oid));\n> +\treturn 0;\n> +}\n> +\n> +static void prepare_rev_argv(struct bisect_terms *terms, struct argv_array *rev_argv)\n> +{\n> +\tstruct string_list good_revs = STRING_LIST_INIT_DUP;\n> +\tchar *term_good = xstrfmt(\"%s-*\", terms->term_good);\n> +\n> +\tfor_each_glob_ref_in(register_good_ref, term_good,\n> +\t\t\t \"refs/bisect/\", &good_revs);\n> +\n> +\targv_array_pushl(rev_argv, \"skipped_commits\", \"refs/bisect/bad\", \"--not\", NULL);\n> +\tfor (int i = 0; i < good_revs.nr; i++)\n> +\t\targv_array_push(rev_argv, good_revs.items[i].string);\n> +\n> +\tstring_list_clear(&good_revs, 0);\n> +\tfree(term_good);\n\nWhy do you need good_revs string_list in the first place? Wouldn't\nit be simpler and easier to understand to pass in the argv as part\nof the callback data and push the good refs in the callback function?\n\n> +static int prepare_revs(struct bisect_terms *terms, struct rev_info *revs)\n> +{\n> +\tint res = 0;\n> +\tstruct argv_array rev_argv = ARGV_ARRAY_INIT;\n> +\n> +\tprepare_rev_argv(terms, &rev_argv);\n> +\n> +\t/*\n> +\t * It is important to reset the flags used by revision walks\n> +\t * as the previous call to bisect_next_all() in turn\n> +\t * setups a revision walk.\n\n\"setup\" is not a verb X-<. \"... in turn sets up a ...\" would be OK.\n\n> +static int process_skipped_commits(FILE *fp, struct bisect_terms *terms, struct rev_info *revs)\n> +{\n> +\tstruct commit *commit;\n> +\tstruct pretty_print_context pp = {0};\n> +\n> +\tif (fprintf(fp, \"# only skipped commits left to test\\n\") < 1)\n> +\t\treturn -1;\n> +\n> +\twhile ((commit = get_revision(revs)) != NULL) {\n> +\t\tstruct strbuf commit_name = STRBUF_INIT;\n> +\t\tformat_commit_message(commit, \"%s\",\n> +\t\t\t\t &commit_name, &pp);\n> +\t\tfprintf(fp, \"# possible first %s commit: [%s] %s\\n\",\n> +\t\t\tterms->term_bad, oid_to_hex(&commit->object.oid),\n> +\t\t\tcommit_name.buf);\n> +\t\tstrbuf_release(&commit_name);\n> +\t\tclear_commit_marks(commit, ALL_REV_FLAGS);\n\nclear_commit_marks() is to clear the given flag bits from the named\ncommit *AND* all of its ancestors (recursively) as long as they have\nthese bits on, and it typically is used in order to clean up the\nstate bits left on objects _after_ a revision walk is _done_.\n\nCalling it, especially to clear ALL_REV_FLAGS that contains crucial\nflag bits necessary to drive get_revision(), inside a loop that is\nstill walking commits one by one by calling get_revision(), is\nextremely unusual.\n\nIt would be surprising if this code were correct. It may be that it\nhappens to (appear to) work because parents of the commit hasn't\nbeen painted and calling the helper only clears the mark from the\ncommit (so we won't see repeated \"painting down to the root commit\")\nin which case this might be an extremely expensive looking variant\nof\n\n\tcommit->object.flags &= ~ALL_REV_FLAGS;\n\nthat only confuses the readers.\n\nEven then, I think by clearing bits like SEEN from commit, it breaks\nthe revision traversal machinery---for example, doesn't this mean\nthat the commit we just processed can be re-visited by\nget_revision() without deduping in a history with forks and merges?\n\nHas this been shown to any of your mentors before sending it to the\nlist?\n\n> +static int bisect_successful(struct bisect_terms *terms)\n> +{\n> +\tstruct object_id oid;\n> +\tstruct commit *commit;\n> +\tstruct pretty_print_context pp = {0};\n> +\tstruct strbuf commit_name = STRBUF_INIT;\n> +\tchar *bad_ref = xstrfmt(\"refs/bisect/%s\",\n> +\t\t\t\tterms->term_bad);\n> +\tchar *content;\n> +\tint res;\n> +\n> +\tread_ref(bad_ref, &oid);\n> +\tprintf(\"%s\\n\", bad_ref);\n> +\tcommit = lookup_commit_reference(the_repository, &oid);\n> +\tformat_commit_message(commit, \"%s\", &commit_name, &pp);\n> +\n> +\tcontent = xstrfmt(\"# first %s commit: [%s] %s\",\n> +\tterms->term_bad, oid_to_hex(&oid),\n> +\tcommit_name.buf);\n\nStrange indentation.\n\n> +\tres = write_in_file(git_path_bisect_log(), content, 1);\n\nSo this is a new use of the helper introduced in [01/10]. It is\ntrue that use of it lets this caller not to worry about opening,\nwriting and closing, but it needs an extra allocation to prepare\n\"content\".\n\nIf the calling convention were more like this:\n\n\tres = write_to_file(git_path_bisect_log(), \"a\",\n\t\t\t \"# first %s commit: [%s] %s\",\n\t\t\t terms->term_bad, oid_to_hex(&oid), commit_name.buf,\n\t\t\t NULL);\n\nthis callsite and the callsite in [01/10] wouldn't have had to make\nan unnecessary allocation, perhaps?\n\n"},{"id":"392616","messageId":"CAN7CjDDwgR=y8gyYmDzmuTW3AKvb1N=EdCtH-8Tr7T=b6cG5gQ@mail.gmail.com","threadId":"52890","inReplyTo":"xmqqzhd5i1na.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 02/10] bisect--helper: reimplement `bisect_next` and `bisect_auto_next` shell functions in C","fromName":"Miriam R.","fromEmail":"mirucam@gmail.com","sentAt":"2020-02-27T15:34:46Z","receivedAt":"2020-02-27T15:35:00Z","isPatch":true,"sender":{"key":"mirucam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56339109?v=4"},"body":"Hi Junio,\n\nEl mié., 26 feb. 2020 a las 20:34, Junio C Hamano\n() escribió:\n>\n> Miriam Rubio writes:\n>\n> > +static int register_good_ref(const char *refname,\n> > + const struct object_id *oid, int flags,\n> > + void *cb_data)\n> > +{\n> > + struct string_list *good_refs = cb_data;\n> > +\n> > + string_list_append(good_refs, oid_to_hex(oid));\n> > + return 0;\n> > +}\n> > +\n> > +static void prepare_rev_argv(struct bisect_terms *terms, struct argv_array *rev_argv)\n> > +{\n> > + struct string_list good_revs = STRING_LIST_INIT_DUP;\n> > + char *term_good = xstrfmt(\"%s-*\", terms->term_good);\n> > +\n> > + for_each_glob_ref_in(register_good_ref, term_good,\n> > + \"refs/bisect/\", &good_revs);\n> > +\n> > + argv_array_pushl(rev_argv, \"skipped_commits\", \"refs/bisect/bad\", \"--not\", NULL);\n> > + for (int i = 0; i < good_revs.nr; i++)\n> > + argv_array_push(rev_argv, good_revs.items[i].string);\n> > +\n> > + string_list_clear(&good_revs, 0);\n> > + free(term_good);\n>\n> Why do you need good_revs string_list in the first place? Wouldn't\n> it be simpler and easier to understand to pass in the argv as part\n> of the callback data and push the good refs in the callback function?\n>\nOk, I will do it that way.\n\n> > +static int prepare_revs(struct bisect_terms *terms, struct rev_info *revs)\n> > +{\n> > + int res = 0;\n> > + struct argv_array rev_argv = ARGV_ARRAY_INIT;\n> > +\n> > + prepare_rev_argv(terms, &rev_argv);\n> > +\n> > + /*\n> > + * It is important to reset the flags used by revision walks\n> > + * as the previous call to bisect_next_all() in turn\n> > + * setups a revision walk.\n>\n> \"setup\" is not a verb X-<. \"... in turn sets up a ...\" would be OK.\n>\nNoted.\n> > +static int process_skipped_commits(FILE *fp, struct bisect_terms *terms, struct rev_info *revs)\n> > +{\n> > + struct commit *commit;\n> > + struct pretty_print_context pp = {0};\n> > +\n> > + if (fprintf(fp, \"# only skipped commits left to test\\n\") < 1)\n> > + return -1;\n> > +\n> > + while ((commit = get_revision(revs)) != NULL) {\n> > + struct strbuf commit_name = STRBUF_INIT;\n> > + format_commit_message(commit, \"%s\",\n> > + &commit_name, &pp);\n> > + fprintf(fp, \"# possible first %s commit: [%s] %s\\n\",\n> > + terms->term_bad, oid_to_hex(&commit->object.oid),\n> > + commit_name.buf);\n> > + strbuf_release(&commit_name);\n> > + clear_commit_marks(commit, ALL_REV_FLAGS);\n>\n> clear_commit_marks() is to clear the given flag bits from the named\n> commit *AND* all of its ancestors (recursively) as long as they have\n> these bits on, and it typically is used in order to clean up the\n> state bits left on objects _after_ a revision walk is _done_.\n>\n> Calling it, especially to clear ALL_REV_FLAGS that contains crucial\n> flag bits necessary to drive get_revision(), inside a loop that is\n> still walking commits one by one by calling get_revision(), is\n> extremely unusual.\n>\n> It would be surprising if this code were correct. It may be that it\n> happens to (appear to) work because parents of the commit hasn't\n> been painted and calling the helper only clears the mark from the\n> commit (so we won't see repeated \"painting down to the root commit\")\n> in which case this might be an extremely expensive looking variant\n> of\n>\n> commit->object.flags &= ~ALL_REV_FLAGS;\n>\n> that only confuses the readers.\n>\n> Even then, I think by clearing bits like SEEN from commit, it breaks\n> the revision traversal machinery---for example, doesn't this mean\n> that the commit we just processed can be re-visited by\n> get_revision() without deduping in a history with forks and merges?\n>\n> Has this been shown to any of your mentors before sending it to the\n> list?\n\nAdding clear_commit_marks() was a suggestion of a previous review of this patch:\nhttps://public-inbox.org/git/nycvar.QRO.7.76.6.2001301619340.46@tvgsbejvaqbjf.bet/\n\nAnd of course, my mentor always reviews my patch series before sending.\n\n>\n> > +static int bisect_successful(struct bisect_terms *terms)\n> > +{\n> > + struct object_id oid;\n> > + struct commit *commit;\n> > + struct pretty_print_context pp = {0};\n> > + struct strbuf commit_name = STRBUF_INIT;\n> > + char *bad_ref = xstrfmt(\"refs/bisect/%s\",\n> > + terms->term_bad);\n> > + char *content;\n> > + int res;\n> > +\n> > + read_ref(bad_ref, &oid);\n> > + printf(\"%s\\n\", bad_ref);\n> > + commit = lookup_commit_reference(the_repository, &oid);\n> > + format_commit_message(commit, \"%s\", &commit_name, &pp);\n> > +\n> > + content = xstrfmt(\"# first %s commit: [%s] %s\",\n> > + terms->term_bad, oid_to_hex(&oid),\n> > + commit_name.buf);\n>\n> Strange indentation.\nNoted.\n>\n> > + res = write_in_file(git_path_bisect_log(), content, 1);\n>\n> So this is a new use of the helper introduced in [01/10]. It is\n> true that use of it lets this caller not to worry about opening,\n> writing and closing, but it needs an extra allocation to prepare\n> \"content\".\n>\n> If the calling convention were more like this:\n>\n> res = write_to_file(git_path_bisect_log(), \"a\",\n> \"# first %s commit: [%s] %s\",\n> terms->term_bad, oid_to_hex(&oid), commit_name.buf,\n> NULL);\n>\n> this callsite and the callsite in [01/10] wouldn't have had to make\n> an unnecessary allocation, perhaps?\n>\nAha. I will change it.\n\nThis helper function was also a suggestion of the previous reviewer:\nhttps://public-inbox.org/git/nycvar.QRO.7.76.6.2001301619340.46@tvgsbejvaqbjf.bet/\n\nThank you very much for reviewing!.\n\nBest,\nMiriam\n"},{"id":"392627","messageId":"xmqqwo88f0do.fsf@gitster-ct.c.googlers.com","threadId":"52890","inReplyTo":"CAN7CjDDwgR=y8gyYmDzmuTW3AKvb1N=EdCtH-8Tr7T=b6cG5gQ@mail.gmail.com","subject":"Re: [PATCH 02/10] bisect--helper: reimplement `bisect_next` and `bisect_auto_next` shell functions in C","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-02-27T16:41:55Z","receivedAt":"2020-02-27T16:42:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Miriam R.\" writes:\n\n>> It would be surprising if this code were correct. It may be that it\n>> happens to (appear to) work because parents of the commit hasn't\n>> been painted and calling the helper only clears the mark from the\n>> commit (so we won't see repeated \"painting down to the root commit\")\n>> in which case this might be an extremely expensive looking variant\n>> of\n>>\n>> commit->object.flags &= ~ALL_REV_FLAGS;\n>>\n>> that only confuses the readers.\n>>\n>> Even then, I think by clearing bits like SEEN from commit, it breaks\n>> the revision traversal machinery---for example, doesn't this mean\n>> that the commit we just processed can be re-visited by\n>> get_revision() without deduping in a history with forks and merges?\n>>\n>> Has this been shown to any of your mentors before sending it to the\n>> list?\n>\n> Adding clear_commit_marks() was a suggestion of a previous review of this patch:\n> https://public-inbox.org/git/nycvar.QRO.7.76.6.2001301619340.46@tvgsbejvaqbjf.bet/\n>\n> And of course, my mentor always reviews my patch series before sending.\n\nOK, I just didn't know how you and your mentors work. Some leave\ntheir door open for mentees and help them when asked but otherwise\nact as an ordinary reviewer who somehow prioritises reviewing the\nwork by their mentees. So your mentors may be a better source of\ninformation why this piece of code, which I still do not know how it\ncould be correct, is supposed to work. Good.\n\nAfter reading the above URL, I think you may have misread the\nsuggestion you were given. Resetting using clear_commit_marks() may\nbe necessary, but you do so when you finished walking so that you\ncan do unrelated revision walk later. For that, you clear the flag\nbits after the while() loop that asks get_revision() to yield\ncommits are done, using the initial set of commits that you used to\nstart iteration.\n\nThat is how bisect.c::check_ancestors() work, that is\n\n - it initializes a rev_info 'revs' from an array of commit rev[]\n\n - it lets bisect_common() use the 'revs', which is allowed to\n smudge the flag bits of commit objects.\n\n - it uses clear_commit_marks_many() to clear the flags of the\n commits whose flag bits may have been smudged and their\n ancestors, recursively. In order to use as the starting points,\n the original array of commit rev[] that started the revision\n traversal is used.\n"},{"id":"392643","messageId":"nycvar.QRO.7.76.6.2002272239330.46@tvgsbejvaqbjf.bet","threadId":"52890","inReplyTo":"20200226101429.81327-7-mirucam@gmail.com","subject":"Re: [PATCH 06/10] bisect--helper: reimplement `bisect_autostart` shell function in C","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-02-27T21:40:47Z","receivedAt":"2020-02-27T21:40:53Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Wed, 26 Feb 2020, Miriam Rubio wrote:\n\n> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\n> index c3bb936a40..f9b04bee23 100644\n> --- a/builtin/bisect--helper.c\n> +++ b/builtin/bisect--helper.c\n> @@ -56,6 +57,8 @@ static void set_terms(struct bisect_terms *terms, const char *bad,\n> static const char vocab_bad[] = \"bad|new\";\n> static const char vocab_good[] = \"good|old\";\n>\n> +static int bisect_autostart(struct bisect_terms *terms);\n> +\n\nCan we move the definition of said function here? If so, we can save\nourselves the forward-declaration of it.\n\n> /*\n> * Check whether the string `term` belongs to the set of strings\n> * included in the variable arguments.\n> @@ -555,6 +558,7 @@ static enum bisect_error bisect_next(struct bisect_terms *terms, const char *pre\n> \tint no_checkout;\n> \tenum bisect_error res;\n>\n> +\tbisect_autostart(terms);\n> \tif (bisect_next_check(terms, terms->term_good))\n> \t\treturn BISECT_FAILED;\n>\n> @@ -803,6 +807,32 @@ static enum bisect_error bisect_start(struct bisect_terms *terms, int no_checkou\n> \treturn res;\n> }\n>\n> +static int bisect_autostart(struct bisect_terms *terms)\n> +{\n> +\tif (is_empty_or_missing_file(git_path_bisect_start())) {\n\nAs a rule, we do prefer to handle early exits first. In other words, I\nwould suggest starting this function by:\n\n\tif (!is_empty_or_missing_file(git_path_bisect_start()))\n\t\treturn BISECT_OK;\n\nThat way, not only can you save on indentation, but it also frees one slot\nof the readers' working memory.\n\n(Of course you would still need to ove the declarations before this\nstatement.)\n\n> +\t\tconst char *yesno;\n> +\t\tconst char *argv[] = {NULL};\n\nPlease separate declarations and statements with an empty line. I would\nalso prefer spaces around the curly brackets, i.e. `{ NULL }` instead of\n`{NULL}`.\n\n> +\t\tfprintf(stderr, _(\"You need to start by \\\"git bisect \"\n> +\t\t\t\t \"start\\\"\\n\"));\n> +\n> +\t\tif (!isatty(STDIN_FILENO))\n> +\t\t\treturn 1;\n> +\n> +\t\t/*\n> +\t\t * TRANSLATORS: Make sure to include [Y] and [n] in your\n> +\t\t * translation. The program will only accept English input\n> +\t\t * at this point.\n> +\t\t */\n> +\t\tyesno = git_prompt(_(\"Do you want me to do it for you \"\n> +\t\t\t\t \"[Y/n]? \"), PROMPT_ECHO);\n> +\t\tif (starts_with(yesno, _(\"n\")) || starts_with(yesno, _(\"N\")))\n> +\t\t\treturn 1;\n> +\n> +\t\treturn bisect_start(terms, 0, argv, 0);\n\nDo we really need this `argv`, or can we pass `NULL` directly? We say that\nthere are zero arguments, after all, therefore `bisect_start()` should not\neven look at `argv`.\n\nThe rest of the patch looks good to me.\n\nThank you,\nDscho\n\n> +\t}\n> +\treturn BISECT_OK;\n> +}\n> +\n> int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n> {\n> \tenum {\n> @@ -816,6 +846,7 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n> \t\tBISECT_START,\n> \t\tBISECT_NEXT,\n> \t\tBISECT_AUTO_NEXT,\n> +\t\tBISECT_AUTOSTART,\n> \t} cmdmode = 0;\n> \tint no_checkout = 0, res = 0, nolog = 0;\n> \tstruct option options[] = {\n> @@ -839,6 +870,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n> \t\t\t N_(\"find the next bisection commit\"), BISECT_NEXT),\n> \t\tOPT_CMDMODE(0, \"bisect-auto-next\", &cmdmode,\n> \t\t\t N_(\"verify the next bisection state then checkout the next bisection commit\"), BISECT_AUTO_NEXT),\n> +\t\tOPT_CMDMODE(0, \"bisect-autostart\", &cmdmode,\n> +\t\t\t N_(\"start the bisection if BISECT_START is empty or missing\"), BISECT_AUTOSTART),\n> \t\tOPT_BOOL(0, \"no-checkout\", &no_checkout,\n> \t\t\t N_(\"update BISECT_HEAD instead of checking out the current commit\")),\n> \t\tOPT_BOOL(0, \"no-log\", &nolog,\n> @@ -905,6 +938,12 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n> \t\tget_terms(&terms);\n> \t\tres = bisect_auto_next(&terms, prefix);\n> \t\tbreak;\n> +\tcase BISECT_AUTOSTART:\n> +\t\tif (argc)\n> +\t\t\treturn error(_(\"--bisect-autostart requires 0 arguments\"));\n> +\t\tset_terms(&terms, \"bad\", \"good\");\n> +\t\tres = bisect_autostart(&terms);\n> +\t\tbreak;\n> \tdefault:\n> \t\treturn error(\"BUG: unknown subcommand '%d'\", cmdmode);\n> \t}\n> diff --git a/git-bisect.sh b/git-bisect.sh\n> index 166f6a64dd..049ffacdff 100755\n> --- a/git-bisect.sh\n> +++ b/git-bisect.sh\n> @@ -49,27 +49,6 @@ bisect_head()\n> \tfi\n> }\n>\n> -bisect_autostart() {\n> -\ttest -s \"$GIT_DIR/BISECT_START\" || {\n> -\t\tgettextln \"You need to start by \\\"git bisect start\\\"\" >&2\n> -\t\tif test -t 0\n> -\t\tthen\n> -\t\t\t# TRANSLATORS: Make sure to include [Y] and [n] in your\n> -\t\t\t# translation. The program will only accept English input\n> -\t\t\t# at this point.\n> -\t\t\tgettext \"Do you want me to do it for you [Y/n]? \" >&2\n> -\t\t\tread yesno\n> -\t\t\tcase \"$yesno\" in\n> -\t\t\t[Nn]*)\n> -\t\t\t\texit ;;\n> -\t\t\tesac\n> -\t\t\tgit bisect--helper --bisect-start\n> -\t\telse\n> -\t\t\texit 1\n> -\t\tfi\n> -\t}\n> -}\n> -\n> bisect_skip() {\n> \tall=''\n> \tfor arg in \"$@\"\n> @@ -86,7 +65,7 @@ bisect_skip() {\n> }\n>\n> bisect_state() {\n> -\tbisect_autostart\n> +\tgit bisect--helper --bisect-autostart\n> \tstate=$1\n> \tgit bisect--helper --check-and-set-terms $state $TERM_GOOD $TERM_BAD || exit\n> \tget_terms\n> --\n> 2.25.0\n>\n>\n"},{"id":"392647","messageId":"nycvar.QRO.7.76.6.2002272244150.9783@tvgsbejvaqbjf.bet","threadId":"52890","inReplyTo":"20200226101429.81327-8-mirucam@gmail.com","subject":"Re: [PATCH 07/10] bisect--helper: reimplement `bisect_state` & `bisect_head` shell functions in C","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-02-27T23:12:09Z","receivedAt":"2020-02-27T23:12:17Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Wed, 26 Feb 2020, Miriam Rubio wrote:\n\n> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\n> index f9b04bee23..fedcad4d9b 100644\n> --- a/builtin/bisect--helper.c\n> +++ b/builtin/bisect--helper.c\n> @@ -833,6 +835,74 @@ static int bisect_autostart(struct bisect_terms *terms)\n> \treturn BISECT_OK;\n> }\n>\n> +static char *bisect_head(void)\n> +{\n> +\tif (is_empty_or_missing_file(git_path_bisect_head()))\n> +\t\treturn \"HEAD\";\n> +\telse\n> +\t\treturn \"BISECT_HEAD\";\n\nIt is relatively common, and makes it easier to read at least to me, to\nomit the `else` here: the conditional `return` already leaves the function\nearly.\n\n> +}\n> +\n> +static enum bisect_error bisect_state(struct bisect_terms *terms, const char **argv,\n> +\t\t\tint argc)\n> +{\n> +\tconst char *state = argv[0];\n> +\n> +\tif (check_and_set_terms(terms, state))\n> +\t\treturn BISECT_FAILED;\n> +\n> +\tif (!argc)\n> +\t\treturn error(_(\"Please call `--bisect-state` with at least one argument\"));\n> +\n> +\tif (argc == 1 && one_of(state, terms->term_good,\n> +\t terms->term_bad, \"skip\", NULL)) {\n> +\t\tconst char *bisected_head = xstrdup(bisect_head());\n\nDo we really want to `strdup()` it? That would leak memory, no? And I\ndon't think that we need this to be allocated anywhere. It's not like we\nare trying to modify the characters or that we need to take custody of the\nstring: `bisect_head()` will return a static, immutable string.\n\nBut then, it is not very natural for C code to let `bisect_head()` return\na string. Why not return the already-parsed object ID? That would also\nmake that function more robust: instead of expecting `BISECT_HEAD` to be\nstored in the file `BISECT_HEAD` in the `.git/` directory, you could\nsimply call `get_oid(\"BISECT_HEAD\", &oid)` and if that fails, fall back to\ndo the same with `HEAD`. That way, you tap into the refs machinery and the\nbisect code would be less dependent on implementation details.\n\n> +\t\tconst char *hex[1];\n> +\t\tstruct object_id oid;\n> +\n> +\t\tif (get_oid(bisected_head, &oid))\n> +\t\t\treturn error(_(\"Bad rev input: %s\"), bisected_head);\n> +\t\tif (bisect_write(state, oid_to_hex(&oid), terms, 0))\n> +\t\t\treturn BISECT_FAILED;\n> +\n> +\t\t*hex = xstrdup(oid_to_hex(&oid));\n> +\t\tcheck_expected_revs(hex, 1);\n\nHmm. Do we expect `check_expected_revs()` to modify what is passed into\nit? If not, we could probably simplify this a lot (and avoid another\nmemory leak) by something like this:\n\n\t\tconst char *hex;\n\n\t\t[...]\n\t\thex = oid_to_hex(&oid);\n\t\tcheck_expected_revs(&hex, 1);\n\nHowever, taking a step back, I wonder whether it makes sense for\n`check_expected_revs()` to accept `const char **revs` instead of `struct\nobject_id *oids`.\n\nLooking at the definition of `check_expected_revs()`, I would think that\nit would be better to pass object IDs in, rather than strings:\n\nstatic int is_expected_rev(const char *expected_hex)\n{\n struct strbuf actual_hex = STRBUF_INIT;\n int res = 0;\n if (strbuf_read_file(&actual_hex, git_path_bisect_expected_rev(), 0) >= 40) {\n strbuf_trim(&actual_hex);\n res = !strcmp(actual_hex.buf, expected_hex);\n }\n strbuf_release(&actual_hex);\n return res;\n}\n\nstatic void check_expected_revs(const char **revs, int rev_nr)\n{\n int i;\n\n for (i = 0; i < rev_nr; i++) {\n if (!is_expected_rev(revs[i])) {\n unlink_or_warn(git_path_bisect_ancestors_ok());\n unlink_or_warn(git_path_bisect_expected_rev());\n }\n }\n}\n\nThere are a couple of observation that immediately come to my mind:\n\n- Reading the bisect_expected_rev file for each rev, only to immediately\n release it before the next iteration, is wasteful\n\n- We can break out of the loop immediately once `is_expected_rev()`\n returns 0 because we are then unlinking the very file that\n `is_expected_rev()` checks against.\n\n- As I suspected above, a much cleaner interface would use `struct\n object_id **revs`.\n\n- Keeping the code of `is_expected_rev()` separate from\n `check_expected_revs()` does not make the code more readable for me, but\n actually less readable instead.\n\n- Why are we hard-coding the 40? At this time and age, we've _got_ to use\n `the_hash_algo->hexsz` instead.\n\nIn other words, I would do something like this instead:\n\nstatic void check_expected_revs(struct object_id **revs, int rev_nr)\n{\n\tstruct object_id expected;\n\tstruct strbuf buf = STRBUF_INIT;\n\tint i;\n\n\tif (!rev_nr)\n\t\treturn;\n\n if (strbuf_read_file(&buf, git_path_bisect_expected_rev(), 0)\n\t < the_hash_algo->hexsz ||\n\t get_oid_hex(buf.buf, &expected) < 0)\n\t\treturn; /* Ignore invalid file contents */\n\n\tfor (i = 0; i < rev_nr; i++)\n\t\tif (!oideq(revs[i], &expected)) {\n\t\t\t... unlink ...\n\t\t\treturn;\n\t\t}\n}\n\nHowever, taking _yet_ another step back, it seems a bit silly to compare\nmany revs (that need to be provided as 40-digit hex strings) to the\ncontents of a file that we expect to contain exactly one 40-digit hex\nstring. Surely, we do not want to take an arbitrary number of revisions,\nthen expect all of them to be identical to the _same_ rev, and otherwise\ndelete that file from where that rev came from?\n\nWe could even skip reading that file if two or more revisions were passed\nto `check_expected_revs()` unless _all_ of them are identical!\n\nSo let's look at the actual caller of this thing:\n\nbisect_state() {\n bisect_autostart\n state=$1\n git bisect--helper --check-and-set-terms $state $TERM_GOOD $TERM_BAD || exit\n get_terms\n case \"$#,$state\" in\n 0,*)\n die \"Please call 'bisect_state' with at least one argument.\" ;;\n 1,\"$TERM_BAD\"|1,\"$TERM_GOOD\"|1,skip)\n bisected_head=$(bisect_head)\n rev=$(git rev-parse --verify \"$bisected_head\") ||\n die \"$(eval_gettext \"Bad rev input: \\$bisected_head\")\"\n git bisect--helper --bisect-write \"$state\" \"$rev\" \"$TERM_GOOD\" \"$TERM_BAD\" || exit\n git bisect--helper --check-expected-revs \"$rev\" ;;\n 2,\"$TERM_BAD\"|*,\"$TERM_GOOD\"|*,skip)\n shift\n hash_list=''\n for rev in \"$@\"\n do\n sha=$(git rev-parse --verify \"$rev^{commit}\") ||\n die \"$(eval_gettext \"Bad rev input: \\$rev\")\"\n hash_list=\"$hash_list $sha\"\n done\n for rev in $hash_list\n do\n git bisect--helper --bisect-write \"$state\" \"$rev\" \"$TERM_GOOD\" \"$TERM_BAD\" || exit\n done\n git bisect--helper --check-expected-revs $hash_list ;;\n *,\"$TERM_BAD\")\n die \"$(eval_gettext \"'git bisect \\$TERM_BAD' can take only one argument.\")\" ;;\n *)\n usage ;;\n esac\n bisect_auto_next\n}\n\nSo there are actually two callers: the first one in the 1,* arm, the\nsecond one in the 2,* arm. The first one is easy: it only passes a single\nrev to `git bisect--helper --check-expected-revs`.\n\nThe second caller indeed can pass multiple arguments to\n`check_expected_revs()`, but only if the first argument was not `bad`.\n\nBut this whole shell function is super ugly, and I do not believe that\nwe're served well by translating it verbatim to even super uglier C code.\n\nI propose to instead rewrite it substantially, in the following way:\n\n- Handle the `argc == 0` case first thing: in this case, die, complaining\n that we need at least one argument.\n\n (You already did that. Although `state` was already assigned to\n `argv[0]` at that stage, and I think we don't really want that, just in\n case that _one_ call chain ends up using `NULL, 0` for `argv, argc`.)\n\n- Next, assign `state = argv[0]` and verify that it is one of `good`,\n `bad` or `skip`, otherwise erroring out.\n\n- At this stage, we should do the equivalent of a `shift`: `argv++;\n argc--;`\n\n- Now, we probably need to special-case the `bad` case and verify that\n `argc <= 1`, otherwise error out.\n\n Side note: I do _not_ understand this restriction. Why shouldn't it be\n valid to pass several revisions to `git bisect bad ...`?\n\n- At this point, if `argc == 0`, we should parse `bisect_head()` into a\n `struct object_id` and use that as `revs`, setting `argc = 0`.\n Otherwise, if `argc > 0`, we should populate a `struct object_array` and\n use that as `revs`.\n\n- And finally, we should iterate over these revs and call `bisect_write()`\n with them, and in the same loop we can also take care of the expected\n rev. It does not make sense to keep those separate from one another\n (i.e. to have _two_ loops).\n\nThat would be a much more natural, and readable, flow, at least to me.\n\n> +\t}\n> +\n> +\tif ((argc == 2 && !strcmp(state, terms->term_bad)) ||\n> +\t\t\tone_of(state, terms->term_good, \"skip\", NULL)) {\n> +\t\tint i;\n> +\t\tstruct string_list hex = STRING_LIST_INIT_DUP;\n> +\n> +\t\tfor (i = 1; i < argc; i++) {\n> +\t\t\tstruct object_id oid;\n> +\n> +\t\t\tif (get_oid(argv[i], &oid)) {\n\nThe original does `git rev-parse --verify \"$rev^{commit}\"` here, so I\nthink we really want `get_oid_commit()` here, not just `get_oid()`.\n\n> +\t\t\t\tstring_list_clear(&hex, 0);\n> +\t\t\t\treturn error(_(\"Bad rev input: %s\"), argv[i]);\n> +\t\t\t}\n> +\t\t\tstring_list_append(&hex, oid_to_hex(&oid));\n\nConverting the parsed object ID to hex and storing _that_ as a string and\nthrowing away the parsed object ID looks not very much like C, but much\nmore like shell script. Let's not do that. Let's store the parsed data in\na proper `struct object_array`, and only resort to `oid_to_hex()` when we\n_have_ to.\n\n> +\t\t}\n> +\t\tfor (i = 0; i < hex.nr; i++) {\n> +\t\t\tconst char **hex_string = (const char **) &hex.items[i].string;\n> +\t\t\tif (bisect_write(state, *hex_string, terms, 0)) {\n> +\t\t\t\tstring_list_clear(&hex, 0);\n> +\t\t\t\treturn BISECT_FAILED;\n> +\t\t\t}\n> +\t\t\tcheck_expected_revs(hex_string, 1);\n> +\t\tstring_list_clear(&hex, 0);\n> +\t\treturn bisect_auto_next(terms, NULL);\n> +\t}\n> +\n> +\tif (!strcmp(state, terms->term_bad))\n> +\t\treturn error(_(\"'git bisect %s' can take only one argument.\"),\n> +\t\t terms->term_bad);\n\nThat is an awful late time in the function for a reviewer to read about a\ncondition that should be an early error.\n\n> +\n> +\treturn BISECT_FAILED;\n> +}\n> +\n> int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n> {\n> \tenum {\n> [...]\n> @@ -944,6 +1017,13 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n> \t\tset_terms(&terms, \"bad\", \"good\");\n> \t\tres = bisect_autostart(&terms);\n> \t\tbreak;\n> +\tcase BISECT_STATE:\n> +\t\tif (!argc)\n> +\t\t\treturn error(_(\"--bisect-state requires at least one revision\"));\n\nWait, we _already_ test for that here? Then we do not need the very same\ncheck in `bisect_state()`.\n\nI'd rather have it in `bisect_state()`.\n\nBesides, we're in a `cmd_*()` function, so we cannot return `error()`\nbecause that translates to `-1` and `cmd_*()` functions need to return\nexit codes, i.e. numbers between 0 and 127.\n\n> +\t\tset_terms(&terms, \"bad\", \"good\");\n> +\t\tget_terms(&terms);\n> +\t\tres = bisect_state(&terms, argv, argc);\n> +\t\tbreak;\n> \tdefault:\n> \t\treturn error(\"BUG: unknown subcommand '%d'\", cmdmode);\n\nGranted, this incorrect `return error()` in a `cmd_*()` function was there\nbefore your patch. It is just as incorrect, though.\n\n> \t}\n> diff --git a/git-bisect.sh b/git-bisect.sh\n> index 049ffacdff..7f10187055 100755\n> --- a/git-bisect.sh\n> +++ b/git-bisect.sh\n> [...]\n> @@ -185,8 +139,7 @@ exit code \\$res from '\\$command' is < 0 or >= 128\" >&2\n> \t\t\tstate=\"$TERM_GOOD\"\n> \t\tfi\n>\n> -\t\t# We have to use a subshell because \"bisect_state\" can exit.\n> -\t\t( bisect_state $state >\"$GIT_DIR/BISECT_RUN\" )\n> +\t\t( git bisect--helper --bisect-state $state >\"$GIT_DIR/BISECT_RUN\" )\n\nThis subshell looks pointless. Before, we wanted to be able to catch the\n`die` in `bisect_state`. But since the `bisect--helper` cannot cause the\n_shell script_ to exit, we can just drop the subshell here.\n\nPhew, that was quite a lot, and the review of this patch also took two\nhours. I'll have to stop here, once again, and hope that I will find time\nsoon to continue the review.\n\nI left you quite a bit of work here, but I hope that all of my suggestions\nare clear enough to act on. If not, please holler.\n\nCiao,\nDscho\n\n> \t\tres=$?\n>\n> \t\tcat \"$GIT_DIR/BISECT_RUN\"\n> @@ -201,7 +154,7 @@ exit code \\$res from '\\$command' is < 0 or >= 128\" >&2\n> \t\tif [ $res -ne 0 ]\n> \t\tthen\n> \t\t\teval_gettextln \"bisect run failed:\n> -'bisect_state \\$state' exited with error code \\$res\" >&2\n> +'git bisect--helper --bisect-state \\$state' exited with error code \\$res\" >&2\n> \t\t\texit $res\n> \t\tfi\n>\n> @@ -242,7 +195,7 @@ case \"$#\" in\n> \tstart)\n> \t\tgit bisect--helper --bisect-start \"$@\" ;;\n> \tbad|good|new|old|\"$TERM_BAD\"|\"$TERM_GOOD\")\n> -\t\tbisect_state \"$cmd\" \"$@\" ;;\n> +\t\tgit bisect--helper --bisect-state \"$cmd\" \"$@\" ;;\n> \tskip)\n> \t\tbisect_skip \"$@\" ;;\n> \tnext)\n> --\n> 2.25.0\n>\n>\n"},{"id":"392914","messageId":"CAN7CjDCb3Bo-reyCZBxMuxX7ecCRLo6XaZHUE5fKGnQav9g_xQ@mail.gmail.com","threadId":"52890","inReplyTo":"xmqqwo88f0do.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 02/10] bisect--helper: reimplement `bisect_next` and `bisect_auto_next` shell functions in C","fromName":"Miriam R.","fromEmail":"mirucam@gmail.com","sentAt":"2020-03-06T18:19:09Z","receivedAt":"2020-03-06T18:19:24Z","isPatch":true,"sender":{"key":"mirucam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56339109?v=4"},"body":"Hi Junio,\n\nEl jue., 27 feb. 2020 a las 17:41, Junio C Hamano\n() escribió:\n>\n> \"Miriam R.\" writes:\n>\n> >> It would be surprising if this code were correct. It may be that it\n> >> happens to (appear to) work because parents of the commit hasn't\n> >> been painted and calling the helper only clears the mark from the\n> >> commit (so we won't see repeated \"painting down to the root commit\")\n> >> in which case this might be an extremely expensive looking variant\n> >> of\n> >>\n> >> commit->object.flags &= ~ALL_REV_FLAGS;\n> >>\n> >> that only confuses the readers.\n> >>\n> >> Even then, I think by clearing bits like SEEN from commit, it breaks\n> >> the revision traversal machinery---for example, doesn't this mean\n> >> that the commit we just processed can be re-visited by\n> >> get_revision() without deduping in a history with forks and merges?\n> >>\n> >> Has this been shown to any of your mentors before sending it to the\n> >> list?\n> >\n> > Adding clear_commit_marks() was a suggestion of a previous review of this patch:\n> > https://public-inbox.org/git/nycvar.QRO.7.76.6.2001301619340.46@tvgsbejvaqbjf.bet/\n> >\n> > And of course, my mentor always reviews my patch series before sending.\n>\n> OK, I just didn't know how you and your mentors work. Some leave\n> their door open for mentees and help them when asked but otherwise\n> act as an ordinary reviewer who somehow prioritises reviewing the\n> work by their mentees. So your mentors may be a better source of\n> information why this piece of code, which I still do not know how it\n> could be correct, is supposed to work. Good.\n>\n> After reading the above URL, I think you may have misread the\n> suggestion you were given. Resetting using clear_commit_marks() may\n> be necessary, but you do so when you finished walking so that you\n> can do unrelated revision walk later. For that, you clear the flag\n> bits after the while() loop that asks get_revision() to yield\n> commits are done, using the initial set of commits that you used to\n> start iteration.\n>\n> That is how bisect.c::check_ancestors() work, that is\n>\n> - it initializes a rev_info 'revs' from an array of commit rev[]\n>\n> - it lets bisect_common() use the 'revs', which is allowed to\n> smudge the flag bits of commit objects.\n>\n> - it uses clear_commit_marks_many() to clear the flags of the\n> commits whose flag bits may have been smudged and their\n> ancestors, recursively. In order to use as the starting points,\n> the original array of commit rev[] that started the revision\n> traversal is used.\n\nThank you for your explanation.\n\nTo my understanding, it looks like calling reset_revision_walk() after\nthe while() loop should be enough. Am I right or am I missing\nsomething?\n"},{"id":"392923","messageId":"xmqqimjh2tjq.fsf@gitster-ct.c.googlers.com","threadId":"52890","inReplyTo":"CAN7CjDCb3Bo-reyCZBxMuxX7ecCRLo6XaZHUE5fKGnQav9g_xQ@mail.gmail.com","subject":"Re: [PATCH 02/10] bisect--helper: reimplement `bisect_next` and `bisect_auto_next` shell functions in C","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-03-06T19:05:13Z","receivedAt":"2020-03-06T19:05:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Miriam R.\" writes:\n\n> To my understanding, it looks like calling reset_revision_walk() after\n> the while() loop should be enough. Am I right or am I missing\n> something?\n\nI suspect that reset_revision_walk() may be too-big a hammer, as it\nclears everything, regardless of the reason why the flag bits were\nset. On the other hand, the clearly strategy that uses\nclear_commit_marks() is to clear only the flag bits that were set\nduring the previous revision walk from only the commits that were\nwalked during the previous revision walk.\n\nI offhand do not know what flag bits on what objects that were not\ninvolved in the previous revision walk are still necessary at the\npoint of the call made by the caller (that's a question for your\nmentors who volunteered their expertise on the program in question),\nso if there isn't any, reset_revision_walk() may be an easy way out.\nI just do not know if it clears too much to break the code that\ncomes after the function returns.\n\nThanks.\n"},{"id":"393068","messageId":"CAP8UFD2G-xhmRTNmicoA7vfRyttVSCU9Giribs6xykvkDZ0_8w@mail.gmail.com","threadId":"52890","inReplyTo":"xmqqimjh2tjq.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 02/10] bisect--helper: reimplement `bisect_next` and `bisect_auto_next` shell functions in C","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2020-03-11T18:58:40Z","receivedAt":"2020-03-11T18:58:56Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Fri, Mar 6, 2020 at 8:06 PM Junio C Hamano wrote:\n>\n> \"Miriam R.\" writes:\n>\n> > To my understanding, it looks like calling reset_revision_walk() after\n> > the while() loop should be enough. Am I right or am I missing\n> > something?\n>\n> I suspect that reset_revision_walk() may be too-big a hammer, as it\n> clears everything, regardless of the reason why the flag bits were\n> set. On the other hand, the clearly strategy that uses\n> clear_commit_marks() is to clear only the flag bits that were set\n> during the previous revision walk from only the commits that were\n> walked during the previous revision walk.\n>\n> I offhand do not know what flag bits on what objects that were not\n> involved in the previous revision walk are still necessary at the\n> point of the call made by the caller (that's a question for your\n> mentors who volunteered their expertise on the program in question),\n> so if there isn't any, reset_revision_walk() may be an easy way out.\n> I just do not know if it clears too much to break the code that\n> comes after the function returns.\n\nprocess_skipped_commits(), the function that does this revision walk,\nis called by bisect_skipped_commits() to print the possible first bad\ncommits when there are only skipped commits left to test and we\ntherefore cannot bisect more. This can be seen in bisect_next() which\ndoes basically the following:\n\nbisect_next()\n{\n ...\n\n /* Perform all bisection computation, display and checkout */\n res = bisect_next_all(the_repository, prefix, no_checkout);\n\n if (res == BISECT_INTERNAL_SUCCESS_1ST_BAD_FOUND) {\n ...\n } else if (res == BISECT_ONLY_SKIPPED_LEFT) {\n res = bisect_skipped_commits(terms);\n return res ? res : BISECT_ONLY_SKIPPED_LEFT;\n }\n return res;\n}\n\nBISECT_ONLY_SKIPPED_LEFT is an error code (-2) so bisect_next() will\nalways return an error in this case.\n\nThis means that the revision walk in process_skipped_commits() is very\nlikely to be the last revision walk performed by the command. So my\nopinion is that not clearing anything at the end of that revision walk\nis fine.\n\nIf we are worried about what could happen one day, when people might\nbe interested in actually doing another revision walk after this one,\nthen as we don't know what they will want to do and might be\ninterested in, cleaning everything with reset_revision_walk() might be\nthe safest thing to do and is probably cheap enough that it's ok to\nuse it right now.\n\nThanks for your review,\nChristian.\n"}]}