{"thread":{"id":"42979","subject":"[RFC/PATCH v11 06/13] wrapper: move is_empty_file() and rename it as is_empty_or_missing_file()","startedAt":"2016-07-31T09:22:16Z","lastAt":"2016-08-04T16:57:52Z","messageCount":39,"participants":["Pranit Bauva","Junio C Hamano"],"isPatch":true,"patchVersion":11,"patchTotal":13},"messages":[{"id":"292669","messageId":"0102015640423cf1-92bad064-76de-4ee5-80b5-af735f940426-000000@eu-west-1.amazonses.com","threadId":"42979","inReplyTo":"0102015640423c26-2060fd70-c90d-4de3-ae8c-1801ad160b1c-000000@eu-west-1.amazonses.com","subject":"[RFC/PATCH v11 06/13] wrapper: move is_empty_file() and rename it as is_empty_or_missing_file()","fromName":"Pranit Bauva","fromEmail":"pranit.bauva@gmail.com","sentAt":"2016-07-31T09:21:38Z","receivedAt":"2016-07-31T09:22:16Z","isPatch":true,"sender":{"key":"pranit.bauva@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2959938?v=4"},"body":"is_empty_file() can help to refactor a lot of code. This will be very\nhelpful in porting \"git bisect\" to C.\n\nSuggested-by: Torsten Bögershausen \nMentored-by: Lars Schneider \nMentored-by: Christian Couder \nSigned-off-by: Pranit Bauva \n---\n builtin/am.c | 20 ++------------------\n cache.h | 3 +++\n wrapper.c | 13 +++++++++++++\n 3 files changed, 18 insertions(+), 18 deletions(-)\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex 3dfe70b..6ee158f 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -30,22 +30,6 @@\n #include \"mailinfo.h\"\n \n /**\n- * Returns 1 if the file is empty or does not exist, 0 otherwise.\n- */\n-static int is_empty_file(const char *filename)\n-{\n-\tstruct stat st;\n-\n-\tif (stat(filename, &st) < 0) {\n-\t\tif (errno == ENOENT)\n-\t\t\treturn 1;\n-\t\tdie_errno(_(\"could not stat %s\"), filename);\n-\t}\n-\n-\treturn !st.st_size;\n-}\n-\n-/**\n * Returns the length of the first line of msg.\n */\n static int linelen(const char *msg)\n@@ -1323,7 +1307,7 @@ static int parse_mail(struct am_state *state, const char *mail)\n \t\tgoto finish;\n \t}\n \n-\tif (is_empty_file(am_path(state, \"patch\"))) {\n+\tif (is_empty_or_missing_file(am_path(state, \"patch\"))) {\n \t\tprintf_ln(_(\"Patch is empty. Was it split wrong?\"));\n \t\tdie_user_resolve(state);\n \t}\n@@ -1911,7 +1895,7 @@ static void am_run(struct am_state *state, int resume)\n \t\tresume = 0;\n \t}\n \n-\tif (!is_empty_file(am_path(state, \"rewritten\"))) {\n+\tif (!is_empty_or_missing_file(am_path(state, \"rewritten\"))) {\n \t\tassert(state->rebasing);\n \t\tcopy_notes_for_rebase(state);\n \t\trun_post_rewrite_hook(state);\ndiff --git a/cache.h b/cache.h\nindex 6049f86..91e2f81 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1870,4 +1870,7 @@ void sleep_millisec(int millisec);\n */\n void safe_create_dir(const char *dir, int share);\n \n+/* Return 1 if the file is empty or does not exists, 0 otherwise. */\n+extern int is_empty_or_missing_file(const char *filename);\n+\n #endif /* CACHE_H */\ndiff --git a/wrapper.c b/wrapper.c\nindex 5dc4e15..e70e4d1 100644\n--- a/wrapper.c\n+++ b/wrapper.c\n@@ -696,3 +696,16 @@ void sleep_millisec(int millisec)\n {\n \tpoll(NULL, 0, millisec);\n }\n+\n+int is_empty_or_missing_file(const char *filename)\n+{\n+\tstruct stat st;\n+\n+\tif (stat(filename, &st) < 0) {\n+\t\tif (errno == ENOENT)\n+\t\t\treturn 1;\n+\t\tdie_errno(_(\"could not stat %s\"), filename);\n+\t}\n+\n+\treturn !st.st_size;\n+}\n\n--\nhttps://github.com/git/git/pull/281\n"},{"id":"292671","messageId":"0102015640423ceb-e1fba6c7-4973-4b72-8ea9-fab72abe043d-000000@eu-west-1.amazonses.com","threadId":"42979","inReplyTo":"0102015640423c26-2060fd70-c90d-4de3-ae8c-1801ad160b1c-000000@eu-west-1.amazonses.com","subject":"[RFC/PATCH v11 05/13] t6030: explicitly test for bisection cleanup","fromName":"Pranit Bauva","fromEmail":"pranit.bauva@gmail.com","sentAt":"2016-07-31T09:21:38Z","receivedAt":"2016-07-31T09:32:24Z","isPatch":true,"sender":{"key":"pranit.bauva@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2959938?v=4"},"body":"Add test to explicitly check that 'git bisect reset' is working as\nexpected. This is already covered implicitly by the test suite.\n\nMentored-by: Lars Schneider \nMentored-by: Christian Couder \nSigned-off-by: Pranit Bauva \n\n---\nI faced this problem while converting `bisect_clean_state` and the tests\nwhere showing breakages but it wasn't clear as to where exactly are they\nbreaking. This will patch will help in that. Also I tested the test\ncoverage of the test suite before this patch and it covers this (I did\nthis by purposely changing names of files in git-bisect.sh and running\nthe test suite).\n---\n t/t6030-bisect-porcelain.sh | 17 +++++++++++++++++\n 1 file changed, 17 insertions(+)\n\ndiff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh\nindex e74662b..a17f7a6 100755\n--- a/t/t6030-bisect-porcelain.sh\n+++ b/t/t6030-bisect-porcelain.sh\n@@ -894,4 +894,21 @@ test_expect_success 'bisect start takes options and revs in any order' '\n \ttest_cmp expected actual\n '\n \n+test_expect_success 'git bisect reset cleans bisection state properly' '\n+\tgit bisect reset &&\n+\tgit bisect start &&\n+\tgit bisect good $HASH1 &&\n+\tgit bisect bad $HASH4 &&\n+\tgit bisect reset &&\n+\ttest -z \"$(git for-each-ref \"refs/bisect/*\")\" &&\n+\ttest_path_is_missing \"$GIT_DIR/BISECT_EXPECTED_REV\" &&\n+\ttest_path_is_missing \"$GIT_DIR/BISECT_ANCESTORS_OK\" &&\n+\ttest_path_is_missing \"$GIT_DIR/BISECT_LOG\" &&\n+\ttest_path_is_missing \"$GIT_DIR/BISECT_RUN\" &&\n+\ttest_path_is_missing \"$GIT_DIR/BISECT_TERMS\" &&\n+\ttest_path_is_missing \"$GIT_DIR/head-name\" &&\n+\ttest_path_is_missing \"$GIT_DIR/BISECT_HEAD\" &&\n+\ttest_path_is_missing \"$GIT_DIR/BISECT_START\"\n+'\n+\n test_done\n\n--\nhttps://github.com/git/git/pull/281\n"},{"id":"292672","messageId":"0102015640423cd0-ae286008-08b7-4ca4-9b2b-f4226aa80a97-000000@eu-west-1.amazonses.com","threadId":"42979","inReplyTo":"0102015640423c26-2060fd70-c90d-4de3-ae8c-1801ad160b1c-000000@eu-west-1.amazonses.com","subject":"[RFC/PATCH v11 03/13] bisect--helper: `write_terms` shell function in C","fromName":"Pranit Bauva","fromEmail":"pranit.bauva@gmail.com","sentAt":"2016-07-31T09:21:38Z","receivedAt":"2016-07-31T09:34:26Z","isPatch":true,"sender":{"key":"pranit.bauva@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2959938?v=4"},"body":"Reimplement the `write_terms` shell function in C and add a `write-terms`\nsubcommand to `git bisect--helper` to call it from git-bisect.sh . Also\nremove the subcommand `--check-term-format` as it can now be called from\ninside the function write_terms() C implementation.\n\nAlso `|| exit` is added when calling write-terms subcommand from\ngit-bisect.sh so as to exit whenever there is an error.\n\nUsing `--write-terms` 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 method.\n\nMentored-by: Lars Schneider \nMentored-by: Christian Couder \nSigned-off-by: Pranit Bauva \n---\n builtin/bisect--helper.c | 36 +++++++++++++++++++++++++++++-------\n git-bisect.sh | 22 +++++++---------------\n 2 files changed, 36 insertions(+), 22 deletions(-)\n\ndiff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\nindex 48285d4..965bcc1 100644\n--- a/builtin/bisect--helper.c\n+++ b/builtin/bisect--helper.c\n@@ -4,9 +4,11 @@\n #include \"bisect.h\"\n #include \"refs.h\"\n \n+static GIT_PATH_FUNC(git_path_bisect_terms, \"BISECT_TERMS\")\n+\n static const char * const git_bisect_helper_usage[] = {\n \tN_(\"git bisect--helper --next-all [--no-checkout]\"),\n-\tN_(\"git bisect--helper --check-term-format \"),\n+\tN_(\"git bisect--helper --write-terms \"),\n \tNULL\n };\n \n@@ -56,18 +58,38 @@ static int check_term_format(const char *term, const char *orig_term)\n \treturn 0;\n }\n \n+static int write_terms(const char *bad, const char *good)\n+{\n+\tFILE *fp;\n+\tint res;\n+\n+\tif (!strcmp(bad, good))\n+\t\treturn error(_(\"please use two different terms\"));\n+\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+\n+\tres = fprintf(fp, \"%s\\n%s\\n\", bad, good);\n+\tres |= fclose(fp);\n+\treturn (res < 0) ? -1 : 0;\n+}\n+\n int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n {\n \tenum {\n \t\tNEXT_ALL = 1,\n-\t\tCHECK_TERM_FMT\n+\t\tWRITE_TERMS\n \t} cmdmode = 0;\n \tint no_checkout = 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, \"check-term-format\", &cmdmode,\n-\t\t\t N_(\"check format of the term\"), CHECK_TERM_FMT),\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_BOOL(0, \"no-checkout\", &no_checkout,\n \t\t\t N_(\"update BISECT_HEAD instead of checking out the current commit\")),\n \t\tOPT_END()\n@@ -82,10 +104,10 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n \tswitch (cmdmode) {\n \tcase NEXT_ALL:\n \t\treturn bisect_next_all(prefix, no_checkout);\n-\tcase CHECK_TERM_FMT:\n+\tcase WRITE_TERMS:\n \t\tif (argc != 2)\n-\t\t\tdie(_(\"--check-term-format requires two arguments\"));\n-\t\treturn check_term_format(argv[0], argv[1]);\n+\t\t\tdie(_(\"--write-terms requires two arguments\"));\n+\t\treturn write_terms(argv[0], argv[1]);\n \tdefault:\n \t\tdie(\"BUG: unknown subcommand '%d'\", cmdmode);\n \t}\ndiff --git a/git-bisect.sh b/git-bisect.sh\nindex 7d7965d..cd39bd0 100755\n--- a/git-bisect.sh\n+++ b/git-bisect.sh\n@@ -210,7 +210,7 @@ bisect_start() {\n \teval \"$eval true\" &&\n \tif test $must_write_terms -eq 1\n \tthen\n-\t\twrite_terms \"$TERM_BAD\" \"$TERM_GOOD\"\n+\t\tgit bisect--helper --write-terms \"$TERM_BAD\" \"$TERM_GOOD\"\n \tfi &&\n \techo \"git bisect start$orig_args\" >>\"$GIT_DIR/BISECT_LOG\" || exit\n \t#\n@@ -557,18 +557,6 @@ get_terms () {\n \tfi\n }\n \n-write_terms () {\n-\tTERM_BAD=$1\n-\tTERM_GOOD=$2\n-\tif test \"$TERM_BAD\" = \"$TERM_GOOD\"\n-\tthen\n-\t\tdie \"$(gettext \"please use two different terms\")\"\n-\tfi\n-\tgit bisect--helper --check-term-format \"$TERM_BAD\" bad || exit\n-\tgit bisect--helper --check-term-format \"$TERM_GOOD\" good || exit\n-\tprintf '%s\\n%s\\n' \"$TERM_BAD\" \"$TERM_GOOD\" >\"$GIT_DIR/BISECT_TERMS\"\n-}\n-\n check_and_set_terms () {\n \tcmd=\"$1\"\n \tcase \"$cmd\" in\n@@ -582,13 +570,17 @@ check_and_set_terms () {\n \t\tbad|good)\n \t\t\tif ! test -s \"$GIT_DIR/BISECT_TERMS\"\n \t\t\tthen\n-\t\t\t\twrite_terms bad good\n+\t\t\t\tTERM_BAD=bad\n+\t\t\t\tTERM_GOOD=good\n+\t\t\t\tgit bisect--helper --write-terms \"$TERM_BAD\" \"$TERM_GOOD\" || exit\n \t\t\tfi\n \t\t\t;;\n \t\tnew|old)\n \t\t\tif ! test -s \"$GIT_DIR/BISECT_TERMS\"\n \t\t\tthen\n-\t\t\t\twrite_terms new old\n+\t\t\t\tTERM_BAD=new\n+\t\t\t\tTERM_GOOD=old\n+\t\t\t\tgit bisect--helper --write-terms \"$TERM_BAD\" \"$TERM_GOOD\" || exit\n \t\t\tfi\n \t\t\t;;\n \t\tesac ;;\n\n--\nhttps://github.com/git/git/pull/281\n"},{"id":"292673","messageId":"0102015640423cf1-89276881-65c0-4fc7-9e3e-ba74a6bd3057-000000@eu-west-1.amazonses.com","threadId":"42979","inReplyTo":"0102015640423c26-2060fd70-c90d-4de3-ae8c-1801ad160b1c-000000@eu-west-1.amazonses.com","subject":"[RFC/PATCH v11 07/13] bisect--helper: `bisect_reset` shell function in C","fromName":"Pranit Bauva","fromEmail":"pranit.bauva@gmail.com","sentAt":"2016-07-31T09:21:38Z","receivedAt":"2016-07-31T09:34:31Z","isPatch":true,"sender":{"key":"pranit.bauva@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2959938?v=4"},"body":"Reimplement `bisect_reset` shell function in C and add a `--bisect-reset`\nsubcommand to `git bisect--helper` to call it from git-bisect.sh .\n\nUsing `bisect_reset` subcommand is a temporary measure to port shell\nfunctions to C so as to use the existing test suite. As more functions\nare ported, this subcommand would be retired and will be called by some\nother method.\n\nNote: --bisect-clean-state subcommand has not been retired as there are\nstill a function namely `bisect_start()` which still uses this\nsubcommand.\n\nMentored-by: Lars Schneider \nMentored-by: Christian Couder \nSigned-off-by: Pranit Bauva \n---\n builtin/bisect--helper.c | 47 ++++++++++++++++++++++++++++++++++++++++++++++-\n git-bisect.sh | 28 ++--------------------------\n 2 files changed, 48 insertions(+), 27 deletions(-)\n\ndiff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\nindex 10dfb68..0e09630 100644\n--- a/builtin/bisect--helper.c\n+++ b/builtin/bisect--helper.c\n@@ -4,6 +4,8 @@\n #include \"bisect.h\"\n #include \"refs.h\"\n #include \"dir.h\"\n+#include \"argv-array.h\"\n+#include \"run-command.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@@ -13,11 +15,13 @@ static GIT_PATH_FUNC(git_path_bisect_names, \"BISECT_NAMES\")\n static GIT_PATH_FUNC(git_path_bisect_run, \"BISECT_RUN\")\n static GIT_PATH_FUNC(git_path_head_name, \"head-name\")\n static GIT_PATH_FUNC(git_path_bisect_start, \"BISECT_START\")\n+static GIT_PATH_FUNC(git_path_bisect_head, \"BISECT_HEAD\")\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-clean-state\"),\n+\tN_(\"git bisect--helper --bisect-reset []\"),\n \tNULL\n };\n \n@@ -124,12 +128,47 @@ static int bisect_clean_state(void)\n \treturn result;\n }\n \n+static int bisect_reset(const char *commit)\n+{\n+\tstruct strbuf branch = STRBUF_INIT;\n+\n+\tif (!commit) {\n+\t\tif (strbuf_read_file(&branch, git_path_bisect_start(), 0) < 1) {\n+\t\t\tprintf(\"We are not bisecting.\\n\");\n+\t\t\treturn 0;\n+\t\t}\n+\t\tstrbuf_rtrim(&branch);\n+\t} else {\n+\t\tstruct object_id oid;\n+\t\tif (get_oid(commit, &oid))\n+\t\t\treturn error(_(\"'%s' is not a valid commit\"), commit);\n+\t\tstrbuf_addstr(&branch, commit);\n+\t}\n+\n+\tif (!file_exists(git_path_bisect_head())) {\n+\t\tstruct argv_array argv = ARGV_ARRAY_INIT;\n+\t\targv_array_pushl(&argv, \"checkout\", branch.buf, \"--\", NULL);\n+\t\tif (run_command_v_opt(argv.argv, RUN_GIT_CMD)) {\n+\t\t\terror(_(\"Could not check out original HEAD '%s'. Try \"\n+\t\t\t\t\"'git bisect reset '.\"), branch.buf);\n+\t\t\tstrbuf_release(&branch);\n+\t\t\targv_array_clear(&argv);\n+\t\t\treturn -1;\n+\t\t}\n+\t\targv_array_clear(&argv);\n+\t}\n+\n+\tstrbuf_release(&branch);\n+\treturn bisect_clean_state();\n+}\n+\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\tBISECT_CLEAN_STATE\n+\t\tBISECT_CLEAN_STATE,\n+\t\tBISECT_RESET\n \t} cmdmode = 0;\n \tint no_checkout = 0;\n \tstruct option options[] = {\n@@ -139,6 +178,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\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, \"bisect-reset\", &cmdmode,\n+\t\t\t N_(\"reset the bisection state\"), BISECT_RESET),\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_END()\n@@ -161,6 +202,10 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n \t\tif (argc != 0)\n \t\t\tdie(_(\"--bisect-clean-state requires no arguments\"));\n \t\treturn bisect_clean_state();\n+\tcase BISECT_RESET:\n+\t\tif (argc > 1)\n+\t\t\tdie(_(\"--bisect-reset requires either zero or one arguments\"));\n+\t\treturn bisect_reset(argc ? argv[0] : NULL);\n \tdefault:\n \t\tdie(\"BUG: unknown subcommand '%d'\", cmdmode);\n \t}\ndiff --git a/git-bisect.sh b/git-bisect.sh\nindex bbc57d2..18580b7 100755\n--- a/git-bisect.sh\n+++ b/git-bisect.sh\n@@ -409,35 +409,11 @@ bisect_visualize() {\n \teval '\"$@\"' --bisect -- $(cat \"$GIT_DIR/BISECT_NAMES\")\n }\n \n-bisect_reset() {\n-\ttest -s \"$GIT_DIR/BISECT_START\" || {\n-\t\tgettextln \"We are not bisecting.\"\n-\t\treturn\n-\t}\n-\tcase \"$#\" in\n-\t0) branch=$(cat \"$GIT_DIR/BISECT_START\") ;;\n-\t1) git rev-parse --quiet --verify \"$1^{commit}\" >/dev/null || {\n-\t\t\tinvalid=\"$1\"\n-\t\t\tdie \"$(eval_gettext \"'\\$invalid' is not a valid commit\")\"\n-\t\t}\n-\t\tbranch=\"$1\" ;;\n-\t*)\n-\t\tusage ;;\n-\tesac\n-\n-\tif ! test -f \"$GIT_DIR/BISECT_HEAD\" && ! git checkout \"$branch\" --\n-\tthen\n-\t\tdie \"$(eval_gettext \"Could not check out original HEAD '\\$branch'.\n-Try 'git bisect reset '.\")\"\n-\tfi\n-\tgit bisect--helper --bisect-clean-state || exit\n-}\n-\n bisect_replay () {\n \tfile=\"$1\"\n \ttest \"$#\" -eq 1 || die \"$(gettext \"No logfile given\")\"\n \ttest -r \"$file\" || die \"$(eval_gettext \"cannot read \\$file for replaying\")\"\n-\tbisect_reset\n+\tgit bisect--helper --bisect-reset || exit\n \twhile read git bisect command rev\n \tdo\n \t\ttest \"$git $bisect\" = \"git bisect\" || test \"$git\" = \"git-bisect\" || continue\n@@ -627,7 +603,7 @@ case \"$#\" in\n \tvisualize|view)\n \t\tbisect_visualize \"$@\" ;;\n \treset)\n-\t\tbisect_reset \"$@\" ;;\n+\t\tgit bisect--helper --bisect-reset \"$@\" ;;\n \treplay)\n \t\tbisect_replay \"$@\" ;;\n \tlog)\n\n--\nhttps://github.com/git/git/pull/281\n"},{"id":"292674","messageId":"0102015640423d08-3af7e66f-4104-4b0c-a3c7-dff29b843b2d-000000@eu-west-1.amazonses.com","threadId":"42979","inReplyTo":"0102015640423c26-2060fd70-c90d-4de3-ae8c-1801ad160b1c-000000@eu-west-1.amazonses.com","subject":"[RFC/PATCH v11 11/13] bisect--helper: `bisect_next_check` shell function in C","fromName":"Pranit Bauva","fromEmail":"pranit.bauva@gmail.com","sentAt":"2016-07-31T09:21:38Z","receivedAt":"2016-07-31T09:35:47Z","isPatch":true,"sender":{"key":"pranit.bauva@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2959938?v=4"},"body":"Reimplement `bisect_next_check` shell function in C and add\n`bisect-next-check` subcommand to `git bisect--helper` to call it from\ngit-bisect.sh .\n\nUsing `--bisect-next-check` 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\nbisect_voc() is removed as it is redundant and does not serve any useful\npurpose. We are better off specifying \"bad|new\" \"good|old\" as and when\nwe require in bisect_next_check().\n\nMentored-by: Lars Schneider \nMentored-by: Christian Couder \nSigned-off-by: Pranit Bauva \n---\n builtin/bisect--helper.c | 79 +++++++++++++++++++++++++++++++++++++++++++++++-\n git-bisect.sh | 60 +++---------------------------------\n 2 files changed, 82 insertions(+), 57 deletions(-)\n\ndiff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\nindex 99c9f90..71f4cf0 100644\n--- a/builtin/bisect--helper.c\n+++ b/builtin/bisect--helper.c\n@@ -6,6 +6,7 @@\n #include \"dir.h\"\n #include \"argv-array.h\"\n #include \"run-command.h\"\n+#include \"prompt.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@@ -24,6 +25,7 @@ static const char * const git_bisect_helper_usage[] = {\n \tN_(\"git bisect--helper --bisect-reset []\"),\n \tN_(\"git bisect--helper --bisect-write []\"),\n \tN_(\"git bisect--helper --bisect-check-and-set-terms \"),\n+\tN_(\"git bisect--helper --bisect-next-check [] term_bad.buf);\n+\tchar *good_glob = xstrfmt(\"%s*\", terms->term_good.buf);\n+\n+\tif (ref_exists(bad_ref))\n+\t\tmissing_bad = 0;\n+\tfree(bad_ref);\n+\n+\tfor_each_glob_ref_in(mark_good, good_glob, \"refs/bisect/\",\n+\t\t\t (void *) &missing_good);\n+\tfree(good_glob);\n+\n+\tif (!missing_good && !missing_bad)\n+\t\treturn 0;\n+\n+\tif (!current_term)\n+\t\treturn -1;\n+\n+\tif (missing_good && !missing_bad && current_term &&\n+\t !strcmp(current_term, terms->term_good.buf)) {\n+\t\tchar *yesno;\n+\t\t/*\n+\t\t * have bad (or new) but not good (or old). We could bisect\n+\t\t * although this is less optimum.\n+\t\t */\n+\t\tfprintf(stderr, \"Warning: bisecting only with a %s commit\\n\",\n+\t\t\tterms->term_bad.buf);\n+\t\tif (!isatty(0))\n+\t\t\treturn 0;\n+\t\t/*\n+\t\t * TRANSLATORS: Make sure to include [Y] and [n] in your\n+\t\t * translation. The program will only accept English input\n+\t\t * at this point.\n+\t\t */\n+\t\tyesno = git_prompt(_(\"Are you sure [Y/n]? \"), PROMPT_ECHO);\n+\t\tif (starts_with(yesno, \"N\") || starts_with(yesno, \"n\"))\n+\t\t\treturn -1;\n+\t\treturn 0;\n+\t}\n+\tif (!is_empty_or_missing_file(git_path_bisect_start()))\n+\t\treturn error(_(\"You need to give me at least one good|old and \"\n+\t\t\t\t\"bad|new revision. You can use \\\"git bisect \"\n+\t\t\t\t\"bad|new\\\" and \\\"git bisect good|old\\\" for \"\n+\t\t\t\t\"that. \\n\"));\n+\telse\n+\t\treturn error(_(\"You need to start by \\\"git bisect start\\\". \"\n+\t\t\t\t\"You then need to give me at least one good|\"\n+\t\t\t\t\"old and bad|new revision. You can use \\\"git \"\n+\t\t\t\t\"bisect bad|new\\\" and \\\"git bisect good|old\\\" \"\n+\t\t\t\t\" for that.\\n\"));\n+\n+\treturn 0;\n+}\n+\n int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n {\n \tenum {\n@@ -301,7 +368,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n \t\tBISECT_RESET,\n \t\tCHECK_EXPECTED_REVS,\n \t\tBISECT_WRITE,\n-\t\tCHECK_AND_SET_TERMS\n+\t\tCHECK_AND_SET_TERMS,\n+\t\tBISECT_NEXT_CHECK\n \t} cmdmode = 0;\n \tint no_checkout = 0, res = 0;\n \tstruct option options[] = {\n@@ -319,6 +387,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n \t\t\t N_(\"write out the bisection state in BISECT_LOG\"), BISECT_WRITE),\n \t\tOPT_CMDMODE(0, \"check-and-set-terms\", &cmdmode,\n \t\t\t N_(\"check and set terms in a bisection state\"), CHECK_AND_SET_TERMS),\n+\t\tOPT_CMDMODE(0, \"bisect-next-check\", &cmdmode,\n+\t\t\t N_(\"check whether bad or good terms exist\"), BISECT_NEXT_CHECK),\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_END()\n@@ -369,6 +439,13 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n \t\tstrbuf_addstr(&terms.term_bad, argv[2]);\n \t\tres = check_and_set_terms(&terms, argv[0]);\n \t\tbreak;\n+\tcase BISECT_NEXT_CHECK:\n+\t\tif (argc != 2 && argc != 3)\n+\t\t\tdie(_(\"--bisect-next-check requires 2 or 3 arguments\"));\n+\t\tstrbuf_addstr(&terms.term_good, argv[0]);\n+\t\tstrbuf_addstr(&terms.term_bad, argv[1]);\n+\t\tres = bisect_next_check(&terms, argc == 3 ? argv[2] : NULL);\n+\t\tbreak;\n \tdefault:\n \t\tdie(\"BUG: unknown subcommand '%d'\", cmdmode);\n \t}\ndiff --git a/git-bisect.sh b/git-bisect.sh\nindex a41e69b..c2d6319 100755\n--- a/git-bisect.sh\n+++ b/git-bisect.sh\n@@ -271,59 +271,14 @@ bisect_state() {\n \tbisect_auto_next\n }\n \n-bisect_next_check() {\n-\tmissing_good= missing_bad=\n-\tgit show-ref -q --verify refs/bisect/$TERM_BAD || missing_bad=t\n-\ttest -n \"$(git for-each-ref \"refs/bisect/$TERM_GOOD-*\")\" || missing_good=t\n-\n-\tcase \"$missing_good,$missing_bad,$1\" in\n-\t,,*)\n-\t\t: have both $TERM_GOOD and $TERM_BAD - ok\n-\t\t;;\n-\t*,)\n-\t\t# do not have both but not asked to fail - just report.\n-\t\tfalse\n-\t\t;;\n-\tt,,\"$TERM_GOOD\")\n-\t\t# have bad (or new) but not good (or old). we could bisect although\n-\t\t# this is less optimum.\n-\t\teval_gettextln \"Warning: bisecting only with a \\$TERM_BAD commit.\" >&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 \"Are you sure [Y/n]? \" >&2\n-\t\t\tread yesno\n-\t\t\tcase \"$yesno\" in [Nn]*) exit 1 ;; esac\n-\t\tfi\n-\t\t: bisect without $TERM_GOOD...\n-\t\t;;\n-\t*)\n-\t\tbad_syn=$(bisect_voc bad)\n-\t\tgood_syn=$(bisect_voc good)\n-\t\tif test -s \"$GIT_DIR/BISECT_START\"\n-\t\tthen\n-\n-\t\t\teval_gettextln \"You need to give me at least one \\$bad_syn and one \\$good_syn revision.\n-(You can use \\\"git bisect \\$bad_syn\\\" and \\\"git bisect \\$good_syn\\\" for that.)\" >&2\n-\t\telse\n-\t\t\teval_gettextln \"You need to start by \\\"git bisect start\\\".\n-You then need to give me at least one \\$good_syn and one \\$bad_syn revision.\n-(You can use \\\"git bisect \\$bad_syn\\\" and \\\"git bisect \\$good_syn\\\" for that.)\" >&2\n-\t\tfi\n-\t\texit 1 ;;\n-\tesac\n-}\n-\n bisect_auto_next() {\n-\tbisect_next_check && bisect_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-\tbisect_next_check $TERM_GOOD\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@@ -355,7 +310,7 @@ bisect_next() {\n }\n \n bisect_visualize() {\n-\tbisect_next_check fail\n+\tgit bisect--helper --bisect-next-check $TERM_GOOD $TERM_BAD fail || exit\n \n \tif test $# = 0\n \tthen\n@@ -409,7 +364,7 @@ bisect_replay () {\n }\n \n bisect_run () {\n-\tbisect_next_check fail\n+\tgit bisect--helper --bisect-next-check $TERM_GOOD $TERM_BAD fail || exit\n \n \twhile true\n \tdo\n@@ -482,13 +437,6 @@ get_terms () {\n \tfi\n }\n \n-bisect_voc () {\n-\tcase \"$1\" in\n-\tbad) echo \"bad|new\" ;;\n-\tgood) echo \"good|old\" ;;\n-\tesac\n-}\n-\n bisect_terms () {\n \tget_terms\n \tif ! test -s \"$GIT_DIR/BISECT_TERMS\"\n\n--\nhttps://github.com/git/git/pull/281\n"},{"id":"292675","messageId":"0102015640423d09-87c8e96f-f10f-47dc-b24e-e30ba1de4408-000000@eu-west-1.amazonses.com","threadId":"42979","inReplyTo":"0102015640423c26-2060fd70-c90d-4de3-ae8c-1801ad160b1c-000000@eu-west-1.amazonses.com","subject":"[RFC/PATCH v11 12/13] bisect--helper: `get_terms` & `bisect_terms` shell function in C","fromName":"Pranit Bauva","fromEmail":"pranit.bauva@gmail.com","sentAt":"2016-07-31T09:21:38Z","receivedAt":"2016-07-31T09:39:52Z","isPatch":true,"sender":{"key":"pranit.bauva@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2959938?v=4"},"body":"Reimplement the `get_terms` and `bisect_terms` shell function in C and\nadd `bisect-terms` subcommand to `git bisect--helper` to call it from\ngit-bisect.sh .\n\nIn the shell version, the terms were identified by strings but in C\nversion its done by bit manipulation and passing the integer value to\nthe function.\n\nUsing `--bisect-terms` subcommand is a temporary measure to port shell\nfunction in 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\nMentored-by: Lars Schneider \nMentored-by: Christian Couder \nSigned-off-by: Pranit Bauva \n---\n builtin/bisect--helper.c | 74 +++++++++++++++++++++++++++++++++++++++++++++++-\n git-bisect.sh | 35 ++---------------------\n 2 files changed, 75 insertions(+), 34 deletions(-)\n\ndiff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\nindex 71f4cf0..81a16a5 100644\n--- a/builtin/bisect--helper.c\n+++ b/builtin/bisect--helper.c\n@@ -8,6 +8,13 @@\n #include \"run-command.h\"\n #include \"prompt.h\"\n \n+enum terms_defined {\n+\tTERM_BAD = 1,\n+\tTERM_GOOD = 2,\n+\tTERM_NEW = 4,\n+\tTERM_OLD = 8\n+};\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 static GIT_PATH_FUNC(git_path_bisect_ancestors_ok, \"BISECT_ANCESTORS_OK\")\n@@ -26,6 +33,7 @@ static const char * const git_bisect_helper_usage[] = {\n \tN_(\"git bisect--helper --bisect-write []\"),\n \tN_(\"git bisect--helper --bisect-check-and-set-terms \"),\n \tN_(\"git bisect--helper --bisect-next-check [] term_bad, fp) == EOF ||\n+\t strbuf_getline(&terms->term_good, fp) == EOF;\n+\n+\tfclose(fp);\n+\treturn res ? -1 : 0;\n+}\n+\n+static int bisect_terms(struct bisect_terms *terms, int term_defined)\n+{\n+\tif (get_terms(terms)) {\n+\t\tfprintf(stderr, \"no terms defined\\n\");\n+\t\treturn -1;\n+\t}\n+\tif (!term_defined) {\n+\t\tprintf(\"Your current terms are %s for the old state\\nand \"\n+\t\t \"%s for the new state.\\n\", terms->term_good.buf,\n+\t\t terms->term_bad.buf);\n+\t\treturn 0;\n+\t}\n+\n+\tif (term_defined == TERM_GOOD || term_defined == TERM_OLD)\n+\t\tprintf(\"%s\\n\", terms->term_good.buf);\n+\tif (term_defined == TERM_BAD || term_defined == TERM_NEW)\n+\t\tprintf(\"%s\\n\", terms->term_bad.buf);\n+\n+\treturn 0;\n+}\n+\n int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n {\n \tenum {\n@@ -369,9 +414,11 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n \t\tCHECK_EXPECTED_REVS,\n \t\tBISECT_WRITE,\n \t\tCHECK_AND_SET_TERMS,\n-\t\tBISECT_NEXT_CHECK\n+\t\tBISECT_NEXT_CHECK,\n+\t\tBISECT_TERMS\n \t} cmdmode = 0;\n \tint no_checkout = 0, res = 0;\n+\tenum terms_defined term_defined = 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@@ -389,6 +436,16 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n \t\t\t N_(\"check and set terms in a bisection state\"), CHECK_AND_SET_TERMS),\n \t\tOPT_CMDMODE(0, \"bisect-next-check\", &cmdmode,\n \t\t\t N_(\"check whether bad or good terms exist\"), BISECT_NEXT_CHECK),\n+\t\tOPT_CMDMODE(0, \"bisect-terms\", &cmdmode,\n+\t\t\t N_(\"print out the bisect terms\"), BISECT_TERMS),\n+\t\tOPT_BIT(0, \"term-bad\", &term_defined,\n+\t\t\t N_(\"show the bad term\"), TERM_BAD),\n+\t\tOPT_BIT(0, \"term-good\", &term_defined,\n+\t\t\t N_(\"show the good term\"), TERM_GOOD),\n+\t\tOPT_BIT(0, \"term-new\", &term_defined,\n+\t\t\t N_(\"show the new term\"), TERM_NEW),\n+\t\tOPT_BIT(0, \"term-old\", &term_defined,\n+\t\t\t N_(\"show the old term\"), TERM_OLD),\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_END()\n@@ -399,6 +456,16 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n \targc = parse_options(argc, argv, prefix, options,\n \t\t\t git_bisect_helper_usage, 0);\n \n+\tif (cmdmode != BISECT_TERMS && term_defined)\n+\t\tdie(_(\"--term-bad, --term-good, --term-new and --term-old \"\n+\t\t \"can be used only with --bisect-terms\"));\n+\n+\tif (term_defined != 0 && term_defined != TERM_BAD &&\n+\t term_defined != TERM_GOOD && term_defined != TERM_NEW &&\n+\t term_defined != TERM_OLD)\n+\t\tdie(_(\"only one option among --term-bad, --term-good, \"\n+\t\t \"--term-new and --term-old can be used.\"));\n+\n \tif (!cmdmode)\n \t\tusage_with_options(git_bisect_helper_usage, options);\n \n@@ -446,6 +513,11 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n \t\tstrbuf_addstr(&terms.term_bad, argv[1]);\n \t\tres = bisect_next_check(&terms, argc == 3 ? argv[2] : NULL);\n \t\tbreak;\n+\tcase BISECT_TERMS:\n+\t\tif (argc > 1)\n+\t\t\tdie(_(\"--bisect-terms requires 0 or 1 argument\"));\n+\t\tres = bisect_terms(&terms, term_defined);\n+\t\tbreak;\n \tdefault:\n \t\tdie(\"BUG: unknown subcommand '%d'\", cmdmode);\n \t}\ndiff --git a/git-bisect.sh b/git-bisect.sh\nindex c2d6319..aea97c5f 100755\n--- a/git-bisect.sh\n+++ b/git-bisect.sh\n@@ -355,7 +355,7 @@ bisect_replay () {\n \t\t\"$TERM_GOOD\"|\"$TERM_BAD\"|skip)\n \t\t\tgit bisect--helper --bisect-write \"$command\" \"$rev\" \"$TERM_GOOD\" \"$TERM_BAD\" || exit;;\n \t\tterms)\n-\t\t\tbisect_terms $rev ;;\n+\t\t\tgit bisect--helper --bisect-terms $rev || exit;;\n \t\t*)\n \t\t\tdie \"$(gettext \"?? what are you talking about?\")\" ;;\n \t\tesac\n@@ -437,37 +437,6 @@ get_terms () {\n \tfi\n }\n \n-bisect_terms () {\n-\tget_terms\n-\tif ! test -s \"$GIT_DIR/BISECT_TERMS\"\n-\tthen\n-\t\tdie \"$(gettext \"no terms defined\")\"\n-\tfi\n-\tcase \"$#\" in\n-\t0)\n-\t\tgettextln \"Your current terms are $TERM_GOOD for the old state\n-and $TERM_BAD for the new state.\"\n-\t\t;;\n-\t1)\n-\t\targ=$1\n-\t\tcase \"$arg\" in\n-\t\t\t--term-good|--term-old)\n-\t\t\t\tprintf '%s\\n' \"$TERM_GOOD\"\n-\t\t\t\t;;\n-\t\t\t--term-bad|--term-new)\n-\t\t\t\tprintf '%s\\n' \"$TERM_BAD\"\n-\t\t\t\t;;\n-\t\t\t*)\n-\t\t\t\tdie \"$(eval_gettext \"invalid argument \\$arg for 'git bisect terms'.\n-Supported options are: --term-good|--term-old and --term-bad|--term-new.\")\"\n-\t\t\t\t;;\n-\t\tesac\n-\t\t;;\n-\t*)\n-\t\tusage ;;\n-\tesac\n-}\n-\n case \"$#\" in\n 0)\n \tusage ;;\n@@ -498,7 +467,7 @@ case \"$#\" in\n \trun)\n \t\tbisect_run \"$@\" ;;\n \tterms)\n-\t\tbisect_terms \"$@\" ;;\n+\t\tgit bisect--helper --bisect-terms \"$@\" || exit;;\n \t*)\n \t\tusage ;;\n \tesac\n\n--\nhttps://github.com/git/git/pull/281\n"},{"id":"292676","messageId":"0102015640423ce7-b633a4e8-1a15-4770-ba6c-4331b2c1d941-000000@eu-west-1.amazonses.com","threadId":"42979","inReplyTo":"0102015640423c26-2060fd70-c90d-4de3-ae8c-1801ad160b1c-000000@eu-west-1.amazonses.com","subject":"[RFC/PATCH v11 08/13] bisect--helper: `is_expected_rev` & `check_expected_revs` shell function in C","fromName":"Pranit Bauva","fromEmail":"pranit.bauva@gmail.com","sentAt":"2016-07-31T09:21:38Z","receivedAt":"2016-07-31T09:41:11Z","isPatch":true,"sender":{"key":"pranit.bauva@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2959938?v=4"},"body":"Reimplement `is_expected_rev` & `check_expected_revs` shell function in\nC and add a `--check-expected-revs` subcommand to `git bisect--helper` to\ncall it from git-bisect.sh .\n\nUsing `--check-expected-revs` subcommand is a temporary measure to port\nshell functions to C so as to use the existing test suite. As more\nfunctions are ported, this subcommand would be retired and will be\ncalled by some other method.\n\nHelped-by: Eric Sunshine \nMentored-by: Lars Schneider \nMentored-by: Christian Couder \nSigned-off-by: Pranit Bauva \n---\n builtin/bisect--helper.c | 33 ++++++++++++++++++++++++++++++++-\n git-bisect.sh | 20 ++------------------\n 2 files changed, 34 insertions(+), 19 deletions(-)\n\ndiff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\nindex 0e09630..c0f7091 100644\n--- a/builtin/bisect--helper.c\n+++ b/builtin/bisect--helper.c\n@@ -162,13 +162,40 @@ static int bisect_reset(const char *commit)\n \treturn bisect_clean_state();\n }\n \n+static int is_expected_rev(const char *expected_hex)\n+{\n+\tstruct strbuf actual_hex = STRBUF_INIT;\n+\tint res = 0;\n+\tif (strbuf_read_file(&actual_hex, git_path_bisect_expected_rev(), 0) >= 0) {\n+\t\tstrbuf_trim(&actual_hex);\n+\t\tres = !strcmp(actual_hex.buf, expected_hex);\n+\t}\n+\tstrbuf_release(&actual_hex);\n+\treturn res;\n+}\n+\n+static int check_expected_revs(const char **revs, int rev_nr)\n+{\n+\tint i;\n+\n+\tfor (i = 0; i < rev_nr; i++) {\n+\t\tif (!is_expected_rev(revs[i])) {\n+\t\t\tremove_path(git_path_bisect_ancestors_ok());\n+\t\t\tremove_path(git_path_bisect_expected_rev());\n+\t\t\treturn 0;\n+\t\t}\n+\t}\n+\treturn 0;\n+}\n+\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\tBISECT_CLEAN_STATE,\n-\t\tBISECT_RESET\n+\t\tBISECT_RESET,\n+\t\tCHECK_EXPECTED_REVS\n \t} cmdmode = 0;\n \tint no_checkout = 0;\n \tstruct option options[] = {\n@@ -180,6 +207,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n \t\t\t N_(\"cleanup the bisection state\"), BISECT_CLEAN_STATE),\n \t\tOPT_CMDMODE(0, \"bisect-reset\", &cmdmode,\n \t\t\t N_(\"reset the bisection state\"), BISECT_RESET),\n+\t\tOPT_CMDMODE(0, \"check-expected-revs\", &cmdmode,\n+\t\t\t N_(\"check for expected revs\"), CHECK_EXPECTED_REVS),\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_END()\n@@ -206,6 +235,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n \t\tif (argc > 1)\n \t\t\tdie(_(\"--bisect-reset requires either zero or one arguments\"));\n \t\treturn bisect_reset(argc ? argv[0] : NULL);\n+\tcase CHECK_EXPECTED_REVS:\n+\t\treturn check_expected_revs(argv, argc);\n \tdefault:\n \t\tdie(\"BUG: unknown subcommand '%d'\", cmdmode);\n \t}\ndiff --git a/git-bisect.sh b/git-bisect.sh\nindex 18580b7..4f6545e 100755\n--- a/git-bisect.sh\n+++ b/git-bisect.sh\n@@ -238,22 +238,6 @@ bisect_write() {\n \ttest -n \"$nolog\" || echo \"git bisect $state $rev\" >>\"$GIT_DIR/BISECT_LOG\"\n }\n \n-is_expected_rev() {\n-\ttest -f \"$GIT_DIR/BISECT_EXPECTED_REV\" &&\n-\ttest \"$1\" = $(cat \"$GIT_DIR/BISECT_EXPECTED_REV\")\n-}\n-\n-check_expected_revs() {\n-\tfor _rev in \"$@\"; do\n-\t\tif ! is_expected_rev \"$_rev\"\n-\t\tthen\n-\t\t\trm -f \"$GIT_DIR/BISECT_ANCESTORS_OK\"\n-\t\t\trm -f \"$GIT_DIR/BISECT_EXPECTED_REV\"\n-\t\t\treturn\n-\t\tfi\n-\tdone\n-}\n-\n bisect_skip() {\n \tall=''\n \tfor arg in \"$@\"\n@@ -280,7 +264,7 @@ bisect_state() {\n \t\trev=$(git rev-parse --verify $(bisect_head)) ||\n \t\t\tdie \"$(gettext \"Bad rev input: $(bisect_head)\")\"\n \t\tbisect_write \"$state\" \"$rev\"\n-\t\tcheck_expected_revs \"$rev\" ;;\n+\t\tgit bisect--helper --check-expected-revs \"$rev\" ;;\n \t2,\"$TERM_BAD\"|*,\"$TERM_GOOD\"|*,skip)\n \t\tshift\n \t\thash_list=''\n@@ -294,7 +278,7 @@ bisect_state() {\n \t\tdo\n \t\t\tbisect_write \"$state\" \"$rev\"\n \t\tdone\n-\t\tcheck_expected_revs $hash_list ;;\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\n--\nhttps://github.com/git/git/pull/281\n"},{"id":"292678","messageId":"0102015640423cf7-078bef36-54e8-4200-9097-17d95d4f3c40-000000@eu-west-1.amazonses.com","threadId":"42979","inReplyTo":"0102015640423c26-2060fd70-c90d-4de3-ae8c-1801ad160b1c-000000@eu-west-1.amazonses.com","subject":"[RFC/PATCH v11 09/13] bisect--helper: `bisect_write` shell function in C","fromName":"Pranit Bauva","fromEmail":"pranit.bauva@gmail.com","sentAt":"2016-07-31T09:21:38Z","receivedAt":"2016-07-31T09:54:51Z","isPatch":true,"sender":{"key":"pranit.bauva@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2959938?v=4"},"body":"Reimplement the `bisect_write` shell function in C and add a\n`bisect-write` subcommand to `git bisect--helper` to call it from\ngit-bisect.sh\n\nUsing `--bisect-write` subcommand is a temporary measure to port shell\nfunction in 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\nNote: bisect_write() uses two variables namely TERM_GOOD and TERM_BAD\nfrom the global shell script thus we need to pass it to the subcommand\nusing the arguments. We then store them in a struct bisect_terms and\npass the memory address around functions.\n\nThis patch also introduces new methods namely bisect_state_init() and\nbisect_terms_release() for easy memory management for the struct\nbisect_terms.\n\nMentored-by: Lars Schneider \nMentored-by: Christian Couder \nSigned-off-by: Pranit Bauva \n---\n builtin/bisect--helper.c | 97 ++++++++++++++++++++++++++++++++++++++++++++----\n git-bisect.sh | 25 ++-----------\n 2 files changed, 94 insertions(+), 28 deletions(-)\n\ndiff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\nindex c0f7091..70b953f 100644\n--- a/builtin/bisect--helper.c\n+++ b/builtin/bisect--helper.c\n@@ -22,9 +22,27 @@ static const char * const git_bisect_helper_usage[] = {\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 []\"),\n \tNULL\n };\n \n+struct bisect_terms {\n+\tstruct strbuf term_good;\n+\tstruct strbuf term_bad;\n+};\n+\n+static void bisect_terms_init(struct bisect_terms *terms)\n+{\n+\tstrbuf_init(&terms->term_good, 0);\n+\tstrbuf_init(&terms->term_bad, 0);\n+}\n+\n+static void bisect_terms_release(struct bisect_terms *terms)\n+{\n+\tstrbuf_release(&terms->term_good);\n+\tstrbuf_release(&terms->term_bad);\n+}\n+\n /*\n * Check whether the string `term` belongs to the set of strings\n * included in the variable arguments.\n@@ -188,6 +206,52 @@ static int check_expected_revs(const char **revs, int rev_nr)\n \treturn 0;\n }\n \n+static int bisect_write(const char *state, const char *rev,\n+\t\t\tconst struct bisect_terms *terms, int nolog)\n+{\n+\tstruct strbuf tag = STRBUF_INIT;\n+\tstruct strbuf commit_name = STRBUF_INIT;\n+\tstruct object_id oid;\n+\tstruct commit *commit;\n+\tstruct pretty_print_context pp = {0};\n+\tFILE *fp;\n+\n+\tif (!strcmp(state, terms->term_bad.buf))\n+\t\tstrbuf_addf(&tag, \"refs/bisect/%s\", state);\n+\telse if (one_of(state, terms->term_good.buf, \"skip\", NULL))\n+\t\tstrbuf_addf(&tag, \"refs/bisect/%s-%s\", state, rev);\n+\telse\n+\t\treturn error(_(\"Bad bisect_write argument: %s\"), state);\n+\n+\tif (get_oid(rev, &oid)) {\n+\t\tstrbuf_release(&tag);\n+\t\treturn error(_(\"couldn't get the oid of the rev '%s'\"), rev);\n+\t}\n+\n+\tif (update_ref(NULL, tag.buf, oid.hash, NULL, 0,\n+\t\t UPDATE_REFS_MSG_ON_ERR)) {\n+\t\tstrbuf_release(&tag);\n+\t\treturn -1;\n+\t}\n+\tstrbuf_release(&tag);\n+\n+\tfp = fopen(git_path_bisect_log(), \"a\");\n+\tif (!fp)\n+\t\treturn error_errno(_(\"couldn't open the file '%s'\"), git_path_bisect_log());\n+\n+\tcommit = lookup_commit_reference(oid.hash);\n+\tformat_commit_message(commit, \"%s\", &commit_name, &pp);\n+\tfprintf(fp, \"# %s: [%s] %s\\n\", state, sha1_to_hex(oid.hash),\n+\t\tcommit_name.buf);\n+\tstrbuf_release(&commit_name);\n+\n+\tif (!nolog)\n+\t\tfprintf(fp, \"git bisect %s %s\\n\", state, rev);\n+\n+\tfclose(fp);\n+\treturn 0;\n+}\n+\n int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n {\n \tenum {\n@@ -195,9 +259,10 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n \t\tWRITE_TERMS,\n \t\tBISECT_CLEAN_STATE,\n \t\tBISECT_RESET,\n-\t\tCHECK_EXPECTED_REVS\n+\t\tCHECK_EXPECTED_REVS,\n+\t\tBISECT_WRITE\n \t} cmdmode = 0;\n-\tint no_checkout = 0;\n+\tint no_checkout = 0, res = 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@@ -209,10 +274,14 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n \t\t\t N_(\"reset the bisection state\"), BISECT_RESET),\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-write\", &cmdmode,\n+\t\t\t N_(\"write out the bisection state in BISECT_LOG\"), BISECT_WRITE),\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_END()\n \t};\n+\tstruct bisect_terms terms;\n+\tbisect_terms_init(&terms);\n \n \targc = parse_options(argc, argv, prefix, options,\n \t\t\t git_bisect_helper_usage, 0);\n@@ -221,24 +290,38 @@ 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+\tint nolog;\n \tcase NEXT_ALL:\n \t\treturn bisect_next_all(prefix, no_checkout);\n \tcase WRITE_TERMS:\n \t\tif (argc != 2)\n \t\t\tdie(_(\"--write-terms requires two arguments\"));\n-\t\treturn write_terms(argv[0], argv[1]);\n+\t\tres = write_terms(argv[0], argv[1]);\n+\t\tbreak;\n \tcase BISECT_CLEAN_STATE:\n \t\tif (argc != 0)\n \t\t\tdie(_(\"--bisect-clean-state requires no arguments\"));\n-\t\treturn bisect_clean_state();\n+\t\tres = bisect_clean_state();\n+\t\tbreak;\n \tcase BISECT_RESET:\n \t\tif (argc > 1)\n \t\t\tdie(_(\"--bisect-reset requires either zero or one arguments\"));\n-\t\treturn bisect_reset(argc ? argv[0] : NULL);\n+\t\tres = bisect_reset(argc ? argv[0] : NULL);\n+\t\tbreak;\n \tcase CHECK_EXPECTED_REVS:\n-\t\treturn check_expected_revs(argv, argc);\n+\t\tres = check_expected_revs(argv, argc);\n+\t\tbreak;\n+\tcase BISECT_WRITE:\n+\t\tif (argc != 4 && argc != 5)\n+\t\t\tdie(_(\"--bisect-write requires either 4 or 5 arguments\"));\n+\t\tnolog = (argc == 5) && !strcmp(argv[4], \"nolog\");\n+\t\tstrbuf_addstr(&terms.term_good, argv[2]);\n+\t\tstrbuf_addstr(&terms.term_bad, argv[3]);\n+\t\tres = bisect_write(argv[0], argv[1], &terms, nolog);\n+\t\tbreak;\n \tdefault:\n \t\tdie(\"BUG: unknown subcommand '%d'\", cmdmode);\n \t}\n-\treturn 0;\n+\tbisect_terms_release(&terms);\n+\treturn res;\n }\ndiff --git a/git-bisect.sh b/git-bisect.sh\nindex 4f6545e..b9896a4 100755\n--- a/git-bisect.sh\n+++ b/git-bisect.sh\n@@ -145,7 +145,7 @@ bisect_start() {\n \t\t0) state=$TERM_BAD ; bad_seen=1 ;;\n \t\t*) state=$TERM_GOOD ;;\n \t\tesac\n-\t\teval=\"$eval bisect_write '$state' '$rev' 'nolog' &&\"\n+\t\teval=\"$eval git bisect--helper --bisect-write '$state' '$rev' '$TERM_GOOD' '$TERM_BAD' 'nolog' &&\"\n \tdone\n \t#\n \t# Verify HEAD.\n@@ -221,23 +221,6 @@ bisect_start() {\n \ttrap '-' 0\n }\n \n-bisect_write() {\n-\tstate=\"$1\"\n-\trev=\"$2\"\n-\tnolog=\"$3\"\n-\tcase \"$state\" in\n-\t\t\"$TERM_BAD\")\n-\t\t\ttag=\"$state\" ;;\n-\t\t\"$TERM_GOOD\"|skip)\n-\t\t\ttag=\"$state\"-\"$rev\" ;;\n-\t\t*)\n-\t\t\tdie \"$(eval_gettext \"Bad bisect_write argument: \\$state\")\" ;;\n-\tesac\n-\tgit update-ref \"refs/bisect/$tag\" \"$rev\" || exit\n-\techo \"# $state: $(git show-branch $rev)\" >>\"$GIT_DIR/BISECT_LOG\"\n-\ttest -n \"$nolog\" || echo \"git bisect $state $rev\" >>\"$GIT_DIR/BISECT_LOG\"\n-}\n-\n bisect_skip() {\n \tall=''\n \tfor arg in \"$@\"\n@@ -263,7 +246,7 @@ bisect_state() {\n \t1,\"$TERM_BAD\"|1,\"$TERM_GOOD\"|1,skip)\n \t\trev=$(git rev-parse --verify $(bisect_head)) ||\n \t\t\tdie \"$(gettext \"Bad rev input: $(bisect_head)\")\"\n-\t\tbisect_write \"$state\" \"$rev\"\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@@ -276,7 +259,7 @@ bisect_state() {\n \t\tdone\n \t\tfor rev in $hash_list\n \t\tdo\n-\t\t\tbisect_write \"$state\" \"$rev\"\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@@ -413,7 +396,7 @@ bisect_replay () {\n \t\t\tcmd=\"bisect_start $rev\"\n \t\t\teval \"$cmd\" ;;\n \t\t\"$TERM_GOOD\"|\"$TERM_BAD\"|skip)\n-\t\t\tbisect_write \"$command\" \"$rev\" ;;\n+\t\t\tgit bisect--helper --bisect-write \"$command\" \"$rev\" \"$TERM_GOOD\" \"$TERM_BAD\" || exit;;\n \t\tterms)\n \t\t\tbisect_terms $rev ;;\n \t\t*)\n\n--\nhttps://github.com/git/git/pull/281\n"},{"id":"292679","messageId":"0102015640423cc5-691952fa-3726-4132-bcc1-dfa55d4c0895-000000@eu-west-1.amazonses.com","threadId":"42979","inReplyTo":"0102015640423c26-2060fd70-c90d-4de3-ae8c-1801ad160b1c-000000@eu-west-1.amazonses.com","subject":"[RFC/PATCH v11 02/13] bisect: rewrite `check_term_format` shell function in C","fromName":"Pranit Bauva","fromEmail":"pranit.bauva@gmail.com","sentAt":"2016-07-31T09:21:38Z","receivedAt":"2016-07-31T09:58:12Z","isPatch":true,"sender":{"key":"pranit.bauva@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2959938?v=4"},"body":"Reimplement the `check_term_format` shell function in C and add\na `--check-term-format` subcommand to `git bisect--helper` to call it\nfrom git-bisect.sh\n\nUsing `--check-term-format` 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 will\nbe called by some other method/subcommand. For eg. In conversion of\nwrite_terms() of git-bisect.sh, the subcommand will be removed and\ninstead check_term_format() will be called in its C implementation while\na new subcommand will be introduced for write_terms().\n\nHelped-by: Johannes Schindelein \nMentored-by: Lars Schneider \nMentored-by: Christian Couder \nSigned-off-by: Pranit Bauva \n---\n builtin/bisect--helper.c | 59 +++++++++++++++++++++++++++++++++++++++++++++++-\n git-bisect.sh | 31 ++-----------------------\n 2 files changed, 60 insertions(+), 30 deletions(-)\n\ndiff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\nindex 8111c91..48285d4 100644\n--- a/builtin/bisect--helper.c\n+++ b/builtin/bisect--helper.c\n@@ -2,19 +2,72 @@\n #include \"cache.h\"\n #include \"parse-options.h\"\n #include \"bisect.h\"\n+#include \"refs.h\"\n \n static const char * const git_bisect_helper_usage[] = {\n \tN_(\"git bisect--helper --next-all [--no-checkout]\"),\n+\tN_(\"git bisect--helper --check-term-format \"),\n \tNULL\n };\n \n+/*\n+ * Check whether the string `term` belongs to the set of strings\n+ * included in the variable arguments.\n+ */\n+static int one_of(const char *term, ...)\n+{\n+\tint res = 0;\n+\tva_list matches;\n+\tconst char *match;\n+\n+\tva_start(matches, term);\n+\twhile (!res && (match = va_arg(matches, const char *)))\n+\t\tres = !strcmp(term, match);\n+\tva_end(matches);\n+\n+\treturn res;\n+}\n+\n+static int check_term_format(const char *term, const char *orig_term)\n+{\n+\tint res;\n+\tchar *new_term = xstrfmt(\"refs/bisect/%s\", term);\n+\n+\tres = check_refname_format(new_term, 0);\n+\tfree(new_term);\n+\n+\tif (res)\n+\t\treturn error(_(\"'%s' is not a valid term\"), term);\n+\n+\tif (one_of(term, \"help\", \"start\", \"skip\", \"next\", \"reset\",\n+\t\t\t\"visualize\", \"replay\", \"log\", \"run\", NULL))\n+\t\treturn error(_(\"can't use the builtin command '%s' as a term\"), term);\n+\n+\t/*\n+\t * In theory, nothing prevents swapping completely good and bad,\n+\t * but this situation could be confusing and hasn't been tested\n+\t * enough. Forbid it for now.\n+\t */\n+\n+\tif ((strcmp(orig_term, \"bad\") && one_of(term, \"bad\", \"new\", NULL)) ||\n+\t\t (strcmp(orig_term, \"good\") && one_of(term, \"good\", \"old\", NULL)))\n+\t\treturn error(_(\"can't change the meaning of the term '%s'\"), term);\n+\n+\treturn 0;\n+}\n+\n int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n {\n-\tenum { NEXT_ALL = 1 } cmdmode = 0;\n+\tenum {\n+\t\tNEXT_ALL = 1,\n+\t\tCHECK_TERM_FMT\n+\t} cmdmode = 0;\n \tint no_checkout = 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, \"check-term-format\", &cmdmode,\n+\t\t\t N_(\"check format of the term\"), CHECK_TERM_FMT),\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_END()\n@@ -29,6 +82,10 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n \tswitch (cmdmode) {\n \tcase NEXT_ALL:\n \t\treturn bisect_next_all(prefix, no_checkout);\n+\tcase CHECK_TERM_FMT:\n+\t\tif (argc != 2)\n+\t\t\tdie(_(\"--check-term-format requires two arguments\"));\n+\t\treturn check_term_format(argv[0], argv[1]);\n \tdefault:\n \t\tdie(\"BUG: unknown subcommand '%d'\", cmdmode);\n \t}\ndiff --git a/git-bisect.sh b/git-bisect.sh\nindex 5d1cb00..7d7965d 100755\n--- a/git-bisect.sh\n+++ b/git-bisect.sh\n@@ -564,38 +564,11 @@ write_terms () {\n \tthen\n \t\tdie \"$(gettext \"please use two different terms\")\"\n \tfi\n-\tcheck_term_format \"$TERM_BAD\" bad\n-\tcheck_term_format \"$TERM_GOOD\" good\n+\tgit bisect--helper --check-term-format \"$TERM_BAD\" bad || exit\n+\tgit bisect--helper --check-term-format \"$TERM_GOOD\" good || exit\n \tprintf '%s\\n%s\\n' \"$TERM_BAD\" \"$TERM_GOOD\" >\"$GIT_DIR/BISECT_TERMS\"\n }\n \n-check_term_format () {\n-\tterm=$1\n-\tgit check-ref-format refs/bisect/\"$term\" ||\n-\tdie \"$(eval_gettext \"'\\$term' is not a valid term\")\"\n-\tcase \"$term\" in\n-\thelp|start|terms|skip|next|reset|visualize|replay|log|run)\n-\t\tdie \"$(eval_gettext \"can't use the builtin command '\\$term' as a term\")\"\n-\t\t;;\n-\tbad|new)\n-\t\tif test \"$2\" != bad\n-\t\tthen\n-\t\t\t# In theory, nothing prevents swapping\n-\t\t\t# completely good and bad, but this situation\n-\t\t\t# could be confusing and hasn't been tested\n-\t\t\t# enough. Forbid it for now.\n-\t\t\tdie \"$(eval_gettext \"can't change the meaning of term '\\$term'\")\"\n-\t\tfi\n-\t\t;;\n-\tgood|old)\n-\t\tif test \"$2\" != good\n-\t\tthen\n-\t\t\tdie \"$(eval_gettext \"can't change the meaning of term '\\$term'\")\"\n-\t\tfi\n-\t\t;;\n-\tesac\n-}\n-\n check_and_set_terms () {\n \tcmd=\"$1\"\n \tcase \"$cmd\" in\n\n--\nhttps://github.com/git/git/pull/281\n"},{"id":"292680","messageId":"0102015640423cfd-dbacb682-b6fe-4417-a3f2-2c2c6673263c-000000@eu-west-1.amazonses.com","threadId":"42979","inReplyTo":"0102015640423c26-2060fd70-c90d-4de3-ae8c-1801ad160b1c-000000@eu-west-1.amazonses.com","subject":"[RFC/PATCH v11 10/13] bisect--helper: `check_and_set_terms` shell function in C","fromName":"Pranit Bauva","fromEmail":"pranit.bauva@gmail.com","sentAt":"2016-07-31T09:21:38Z","receivedAt":"2016-07-31T10:04:31Z","isPatch":true,"sender":{"key":"pranit.bauva@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2959938?v=4"},"body":"Reimplement the `check_and_set_terms` shell function in C and add\n`check-and-set-terms` subcommand to `git bisect--helper` to call it from\ngit-bisect.sh\n\nUsing `--check-and-set-terms` subcommand is a temporary measure to port\nshell function in C so as to use the existing test suite. As more\nfunctions are ported, this subcommand will be retired and will be called\nby some other methods.\n\ncheck_and_set_terms() sets and receives two global variables namely\nTERM_GOOD and TERM_BAD in the shell script. Luckily the file BISECT_TERMS\nalso contains the value of those variables so its appropriate to evoke the\nmethod get_terms() after calling the subcommand so that it retrieves the\nvalue of TERM_GOOD and TERM_BAD from the file BISECT_TERMS. The two\nglobal variables are passed as arguments to the subcommand.\n\nAlso introduce bisect_terms_reset() to empty the contents of `term_good`\nand `term_bad` of `struct bisect_terms`.\n\nAlso introduce set_terms() to copy the `term_good` and `term_bad` into\n`struct bisect_terms` and write it out to the file BISECT_TERMS.\n\nMentored-by: Lars Schneider \nMentored-by: Christian Couder \nSigned-off-by: Pranit Bauva \n---\n builtin/bisect--helper.c | 52 +++++++++++++++++++++++++++++++++++++++++++++++-\n git-bisect.sh | 36 ++++-----------------------------\n 2 files changed, 55 insertions(+), 33 deletions(-)\n\ndiff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\nindex 70b953f..99c9f90 100644\n--- a/builtin/bisect--helper.c\n+++ b/builtin/bisect--helper.c\n@@ -23,6 +23,7 @@ static const char * const git_bisect_helper_usage[] = {\n \tN_(\"git bisect--helper --bisect-clean-state\"),\n \tN_(\"git bisect--helper --bisect-reset []\"),\n \tN_(\"git bisect--helper --bisect-write []\"),\n+\tN_(\"git bisect--helper --bisect-check-and-set-terms \"),\n \tNULL\n };\n \n@@ -43,6 +44,12 @@ static void bisect_terms_release(struct bisect_terms *terms)\n \tstrbuf_release(&terms->term_bad);\n }\n \n+static void bisect_terms_reset(struct bisect_terms *term)\n+{\n+\tstrbuf_reset(&term->term_good);\n+\tstrbuf_reset(&term->term_bad);\n+}\n+\n /*\n * Check whether the string `term` belongs to the set of strings\n * included in the variable arguments.\n@@ -252,6 +259,39 @@ static int bisect_write(const char *state, const char *rev,\n \treturn 0;\n }\n \n+static int set_terms(struct bisect_terms *terms, const char *bad,\n+\t\t const char *good)\n+{\n+\tbisect_terms_reset(terms);\n+\tstrbuf_addstr(&terms->term_good, good);\n+\tstrbuf_addstr(&terms->term_bad, bad);\n+\treturn write_terms(terms->term_bad.buf, terms->term_good.buf);\n+}\n+\n+static int check_and_set_terms(struct bisect_terms *terms, const char *cmd)\n+{\n+\tint no_term_file = is_empty_or_missing_file(git_path_bisect_terms());\n+\n+\tif (one_of(cmd, \"skip\", \"start\", \"terms\", NULL))\n+\t\treturn 0;\n+\n+\tif (!no_term_file &&\n+\t strcmp(cmd, terms->term_bad.buf) &&\n+\t strcmp(cmd, terms->term_good.buf))\n+\t\treturn error(_(\"Invalid command: you're currently in a \"\n+\t\t\t\t\"'%s' '%s' bisect\"), terms->term_bad.buf,\n+\t\t\t\tterms->term_good.buf);\n+\n+\tif (no_term_file) {\n+\t\tif (one_of(cmd, \"bad\", \"good\", NULL))\n+\t\t\treturn set_terms(terms, \"bad\", \"good\");\n+\t\tif (one_of(cmd, \"new\", \"old\", NULL))\n+\t\t\treturn set_terms(terms, \"new\", \"old\");\n+\t}\n+\n+\treturn 0;\n+}\n+\n int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n {\n \tenum {\n@@ -260,7 +300,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n \t\tBISECT_CLEAN_STATE,\n \t\tBISECT_RESET,\n \t\tCHECK_EXPECTED_REVS,\n-\t\tBISECT_WRITE\n+\t\tBISECT_WRITE,\n+\t\tCHECK_AND_SET_TERMS\n \t} cmdmode = 0;\n \tint no_checkout = 0, res = 0;\n \tstruct option options[] = {\n@@ -276,6 +317,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n \t\t\t N_(\"check for expected revs\"), CHECK_EXPECTED_REVS),\n \t\tOPT_CMDMODE(0, \"bisect-write\", &cmdmode,\n \t\t\t N_(\"write out the bisection state in BISECT_LOG\"), BISECT_WRITE),\n+\t\tOPT_CMDMODE(0, \"check-and-set-terms\", &cmdmode,\n+\t\t\t N_(\"check and set terms in a bisection state\"), CHECK_AND_SET_TERMS),\n \t\tOPT_BOOL(0, \"no-checkout\", &no_checkout,\n \t\t\t N_(\"update BISECT_HEAD instead of checking out the current commit\")),\n \t\tOPT_END()\n@@ -319,6 +362,13 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n \t\tstrbuf_addstr(&terms.term_bad, argv[3]);\n \t\tres = bisect_write(argv[0], argv[1], &terms, nolog);\n \t\tbreak;\n+\tcase CHECK_AND_SET_TERMS:\n+\t\tif (argc != 3)\n+\t\t\tdie(_(\"--check-and-set-terms requires 3 arguments\"));\n+\t\tstrbuf_addstr(&terms.term_good, argv[1]);\n+\t\tstrbuf_addstr(&terms.term_bad, argv[2]);\n+\t\tres = check_and_set_terms(&terms, argv[0]);\n+\t\tbreak;\n \tdefault:\n \t\tdie(\"BUG: unknown subcommand '%d'\", cmdmode);\n \t}\ndiff --git a/git-bisect.sh b/git-bisect.sh\nindex b9896a4..a41e69b 100755\n--- a/git-bisect.sh\n+++ b/git-bisect.sh\n@@ -239,7 +239,8 @@ bisect_skip() {\n bisect_state() {\n \tbisect_autostart\n \tstate=$1\n-\tcheck_and_set_terms $state\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 \"$(gettext \"Please call 'bisect_state' with at least one argument.\")\" ;;\n@@ -390,7 +391,8 @@ bisect_replay () {\n \t\t\tcommand=\"$bisect\"\n \t\tfi\n \t\tget_terms\n-\t\tcheck_and_set_terms \"$command\"\n+\t\tgit bisect--helper --check-and-set-terms \"$command\" \"$TERM_GOOD\" \"$TERM_BAD\" || exit\n+\t\tget_terms\n \t\tcase \"$command\" in\n \t\tstart)\n \t\t\tcmd=\"bisect_start $rev\"\n@@ -480,36 +482,6 @@ get_terms () {\n \tfi\n }\n \n-check_and_set_terms () {\n-\tcmd=\"$1\"\n-\tcase \"$cmd\" in\n-\tskip|start|terms) ;;\n-\t*)\n-\t\tif test -s \"$GIT_DIR/BISECT_TERMS\" && test \"$cmd\" != \"$TERM_BAD\" && test \"$cmd\" != \"$TERM_GOOD\"\n-\t\tthen\n-\t\t\tdie \"$(eval_gettext \"Invalid command: you're currently in a \\$TERM_BAD/\\$TERM_GOOD bisect.\")\"\n-\t\tfi\n-\t\tcase \"$cmd\" in\n-\t\tbad|good)\n-\t\t\tif ! test -s \"$GIT_DIR/BISECT_TERMS\"\n-\t\t\tthen\n-\t\t\t\tTERM_BAD=bad\n-\t\t\t\tTERM_GOOD=good\n-\t\t\t\tgit bisect--helper --write-terms \"$TERM_BAD\" \"$TERM_GOOD\" || exit\n-\t\t\tfi\n-\t\t\t;;\n-\t\tnew|old)\n-\t\t\tif ! test -s \"$GIT_DIR/BISECT_TERMS\"\n-\t\t\tthen\n-\t\t\t\tTERM_BAD=new\n-\t\t\t\tTERM_GOOD=old\n-\t\t\t\tgit bisect--helper --write-terms \"$TERM_BAD\" \"$TERM_GOOD\" || exit\n-\t\t\tfi\n-\t\t\t;;\n-\t\tesac ;;\n-\tesac\n-}\n-\n bisect_voc () {\n \tcase \"$1\" in\n \tbad) echo \"bad|new\" ;;\n\n--\nhttps://github.com/git/git/pull/281\n"},{"id":"292681","messageId":"0102015640423c26-2060fd70-c90d-4de3-ae8c-1801ad160b1c-000000@eu-west-1.amazonses.com","threadId":"42979","inReplyTo":"010201560a4781be-e8418d6a-5996-45cd-b11b-ca25894ad7f2-000000@eu-west-1.amazonses.com","subject":"[RFC/PATCH v11 01/13] bisect--helper: use OPT_CMDMODE instead of OPT_BOOL","fromName":"Pranit Bauva","fromEmail":"pranit.bauva@gmail.com","sentAt":"2016-07-31T09:21:37Z","receivedAt":"2016-07-31T10:08:52Z","isPatch":true,"sender":{"key":"pranit.bauva@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2959938?v=4"},"body":"`--next-all` is meant to be used as a subcommand to support multiple\n\"operation mode\" though the current implementation does not contain any\nother subcommand along side with `--next-all` but further commits will\ninclude some more subcommands.\n\nHelped-by: Johannes Schindelin \nMentored-by: Lars Schneider \nMentored-by: Christian Couder \nSigned-off-by: Pranit Bauva \n---\n builtin/bisect--helper.c | 17 +++++++++++------\n 1 file changed, 11 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\nindex 3324229..8111c91 100644\n--- a/builtin/bisect--helper.c\n+++ b/builtin/bisect--helper.c\n@@ -10,11 +10,11 @@ static const char * const git_bisect_helper_usage[] = {\n \n int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n {\n-\tint next_all = 0;\n+\tenum { NEXT_ALL = 1 } cmdmode = 0;\n \tint no_checkout = 0;\n \tstruct option options[] = {\n-\t\tOPT_BOOL(0, \"next-all\", &next_all,\n-\t\t\t N_(\"perform 'git bisect next'\")),\n+\t\tOPT_CMDMODE(0, \"next-all\", &cmdmode,\n+\t\t\t N_(\"perform 'git bisect next'\"), NEXT_ALL),\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_END()\n@@ -23,9 +23,14 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n \targc = parse_options(argc, argv, prefix, options,\n \t\t\t git_bisect_helper_usage, 0);\n \n-\tif (!next_all)\n+\tif (!cmdmode)\n \t\tusage_with_options(git_bisect_helper_usage, options);\n \n-\t/* next-all */\n-\treturn bisect_next_all(prefix, no_checkout);\n+\tswitch (cmdmode) {\n+\tcase NEXT_ALL:\n+\t\treturn bisect_next_all(prefix, no_checkout);\n+\tdefault:\n+\t\tdie(\"BUG: unknown subcommand '%d'\", cmdmode);\n+\t}\n+\treturn 0;\n }\n\n--\nhttps://github.com/git/git/pull/281\n"},{"id":"292682","messageId":"0102015640423d18-86d50542-5e5d-40ff-860e-caf56146982d-000000@eu-west-1.amazonses.com","threadId":"42979","inReplyTo":"0102015640423c26-2060fd70-c90d-4de3-ae8c-1801ad160b1c-000000@eu-west-1.amazonses.com","subject":"[RFC/PATCH v11 13/13] bisect--helper: `bisect_start` shell function partially in C","fromName":"Pranit Bauva","fromEmail":"pranit.bauva@gmail.com","sentAt":"2016-07-31T09:21:38Z","receivedAt":"2016-07-31T11:00:06Z","isPatch":true,"sender":{"key":"pranit.bauva@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2959938?v=4"},"body":"Reimplement the `bisect_start` shell function partially in C and add\n`bisect-start` subcommand to `git bisect--helper` to call it from\ngit-bisect.sh .\n\nThe last part is not converted because it calls another shell function.\n`bisect_start` shell function will be completed after the `bisect_next`\nshell function is ported in C.\n\nUsing `--bisect-start` subcommand is a temporary measure to port shell\nfunction in 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\nMentored-by: Lars Schneider \nMentored-by: Christian Couder \nSigned-off-by: Pranit Bauva \n\n---\nThis patch contains a small bug. The option handling for `--term-good`\nand `--term-bad` needs to be decided as it is now shared between\n`--bisect-terms` and `--bisect-start` and the later one also requires\nstring support. Can comments on which approach would seem the most\nfeasible? Here[1] is a normal output of t6030 and here[2] is a verbose\noutput of [2].\n\n[1]: http://paste.ubuntu.com/21252069/\n[2]: http://paste.ubuntu.com/21252140/\n---\n builtin/bisect--helper.c | 232 ++++++++++++++++++++++++++++++++++++++++++++++-\n git-bisect.sh | 132 +--------------------------\n 2 files changed, 232 insertions(+), 132 deletions(-)\n\ndiff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\nindex 81a16a5..ab18786 100644\n--- a/builtin/bisect--helper.c\n+++ b/builtin/bisect--helper.c\n@@ -404,6 +404,228 @@ static int bisect_terms(struct bisect_terms *terms, int term_defined)\n \treturn 0;\n }\n \n+static int bisect_start(struct bisect_terms *terms, int no_checkout,\n+\t\t\tconst char **argv, int argc)\n+{\n+\tint i, j, has_double_dash = 0, must_write_terms = 0, bad_seen = 0;\n+\tint flag;\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+\tconst char *head;\n+\tunsigned char sha1[20];\n+\tFILE *fp;\n+\tstruct object_id oid;\n+\n+\tif (is_bare_repository())\n+\t\tno_checkout = 1;\n+\n+\tfor(i = 0; i < argc; i++) {\n+\t\tif (!strcmp(argv[i], \"--\")) {\n+\t\t\thas_double_dash = 1;\n+\t\t\tbreak;\n+\t\t}\n+\t\tif (!strcmp(argv[i], \"--term-good\")) {\n+\t\t\tmust_write_terms = 1;\n+\t\t\tstrbuf_reset(&terms->term_good);\n+\t\t\tstrbuf_addstr(&terms->term_good, argv[++i]);\n+\t\t\tbreak;\n+\t\t}\n+\t\tif (!strcmp(argv[i], \"--term-bad\")) {\n+\t\t\tmust_write_terms = 1;\n+\t\t\tstrbuf_reset(&terms->term_bad);\n+\t\t\tstrbuf_addstr(&terms->term_bad, argv[++i]);\n+\t\t\tbreak;\n+\t\t}\n+\t\tif (starts_with(argv[i], \"--\") &&\n+\t\t !one_of(argv[i], \"--term-good\", \"--term-bad\", NULL)) {\n+\t\t\tstring_list_clear(&revs, 0);\n+\t\t\tstring_list_clear(&states, 0);\n+\t\t\tdie(_(\"unrecognised option: '%s'\"), argv[i]);\n+\t\t}\n+\t\tif (get_oid(argv[i], &oid) || has_double_dash) {\n+\t\t\tstring_list_clear(&revs, 0);\n+\t\t\tstring_list_clear(&revs, 0);\n+\t\t\tdie(_(\"'%s' does not appear to be a valid revision\"), argv[i]);\n+\t\t}\n+\t\telse\n+\t\t\tstring_list_append(&revs, oid_to_hex(&oid));\n+\t}\n+\n+\tfor (j = 0; j < revs.nr; j++) {\n+\t\tstruct strbuf state = STRBUF_INIT;\n+\t\t/*\n+\t\t * The user ran \"git bisect start \", hence\n+\t\t * did not explicitly specify the terms, but we are already\n+\t\t * starting to set references named with the default terms,\n+\t\t * and won't be able to change afterwards.\n+\t\t */\n+\t\tmust_write_terms = 1;\n+\n+\t\tif (bad_seen)\n+\t\t\tstrbuf_addstr(&state, terms->term_good.buf);\n+\t\telse {\n+\t\t\tbad_seen = 1;\n+\t\t\tstrbuf_addstr(&state, terms->term_bad.buf);\n+\t\t}\n+\t\tstring_list_append(&states, state.buf);\n+\t\tstrbuf_release(&state);\n+\t}\n+\n+\t/*\n+\t * Verify HEAD\n+\t */\n+\thead = resolve_ref_unsafe(\"HEAD\", 0, sha1, &flag);\n+\tif (!head) {\n+\t\tif (get_sha1(\"HEAD\", sha1)) {\n+\t\t\tstring_list_clear(&revs, 0);\n+\t\t\tstring_list_clear(&states, 0);\n+\t\t\tdie(_(\"Bad HEAD - I need a HEAD\"));\n+\t\t}\n+\t}\n+\tif (!is_empty_or_missing_file(git_path_bisect_start())) {\n+\t\t/* Reset to the rev from where we started */\n+\t\tstrbuf_read_file(&start_head, git_path_bisect_start(), 0);\n+\t\tstrbuf_trim(&start_head);\n+\t\tif (!no_checkout) {\n+\t\t\tstruct argv_array argv = ARGV_ARRAY_INIT;\n+\t\t\targv_array_pushl(&argv, \"checkout\", start_head.buf,\n+\t\t\t\t\t \"--\", NULL);\n+\t\t\tif (run_command_v_opt(argv.argv, RUN_GIT_CMD)) {\n+\t\t\t\terror(_(\"checking out '%s' failed. Try again.\"),\n+\t\t\t\t start_head.buf);\n+\t\t\t\tstrbuf_release(&start_head);\n+\t\t\t\tstring_list_clear(&revs, 0);\n+\t\t\t\tstring_list_clear(&states, 0);\n+\t\t\t\treturn -1;\n+\t\t\t}\n+\t\t}\n+\t} else {\n+\t\tif (starts_with(head, \"refs/head/\") || !get_oid(head, &oid)) {\n+\t\t\t/*\n+\t\t\t * This error message should only be triggered by\n+\t\t\t * cogito usage, and cogito users should understand\n+\t\t\t * it relates to cg-seek.\n+\t\t\t */\n+\t\t\tif (!is_empty_or_missing_file(git_path_head_name())) {\n+\t\t\t\tstrbuf_release(&start_head);\n+\t\t\t\tstring_list_clear(&revs, 0);\n+\t\t\t\tstring_list_clear(&states, 0);\n+\t\t\t\tdie(_(\"won't bisect on cg-seek'ed tree\"));\n+\t\t\t}\n+\t\t\tif (starts_with(head, \"refs/heads/\")) {\n+\t\t\t\tstrbuf_reset(&start_head);\n+\t\t\t\tstrbuf_addstr(&start_head, head + 11);\n+\t\t\t}\n+\t\t\telse {\n+\t\t\t\tstrbuf_reset(&start_head);\n+\t\t\t\tstrbuf_addstr(&start_head, sha1_to_hex(sha1));\n+\t\t\t}\n+\t\t} else {\n+\t\t\tstrbuf_release(&start_head);\n+\t\t\tstring_list_clear(&revs, 0);\n+\t\t\tstring_list_clear(&states, 0);\n+\t\t\tdie(_(\"Bad HEAD - strange symbolic ref\"));\n+\t\t}\n+\t}\n+\n+\t/*\n+\t * Get rid of any old bisect state.\n+\t */\n+\tif (bisect_clean_state()) {\n+\t\treturn -1;\n+\t}\n+\n+\t/*\n+\t * Write new start state\n+\t */\n+\tfp = fopen(git_path_bisect_start(), \"w\");\n+\tif (!fp) {\n+\t\tbisect_clean_state();\n+\t\tstrbuf_release(&start_head);\n+\t\tstring_list_clear(&revs, 0);\n+\t\tstring_list_clear(&states, 0);\n+\t\treturn -1;\n+\t}\n+\tif (!fprintf(fp, \"%s\\n\", start_head.buf)) {\n+\t\tfclose(fp);\n+\t\tbisect_clean_state();\n+\t\tstrbuf_release(&start_head);\n+\t\tstring_list_clear(&revs, 0);\n+\t\tstring_list_clear(&states, 0);\n+\t\treturn -1;\n+\t}\n+\tfclose(fp);\n+\n+\tif (no_checkout) {\n+\t\tget_oid(start_head.buf, &oid);\n+\t\tif (update_ref(NULL, \"BISECT_HEAD\", oid.hash, NULL, 0,\n+\t\t\t UPDATE_REFS_MSG_ON_ERR)) {\n+\t\t\tbisect_clean_state();\n+\t\t\tstrbuf_release(&start_head);\n+\t\t\tstring_list_clear(&revs, 0);\n+\t\t\tstring_list_clear(&states, 0);\n+\t\t\treturn -1;\n+\t\t}\n+\t}\n+\tstrbuf_release(&start_head);\n+\tfp = fopen(git_path_bisect_names(), \"w\");\n+\n+\tfor (; i < argc; i++) {\n+\t\tif (!fprintf(fp, \"%s \", argv[i])) {\n+\t\t\tfclose(fp);\n+\t\t\tbisect_clean_state();\n+\t\t\tstring_list_clear(&revs, 0);\n+\t\t\tstring_list_clear(&states, 0);\n+\t\t\treturn -1;\n+\t\t}\n+\t}\n+\tfclose(fp);\n+\n+\tfor (j = 0; j < states.nr; j ++) {\n+\t\tif (bisect_write(states.items[j].string,\n+\t\t\t\t revs.items[j].string, terms, 1)) {\n+\t\t\tbisect_clean_state();\n+\t\t\tstring_list_clear(&revs, 0);\n+\t\t\tstring_list_clear(&states, 0);\n+\t\t\treturn -1;\n+\t\t}\n+\t}\n+\tstring_list_clear(&revs, 0);\n+\tstring_list_clear(&states, 0);\n+\n+\tif (must_write_terms)\n+\t\tif (write_terms(terms->term_bad.buf, terms->term_good.buf)) {\n+\t\t\tbisect_clean_state();\n+\t\t\treturn -1;\n+\t\t}\n+\n+\tfp = fopen(git_path_bisect_log(), \"a\");\n+\tif (!fp) {\n+\t\tbisect_clean_state();\n+\t\treturn -1;\n+\t}\n+\tif (!fprintf(fp, \"git bisect start\")) {\n+\t\tbisect_clean_state();\n+\t\treturn -1;\n+\t}\n+\tfor (i = 0; i < argc; i++) {\n+\t\tif (!fprintf(fp, \" '%s'\", argv[i])) {\n+\t\t\tfclose(fp);\n+\t\t\tbisect_clean_state();\n+\t\t\treturn -1;\n+\t\t}\n+\t}\n+\tif (!fprintf(fp, \"\\n\")) {\n+\t\tfclose(fp);\n+\t\tbisect_clean_state();\n+\t\treturn -1;\n+\t}\n+\tfclose(fp);\n+\n+\treturn 0;\n+}\n+\n int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n {\n \tenum {\n@@ -415,7 +637,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n \t\tBISECT_WRITE,\n \t\tCHECK_AND_SET_TERMS,\n \t\tBISECT_NEXT_CHECK,\n-\t\tBISECT_TERMS\n+\t\tBISECT_TERMS,\n+\t\tBISECT_START\n \t} cmdmode = 0;\n \tint no_checkout = 0, res = 0;\n \tenum terms_defined term_defined = 0;\n@@ -446,6 +669,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n \t\t\t N_(\"show the new term\"), TERM_NEW),\n \t\tOPT_BIT(0, \"term-old\", &term_defined,\n \t\t\t N_(\"show the old term\"), TERM_OLD),\n+\t\tOPT_CMDMODE(0, \"bisect-start\", &cmdmode,\n+\t\t\t N_(\"start the bisect session\"), BISECT_START),\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_END()\n@@ -518,6 +743,11 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n \t\t\tdie(_(\"--bisect-terms requires 0 or 1 argument\"));\n \t\tres = bisect_terms(&terms, term_defined);\n \t\tbreak;\n+\tcase BISECT_START:\n+\t\tstrbuf_addstr(&terms.term_good, \"good\");\n+\t\tstrbuf_addstr(&terms.term_bad, \"bad\");\n+\t\tres = bisect_start(&terms, no_checkout, argv, argc);\n+\t\tbreak;\n \tdefault:\n \t\tdie(\"BUG: unknown subcommand '%d'\", cmdmode);\n \t}\ndiff --git a/git-bisect.sh b/git-bisect.sh\nindex aea97c5f..51d0a71 100755\n--- a/git-bisect.sh\n+++ b/git-bisect.sh\n@@ -72,122 +72,7 @@ bisect_autostart() {\n }\n \n bisect_start() {\n-\t#\n-\t# Check for one bad and then some good revisions.\n-\t#\n-\thas_double_dash=0\n-\tfor arg; do\n-\t\tcase \"$arg\" in --) has_double_dash=1; break ;; esac\n-\tdone\n-\torig_args=$(git rev-parse --sq-quote \"$@\")\n-\tbad_seen=0\n-\teval=''\n-\tmust_write_terms=0\n-\trevs=''\n-\tif test \"z$(git rev-parse --is-bare-repository)\" != zfalse\n-\tthen\n-\t\tmode=--no-checkout\n-\telse\n-\t\tmode=''\n-\tfi\n-\twhile [ $# -gt 0 ]; do\n-\t\targ=\"$1\"\n-\t\tcase \"$arg\" in\n-\t\t--)\n-\t\t\tshift\n-\t\t\tbreak\n-\t\t;;\n-\t\t--no-checkout)\n-\t\t\tmode=--no-checkout\n-\t\t\tshift ;;\n-\t\t--term-good|--term-old)\n-\t\t\tshift\n-\t\t\tmust_write_terms=1\n-\t\t\tTERM_GOOD=$1\n-\t\t\tshift ;;\n-\t\t--term-good=*|--term-old=*)\n-\t\t\tmust_write_terms=1\n-\t\t\tTERM_GOOD=${1#*=}\n-\t\t\tshift ;;\n-\t\t--term-bad|--term-new)\n-\t\t\tshift\n-\t\t\tmust_write_terms=1\n-\t\t\tTERM_BAD=$1\n-\t\t\tshift ;;\n-\t\t--term-bad=*|--term-new=*)\n-\t\t\tmust_write_terms=1\n-\t\t\tTERM_BAD=${1#*=}\n-\t\t\tshift ;;\n-\t\t--*)\n-\t\t\tdie \"$(eval_gettext \"unrecognised option: '\\$arg'\")\" ;;\n-\t\t*)\n-\t\t\trev=$(git rev-parse -q --verify \"$arg^{commit}\") || {\n-\t\t\t\ttest $has_double_dash -eq 1 &&\n-\t\t\t\tdie \"$(eval_gettext \"'\\$arg' does not appear to be a valid revision\")\"\n-\t\t\t\tbreak\n-\t\t\t}\n-\t\t\trevs=\"$revs $rev\"\n-\t\t\tshift\n-\t\t\t;;\n-\t\tesac\n-\tdone\n-\n-\tfor rev in $revs\n-\tdo\n-\t\t# The user ran \"git bisect start \n-\t\t# \", hence did not explicitly specify\n-\t\t# the terms, but we are already starting to\n-\t\t# set references named with the default terms,\n-\t\t# and won't be able to change afterwards.\n-\t\tmust_write_terms=1\n-\n-\t\tcase $bad_seen in\n-\t\t0) state=$TERM_BAD ; bad_seen=1 ;;\n-\t\t*) state=$TERM_GOOD ;;\n-\t\tesac\n-\t\teval=\"$eval git bisect--helper --bisect-write '$state' '$rev' '$TERM_GOOD' '$TERM_BAD' 'nolog' &&\"\n-\tdone\n-\t#\n-\t# Verify HEAD.\n-\t#\n-\thead=$(GIT_DIR=\"$GIT_DIR\" git symbolic-ref -q HEAD) ||\n-\thead=$(GIT_DIR=\"$GIT_DIR\" git rev-parse --verify HEAD) ||\n-\tdie \"$(gettext \"Bad HEAD - I need a HEAD\")\"\n-\n-\t#\n-\t# Check if we are bisecting.\n-\t#\n-\tstart_head=''\n-\tif test -s \"$GIT_DIR/BISECT_START\"\n-\tthen\n-\t\t# Reset to the rev from where we started.\n-\t\tstart_head=$(cat \"$GIT_DIR/BISECT_START\")\n-\t\tif test \"z$mode\" != \"z--no-checkout\"\n-\t\tthen\n-\t\t\tgit checkout \"$start_head\" -- ||\n-\t\t\tdie \"$(eval_gettext \"Checking out '\\$start_head' failed. Try 'git bisect reset '.\")\"\n-\t\tfi\n-\telse\n-\t\t# Get rev from where we start.\n-\t\tcase \"$head\" in\n-\t\trefs/heads/*|$_x40)\n-\t\t\t# This error message should only be triggered by\n-\t\t\t# cogito usage, and cogito users should understand\n-\t\t\t# it relates to cg-seek.\n-\t\t\t[ -s \"$GIT_DIR/head-name\" ] &&\n-\t\t\t\tdie \"$(gettext \"won't bisect on cg-seek'ed tree\")\"\n-\t\t\tstart_head=\"${head#refs/heads/}\"\n-\t\t\t;;\n-\t\t*)\n-\t\t\tdie \"$(gettext \"Bad HEAD - strange symbolic ref\")\"\n-\t\t\t;;\n-\t\tesac\n-\tfi\n-\n-\t#\n-\t# Get rid of any old bisect state.\n-\t#\n-\tgit bisect--helper --bisect-clean-state || exit\n+\tgit bisect--helper --bisect-start $@ || exit\n \n \t#\n \t# Change state.\n@@ -198,21 +83,6 @@ bisect_start() {\n \t#\n \ttrap 'git bisect--helper --bisect-clean-state' 0\n \ttrap 'exit 255' 1 2 3 15\n-\n-\t#\n-\t# Write new start state.\n-\t#\n-\techo \"$start_head\" >\"$GIT_DIR/BISECT_START\" && {\n-\t\ttest \"z$mode\" != \"z--no-checkout\" ||\n-\t\tgit update-ref --no-deref BISECT_HEAD \"$start_head\"\n-\t} &&\n-\tgit rev-parse --sq-quote \"$@\" >\"$GIT_DIR/BISECT_NAMES\" &&\n-\teval \"$eval true\" &&\n-\tif test $must_write_terms -eq 1\n-\tthen\n-\t\tgit bisect--helper --write-terms \"$TERM_BAD\" \"$TERM_GOOD\"\n-\tfi &&\n-\techo \"git bisect start$orig_args\" >>\"$GIT_DIR/BISECT_LOG\" || exit\n \t#\n \t# Check if we can proceed to the next bisect state.\n \t#\n\n--\nhttps://github.com/git/git/pull/281\n"},{"id":"292683","messageId":"0102015640423ce6-5b11201e-736d-413f-be12-7fed613ae484-000000@eu-west-1.amazonses.com","threadId":"42979","inReplyTo":"0102015640423c26-2060fd70-c90d-4de3-ae8c-1801ad160b1c-000000@eu-west-1.amazonses.com","subject":"[RFC/PATCH v11 04/13] bisect--helper: `bisect_clean_state` shell function in C","fromName":"Pranit Bauva","fromEmail":"pranit.bauva@gmail.com","sentAt":"2016-07-31T09:21:38Z","receivedAt":"2016-07-31T11:07:29Z","isPatch":true,"sender":{"key":"pranit.bauva@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2959938?v=4"},"body":"Reimplement `bisect_clean_state` shell function in C and add a\n`bisect-clean-state` subcommand to `git bisect--helper` to call it from\ngit-bisect.sh .\n\nUsing `--bisect-clean-state` subcommand is a 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\nbisect_reset() and bisect_start().\n\nMentored-by: Lars Schneider \nMentored-by: Christian Couder \nSigned-off-by: Pranit Bauva \n---\n builtin/bisect--helper.c | 55 +++++++++++++++++++++++++++++++++++++++++++++++-\n git-bisect.sh | 26 +++--------------------\n 2 files changed, 57 insertions(+), 24 deletions(-)\n\ndiff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\nindex 965bcc1..10dfb68 100644\n--- a/builtin/bisect--helper.c\n+++ b/builtin/bisect--helper.c\n@@ -3,12 +3,21 @@\n #include \"parse-options.h\"\n #include \"bisect.h\"\n #include \"refs.h\"\n+#include \"dir.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+static GIT_PATH_FUNC(git_path_bisect_ancestors_ok, \"BISECT_ANCESTORS_OK\")\n+static GIT_PATH_FUNC(git_path_bisect_log, \"BISECT_LOG\")\n+static GIT_PATH_FUNC(git_path_bisect_names, \"BISECT_NAMES\")\n+static GIT_PATH_FUNC(git_path_bisect_run, \"BISECT_RUN\")\n+static GIT_PATH_FUNC(git_path_head_name, \"head-name\")\n+static GIT_PATH_FUNC(git_path_bisect_start, \"BISECT_START\")\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-clean-state\"),\n \tNULL\n };\n \n@@ -78,11 +87,49 @@ static int write_terms(const char *bad, const char *good)\n \treturn (res < 0) ? -1 : 0;\n }\n \n+static int mark_for_removal(const char *refname, const struct object_id *oid,\n+\t\t\t int flag, void *cb_data)\n+{\n+\tstruct string_list *refs = cb_data;\n+\tchar *ref = xstrfmt(\"refs/bisect/%s\", refname);\n+\tstring_list_append(refs, ref);\n+\treturn 0;\n+}\n+\n+static int bisect_clean_state(void)\n+{\n+\tint result = 0;\n+\n+\t/* There may be some refs packed during bisection */\n+\tstruct string_list refs_for_removal = STRING_LIST_INIT_NODUP;\n+\tfor_each_ref_in(\"refs/bisect/\", mark_for_removal, (void *) &refs_for_removal);\n+\tstring_list_append(&refs_for_removal, xstrdup(\"BISECT_HEAD\"));\n+\tresult = delete_refs(&refs_for_removal);\n+\trefs_for_removal.strdup_strings = 1;\n+\tstring_list_clear(&refs_for_removal, 0);\n+\tremove_path(git_path_bisect_expected_rev());\n+\tremove_path(git_path_bisect_ancestors_ok());\n+\tremove_path(git_path_bisect_log());\n+\tremove_path(git_path_bisect_names());\n+\tremove_path(git_path_bisect_run());\n+\tremove_path(git_path_bisect_terms());\n+\t/* Cleanup head-name if it got left by an old version of git-bisect */\n+\tremove_path(git_path_head_name());\n+\t/*\n+\t * Cleanup BISECT_START last to support the --no-checkout option\n+\t * introduced in the commit 4796e823a.\n+\t */\n+\tremove_path(git_path_bisect_start());\n+\n+\treturn result;\n+}\n+\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,\n+\t\tBISECT_CLEAN_STATE\n \t} cmdmode = 0;\n \tint no_checkout = 0;\n \tstruct option options[] = {\n@@ -90,6 +137,8 @@ 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_BOOL(0, \"no-checkout\", &no_checkout,\n \t\t\t N_(\"update BISECT_HEAD instead of checking out the current commit\")),\n \t\tOPT_END()\n@@ -108,6 +157,10 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n \t\tif (argc != 2)\n \t\t\tdie(_(\"--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\tdie(_(\"--bisect-clean-state requires no arguments\"));\n+\t\treturn bisect_clean_state();\n \tdefault:\n \t\tdie(\"BUG: unknown subcommand '%d'\", cmdmode);\n \t}\ndiff --git a/git-bisect.sh b/git-bisect.sh\nindex cd39bd0..bbc57d2 100755\n--- a/git-bisect.sh\n+++ b/git-bisect.sh\n@@ -187,7 +187,7 @@ bisect_start() {\n \t#\n \t# Get rid of any old bisect state.\n \t#\n-\tbisect_clean_state || exit\n+\tgit bisect--helper --bisect-clean-state || exit\n \n \t#\n \t# Change state.\n@@ -196,7 +196,7 @@ bisect_start() {\n \t# We have to trap this to be able to clean up using\n \t# \"bisect_clean_state\".\n \t#\n-\ttrap 'bisect_clean_state' 0\n+\ttrap 'git bisect--helper --bisect-clean-state' 0\n \ttrap 'exit 255' 1 2 3 15\n \n \t#\n@@ -430,27 +430,7 @@ bisect_reset() {\n \t\tdie \"$(eval_gettext \"Could not check out original HEAD '\\$branch'.\n Try 'git bisect reset '.\")\"\n \tfi\n-\tbisect_clean_state\n-}\n-\n-bisect_clean_state() {\n-\t# There may be some refs packed during bisection.\n-\tgit for-each-ref --format='%(refname) %(objectname)' refs/bisect/\\* |\n-\twhile read ref hash\n-\tdo\n-\t\tgit update-ref -d $ref $hash || exit\n-\tdone\n-\trm -f \"$GIT_DIR/BISECT_EXPECTED_REV\" &&\n-\trm -f \"$GIT_DIR/BISECT_ANCESTORS_OK\" &&\n-\trm -f \"$GIT_DIR/BISECT_LOG\" &&\n-\trm -f \"$GIT_DIR/BISECT_NAMES\" &&\n-\trm -f \"$GIT_DIR/BISECT_RUN\" &&\n-\trm -f \"$GIT_DIR/BISECT_TERMS\" &&\n-\t# Cleanup head-name if it got left by an old version of git-bisect\n-\trm -f \"$GIT_DIR/head-name\" &&\n-\tgit update-ref -d --no-deref BISECT_HEAD &&\n-\t# clean up BISECT_START last\n-\trm -f \"$GIT_DIR/BISECT_START\"\n+\tgit bisect--helper --bisect-clean-state || exit\n }\n \n bisect_replay () {\n\n--\nhttps://github.com/git/git/pull/281\n"},{"id":"292848","messageId":"xmqqbn1bf5gj.fsf@gitster.mtv.corp.google.com","threadId":"42979","inReplyTo":"0102015640423c26-2060fd70-c90d-4de3-ae8c-1801ad160b1c-000000@eu-west-1.amazonses.com","subject":"Re: [RFC/PATCH v11 01/13] bisect--helper: use OPT_CMDMODE instead of OPT_BOOL","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-08-02T17:25:48Z","receivedAt":"2016-08-02T17:27:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Pranit Bauva writes:\n\n> `--next-all` is meant to be used as a subcommand to support multiple\n> \"operation mode\" though the current implementation does not contain any\n> other subcommand along side with `--next-all` but further commits will\n> include some more subcommands.\n\nSounds sensible.\n\nAs long as the dispatch happens inside cmd_bisect__helper() itself,\nlimiting the enum definition local to the function also looks like a\ngood thing to do (and I do not see a reason why we need the world\noutside this function to know about the enum in the future).\n\n\n\n> Helped-by: Johannes Schindelin \n> Mentored-by: Lars Schneider \n> Mentored-by: Christian Couder \n> Signed-off-by: Pranit Bauva \n> ---\n\n\n> builtin/bisect--helper.c | 17 +++++++++++------\n> 1 file changed, 11 insertions(+), 6 deletions(-)\n>\n> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\n> index 3324229..8111c91 100644\n> --- a/builtin/bisect--helper.c\n> +++ b/builtin/bisect--helper.c\n> @@ -10,11 +10,11 @@ static const char * const git_bisect_helper_usage[] = {\n> \n> int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n> {\n> -\tint next_all = 0;\n> +\tenum { NEXT_ALL = 1 } cmdmode = 0;\n> \tint no_checkout = 0;\n> \tstruct option options[] = {\n> -\t\tOPT_BOOL(0, \"next-all\", &next_all,\n> -\t\t\t N_(\"perform 'git bisect next'\")),\n> +\t\tOPT_CMDMODE(0, \"next-all\", &cmdmode,\n> +\t\t\t N_(\"perform 'git bisect next'\"), NEXT_ALL),\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_END()\n> @@ -23,9 +23,14 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n> \targc = parse_options(argc, argv, prefix, options,\n> \t\t\t git_bisect_helper_usage, 0);\n> \n> -\tif (!next_all)\n> +\tif (!cmdmode)\n> \t\tusage_with_options(git_bisect_helper_usage, options);\n> \n> -\t/* next-all */\n> -\treturn bisect_next_all(prefix, no_checkout);\n> +\tswitch (cmdmode) {\n> +\tcase NEXT_ALL:\n> +\t\treturn bisect_next_all(prefix, no_checkout);\n> +\tdefault:\n> +\t\tdie(\"BUG: unknown subcommand '%d'\", cmdmode);\n> +\t}\n> +\treturn 0;\n> }\n>\n> --\n> https://github.com/git/git/pull/281\n"},{"id":"292850","messageId":"xmqq37mnf4vk.fsf@gitster.mtv.corp.google.com","threadId":"42979","inReplyTo":"0102015640423cd0-ae286008-08b7-4ca4-9b2b-f4226aa80a97-000000@eu-west-1.amazonses.com","subject":"Re: [RFC/PATCH v11 03/13] bisect--helper: `write_terms` shell function in C","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-08-02T17:38:23Z","receivedAt":"2016-08-02T17:46:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Pranit Bauva writes:\n\n> +static int write_terms(const char *bad, const char *good)\n> +{\n> +\tFILE *fp;\n> +\tint res;\n> +\n> +\tif (!strcmp(bad, good))\n> +\t\treturn error(_(\"please use two different terms\"));\n> +\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> +\n> +\tres = fprintf(fp, \"%s\\n%s\\n\", bad, good);\n> +\tres |= fclose(fp);\n> +\treturn (res < 0) ? -1 : 0;\n> +}\n\nIf fprintf(3) were a function that returns 0 on success and negative\non error (like fclose(3) is), the pattern to cascade the error\nreturn with \"res |= another_call()\" is appropriate, but the made me\nhiccup a bit while reading it. It is not wrong per-se and it would\ncertainly be making it worse if we did something silly like\n\n\tres = fprintf(...) < 0 ? -1 : 0;\n res |= fclose(fp);\n\nso I guess what you have is the most succinct way to do this.\n\n"},{"id":"292851","messageId":"xmqq7fbzf56s.fsf@gitster.mtv.corp.google.com","threadId":"42979","inReplyTo":"0102015640423cc5-691952fa-3726-4132-bcc1-dfa55d4c0895-000000@eu-west-1.amazonses.com","subject":"Re: [RFC/PATCH v11 02/13] bisect: rewrite `check_term_format` shell function in C","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-08-02T17:31:39Z","receivedAt":"2016-08-02T18:06:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Pranit Bauva writes:\n\n> +/*\n> + * Check whether the string `term` belongs to the set of strings\n> + * included in the variable arguments.\n> + */\n> +static int one_of(const char *term, ...)\n> +{\n> +\tint res = 0;\n> +\tva_list matches;\n> +\tconst char *match;\n> +\n> +\tva_start(matches, term);\n> +\twhile (!res && (match = va_arg(matches, const char *)))\n> +\t\tres = !strcmp(term, match);\n> +\tva_end(matches);\n> +\n> +\treturn res;\n> +}\n\nIt might be safer to mark this function with LAST_ARG_MUST_BE_NULL,\nbut because this is static to this function, it may not matter too\nmuch. Just an observation, not a strong suggestion to change the\npatch.\n\n> +static int check_term_format(const char *term, const char *orig_term)\n> +{\n> +\tint res;\n> +\tchar *new_term = xstrfmt(\"refs/bisect/%s\", term);\n> +\n> +\tres = check_refname_format(new_term, 0);\n> +\tfree(new_term);\n\nYup, that looks much more straight-forward than using a one-time-use\nstrbuf.\n"},{"id":"292856","messageId":"xmqqinvjdn89.fsf@gitster.mtv.corp.google.com","threadId":"42979","inReplyTo":"0102015640423cf1-89276881-65c0-4fc7-9e3e-ba74a6bd3057-000000@eu-west-1.amazonses.com","subject":"Re: [RFC/PATCH v11 07/13] bisect--helper: `bisect_reset` shell function in C","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-08-02T18:44:54Z","receivedAt":"2016-08-02T18:45:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Pranit Bauva writes:\n\n> +\tif (!file_exists(git_path_bisect_head())) {\n> +\t\tstruct argv_array argv = ARGV_ARRAY_INIT;\n> +\t\targv_array_pushl(&argv, \"checkout\", branch.buf, \"--\", NULL);\n> +\t\tif (run_command_v_opt(argv.argv, RUN_GIT_CMD)) {\n> +\t\t\terror(_(\"Could not check out original HEAD '%s'. Try \"\n> +\t\t\t\t\"'git bisect reset '.\"), branch.buf);\n\nSomebody seems to have a keen eye. Looks much better with a space\nafter \"Try\" ;-)\n"},{"id":"292857","messageId":"xmqqeg67dn68.fsf@gitster.mtv.corp.google.com","threadId":"42979","inReplyTo":"0102015640423ce7-b633a4e8-1a15-4770-ba6c-4331b2c1d941-000000@eu-west-1.amazonses.com","subject":"Re: [RFC/PATCH v11 08/13] bisect--helper: `is_expected_rev` & `check_expected_revs` shell function in C","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-08-02T18:46:07Z","receivedAt":"2016-08-02T18:47:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Pranit Bauva writes:\n\n> +\tfor (i = 0; i < rev_nr; i++) {\n> +\t\tif (!is_expected_rev(revs[i])) {\n> +\t\t\tremove_path(git_path_bisect_ancestors_ok());\n> +\t\t\tremove_path(git_path_bisect_expected_rev());\n> +\t\t\treturn 0;\n\nSame comment on the use of this helper function applies here, too.\n"},{"id":"292858","messageId":"xmqqa8gvdmtx.fsf@gitster.mtv.corp.google.com","threadId":"42979","inReplyTo":"0102015640423cfd-dbacb682-b6fe-4417-a3f2-2c2c6673263c-000000@eu-west-1.amazonses.com","subject":"Re: [RFC/PATCH v11 10/13] bisect--helper: `check_and_set_terms` shell function in C","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-08-02T18:53:30Z","receivedAt":"2016-08-02T18:57:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Pranit Bauva writes:\n\n> Reimplement the `check_and_set_terms` shell function in C and add\n> `check-and-set-terms` subcommand to `git bisect--helper` to call it from\n> git-bisect.sh\n>\n> Using `--check-and-set-terms` subcommand is a temporary measure to port\n> shell function in C so as to use the existing test suite. As more\n> functions are ported, this subcommand will be retired and will be called\n> by some other methods.\n\nI think \"this subcommand will be retired but its implementation will\nbe called by ...\" would clarify the direction.\n\n> +\tif (!no_term_file &&\n> +\t strcmp(cmd, terms->term_bad.buf) &&\n> +\t strcmp(cmd, terms->term_good.buf))\n> +\t\treturn error(_(\"Invalid command: you're currently in a \"\n> +\t\t\t\t\"'%s' '%s' bisect\"), terms->term_bad.buf,\n\nThis changes a message text, switching from \"... good/bad bisect.\"\nto \"... 'good' 'bad' bisect\". Intended?\n\n"},{"id":"292859","messageId":"xmqqy44fdpxd.fsf@gitster.mtv.corp.google.com","threadId":"42979","inReplyTo":"0102015640423ce6-5b11201e-736d-413f-be12-7fed613ae484-000000@eu-west-1.amazonses.com","subject":"Re: [RFC/PATCH v11 04/13] bisect--helper: `bisect_clean_state` shell function in C","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-08-02T17:46:38Z","receivedAt":"2016-08-02T19:00:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Pranit Bauva writes:\n\n> +static int bisect_clean_state(void)\n> +{\n> +\tint result = 0;\n> +\n> +\t/* There may be some refs packed during bisection */\n> +\tstruct string_list refs_for_removal = STRING_LIST_INIT_NODUP;\n> +\tfor_each_ref_in(\"refs/bisect/\", mark_for_removal, (void *) &refs_for_removal);\n> +\tstring_list_append(&refs_for_removal, xstrdup(\"BISECT_HEAD\"));\n> +\tresult = delete_refs(&refs_for_removal);\n> +\trefs_for_removal.strdup_strings = 1;\n> +\tstring_list_clear(&refs_for_removal, 0);\n> +\tremove_path(git_path_bisect_expected_rev());\n> +\tremove_path(git_path_bisect_ancestors_ok());\n> +\tremove_path(git_path_bisect_log());\n> +\tremove_path(git_path_bisect_names());\n> +\tremove_path(git_path_bisect_run());\n> +\tremove_path(git_path_bisect_terms());\n> +\t/* Cleanup head-name if it got left by an old version of git-bisect */\n> +\tremove_path(git_path_head_name());\n> +\t * Cleanup BISECT_START last to support the --no-checkout option\n> +\t * introduced in the commit 4796e823a.\n> +\t */\n> +\tremove_path(git_path_bisect_start());\n\nI can see that refs/files-backend.c misuses it already, but\nremove_path() helper is about removing a path in the working tree,\ntogether with any parent directory that becomes empty due to the\nremoval. You do not expect $GIT_DIR/ to become an empty directory\nafter removing $GIT_DIR/BISECT_LOG nor want to rmdir $GIT_DIR even\nif it becomes empty. It is a wrong helper function to use here.\n\nAlso you do not seem to check the error from the function to smudge\nthe \"result\" you are returning from this function.\n\nIsn't unlink_or_warn() more correct helper to use here?\n\n> +\treturn result;\n> +}\n"},{"id":"292860","messageId":"xmqq60rjdlq4.fsf@gitster.mtv.corp.google.com","threadId":"42979","inReplyTo":"0102015640423d08-3af7e66f-4104-4b0c-a3c7-dff29b843b2d-000000@eu-west-1.amazonses.com","subject":"Re: [RFC/PATCH v11 11/13] bisect--helper: `bisect_next_check` shell function in C","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-08-02T19:17:23Z","receivedAt":"2016-08-02T19:17:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Pranit Bauva writes:\n\n> +static int mark_good(const char *refname, const struct object_id *oid,\n> +\t\t int flag, void *cb_data)\n> +{\n> +\tint *m_good = (int *)cb_data;\n> +\t*m_good = 0;\n> +\treturn 0;\n> +}\n\nSee below.\n\n> +static int bisect_next_check(const struct bisect_terms *terms,\n> +\t\t\t const char *current_term)\n> +{\n> +\tint missing_good = 1, missing_bad = 1;\n\nIt is somewhat unusual to start with \"assume we are OK\" and then\n\"it turns out that we are not\".\n\n> +\tchar *bad_ref = xstrfmt(\"refs/bisect/%s\", terms->term_bad.buf);\n> +\tchar *good_glob = xstrfmt(\"%s*\", terms->term_good.buf);\n\nThe original runs\n\n git for-each-ref \"refs/bisect/$TERM_GOOD-*\n\nbut this one lacks the final dash.\n\n> +\tif (ref_exists(bad_ref))\n> +\t\tmissing_bad = 0;\n> +\tfree(bad_ref);\n> +\n> +\tfor_each_glob_ref_in(mark_good, good_glob, \"refs/bisect/\",\n> +\t\t\t (void *) &missing_good);\n> +\tfree(good_glob);\n\nThe for-each helper does not return until it iterates over all the\nmatching refs, but you are only interested in seeing if at least one\nexists. It may make sense to return 1 from mark_good() to terminate\nthe traversal early.\n\n> +\tif (!missing_good && !missing_bad)\n> +\t\treturn 0;\n> +\n> +\tif (!current_term)\n> +\t\treturn -1;\n> +\n> +\tif (missing_good && !missing_bad && current_term &&\n> +\t !strcmp(current_term, terms->term_good.buf)) {\n> +\t\tchar *yesno;\n> +\t\t/*\n> +\t\t * have bad (or new) but not good (or old). We could bisect\n> +\t\t * although this is less optimum.\n> +\t\t */\n> +\t\tfprintf(stderr, \"Warning: bisecting only with a %s commit\\n\",\n> +\t\t\tterms->term_bad.buf);\n\nIn the original, this message goes through gettext.\n\n> +\t\tif (!isatty(0))\n> +\t\t\treturn 0;\n> +\t\t/*\n> +\t\t * TRANSLATORS: Make sure to include [Y] and [n] in your\n> +\t\t * translation. The program will only accept English input\n> +\t\t * at this point.\n> +\t\t */\n> +\t\tyesno = git_prompt(_(\"Are you sure [Y/n]? \"), PROMPT_ECHO);\n> +\t\tif (starts_with(yesno, \"N\") || starts_with(yesno, \"n\"))\n> +\t\t\treturn -1;\n> +\t\treturn 0;\n> +\t}\n\nWhen the control falls into the above if(){} block, the function\nwill always return. It will clarify that this is the end of such a\nlogical block to have a blank line here.\n\n> +\tif (!is_empty_or_missing_file(git_path_bisect_start()))\n> +\t\treturn error(_(\"You need to give me at least one good|old and \"\n> +\t\t\t\t\"bad|new revision. You can use \\\"git bisect \"\n> +\t\t\t\t\"bad|new\\\" and \\\"git bisect good|old\\\" for \"\n> +\t\t\t\t\"that. \\n\"));\n> +\telse\n> +\t\treturn error(_(\"You need to start by \\\"git bisect start\\\". \"\n> +\t\t\t\t\"You then need to give me at least one good|\"\n> +\t\t\t\t\"old and bad|new revision. You can use \\\"git \"\n> +\t\t\t\t\"bisect bad|new\\\" and \\\"git bisect good|old\\\" \"\n> +\t\t\t\t\" for that.\\n\"));\n\nThe i18n on these two messages seem to be different from the\noriginal, which asks bisect_voc to learn what 'bad' and 'good' are\ncalled and attempts to use these words from the vocabulary.\n"},{"id":"292861","messageId":"xmqq1t27dlhr.fsf@gitster.mtv.corp.google.com","threadId":"42979","inReplyTo":"0102015640423d09-87c8e96f-f10f-47dc-b24e-e30ba1de4408-000000@eu-west-1.amazonses.com","subject":"Re: [RFC/PATCH v11 12/13] bisect--helper: `get_terms` & `bisect_terms` shell function in C","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-08-02T19:22:24Z","receivedAt":"2016-08-02T19:22:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Pranit Bauva writes:\n\n> +static int bisect_terms(struct bisect_terms *terms, int term_defined)\n> +{\n> +\tif (get_terms(terms)) {\n> +\t\tfprintf(stderr, \"no terms defined\\n\");\n> +\t\treturn -1;\n> +\t}\n> +\tif (!term_defined) {\n> +\t\tprintf(\"Your current terms are %s for the old state\\nand \"\n> +\t\t \"%s for the new state.\\n\", terms->term_good.buf,\n> +\t\t terms->term_bad.buf);\n> +\t\treturn 0;\n> +\t}\n\nIn the original, all of the above messages go through gettext; this\nrewrite should do the same.\n"},{"id":"292863","messageId":"xmqqr3a6divf.fsf@gitster.mtv.corp.google.com","threadId":"42979","inReplyTo":"0102015640423d18-86d50542-5e5d-40ff-860e-caf56146982d-000000@eu-west-1.amazonses.com","subject":"Re: [RFC/PATCH v11 13/13] bisect--helper: `bisect_start` shell function partially in C","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-08-02T20:19:00Z","receivedAt":"2016-08-02T20:25:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Pranit Bauva writes:\n\n> +static int bisect_start(struct bisect_terms *terms, int no_checkout,\n> +\t\t\tconst char **argv, int argc)\n> +{\n> +\tint i, j, has_double_dash = 0, must_write_terms = 0, bad_seen = 0;\n> +\tint flag;\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> +\tconst char *head;\n> +\tunsigned char sha1[20];\n> +\tFILE *fp;\n> +\tstruct object_id oid;\n> +\n> +\tif (is_bare_repository())\n> +\t\tno_checkout = 1;\n> +\n> +\tfor(i = 0; i < argc; i++) {\n\nSP after for.\n\n> +\t\tif (!strcmp(argv[i], \"--\")) {\n> +\t\t\thas_double_dash = 1;\n> +\t\t\tbreak;\n> +\t\t}\n> +\t\tif (!strcmp(argv[i], \"--term-good\")) {\n> +\t\t\tmust_write_terms = 1;\n> +\t\t\tstrbuf_reset(&terms->term_good);\n> +\t\t\tstrbuf_addstr(&terms->term_good, argv[++i]);\n> +\t\t\tbreak;\n> +\t\t}\n> +\t\tif (!strcmp(argv[i], \"--term-bad\")) {\n> +\t\t\tmust_write_terms = 1;\n> +\t\t\tstrbuf_reset(&terms->term_bad);\n> +\t\t\tstrbuf_addstr(&terms->term_bad, argv[++i]);\n> +\t\t\tbreak;\n> +\t\t}\n\nThe original was not careful, either, but what if the user ends the\ncommand line with \"--term-good\", without anything after it?\n\nAlso the original is prepared to handle --term-good=boa; because\nthis function can be be called directly from the UI (i.e. \"git\nbisect start --term-good=boa\"), not supporting that form would be\nseen as a regression.\n\n> +\t\tif (starts_with(argv[i], \"--\") &&\n> +\t\t !one_of(argv[i], \"--term-good\", \"--term-bad\", NULL)) {\n> +\t\t\tstring_list_clear(&revs, 0);\n> +\t\t\tstring_list_clear(&states, 0);\n> +\t\t\tdie(_(\"unrecognised option: '%s'\"), argv[i]);\n> +\t\t}\n> +\t\tif (get_oid(argv[i], &oid) || has_double_dash) {\n\nCalling get_oid() alone is insufficient to make sure argv[i] refers\nto an existing object that is a committish. The \"^{commit}\" suffix\nin the original is there for a reason.\n\n> +\t\t\tstring_list_clear(&revs, 0);\n> +\t\t\tstring_list_clear(&revs, 0);\n\nYou seem to want the revs list really really clean ;-)\n\n> +\t\t\tdie(_(\"'%s' does not appear to be a valid revision\"), argv[i]);\n> +\t\t}\n> +\t\telse\n> +\t\t\tstring_list_append(&revs, oid_to_hex(&oid));\n> +\t}\n> +\n> +\tfor (j = 0; j < revs.nr; j++) {\n\nWhy \"j\", not \"i\", as clearly the previous loop has finished at this\npoint? The only reason why replacing \"j\" with \"i\" would make this\nfunction buggy would be if a later part of this function depended on\nthe value of \"i\" when the control left the above loop, but if that\nwere the case (I didn't check carefully), such a precious value that\nhas long term effect throughout the remainder of the function must\nnot be kept in an otherwise throw-away loop counter variable \"i\".\n\nIntroduce a new \"int pathspec_pos\" and set it to \"i\" immediately\nafter the \"for (i = 0; i < argc; i++) { ... }\" loop above, perhaps.\n\n> +\t\tstruct strbuf state = STRBUF_INIT;\n> +\t\t/*\n> +\t\t * The user ran \"git bisect start \", hence\n> +\t\t * did not explicitly specify the terms, but we are already\n> +\t\t * starting to set references named with the default terms,\n> +\t\t * and won't be able to change afterwards.\n> +\t\t */\n> +\t\tmust_write_terms = 1;\n> +\n> +\t\tif (bad_seen)\n> +\t\t\tstrbuf_addstr(&state, terms->term_good.buf);\n> +\t\telse {\n> +\t\t\tbad_seen = 1;\n> +\t\t\tstrbuf_addstr(&state, terms->term_bad.buf);\n> +\t\t}\n> +\t\tstring_list_append(&states, state.buf);\n> +\t\tstrbuf_release(&state);\n> +\t}\n\nHow about this instead?\n\n\t/*\n * that comment block goes here\n */\n \tmust_write_terms = !!revs.nr;\n\tfor (i = 0; i < revs.nr; i++) {\n if (bad_seen)\n \tstring_list_append(&states, terms->term_good.buf);\n\t\telse\n \tstring_list_append(&states, terms->term_bad.buf);\n\t}\n\n> +\n> +\t/*\n> +\t * Verify HEAD\n> +\t */\n> +\thead = resolve_ref_unsafe(\"HEAD\", 0, sha1, &flag);\n\nThe last parameter is a set of flag bits, so call it flags.\n\n> +\tif (!head) {\n> +\t\tif (get_sha1(\"HEAD\", sha1)) {\n> +\t\t\tstring_list_clear(&revs, 0);\n> +\t\t\tstring_list_clear(&states, 0);\n> +\t\t\tdie(_(\"Bad HEAD - I need a HEAD\"));\n\nWe see many repeated calls to clear these two string lists before\nexiting with failure, either by dying or return -1.\n\nI wonder how bad the resulting code would look like if we employed\nthe standard pattern of having a \"fail_return:\" label at the end of\nthe function (after the \"return\" for the usual control flow) to\nclear them. If the result becomes less readable (and I suspect that\nyou would end up making it less readable), leaving the current code\nstructure is OK.\n\n> +\t\t}\n> +\t}\n> +\tif (!is_empty_or_missing_file(git_path_bisect_start())) {\n> +\t\t/* Reset to the rev from where we started */\n> +\t\tstrbuf_read_file(&start_head, git_path_bisect_start(), 0);\n> +\t\tstrbuf_trim(&start_head);\n> +\t\tif (!no_checkout) {\n> +\t\t\tstruct argv_array argv = ARGV_ARRAY_INIT;\n> +\t\t\targv_array_pushl(&argv, \"checkout\", start_head.buf,\n> +\t\t\t\t\t \"--\", NULL);\n> +\t\t\tif (run_command_v_opt(argv.argv, RUN_GIT_CMD)) {\n> +\t\t\t\terror(_(\"checking out '%s' failed. Try again.\"),\n> +\t\t\t\t start_head.buf);\n\nThe original suggests to try \"git bisect reset\" here to recover.\n\n> +\t\t\t\tstrbuf_release(&start_head);\n> +\t\t\t\tstring_list_clear(&revs, 0);\n> +\t\t\t\tstring_list_clear(&states, 0);\n> +\t\t\t\treturn -1;\n> +\t\t\t}\n> +\t\t}\n> +\t} else {\n> +\t\tif (starts_with(head, \"refs/head/\") || !get_oid(head, &oid)) {\n\nget_oid() is insufficient to ensure what you have in $head is\n40-hex. I think you meant get_oid_hex() here.\n\n> +\t\t\t/*\n> +\t\t\t * This error message should only be triggered by\n> +\t\t\t * cogito usage, and cogito users should understand\n> +\t\t\t * it relates to cg-seek.\n> +\t\t\t */\n> +\t\t\tif (!is_empty_or_missing_file(git_path_head_name())) {\n> +\t\t\t\tstrbuf_release(&start_head);\n> +\t\t\t\tstring_list_clear(&revs, 0);\n> +\t\t\t\tstring_list_clear(&states, 0);\n> +\t\t\t\tdie(_(\"won't bisect on cg-seek'ed tree\"));\n> +\t\t\t}\n> +\t\t\tif (starts_with(head, \"refs/heads/\")) {\n\nskip_prefix(), perhaps, if \"head\" is no longer used from here on?\n\n> +\t/*\n> +\t * Write new start state\n> +\t */\n> +\tfp = fopen(git_path_bisect_start(), \"w\");\n> +\tif (!fp) {\n> +\t\tbisect_clean_state();\n> +\t\tstrbuf_release(&start_head);\n> +\t\tstring_list_clear(&revs, 0);\n> +\t\tstring_list_clear(&states, 0);\n> +\t\treturn -1;\n> +\t}\n> +\tif (!fprintf(fp, \"%s\\n\", start_head.buf)) {\n\nman 3 fprintf and look for \"Return Value\"?\n\n> +\t\tfclose(fp);\n> +\t\tbisect_clean_state();\n> +\t\tstrbuf_release(&start_head);\n> +\t\tstring_list_clear(&revs, 0);\n> +\t\tstring_list_clear(&states, 0);\n> +\t\treturn -1;\n> +\t}\n> +\tfclose(fp);\n\nPerhaps use write_file() instead of the above block of text?\n\n> +\tif (no_checkout) {\n> +\t\tget_oid(start_head.buf, &oid);\n> +\t\tif (update_ref(NULL, \"BISECT_HEAD\", oid.hash, NULL, 0,\n> +\t\t\t UPDATE_REFS_MSG_ON_ERR)) {\n\nDoesn't the original use --no-deref for this update-ref call?\n\n> +\t\t\tbisect_clean_state();\n> +\t\t\tstrbuf_release(&start_head);\n> +\t\t\tstring_list_clear(&revs, 0);\n> +\t\t\tstring_list_clear(&states, 0);\n> +\t\t\treturn -1;\n> +\t\t}\n> +\t}\n> +\tstrbuf_release(&start_head);\n> +\tfp = fopen(git_path_bisect_names(), \"w\");\n> +\n> +\tfor (; i < argc; i++) {\n> +\t\tif (!fprintf(fp, \"%s \", argv[i])) {\n\nman 3 fprintf and look for \"Return Value\"?\n\nMore importantly, the original does --sq-quote so that BISECT_NAMES\nfile can be read back by a shell. This is important as argv[i] can\nhave whitespace in it, and you are concatenating them with SP in\nbetween here. Also you are not terminating that line.\n\n> +\t\t\tfclose(fp);\n> +\t\t\tbisect_clean_state();\n> +\t\t\tstring_list_clear(&revs, 0);\n> +\t\t\tstring_list_clear(&states, 0);\n> +\t\t\treturn -1;\n> +\t\t}\n> +\t}\n> +\tfclose(fp);\n\nPerhaps\n\n\tstrbuf_reset(&bisect_names);\n\tif (pathspec_pos < argc)\n\t\tsq_quote_argv(&bisect_names, argv + pathspec_pos, 0);\n\twrite_file(git_path_bisect_names(), \"%s\\n\", bisect_names.buf);\n\nor something like that?\n\n> +\tfor (j = 0; j < states.nr; j ++) {\n\nAgain, is \"i\" still precious here? Style: drop SP between j and ++.\n\n> +\tfp = fopen(git_path_bisect_log(), \"a\");\n> +\tif (!fp) {\n> +\t\tbisect_clean_state();\n> +\t\treturn -1;\n> +\t}\n> +\tif (!fprintf(fp, \"git bisect start\")) {\n> +\t\tbisect_clean_state();\n> +\t\treturn -1;\n> +\t}\n> +\tfor (i = 0; i < argc; i++) {\n> +\t\tif (!fprintf(fp, \" '%s'\", argv[i])) {\n> +\t\t\tfclose(fp);\n> +\t\t\tbisect_clean_state();\n> +\t\t\treturn -1;\n> +\t\t}\n> +\t}\n> +\tif (!fprintf(fp, \"\\n\")) {\n> +\t\tfclose(fp);\n> +\t\tbisect_clean_state();\n> +\t\treturn -1;\n> +\t}\n\nAgain, the original writes orig_args which was protected with --sq-quote.\n\n> +\tfclose(fp);\n> +\n> +\treturn 0;\n> +}\n> +\n"},{"id":"292864","messageId":"xmqqmvkudiju.fsf@gitster.mtv.corp.google.com","threadId":"42979","inReplyTo":"0102015640423cf7-078bef36-54e8-4200-9097-17d95d4f3c40-000000@eu-west-1.amazonses.com","subject":"Re: [RFC/PATCH v11 09/13] bisect--helper: `bisect_write` shell function in C","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-08-02T20:25:57Z","receivedAt":"2016-08-02T20:26:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Pranit Bauva writes:\n\n> Reimplement the `bisect_write` shell function in C and add a\n> `bisect-write` subcommand to `git bisect--helper` to call it from\n> git-bisect.sh\n\nUp to around this step we've seen these patches well enough and I\nthink with another reroll or two, they are in good enough shape to\nbe split out and frozen for 'next'. We may not be there quite yet,\nbut I think we are getting pretty close.\n\nThanks.\n\n"},{"id":"292876","messageId":"xmqqpopqbysv.fsf@gitster.mtv.corp.google.com","threadId":"42979","inReplyTo":"xmqqmvkudiju.fsf@gitster.mtv.corp.google.com","subject":"Re: [RFC/PATCH v11 09/13] bisect--helper: `bisect_write` shell function in C","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-08-02T22:17:52Z","receivedAt":"2016-08-02T22:30:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Pranit Bauva writes:\n>\n>> Reimplement the `bisect_write` shell function in C and add a\n>> `bisect-write` subcommand to `git bisect--helper` to call it from\n>> git-bisect.sh\n>\n> Up to around this step we've seen these patches well enough and I\n> think with another reroll or two, they are in good enough shape to\n> be split out and frozen for 'next'. We may not be there quite yet,\n> but I think we are getting pretty close.\n>\n> Thanks.\n\nBy the way, this series applied as a whole seems to break t6030.\n"},{"id":"292982","messageId":"CAFZEwPMwen+5wrs3fEOiLUOyf3RrUTV2rftt1c0D4XCVm-UzHg@mail.gmail.com","threadId":"42979","inReplyTo":"xmqq7fbzf56s.fsf@gitster.mtv.corp.google.com","subject":"Re: [RFC/PATCH v11 02/13] bisect: rewrite `check_term_format` shell function in C","fromName":"Pranit Bauva","fromEmail":"pranit.bauva@gmail.com","sentAt":"2016-08-03T20:20:34Z","receivedAt":"2016-08-03T20:21:47Z","isPatch":true,"sender":{"key":"pranit.bauva@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2959938?v=4"},"body":"Hey Junio,\n\nOn Tue, Aug 2, 2016 at 11:01 PM, Junio C Hamano wrote:\n> Pranit Bauva writes:\n>\n>> +/*\n>> + * Check whether the string `term` belongs to the set of strings\n>> + * included in the variable arguments.\n>> + */\n>> +static int one_of(const char *term, ...)\n>> +{\n>> + int res = 0;\n>> + va_list matches;\n>> + const char *match;\n>> +\n>> + va_start(matches, term);\n>> + while (!res && (match = va_arg(matches, const char *)))\n>> + res = !strcmp(term, match);\n>> + va_end(matches);\n>> +\n>> + return res;\n>> +}\n>\n> It might be safer to mark this function with LAST_ARG_MUST_BE_NULL,\n> but because this is static to this function, it may not matter too\n> much. Just an observation, not a strong suggestion to change the\n> patch.\n\nYes, I could do that.\n\n>> +static int check_term_format(const char *term, const char *orig_term)\n>> +{\n>> + int res;\n>> + char *new_term = xstrfmt(\"refs/bisect/%s\", term);\n>> +\n>> + res = check_refname_format(new_term, 0);\n>> + free(new_term);\n>\n> Yup, that looks much more straight-forward than using a one-time-use\n> strbuf.\n\nThanks!\n\nRegards,\nPranit Bauva\n"},{"id":"292983","messageId":"CAFZEwPNFz60GrX9ZqaFAvCU4HoNg8rmuqa0t5+DXFjaEHTmDeg@mail.gmail.com","threadId":"42979","inReplyTo":"xmqq37mnf4vk.fsf@gitster.mtv.corp.google.com","subject":"Re: [RFC/PATCH v11 03/13] bisect--helper: `write_terms` shell function in C","fromName":"Pranit Bauva","fromEmail":"pranit.bauva@gmail.com","sentAt":"2016-08-03T20:21:48Z","receivedAt":"2016-08-03T20:22:08Z","isPatch":true,"sender":{"key":"pranit.bauva@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2959938?v=4"},"body":"Hey Junio,\n\nOn Tue, Aug 2, 2016 at 11:08 PM, Junio C Hamano wrote:\n> Pranit Bauva writes:\n>\n>> +static int write_terms(const char *bad, const char *good)\n>> +{\n>> + FILE *fp;\n>> + int res;\n>> +\n>> + if (!strcmp(bad, good))\n>> + return error(_(\"please use two different terms\"));\n>> +\n>> + if (check_term_format(bad, \"bad\") || check_term_format(good, \"good\"))\n>> + return -1;\n>> +\n>> + fp = fopen(git_path_bisect_terms(), \"w\");\n>> + if (!fp)\n>> + return error_errno(_(\"could not open the file BISECT_TERMS\"));\n>> +\n>> + res = fprintf(fp, \"%s\\n%s\\n\", bad, good);\n>> + res |= fclose(fp);\n>> + return (res < 0) ? -1 : 0;\n>> +}\n>\n> If fprintf(3) were a function that returns 0 on success and negative\n> on error (like fclose(3) is), the pattern to cascade the error\n> return with \"res |= another_call()\" is appropriate, but the made me\n> hiccup a bit while reading it. It is not wrong per-se and it would\n> certainly be making it worse if we did something silly like\n>\n> res = fprintf(...) < 0 ? -1 : 0;\n> res |= fclose(fp);\n>\n> so I guess what you have is the most succinct way to do this.\n\nI agree with your point and your suggested code is better!\n\nRegards,\nPranit Bauva\n"},{"id":"292987","messageId":"CAFZEwPOERUocOiRvWR6fj1KZm7wcVrp41FVFbhWpdZBMAabUpA@mail.gmail.com","threadId":"42979","inReplyTo":"xmqqa8gvdmtx.fsf@gitster.mtv.corp.google.com","subject":"Re: [RFC/PATCH v11 10/13] bisect--helper: `check_and_set_terms` shell function in C","fromName":"Pranit Bauva","fromEmail":"pranit.bauva@gmail.com","sentAt":"2016-08-03T20:29:39Z","receivedAt":"2016-08-03T20:31:12Z","isPatch":true,"sender":{"key":"pranit.bauva@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2959938?v=4"},"body":"Hey Junio,\n\nOn Wed, Aug 3, 2016 at 12:23 AM, Junio C Hamano wrote:\n> Pranit Bauva writes:\n>\n>> Reimplement the `check_and_set_terms` shell function in C and add\n>> `check-and-set-terms` subcommand to `git bisect--helper` to call it from\n>> git-bisect.sh\n>>\n>> Using `--check-and-set-terms` subcommand is a temporary measure to port\n>> shell function in C so as to use the existing test suite. As more\n>> functions are ported, this subcommand will be retired and will be called\n>> by some other methods.\n>\n> I think \"this subcommand will be retired but its implementation will\n> be called by ...\" would clarify the direction.\n\nSure. That seems better.\n\n>> + if (!no_term_file &&\n>> + strcmp(cmd, terms->term_bad.buf) &&\n>> + strcmp(cmd, terms->term_good.buf))\n>> + return error(_(\"Invalid command: you're currently in a \"\n>> + \"'%s' '%s' bisect\"), terms->term_bad.buf,\n>\n> This changes a message text, switching from \"... good/bad bisect.\"\n> to \"... 'good' 'bad' bisect\". Intended?\n\nNope its not intended but its a mistake from my side. Will rectify.\n\nRegards,\nPranit Bauva\n"},{"id":"292988","messageId":"CAFZEwPMK5-WXVNts1r=wxZdDVJTTCf6tauSHdWDjRbijqFJ6pg@mail.gmail.com","threadId":"42979","inReplyTo":"xmqq1t27dlhr.fsf@gitster.mtv.corp.google.com","subject":"Re: [RFC/PATCH v11 12/13] bisect--helper: `get_terms` & `bisect_terms` shell function in C","fromName":"Pranit Bauva","fromEmail":"pranit.bauva@gmail.com","sentAt":"2016-08-03T20:33:59Z","receivedAt":"2016-08-03T20:34:28Z","isPatch":true,"sender":{"key":"pranit.bauva@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2959938?v=4"},"body":"Hey Junio,\n\nOn Wed, Aug 3, 2016 at 12:52 AM, Junio C Hamano wrote:\n> Pranit Bauva writes:\n>\n>> +static int bisect_terms(struct bisect_terms *terms, int term_defined)\n>> +{\n>> + if (get_terms(terms)) {\n>> + fprintf(stderr, \"no terms defined\\n\");\n>> + return -1;\n>> + }\n>> + if (!term_defined) {\n>> + printf(\"Your current terms are %s for the old state\\nand \"\n>> + \"%s for the new state.\\n\", terms->term_good.buf,\n>> + terms->term_bad.buf);\n>> + return 0;\n>> + }\n>\n> In the original, all of the above messages go through gettext; this\n> rewrite should do the same.\n\nSure!\n\nRegards,\nPranit Bauva\n"},{"id":"292992","messageId":"CAFZEwPPOn5tRk1MWaGJ5NHakNvtv7W0QfDAYDh+hzn8xLPA1NA@mail.gmail.com","threadId":"42979","inReplyTo":"xmqq60rjdlq4.fsf@gitster.mtv.corp.google.com","subject":"Re: [RFC/PATCH v11 11/13] bisect--helper: `bisect_next_check` shell function in C","fromName":"Pranit Bauva","fromEmail":"pranit.bauva@gmail.com","sentAt":"2016-08-03T20:33:32Z","receivedAt":"2016-08-03T20:47:34Z","isPatch":true,"sender":{"key":"pranit.bauva@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2959938?v=4"},"body":"Hey Junio,\n\nOn Wed, Aug 3, 2016 at 12:47 AM, Junio C Hamano wrote:\n> Pranit Bauva writes:\n>\n>> +static int mark_good(const char *refname, const struct object_id *oid,\n>> + int flag, void *cb_data)\n>> +{\n>> + int *m_good = (int *)cb_data;\n>> + *m_good = 0;\n>> + return 0;\n>> +}\n>\n> See below.\n>\n>> +static int bisect_next_check(const struct bisect_terms *terms,\n>> + const char *current_term)\n>> +{\n>> + int missing_good = 1, missing_bad = 1;\n>\n> It is somewhat unusual to start with \"assume we are OK\" and then\n> \"it turns out that we are not\".\n>\n>> + char *bad_ref = xstrfmt(\"refs/bisect/%s\", terms->term_bad.buf);\n>> + char *good_glob = xstrfmt(\"%s*\", terms->term_good.buf);\n>\n> The original runs\n>\n> git for-each-ref \"refs/bisect/$TERM_GOOD-*\n>\n> but this one lacks the final dash.\n\nMy bad. Will include it.\n\n>> + if (ref_exists(bad_ref))\n>> + missing_bad = 0;\n>> + free(bad_ref);\n>> +\n>> + for_each_glob_ref_in(mark_good, good_glob, \"refs/bisect/\",\n>> + (void *) &missing_good);\n>> + free(good_glob);\n>\n> The for-each helper does not return until it iterates over all the\n> matching refs, but you are only interested in seeing if at least one\n> exists. It may make sense to return 1 from mark_good() to terminate\n> the traversal early.\n\nSeems a better way. Thanks!\n\n>> + if (!missing_good && !missing_bad)\n>> + return 0;\n>> +\n>> + if (!current_term)\n>> + return -1;\n>> +\n>> + if (missing_good && !missing_bad && current_term &&\n>> + !strcmp(current_term, terms->term_good.buf)) {\n>> + char *yesno;\n>> + /*\n>> + * have bad (or new) but not good (or old). We could bisect\n>> + * although this is less optimum.\n>> + */\n>> + fprintf(stderr, \"Warning: bisecting only with a %s commit\\n\",\n>> + terms->term_bad.buf);\n>\n> In the original, this message goes through gettext.\n\nWill do.\n\n>> + if (!isatty(0))\n>> + return 0;\n>> + /*\n>> + * TRANSLATORS: Make sure to include [Y] and [n] in your\n>> + * translation. The program will only accept English input\n>> + * at this point.\n>> + */\n>> + yesno = git_prompt(_(\"Are you sure [Y/n]? \"), PROMPT_ECHO);\n>> + if (starts_with(yesno, \"N\") || starts_with(yesno, \"n\"))\n>> + return -1;\n>> + return 0;\n>> + }\n>\n> When the control falls into the above if(){} block, the function\n> will always return. It will clarify that this is the end of such a\n> logical block to have a blank line here.\n\nWill do.\n\n>> + if (!is_empty_or_missing_file(git_path_bisect_start()))\n>> + return error(_(\"You need to give me at least one good|old and \"\n>> + \"bad|new revision. You can use \\\"git bisect \"\n>> + \"bad|new\\\" and \\\"git bisect good|old\\\" for \"\n>> + \"that. \\n\"));\n>> + else\n>> + return error(_(\"You need to start by \\\"git bisect start\\\". \"\n>> + \"You then need to give me at least one good|\"\n>> + \"old and bad|new revision. You can use \\\"git \"\n>> + \"bisect bad|new\\\" and \\\"git bisect good|old\\\" \"\n>> + \" for that.\\n\"));\n>\n> The i18n on these two messages seem to be different from the\n> original, which asks bisect_voc to learn what 'bad' and 'good' are\n> called and attempts to use these words from the vocabulary.\n\nI have little idea about i18n. Will look more into it.\n\nRegards,\nPranit Bauva\n"},{"id":"292993","messageId":"CAFZEwPOv4VHmTKUOunCnri6WsXzyM-8gPLHgbW-LoxqVD1k2-g@mail.gmail.com","threadId":"42979","inReplyTo":"xmqqr3a6divf.fsf@gitster.mtv.corp.google.com","subject":"Re: [RFC/PATCH v11 13/13] bisect--helper: `bisect_start` shell function partially in C","fromName":"Pranit Bauva","fromEmail":"pranit.bauva@gmail.com","sentAt":"2016-08-03T20:49:01Z","receivedAt":"2016-08-03T20:50:01Z","isPatch":true,"sender":{"key":"pranit.bauva@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2959938?v=4"},"body":"Hey Junio,\n\nOn Wed, Aug 3, 2016 at 1:49 AM, Junio C Hamano wrote:\n> Pranit Bauva writes:\n>\n>> +static int bisect_start(struct bisect_terms *terms, int no_checkout,\n>> + const char **argv, int argc)\n>> +{\n>> + int i, j, has_double_dash = 0, must_write_terms = 0, bad_seen = 0;\n>> + int flag;\n>> + struct string_list revs = STRING_LIST_INIT_DUP;\n>> + struct string_list states = STRING_LIST_INIT_DUP;\n>> + struct strbuf start_head = STRBUF_INIT;\n>> + const char *head;\n>> + unsigned char sha1[20];\n>> + FILE *fp;\n>> + struct object_id oid;\n>> +\n>> + if (is_bare_repository())\n>> + no_checkout = 1;\n>> +\n>> + for(i = 0; i < argc; i++) {\n>\n> SP after for.\n\nSure!\n\n>> + if (!strcmp(argv[i], \"--\")) {\n>> + has_double_dash = 1;\n>> + break;\n>> + }\n>> + if (!strcmp(argv[i], \"--term-good\")) {\n>> + must_write_terms = 1;\n>> + strbuf_reset(&terms->term_good);\n>> + strbuf_addstr(&terms->term_good, argv[++i]);\n>> + break;\n>> + }\n>> + if (!strcmp(argv[i], \"--term-bad\")) {\n>> + must_write_terms = 1;\n>> + strbuf_reset(&terms->term_bad);\n>> + strbuf_addstr(&terms->term_bad, argv[++i]);\n>> + break;\n>> + }\n>\n> The original was not careful, either, but what if the user ends the\n> command line with \"--term-good\", without anything after it?\n\n> Also the original is prepared to handle --term-good=boa; because\n> this function can be be called directly from the UI (i.e. \"git\n> bisect start --term-good=boa\"), not supporting that form would be\n> seen as a regression.\n\n\nI wanted to discuss this precisely by this RFC. I was initially\nthinking of using OPT_ARGUMENT() for bisect_terms() which would in\nturn cover up for bisect_start() too. Currently the code does not\nsupport --term-good=boa because it treats --term-good as a boolean Do\nyou have any other thing in mind?\n\n>> + if (starts_with(argv[i], \"--\") &&\n>> + !one_of(argv[i], \"--term-good\", \"--term-bad\", NULL)) {\n>> + string_list_clear(&revs, 0);\n>> + string_list_clear(&states, 0);\n>> + die(_(\"unrecognised option: '%s'\"), argv[i]);\n>> + }\n>> + if (get_oid(argv[i], &oid) || has_double_dash) {\n>\n> Calling get_oid() alone is insufficient to make sure argv[i] refers\n> to an existing object that is a committish. The \"^{commit}\" suffix\n> in the original is there for a reason.\n\nYes sure!\n\n>> + string_list_clear(&revs, 0);\n>> + string_list_clear(&revs, 0);\n>\n> You seem to want the revs list really really clean ;-)\n\nHaha! ;) My bad. Will remove the extra line!\n\n>> + die(_(\"'%s' does not appear to be a valid revision\"), argv[i]);\n>> + }\n>> + else\n>> + string_list_append(&revs, oid_to_hex(&oid));\n>> + }\n>> +\n>> + for (j = 0; j < revs.nr; j++) {\n>\n> Why \"j\", not \"i\", as clearly the previous loop has finished at this\n> point? The only reason why replacing \"j\" with \"i\" would make this\n> function buggy would be if a later part of this function depended on\n> the value of \"i\" when the control left the above loop, but if that\n> were the case (I didn't check carefully), such a precious value that\n> has long term effect throughout the remainder of the function must\n> not be kept in an otherwise throw-away loop counter variable \"i\".\n>\n> Introduce a new \"int pathspec_pos\" and set it to \"i\" immediately\n> after the \"for (i = 0; i < argc; i++) { ... }\" loop above, perhaps.\n\nI am using i afterwards for writing the arguments to BISECT_NAMES. But\nI think it would be better to use pathspec_pos and discard j\naltogether. Thanks!\n>\n>> + struct strbuf state = STRBUF_INIT;\n>> + /*\n>> + * The user ran \"git bisect start \", hence\n>> + * did not explicitly specify the terms, but we are already\n>> + * starting to set references named with the default terms,\n>> + * and won't be able to change afterwards.\n>> + */\n>> + must_write_terms = 1;\n>> +\n>> + if (bad_seen)\n>> + strbuf_addstr(&state, terms->term_good.buf);\n>> + else {\n>> + bad_seen = 1;\n>> + strbuf_addstr(&state, terms->term_bad.buf);\n>> + }\n>> + string_list_append(&states, state.buf);\n>> + strbuf_release(&state);\n>> + }\n>\n> How about this instead?\n>\n> /*\n> * that comment block goes here\n> */\n> must_write_terms = !!revs.nr;\n> for (i = 0; i < revs.nr; i++) {\n> if (bad_seen)\n> string_list_append(&states, terms->term_good.buf);\n> else\n> string_list_append(&states, terms->term_bad.buf);\n> }\n\nSeems better. Thanks!\n\n>> +\n>> + /*\n>> + * Verify HEAD\n>> + */\n>> + head = resolve_ref_unsafe(\"HEAD\", 0, sha1, &flag);\n>\n> The last parameter is a set of flag bits, so call it flags.\n\nSure!\n\n>> + if (!head) {\n>> + if (get_sha1(\"HEAD\", sha1)) {\n>> + string_list_clear(&revs, 0);\n>> + string_list_clear(&states, 0);\n>> + die(_(\"Bad HEAD - I need a HEAD\"));\n>\n> We see many repeated calls to clear these two string lists before\n> exiting with failure, either by dying or return -1.\n>\n> I wonder how bad the resulting code would look like if we employed\n> the standard pattern of having a \"fail_return:\" label at the end of\n> the function (after the \"return\" for the usual control flow) to\n> clear them. If the result becomes less readable (and I suspect that\n> you would end up making it less readable), leaving the current code\n> structure is OK.\n\nIts becoming really bad. I tried it once. Different things are being\ncleaned up at different times which makes it all the more tedious to\nmaintain.\n\n>> + }\n>> + }\n>> + if (!is_empty_or_missing_file(git_path_bisect_start())) {\n>> + /* Reset to the rev from where we started */\n>> + strbuf_read_file(&start_head, git_path_bisect_start(), 0);\n>> + strbuf_trim(&start_head);\n>> + if (!no_checkout) {\n>> + struct argv_array argv = ARGV_ARRAY_INIT;\n>> + argv_array_pushl(&argv, \"checkout\", start_head.buf,\n>> + \"--\", NULL);\n>> + if (run_command_v_opt(argv.argv, RUN_GIT_CMD)) {\n>> + error(_(\"checking out '%s' failed. Try again.\"),\n>> + start_head.buf);\n>\n> The original suggests to try \"git bisect reset\" here to recover.\n\nWill include it.\n\n>> + strbuf_release(&start_head);\n>> + string_list_clear(&revs, 0);\n>> + string_list_clear(&states, 0);\n>> + return -1;\n>> + }\n>> + }\n>> + } else {\n>> + if (starts_with(head, \"refs/head/\") || !get_oid(head, &oid)) {\n>\n> get_oid() is insufficient to ensure what you have in $head is\n> 40-hex. I think you meant get_oid_hex() here.\n\nYes definitely. Might have been missed.\n\n>> + /*\n>> + * This error message should only be triggered by\n>> + * cogito usage, and cogito users should understand\n>> + * it relates to cg-seek.\n>> + */\n>> + if (!is_empty_or_missing_file(git_path_head_name())) {\n>> + strbuf_release(&start_head);\n>> + string_list_clear(&revs, 0);\n>> + string_list_clear(&states, 0);\n>> + die(_(\"won't bisect on cg-seek'ed tree\"));\n>> + }\n>> + if (starts_with(head, \"refs/heads/\")) {\n>\n> skip_prefix(), perhaps, if \"head\" is no longer used from here on?\n\nSure!\n\n>> + /*\n>> + * Write new start state\n>> + */\n>> + fp = fopen(git_path_bisect_start(), \"w\");\n>> + if (!fp) {\n>> + bisect_clean_state();\n>> + strbuf_release(&start_head);\n>> + string_list_clear(&revs, 0);\n>> + string_list_clear(&states, 0);\n>> + return -1;\n>> + }\n>> + if (!fprintf(fp, \"%s\\n\", start_head.buf)) {\n>\n> man 3 fprintf and look for \"Return Value\"?\n\nI should have been more careful about fprintf's return value.\n\n>> + fclose(fp);\n>> + bisect_clean_state();\n>> + strbuf_release(&start_head);\n>> + string_list_clear(&revs, 0);\n>> + string_list_clear(&states, 0);\n>> + return -1;\n>> + }\n>> + fclose(fp);\n>\n> Perhaps use write_file() instead of the above block of text?\n\nYes, that seems better. Thanks!\n\n>> + if (no_checkout) {\n>> + get_oid(start_head.buf, &oid);\n>> + if (update_ref(NULL, \"BISECT_HEAD\", oid.hash, NULL, 0,\n>> + UPDATE_REFS_MSG_ON_ERR)) {\n>\n> Doesn't the original use --no-deref for this update-ref call?\n\nYes, will change.\n\n>> + bisect_clean_state();\n>> + strbuf_release(&start_head);\n>> + string_list_clear(&revs, 0);\n>> + string_list_clear(&states, 0);\n>> + return -1;\n>> + }\n>> + }\n>> + strbuf_release(&start_head);\n>> + fp = fopen(git_path_bisect_names(), \"w\");\n>> +\n>> + for (; i < argc; i++) {\n>> + if (!fprintf(fp, \"%s \", argv[i])) {\n>\n> man 3 fprintf and look for \"Return Value\"?\n>\n> More importantly, the original does --sq-quote so that BISECT_NAMES\n> file can be read back by a shell. This is important as argv[i] can\n> have whitespace in it, and you are concatenating them with SP in\n> between here. Also you are not terminating that line.\n\nYes its a good idea to retain its --sq-quote nature.\n\n>> + fclose(fp);\n>> + bisect_clean_state();\n>> + string_list_clear(&revs, 0);\n>> + string_list_clear(&states, 0);\n>> + return -1;\n>> + }\n>> + }\n>> + fclose(fp);\n>\n> Perhaps\n>\n> strbuf_reset(&bisect_names);\n> if (pathspec_pos < argc)\n> sq_quote_argv(&bisect_names, argv + pathspec_pos, 0);\n> write_file(git_path_bisect_names(), \"%s\\n\", bisect_names.buf);\n>\n> or something like that?\n\nYes! Thanks! Looks pretty good to me.\n\n>> + for (j = 0; j < states.nr; j ++) {\n>\n> Again, is \"i\" still precious here? Style: drop SP between j and ++.\n\nAfter BISECT_NAMES, \"i\" ceases to be precious.\n\n>> + fp = fopen(git_path_bisect_log(), \"a\");\n>> + if (!fp) {\n>> + bisect_clean_state();\n>> + return -1;\n>> + }\n>> + if (!fprintf(fp, \"git bisect start\")) {\n>> + bisect_clean_state();\n>> + return -1;\n>> + }\n>> + for (i = 0; i < argc; i++) {\n>> + if (!fprintf(fp, \" '%s'\", argv[i])) {\n>> + fclose(fp);\n>> + bisect_clean_state();\n>> + return -1;\n>> + }\n>> + }\n>> + if (!fprintf(fp, \"\\n\")) {\n>> + fclose(fp);\n>> + bisect_clean_state();\n>> + return -1;\n>> + }\n>\n> Again, the original writes orig_args which was protected with --sq-quote.\n\nYes. Will take care of it.\n\n>> + fclose(fp);\n>> +\n>> + return 0;\n>> +}\n>> +\n\nRegards,\nPranit Bauva\n"},{"id":"292994","messageId":"CAFZEwPNeSV6b-MM7eabjAqBYWvYoJDhGYPrqzL9sy84+GxA6hA@mail.gmail.com","threadId":"42979","inReplyTo":"xmqqmvkudiju.fsf@gitster.mtv.corp.google.com","subject":"Re: [RFC/PATCH v11 09/13] bisect--helper: `bisect_write` shell function in C","fromName":"Pranit Bauva","fromEmail":"pranit.bauva@gmail.com","sentAt":"2016-08-03T20:51:08Z","receivedAt":"2016-08-03T20:52:04Z","isPatch":true,"sender":{"key":"pranit.bauva@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2959938?v=4"},"body":"Hey Junio,\n\nOn Wed, Aug 3, 2016 at 1:55 AM, Junio C Hamano wrote:\n> Pranit Bauva writes:\n>\n>> Reimplement the `bisect_write` shell function in C and add a\n>> `bisect-write` subcommand to `git bisect--helper` to call it from\n>> git-bisect.sh\n>\n> Up to around this step we've seen these patches well enough and I\n> think with another reroll or two, they are in good enough shape to\n> be split out and frozen for 'next'. We may not be there quite yet,\n> but I think we are getting pretty close.\n\nThanks for taking out your time to review this series. I will follow\nup with the patches in a day or so. I think the first 11, many things\nwouldn't change (except for your reviews). Most of the changes would\nhappen in 12 and 13 and more specifically the argument handling.\n\nRegards,\nPranit Bauva\n"},{"id":"292995","messageId":"CAFZEwPMLUCjafW44zdwYS4T-0F9ejgRSW8cN70TsUj26aeEWPA@mail.gmail.com","threadId":"42979","inReplyTo":"xmqqpopqbysv.fsf@gitster.mtv.corp.google.com","subject":"Re: [RFC/PATCH v11 09/13] bisect--helper: `bisect_write` shell function in C","fromName":"Pranit Bauva","fromEmail":"pranit.bauva@gmail.com","sentAt":"2016-08-03T20:52:03Z","receivedAt":"2016-08-03T20:52:10Z","isPatch":true,"sender":{"key":"pranit.bauva@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2959938?v=4"},"body":"Hey Junio,\n\nOn Wed, Aug 3, 2016 at 3:47 AM, Junio C Hamano wrote:\n> Junio C Hamano writes:\n>\n>> Pranit Bauva writes:\n>>\n>>> Reimplement the `bisect_write` shell function in C and add a\n>>> `bisect-write` subcommand to `git bisect--helper` to call it from\n>>> git-bisect.sh\n>>\n>> Up to around this step we've seen these patches well enough and I\n>> think with another reroll or two, they are in good enough shape to\n>> be split out and frozen for 'next'. We may not be there quite yet,\n>> but I think we are getting pretty close.\n>>\n>> Thanks.\n>\n> By the way, this series applied as a whole seems to break t6030.\n\nThis is only because of argument handling problem. I seemed to have\nmentioned it in patch 13/13 and also gave the output of the tests.\n\nRegards,\nPranit Bauva\n"},{"id":"293025","messageId":"CAFZEwPNFohRLCS4piB1t0LO-=keucwzRKZ2Jyhhf234tnWnVGg@mail.gmail.com","threadId":"42979","inReplyTo":"xmqqy44fdpxd.fsf@gitster.mtv.corp.google.com","subject":"Re: [RFC/PATCH v11 04/13] bisect--helper: `bisect_clean_state` shell function in C","fromName":"Pranit Bauva","fromEmail":"pranit.bauva@gmail.com","sentAt":"2016-08-03T20:27:54Z","receivedAt":"2016-08-03T22:25:50Z","isPatch":true,"sender":{"key":"pranit.bauva@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2959938?v=4"},"body":"Hey Junio,\n\nOn Tue, Aug 2, 2016 at 11:16 PM, Junio C Hamano wrote:\n> Pranit Bauva writes:\n>\n>> +static int bisect_clean_state(void)\n>> +{\n>> + int result = 0;\n>> +\n>> + /* There may be some refs packed during bisection */\n>> + struct string_list refs_for_removal = STRING_LIST_INIT_NODUP;\n>> + for_each_ref_in(\"refs/bisect/\", mark_for_removal, (void *) &refs_for_removal);\n>> + string_list_append(&refs_for_removal, xstrdup(\"BISECT_HEAD\"));\n>> + result = delete_refs(&refs_for_removal);\n>> + refs_for_removal.strdup_strings = 1;\n>> + string_list_clear(&refs_for_removal, 0);\n>> + remove_path(git_path_bisect_expected_rev());\n>> + remove_path(git_path_bisect_ancestors_ok());\n>> + remove_path(git_path_bisect_log());\n>> + remove_path(git_path_bisect_names());\n>> + remove_path(git_path_bisect_run());\n>> + remove_path(git_path_bisect_terms());\n>> + /* Cleanup head-name if it got left by an old version of git-bisect */\n>> + remove_path(git_path_head_name());\n>> + * Cleanup BISECT_START last to support the --no-checkout option\n>> + * introduced in the commit 4796e823a.\n>> + */\n>> + remove_path(git_path_bisect_start());\n>\n> I can see that refs/files-backend.c misuses it already, but\n> remove_path() helper is about removing a path in the working tree,\n> together with any parent directory that becomes empty due to the\n> removal. You do not expect $GIT_DIR/ to become an empty directory\n> after removing $GIT_DIR/BISECT_LOG nor want to rmdir $GIT_DIR even\n> if it becomes empty. It is a wrong helper function to use here.\n>\n> Also you do not seem to check the error from the function to smudge\n> the \"result\" you are returning from this function.\n\nYes I should combine the results from every removal.\n\n> Isn't unlink_or_warn() more correct helper to use here?\n\nThe shell code uses rm -f which is silent and it removes only if\npresent. So it makes me wonder which would be more appropriate\nunlink_or_warn() or remove_or_warn() or remove_path(). Is\nremove_path() different from its shell equivalent \"rm -f\"?\n\n>> + return result;\n>> +}\n\nRegards,\nPranit Bauva\n"},{"id":"293088","messageId":"xmqqvazg5ysb.fsf@gitster.mtv.corp.google.com","threadId":"42979","inReplyTo":"CAFZEwPNFz60GrX9ZqaFAvCU4HoNg8rmuqa0t5+DXFjaEHTmDeg@mail.gmail.com","subject":"Re: [RFC/PATCH v11 03/13] bisect--helper: `write_terms` shell function in C","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-08-04T15:39:16Z","receivedAt":"2016-08-04T15:39:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Pranit Bauva writes:\n\n>>> + res = fprintf(fp, \"%s\\n%s\\n\", bad, good);\n>>> + res |= fclose(fp);\n>>> + return (res < 0) ? -1 : 0;\n>>> +}\n>>\n>> If fprintf(3) were a function that returns 0 on success and negative\n>> on error (like fclose(3) is), the pattern to cascade the error\n>> return with \"res |= another_call()\" is appropriate, but the made me\n>> hiccup a bit while reading it. It is not wrong per-se and it would\n>> certainly be making it worse if we did something silly like\n>>\n>> res = fprintf(...) < 0 ? -1 : 0;\n>> res |= fclose(fp);\n>>\n>> so I guess what you have is the most succinct way to do this.\n>\n> I agree with your point and your suggested code is better!\n\nPuzzled... Read it again, I was not suggesting it---I was saying\n\"this could be a silly rewrite, which I think is making it worse\".\n"},{"id":"293090","messageId":"xmqqr3a45yi7.fsf@gitster.mtv.corp.google.com","threadId":"42979","inReplyTo":"CAFZEwPNFohRLCS4piB1t0LO-=keucwzRKZ2Jyhhf234tnWnVGg@mail.gmail.com","subject":"Re: [RFC/PATCH v11 04/13] bisect--helper: `bisect_clean_state` shell function in C","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-08-04T15:45:20Z","receivedAt":"2016-08-04T15:46:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Pranit Bauva writes:\n\n>> Also you do not seem to check the error from the function to smudge\n>> the \"result\" you are returning from this function.\n>\n> Yes I should combine the results from every removal.\n>\n>> Isn't unlink_or_warn() more correct helper to use here?\n>\n> The shell code uses rm -f which is silent and it removes only if\n> present.\n\nIsn't that what unlink_or_warn() do? Call unlink() and happily\nreturn if unlink() succeeds or errors with ENOENT (i.e. path didn't\nexist in the first place), but otherwise reports an error (imagine:\nEPERM).\n\n> So it makes me wonder which would be more appropriate\n> unlink_or_warn() or remove_or_warn() or remove_path(). Is\n> remove_path() different from its shell equivalent \"rm -f\"?\n\nRead it again.\n\n>>> + remove_path(git_path_bisect_start());\n>>\n>> I can see that refs/files-backend.c misuses it already, but\n>> remove_path() helper is about removing a path in the working tree,\n>> together with any parent directory that becomes empty due to the\n>> removal. You do not expect $GIT_DIR/ to become an empty directory\n>> after removing $GIT_DIR/BISECT_LOG nor want to rmdir $GIT_DIR even\n>> if it becomes empty. It is a wrong helper function to use here.\n"},{"id":"293095","messageId":"CAFZEwPMdOawVT6W6Ko=xkMLu4PcWkd0s_GiJA-T5uu-oY2m23g@mail.gmail.com","threadId":"42979","inReplyTo":"xmqqr3a45yi7.fsf@gitster.mtv.corp.google.com","subject":"Re: [RFC/PATCH v11 04/13] bisect--helper: `bisect_clean_state` shell function in C","fromName":"Pranit Bauva","fromEmail":"pranit.bauva@gmail.com","sentAt":"2016-08-04T16:07:10Z","receivedAt":"2016-08-04T16:07:18Z","isPatch":true,"sender":{"key":"pranit.bauva@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2959938?v=4"},"body":"Hey Junio,\n\nOn Thu, Aug 4, 2016 at 9:15 PM, Junio C Hamano wrote:\n> Pranit Bauva writes:\n>\n>>> Also you do not seem to check the error from the function to smudge\n>>> the \"result\" you are returning from this function.\n>>\n>> Yes I should combine the results from every removal.\n>>\n>>> Isn't unlink_or_warn() more correct helper to use here?\n>>\n>> The shell code uses rm -f which is silent and it removes only if\n>> present.\n>\n> Isn't that what unlink_or_warn() do? Call unlink() and happily\n> return if unlink() succeeds or errors with ENOENT (i.e. path didn't\n> exist in the first place), but otherwise reports an error (imagine:\n> EPERM).\n\nUmm, I am confused. I tried \"rm -f\" with a non-existing file and it\ndoes not show any warning or error.\n\nRegards,\nPranit Bauva\n"},{"id":"293102","messageId":"xmqq1t245vhj.fsf@gitster.mtv.corp.google.com","threadId":"42979","inReplyTo":"CAFZEwPMdOawVT6W6Ko=xkMLu4PcWkd0s_GiJA-T5uu-oY2m23g@mail.gmail.com","subject":"Re: [RFC/PATCH v11 04/13] bisect--helper: `bisect_clean_state` shell function in C","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-08-04T16:50:32Z","receivedAt":"2016-08-04T16:50:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Pranit Bauva writes:\n\n> Hey Junio,\n>\n> On Thu, Aug 4, 2016 at 9:15 PM, Junio C Hamano wrote:\n>> Pranit Bauva writes:\n>>\n>>>> Also you do not seem to check the error from the function to smudge\n>>>> the \"result\" you are returning from this function.\n>>>\n>>> Yes I should combine the results from every removal.\n>>>\n>>>> Isn't unlink_or_warn() more correct helper to use here?\n>>>\n>>> The shell code uses rm -f which is silent and it removes only if\n>>> present.\n>>\n>> Isn't that what unlink_or_warn() do? Call unlink() and happily\n>> return if unlink() succeeds or errors with ENOENT (i.e. path didn't\n>> exist in the first place), but otherwise reports an error (imagine:\n>> EPERM).\n>\n> Umm, I am confused. I tried \"rm -f\" with a non-existing file and it\n> does not show any warning or error.\n\nYou are, or you were? I hope it is the latter, iow, you are no\nlonger confused and now understand why unlink_or_warn() was\nsuggested.\n\n"},{"id":"293104","messageId":"CAFZEwPMpgJDcEqO0A5xqYc99=kECqAr57SL+3z07CRgt-gbUSg@mail.gmail.com","threadId":"42979","inReplyTo":"xmqq1t245vhj.fsf@gitster.mtv.corp.google.com","subject":"Re: [RFC/PATCH v11 04/13] bisect--helper: `bisect_clean_state` shell function in C","fromName":"Pranit Bauva","fromEmail":"pranit.bauva@gmail.com","sentAt":"2016-08-04T16:57:39Z","receivedAt":"2016-08-04T16:57:52Z","isPatch":true,"sender":{"key":"pranit.bauva@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2959938?v=4"},"body":"Hey Junio,\n\nOn Thu, Aug 4, 2016 at 10:20 PM, Junio C Hamano wrote:\n> Pranit Bauva writes:\n>\n>> Hey Junio,\n>>\n>> On Thu, Aug 4, 2016 at 9:15 PM, Junio C Hamano wrote:\n>>> Pranit Bauva writes:\n>>>\n>>>>> Also you do not seem to check the error from the function to smudge\n>>>>> the \"result\" you are returning from this function.\n>>>>\n>>>> Yes I should combine the results from every removal.\n>>>>\n>>>>> Isn't unlink_or_warn() more correct helper to use here?\n>>>>\n>>>> The shell code uses rm -f which is silent and it removes only if\n>>>> present.\n>>>\n>>> Isn't that what unlink_or_warn() do? Call unlink() and happily\n>>> return if unlink() succeeds or errors with ENOENT (i.e. path didn't\n>>> exist in the first place), but otherwise reports an error (imagine:\n>>> EPERM).\n>>\n>> Umm, I am confused. I tried \"rm -f\" with a non-existing file and it\n>> does not show any warning or error.\n>\n> You are, or you were? I hope it is the latter, iow, you are no\n> longer confused and now understand why unlink_or_warn() was\n> suggested.\n\nI meant to use past tense. Did not re-check before sending it.\n"}]}