{"thread":{"id":"61231","subject":"[RFC] bisect: Introduce skip-when to automatically skip commits","startedAt":"2024-03-30T08:11:42Z","lastAt":"2024-04-12T13:35:19Z","messageCount":20,"participants":["Olliver Schinagl","Junio C Hamano","Phillip Wood","phillip.wood123@gmail.com"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"491871","messageId":"20240330081026.362962-2-oliver@schinagl.nl","threadId":"61231","inReplyTo":null,"subject":"[RFC] bisect: Introduce skip-when to automatically skip commits","fromName":"Olliver Schinagl","fromEmail":"oliver@schinagl.nl","sentAt":"2024-03-30T08:10:27Z","receivedAt":"2024-03-30T08:11:42Z","isPatch":false,"sender":{"key":"oliver@schinagl.nl","avatar":"https://gravatar.com/avatar/abb8ef5f9b23563b7703a4114da6f9bd6e7f6d7e8e0296677547b20ff7740c56?d=mp&s=160"},"body":"Before I go dig myself in deeper, I'd like some feedback and opinions on\nwhether this is the correct direction.\n\nIf I got it right, do say so, as then I can start adding some tests and\nupdate the documentation.\n\nOlliver\n\n---\n\nIn some situations, it is needed to skip certain commits when bisecting,\nbecause the compile doesn't work, or tests are known to fail.\n\nFor this purpose, we introduce the `--skip-when` flag which takes a\nscript as an input and is expected to return exit code 125 if a commit\nis to be skipped, which uses a regular `git bisect skip` and the commit\nthus ends up on the skipped pile.\n\nIn addition we also offer a git-hook, to make this as predictable and\npainless as possible.\n\nThe script can do whatever it wants to to determine if a commit is to be\nskipped; From comparing the hash against a known list, to checking git\nnotes for a keyword or, as the included example, the commit body.\n\nSigned-off-by: Olliver Schinagl <oliver@schinagl.nl>\n---\n bisect.c                                 |  2 +\n builtin/bisect.c                         | 93 +++++++++++++++++++++++-\n templates/hooks--bisect-skip_when.sample | 10 +++\n 3 files changed, 101 insertions(+), 4 deletions(-)\n create mode 100755 templates/hooks--bisect-skip_when.sample\n\ndiff --git a/bisect.c b/bisect.c\nindex 60aae2fe50..185909cca9 100644\n--- a/bisect.c\n+++ b/bisect.c\n@@ -476,6 +476,7 @@ static GIT_PATH_FUNC(git_path_bisect_names, \"BISECT_NAMES\")\n static GIT_PATH_FUNC(git_path_bisect_ancestors_ok, \"BISECT_ANCESTORS_OK\")\n static GIT_PATH_FUNC(git_path_bisect_run, \"BISECT_RUN\")\n static GIT_PATH_FUNC(git_path_bisect_start, \"BISECT_START\")\n+static GIT_PATH_FUNC(git_path_bisect_skip_when, \"BISECT_SKIP_WHEN\")\n static GIT_PATH_FUNC(git_path_bisect_log, \"BISECT_LOG\")\n static GIT_PATH_FUNC(git_path_bisect_terms, \"BISECT_TERMS\")\n static GIT_PATH_FUNC(git_path_bisect_first_parent, \"BISECT_FIRST_PARENT\")\n@@ -1179,6 +1180,7 @@ int bisect_clean_state(void)\n \tunlink_or_warn(git_path_bisect_log());\n \tunlink_or_warn(git_path_bisect_names());\n \tunlink_or_warn(git_path_bisect_run());\n+\tunlink_or_warn(git_path_bisect_skip_when());\n \tunlink_or_warn(git_path_bisect_terms());\n \tunlink_or_warn(git_path_bisect_first_parent());\n \t/*\ndiff --git a/builtin/bisect.c b/builtin/bisect.c\nindex 9891cf2604..6870142b85 100644\n--- a/builtin/bisect.c\n+++ b/builtin/bisect.c\n@@ -4,6 +4,7 @@\n #include \"environment.h\"\n #include \"gettext.h\"\n #include \"hex.h\"\n+#include \"hook.h\"\n #include \"object-name.h\"\n #include \"oid-array.h\"\n #include \"parse-options.h\"\n@@ -14,19 +15,21 @@\n #include \"revision.h\"\n #include \"run-command.h\"\n #include \"strvec.h\"\n+#include \"wrapper.h\"\n \n static GIT_PATH_FUNC(git_path_bisect_terms, \"BISECT_TERMS\")\n static GIT_PATH_FUNC(git_path_bisect_ancestors_ok, \"BISECT_ANCESTORS_OK\")\n static GIT_PATH_FUNC(git_path_bisect_start, \"BISECT_START\")\n+static GIT_PATH_FUNC(git_path_bisect_skip_when, \"BISECT_SKIP_WHEN\")\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_first_parent, \"BISECT_FIRST_PARENT\")\n static GIT_PATH_FUNC(git_path_bisect_run, \"BISECT_RUN\")\n \n #define BUILTIN_GIT_BISECT_START_USAGE \\\n-\tN_(\"git bisect start [--term-(new|bad)=<term> --term-(old|good)=<term>]\" \\\n-\t   \"    [--no-checkout] [--first-parent] [<bad> [<good>...]] [--]\" \\\n-\t   \"    [<pathspec>...]\")\n+\tN_(\"git bisect start [--term-(new|bad)=<term> --term-(old|good)=<term>]\\n\" \\\n+\t   \"                 [--no-checkout] [--first-parent] [--skip-when=<script>]\\n\" \\\n+\t   \"                 [<bad> [<good>...]] [--] [<pathspec>...]\")\n #define BUILTIN_GIT_BISECT_STATE_USAGE \\\n \tN_(\"git bisect (good|bad) [<rev>...]\")\n #define BUILTIN_GIT_BISECT_TERMS_USAGE \\\n@@ -89,6 +92,7 @@ static const char vocab_bad[] = \"bad|new\";\n static const char vocab_good[] = \"good|old\";\n \n static int bisect_autostart(struct bisect_terms *terms);\n+static enum bisect_error bisect_skip(struct bisect_terms *terms, int argc, const char **argv);\n \n /*\n  * Check whether the string `term` belongs to the set of strings\n@@ -680,14 +684,74 @@ static enum bisect_error bisect_next(struct bisect_terms *terms, const char *pre\n \treturn res;\n }\n \n+static int get_skip_when(const char **skip_when)\n+{\n+\tstruct strbuf str = STRBUF_INIT;\n+\tFILE *fp = NULL;\n+\tint res = 0;\n+\n+\tfp = fopen(git_path_bisect_skip_when(), \"r\");\n+\tif (!fp) {\n+\t\tres = -1;\n+\t\tgoto finish;\n+\t}\n+\n+\tstrbuf_getline_lf(&str, fp);\n+\t*skip_when = strbuf_detach(&str, NULL);\n+\n+finish:\n+\tif (fp)\n+\t\tfclose(fp);\n+\tstrbuf_release(&str);\n+\n+\treturn res;\n+}\n+\n static enum bisect_error bisect_auto_next(struct bisect_terms *terms, const char *prefix)\n {\n+\tint no_checkout = ref_exists(\"BISECT_HEAD\");\n+\tenum bisect_error res;\n+\tstruct object_id oid;\n+\n \tif (bisect_next_check(terms, NULL)) {\n \t\tbisect_print_status(terms);\n \t\treturn BISECT_OK;\n \t}\n \n-\treturn bisect_next(terms, prefix);\n+\tres = bisect_next(terms, prefix);\n+\tif (res)\n+\t\treturn res;\n+\n+\tif (!read_ref(no_checkout ? \"BISECT_HEAD\" : \"HEAD\", &oid)) {\n+\t\tstruct run_hooks_opt opt = RUN_HOOKS_OPT_INIT;\n+\t\tchar *rev = oid_to_hex(&oid);\n+\t\tconst char *skip_when = NULL;\n+\t\tint ret = 0;\n+\n+\t\tget_skip_when(&skip_when);\n+\t\tif (skip_when != NULL) {\n+\t\t\tstruct child_process cmd = CHILD_PROCESS_INIT;\n+\n+\t\t\tcmd.use_shell = 1;\n+\t\t\tcmd.no_stdin = 1;\n+\t\t\tstrvec_pushl(&cmd.args, skip_when, rev, NULL);\n+\n+\t\t\tprintf(_(\"running '%s'\\n\"), skip_when);\n+\t\t\tret = run_command(&cmd);\n+\t\t}\n+\n+\t\tstrvec_push(&opt.args, rev);\n+\t\tif ((ret == 125) ||\n+\t\t    (run_hooks_opt(\"bisect-skip_when\", &opt) == 125)) {\n+\t\t\tstruct strvec argv = STRVEC_INIT;\n+\n+\t\t\tprintf(_(\"auto skipping commit [%s]...\\n\"), rev);\n+\t\t\tsq_dequote_to_strvec(\"skip\", &argv);\n+\t\t\tres = bisect_skip(terms, argv.nr, argv.v);\n+\t\t}\n+\t}\n+\n+\treturn res;\n }\n \n static enum bisect_error bisect_start(struct bisect_terms *terms, int argc,\n@@ -703,6 +767,7 @@ static enum bisect_error bisect_start(struct bisect_terms *terms, int argc,\n \tstruct strbuf start_head = STRBUF_INIT;\n \tstruct strbuf bisect_names = STRBUF_INIT;\n \tstruct object_id head_oid;\n+\tchar *skip_when = NULL;\n \tstruct object_id oid;\n \tconst char *head;\n \n@@ -727,6 +792,15 @@ static enum bisect_error bisect_start(struct bisect_terms *terms, int argc,\n \t\t\tno_checkout = 1;\n \t\t} else if (!strcmp(arg, \"--first-parent\")) {\n \t\t\tfirst_parent_only = 1;\n+\t\t} else if (!strcmp(arg, \"--skip-when\")) {\n+\t\t\ti++;\n+\n+\t\t\tif (argc <= i)\n+\t\t\t\treturn error(_(\"'' is not a valid skip-when script\"));\n+\n+\t\t\tskip_when = xstrdup(argv[i]);\n+\t\t} else if (skip_prefix(arg, \"--skip-when=\", &arg)) {\n+\t\t\tskip_when = xstrdup(arg);\n \t\t} else if (!strcmp(arg, \"--term-good\") ||\n \t\t\t !strcmp(arg, \"--term-old\")) {\n \t\t\ti++;\n@@ -867,11 +941,22 @@ static enum bisect_error bisect_start(struct bisect_terms *terms, int argc,\n \t\tgoto finish;\n \t}\n \n+\tif (skip_when) {\n+\t\tif (access(skip_when, X_OK)) {\n+\t\t\tres = error(_(\"%s: no such path in the working tree.\\n\"), skip_when);\n+\t\t\tgoto finish;\n+\t\t}\n+\t\twrite_to_file(git_path_bisect_skip_when(), \"%s\\n\", skip_when);\n+\t}\n+\n \tres = bisect_append_log_quoted(argv);\n \tif (res)\n \t\tres = BISECT_FAILED;\n \n finish:\n+\tif (skip_when)\n+\t\tfree(skip_when);\n+\n \tstring_list_clear(&revs, 0);\n \tstring_list_clear(&states, 0);\n \tstrbuf_release(&start_head);\ndiff --git a/templates/hooks--bisect-skip_when.sample b/templates/hooks--bisect-skip_when.sample\nnew file mode 100755\nindex 0000000000..ff3960841f\n--- /dev/null\n+++ b/templates/hooks--bisect-skip_when.sample\n@@ -0,0 +1,10 @@\n+#!/bin/sh\n+#\n+# usage: ${0} <commit_object_name>\n+# expected to exit with 125 when the commit should be skipped\n+\n+if git cat-file commit \"${1:-HEAD}\" | grep -q \"^GIT_BISECT_SKIP=1$\"; then\n+\texit 125\n+fi\n+\n+exit 0\n-- \n2.44.0\n\n"},{"id":"492279","messageId":"864b0f22-b07b-469b-8fc2-56940fd89a8b@schinagl.nl","threadId":"61231","inReplyTo":"20240330081026.362962-2-oliver@schinagl.nl","subject":"Re: [RFC] bisect: Introduce skip-when to automatically skip commits","fromName":"Olliver Schinagl","fromEmail":"oliver@schinagl.nl","sentAt":"2024-04-05T06:50:38Z","receivedAt":"2024-04-05T06:50:46Z","isPatch":false,"sender":{"key":"oliver@schinagl.nl","avatar":"https://gravatar.com/avatar/abb8ef5f9b23563b7703a4114da6f9bd6e7f6d7e8e0296677547b20ff7740c56?d=mp&s=160"},"body":"Hey all,\n\nI've also got my work on a branch in my repo, if that helps to look at \nthings, https://gitlab.com/olliver/git/-/tree/skip_bisect\n\nAlso included is a script to be used as an example. I opted to use `git \nshow`, which is nice because it works both on commits, but also on notes.\n\nAnyway, any thoughts on the bellow before I send the full series?\n\nOlliver\n\n\nOn 30-03-2024 09:10, Olliver Schinagl wrote:\n> Before I go dig myself in deeper, I'd like some feedback and opinions on\n> whether this is the correct direction.\n>\n> If I got it right, do say so, as then I can start adding some tests and\n> update the documentation.\n>\n> Olliver\n>\n> ---\n>\n> In some situations, it is needed to skip certain commits when bisecting,\n> because the compile doesn't work, or tests are known to fail.\n>\n> For this purpose, we introduce the `--skip-when` flag which takes a\n> script as an input and is expected to return exit code 125 if a commit\n> is to be skipped, which uses a regular `git bisect skip` and the commit\n> thus ends up on the skipped pile.\n>\n> In addition we also offer a git-hook, to make this as predictable and\n> painless as possible.\n>\n> The script can do whatever it wants to to determine if a commit is to be\n> skipped; From comparing the hash against a known list, to checking git\n> notes for a keyword or, as the included example, the commit body.\n>\n> Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>\n> ---\n>   bisect.c                                 |  2 +\n>   builtin/bisect.c                         | 93 +++++++++++++++++++++++-\n>   templates/hooks--bisect-skip_when.sample | 10 +++\n>   3 files changed, 101 insertions(+), 4 deletions(-)\n>   create mode 100755 templates/hooks--bisect-skip_when.sample\n>\n> diff --git a/bisect.c b/bisect.c\n> index 60aae2fe50..185909cca9 100644\n> --- a/bisect.c\n> +++ b/bisect.c\n> @@ -476,6 +476,7 @@ static GIT_PATH_FUNC(git_path_bisect_names, \"BISECT_NAMES\")\n>   static GIT_PATH_FUNC(git_path_bisect_ancestors_ok, \"BISECT_ANCESTORS_OK\")\n>   static GIT_PATH_FUNC(git_path_bisect_run, \"BISECT_RUN\")\n>   static GIT_PATH_FUNC(git_path_bisect_start, \"BISECT_START\")\n> +static GIT_PATH_FUNC(git_path_bisect_skip_when, \"BISECT_SKIP_WHEN\")\n>   static GIT_PATH_FUNC(git_path_bisect_log, \"BISECT_LOG\")\n>   static GIT_PATH_FUNC(git_path_bisect_terms, \"BISECT_TERMS\")\n>   static GIT_PATH_FUNC(git_path_bisect_first_parent, \"BISECT_FIRST_PARENT\")\n> @@ -1179,6 +1180,7 @@ int bisect_clean_state(void)\n>   \tunlink_or_warn(git_path_bisect_log());\n>   \tunlink_or_warn(git_path_bisect_names());\n>   \tunlink_or_warn(git_path_bisect_run());\n> +\tunlink_or_warn(git_path_bisect_skip_when());\n>   \tunlink_or_warn(git_path_bisect_terms());\n>   \tunlink_or_warn(git_path_bisect_first_parent());\n>   \t/*\n> diff --git a/builtin/bisect.c b/builtin/bisect.c\n> index 9891cf2604..6870142b85 100644\n> --- a/builtin/bisect.c\n> +++ b/builtin/bisect.c\n> @@ -4,6 +4,7 @@\n>   #include \"environment.h\"\n>   #include \"gettext.h\"\n>   #include \"hex.h\"\n> +#include \"hook.h\"\n>   #include \"object-name.h\"\n>   #include \"oid-array.h\"\n>   #include \"parse-options.h\"\n> @@ -14,19 +15,21 @@\n>   #include \"revision.h\"\n>   #include \"run-command.h\"\n>   #include \"strvec.h\"\n> +#include \"wrapper.h\"\n>   \n>   static GIT_PATH_FUNC(git_path_bisect_terms, \"BISECT_TERMS\")\n>   static GIT_PATH_FUNC(git_path_bisect_ancestors_ok, \"BISECT_ANCESTORS_OK\")\n>   static GIT_PATH_FUNC(git_path_bisect_start, \"BISECT_START\")\n> +static GIT_PATH_FUNC(git_path_bisect_skip_when, \"BISECT_SKIP_WHEN\")\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_first_parent, \"BISECT_FIRST_PARENT\")\n>   static GIT_PATH_FUNC(git_path_bisect_run, \"BISECT_RUN\")\n>   \n>   #define BUILTIN_GIT_BISECT_START_USAGE \\\n> -\tN_(\"git bisect start [--term-(new|bad)=<term> --term-(old|good)=<term>]\" \\\n> -\t   \"    [--no-checkout] [--first-parent] [<bad> [<good>...]] [--]\" \\\n> -\t   \"    [<pathspec>...]\")\n> +\tN_(\"git bisect start [--term-(new|bad)=<term> --term-(old|good)=<term>]\\n\" \\\n> +\t   \"                 [--no-checkout] [--first-parent] [--skip-when=<script>]\\n\" \\\n> +\t   \"                 [<bad> [<good>...]] [--] [<pathspec>...]\")\n>   #define BUILTIN_GIT_BISECT_STATE_USAGE \\\n>   \tN_(\"git bisect (good|bad) [<rev>...]\")\n>   #define BUILTIN_GIT_BISECT_TERMS_USAGE \\\n> @@ -89,6 +92,7 @@ static const char vocab_bad[] = \"bad|new\";\n>   static const char vocab_good[] = \"good|old\";\n>   \n>   static int bisect_autostart(struct bisect_terms *terms);\n> +static enum bisect_error bisect_skip(struct bisect_terms *terms, int argc, const char **argv);\n>   \n>   /*\n>    * Check whether the string `term` belongs to the set of strings\n> @@ -680,14 +684,74 @@ static enum bisect_error bisect_next(struct bisect_terms *terms, const char *pre\n>   \treturn res;\n>   }\n>   \n> +static int get_skip_when(const char **skip_when)\n> +{\n> +\tstruct strbuf str = STRBUF_INIT;\n> +\tFILE *fp = NULL;\n> +\tint res = 0;\n> +\n> +\tfp = fopen(git_path_bisect_skip_when(), \"r\");\n> +\tif (!fp) {\n> +\t\tres = -1;\n> +\t\tgoto finish;\n> +\t}\n> +\n> +\tstrbuf_getline_lf(&str, fp);\n> +\t*skip_when = strbuf_detach(&str, NULL);\n> +\n> +finish:\n> +\tif (fp)\n> +\t\tfclose(fp);\n> +\tstrbuf_release(&str);\n> +\n> +\treturn res;\n> +}\n> +\n>   static enum bisect_error bisect_auto_next(struct bisect_terms *terms, const char *prefix)\n>   {\n> +\tint no_checkout = ref_exists(\"BISECT_HEAD\");\n> +\tenum bisect_error res;\n> +\tstruct object_id oid;\n> +\n>   \tif (bisect_next_check(terms, NULL)) {\n>   \t\tbisect_print_status(terms);\n>   \t\treturn BISECT_OK;\n>   \t}\n>   \n> -\treturn bisect_next(terms, prefix);\n> +\tres = bisect_next(terms, prefix);\n> +\tif (res)\n> +\t\treturn res;\n> +\n> +\tif (!read_ref(no_checkout ? \"BISECT_HEAD\" : \"HEAD\", &oid)) {\n> +\t\tstruct run_hooks_opt opt = RUN_HOOKS_OPT_INIT;\n> +\t\tchar *rev = oid_to_hex(&oid);\n> +\t\tconst char *skip_when = NULL;\n> +\t\tint ret = 0;\n> +\n> +\t\tget_skip_when(&skip_when);\n> +\t\tif (skip_when != NULL) {\n> +\t\t\tstruct child_process cmd = CHILD_PROCESS_INIT;\n> +\n> +\t\t\tcmd.use_shell = 1;\n> +\t\t\tcmd.no_stdin = 1;\n> +\t\t\tstrvec_pushl(&cmd.args, skip_when, rev, NULL);\n> +\n> +\t\t\tprintf(_(\"running '%s'\\n\"), skip_when);\n> +\t\t\tret = run_command(&cmd);\n> +\t\t}\n> +\n> +\t\tstrvec_push(&opt.args, rev);\n> +\t\tif ((ret == 125) ||\n> +\t\t    (run_hooks_opt(\"bisect-skip_when\", &opt) == 125)) {\n> +\t\t\tstruct strvec argv = STRVEC_INIT;\n> +\n> +\t\t\tprintf(_(\"auto skipping commit [%s]...\\n\"), rev);\n> +\t\t\tsq_dequote_to_strvec(\"skip\", &argv);\n> +\t\t\tres = bisect_skip(terms, argv.nr, argv.v);\n> +\t\t}\n> +\t}\n> +\n> +\treturn res;\n>   }\n>   \n>   static enum bisect_error bisect_start(struct bisect_terms *terms, int argc,\n> @@ -703,6 +767,7 @@ static enum bisect_error bisect_start(struct bisect_terms *terms, int argc,\n>   \tstruct strbuf start_head = STRBUF_INIT;\n>   \tstruct strbuf bisect_names = STRBUF_INIT;\n>   \tstruct object_id head_oid;\n> +\tchar *skip_when = NULL;\n>   \tstruct object_id oid;\n>   \tconst char *head;\n>   \n> @@ -727,6 +792,15 @@ static enum bisect_error bisect_start(struct bisect_terms *terms, int argc,\n>   \t\t\tno_checkout = 1;\n>   \t\t} else if (!strcmp(arg, \"--first-parent\")) {\n>   \t\t\tfirst_parent_only = 1;\n> +\t\t} else if (!strcmp(arg, \"--skip-when\")) {\n> +\t\t\ti++;\n> +\n> +\t\t\tif (argc <= i)\n> +\t\t\t\treturn error(_(\"'' is not a valid skip-when script\"));\n> +\n> +\t\t\tskip_when = xstrdup(argv[i]);\n> +\t\t} else if (skip_prefix(arg, \"--skip-when=\", &arg)) {\n> +\t\t\tskip_when = xstrdup(arg);\n>   \t\t} else if (!strcmp(arg, \"--term-good\") ||\n>   \t\t\t !strcmp(arg, \"--term-old\")) {\n>   \t\t\ti++;\n> @@ -867,11 +941,22 @@ static enum bisect_error bisect_start(struct bisect_terms *terms, int argc,\n>   \t\tgoto finish;\n>   \t}\n>   \n> +\tif (skip_when) {\n> +\t\tif (access(skip_when, X_OK)) {\n> +\t\t\tres = error(_(\"%s: no such path in the working tree.\\n\"), skip_when);\n> +\t\t\tgoto finish;\n> +\t\t}\n> +\t\twrite_to_file(git_path_bisect_skip_when(), \"%s\\n\", skip_when);\n> +\t}\n> +\n>   \tres = bisect_append_log_quoted(argv);\n>   \tif (res)\n>   \t\tres = BISECT_FAILED;\n>   \n>   finish:\n> +\tif (skip_when)\n> +\t\tfree(skip_when);\n> +\n>   \tstring_list_clear(&revs, 0);\n>   \tstring_list_clear(&states, 0);\n>   \tstrbuf_release(&start_head);\n> diff --git a/templates/hooks--bisect-skip_when.sample b/templates/hooks--bisect-skip_when.sample\n> new file mode 100755\n> index 0000000000..ff3960841f\n> --- /dev/null\n> +++ b/templates/hooks--bisect-skip_when.sample\n> @@ -0,0 +1,10 @@\n> +#!/bin/sh\n> +#\n> +# usage: ${0} <commit_object_name>\n> +# expected to exit with 125 when the commit should be skipped\n> +\n> +if git cat-file commit \"${1:-HEAD}\" | grep -q \"^GIT_BISECT_SKIP=1$\"; then\n> +\texit 125\n> +fi\n> +\n> +exit 0\n\n\n"},{"id":"492343","messageId":"xmqqcyr3s3gj.fsf@gitster.g","threadId":"61231","inReplyTo":"864b0f22-b07b-469b-8fc2-56940fd89a8b@schinagl.nl","subject":"Re: [RFC] bisect: Introduce skip-when to automatically skip commits","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-04-06T01:08:12Z","receivedAt":"2024-04-06T01:08:15Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Olliver Schinagl <oliver@schinagl.nl> writes:\n\n> Hey all,\n>\n> I've also got my work on a branch in my repo, if that helps to look at\n> things, https://gitlab.com/olliver/git/-/tree/skip_bisect\n>\n> Also included is a script to be used as an example. I opted to use\n> `git show`, which is nice because it works both on commits, but also\n> on notes.\n>\n> Anyway, any thoughts on the bellow before I send the full series?\n>\n> Olliver\n\nI would not write get_skip_when() before studying the same file to\nsee if there already is a helper to read the whole file used in the\nvicinity (like strbuf_read_file(), perhaps).\n\nI do not have enough concentration to follow changes to\nbisect_auto_next() is reasonable.  Especially I do not know why\n\"bisect-skip_when\" wants to exist and what it is trying to do,\nbesides the fact that its name looks horrible ;-).\n\n"},{"id":"492378","messageId":"b194ba7c-454b-494f-bef2-e9eac7ca87f1@schinagl.nl","threadId":"61231","inReplyTo":"xmqqcyr3s3gj.fsf@gitster.g","subject":"Re: [RFC] bisect: Introduce skip-when to automatically skip commits","fromName":"Olliver Schinagl","fromEmail":"oliver@schinagl.nl","sentAt":"2024-04-06T10:06:25Z","receivedAt":"2024-04-06T10:06:33Z","isPatch":false,"sender":{"key":"oliver@schinagl.nl","avatar":"https://gravatar.com/avatar/abb8ef5f9b23563b7703a4114da6f9bd6e7f6d7e8e0296677547b20ff7740c56?d=mp&s=160"},"body":"On 06-04-2024 03:08, Junio C Hamano wrote:\n> Olliver Schinagl <oliver@schinagl.nl> writes:\n> \n>> Hey all,\n>>\n>> I've also got my work on a branch in my repo, if that helps to look at\n>> things, https://gitlab.com/olliver/git/-/tree/skip_bisect\n>>\n>> Also included is a script to be used as an example. I opted to use\n>> `git show`, which is nice because it works both on commits, but also\n>> on notes.\n>>\n>> Anyway, any thoughts on the bellow before I send the full series?\n>>\n>> Olliver\n> \n> I would not write get_skip_when() before studying the same file to\n> see if there already is a helper to read the whole file used in the\n> vicinity (like strbuf_read_file(), perhaps).\n\nFair enough. I'm a little worried about optimization vs readability. I \nthink it makes it mre clear what the code does in its current form; but \nI'll investigate. Bisecting shouldn't be a computational often happening \nthing, so I'm not to worried about performance. But I'm not too familiar \nwith the git code base, so I don't know either :p\n\n> \n> I do not have enough concentration to follow changes to\n> bisect_auto_next() is reasonable.  Especially I do not know why\n> \"bisect-skip_when\" wants to exist and what it is trying to do,\n> besides the fact that its name looks horrible ;-).\n> \nnaming things, sure. I can look into this absolutly :)\n\nBut in short, bisect_auto_next was returning just after checkout It \nseemed. So after checkout, running the script seemed sensible. But I \nlook at it as a normal git user. So you checkout, test your commit, skip \nto the next one if applicable.\n\n\nI'll think of your two comments, and see if I can address them as you \nregain your concentration :p\n\nBut seeing that these are your main concerns, I'm more confident I'm not \ncompletly on the wrong path here.\n\nOlliver\n"},{"id":"492380","messageId":"30eccdbc-f0c4-4b8f-b735-cf5613912a9f@schinagl.nl","threadId":"61231","inReplyTo":"xmqqcyr3s3gj.fsf@gitster.g","subject":"Re: [RFC] bisect: Introduce skip-when to automatically skip commits","fromName":"Olliver Schinagl","fromEmail":"oliver@schinagl.nl","sentAt":"2024-04-06T10:22:44Z","receivedAt":"2024-04-06T10:22:47Z","isPatch":false,"sender":{"key":"oliver@schinagl.nl","avatar":"https://gravatar.com/avatar/abb8ef5f9b23563b7703a4114da6f9bd6e7f6d7e8e0296677547b20ff7740c56?d=mp&s=160"},"body":"On 06-04-2024 03:08, Junio C Hamano wrote:\n> Olliver Schinagl <oliver@schinagl.nl> writes:\n> \n>> Hey all,\n>>\n>> I've also got my work on a branch in my repo, if that helps to look at\n>> things, https://gitlab.com/olliver/git/-/tree/skip_bisect\n>>\n>> Also included is a script to be used as an example. I opted to use\n>> `git show`, which is nice because it works both on commits, but also\n>> on notes.\n>>\n>> Anyway, any thoughts on the bellow before I send the full series?\n>>\n>> Olliver\n> \n> I would not write get_skip_when() before studying the same file to\n> see if there already is a helper to read the whole file used in the\n> vicinity (like strbuf_read_file(), perhaps).\n\nSo I just remembered, when I started this journey, I wanted to squeeze \nit all into `get_terms()` and make it part of the terms struct, as that \nwas passed around everywhere. I figured, I can rename it into being \nsomething more generic. But I realized that skip_when doesn't actually \nneed to be passed around at all (which we can see in the current \nimplementation). With get_terms() in my mind, I just what that function did.\n\nI saw strbuf_read_file() but I didn't quite understand what it was \ndoing, it was a bit cryptic at first. Now that you mention it however, I \nsee the error of my ways, and that strbuf_read_file() might be good \nenough and do exactly what get_skip_when does.\n\nSo thank you for that hint :)\n\nOlliver\n\n> \n> I do not have enough concentration to follow changes to\n> bisect_auto_next() is reasonable.  Especially I do not know why\n> \"bisect-skip_when\" wants to exist and what it is trying to do,\n> besides the fact that its name looks horrible ;-).\n> \n"},{"id":"492383","messageId":"4bedcad2-218a-4b16-88a7-cc70cc126af3@gmail.com","threadId":"61231","inReplyTo":"b194ba7c-454b-494f-bef2-e9eac7ca87f1@schinagl.nl","subject":"Re: [RFC] bisect: Introduce skip-when to automatically skip commits","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-04-06T13:50:29Z","receivedAt":"2024-04-06T13:50:32Z","isPatch":false,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Olliver\n\nOn 06/04/2024 11:06, Olliver Schinagl wrote:\n> On 06-04-2024 03:08, Junio C Hamano wrote:\n>> Olliver Schinagl <oliver@schinagl.nl> writes:\n>>\n>>> Hey all,\n>>>\n>>> I've also got my work on a branch in my repo, if that helps to look at\n>>> things, https://gitlab.com/olliver/git/-/tree/skip_bisect\n>>>\n>>> Also included is a script to be used as an example. I opted to use\n>>> `git show`, which is nice because it works both on commits, but also\n>>> on notes.\n>>>\n>>> Anyway, any thoughts on the bellow before I send the full series?\n>>>\n>>> Olliver\n>>\n>> I would not write get_skip_when() before studying the same file to\n>> see if there already is a helper to read the whole file used in the\n>> vicinity (like strbuf_read_file(), perhaps).\n> \n> Fair enough. I'm a little worried about optimization vs readability. I \n> think it makes it mre clear what the code does in its current form; but \n> I'll investigate. Bisecting shouldn't be a computational often happening \n> thing, so I'm not to worried about performance. But I'm not too familiar \n> with the git code base, so I don't know either :p\n\nIf you search builtin/bisect.c you'll see some existing callers of \nstrbuf_read_file() that read other files like BISECT_START. Those \ncallers should give you an idea of how to use it.\n\n>>\n>> I do not have enough concentration to follow changes to\n>> bisect_auto_next() is reasonable.  Especially I do not know why\n>> \"bisect-skip_when\" wants to exist and what it is trying to do,\n>> besides the fact that its name looks horrible ;-).\n>>\n> naming things, sure. I can look into this absolutly :)\n\nFor me it's not just the name but the whole hook thing - do we really \nneed that rather than just the command line option?\n\nThe other thing I wondered about is the exit code handling for the \n\"--skip-when\" script. In Junio's example in an earlier message he used a \nsuccessful exit to mean \"skip this commit\" and an unsuccessful exit to \nmean \"test this commit\". To me that matches the name of the option - we \nskip when the script given to \"--skip-when\" is successful. Copying the \nmechanism used by \"git bisect run\" seems a bit cumbersome as we only \nneed to know whether to skip or not, we don't need a special way of \ndistinguishing \"skip this commit\" from \"this commit is good\" and \"this \ncommit is bad\"\n\nBest Wishes\n\nPhillip\n\n> But in short, bisect_auto_next was returning just after checkout It \n> seemed. So after checkout, running the script seemed sensible. But I \n> look at it as a normal git user. So you checkout, test your commit, skip \n> to the next one if applicable.\n> \n> \n> I'll think of your two comments, and see if I can address them as you \n> regain your concentration :p\n> \n> But seeing that these are your main concerns, I'm more confident I'm not \n> completly on the wrong path here.\n> \n> Olliver\n> \n"},{"id":"492398","messageId":"xmqq1q7il8re.fsf@gitster.g","threadId":"61231","inReplyTo":"b194ba7c-454b-494f-bef2-e9eac7ca87f1@schinagl.nl","subject":"Re: [RFC] bisect: Introduce skip-when to automatically skip commits","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-04-06T17:07:49Z","receivedAt":"2024-04-06T17:07:55Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Olliver Schinagl <oliver@schinagl.nl> writes:\n\n> But seeing that these are your main concerns, I'm more confident I'm\n> not completly on the wrong path here.\n\nMind you that they are not \"MAIN\" concerns.  They were the ones that\njumped out at me from your sketch.  After seeing the real thing, I\nmay find completely different issues that I could have spotted in\nthis version as well---it is natural that people notice things they\ndid not initially notice with a richer context.\n"},{"id":"492405","messageId":"6dd4a5a4-9999-4c04-a854-09fc238c91bb@schinagl.nl","threadId":"61231","inReplyTo":"4bedcad2-218a-4b16-88a7-cc70cc126af3@gmail.com","subject":"Re: [RFC] bisect: Introduce skip-when to automatically skip commits","fromName":"Olliver Schinagl","fromEmail":"oliver@schinagl.nl","sentAt":"2024-04-06T19:17:59Z","receivedAt":"2024-04-06T19:18:01Z","isPatch":false,"sender":{"key":"oliver@schinagl.nl","avatar":"https://gravatar.com/avatar/abb8ef5f9b23563b7703a4114da6f9bd6e7f6d7e8e0296677547b20ff7740c56?d=mp&s=160"},"body":"Hey Phillip,\n\nOn 06-04-2024 15:50, Phillip Wood wrote:\n> Hi Olliver\n> \n> On 06/04/2024 11:06, Olliver Schinagl wrote:\n>> On 06-04-2024 03:08, Junio C Hamano wrote:\n>>> Olliver Schinagl <oliver@schinagl.nl> writes:\n>>>\n>>>> Hey all,\n>>>>\n>>>> I've also got my work on a branch in my repo, if that helps to look at\n>>>> things, https://gitlab.com/olliver/git/-/tree/skip_bisect\n>>>>\n>>>> Also included is a script to be used as an example. I opted to use\n>>>> `git show`, which is nice because it works both on commits, but also\n>>>> on notes.\n>>>>\n>>>> Anyway, any thoughts on the bellow before I send the full series?\n>>>>\n>>>> Olliver\n>>>\n>>> I would not write get_skip_when() before studying the same file to\n>>> see if there already is a helper to read the whole file used in the\n>>> vicinity (like strbuf_read_file(), perhaps).\n>>\n>> Fair enough. I'm a little worried about optimization vs readability. I \n>> think it makes it mre clear what the code does in its current form; \n>> but I'll investigate. Bisecting shouldn't be a computational often \n>> happening thing, so I'm not to worried about performance. But I'm not \n>> too familiar with the git code base, so I don't know either :p\n> \n> If you search builtin/bisect.c you'll see some existing callers of \n> strbuf_read_file() that read other files like BISECT_START. Those \n> callers should give you an idea of how to use it.\n\nYeah, I found after Junio's hint :) What threw me off, as I wrote \nearlier, get_terms(). I wonder now, why is get_terms() implemented as it \nis, and should it not use the same functions? Or is it because terms is \na multi-line file, whereas the others are all single line (I didn't \nlook, though I see addline functions for the strbuf functions. Should \nthis be refactored?\n\n> \n>>>\n>>> I do not have enough concentration to follow changes to\n>>> bisect_auto_next() is reasonable.  Especially I do not know why\n>>> \"bisect-skip_when\" wants to exist and what it is trying to do,\n>>> besides the fact that its name looks horrible ;-).\n>>>\n>> naming things, sure. I can look into this absolutly :)\n> \n> For me it's not just the name but the whole hook thing - do we really \n> need that rather than just the command line option?\n\nSo with the name, I started to think some more about it, and after \nplaying with some names, I settled on 'bisect-post-checkout'. Things \nthen sort of fell more into place. It is still a hook/commandline \noption, but it's a much smaller change (since we don't have any special \ncode to check the exit code anymore) as we can (obviously) run `git \nbisect skip` instead of `exit 125` as well of course. The `exit 125` \nthinking came from `git bisect run` and maybe a suggestion on the ML \nearlier (I don't quite recall).\n\n> \n> The other thing I wondered about is the exit code handling for the \n> \"--skip-when\" script. In Junio's example in an earlier message he used a \n> successful exit to mean \"skip this commit\" and an unsuccessful exit to \n> mean \"test this commit\". To me that matches the name of the option - we \n> skip when the script given to \"--skip-when\" is successful. Copying the \n> mechanism used by \"git bisect run\" seems a bit cumbersome as we only \n> need to know whether to skip or not, we don't need a special way of \n> distinguishing \"skip this commit\" from \"this commit is good\" and \"this \n> commit is bad\"\n\nSo as I explained above, I think just offering a 'post-checkout' step, \nand let the user decide what to do there, makes things even simpler and \nmore flexible.\n\nI've just pushed my latest changes to \nhttps://gitlab.com/olliver/git/-/commit/4361a5deb0c5ee4c113c25b57752af61b74aabf3 \nand will start working on some tests before offering it for review again.\n\nThank you!\n\nOlliver\n> \n> Best Wishes\n> \n> Phillip\n> \n>> But in short, bisect_auto_next was returning just after checkout It \n>> seemed. So after checkout, running the script seemed sensible. But I \n>> look at it as a normal git user. So you checkout, test your commit, \n>> skip to the next one if applicable.\n>>\n>>\n>> I'll think of your two comments, and see if I can address them as you \n>> regain your concentration :p\n>>\n>> But seeing that these are your main concerns, I'm more confident I'm \n>> not completly on the wrong path here.\n>>\n>> Olliver\n>>\n"},{"id":"492406","messageId":"a861341d-4550-4729-8f86-c3378b1c3c6e@schinagl.nl","threadId":"61231","inReplyTo":"xmqq1q7il8re.fsf@gitster.g","subject":"Re: [RFC] bisect: Introduce skip-when to automatically skip commits","fromName":"Olliver Schinagl","fromEmail":"oliver@schinagl.nl","sentAt":"2024-04-06T19:19:39Z","receivedAt":"2024-04-06T19:19:40Z","isPatch":false,"sender":{"key":"oliver@schinagl.nl","avatar":"https://gravatar.com/avatar/abb8ef5f9b23563b7703a4114da6f9bd6e7f6d7e8e0296677547b20ff7740c56?d=mp&s=160"},"body":"Hey Junio,\n\nOn 06-04-2024 19:07, Junio C Hamano wrote:\n> Olliver Schinagl <oliver@schinagl.nl> writes:\n> \n>> But seeing that these are your main concerns, I'm more confident I'm\n>> not completly on the wrong path here.\n> \n> Mind you that they are not \"MAIN\" concerns.  They were the ones that\n> jumped out at me from your sketch.  After seeing the real thing, I\n> may find completely different issues that I could have spotted in\n> this version as well---it is natural that people notice things they\n> did not initially notice with a richer context.\n\nI completely understand and agree. Just as I changed my design 3 times \nnow already after learning new/more things.\n\nThank you for your time, patience and feedback however, it is much \nappreciated!\n\nOlliver\n"},{"id":"492463","messageId":"d10bd772-2cf1-4838-bec2-ea2a639cabab@gmail.com","threadId":"61231","inReplyTo":"6dd4a5a4-9999-4c04-a854-09fc238c91bb@schinagl.nl","subject":"Re: [RFC] bisect: Introduce skip-when to automatically skip commits","fromName":"","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-04-07T14:09:56Z","receivedAt":"2024-04-07T14:09:59Z","isPatch":false,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 06/04/2024 20:17, Olliver Schinagl wrote:\n> Hey Phillip,\n> \n> On 06-04-2024 15:50, Phillip Wood wrote:\n>> Hi Olliver\n>>\n>> On 06/04/2024 11:06, Olliver Schinagl wrote:\n>>> On 06-04-2024 03:08, Junio C Hamano wrote:\n>>>> Olliver Schinagl <oliver@schinagl.nl> writes:\n>> If you search builtin/bisect.c you'll see some existing callers of \n>> strbuf_read_file() that read other files like BISECT_START. Those \n>> callers should give you an idea of how to use it.\n> \n> Yeah, I found after Junio's hint :) What threw me off, as I wrote \n> earlier, get_terms(). I wonder now, why is get_terms() implemented as it \n> is, and should it not use the same functions? Or is it because terms is \n> a multi-line file, whereas the others are all single line (I didn't \n> look, though I see addline functions for the strbuf functions. Should \n> this be refactored?\n\nget_terms() wants to read the first line into `term_bad` and the second \nline into `term_good` so it makes sense that it uses two calls to \n`strbuf_getline()` to do that. It does not want to read the whole file \ninto a single buffer as we do here.\n\n> So with the name, I started to think some more about it, and after \n> playing with some names, I settled on 'bisect-post-checkout'. Things \n> then sort of fell more into place. It is still a hook/commandline \n> option, but it's a much smaller change (since we don't have any special \n> code to check the exit code anymore) as we can (obviously) run `git \n> bisect skip` instead of `exit 125` as well of course.\n\nDoes that mean you will be starting \"git bisect skip\" from the script \nrun by the current \"git bisect\" process. I don't think calling git \nrecursively like that is a good idea as you'll potentially end up with a \nbunch of \"git bisect\" processes all waiting for their post checkout \nscript to finish running.\n\nBest Wishes\n\nPhillip\n"},{"id":"492465","messageId":"2542ebd6-11ce-496b-b10b-b55c3a211705@schinagl.nl","threadId":"61231","inReplyTo":"d10bd772-2cf1-4838-bec2-ea2a639cabab@gmail.com","subject":"Re: [RFC] bisect: Introduce skip-when to automatically skip commits","fromName":"Olliver Schinagl","fromEmail":"oliver@schinagl.nl","sentAt":"2024-04-07T14:52:37Z","receivedAt":"2024-04-07T14:52:45Z","isPatch":false,"sender":{"key":"oliver@schinagl.nl","avatar":"https://gravatar.com/avatar/abb8ef5f9b23563b7703a4114da6f9bd6e7f6d7e8e0296677547b20ff7740c56?d=mp&s=160"},"body":"Hey Phillip,\n\nOn 07-04-2024 16:09, phillip.wood123@gmail.com wrote:\n> On 06/04/2024 20:17, Olliver Schinagl wrote:\n>> Hey Phillip,\n>>\n>> On 06-04-2024 15:50, Phillip Wood wrote:\n>>> Hi Olliver\n>>>\n>>> On 06/04/2024 11:06, Olliver Schinagl wrote:\n>>>> On 06-04-2024 03:08, Junio C Hamano wrote:\n>>>>> Olliver Schinagl <oliver@schinagl.nl> writes:\n>>> If you search builtin/bisect.c you'll see some existing callers of \n>>> strbuf_read_file() that read other files like BISECT_START. Those \n>>> callers should give you an idea of how to use it.\n>>\n>> Yeah, I found after Junio's hint :) What threw me off, as I wrote \n>> earlier, get_terms(). I wonder now, why is get_terms() implemented as \n>> it is, and should it not use the same functions? Or is it because \n>> terms is a multi-line file, whereas the others are all single line (I \n>> didn't look, though I see addline functions for the strbuf functions. \n>> Should this be refactored?\n> \n> get_terms() wants to read the first line into `term_bad` and the second \n> line into `term_good` so it makes sense that it uses two calls to \n> `strbuf_getline()` to do that. It does not want to read the whole file \n> into a single buffer as we do here.\n\nRight, but I why not use strbuf_getline()?\n\n> \n>> So with the name, I started to think some more about it, and after \n>> playing with some names, I settled on 'bisect-post-checkout'. Things \n>> then sort of fell more into place. It is still a hook/commandline \n>> option, but it's a much smaller change (since we don't have any \n>> special code to check the exit code anymore) as we can (obviously) run \n>> `git bisect skip` instead of `exit 125` as well of course.\n> \n> Does that mean you will be starting \"git bisect skip\" from the script \n> run by the current \"git bisect\" process. I don't think calling git \n> recursively like that is a good idea as you'll potentially end up with a \n> bunch of \"git bisect\" processes all waiting for their post checkout \n> script to finish running.\n\nWell the process is inherently recursive, though that's up to the user \ndepending on what they put in their script of course. I don't think git \nis 'waiting' is it? In that, git bisect runs the command, the command \nruns git bisect, git bisect stores the commit hash in the skip file and \n'exists', which goes then back to the bisect job, which then continues \nas it normally would.\n\nSo technically, we're not doing anything bad in git, but a user might do \nsomething bad.\n\nThank you,\n\nOlliver\n\n> \n> Best Wishes\n> \n> Phillip\n"},{"id":"492466","messageId":"c4ed3e05-ae9f-42dd-835e-a52e710e70fd@gmail.com","threadId":"61231","inReplyTo":"2542ebd6-11ce-496b-b10b-b55c3a211705@schinagl.nl","subject":"Re: [RFC] bisect: Introduce skip-when to automatically skip commits","fromName":"","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-04-07T15:12:28Z","receivedAt":"2024-04-07T15:12:32Z","isPatch":false,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 07/04/2024 15:52, Olliver Schinagl wrote:\n> Hey Phillip,\n> \n> On 07-04-2024 16:09, phillip.wood123@gmail.com wrote:\n>> On 06/04/2024 20:17, Olliver Schinagl wrote:\n>>> Hey Phillip,\n>>>\n>>> On 06-04-2024 15:50, Phillip Wood wrote:\n>>>> Hi Olliver\n>>>>\n>>>> On 06/04/2024 11:06, Olliver Schinagl wrote:\n>>>>> On 06-04-2024 03:08, Junio C Hamano wrote:\n>>>>>> Olliver Schinagl <oliver@schinagl.nl> writes:\n>>>> If you search builtin/bisect.c you'll see some existing callers of \n>>>> strbuf_read_file() that read other files like BISECT_START. Those \n>>>> callers should give you an idea of how to use it.\n>>>\n>>> Yeah, I found after Junio's hint :) What threw me off, as I wrote \n>>> earlier, get_terms(). I wonder now, why is get_terms() implemented as \n>>> it is, and should it not use the same functions? Or is it because \n>>> terms is a multi-line file, whereas the others are all single line (I \n>>> didn't look, though I see addline functions for the strbuf functions. \n>>> Should this be refactored?\n>>\n>> get_terms() wants to read the first line into `term_bad` and the \n>> second line into `term_good` so it makes sense that it uses two calls \n>> to `strbuf_getline()` to do that. It does not want to read the whole \n>> file into a single buffer as we do here.\n> \n> Right, but I why not use strbuf_getline()?\n\nBecause you want the whole file, not just one line as the script name \ncould potentially contain a newline\n\n>>> So with the name, I started to think some more about it, and after \n>>> playing with some names, I settled on 'bisect-post-checkout'. Things \n>>> then sort of fell more into place. It is still a hook/commandline \n>>> option, but it's a much smaller change (since we don't have any \n>>> special code to check the exit code anymore) as we can (obviously) \n>>> run `git bisect skip` instead of `exit 125` as well of course.\n>>\n>> Does that mean you will be starting \"git bisect skip\" from the script \n>> run by the current \"git bisect\" process. I don't think calling git \n>> recursively like that is a good idea as you'll potentially end up with \n>> a bunch of \"git bisect\" processes all waiting for their post checkout \n>> script to finish running.\n> \n> Well the process is inherently recursive, though that's up to the user \n> depending on what they put in their script of course. I don't think git \n> is 'waiting' is it? In that, git bisect runs the command, the command \n> runs git bisect, git bisect stores the commit hash in the skip file and \n> 'exists', which goes then back to the bisect job, which then continues \n> as it normally would.\n> \n> So technically, we're not doing anything bad in git, but a user might do \n> something bad.\n\nIf I understand correctly we're encouraging the user to run \"git bisect \nskip\" from the post checkout script. Doesn't that mean we'll end up with \na set of processes that look like\n\n\t- git bisect start\n\t  - post checkout script\n             - git bisect skip\n               - post checkout script\n                 - git bisect skip\n                   ...\n\nas the \"git bisect start\" is waiting for the post checkout script to \nfinish running, but that script is waiting for \"git bisect skip\" to \nfinish running and so on. Each of those processes takes up system \nresources, similar to how a recursive function can exhaust the available \nstack space by calling itself over and over again.\n\nBest Wishes\n\nPhillip\n"},{"id":"492475","messageId":"97e57635-48cc-4ebd-a50d-6109cba8f7e0@schinagl.nl","threadId":"61231","inReplyTo":"c4ed3e05-ae9f-42dd-835e-a52e710e70fd@gmail.com","subject":"Re: [RFC] bisect: Introduce skip-when to automatically skip commits","fromName":"Olliver Schinagl","fromEmail":"oliver@schinagl.nl","sentAt":"2024-04-07T21:11:22Z","receivedAt":"2024-04-07T21:11:25Z","isPatch":false,"sender":{"key":"oliver@schinagl.nl","avatar":"https://gravatar.com/avatar/abb8ef5f9b23563b7703a4114da6f9bd6e7f6d7e8e0296677547b20ff7740c56?d=mp&s=160"},"body":"On 07-04-2024 17:12, phillip.wood123@gmail.com wrote:\n> On 07/04/2024 15:52, Olliver Schinagl wrote:\n>> Hey Phillip,\n>>\n>> On 07-04-2024 16:09, phillip.wood123@gmail.com wrote:\n>>> On 06/04/2024 20:17, Olliver Schinagl wrote:\n>>>> Hey Phillip,\n>>>>\n>>>> On 06-04-2024 15:50, Phillip Wood wrote:\n>>>>> Hi Olliver\n>>>>>\n>>>>> On 06/04/2024 11:06, Olliver Schinagl wrote:\n>>>>>> On 06-04-2024 03:08, Junio C Hamano wrote:\n>>>>>>> Olliver Schinagl <oliver@schinagl.nl> writes:\n>>>>> If you search builtin/bisect.c you'll see some existing callers of \n>>>>> strbuf_read_file() that read other files like BISECT_START. Those \n>>>>> callers should give you an idea of how to use it.\n>>>>\n>>>> Yeah, I found after Junio's hint :) What threw me off, as I wrote \n>>>> earlier, get_terms(). I wonder now, why is get_terms() implemented \n>>>> as it is, and should it not use the same functions? Or is it because \n>>>> terms is a multi-line file, whereas the others are all single line \n>>>> (I didn't look, though I see addline functions for the strbuf \n>>>> functions. Should this be refactored?\n>>>\n>>> get_terms() wants to read the first line into `term_bad` and the \n>>> second line into `term_good` so it makes sense that it uses two calls \n>>> to `strbuf_getline()` to do that. It does not want to read the whole \n>>> file into a single buffer as we do here.\n>>\n>> Right, but I why not use strbuf_getline()?\n> \n> Because you want the whole file, not just one line as the script name \n> could potentially contain a newline\n\nI suppose; I'd think that there would be a strbuf function call to do \nexactly (more or less) of what was needed. But I'll let it go ;)\n\n> \n>>>> So with the name, I started to think some more about it, and after \n>>>> playing with some names, I settled on 'bisect-post-checkout'. Things \n>>>> then sort of fell more into place. It is still a hook/commandline \n>>>> option, but it's a much smaller change (since we don't have any \n>>>> special code to check the exit code anymore) as we can (obviously) \n>>>> run `git bisect skip` instead of `exit 125` as well of course.\n>>>\n>>> Does that mean you will be starting \"git bisect skip\" from the script \n>>> run by the current \"git bisect\" process. I don't think calling git \n>>> recursively like that is a good idea as you'll potentially end up \n>>> with a bunch of \"git bisect\" processes all waiting for their post \n>>> checkout script to finish running.\n>>\n>> Well the process is inherently recursive, though that's up to the user \n>> depending on what they put in their script of course. I don't think \n>> git is 'waiting' is it? In that, git bisect runs the command, the \n>> command runs git bisect, git bisect stores the commit hash in the skip \n>> file and 'exists', which goes then back to the bisect job, which then \n>> continues as it normally would.\n>>\n>> So technically, we're not doing anything bad in git, but a user might \n>> do something bad.\n> \n> If I understand correctly we're encouraging the user to run \"git bisect \n> skip\" from the post checkout script. Doesn't that mean we'll end up with \n> a set of processes that look like\n> \n>      - git bisect start\n>        - post checkout script\n>              - git bisect skip\n>                - post checkout script\n>                  - git bisect skip\n>                    ...\n> \n> as the \"git bisect start\" is waiting for the post checkout script to \n> finish running, but that script is waiting for \"git bisect skip\" to \n> finish running and so on. Each of those processes takes up system \n> resources, similar to how a recursive function can exhaust the available \n> stack space by calling itself over and over again.\n\nHmm, you might be right. I was thinking that `git bisect skip` would put \nthe hash in the file, and then exit, but of course it also goes to the \nnext checkout and thus triggers the script again (potentially), we don't \nwant that. We do want the hash to end up in the file, but then not \ncontinue, as that would be the job of git bisect.\n\nSo then I go back to my previous solution, which expects exit code 125, \nlike the other case in bisect. That shouldn't cause that behavior, as \nwe'd otherwise have the same problem with the other exit code 125.\n\nThank you,\n\nOlliver\n\n> \n> Best Wishes\n> \n> Phillip\n"},{"id":"492562","messageId":"xmqqzfu3dcl1.fsf@gitster.g","threadId":"61231","inReplyTo":"c4ed3e05-ae9f-42dd-835e-a52e710e70fd@gmail.com","subject":"Re: [RFC] bisect: Introduce skip-when to automatically skip commits","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-04-08T16:49:14Z","receivedAt":"2024-04-08T16:49:17Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"phillip.wood123@gmail.com writes:\n\n>>> get_terms() wants to read the first line into `term_bad` and the\n>>> second line into `term_good` so it makes sense that it uses two\n>>> calls to `strbuf_getline()` to do that. It does not want to read\n>>> the whole file into a single buffer as we do here.\n>> Right, but I why not use strbuf_getline()?\n>\n> Because you want the whole file, not just one line as the script name\n> could potentially contain a newline\n\nIt is technically true, but it somehow sounds like an implausible\nscenario to me.  The real reason why read_file() is preferrable is\nbecause you do not have to write, and we do not want to see you write,\nthe whole \"open (and handle error), read, chomp, and return\" sequence.\n\nI would even suspect that get_terms() is a poorly written\nanti-pattern.  If I were adding that function to the system today, I\nwouldn't be surprised if I did read_file() the whole thing and\nworked in-core to split two items out.\n\n> If I understand correctly we're encouraging the user to run \"git\n> bisect skip\" from the post checkout script. Doesn't that mean we'll\n> end up with a set of processes that look like\n>\n> \t- git bisect start\n> \t  - post checkout script\n>             - git bisect skip\n>               - post checkout script\n>                 - git bisect skip\n>                   ...\n>\n> as the \"git bisect start\" is waiting for the post checkout script to\n> finish running, but that script is waiting for \"git bisect skip\" to\n> finish running and so on. Each of those processes takes up system\n> resources, similar to how a recursive function can exhaust the\n> available stack space by calling itself over and over again.\n\nTrue.  What such a post-checkout script can do is to only mark the\nHEAD as \"untestable\", just like a run script given to \"bisect run\"\nsignals that fact by returnint 125.  And at that point, I doubt it\nmakes sense to add such a post-checkout script for the purpose of\nallowing \"bisect skip\".\n\nHaving said that, a post-checkout script and pre-resume script may\nhave a huge value in helping those whose tests cannot be automated\n(in other words, they cannot do \"git bisect run\") when they need to\ntweak the working tree during bisection.  We all have seen, during a\nbisection session that spans a segment of history that has another\nbug that affects our test *but* is orthogonal to the bug we are\nchasing, that we \"cherry-pick --no-commit\" the fix for that other\nproblem inside \"git bisect run\" script.  It might look something\nlike\n\n    #!/bin/sh\n    if git merge-base --is-ancestor $the_other_bug HEAD\n    then\n\t# we need the fix\n\tgit cherry-pick --no-commit $fix_for_the_other_bug ||\n\texit 125\n    fi\n\n    make test\n    status=$?\n    git reset --hard ;# undo the cherry-pick\n    exit $status\n\nBut to those whose test is not a good match to \"git bisect run\", if\nwe had a mechanism to tweak the checked out working tree after the\n\"bisect next\" (which is an internal mechanism that \"bisect good\",\n\"bisect bad\", and \"bisect skip\" share to give you the next HEAD and\nthe working tree to test) checks out the working tree before it\ngives the control back to you, we could split the above script into\ntwo parts and throw the \"conditionally cherry-pick the fix\" part\ninto that mechanism.  We'd need to have a companion script to \"redo\nthe damage\" (the \"reset --hard\" in the above illustration) if this\nwere to work seamlessly.  That obviously is totally orthogonal to\nwhat we are discussing in this thread, but may make a good #leftoverbits\nmaterial (but not for novices).\n"},{"id":"492712","messageId":"116dd27e-2e30-4915-a131-6c71c999fccd@schinagl.nl","threadId":"61231","inReplyTo":"xmqqzfu3dcl1.fsf@gitster.g","subject":"Re: [RFC] bisect: Introduce skip-when to automatically skip commits","fromName":"Olliver Schinagl","fromEmail":"oliver@schinagl.nl","sentAt":"2024-04-10T10:39:03Z","receivedAt":"2024-04-10T10:39:11Z","isPatch":false,"sender":{"key":"oliver@schinagl.nl","avatar":"https://gravatar.com/avatar/abb8ef5f9b23563b7703a4114da6f9bd6e7f6d7e8e0296677547b20ff7740c56?d=mp&s=160"},"body":"On 08-04-2024 18:49, Junio C Hamano wrote:\n> phillip.wood123@gmail.com writes:\n>\n>>>> get_terms() wants to read the first line into `term_bad` and the\n>>>> second line into `term_good` so it makes sense that it uses two\n>>>> calls to `strbuf_getline()` to do that. It does not want to read\n>>>> the whole file into a single buffer as we do here.\n>>> Right, but I why not use strbuf_getline()?\n>> Because you want the whole file, not just one line as the script name\n>> could potentially contain a newline\n> It is technically true, but it somehow sounds like an implausible\n> scenario to me.  The real reason why read_file() is preferrable is\n> because you do not have to write, and we do not want to see you write,\n> the whole \"open (and handle error), read, chomp, and return\" sequence.\n>\n> I would even suspect that get_terms() is a poorly written\n> anti-pattern.  If I were adding that function to the system today, I\n> wouldn't be surprised if I did read_file() the whole thing and\n> worked in-core to split two items out.\n\nSo I've peaked at it, and think something like:\n\n+int bisect_read_terms(const char **read_bad, const char **read_good)\n+{\n+       struct strbuf sb = STRBUF_INIT;\n+\n+       if (!strbuf_read_file(&sb, git_path_bisect_terms(), 0)) {\n+               *read_bad = \"bad\";\n+               *read_good = \"good\";\n+               return -1;\n+       }\n+\n+       terms = strbuf_split(&sb);\n+       *term_bad = strbuf_detach(terms[0], NULL);\n+       *term_good = strbuf_detach(terms[1], NULL);\n+\n+       strbuf_release(&sb);\n+\n+       return 0;\n+}\n\nwould do the trick. This function could then be called from \nbuiltin/bisect.c as well to have a single interface. Right now, there's \ntwo ways to do the same thing, just because the arguments to the \nfunction are different, and the body is slightly different, but the same.\n\nShall I send a MR with this?\n\n>\n>> If I understand correctly we're encouraging the user to run \"git\n>> bisect skip\" from the post checkout script. Doesn't that mean we'll\n>> end up with a set of processes that look like\n>>\n>> \t- git bisect start\n>> \t  - post checkout script\n>>              - git bisect skip\n>>                - post checkout script\n>>                  - git bisect skip\n>>                    ...\n>>\n>> as the \"git bisect start\" is waiting for the post checkout script to\n>> finish running, but that script is waiting for \"git bisect skip\" to\n>> finish running and so on. Each of those processes takes up system\n>> resources, similar to how a recursive function can exhaust the\n>> available stack space by calling itself over and over again.\n> True.  What such a post-checkout script can do is to only mark the\n> HEAD as \"untestable\", just like a run script given to \"bisect run\"\n> signals that fact by returnint 125.  And at that point, I doubt it\n> makes sense to add such a post-checkout script for the purpose of\n> allowing \"bisect skip\".\n>\n> Having said that, a post-checkout script and pre-resume script may\n> have a huge value in helping those whose tests cannot be automated\n> (in other words, they cannot do \"git bisect run\") when they need to\n> tweak the working tree during bisection.  We all have seen, during a\n> bisection session that spans a segment of history that has another\n> bug that affects our test *but* is orthogonal to the bug we are\n> chasing, that we \"cherry-pick --no-commit\" the fix for that other\n> problem inside \"git bisect run\" script.  It might look something\n> like\n>\n>      #!/bin/sh\n>      if git merge-base --is-ancestor $the_other_bug HEAD\n>      then\n> \t# we need the fix\n> \tgit cherry-pick --no-commit $fix_for_the_other_bug ||\n> \texit 125\n>      fi\n>\n>      make test\n>      status=$?\n>      git reset --hard ;# undo the cherry-pick\n>      exit $status\n>\n> But to those whose test is not a good match to \"git bisect run\", if\n> we had a mechanism to tweak the checked out working tree after the\n> \"bisect next\" (which is an internal mechanism that \"bisect good\",\n> \"bisect bad\", and \"bisect skip\" share to give you the next HEAD and\n> the working tree to test) checks out the working tree before it\n> gives the control back to you, we could split the above script into\n> two parts and throw the \"conditionally cherry-pick the fix\" part\n> into that mechanism.  We'd need to have a companion script to \"redo\n> the damage\" (the \"reset --hard\" in the above illustration) if this\n> were to work seamlessly.  That obviously is totally orthogonal to\n> what we are discussing in this thread, but may make a good #leftoverbits\n> material (but not for novices).\n\nWhile completly orthonogal, I agree; it would be nice to have and \n'abuse' for the bisect-skip usecase. So if we ignore the fact that it \ncan be abused for this (which I don't think is a bad thing, it just \nrisks the recursive issue Phillip mentioned.\n\n\nAs I'm not familiar with the deeps of bisect, I just use it as a dumb \nsimple user, e.g. start; good, bad; I'm not sure the usecase you are \ndescribing is completly clear to me.\n\nAre you saying 'git bisect run` is great, but not useful in all \nsituations, and so in some cases, we want what you just said? Or would \nthis also be part of git bisect run?\n\nI've drafted the post-checkout and pre-resume here: \nhttps://gitlab.com/olliver/git/-/commit/6b5415377600551c0d94a359fd4b8ca7a3678dcf \nwhere I'm not clear on what the best points are for for the pre/post \npoints. I've put the 'pre' bit in the bisect_state function, as that was \nbeing triggered by many suboptions, but might not be correct based on \nyour answer to the above \n(https://gitlab.com/olliver/git/-/commit/6b5415377600551c0d94a359fd4b8ca7a3678dcf#46324e17f99db64a67eb9a5983ffc3a680914ee3_1001_1028). \nThe post-checkout part, I've put in bisect_next \n(https://gitlab.com/olliver/git/-/commit/43993fca32f174f1005c7a445887c0ba5c4036b5#46324e17f99db64a67eb9a5983ffc3a680914ee3_672_717) \nwhich seems to match what you described.\n\n\nOlliver\n\n"},{"id":"492725","messageId":"xmqqzfu1upty.fsf@gitster.g","threadId":"61231","inReplyTo":"116dd27e-2e30-4915-a131-6c71c999fccd@schinagl.nl","subject":"Re: [RFC] bisect: Introduce skip-when to automatically skip commits","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-04-10T16:47:53Z","receivedAt":"2024-04-10T16:47:59Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Olliver Schinagl <oliver@schinagl.nl> writes:\n\n> While completly orthonogal, I agree; it would be nice to have and\n> 'abuse' for the bisect-skip usecase. So if we ignore the fact that it\n> can be abused for this (which I don't think is a bad thing, it just\n> risks the recursive issue Phillip mentioned.\n\nI do not see the \"recursive\" issue here, though.  If we had such a\nmechansim, those whose test cannot be driven by \"bisect run\" can\nstill use the \"--post-checkout\" and \"--pre-resume\" options, where\nthe post-checkout option names a file that has:\n\n\t#!/bin/sh\n        if git merge-base --is-ancestor $the_other_bug HEAD\n        then\n          # we need the fix\n          git cherry-pick --no-commit $fix_for_the_other_bug ||\n          exit 125\n        fi\n\nin it.  There is no \"recursive\"-ness here.  And then after manually\ntesting the checked out stuff (with tweak, thanks to the post-checkout\nscript), they can now say \"git bisect good/bad/skip\" and that is\nwhen their --pre-resume script kicks in, which may do\n\n\t#!/bin/sh\n\tgit reset --hard ;# undo the damage done by post-checkout\n\nbefore the bisect machinery goes and picks the next commit to test.\n\nNotice that I still kept the \"exit 125\" in the above post-checkout\nexample?  That is where the \"bisect next\" that picked the commit to\ntest, checked out that commit and updated the working tree, and run\nyour post-checkout script, can be told that the version checked out\nis untestable and to be skipped.  So such a post-checkout script can\nbe treated as a strict superset of --skip-when script we have been\ndiscussing.\n\nNeedless to say, if we were to do this, we probably should let\n\"bisect run\" also pay attention to these two scripts.  They are most\nlikely to become new parameters specified when \"bisect start\" is run\nto be recorded as one of the many states \"git bisect\" creates.\n"},{"id":"492736","messageId":"bb0dc4a7-e598-45f5-b707-e22de0890f26@schinagl.nl","threadId":"61231","inReplyTo":"xmqqzfu1upty.fsf@gitster.g","subject":"Re: [RFC] bisect: Introduce skip-when to automatically skip commits","fromName":"Olliver Schinagl","fromEmail":"oliver@schinagl.nl","sentAt":"2024-04-10T19:22:31Z","receivedAt":"2024-04-10T19:22:34Z","isPatch":false,"sender":{"key":"oliver@schinagl.nl","avatar":"https://gravatar.com/avatar/abb8ef5f9b23563b7703a4114da6f9bd6e7f6d7e8e0296677547b20ff7740c56?d=mp&s=160"},"body":"On 10-04-2024 18:47, Junio C Hamano wrote:\n> Olliver Schinagl <oliver@schinagl.nl> writes:\n> \n>> While completly orthonogal, I agree; it would be nice to have and\n>> 'abuse' for the bisect-skip usecase. So if we ignore the fact that it\n>> can be abused for this (which I don't think is a bad thing, it just\n>> risks the recursive issue Phillip mentioned.\n> \n> I do not see the \"recursive\" issue here, though.  If we had such a\n> mechansim, those whose test cannot be driven by \"bisect run\" can\n> still use the \"--post-checkout\" and \"--pre-resume\" options, where\n> the post-checkout option names a file that has:\n> \n> \t#!/bin/sh\n>          if git merge-base --is-ancestor $the_other_bug HEAD\n>          then\n>            # we need the fix\n>            git cherry-pick --no-commit $fix_for_the_other_bug ||\n>            exit 125\n>          fi\n> \n> in it.  There is no \"recursive\"-ness here.  And then after manually\n> testing the checked out stuff (with tweak, thanks to the post-checkout\n> script), they can now say \"git bisect good/bad/skip\" and that is\n> when their --pre-resume script kicks in, which may do\n> \n> \t#!/bin/sh\n> \tgit reset --hard ;# undo the damage done by post-checkout\n> \n> before the bisect machinery goes and picks the next commit to test.\n\nYep, that was all perfectly clear to me :) Though I do admit, I \ninitially overlooked the 'not' in your comment on 'those whose test \ncan**not** be driven by \"bisect run\"' bit.\n\n> \n> Notice that I still kept the \"exit 125\" in the above post-checkout\n> example?  That is where the \"bisect next\" that picked the commit to\n> test, checked out that commit and updated the working tree, and run\n> your post-checkout script, can be told that the version checked out\n> is untestable and to be skipped.\n\nThis is where things got stuck for me. I had the 'exit 125' bit for a \nwhile, but couldn't figure out 'how to mark' stuff. Right now, it just \ncalls the script, and if you are a bad user, you can call `git bisect \nskip` and things work as expected, albeit with the aforementioned recursion.\n\nIn the past, as I reported here as well, I tried to capture exit 125, \nthe above would still be true of course, but exit 125 would be a way to \n'catch' this and respond accordingly. The 'accordingly' is where I get \nstuck.\n\nSee, the hook is named 'post-checkout' and thus, it runs after checkout \nhas been performed. So we are now on the 'broken' commit we do not want \nto test, git should have skipped this already, and not checked it out.\n\nSo ok fine, we can call it 'pre-checkout'. But then what. I experimented \nwith marking the skippyness just after `find_bisection()` here: \nhttps://gitlab.com/olliver/git/-/blob/post_search/bisect.c?ref_type=heads#L1090\n\nwith a simple\n\tstrbuf_addf(&skip, \"refs/bisect/skip-%s\", oid_to_hex(oid));\n\toid_array_append(&skipped_revs, &oid);\n\nWhile this kinda worked, it failed when two (or more) commits in order \nwhere to be skipped and 'finished' with 'no possible commits to check'\n\nSo I kinda gave up here and went back to post-checkout.\n\n> So such a post-checkout script can\n> be treated as a strict superset of --skip-when script we have been\n> discussing.\n> \n> Needless to say, if we were to do this, we probably should let\n> \"bisect run\" also pay attention to these two scripts.  They are most\n> likely to become new parameters specified when \"bisect start\" is run\n> to be recorded as one of the many states \"git bisect\" creates.\n\nThe way I've done it now is that it's called from `bisect_next()` here \nhttps://gitlab.com/olliver/git/-/commit/20dd6f5f0e2a55f940bab1e3aced0686d8dfd0c5#46324e17f99db64a67eb9a5983ffc3a680914ee3_672_717 \nbut as I said, checkout has already commenced. Doing it here seems to \nwork with bisect run as well. Resume is done in `bisect_state()` here \nhttps://gitlab.com/olliver/git/-/commit/f9b14a66ea5c4c98f48236db119d3eb60427c1bd#46324e17f99db64a67eb9a5983ffc3a680914ee3_1001_1028 \nwhich also happens in the run case.\n\n\nThe whole exit 125 and avoid recursion thing, is to me more like an \nadditional 'nice-to-have' feature. Recursion wouldn't be a huge thing \nfor modern systems generally anyway in the 'normal/common' case where \nyou recurse 10-ish times. It'll of course get worse if there's multiple \ncommits that would need to be skipped. So even if the recursion is \n20-ish, it's ugly, but not horrible.\n\nIn any case, if the recursion thing is considered bad and must be solved \nif a user does this, as I said before it's not clear to me how to \ntrigger git to say 'oh, I have to go back and do something else; or, \ncall git bisect skip internally, without causing the recursion; or 'put \nthe commit in the queue to be skipped, before it's checked out (which of \ncourse is not a 'post-checkout' of course.\n"},{"id":"492737","messageId":"xmqqle5lrp46.fsf@gitster.g","threadId":"61231","inReplyTo":"bb0dc4a7-e598-45f5-b707-e22de0890f26@schinagl.nl","subject":"Re: [RFC] bisect: Introduce skip-when to automatically skip commits","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-04-10T19:31:37Z","receivedAt":"2024-04-10T19:31:42Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Olliver Schinagl <oliver@schinagl.nl> writes:\n\n> See, the hook is named 'post-checkout' and thus, it runs after\n> checkout has been performed. So we are now on the 'broken' commit we\n> do not want to test, git should have skipped this already, and not\n> checked it out.\n\nYou are not the only user of this feature (by the way, do not call\nthis a \"hook\".  It should be per \"git bisect\" session) and others\nmay need to actually inspect their working tree state before being\nable to say \"nah, I do not want to test this version, please give me\nanother one\" by exiting with 125.  That is why post-checkout is more\nuseful in general.  Contrasted with that, a check that happens\nbefore the checkout is useful only in a much narrower \"I can tell by\nlooking only at the commit object name\" use case, which I would not\nbe interested in seeing.\n\nThanks.\n\n\n"},{"id":"492738","messageId":"0c9ad7f2-55e8-4763-8201-f29f15a39616@schinagl.nl","threadId":"61231","inReplyTo":"xmqqle5lrp46.fsf@gitster.g","subject":"Re: [RFC] bisect: Introduce skip-when to automatically skip commits","fromName":"Olliver Schinagl","fromEmail":"oliver@schinagl.nl","sentAt":"2024-04-10T19:39:52Z","receivedAt":"2024-04-10T19:39:55Z","isPatch":false,"sender":{"key":"oliver@schinagl.nl","avatar":"https://gravatar.com/avatar/abb8ef5f9b23563b7703a4114da6f9bd6e7f6d7e8e0296677547b20ff7740c56?d=mp&s=160"},"body":"On 10-04-2024 21:31, Junio C Hamano wrote:\n> Olliver Schinagl <oliver@schinagl.nl> writes:\n> \n>> See, the hook is named 'post-checkout' and thus, it runs after\n>> checkout has been performed. So we are now on the 'broken' commit we\n>> do not want to test, git should have skipped this already, and not\n>> checked it out.\n> \n> You are not the only user of this feature (by the way, do not call\n> this a \"hook\".  It should be per \"git bisect\" session)\n\nYep, it is stored as part of the bisect session when invoked via as a CLI.\n\n> and others may need to actually inspect their working tree state before being\n> able to say \"nah, I do not want to test this version, please give me\n> another one\" by exiting with 125.\n\nSo your use of 'You' and 'others' is a bit confusing, if it's me, the \nperson, and 'others' the script itself. But yes, I fully agree with what \nyou are saying I think. Just 'please give me a nother one' could be done \nin the script itself, or via exit 125 of course (where I totally get \nthat doing the 125 routine is much better).\n\n\n> That is why post-checkout is more\n> useful in general.  Contrasted with that, a check that happens\n> before the checkout is useful only in a much narrower \"I can tell by\n> looking only at the commit object name\" use case, which I would not\n> be interested in seeing.\n\nSo that leaves me in the same lack of understanding :p\n\nIf we run something post checkout, `bisect_checkout()` has run, and git \nbisect (the application) is (almost) done. The directory has been \n'checked out' (as `bisect_checkout()` of course does `git checkout` \n(unless `no_checkout` is used) The tree is of course in a bisect session.\n\nSo if a script runs, and does the 'please give me a nother commit' \nthing, how would that work within `builtin/bisect.c` (or in `bisect.c`). \nThis is the part that would puzzle me. The command returns with 125, \nthen what? How can git 'go on and give you a nother one'. afaik the only \nreturn codes from `bisect_next()` etc are BISECT_OK and BISECT_FAIL?\n\nOlliver\n\n> \n> Thanks.\n> \n> \n"},{"id":"492870","messageId":"668a443f-8b44-45a9-ae2d-ef3e429456ef@gmail.com","threadId":"61231","inReplyTo":"xmqqzfu3dcl1.fsf@gitster.g","subject":"Re: [RFC] bisect: Introduce skip-when to automatically skip commits","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-04-12T13:35:16Z","receivedAt":"2024-04-12T13:35:19Z","isPatch":false,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Junio\n\nOn 08/04/2024 17:49, Junio C Hamano wrote:\n> phillip.wood123@gmail.com writes:\n >\n> Having said that, a post-checkout script and pre-resume script may\n> have a huge value in helping those whose tests cannot be automated\n> (in other words, they cannot do \"git bisect run\") when they need to\n> tweak the working tree during bisection.  We all have seen, during a\n> bisection session that spans a segment of history that has another\n> bug that affects our test *but* is orthogonal to the bug we are\n> chasing, that we \"cherry-pick --no-commit\" the fix for that other\n> problem inside \"git bisect run\" script.  It might look something\n> like\n> \n>      #!/bin/sh\n>      if git merge-base --is-ancestor $the_other_bug HEAD\n>      then\n> \t# we need the fix\n> \tgit cherry-pick --no-commit $fix_for_the_other_bug ||\n> \texit 125\n>      fi\n> \n>      make test\n>      status=$?\n>      git reset --hard ;# undo the cherry-pick\n>      exit $status\n\nI like this suggestion. A generalized post-checkout script that prepares \nthe wortree and can indicate that this commit should be skipped could be \nreally useful. When having to test manually being able to automate \nbuilding each checkout would be convenient even if one does not have an \northogonal bug to worry about.\n\nBest Wishes\n\nPhillip\n"}]}