{"thread":{"id":"55475","subject":"[PATCH v3 0/4] Finish converting git bisect to C part 4","startedAt":"2021-04-11T09:56:13Z","lastAt":"2021-05-05T09:04:59Z","messageCount":11,"participants":["Miriam Rubio","Junio C Hamano","Andrzej Hunt","Christian Couder"],"isPatch":true,"patchVersion":3,"patchTotal":4},"messages":[{"id":"421577","messageId":"20210411095538.34129-1-mirucam@gmail.com","threadId":"55475","inReplyTo":null,"subject":"[PATCH v3 0/4] Finish converting git bisect to C part 4","fromName":"Miriam Rubio","fromEmail":"mirucam@gmail.com","sentAt":"2021-04-11T09:55:34Z","receivedAt":"2021-04-11T09:56:13Z","isPatch":true,"sender":{"key":"mirucam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56339109?v=4"},"body":"These patches correspond to a fourth part of patch series \nof Outreachy project \"Finish converting `git bisect` from shell to C\" \nstarted by Pranit Bauva and Tanushree Tumane\n(https://public-inbox.org/git/pull.117.git.gitgitgadget@gmail.com) and\ncontinued by me.\n\nThis fourth part is formed by reimplementations of some `git bisect` \nsubcommands and removal of some temporary subcommands.\n\nThese patch series emails were generated from:\nhttps://gitlab.com/mirucam/git/commits/git-bisect-work-part4-v3.\n\nSpecific changes\n----------------\n\n[1/4] run-command: make `exists_in_PATH()` non-static\n* Amend comments.\n---\n\n[2/4] bisect--helper: reimplement `bisect_visualize()`shell function in C\n* Replace strvec_split() to sq_dequote_to_strcvec().\n* Add missing \"--\" argument.\n* Add missing comma in cmdmode enum.\n\n---\n\n[3/4] bisect--helper: reimplement `bisect_run` shell function in C\n* Create BISECT_RUN file.\n* Fix if condition.\n* Remove continue statement in loop.\n---\n\n\nMiriam Rubio (1):\n bisect--helper: retire `--bisect-next-check` subcommand\n\nPranit Bauva (2):\n run-command: make `exists_in_PATH()` non-static\n bisect--helper: reimplement `bisect_visualize()`shell function in C\n\nTanushree Tumane (1):\n bisect--helper: reimplement `bisect_run` shell function in C\n\n builtin/bisect--helper.c | 138 ++++++++++++++++++++++++++++++++++++---\n git-bisect.sh | 87 +-----------------------\n run-command.c | 2 +-\n run-command.h | 12 ++++\n 4 files changed, 145 insertions(+), 94 deletions(-)\n\n-- \n2.29.2\n\n"},{"id":"421578","messageId":"20210411095538.34129-2-mirucam@gmail.com","threadId":"55475","inReplyTo":"20210411095538.34129-1-mirucam@gmail.com","subject":"[PATCH v3 1/4] run-command: make `exists_in_PATH()` non-static","fromName":"Miriam Rubio","fromEmail":"mirucam@gmail.com","sentAt":"2021-04-11T09:55:35Z","receivedAt":"2021-04-11T09:56:15Z","isPatch":true,"sender":{"key":"mirucam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56339109?v=4"},"body":"From: Pranit Bauva \n\nRemoves the `static` keyword from `exists_in_PATH()` function\nand declares the function in `run-command.h` file.\nThe function will be used in bisect_visualize() in a later\ncommit.\n\nMentored by: Christian Couder \nMentored by: Johannes Schindelin \nSigned-off-by: Tanushree Tumane \nSigned-off-by: Miriam Rubio \n---\n run-command.c | 2 +-\n run-command.h | 12 ++++++++++++\n 2 files changed, 13 insertions(+), 1 deletion(-)\n\ndiff --git a/run-command.c b/run-command.c\nindex be6bc128cd..210b8858f7 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -211,7 +211,7 @@ static char *locate_in_PATH(const char *file)\n \treturn NULL;\n }\n \n-static int exists_in_PATH(const char *file)\n+int exists_in_PATH(const char *file)\n {\n \tchar *r = locate_in_PATH(file);\n \tint found = r != NULL;\ndiff --git a/run-command.h b/run-command.h\nindex d08414a92e..cc6f1bad20 100644\n--- a/run-command.h\n+++ b/run-command.h\n@@ -179,6 +179,18 @@ void child_process_clear(struct child_process *);\n \n int is_executable(const char *name);\n \n+/**\n+ * Search if a $PATH for a command exists. This emulates the path search that\n+ * execvp would perform, without actually executing the command so it\n+ * can be used before fork() to prepare to run a command using\n+ * execve() or after execvp() to diagnose why it failed.\n+ *\n+ * The caller should ensure that file contains no directory separators.\n+ *\n+ * Returns 1 if it is found in $PATH or 0 if the command could not be found.\n+ */\n+int exists_in_PATH(const char *file);\n+\n /**\n * Start a sub-process. Takes a pointer to a `struct child_process`\n * that specifies the details and returns pipe FDs (if requested).\n-- \n2.29.2\n\n"},{"id":"421579","messageId":"20210411095538.34129-3-mirucam@gmail.com","threadId":"55475","inReplyTo":"20210411095538.34129-1-mirucam@gmail.com","subject":"[PATCH v3 2/4] bisect--helper: reimplement `bisect_visualize()`shell function in C","fromName":"Miriam Rubio","fromEmail":"mirucam@gmail.com","sentAt":"2021-04-11T09:55:36Z","receivedAt":"2021-04-11T09:56:15Z","isPatch":true,"sender":{"key":"mirucam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56339109?v=4"},"body":"From: Pranit Bauva \n\nReimplement the `bisect_visualize()` shell function\nin C and also add `--bisect-visualize` subcommand to\n`git bisect--helper` to call it from git-bisect.sh.\n\nMentored-by: Christian Couder \nMentored-by: Johannes Schindelin \nSigned-off-by: Tanushree Tumane \nSigned-off-by: Miriam Rubio \n---\n builtin/bisect--helper.c | 48 +++++++++++++++++++++++++++++++++++++++-\n git-bisect.sh | 25 +--------------------\n 2 files changed, 48 insertions(+), 25 deletions(-)\n\ndiff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\nindex 1fdb7d9d10..71963979b1 100644\n--- a/builtin/bisect--helper.c\n+++ b/builtin/bisect--helper.c\n@@ -30,6 +30,7 @@ static const char * const git_bisect_helper_usage[] = {\n \tN_(\"git bisect--helper --bisect-state (good|old) [...]\"),\n \tN_(\"git bisect--helper --bisect-replay \"),\n \tN_(\"git bisect--helper --bisect-skip [(|)...]\"),\n+\tN_(\"git bisect--helper --bisect-visualize\"),\n \tNULL\n };\n \n@@ -1034,6 +1035,44 @@ static enum bisect_error bisect_skip(struct bisect_terms *terms, const char **ar\n \treturn res;\n }\n \n+static int bisect_visualize(struct bisect_terms *terms, const char **argv, int argc)\n+{\n+\tstruct strvec args = STRVEC_INIT;\n+\tint flags = RUN_COMMAND_NO_STDIN, res = 0;\n+\tstruct strbuf sb = STRBUF_INIT;\n+\n+\tif (bisect_next_check(terms, NULL) != 0)\n+\t\treturn BISECT_FAILED;\n+\n+\tif (!argc) {\n+\t\tif ((getenv(\"DISPLAY\") || getenv(\"SESSIONNAME\") || getenv(\"MSYSTEM\") ||\n+\t\t getenv(\"SECURITYSESSIONID\")) && exists_in_PATH(\"gitk\"))\n+\t\t\tstrvec_push(&args, \"gitk\");\n+\t\telse {\n+\t\t\tstrvec_pushl(&args, \"log\", NULL);\n+\t\t\tflags |= RUN_GIT_CMD;\n+\t\t}\n+\t} else {\n+\t\tif (argv[0][0] == '-') {\n+\t\t\tstrvec_pushl(&args, \"log\", NULL);\n+\t\t\tflags |= RUN_GIT_CMD;\n+\t\t} else if (strcmp(argv[0], \"tig\") && !starts_with(argv[0], \"git\"))\n+\t\t\tflags |= RUN_GIT_CMD;\n+\n+\t\tstrvec_pushv(&args, argv);\n+\t}\n+\n+\tstrvec_pushl(&args, \"--bisect\", \"--\", NULL);\n+\n+\tstrbuf_read_file(&sb, git_path_bisect_names(), 0);\n+\tsq_dequote_to_strvec(sb.buf, &args);\n+\tstrbuf_release(&sb);\n+\n+\tres = run_command_v_opt(args.v, flags);\n+\tstrvec_clear(&args);\n+\treturn res;\n+}\n+\n int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n {\n \tenum {\n@@ -1046,7 +1085,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n \t\tBISECT_STATE,\n \t\tBISECT_LOG,\n \t\tBISECT_REPLAY,\n-\t\tBISECT_SKIP\n+\t\tBISECT_SKIP,\n+\t\tBISECT_VISUALIZE,\n \t} cmdmode = 0;\n \tint res = 0, nolog = 0;\n \tstruct option options[] = {\n@@ -1068,6 +1108,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n \t\t\t N_(\"replay the bisection process from the given file\"), BISECT_REPLAY),\n \t\tOPT_CMDMODE(0, \"bisect-skip\", &cmdmode,\n \t\t\t N_(\"skip some commits for checkout\"), BISECT_SKIP),\n+\t\tOPT_CMDMODE(0, \"bisect-visualize\", &cmdmode,\n+\t\t\t N_(\"visualize the bisection\"), BISECT_VISUALIZE),\n \t\tOPT_BOOL(0, \"no-log\", &nolog,\n \t\t\t N_(\"no log for BISECT_WRITE\")),\n \t\tOPT_END()\n@@ -1128,6 +1170,10 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n \t\tset_terms(&terms, \"bad\", \"good\");\n \t\tres = bisect_skip(&terms, argv, argc);\n \t\tbreak;\n+\tcase BISECT_VISUALIZE:\n+\t\tget_terms(&terms);\n+\t\tres = bisect_visualize(&terms, argv, argc);\n+\t\tbreak;\n \tdefault:\n \t\tBUG(\"unknown subcommand %d\", cmdmode);\n \t}\ndiff --git a/git-bisect.sh b/git-bisect.sh\nindex 6a7afaea8d..95f7f3fb8c 100755\n--- a/git-bisect.sh\n+++ b/git-bisect.sh\n@@ -39,29 +39,6 @@ _x40=\"$_x40$_x40$_x40$_x40$_x40$_x40$_x40$_x40\"\n TERM_BAD=bad\n TERM_GOOD=good\n \n-bisect_visualize() {\n-\tgit bisect--helper --bisect-next-check $TERM_GOOD $TERM_BAD fail || exit\n-\n-\tif test $# = 0\n-\tthen\n-\t\tif test -n \"${DISPLAY+set}${SESSIONNAME+set}${MSYSTEM+set}${SECURITYSESSIONID+set}\" &&\n-\t\t\ttype gitk >/dev/null 2>&1\n-\t\tthen\n-\t\t\tset gitk\n-\t\telse\n-\t\t\tset git log\n-\t\tfi\n-\telse\n-\t\tcase \"$1\" in\n-\t\tgit*|tig) ;;\n-\t\t-*)\tset git log \"$@\" ;;\n-\t\t*)\tset git \"$@\" ;;\n-\t\tesac\n-\tfi\n-\n-\teval '\"$@\"' --bisect -- $(cat \"$GIT_DIR/BISECT_NAMES\")\n-}\n-\n bisect_run () {\n \tgit bisect--helper --bisect-next-check $TERM_GOOD $TERM_BAD fail || exit\n \n@@ -152,7 +129,7 @@ case \"$#\" in\n \t\t# Not sure we want \"next\" at the UI level anymore.\n \t\tgit bisect--helper --bisect-next \"$@\" || exit ;;\n \tvisualize|view)\n-\t\tbisect_visualize \"$@\" ;;\n+\t\tgit bisect--helper --bisect-visualize \"$@\" || exit;;\n \treset)\n \t\tgit bisect--helper --bisect-reset \"$@\" ;;\n \treplay)\n-- \n2.29.2\n\n"},{"id":"421580","messageId":"20210411095538.34129-4-mirucam@gmail.com","threadId":"55475","inReplyTo":"20210411095538.34129-1-mirucam@gmail.com","subject":"[PATCH v3 3/4] bisect--helper: reimplement `bisect_run` shell function in C","fromName":"Miriam Rubio","fromEmail":"mirucam@gmail.com","sentAt":"2021-04-11T09:55:37Z","receivedAt":"2021-04-11T09:56:17Z","isPatch":true,"sender":{"key":"mirucam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56339109?v=4"},"body":"From: Tanushree Tumane \n\nReimplement the `bisect_run()` shell function\nin C and also add `--bisect-run` subcommand to\n`git bisect--helper` to call it from git-bisect.sh.\n\nMentored-by: Christian Couder \nSigned-off-by: Tanushree Tumane \nSigned-off-by: Miriam Rubio \n---\n builtin/bisect--helper.c | 83 ++++++++++++++++++++++++++++++++++++++++\n git-bisect.sh | 62 +-----------------------------\n 2 files changed, 84 insertions(+), 61 deletions(-)\n\ndiff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\nindex 71963979b1..dc87fc7dd0 100644\n--- a/builtin/bisect--helper.c\n+++ b/builtin/bisect--helper.c\n@@ -18,6 +18,7 @@ static GIT_PATH_FUNC(git_path_bisect_log, \"BISECT_LOG\")\n static GIT_PATH_FUNC(git_path_head_name, \"head-name\")\n static GIT_PATH_FUNC(git_path_bisect_names, \"BISECT_NAMES\")\n static GIT_PATH_FUNC(git_path_bisect_first_parent, \"BISECT_FIRST_PARENT\")\n+static GIT_PATH_FUNC(git_path_bisect_run, \"BISECT_RUN\")\n \n static const char * const git_bisect_helper_usage[] = {\n \tN_(\"git bisect--helper --bisect-reset []\"),\n@@ -31,6 +32,7 @@ static const char * const git_bisect_helper_usage[] = {\n \tN_(\"git bisect--helper --bisect-replay \"),\n \tN_(\"git bisect--helper --bisect-skip [(|)...]\"),\n \tN_(\"git bisect--helper --bisect-visualize\"),\n+\tN_(\"git bisect--helper --bisect-run ...\"),\n \tNULL\n };\n \n@@ -1073,6 +1075,78 @@ static int bisect_visualize(struct bisect_terms *terms, const char **argv, int a\n \treturn res;\n }\n \n+static int bisect_run(struct bisect_terms *terms, const char **argv, int argc)\n+{\n+\tint res = BISECT_OK;\n+\tstruct strbuf command = STRBUF_INIT;\n+\tstruct strvec args = STRVEC_INIT;\n+\tstruct strvec run_args = STRVEC_INIT;\n+\tint exit = 0;\n+\tint temporary_stdout_fd, saved_stdout;\n+\n+\tif (bisect_next_check(terms, NULL))\n+\t\treturn BISECT_FAILED;\n+\n+\tif (argc)\n+\t\tsq_quote_argv(&command, argv);\n+\telse\n+\t\treturn BISECT_FAILED;\n+\n+\trun_args.v[0] = xstrdup(command.buf);\n+\trun_args.nr = 1;\n+\n+\twhile (1) {\n+\t\tstrvec_clear(&args);\n+\t\texit = 1;\n+\n+\t\tprintf(_(\"running %s\"), command.buf);\n+\t\tres = run_command_v_opt(run_args.v, RUN_USING_SHELL);\n+\n+\t\tif (res < 0 || 128 <= res) {\n+\t\t\terror(_(\"bisect run failed: exit code %d from\"\n+\t\t\t\t\" '%s' is < 0 or >= 128\"), res, command.buf);\n+\t\t\tstrbuf_release(&command);\n+\t\t\treturn res;\n+\t\t}\n+\n+\t\tif (res == 125)\n+\t\t\tstrvec_push(&args, \"skip\");\n+\t\telse if (res > 0)\n+\t\t\tstrvec_push(&args, terms->term_bad);\n+\t\telse\n+\t\t\tstrvec_push(&args, terms->term_good);\n+\n+\t\ttemporary_stdout_fd = open(git_path_bisect_run(), O_CREAT | O_WRONLY | O_TRUNC, 0666);\n+\t\tsaved_stdout = dup(1);\n+\t\tdup2(temporary_stdout_fd, 1);\n+\n+\t\tres = bisect_state(terms, args.v, args.nr);\n+\n+\t\tdup2(saved_stdout, 1);\n+\t\tclose(saved_stdout);\n+\t\tclose(temporary_stdout_fd);\n+\n+\t\tif (res == BISECT_ONLY_SKIPPED_LEFT)\n+\t\t\terror(_(\"bisect run cannot continue any more\"));\n+\t\telse if (res == BISECT_INTERNAL_SUCCESS_MERGE_BASE) {\n+\t\t\tprintf(_(\"bisect run success\"));\n+\t\t\tres = BISECT_OK;\n+\t\t} else if (res) {\n+\t\t\terror(_(\"bisect run failed:'git bisect--helper --bisect-state\"\n+\t\t\t\" %s' exited with error code %d\"), args.v[0], res);\n+\t\t} else {\n+\t\t\texit = 0;\n+\t\t}\n+\n+\t\tif (exit) {\n+\t\t\tstrbuf_release(&command);\n+\t\t\tstrvec_clear(&args);\n+\t\t\tstrvec_clear(&run_args);\n+\t\t\treturn res;\n+\t\t}\n+\t}\n+}\n+\n int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n {\n \tenum {\n@@ -1087,6 +1161,7 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n \t\tBISECT_REPLAY,\n \t\tBISECT_SKIP,\n \t\tBISECT_VISUALIZE,\n+\t\tBISECT_RUN,\n \t} cmdmode = 0;\n \tint res = 0, nolog = 0;\n \tstruct option options[] = {\n@@ -1110,6 +1185,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n \t\t\t N_(\"skip some commits for checkout\"), BISECT_SKIP),\n \t\tOPT_CMDMODE(0, \"bisect-visualize\", &cmdmode,\n \t\t\t N_(\"visualize the bisection\"), BISECT_VISUALIZE),\n+\t\tOPT_CMDMODE(0, \"bisect-run\", &cmdmode,\n+\t\t\t N_(\"use ... to automatically bisect.\"), BISECT_RUN),\n \t\tOPT_BOOL(0, \"no-log\", &nolog,\n \t\t\t N_(\"no log for BISECT_WRITE\")),\n \t\tOPT_END()\n@@ -1174,6 +1251,12 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n \t\tget_terms(&terms);\n \t\tres = bisect_visualize(&terms, argv, argc);\n \t\tbreak;\n+\tcase BISECT_RUN:\n+\t\tif (!argc)\n+\t\t\treturn error(_(\"bisect run failed: no command provided.\"));\n+\t\tget_terms(&terms);\n+\t\tres = bisect_run(&terms, argv, argc);\n+\t\tbreak;\n \tdefault:\n \t\tBUG(\"unknown subcommand %d\", cmdmode);\n \t}\ndiff --git a/git-bisect.sh b/git-bisect.sh\nindex 95f7f3fb8c..e83d011e17 100755\n--- a/git-bisect.sh\n+++ b/git-bisect.sh\n@@ -39,66 +39,6 @@ _x40=\"$_x40$_x40$_x40$_x40$_x40$_x40$_x40$_x40\"\n TERM_BAD=bad\n TERM_GOOD=good\n \n-bisect_run () {\n-\tgit bisect--helper --bisect-next-check $TERM_GOOD $TERM_BAD fail || exit\n-\n-\ttest -n \"$*\" || die \"$(gettext \"bisect run failed: no command provided.\")\"\n-\n-\twhile true\n-\tdo\n-\t\tcommand=\"$@\"\n-\t\teval_gettextln \"running \\$command\"\n-\t\t\"$@\"\n-\t\tres=$?\n-\n-\t\t# Check for really bad run error.\n-\t\tif [ $res -lt 0 -o $res -ge 128 ]\n-\t\tthen\n-\t\t\teval_gettextln \"bisect run failed:\n-exit code \\$res from '\\$command' is < 0 or >= 128\" >&2\n-\t\t\texit $res\n-\t\tfi\n-\n-\t\t# Find current state depending on run success or failure.\n-\t\t# A special exit code of 125 means cannot test.\n-\t\tif [ $res -eq 125 ]\n-\t\tthen\n-\t\t\tstate='skip'\n-\t\telif [ $res -gt 0 ]\n-\t\tthen\n-\t\t\tstate=\"$TERM_BAD\"\n-\t\telse\n-\t\t\tstate=\"$TERM_GOOD\"\n-\t\tfi\n-\n-\t\tgit bisect--helper --bisect-state $state >\"$GIT_DIR/BISECT_RUN\"\n-\t\tres=$?\n-\n-\t\tcat \"$GIT_DIR/BISECT_RUN\"\n-\n-\t\tif sane_grep \"first $TERM_BAD commit could be any of\" \"$GIT_DIR/BISECT_RUN\" \\\n-\t\t\t>/dev/null\n-\t\tthen\n-\t\t\tgettextln \"bisect run cannot continue any more\" >&2\n-\t\t\texit $res\n-\t\tfi\n-\n-\t\tif [ $res -ne 0 ]\n-\t\tthen\n-\t\t\teval_gettextln \"bisect run failed:\n-'bisect-state \\$state' exited with error code \\$res\" >&2\n-\t\t\texit $res\n-\t\tfi\n-\n-\t\tif sane_grep \"is the first $TERM_BAD commit\" \"$GIT_DIR/BISECT_RUN\" >/dev/null\n-\t\tthen\n-\t\t\tgettextln \"bisect run success\"\n-\t\t\texit 0;\n-\t\tfi\n-\n-\tdone\n-}\n-\n get_terms () {\n \tif test -s \"$GIT_DIR/BISECT_TERMS\"\n \tthen\n@@ -137,7 +77,7 @@ case \"$#\" in\n \tlog)\n \t\tgit bisect--helper --bisect-log || exit ;;\n \trun)\n-\t\tbisect_run \"$@\" ;;\n+\t\tgit bisect--helper --bisect-run \"$@\" || exit;;\n \tterms)\n \t\tgit bisect--helper --bisect-terms \"$@\" || exit;;\n \t*)\n-- \n2.29.2\n\n"},{"id":"421581","messageId":"20210411095538.34129-5-mirucam@gmail.com","threadId":"55475","inReplyTo":"20210411095538.34129-1-mirucam@gmail.com","subject":"[PATCH v3 4/4] bisect--helper: retire `--bisect-next-check` subcommand","fromName":"Miriam Rubio","fromEmail":"mirucam@gmail.com","sentAt":"2021-04-11T09:55:38Z","receivedAt":"2021-04-11T09:56:18Z","isPatch":true,"sender":{"key":"mirucam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56339109?v=4"},"body":"After reimplementation of `git bisect run` in C,\n`--bisect-next-check` subcommand is not needed anymore.\n\nLet's remove it from options list and code.\n\nMentored by: Christian Couder \nSigned-off-by: Miriam Rubio \n---\n builtin/bisect--helper.c | 7 -------\n 1 file changed, 7 deletions(-)\n\ndiff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\nindex dc87fc7dd0..0ff34007be 100644\n--- a/builtin/bisect--helper.c\n+++ b/builtin/bisect--helper.c\n@@ -22,7 +22,6 @@ static GIT_PATH_FUNC(git_path_bisect_run, \"BISECT_RUN\")\n \n static const char * const git_bisect_helper_usage[] = {\n \tN_(\"git bisect--helper --bisect-reset []\"),\n-\tN_(\"git bisect--helper --bisect-next-check []\"),\n \tN_(\"git bisect--helper --bisect-terms [--term-good | --term-old | --term-bad | --term-new]\"),\n \tN_(\"git bisect--helper --bisect-start [--term-{new,bad}= --term-{old,good}=]\"\n \t\t\t\t\t \" [--no-checkout] [--first-parent] [ [...]] [--] [...]\"),\n@@ -1206,12 +1205,6 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n \t\t\treturn error(_(\"--bisect-reset requires either no argument or a commit\"));\n \t\tres = bisect_reset(argc ? argv[0] : NULL);\n \t\tbreak;\n-\tcase BISECT_NEXT_CHECK:\n-\t\tif (argc != 2 && argc != 3)\n-\t\t\treturn error(_(\"--bisect-next-check requires 2 or 3 arguments\"));\n-\t\tset_terms(&terms, argv[1], argv[0]);\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\treturn error(_(\"--bisect-terms requires 0 or 1 argument\"));\n-- \n2.29.2\n\n"},{"id":"421618","messageId":"xmqq35vwh8qk.fsf@gitster.g","threadId":"55475","inReplyTo":"20210411095538.34129-3-mirucam@gmail.com","subject":"Re: [PATCH v3 2/4] bisect--helper: reimplement `bisect_visualize()`shell function in C","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-04-11T20:22:27Z","receivedAt":"2021-04-11T20:22:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Miriam Rubio writes:\n\n> +\tstrvec_pushl(&args, \"--bisect\", \"--\", NULL);\n> +\n> +\tstrbuf_read_file(&sb, git_path_bisect_names(), 0);\n> +\tsq_dequote_to_strvec(sb.buf, &args);\n> +\tstrbuf_release(&sb);\n\nEven though it is textually not all that different, this part\nchanged the behaviour (hopefully in a good way) quite a bit from the\nprevious round. It would make a big difference when an element in\nthe pathspec has shell specials.\n\nI suspect that the reason why you didn't notice the breakage before\nsubmitting the previous round (v2) is because of a gap in the test\ncoverage. Can we add a test, or tweak an existing one? I think a\nfile with a space in its name would be sufficient to demonstrate the\nbreakage in v2 and its fix in this round around \"split\" (in v2) vs\n\"dequote\" (in v3). A file whose name begins with a dash would\ndemonstrate the bug that would arise due to lack of \"--\".\n\nThanks.\n"},{"id":"421619","messageId":"xmqqy2doftrj.fsf@gitster.g","threadId":"55475","inReplyTo":"20210411095538.34129-4-mirucam@gmail.com","subject":"Re: [PATCH v3 3/4] bisect--helper: reimplement `bisect_run` shell function in C","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-04-11T20:31:12Z","receivedAt":"2021-04-11T20:31:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Miriam Rubio writes:\n\n> +\t\tif (res < 0 || 128 <= res) {\n> +\t\t\terror(_(\"bisect run failed: exit code %d from\"\n> +\t\t\t\t\" '%s' is < 0 or >= 128\"), res, command.buf);\n\nGood now.\n\n> +\t\ttemporary_stdout_fd = open(git_path_bisect_run(), O_CREAT | O_WRONLY | O_TRUNC, 0666);\n> +\t\tsaved_stdout = dup(1);\n> +\t\tdup2(temporary_stdout_fd, 1);\n> +\n> +\t\tres = bisect_state(terms, args.v, args.nr);\n> +\n> +\t\tdup2(saved_stdout, 1);\n> +\t\tclose(saved_stdout);\n> +\t\tclose(temporary_stdout_fd);\n\nIn v2, we just let bisect_state() to write to our standard output,\nwhich was the reason why the loss of \"cat\" in the \"write to\nBISECT_RUN file and cat it to show to the user\" in the scripted\nversion in v2 was OK. Now, we are writing out, just like the\nscripted version did, to BISECT_RUN, the user does not see its\ncontents.\n\nDoes the distinction matter? Christian, what's your call on this?\n\nIf it does not matter, then the code and tests are good as-is, but\nif it does, the fact that you posted this round without noticing any\nbroken tests means that we have a gap in the test coverage. Can we\nhave a new test about this (i.e. at the end of each round in \"bisect\nrun\", the output from bisect_state() is shown to the user)?\n\n> +\t\tif (res == BISECT_ONLY_SKIPPED_LEFT)\n> +\t\t\terror(_(\"bisect run cannot continue any more\"));\n> +\t\telse if (res == BISECT_INTERNAL_SUCCESS_MERGE_BASE) {\n> +\t\t\tprintf(_(\"bisect run success\"));\n> +\t\t\tres = BISECT_OK;\n> +\t\t} else if (res) {\n> +\t\t\terror(_(\"bisect run failed:'git bisect--helper --bisect-state\"\n> +\t\t\t\" %s' exited with error code %d\"), args.v[0], res);\n> +\t\t} else {\n> +\t\t\texit = 0;\n> +\t\t}\n"},{"id":"421620","messageId":"xmqqtuocfto7.fsf@gitster.g","threadId":"55475","inReplyTo":"xmqqy2doftrj.fsf@gitster.g","subject":"Re: [PATCH v3 3/4] bisect--helper: reimplement `bisect_run` shell function in C","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-04-11T20:33:12Z","receivedAt":"2021-04-11T20:33:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Miriam Rubio writes:\n>\n>> +\t\tif (res < 0 || 128 <= res) {\n>> +\t\t\terror(_(\"bisect run failed: exit code %d from\"\n>> +\t\t\t\t\" '%s' is < 0 or >= 128\"), res, command.buf);\n>\n> Good now.\n\nOh, while asking for better test coverage, it is somewhat surprising\nthat the broken error condition check in v2 was never noticed. Can\nwe add a test for this, too?\n\nThanks.\n"},{"id":"423599","messageId":"3771bfc2-e4a4-3c5d-bcf5-673b403358ca@ahunt.org","threadId":"55475","inReplyTo":"20210411095538.34129-4-mirucam@gmail.com","subject":"Re: [PATCH v3 3/4] bisect--helper: reimplement `bisect_run` shell function in C","fromName":"Andrzej Hunt","fromEmail":"andrzej@ahunt.org","sentAt":"2021-05-04T17:26:01Z","receivedAt":"2021-05-04T17:26:10Z","isPatch":true,"sender":{"key":"andrzej@ahunt.org","avatar":"https://avatars.githubusercontent.com/u/1546915?v=4"},"body":"\n\nOn 11/04/2021 11:55, Miriam Rubio wrote:\n> From: Tanushree Tumane \n> \n> Reimplement the `bisect_run()` shell function\n> in C and also add `--bisect-run` subcommand to\n> `git bisect--helper` to call it from git-bisect.sh.\n> \n> Mentored-by: Christian Couder \n> Signed-off-by: Tanushree Tumane \n> Signed-off-by: Miriam Rubio \n> ---\n> builtin/bisect--helper.c | 83 ++++++++++++++++++++++++++++++++++++++++\n> git-bisect.sh | 62 +-----------------------------\n> 2 files changed, 84 insertions(+), 61 deletions(-)\n> \n> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\n> index 71963979b1..dc87fc7dd0 100644\n> --- a/builtin/bisect--helper.c\n> +++ b/builtin/bisect--helper.c\n> @@ -18,6 +18,7 @@ static GIT_PATH_FUNC(git_path_bisect_log, \"BISECT_LOG\")\n> static GIT_PATH_FUNC(git_path_head_name, \"head-name\")\n> static GIT_PATH_FUNC(git_path_bisect_names, \"BISECT_NAMES\")\n> static GIT_PATH_FUNC(git_path_bisect_first_parent, \"BISECT_FIRST_PARENT\")\n> +static GIT_PATH_FUNC(git_path_bisect_run, \"BISECT_RUN\")\n> \n> static const char * const git_bisect_helper_usage[] = {\n> \tN_(\"git bisect--helper --bisect-reset []\"),\n> @@ -31,6 +32,7 @@ static const char * const git_bisect_helper_usage[] = {\n> \tN_(\"git bisect--helper --bisect-replay \"),\n> \tN_(\"git bisect--helper --bisect-skip [(|)...]\"),\n> \tN_(\"git bisect--helper --bisect-visualize\"),\n> +\tN_(\"git bisect--helper --bisect-run ...\"),\n> \tNULL\n> };\n> \n> @@ -1073,6 +1075,78 @@ static int bisect_visualize(struct bisect_terms *terms, const char **argv, int a\n> \treturn res;\n> }\n> \n> +static int bisect_run(struct bisect_terms *terms, const char **argv, int argc)\n> +{\n> +\tint res = BISECT_OK;\n> +\tstruct strbuf command = STRBUF_INIT;\n> +\tstruct strvec args = STRVEC_INIT;\n> +\tstruct strvec run_args = STRVEC_INIT;\n> +\tint exit = 0;\n> +\tint temporary_stdout_fd, saved_stdout;\n> +\n> +\tif (bisect_next_check(terms, NULL))\n> +\t\treturn BISECT_FAILED;\n> +\n> +\tif (argc)\n> +\t\tsq_quote_argv(&command, argv);\n> +\telse\n> +\t\treturn BISECT_FAILED;\n> +\n> +\trun_args.v[0] = xstrdup(command.buf);\n> +\trun_args.nr = 1;\n\nAFAIUI manipulating the strvec directly like this means that we will \nviolate the promise that strvec.v is always NULL-terminated. It's \nprobably safer to call 'strvec_push(run_args, command.buf)' instead of \nmanipulating v and nr?\n\nViolating the NULL-termination promise a problem because... (continued \nbelow)\n\n> +\n> +\twhile (1) {\n> +\t\tstrvec_clear(&args);\n> +\t\texit = 1;\n> +\n> +\t\tprintf(_(\"running %s\"), command.buf);\n> +\t\tres = run_command_v_opt(run_args.v, RUN_USING_SHELL);\n\nrun_command_v_opt() implicitly expects a NULL-terminated list of \nstrings. It's not documented in run_command_v_opt()'s comments, however \nrun_command_v_opt() does explain that it's a wrapper around \nstart_command(), which uses child_process, and child_process.argv is \ndocumented to require a NULL-terminated list.\n\nIf argv is not NULL-terminated, we hit a buffer overflow read in \nprepare_shell_cmd(), which can be reproduced by running something like:\n\n make CC=clang-11 SANITIZE=address COPTS=\"-Og -g\" GIT_TEST_OPTS=-v \nT=t6030-bisect-porcelain.sh test\n\nwhich results in ASAN reporting this error:\n\n==2116==ERROR: AddressSanitizer: global-buffer-overflow on address \n0x000001a51e28 at pc 0x0000009c6c1f bp 0x7ffcf0f60670 sp 0x7ffcf0f60668\nREAD of size 8 at 0x000001a51e28 thread T0\n #0 0x9c6c1e in prepare_shell_cmd run-command.c:284:8\n #1 0x9c6c1e in prepare_cmd run-command.c:419:3\n #2 0x9c6c1e in start_command run-command.c:753:6\n #3 0x9c7d35 in run_command run-command.c:1015:9\n #4 0x9c800c in run_command_v_opt_cd_env_tr2 run-command.c:1051:9\n #5 0x9c800c in run_command_v_opt_cd_env run-command.c:1033:9\n #6 0x9c800c in run_command_v_opt run-command.c:1023:9\n #7 0x4e5b60 in bisect_run builtin/bisect--helper.c:1102:9\n #8 0x4e5b60 in cmd_bisect__helper builtin/bisect--helper.c:1252:9\n #9 0x4ce8fe in run_builtin git.c:461:11\n #10 0x4ccbc8 in handle_builtin git.c:718:3\n #11 0x4cb0cc in run_argv git.c:785:4\n #12 0x4cb0cc in cmd_main git.c:916:19\n #13 0x6beded in main common-main.c:52:11\n #14 0x7f28636f7349 in __libc_start_main (/lib64/libc.so.6+0x24349)\n #15 0x420769 in _start ../sysdeps/x86_64/start.S:120\n\n0x000001a51e28 is located 56 bytes to the left of global variable \n'config_update_recurse_submodules' defined in 'submodule.c:26:12' \n(0x1a51e60) of size 4\n0x000001a51e28 is located 0 bytes to the right of global variable \n'empty_strvec' defined in 'strvec.c:5:13' (0x1a51e20) of size 8\nSUMMARY: AddressSanitizer: global-buffer-overflow run-command.c:284:8 in \nprepare_shell_cmd\n\n\n[... snip the rest ...]\n"},{"id":"423634","messageId":"xmqqzgxa3pij.fsf@gitster.g","threadId":"55475","inReplyTo":"3771bfc2-e4a4-3c5d-bcf5-673b403358ca@ahunt.org","subject":"Re: [PATCH v3 3/4] bisect--helper: reimplement `bisect_run` shell function in C","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-05-05T02:04:04Z","receivedAt":"2021-05-05T02:04:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Andrzej Hunt writes:\n\n>> +\tstruct strbuf command = STRBUF_INIT;\n>> +\tstruct strvec args = STRVEC_INIT;\n>> +\tstruct strvec run_args = STRVEC_INIT;\n>> + ...\n>> +\trun_args.v[0] = xstrdup(command.buf);\n>> +\trun_args.nr = 1;\n>\n> AFAIUI manipulating the strvec directly like this means that we will\n> violate the promise that strvec.v is always NULL-terminated. It's \n> probably safer to call 'strvec_push(run_args, command.buf)' instead of\n> manipulating v and nr?\n\nTrue.\n\n> Violating the NULL-termination promise a problem because... (continued\n> below)\n>\n>> +\n>> +\twhile (1) {\n>> +\t\tstrvec_clear(&args);\n>> +\t\texit = 1;\n>> +\n>> +\t\tprintf(_(\"running %s\"), command.buf);\n>> +\t\tres = run_command_v_opt(run_args.v, RUN_USING_SHELL);\n>\n> run_command_v_opt() implicitly expects a NULL-terminated list of\n> strings. It's not documented in run_command_v_opt()'s comments,\n> however run_command_v_opt() does explain that it's a wrapper around \n> start_command(), which uses child_process, and child_process.argv is\n> documented to require a NULL-terminated list.\n>\n> If argv is not NULL-terminated, we hit a buffer overflow read in\n> prepare_shell_cmd(), which can be reproduced by running something\n> like:\n>\n> make CC=clang-11 SANITIZE=address COPTS=\"-Og -g\" GIT_TEST_OPTS=-v\n> T=t6030-bisect-porcelain.sh test\n>\n> which results in ASAN reporting this error:\n> ...\n\nThanks for a careful explanation.\n\n"},{"id":"423648","messageId":"CAP8UFD3X24F3qgefHpi00PM-KUk+vcqxwy2Dbngbyj7ciavCVQ@mail.gmail.com","threadId":"55475","inReplyTo":"xmqqy2doftrj.fsf@gitster.g","subject":"Re: [PATCH v3 3/4] bisect--helper: reimplement `bisect_run` shell function in C","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2021-05-05T09:04:45Z","receivedAt":"2021-05-05T09:04:59Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Sun, Apr 11, 2021 at 10:31 PM Junio C Hamano wrote:\n>\n> Miriam Rubio writes:\n\n> > + temporary_stdout_fd = open(git_path_bisect_run(), O_CREAT | O_WRONLY | O_TRUNC, 0666);\n> > + saved_stdout = dup(1);\n> > + dup2(temporary_stdout_fd, 1);\n> > +\n> > + res = bisect_state(terms, args.v, args.nr);\n> > +\n> > + dup2(saved_stdout, 1);\n> > + close(saved_stdout);\n> > + close(temporary_stdout_fd);\n>\n> In v2, we just let bisect_state() to write to our standard output,\n> which was the reason why the loss of \"cat\" in the \"write to\n> BISECT_RUN file and cat it to show to the user\" in the scripted\n> version in v2 was OK. Now, we are writing out, just like the\n> scripted version did, to BISECT_RUN, the user does not see its\n> contents.\n>\n> Does the distinction matter? Christian, what's your call on this?\n\nSorry for the late answer. I think the content of the BISECT_RUN file\nshould indeed be shown.\n\nbisect_state() calls bisect_auto_next() which calls bisect_next()\nwhich calls bisect_next_all(), and bisect_next_all() uses printf() to\nshow things like \"XXX is the first bad commit\" which should be shown\nwhen using `git bisect run`.\n\nAlso when adding an \"exit 1 &&\" before \"git bisect reset\" at the end\nof the `\"git bisect run\" simple case` test, with 'next' I get:\n\n-----------------\n$ ./t6030-bisect-porcelain.sh -i -v\n...\nexpecting success of 6030.21 '\"git bisect run\" simple case':\n write_script test_script.sh <<-\\EOF &&\n ! grep Another hello >/dev/null\n EOF\n git bisect start &&\n git bisect good $HASH1 &&\n git bisect bad $HASH4 &&\n git bisect run ./test_script.sh >my_bisect_log.txt &&\n grep \"$HASH3 is the first bad commit\" my_bisect_log.txt &&\n exit 1 &&\n git bisect reset\n\nBisecting: 0 revisions left to test after this (roughly 1 step)\n[3de952f2416b6084f557ec417709eac740c6818c] Add <3: Another new day for\ngit> into .\n3de952f2416b6084f557ec417709eac740c6818c is the first bad commit\nFATAL: Unexpected exit with code 1\n-----------------\n\nand:\n\n-----------------\n$ cat trash\\ directory.t6030-bisect-porcelain/.git/BISECT_RUN\n3de952f2416b6084f557ec417709eac740c6818c is the first bad commit\ncommit 3de952f2416b6084f557ec417709eac740c6818c\nAuthor: A U Thor \nDate: Thu Apr 7 15:15:13 2005 -0700\n\n Add <3: Another new day for git> into .\n\nhello | 1 +\n1 file changed, 1 insertion(+)\n-----------------\n\nwhile with 'seen' I get:\n\n-----------------\n$ ./t6030-bisect-porcelain.sh -i -v\n...\nexpecting success of 6030.21 '\"git bisect run\" simple case':\n write_script test_script.sh <<-\\EOF &&\n ! grep Another hello >/dev/null\n EOF\n git bisect start &&\n git bisect good $HASH1 &&\n git bisect bad $HASH4 &&\n git bisect run ./test_script.sh >my_bisect_log.txt &&\n grep \"$HASH3 is the first bad commit\" my_bisect_log.txt &&\n exit 1 &&\n git bisect reset\n\nBisecting: 0 revisions left to test after this (roughly 1 step)\n[3de952f2416b6084f557ec417709eac740c6818c] Add <3: Another new day for\ngit> into .\nerror: bisect run failed:'git bisect--helper --bisect-state good'\nexited with error code -10\nrunning './test_script.sh'running\n'./test_script.sh'3de952f2416b6084f557ec417709eac740c6818c is the\nfirst bad commit\nFATAL: Unexpected exit with code 1\n-----------------\n\nand:\n\n-----------------\n$ cat trash\\ directory.t6030-bisect-porcelain/.git/BISECT_RUN\n-----------------\n\nSo it looks like there might be another issue with what's in 'seen'.\n\n> If it does not matter, then the code and tests are good as-is, but\n> if it does, the fact that you posted this round without noticing any\n> broken tests means that we have a gap in the test coverage. Can we\n> have a new test about this (i.e. at the end of each round in \"bisect\n> run\", the output from bisect_state() is shown to the user)?\n\nDefinitely it seems that taking a look at the tests is a good idea.\nFor example, comparing the verbose (with -v) output of t6030 before\nand after each patch (and before and after this patch series) might\nhelp find issues.\n"}]}