{"thread":{"id":"53050","subject":"[PATCH v2 01/11] bisect--helper: fix `cmd_*()` function switch default return","startedAt":"2020-03-21T16:13:52Z","lastAt":"2020-04-23T07:18:22Z","messageCount":18,"participants":["Miriam Rubio","Junio C Hamano","Christian Couder","Miriam R."],"isPatch":true,"patchVersion":2,"patchTotal":11},"messages":[{"id":"393616","messageId":"20200321161020.22817-2-mirucam@gmail.com","threadId":"53050","inReplyTo":"20200321161020.22817-1-mirucam@gmail.com","subject":"[PATCH v2 01/11] bisect--helper: fix `cmd_*()` function switch default return","fromName":"Miriam Rubio","fromEmail":"mirucam@gmail.com","sentAt":"2020-03-21T16:10:10Z","receivedAt":"2020-03-21T16:13:52Z","isPatch":true,"sender":{"key":"mirucam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56339109?v=4"},"body":"In a `cmd_*()` function, return `error()` cannot be used\nbecause that translates to `-1` and `cmd_*()` functions need\nto return exit codes.\n\nLet's fix switch default return.\n\nMentored-by: Christian Couder \nMentored-by: Johannes Schindelin \nSigned-off-by: Miriam Rubio \n---\n builtin/bisect--helper.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\nindex c1c40b516d..1f81cff1d8 100644\n--- a/builtin/bisect--helper.c\n+++ b/builtin/bisect--helper.c\n@@ -711,7 +711,7 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n \t\tres = bisect_start(&terms, no_checkout, argv, argc);\n \t\tbreak;\n \tdefault:\n-\t\treturn error(\"BUG: unknown subcommand '%d'\", cmdmode);\n+\t\tres = error(_(\"BUG: unknown subcommand.\"));\n \t}\n \tfree_terms(&terms);\n \n-- \n2.25.0\n\n"},{"id":"393617","messageId":"20200321161020.22817-1-mirucam@gmail.com","threadId":"53050","inReplyTo":null,"subject":"[PATCH v2 00/11] Finish converting git bisect to C part 2","fromName":"Miriam Rubio","fromEmail":"mirucam@gmail.com","sentAt":"2020-03-21T16:10:09Z","receivedAt":"2020-03-21T16:13:53Z","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 is formed by reimplementations of some `git bisect`\nsubcommands and removal of other temporary subcommands.\n\nThese patch series emails were generated from:\nhttps://gitlab.com/mirucam/git/commits/git-bisect-part2-v2.\n\nI would like to thank Junio Hamano and Johannes Schindelin for their\nreviews and suggestions.\n\n--- Changes since v1 Finish converting git bisect to C part 2 patch series ---\n\nGeneral changes\n---------------\n\n* Rebase on master branch: 98cedd0233 (Merge https://github.com/prati0100/git-gui, 2020-03-19)\n\nSpecific changes\n----------------\n\n[1/11] bisect--helper: fix `cmd_*()` function switch default return\n\n* New patch that fixes a return `error()` in a `cmd_*()` function.\n\n[2/11] bisect--helper: introduce new `write_in_file()` function\n\n* Improve commit message.\n* Change `write_in_file()` function to avoid extra allocation.\n* Adapt new `write_in_file()` call in `write_terms()`.\n\n--\n\n[3/11] bisect--helper: reimplement `bisect_next` and `bisect_auto_next` shell functions in C\n\n* Adapt new `write_in_file()` call in `bisect_successful()`.\n* Rewrite `register_good_ref()`\n* Adapt `prepare_rev_argv()` to new `register_good_ref()`.\n* Use `reset_revision_walk()` to reset flags in `process_skipped_commits()`.\n* Add code comment.\n\n--\n[7/11] bisect--helper: reimplement `bisect_autostart` shell function in C\n\n* Handle exit first in `bisect_autostart()`.\n* Add spaces around a curly brackets.\n* Pass NULL instead of argv to `bisect_start()`.\n\nNote to previous reviewers:`bisect_autostart` cannot move to forward-declaration \nlocation, it would imply many functions relocations.\n\n--\n\n[8/11] bisect--helper: reimplement `bisect_state` & `bisect_head` shell functions in C\n\n* Change `bisect_state()` logic to accept `struct object_id *` as input.\n* Rewrite `bisect_state()` function following reviewer suggestions.\n* Remove subshell use in `git-bisect.sh`.\n\nNote to previous reviewers: Refactor of check_expected_revs() function \nwill be in a separate patch in the next patch series (part 3).\n\n\nMiriam Rubio (2):\n bisect--helper: fix `cmd_*()` function switch default return\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 | 377 +++++++++++++++++++++++++++++++++------\n git-bisect.sh | 145 +--------------\n 3 files changed, 346 insertions(+), 187 deletions(-)\n\n-- \n2.25.0\n\n"},{"id":"393618","messageId":"20200321161020.22817-3-mirucam@gmail.com","threadId":"53050","inReplyTo":"20200321161020.22817-1-mirucam@gmail.com","subject":"[PATCH v2 02/11] bisect--helper: introduce new `write_in_file()` function","fromName":"Miriam Rubio","fromEmail":"mirucam@gmail.com","sentAt":"2020-03-21T16:10:11Z","receivedAt":"2020-03-21T16:13:54Z","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 helper will be used in later steps and makes the code\nsimpler and easier to understand.\n\nMentored-by: Christian Couder \nMentored-by: Johannes Schindelin \nSigned-off-by: Miriam Rubio \n---\n builtin/bisect--helper.c | 27 +++++++++++++++++++++------\n 1 file changed, 21 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\nindex 1f81cff1d8..e949ea74e2 100644\n--- a/builtin/bisect--helper.c\n+++ b/builtin/bisect--helper.c\n@@ -74,6 +74,26 @@ static int one_of(const char *term, ...)\n \treturn res;\n }\n \n+static int write_in_file(const char *filepath, const char *mode, const char *format,...)\n+{\n+\tFILE *fp = NULL;\n+\tva_list args;\n+\tint res = 0;\n+\n+\tif (!strcmp(mode, \"a\") && !strcmp(mode, \"w\"))\n+\t\treturn error_errno(_(\"wrong writing mode '%s'\"), mode);\n+\tfp = fopen(filepath, mode);\n+\tif (!fp)\n+\t\treturn error_errno(_(\"could not open file '%s'\"), filepath);\n+\tva_start(args, format);\n+\tres = vfprintf(fp, format, args);\n+\tva_end(args);\n+\tif (!res)\n+\t\treturn error_errno(_(\"could not write to file '%s'\"), filepath);\n+\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 +124,6 @@ static int check_term_format(const char *term, const char *orig_term)\n \n static int write_terms(const char *bad, const char *good)\n {\n-\tFILE *fp = NULL;\n \tint res;\n \n \tif (!strcmp(bad, good))\n@@ -113,12 +132,8 @@ 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(), \"w\", \"%s\\n%s\\n\", bad, good);\n \n-\tres = fprintf(fp, \"%s\\n%s\\n\", bad, good);\n-\tres |= fclose(fp);\n \treturn (res < 0) ? -1 : 0;\n }\n \n-- \n2.25.0\n\n"},{"id":"393619","messageId":"20200321161020.22817-4-mirucam@gmail.com","threadId":"53050","inReplyTo":"20200321161020.22817-1-mirucam@gmail.com","subject":"[PATCH v2 03/11] bisect--helper: reimplement `bisect_next` and `bisect_auto_next` shell functions in C","fromName":"Miriam Rubio","fromEmail":"mirucam@gmail.com","sentAt":"2020-03-21T16:10:12Z","receivedAt":"2020-03-21T16:13:56Z","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 | 166 ++++++++++++++++++++++++++++++++++++++-\n git-bisect.sh | 47 +----------\n 3 files changed, 180 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 e949ea74e2..447e9e75db 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@@ -436,6 +439,149 @@ 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 argv_array *rev_argv = cb_data;\n+\n+\targv_array_push(rev_argv, 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+\tchar *term_good = xstrfmt(\"%s-*\", terms->term_good);\n+\n+\targv_array_pushl(rev_argv, \"skipped_commits\", \"refs/bisect/bad\", \"--not\", NULL);\n+\tfor_each_glob_ref_in(register_good_ref, term_good, \"refs/bisect/\", rev_argv);\n+\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 * sets up a revision walk.\n+\t */\n+\treset_revision_walk();\n+\tinit_revisions(revs, NULL);\n+\trev_argv.argc = setup_revisions(rev_argv.argc, rev_argv.argv, revs, NULL);\n+\tif (prepare_revision_walk(revs))\n+\t\tres = error(_(\"revision walk setup failed\\n\"));\n+\n+\targv_array_clear(&rev_argv);\n+\treturn res;\n+}\n+\n+static int process_skipped_commits(FILE *fp, struct bisect_terms *terms, struct rev_info *revs)\n+{\n+\tstruct commit *commit;\n+\tstruct pretty_print_context pp = {0};\n+\n+\tif (fprintf(fp, \"# only skipped commits left to test\\n\") < 1)\n+\t\treturn -1;\n+\n+\twhile ((commit = get_revision(revs)) != NULL) {\n+\t\tstruct strbuf commit_name = STRBUF_INIT;\n+\t\tformat_commit_message(commit, \"%s\",\n+\t\t\t\t &commit_name, &pp);\n+\t\tfprintf(fp, \"# possible first %s commit: [%s] %s\\n\",\n+\t\t\tterms->term_bad, oid_to_hex(&commit->object.oid),\n+\t\t\tcommit_name.buf);\n+\t\tstrbuf_release(&commit_name);\n+\t}\n+\n+\t/*\n+\t * Reset the flags used by revision walks in case\n+\t * there is another revision walk after this one.\n+\t */\n+\treset_revision_walk();\n+\n+\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\",terms->term_bad);\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+\tres = write_in_file(git_path_bisect_log(), \"a\", \"# first %s commit: [%s] %s\\n\",\n+\t\t\t terms->term_bad, oid_to_hex(&oid),\n+\t\t\t commit_name.buf);\n+\n+\tstrbuf_release(&commit_name);\n+\tfree(bad_ref);\n+\treturn res;\n+}\n+\n+static enum bisect_error bisect_next(struct bisect_terms *terms, const char *prefix)\n+{\n+\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@@ -640,7 +786,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@@ -664,6 +812,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@@ -725,6 +877,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\tres = error(_(\"BUG: unknown subcommand.\"));\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":"393620","messageId":"20200321161020.22817-6-mirucam@gmail.com","threadId":"53050","inReplyTo":"20200321161020.22817-1-mirucam@gmail.com","subject":"[PATCH v2 05/11] bisect--helper: retire `--bisect-clean-state` subcommand","fromName":"Miriam Rubio","fromEmail":"mirucam@gmail.com","sentAt":"2020-03-21T16:10:14Z","receivedAt":"2020-03-21T16:13:59Z","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 84aac08ced..0534adf216 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@@ -809,7 +808,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@@ -826,8 +824,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@@ -869,10 +865,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":"393621","messageId":"20200321161020.22817-5-mirucam@gmail.com","threadId":"53050","inReplyTo":"20200321161020.22817-1-mirucam@gmail.com","subject":"[PATCH v2 04/11] bisect--helper: finish porting `bisect_start()` to C","fromName":"Miriam Rubio","fromEmail":"mirucam@gmail.com","sentAt":"2020-03-21T16:10:13Z","receivedAt":"2020-03-21T16:13:59Z","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 447e9e75db..84aac08ced 100644\n--- a/builtin/bisect--helper.c\n+++ b/builtin/bisect--helper.c\n@@ -582,11 +582,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@@ -639,9 +640,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@@ -719,12 +723,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@@ -740,7 +744,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@@ -752,25 +756,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":"393622","messageId":"20200321161020.22817-7-mirucam@gmail.com","threadId":"53050","inReplyTo":"20200321161020.22817-1-mirucam@gmail.com","subject":"[PATCH v2 06/11] bisect--helper: retire `--next-all` subcommand","fromName":"Miriam Rubio","fromEmail":"mirucam@gmail.com","sentAt":"2020-03-21T16:10:15Z","receivedAt":"2020-03-21T16:14:01Z","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 0534adf216..086ab8e46b 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@@ -806,8 +805,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@@ -820,8 +818,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@@ -858,9 +854,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":"393623","messageId":"20200321161020.22817-11-mirucam@gmail.com","threadId":"53050","inReplyTo":"20200321161020.22817-1-mirucam@gmail.com","subject":"[PATCH v2 10/11] bisect--helper: retire `--write-terms` subcommand","fromName":"Miriam Rubio","fromEmail":"mirucam@gmail.com","sentAt":"2020-03-21T16:10:19Z","receivedAt":"2020-03-21T16:14:06Z","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 2b1d918497..8fdefb611b 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@@ -895,8 +894,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@@ -909,8 +907,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@@ -947,10 +943,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":"393624","messageId":"20200321161020.22817-10-mirucam@gmail.com","threadId":"53050","inReplyTo":"20200321161020.22817-1-mirucam@gmail.com","subject":"[PATCH v2 09/11] bisect--helper: retire `--check-expected-revs` subcommand","fromName":"Miriam Rubio","fromEmail":"mirucam@gmail.com","sentAt":"2020-03-21T16:10:18Z","receivedAt":"2020-03-21T16:14:06Z","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 6364bd82c0..2b1d918497 100644\n--- a/builtin/bisect--helper.c\n+++ b/builtin/bisect--helper.c\n@@ -896,7 +896,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@@ -912,8 +911,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@@ -954,9 +951,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":"393625","messageId":"20200321161020.22817-8-mirucam@gmail.com","threadId":"53050","inReplyTo":"20200321161020.22817-1-mirucam@gmail.com","subject":"[PATCH v2 07/11] bisect--helper: reimplement `bisect_autostart` shell function in C","fromName":"Miriam Rubio","fromEmail":"mirucam@gmail.com","sentAt":"2020-03-21T16:10:16Z","receivedAt":"2020-03-21T16:14:07Z","isPatch":true,"sender":{"key":"mirucam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56339109?v=4"},"body":"From: Pranit Bauva \n\nReimplement the `bisect_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 086ab8e46b..4496fd2228 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@@ -554,6 +557,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@@ -802,6 +806,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+\tconst char *yesno;\n+\n+\tif (!is_empty_or_missing_file(git_path_bisect_start()))\n+\t\treturn 0;\n+\n+\tfprintf(stderr, _(\"You need to start by \\\"git bisect \"\n+\t\t\t \"start\\\"\\n\"));\n+\n+\tif (!isatty(STDIN_FILENO))\n+\t\treturn 1;\n+\n+\t/*\n+\t * TRANSLATORS: Make sure to include [Y] and [n] in your\n+\t * translation. The program will only accept English input\n+\t * at this point.\n+\t */\n+\tyesno = git_prompt(_(\"Do you want me to do it for you \"\n+\t\t\t \"[Y/n]? \"), PROMPT_ECHO);\n+\tif (starts_with(yesno, _(\"n\")) || starts_with(yesno, _(\"N\")))\n+\t\treturn 1;\n+\n+\treturn bisect_start(terms, 0, NULL, 0);\n+}\n+\n int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n {\n \tenum {\n@@ -815,6 +845,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@@ -838,6 +869,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@@ -904,6 +937,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\tres = error(_(\"BUG: unknown subcommand.\"));\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":"393626","messageId":"20200321161020.22817-9-mirucam@gmail.com","threadId":"53050","inReplyTo":"20200321161020.22817-1-mirucam@gmail.com","subject":"[PATCH v2 08/11] bisect--helper: reimplement `bisect_state` & `bisect_head` shell functions in C","fromName":"Miriam Rubio","fromEmail":"mirucam@gmail.com","sentAt":"2020-03-21T16:10:17Z","receivedAt":"2020-03-21T16:14:08Z","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 | 68 ++++++++++++++++++++++++++++++++++++++++\n git-bisect.sh | 55 +++-----------------------------\n 2 files changed, 72 insertions(+), 51 deletions(-)\n\ndiff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\nindex 4496fd2228..6364bd82c0 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@@ -832,6 +834,64 @@ static int bisect_autostart(struct bisect_terms *terms)\n \treturn bisect_start(terms, 0, NULL, 0);\n }\n \n+static int bisect_head(struct object_id *oid)\n+{\n+\tif (is_empty_or_missing_file(git_path_bisect_head()))\n+\t\treturn get_oid(\"HEAD\", oid);\n+\n+\treturn get_oid(\"BISECT_HEAD\", oid);\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;\n+\tconst char *hex;\n+\tint i;\n+\tstruct oid_array revs = OID_ARRAY_INIT;\n+\tstruct object_id oid;\n+\n+\tif (!argc)\n+\t\treturn error(_(\"Please call `--bisect-state` with at least one argument\"));\n+\tstate = argv[0];\n+\tif (check_and_set_terms(terms, state) ||\n+\t !one_of(state, terms->term_good,terms->term_bad, \"skip\", NULL))\n+\t\treturn BISECT_FAILED;\n+\targv++;\n+\targc--;\n+\tif (!strcmp(state, terms->term_bad) && (argc > 1))\n+\t\treturn error(_(\"'git bisect %s' can take only one argument.\"),terms->term_bad);\n+\tif (argc == 0) {\n+\t\tif (bisect_head(&oid))\n+\t\t\treturn error(_(\"Bad bisect_head rev input\"));\n+\t\thex = oid_to_hex(&oid);\n+\t\tif (bisect_write(state, hex, terms, 0))\n+\t\t\treturn BISECT_FAILED;\n+\t\tcheck_expected_revs(&hex, 1);\n+\t\treturn bisect_auto_next(terms, NULL);\n+\t}\n+\n+\t/* Here argc > 0 */\n+\tfor (; argc; argc--, argv++) {\n+\t\tstruct object_id oid;\n+\t\tif (get_oid(*argv, &oid))\n+\t\t\treturn error(_(\"Bad rev input: %s\"), *argv);\n+\t\toid_array_append(&revs, &oid);\n+\t}\n+\n+\tfor (i = 0; i < revs.nr; i++) {\n+\t\thex = oid_to_hex(&revs.oid[i]);\n+\t\tif (bisect_write(state, hex, terms, 0)) {\n+\t\t\toid_array_clear(&revs);\n+\t\t\treturn BISECT_FAILED;\n+\t\t}\n+\t\tcheck_expected_revs(&hex, 1);\n+\t}\n+\n+\toid_array_clear(&revs);\n+\treturn bisect_auto_next(terms, NULL);\n+}\n+\n int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n {\n \tenum {\n@@ -846,6 +906,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@@ -871,6 +932,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@@ -943,6 +1006,11 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n \t\tset_terms(&terms, \"bad\", \"good\");\n \t\tres = bisect_autostart(&terms);\n \t\tbreak;\n+\tcase BISECT_STATE:\n+\t\tset_terms(&terms, \"bad\", \"good\");\n+\t\tget_terms(&terms);\n+\t\tres = bisect_state(&terms, argv, argc);\n+\t\tbreak;\n \tdefault:\n \t\tres = error(_(\"BUG: unknown subcommand.\"));\n \t}\ndiff --git a/git-bisect.sh b/git-bisect.sh\nindex 049ffacdff..2da0810b1a 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\tgit 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":"393627","messageId":"20200321161020.22817-12-mirucam@gmail.com","threadId":"53050","inReplyTo":"20200321161020.22817-1-mirucam@gmail.com","subject":"[PATCH v2 11/11] bisect--helper: retire `--bisect-autostart` subcommand","fromName":"Miriam Rubio","fromEmail":"mirucam@gmail.com","sentAt":"2020-03-21T16:10:20Z","receivedAt":"2020-03-21T16:14:09Z","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 8fdefb611b..d3ec926ae4 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@@ -902,7 +901,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@@ -923,8 +921,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@@ -986,12 +982,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\tset_terms(&terms, \"bad\", \"good\");\n \t\tget_terms(&terms);\n-- \n2.25.0\n\n"},{"id":"394639","messageId":"xmqqk12x17za.fsf@gitster.c.googlers.com","threadId":"53050","inReplyTo":"20200321161020.22817-2-mirucam@gmail.com","subject":"Re: [PATCH v2 01/11] bisect--helper: fix `cmd_*()` function switch default return","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-04-03T04:58:01Z","receivedAt":"2020-04-03T04:58:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Miriam Rubio writes:\n\n> In a `cmd_*()` function, return `error()` cannot be used\n> because that translates to `-1` and `cmd_*()` functions need\n> to return exit codes.\n>\n> Let's fix switch default return.\n>\n> Mentored-by: Christian Couder \n> Mentored-by: Johannes Schindelin \n> Signed-off-by: Miriam Rubio \n> ---\n> builtin/bisect--helper.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\n> index c1c40b516d..1f81cff1d8 100644\n> --- a/builtin/bisect--helper.c\n> +++ b/builtin/bisect--helper.c\n> @@ -711,7 +711,7 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n> \t\tres = bisect_start(&terms, no_checkout, argv, argc);\n> \t\tbreak;\n> \tdefault:\n> -\t\treturn error(\"BUG: unknown subcommand '%d'\", cmdmode);\n> +\t\tres = error(_(\"BUG: unknown subcommand.\"));\n\nThe return value from error() is *NOT* taken from \"enum\nbisect_error\"; its value (-1) happens to be the same as\nBISECT_FAILED, but that is by accident, and not by design.\n\nSo the above code is accident waiting to happen, while\n\n\tdefault:\n\t\terror(_(\"BUG: ...\"));\n\t\tres = BISECT_FAILED;\n\nwould be a lot more correct (by design).\n\n> \t}\n> \tfree_terms(&terms);\n\nAfter this part, there is this code:\n\n if (res == BISECT_INTERNAL_SUCCESS_MERGE_BASE)\n res = BISECT_OK;\n\n return abs(res);\n\nThis is not a problem with this patch, but the use of abs() is very\nmisleading, as res is always non-positive, as it is (after fixing\nthe patch I am responding to) taken from \"enum bisect_error\"\nvocabulary. \"return -res;\" would make the intent of the code\nclearer, I think.\n\nBy the way, under what condition can the \"BUG:\" be reached? Would\nit only be reachable by a programming error? If so, it would be\ncorrect to use BUG(\"...\") and force it die there. If it can be\nreached in some other way (e.g. an incorrect input by the user,\ncorruption in state files \"git bisect\" uses on the filesystem), then\nit is *not* a \"BUG\".\n\nI think \"bisect--helper\" is *not* called by end-user, so an unknown\ncommand would be a BUG in the calling program, which is still part\nof git, so it probably is more prudent to do something like the\nfollowing instead.\n\nThanks.\n\n builtin/bisect--helper.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\nindex c1c40b516d..0fbd924aac 100644\n--- a/builtin/bisect--helper.c\n+++ b/builtin/bisect--helper.c\n@@ -711,7 +711,7 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n \t\tres = bisect_start(&terms, no_checkout, argv, argc);\n \t\tbreak;\n \tdefault:\n-\t\treturn error(\"BUG: unknown subcommand '%d'\", cmdmode);\n+\t\tBUG(\"unknown subcommand %d\", (int)cmdmode);\n \t}\n \tfree_terms(&terms);\n \n@@ -722,5 +722,5 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n \tif (res == BISECT_INTERNAL_SUCCESS_MERGE_BASE)\n \t\tres = BISECT_OK;\n \n-\treturn abs(res);\n+\treturn -res;\n }\n"},{"id":"394640","messageId":"xmqqeet51794.fsf@gitster.c.googlers.com","threadId":"53050","inReplyTo":"20200321161020.22817-3-mirucam@gmail.com","subject":"Re: [PATCH v2 02/11] bisect--helper: introduce new `write_in_file()` function","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-04-03T05:13:43Z","receivedAt":"2020-04-03T05:13:48Z","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 helper will be used in later steps and makes the code\n> simpler and easier to understand.\n>\n> Mentored-by: Christian Couder \n> Mentored-by: Johannes Schindelin \n> Signed-off-by: Miriam Rubio \n> ---\n> builtin/bisect--helper.c | 27 +++++++++++++++++++++------\n> 1 file changed, 21 insertions(+), 6 deletions(-)\n>\n> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\n> index 1f81cff1d8..e949ea74e2 100644\n> --- a/builtin/bisect--helper.c\n> +++ b/builtin/bisect--helper.c\n> @@ -74,6 +74,26 @@ static int one_of(const char *term, ...)\n> \treturn res;\n> }\n> \n> +static int write_in_file(const char *filepath, const char *mode, const char *format,...)\n> +{\n> +\tFILE *fp = NULL;\n\nIt is crystal clear in this concise helper function that fp will\nnever be used without getting assigned the returned value from\nfopen(), so I do not think there is any need to initialize it to\nNULL.\n\nI'd use \"path\", not \"filepath\" (which I do not think we use anywhere\nin our codebase), if I were writing this function, by the way.\n\n> +\tva_list args;\n> +\tint res = 0;\n> +\n> +\tif (!strcmp(mode, \"a\") && !strcmp(mode, \"w\"))\n> +\t\treturn error_errno(_(\"wrong writing mode '%s'\"), mode);\n\nI do not see where you saw a failure from a call to system library\nfunction, which would make 'errno' variable valid at this point, so\nI am puzzled. By using error_errno(), whose error status are you\ntrying to show?\n\nPuzzled. Shouldn't it be just error(_(\"...\"), mode)?\n\n> +\tfp = fopen(filepath, mode);\n> +\tif (!fp)\n> +\t\treturn error_errno(_(\"could not open file '%s'\"), filepath);\n\nThis one would show why fopen() failed, so error_errno() would be good.\nDoes it help us help the users if they can tell us which mode we failed\nto write to the file? Something like\n\n\tcannot open file '%s' in mode '%s'\n\nperhaps?\n\n> +\tva_start(args, format);\n> +\tres = vfprintf(fp, format, args);\n> +\tva_end(args);\n> +\tif (!res)\n> +\t\treturn error_errno(_(\"could not write to file '%s'\"), filepath);\n\nThis would show errors from vfprintf(), which is good. However, you\nfail to fclose the FILE when this return is hit, which is not good.\n\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 +124,6 @@ static int check_term_format(const char *term, const char *orig_term)\n> \n> static int write_terms(const char *bad, const char *good)\n> {\n> -\tFILE *fp = NULL;\n> \tint res;\n> \n> \tif (!strcmp(bad, good))\n> @@ -113,12 +132,8 @@ 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(), \"w\", \"%s\\n%s\\n\", bad, good);\n> \n> -\tres = fprintf(fp, \"%s\\n%s\\n\", bad, good);\n> -\tres |= fclose(fp);\n> \treturn (res < 0) ? -1 : 0;\n> }\n"},{"id":"394661","messageId":"CAP8UFD3t5ZukXqfEr9W8FFror=SemmoB0hokri2BK6ZYcq619Q@mail.gmail.com","threadId":"53050","inReplyTo":"xmqqk12x17za.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2 01/11] bisect--helper: fix `cmd_*()` function switch default return","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2020-04-03T13:17:54Z","receivedAt":"2020-04-03T13:18:10Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Fri, Apr 3, 2020 at 6:58 AM Junio C Hamano wrote:\n>\n> Miriam Rubio writes:\n>\n> > In a `cmd_*()` function, return `error()` cannot be used\n> > because that translates to `-1` and `cmd_*()` functions need\n> > to return exit codes.\n> >\n> > Let's fix switch default return.\n> >\n> > Mentored-by: Christian Couder \n> > Mentored-by: Johannes Schindelin \n> > Signed-off-by: Miriam Rubio \n> > ---\n> > builtin/bisect--helper.c | 2 +-\n> > 1 file changed, 1 insertion(+), 1 deletion(-)\n> >\n> > diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\n> > index c1c40b516d..1f81cff1d8 100644\n> > --- a/builtin/bisect--helper.c\n> > +++ b/builtin/bisect--helper.c\n> > @@ -711,7 +711,7 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n> > res = bisect_start(&terms, no_checkout, argv, argc);\n> > break;\n> > default:\n> > - return error(\"BUG: unknown subcommand '%d'\", cmdmode);\n> > + res = error(_(\"BUG: unknown subcommand.\"));\n>\n> The return value from error() is *NOT* taken from \"enum\n> bisect_error\"; its value (-1) happens to be the same as\n> BISECT_FAILED, but that is by accident, and not by design.\n\nIn bisect.h we have made sure that BISECT_FAILED would be -1, so it is\nnot by accident:\n\nenum bisect_error {\n BISECT_OK = 0,\n BISECT_FAILED = -1,\n BISECT_ONLY_SKIPPED_LEFT = -2,\n BISECT_MERGE_BASE_CHECK = -3,\n BISECT_NO_TESTABLE_COMMIT = -4,\n BISECT_INTERNAL_SUCCESS_1ST_BAD_FOUND = -10,\n BISECT_INTERNAL_SUCCESS_MERGE_BASE = -11\n};\n\n> So the above code is accident waiting to happen, while\n>\n> default:\n> error(_(\"BUG: ...\"));\n> res = BISECT_FAILED;\n>\n> would be a lot more correct (by design).\n\nI think it is very unlikely that we will ever change the value\nreturned by error(), so I don't think there is an accident waiting to\nhappen.\n\nMaybe we should make it clearer though in bisect.h in the comment\nbefore the enum, that we chose -1 for BISECT_FAILED so that it is the\nsame as what error() returns. Maybe something like \"BISECT_FAILED\nerror code: default error code, should be the same value as what\nerror() returns.\"\n\n> After this part, there is this code:\n>\n> if (res == BISECT_INTERNAL_SUCCESS_MERGE_BASE)\n> res = BISECT_OK;\n>\n> return abs(res);\n>\n> This is not a problem with this patch, but the use of abs() is very\n> misleading, as res is always non-positive, as it is (after fixing\n> the patch I am responding to) taken from \"enum bisect_error\"\n> vocabulary. \"return -res;\" would make the intent of the code\n> clearer, I think.\n\nI am ok with using \"-res\" here. There are other places where\n\"abs(res)\" is needed though, so code could look a bit more consistent\nif \"abs(res)\" was used here too.\n\n> By the way, under what condition can the \"BUG:\" be reached? Would\n> it only be reachable by a programming error?\n\nIt could happen if a user would try to directly use `git\nbisect--helper ...` with an unsupported . Users are not\nsupposed to directly use bisect--helper though.\n\nIt could also happen if a developer uses `git bisect--helper \n...` in a script, program or alias if is not properly spelled or\nis unavailable for some reason.\n\n> If so, it would be\n> correct to use BUG(\"...\") and force it die there. If it can be\n> reached in some other way (e.g. an incorrect input by the user,\n> corruption in state files \"git bisect\" uses on the filesystem), then\n> it is *not* a \"BUG\".\n\nIn this case I think it's difficult to tell if it will be a bug or not.\n\n> I think \"bisect--helper\" is *not* called by end-user, so an unknown\n> command would be a BUG in the calling program, which is still part\n> of git, so it probably is more prudent to do something like the\n> following instead.\n\nI am ok with both ways.\n\nThanks,\nChristian.\n"},{"id":"394680","messageId":"xmqqv9mgzajx.fsf@gitster.c.googlers.com","threadId":"53050","inReplyTo":"CAP8UFD3t5ZukXqfEr9W8FFror=SemmoB0hokri2BK6ZYcq619Q@mail.gmail.com","subject":"Re: [PATCH v2 01/11] bisect--helper: fix `cmd_*()` function switch default return","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-04-03T18:30:42Z","receivedAt":"2020-04-03T18:30:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder writes:\n\n>> The return value from error() is *NOT* taken from \"enum\n>> bisect_error\"; its value (-1) happens to be the same as\n>> BISECT_FAILED, but that is by accident, and not by design.\n>\n> In bisect.h we have made sure that BISECT_FAILED would be -1, so it is\n> not by accident:\n\nIt *is* accident waiting to happen, unless you have a comment to\ntell future developers that they are forbidden from changing the\nassignment of values; \"We've made sure\" alone is not a good excuse.\n\n> enum bisect_error {\n> BISECT_OK = 0,\n> BISECT_FAILED = -1,\n> BISECT_ONLY_SKIPPED_LEFT = -2,\n> BISECT_MERGE_BASE_CHECK = -3,\n> BISECT_NO_TESTABLE_COMMIT = -4,\n> BISECT_INTERNAL_SUCCESS_1ST_BAD_FOUND = -10,\n> BISECT_INTERNAL_SUCCESS_MERGE_BASE = -11\n> };\n>\n>> So the above code is accident waiting to happen, while\n>>\n>> default:\n>> error(_(\"BUG: ...\"));\n>> res = BISECT_FAILED;\n>>\n>> would be a lot more correct (by design).\n>\n> I think it is very unlikely that we will ever change the value\n> returned by error(), so I don't think there is an accident waiting to\n> happen.\n>\n> Maybe we should make it clearer though in bisect.h in the comment\n> before the enum, that we chose -1 for BISECT_FAILED so that it is the\n> same as what error() returns....\n\nIn this particular case, you do not even need to rely on such a\ncomment to tie hands of future developers' needs (e.g. they may need\nto add a new enum value that must come between OK and FAILED because\nthey will find \"if (err < FAILED)\" is an easy way to do something\nthey need to do; an ordering requirement similar to how \"enum\ntodo_command\" in sequencer.h wants to enforce certain ordering of\nvalues is not uncommon, and they will find it awkward if they are\ntold that they cannot move FAILED to some value other than -1). You\nwere even shown a better way to separate \"res\" from the value\nerror() returns (which will always be -1) and BISECT_FAILED (which\nmay be -1 right now, but future developers may want to change it,\nand you have the power to allow it).\n\nI do not see why you are still giving a lame excuse after that.\n\nI even do not like the fact that you are doing so in the context of\nbeing a mentor---please do not spoil the opportunity to educate good\ndevelopers of our future; instead please lead them by showing a good\nexample.\n\n> I am ok with using \"-res\" here. There are other places where\n> \"abs(res)\" is needed though, so code could look a bit more consistent\n> if \"abs(res)\" was used here too.\n\nIf there are two kinds of codepaths, some *need* to deal with both\npositive and negative for good reasons, and others only need to deal\nwith non-positive values, it would make it easier to understand the\ncode by consistently using -res for the latter while using abs() for\nthe former.\n\nThis is a tangent, but a codepath that needs abs(res) may need to be\nreexaimined for correctness, as it is likely that it is a sign that\na sloppy developer swept a deeper underlying problem under the rug.\n\nImagine that a function A, in one if() statement in it, returns\nerror() whose value is -1, and in some other if() statement returns\nBAD_XYZZY whose value is 1. The function A also returns BAD_FROTZ\nwhose value is 2. The only guarantee the caller gets from the\nfunction A is that an error is signaled by non-zero value, and zero\nmeans success.\n\nAnd if you use abs() to squash an error and BAD_XYZZY into 1 in your\nfunction B that calls A, what good are you doing to the callers of\nyour B? They cannot tell between error and BAD_XYZZY, but they can\ntell them from BAD_FROTZ, but does such an arrangement make any\nsense? It would be far more rational to make your B either (1)\nreturn -1 for any error, if B thinks callers do not have to care\n(which could be a valid stance to take, depending on the nature of\nB), or (2) add an error code to BAD_{XYZZY,FROTZ} family and map -1\nthat comes from an error to that value, so that the callers can tell\nthem apart, or (3) do the equivalent of (2) but do so inside A (not\nin B), and update call the callers of A.\n\nAny of the above is more sensible and future-proof, compared to\nblindly using abs(res) and claim that you are safe because you are\nnot returning a negative value.\n\n>> By the way, under what condition can the \"BUG:\" be reached? Would\n>> it only be reachable by a programming error?\n>\n> It could happen if a user would try to directly use `git\n> bisect--helper ...` with an unsupported . Users are not\n> supposed to directly use bisect--helper though.\n>\n> It could also happen if a developer uses `git bisect--helper \n> ...` in a script, program or alias if is not properly spelled or\n> is unavailable for some reason.\n\nIf the user can legitimately trigger it, it is not a \"BUG:\". Let's\nmake sure we use \"BUG:\" (whether it comes from BUG(\"...\") or handcrafted\nmessage like this one here) only when there is a bug in our program.\nIn other words, when a user sees \"BUG:\" emitted from our program and\nreports it to us, there shouldn't be a room for us to say, \"eh,\nthanks for reporting, but it is an intended behaviour---you are just\nholding it wrong\".\n\nIf I did not know bisect--helper is its way out (which would be the\nendgame of making \"git bisect\" fully converted to C), I would say\nthat we should just mark it as an error, but in the endgame state,\nthere won't be any end-user visible bisect--helper, so I am OK to\nlabel it as a \"BUG:\" in this case. It will be in the endgame state.\n\nThanks.\n"},{"id":"394718","messageId":"xmqq4ku0z2r0.fsf@gitster.c.googlers.com","threadId":"53050","inReplyTo":"20200321161020.22817-4-mirucam@gmail.com","subject":"Re: [PATCH v2 03/11] bisect--helper: reimplement `bisect_next` and `bisect_auto_next` shell functions in C","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-04-03T21:19:15Z","receivedAt":"2020-04-03T21:19:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Miriam Rubio writes:\n\n> diff --git a/bisect.c b/bisect.c\n> index 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\nI don't have enough time and concentration to follow all the\ncodepaths in \"bisect\" that walk the commit graph starting here, but\nseeing one codepath, e.g. check_ancestors(), after calling this and\nwalking with bisect_common(), uses clear_commit_marks_many() to\nclear ALL_REV_FLAGS, not just the ones that reset_revision_walk()\nclears, I am not sure what value this addition has.\n\nTo put it differently, what codepath are you protecting the revision\nwalk that is about to happen against with this \"reset\"? Who are the\ncallers that could have used `revs` before calling this function and\ntouched only SEEN, ADDED, SHOWN, etc. without touching other bits\nlike COUNTED, TREESAME, UNINTERESTING that matter to the correct\noperation of \"bisect\"?\n\nThe rest of the patch looks quite reasonably done. Let me comment\non the patch out of order (in other words, I'll rearrange the\nfunctions in the order I read them). I am realizing that I feel it\neasier to understand to read the code in a bottom-up fashion.\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\nThe bisect_next_check() function returns what decide_next() returns,\nwhich is either 0 or error() which is -1. So this says \"if\nnect-check says there was an error, we return OK\". For the purpose\nof not proceeding to bisect_next(), returning is perfectly correct,\nbut is the value returned correct? The scripted original does\n\n git bisect--helper --bisect-next-check $TERM_GOOD $TERM_BAD && bisect_next || :\n\nmeaning \"try next-check and go on to bisect_next if check succeeds;\neither way, ignore all errors from them\". So it probably is more\nfaithful conversion to make the returned value from auto_next()\nvoid.\n\n> +\n> +\treturn bisect_next(terms, prefix);\n> +}\n\nIn any case, this conversion of auto_next looks like a good one,\nwith or without fixing its type. The caller in cmd_bisect__helper()\nseems to store the returned value in 'res' and uses it for the exit\nstatus, but for this to be a faithful conversion, it should ignore\nthe returned value from here and always exit with success (and if we\ndo so, it is one more reason why we would want to update the type of\nthis function to return void).\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\nThe original makes sure it does not get any argument, but that is\ndone in cmd_bisect__helper(), so it is OK.\n\nThe next thing the original does is to call bisect_autostart, before\ndoing the next-check. Was it a dead code that wasn't necessary, or\nis its loss a regression during the conversion?\n\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\nLooking good. We've already converted next_all() in the previous\nseries, and we call it the same way as the original by checking if\n$GIT_DIR/BISECT_HEAD is there. The original said \"if BISECT_HEAD\nexists as a file, use '--no-checkout'\" and did not care if its empty\nor not, but the updated code seems to care. Does the difference\nmatter (i.e. is it more correct to ignore an empty BISECT_HEAD and\npretend as if it did not exist)?\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\nIt is unclear why \"1st bad commit found\" is turned into \"success\nmerge base\" here. bisect_successfull() returns an error when it\ncannot append to the log, and otherwise we would want to relay \"we\nare done successfully\" back to the caller, no? Puzzled....\n\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\nThis side, I can understand what it wants to do to the \"we only have\nskipped ones so we cannot continue\" status.\n\nWhat is done in bisect_skipped_commits() is dubious, though (we'll\nsee it later).\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\nOpens the log to append to, or returns if we cannot. Calls\nprepare_revs() and process() if successfully prepared, and then\nclose. No resource leak, and the returned value is the result code\nfrom the last function that matters. Looks good.\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 * sets up 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\nUnlike the one in rev_setup() above, the only codepath this thing is\nused is quite limited (i.e. after doing all the bisection\ncomputation including walking the graph and counting the weights\nwith various bits in bisect_next) and we know all that is left to do\nis to run a single rev-list call, so it is clear to see that\nreset_revision_walk() is sufficient here.\n\n> +static void prepare_rev_argv(struct bisect_terms *terms, struct argv_array *rev_argv)\n> +{\n> +\tchar *term_good = xstrfmt(\"%s-*\", terms->term_good);\n> +\n> +\targv_array_pushl(rev_argv, \"skipped_commits\", \"refs/bisect/bad\", \"--not\", NULL);\n> +\tfor_each_glob_ref_in(register_good_ref, term_good, \"refs/bisect/\", rev_argv);\n> +\n> +\tfree(term_good);\n> +}\n> +\n\nThis sets up to find commits that can be reached by BAD that cannot\nbe reached by any GOOD revs, which is a quite faithful translation\nfrom the original one.\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\nWhat's that comparison with \"< 1\" doing? That's a 36-byte message,\nand you are saying that it is OK if we showed only 10-byte from it,\nbut it is not OK, even if the function did not report an output error\nby returning a negative value, if it returned that it wrote 0 bytes?\n\nI can understand it if it were\n\n\tif (fprintf(fp, \"...\") < 0)\n\t\treturn error_errno(_(\"failed to write to ...\"));\n\nthough.\n\n> +\twhile ((commit = get_revision(revs)) != NULL) {\n> +\t\tstruct strbuf commit_name = STRBUF_INIT;\n> +\t\tformat_commit_message(commit, \"%s\",\n> +\t\t\t\t &commit_name, &pp);\n> +\t\tfprintf(fp, \"# possible first %s commit: [%s] %s\\n\",\n> +\t\t\tterms->term_bad, oid_to_hex(&commit->object.oid),\n> +\t\t\tcommit_name.buf);\n> +\t\tstrbuf_release(&commit_name);\n> +\t}\n\nAgain, this is a faithful translation of the rev-list that appears\nin the original, provided that &revs was set up correctly, and\nrelevant object flags cleared correctly before we start the\ntraversal, both of which seem to be the case.\n\n> +\t/*\n> +\t * Reset the flags used by revision walks in case\n> +\t * there is another revision walk after this one.\n> +\t */\n> +\treset_revision_walk();\n> +\n> +\treturn 0;\n> +}\n> +\n\nSo, overall, this step was a quite pleasant read, except for the\nvery first bit above.\n\nThanks.\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 argv_array *rev_argv = cb_data;\n> +\n> +\targv_array_push(rev_argv, oid_to_hex(oid));\n> +\treturn 0;\n> +}\n> +\n> +static int bisect_successful(struct bisect_terms *terms)\n> +{\n> +\tstruct object_id oid;\n> +\tstruct commit *commit;\n> +\tstruct pretty_print_context pp = {0};\n> +\tstruct strbuf commit_name = STRBUF_INIT;\n> +\tchar *bad_ref = xstrfmt(\"refs/bisect/%s\",terms->term_bad);\n> +\tint res;\n> +\n> +\tread_ref(bad_ref, &oid);\n> +\tprintf(\"%s\\n\", bad_ref);\n> +\tcommit = lookup_commit_reference(the_repository, &oid);\n> +\tformat_commit_message(commit, \"%s\", &commit_name, &pp);\n> +\n> +\tres = write_in_file(git_path_bisect_log(), \"a\", \"# first %s commit: [%s] %s\\n\",\n> +\t\t\t terms->term_bad, oid_to_hex(&oid),\n> +\t\t\t commit_name.buf);\n> +\n> +\tstrbuf_release(&commit_name);\n> +\tfree(bad_ref);\n> +\treturn res;\n> +}\n> +\n"},{"id":"395995","messageId":"CAN7CjDDcqZO=Mh9-Hjj6kgMGcfg2o6=7F4iPAc68JWpnWMFMGQ@mail.gmail.com","threadId":"53050","inReplyTo":"xmqq4ku0z2r0.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2 03/11] bisect--helper: reimplement `bisect_next` and `bisect_auto_next` shell functions in C","fromName":"Miriam R.","fromEmail":"mirucam@gmail.com","sentAt":"2020-04-23T07:18:08Z","receivedAt":"2020-04-23T07:18:22Z","isPatch":true,"sender":{"key":"mirucam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56339109?v=4"},"body":"Hi Junio,\nthank you very much for reviewing, I have just sent the new version of\nthis patch series with your suggestions.\n\nEl vie., 3 abr. 2020 a las 23:19, Junio C Hamano () escribió:\n>\n> Miriam Rubio writes:\n>\n> > diff --git a/bisect.c b/bisect.c\n> > index 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> > struct argv_array rev_argv = ARGV_ARRAY_INIT;\n> > int i;\n> >\n> > + /*\n> > + * `revs` could has been used before.\n> > + * Thus we first need to reset it.\n> > + */\n> > + reset_revision_walk();\n> > repo_init_revisions(r, revs, prefix);\n>\n> I don't have enough time and concentration to follow all the\n> codepaths in \"bisect\" that walk the commit graph starting here, but\n> seeing one codepath, e.g. check_ancestors(), after calling this and\n> walking with bisect_common(), uses clear_commit_marks_many() to\n> clear ALL_REV_FLAGS, not just the ones that reset_revision_walk()\n> clears, I am not sure what value this addition has.\n>\n> To put it differently, what codepath are you protecting the revision\n> walk that is about to happen against with this \"reset\"? Who are the\n> callers that could have used `revs` before calling this function and\n> touched only SEEN, ADDED, SHOWN, etc. without touching other bits\n> like COUNTED, TREESAME, UNINTERESTING that matter to the correct\n> operation of \"bisect\"?\n>\n> The rest of the patch looks quite reasonably done. Let me comment\n> on the patch out of order (in other words, I'll rearrange the\n> functions in the order I read them). I am realizing that I feel it\n> easier to understand to read the code in a bottom-up fashion.\n>\n> > +static enum bisect_error bisect_auto_next(struct bisect_terms *terms, const char *prefix)\n> > +{\n> > + if (bisect_next_check(terms, NULL))\n> > + return BISECT_OK;\n>\n> The bisect_next_check() function returns what decide_next() returns,\n> which is either 0 or error() which is -1. So this says \"if\n> nect-check says there was an error, we return OK\". For the purpose\n> of not proceeding to bisect_next(), returning is perfectly correct,\n> but is the value returned correct? The scripted original does\n>\n> git bisect--helper --bisect-next-check $TERM_GOOD $TERM_BAD && bisect_next || :\n>\n> meaning \"try next-check and go on to bisect_next if check succeeds;\n> either way, ignore all errors from them\". So it probably is more\n> faithful conversion to make the returned value from auto_next()\n> void.\n>\n> > +\n> > + return bisect_next(terms, prefix);\n> > +}\n>\n> In any case, this conversion of auto_next looks like a good one,\n> with or without fixing its type. The caller in cmd_bisect__helper()\n> seems to store the returned value in 'res' and uses it for the exit\n> status, but for this to be a faithful conversion, it should ignore\n> the returned value from here and always exit with success (and if we\n> do so, it is one more reason why we would want to update the type of\n> this function to return void).\n>\n\nI cannot change bisect_auto_next() to return void because\nin shell the bisect_next() function used \"exit $res\" , so the following code:\n\ngit bisect--helper --bisect-next-check $TERM_GOOD $TERM_BAD && bisect_next || :\n\ncan result that whole `git bisect` exits with an error code when\nbisect_next() does an \"exit $res\" with $res != 0.\n\nSo errors from bisect_next() are not ignored and I cannot make\nbisect_auto_next() return void.\n\nBest,\nMiriam.\n\n\n> > +static enum bisect_error bisect_next(struct bisect_terms *terms, const char *prefix)\n> > +{\n> > + int no_checkout;\n> > + enum bisect_error res;\n> >\n> > + if (bisect_next_check(terms, terms->term_good))\n> > + return BISECT_FAILED;\n>\n> The original makes sure it does not get any argument, but that is\n> done in cmd_bisect__helper(), so it is OK.\n>\n> The next thing the original does is to call bisect_autostart, before\n> doing the next-check. Was it a dead code that wasn't necessary, or\n> is its loss a regression during the conversion?\n>\n> > +\n> > + no_checkout = !is_empty_or_missing_file(git_path_bisect_head());\n> > +\n> > + /* Perform all bisection computation, display and checkout */\n> > + res = bisect_next_all(the_repository, prefix, no_checkout);\n>\n> Looking good. We've already converted next_all() in the previous\n> series, and we call it the same way as the original by checking if\n> $GIT_DIR/BISECT_HEAD is there. The original said \"if BISECT_HEAD\n> exists as a file, use '--no-checkout'\" and did not care if its empty\n> or not, but the updated code seems to care. Does the difference\n> matter (i.e. is it more correct to ignore an empty BISECT_HEAD and\n> pretend as if it did not exist)?\n>\n> > + if (res == BISECT_INTERNAL_SUCCESS_1ST_BAD_FOUND) {\n> > + res = bisect_successful(terms);\n> > + return res ? res : BISECT_INTERNAL_SUCCESS_MERGE_BASE;\n>\n> It is unclear why \"1st bad commit found\" is turned into \"success\n> merge base\" here. bisect_successfull() returns an error when it\n> cannot append to the log, and otherwise we would want to relay \"we\n> are done successfully\" back to the caller, no? Puzzled....\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> > +\n>\n> This side, I can understand what it wants to do to the \"we only have\n> skipped ones so we cannot continue\" status.\n>\n> What is done in bisect_skipped_commits() is dubious, though (we'll\n> see it later).\n>\n>\n> > +static int bisect_skipped_commits(struct bisect_terms *terms)\n> > +{\n> > + int res = 0;\n> > + FILE *fp = NULL;\n> > + struct rev_info revs;\n> > +\n> > + fp = fopen(git_path_bisect_log(), \"a\");\n> > + if (!fp)\n> > + return error_errno(_(\"could not open '%s' for appending\"),\n> > + git_path_bisect_log());\n> > +\n> > + res = prepare_revs(terms, &revs);\n> > +\n> > + if (!res)\n> > + res = process_skipped_commits(fp, terms, &revs);\n> > +\n> > + fclose(fp);\n> > + return res;\n> > +}\n> > +\n>\n> Opens the log to append to, or returns if we cannot. Calls\n> prepare_revs() and process() if successfully prepared, and then\n> close. No resource leak, and the returned value is the result code\n> from the last function that matters. Looks good.\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> > + * sets up a revision walk.\n> > + */\n> > + reset_revision_walk();\n> > + init_revisions(revs, NULL);\n> > + rev_argv.argc = setup_revisions(rev_argv.argc, rev_argv.argv, revs, NULL);\n> > + if (prepare_revision_walk(revs))\n> > + res = error(_(\"revision walk setup failed\\n\"));\n> > +\n> > + argv_array_clear(&rev_argv);\n> > + return res;\n> > +}\n> > +\n>\n> Unlike the one in rev_setup() above, the only codepath this thing is\n> used is quite limited (i.e. after doing all the bisection\n> computation including walking the graph and counting the weights\n> with various bits in bisect_next) and we know all that is left to do\n> is to run a single rev-list call, so it is clear to see that\n> reset_revision_walk() is sufficient here.\n>\n> > +static void prepare_rev_argv(struct bisect_terms *terms, struct argv_array *rev_argv)\n> > +{\n> > + char *term_good = xstrfmt(\"%s-*\", terms->term_good);\n> > +\n> > + argv_array_pushl(rev_argv, \"skipped_commits\", \"refs/bisect/bad\", \"--not\", NULL);\n> > + for_each_glob_ref_in(register_good_ref, term_good, \"refs/bisect/\", rev_argv);\n> > +\n> > + free(term_good);\n> > +}\n> > +\n>\n> This sets up to find commits that can be reached by BAD that cannot\n> be reached by any GOOD revs, which is a quite faithful translation\n> from the original one.\n>\n> > +static int process_skipped_commits(FILE *fp, struct bisect_terms *terms, struct rev_info *revs)\n> > +{\n> > + struct commit *commit;\n> > + struct pretty_print_context pp = {0};\n> > +\n> > + if (fprintf(fp, \"# only skipped commits left to test\\n\") < 1)\n> > + return -1;\n>\n> What's that comparison with \"< 1\" doing? That's a 36-byte message,\n> and you are saying that it is OK if we showed only 10-byte from it,\n> but it is not OK, even if the function did not report an output error\n> by returning a negative value, if it returned that it wrote 0 bytes?\n>\n> I can understand it if it were\n>\n> if (fprintf(fp, \"...\") < 0)\n> return error_errno(_(\"failed to write to ...\"));\n>\n> though.\n>\n> > + while ((commit = get_revision(revs)) != NULL) {\n> > + struct strbuf commit_name = STRBUF_INIT;\n> > + format_commit_message(commit, \"%s\",\n> > + &commit_name, &pp);\n> > + fprintf(fp, \"# possible first %s commit: [%s] %s\\n\",\n> > + terms->term_bad, oid_to_hex(&commit->object.oid),\n> > + commit_name.buf);\n> > + strbuf_release(&commit_name);\n> > + }\n>\n> Again, this is a faithful translation of the rev-list that appears\n> in the original, provided that &revs was set up correctly, and\n> relevant object flags cleared correctly before we start the\n> traversal, both of which seem to be the case.\n>\n> > + /*\n> > + * Reset the flags used by revision walks in case\n> > + * there is another revision walk after this one.\n> > + */\n> > + reset_revision_walk();\n> > +\n> > + return 0;\n> > +}\n> > +\n>\n> So, overall, this step was a quite pleasant read, except for the\n> very first bit above.\n>\n> Thanks.\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 argv_array *rev_argv = cb_data;\n> > +\n> > + argv_array_push(rev_argv, oid_to_hex(oid));\n> > + return 0;\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\",terms->term_bad);\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> > + res = write_in_file(git_path_bisect_log(), \"a\", \"# first %s commit: [%s] %s\\n\",\n> > + terms->term_bad, oid_to_hex(&oid),\n> > + commit_name.buf);\n> > +\n> > + strbuf_release(&commit_name);\n> > + free(bad_ref);\n> > + return res;\n> > +}\n> > +\n"}]}