{"thread":{"id":"57870","subject":"[PATCH v3 0/2] bisect: status improvements when bisect is not fully fleshed out","startedAt":"2022-05-11T18:00:35Z","lastAt":"2022-05-11T19:41:31Z","messageCount":4,"participants":["Chris Down","Junio C Hamano"],"isPatch":true,"patchVersion":3,"patchTotal":2},"messages":[{"id":"455170","messageId":"cover.1652291688.git.chris@chrisdown.name","threadId":"57870","inReplyTo":null,"subject":"[PATCH v3 0/2] bisect: status improvements when bisect is not fully fleshed out","fromName":"Chris Down","fromEmail":"chris@chrisdown.name","sentAt":"2022-05-11T18:00:04Z","receivedAt":"2022-05-11T18:00:35Z","isPatch":true,"sender":{"key":"chris@chrisdown.name","avatar":"https://avatars.githubusercontent.com/u/660663?v=4"},"body":"When bisecting, we currently don't output anything before bisection\nstarts in earnest, which can result in some confusion. For example, in\nthe case illustrated in the first commit in this patch series, it's\ntrivial to accidentally misspell a tag or branch and accidentally end up\nin an unintended state with no clear indication about what happened.\n\nThis patch series makes it so that we give information about bisect\nstate even before the bisect is ready to begin. We also store these\nchanges in state to the bisect log.\n\nv2:\n\n- Move to improve bisect output overall, instead of just warning for the\n  specific unintended pathspec case.\n\nv3:\n\n- Fix test indentation\n- Rename bs to state\n- Use `unsigned int' for nr_{good,bad}\n- Pass the bisect state struct as an argument into bisect_print_status\n- Zero-initialise bisect_state directly, don't use memset()\n- Fix multiline comment style in bisect.h\n- Use strbuf in bisect_log_printf\n- Change `git bisect log' use an output file in tests instead of piping\n\nChris Down (2):\n  bisect: output state before we are ready to compute bisection\n  bisect: output bisect setup status in bisect log\n\n bisect.h                    |  9 +++++\n builtin/bisect--helper.c    | 69 ++++++++++++++++++++++++++++++-------\n t/t6030-bisect-porcelain.sh | 28 +++++++++++++++\n 3 files changed, 93 insertions(+), 13 deletions(-)\n\n-- \n2.36.0\n\n"},{"id":"455171","messageId":"2cd916580524d0d303b252687e29a064a3aaa387.1652291688.git.chris@chrisdown.name","threadId":"57870","inReplyTo":"cover.1652291688.git.chris@chrisdown.name","subject":"[PATCH v3 1/2] bisect: output state before we are ready to compute bisection","fromName":"Chris Down","fromEmail":"chris@chrisdown.name","sentAt":"2022-05-11T18:00:09Z","receivedAt":"2022-05-11T18:00:41Z","isPatch":true,"sender":{"key":"chris@chrisdown.name","avatar":"https://avatars.githubusercontent.com/u/660663?v=4"},"body":"Commit 73c6de06aff8 (\"bisect: don't use invalid oid as rev when\nstarting\") changes the behaviour of `git bisect` to consider invalid\noids as pathspecs again, as in the old shell implementation.\n\nWhile that behaviour may be desirable, it can also cause confusion. For\nexample, while bisecting in a particular repo I encountered this:\n\n    $ git bisect start d93ff48803f0 v6.3\n    $\n\n...which led to me sitting for a few moments, wondering why there's no\nprintout stating the first rev to check.\n\nIt turns out that the tag was actually \"6.3\", not \"v6.3\", and thus the\nbisect was still silently started with only a bad rev, because\nd93ff48803f0 was a valid oid and \"v6.3\" was silently considered to be a\npathspec.\n\nWhile this behaviour may be desirable, it can be confusing, especially\nwith different repo conventions either using or not using \"v\" before\nrelease names, or when a branch name or tag is simply misspelled on the\ncommand line.\n\nIn order to avoid situations like this, make it more clear what we're\nwaiting for:\n\n    $ git bisect start d93ff48803f0 v6.3\n    status: waiting for good commit(s), bad commit known\n\nWe already have good output once the bisect process has begun in\nearnest, so we don't need to do anything more there.\n\nSigned-off-by: Chris Down <chris@chrisdown.name>\n---\n bisect.h                    |  9 +++++++\n builtin/bisect--helper.c    | 53 ++++++++++++++++++++++++++++---------\n t/t6030-bisect-porcelain.sh | 18 +++++++++++++\n 3 files changed, 67 insertions(+), 13 deletions(-)\n\ndiff --git a/bisect.h b/bisect.h\nindex 1015aeb8ea..ee3fd65f3b 100644\n--- a/bisect.h\n+++ b/bisect.h\n@@ -62,6 +62,15 @@ enum bisect_error {\n \tBISECT_INTERNAL_SUCCESS_MERGE_BASE = -11\n };\n \n+/*\n+ * Stores how many good/bad commits we have stored for a bisect. nr_bad can\n+ * only be 0 or 1.\n+ */\n+struct bisect_state {\n+\tunsigned int nr_good;\n+\tunsigned int nr_bad;\n+};\n+\n enum bisect_error bisect_next_all(struct repository *r, const char *prefix);\n \n int estimate_bisect_steps(int all);\ndiff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\nindex 8b2b259ff0..fa8024a864 100644\n--- a/builtin/bisect--helper.c\n+++ b/builtin/bisect--helper.c\n@@ -329,12 +329,12 @@ static int check_and_set_terms(struct bisect_terms *terms, const char *cmd)\n \treturn 0;\n }\n \n-static int mark_good(const char *refname, const struct object_id *oid,\n-\t\t     int flag, void *cb_data)\n+static int inc_nr(const char *refname, const struct object_id *oid,\n+\t\t  int flag, void *cb_data)\n {\n-\tint *m_good = (int *)cb_data;\n-\t*m_good = 0;\n-\treturn 1;\n+\tunsigned int *nr = (unsigned int *)cb_data;\n+\t(*nr)++;\n+\treturn 0;\n }\n \n static const char need_bad_and_good_revision_warning[] =\n@@ -384,23 +384,48 @@ static int decide_next(const struct bisect_terms *terms,\n \t\t\t     vocab_good, vocab_bad, vocab_good, vocab_bad);\n }\n \n-static int bisect_next_check(const struct bisect_terms *terms,\n-\t\t\t     const char *current_term)\n+static void bisect_status(struct bisect_state *state,\n+\t\t\t  const struct bisect_terms *terms)\n {\n-\tint missing_good = 1, missing_bad = 1;\n \tchar *bad_ref = xstrfmt(\"refs/bisect/%s\", terms->term_bad);\n \tchar *good_glob = xstrfmt(\"%s-*\", terms->term_good);\n \n \tif (ref_exists(bad_ref))\n-\t\tmissing_bad = 0;\n+\t\tstate->nr_bad = 1;\n \n-\tfor_each_glob_ref_in(mark_good, good_glob, \"refs/bisect/\",\n-\t\t\t     (void *) &missing_good);\n+\tfor_each_glob_ref_in(inc_nr, good_glob, \"refs/bisect/\",\n+\t\t\t     (void *) &state->nr_good);\n \n \tfree(good_glob);\n \tfree(bad_ref);\n+}\n+\n+static void bisect_print_status(const struct bisect_terms *terms)\n+{\n+\tstruct bisect_state state = { 0 };\n+\n+\tbisect_status(&state, terms);\n+\n+\t/* If we had both, we'd already be started, and shouldn't get here. */\n+\tif (state.nr_good && state.nr_bad)\n+\t\treturn;\n \n-\treturn decide_next(terms, current_term, missing_good, missing_bad);\n+\tif (!state.nr_good && !state.nr_bad)\n+\t\tprintf(_(\"status: waiting for both good and bad commits\\n\"));\n+\telse if (state.nr_good)\n+\t\tprintf(Q_(\"status: waiting for bad commit, %d good commit known\\n\",\n+\t\t\t  \"status: waiting for bad commit, %d good commits known\\n\",\n+\t\t\t  state.nr_good), state.nr_good);\n+\telse\n+\t\tprintf(_(\"status: waiting for good commit(s), bad commit known\\n\"));\n+}\n+\n+static int bisect_next_check(const struct bisect_terms *terms,\n+\t\t\t     const char *current_term)\n+{\n+\tstruct bisect_state state = { 0 };\n+\tbisect_status(&state, terms);\n+\treturn decide_next(terms, current_term, !state.nr_good, !state.nr_bad);\n }\n \n static int get_terms(struct bisect_terms *terms)\n@@ -606,8 +631,10 @@ static enum bisect_error bisect_next(struct bisect_terms *terms, const char *pre\n \n static enum bisect_error bisect_auto_next(struct bisect_terms *terms, const char *prefix)\n {\n-\tif (bisect_next_check(terms, NULL))\n+\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 }\ndiff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh\nindex 5382e5d216..686f6d5c7f 100755\n--- a/t/t6030-bisect-porcelain.sh\n+++ b/t/t6030-bisect-porcelain.sh\n@@ -1025,4 +1025,22 @@ test_expect_success 'bisect visualize with a filename with dash and space' '\n \tgit bisect visualize -p -- \"-hello 2\"\n '\n \n+test_expect_success 'bisect state output with multiple good commits' '\n+\tgit bisect reset &&\n+\tgit bisect start >output &&\n+\tgrep \"waiting for both good and bad commits\" output &&\n+\tgit bisect good \"$HASH1\" >output &&\n+\tgrep \"waiting for bad commit, 1 good commit known\" output &&\n+\tgit bisect good \"$HASH2\" >output &&\n+\tgrep \"waiting for bad commit, 2 good commits known\" output\n+'\n+\n+test_expect_success 'bisect state output with bad commit' '\n+\tgit bisect reset &&\n+\tgit bisect start >output &&\n+\tgrep \"waiting for both good and bad commits\" output &&\n+\tgit bisect bad \"$HASH4\" >output &&\n+\tgrep -F \"waiting for good commit(s), bad commit known\" output\n+'\n+\n test_done\n-- \n2.36.0\n\n"},{"id":"455172","messageId":"83c76401ad2e066bc25cab542214bc3a021afd02.1652291688.git.chris@chrisdown.name","threadId":"57870","inReplyTo":"cover.1652291688.git.chris@chrisdown.name","subject":"[PATCH v3 2/2] bisect: output bisect setup status in bisect log","fromName":"Chris Down","fromEmail":"chris@chrisdown.name","sentAt":"2022-05-11T18:00:14Z","receivedAt":"2022-05-11T18:00:42Z","isPatch":true,"sender":{"key":"chris@chrisdown.name","avatar":"https://avatars.githubusercontent.com/u/660663?v=4"},"body":"This allows seeing the current intermediate status without adding a new\ngood or bad commit:\n\n    $ git bisect log | tail -1\n    # status: waiting for bad commit, 1 good commit known\n\nSigned-off-by: Chris Down <chris@chrisdown.name>\n---\n builtin/bisect--helper.c    | 26 +++++++++++++++++++++-----\n t/t6030-bisect-porcelain.sh | 10 ++++++++++\n 2 files changed, 31 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\nindex fa8024a864..e358e3dd5b 100644\n--- a/builtin/bisect--helper.c\n+++ b/builtin/bisect--helper.c\n@@ -400,6 +400,22 @@ static void bisect_status(struct bisect_state *state,\n \tfree(bad_ref);\n }\n \n+__attribute__((format (printf, 1, 2)))\n+static void bisect_log_printf(const char *fmt, ...)\n+{\n+\tstruct strbuf buf = STRBUF_INIT;\n+\tva_list ap;\n+\n+\tva_start(ap, fmt);\n+\tstrbuf_vaddf(&buf, fmt, ap);\n+\tva_end(ap);\n+\n+\tprintf(\"%s\", buf.buf);\n+\tappend_to_file(git_path_bisect_log(), \"# %s\", buf.buf);\n+\n+\tstrbuf_release(&buf);\n+}\n+\n static void bisect_print_status(const struct bisect_terms *terms)\n {\n \tstruct bisect_state state = { 0 };\n@@ -411,13 +427,13 @@ static void bisect_print_status(const struct bisect_terms *terms)\n \t\treturn;\n \n \tif (!state.nr_good && !state.nr_bad)\n-\t\tprintf(_(\"status: waiting for both good and bad commits\\n\"));\n+\t\tbisect_log_printf(_(\"status: waiting for both good and bad commits\\n\"));\n \telse if (state.nr_good)\n-\t\tprintf(Q_(\"status: waiting for bad commit, %d good commit known\\n\",\n-\t\t\t  \"status: waiting for bad commit, %d good commits known\\n\",\n-\t\t\t  state.nr_good), state.nr_good);\n+\t\tbisect_log_printf(Q_(\"status: waiting for bad commit, %d good commit known\\n\",\n+\t\t\t\t     \"status: waiting for bad commit, %d good commits known\\n\",\n+\t\t\t\t     state.nr_good), state.nr_good);\n \telse\n-\t\tprintf(_(\"status: waiting for good commit(s), bad commit known\\n\"));\n+\t\tbisect_log_printf(_(\"status: waiting for good commit(s), bad commit known\\n\"));\n }\n \n static int bisect_next_check(const struct bisect_terms *terms,\ndiff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh\nindex 686f6d5c7f..83931d482f 100755\n--- a/t/t6030-bisect-porcelain.sh\n+++ b/t/t6030-bisect-porcelain.sh\n@@ -1029,9 +1029,15 @@ test_expect_success 'bisect state output with multiple good commits' '\n \tgit bisect reset &&\n \tgit bisect start >output &&\n \tgrep \"waiting for both good and bad commits\" output &&\n+\tgit bisect log >output &&\n+\tgrep \"waiting for both good and bad commits\" output &&\n \tgit bisect good \"$HASH1\" >output &&\n \tgrep \"waiting for bad commit, 1 good commit known\" output &&\n+\tgit bisect log >output &&\n+\tgrep \"waiting for bad commit, 1 good commit known\" output &&\n \tgit bisect good \"$HASH2\" >output &&\n+\tgrep \"waiting for bad commit, 2 good commits known\" output &&\n+\tgit bisect log >output &&\n \tgrep \"waiting for bad commit, 2 good commits known\" output\n '\n \n@@ -1039,7 +1045,11 @@ test_expect_success 'bisect state output with bad commit' '\n \tgit bisect reset &&\n \tgit bisect start >output &&\n \tgrep \"waiting for both good and bad commits\" output &&\n+\tgit bisect log >output &&\n+\tgrep \"waiting for both good and bad commits\" output &&\n \tgit bisect bad \"$HASH4\" >output &&\n+\tgrep -F \"waiting for good commit(s), bad commit known\" output &&\n+\tgit bisect log >output &&\n \tgrep -F \"waiting for good commit(s), bad commit known\" output\n '\n \n-- \n2.36.0\n\n"},{"id":"455174","messageId":"xmqqv8ubkgbo.fsf@gitster.g","threadId":"57870","inReplyTo":"cover.1652291688.git.chris@chrisdown.name","subject":"Re: [PATCH v3 0/2] bisect: status improvements when bisect is not fully fleshed out","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-05-11T19:41:15Z","receivedAt":"2022-05-11T19:41:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Chris Down <chris@chrisdown.name> writes:\n\n> When bisecting, we currently don't output anything before bisection\n> starts in earnest, which can result in some confusion. For example, in\n> the case illustrated in the first commit in this patch series, it's\n> trivial to accidentally misspell a tag or branch and accidentally end up\n> in an unintended state with no clear indication about what happened.\n>\n> This patch series makes it so that we give information about bisect\n> state even before the bisect is ready to begin. We also store these\n> changes in state to the bisect log.\n>\n> v2:\n>\n> - Move to improve bisect output overall, instead of just warning for the\n>   specific unintended pathspec case.\n>\n> v3:\n>\n> - Fix test indentation\n> - Rename bs to state\n> - Use `unsigned int' for nr_{good,bad}\n> - Pass the bisect state struct as an argument into bisect_print_status\n> - Zero-initialise bisect_state directly, don't use memset()\n> - Fix multiline comment style in bisect.h\n> - Use strbuf in bisect_log_printf\n> - Change `git bisect log' use an output file in tests instead of piping\n\nWill replace.  Unless there are more inputs, let's merge it to\n'next'.  This round looks more than good enough to me.\n\nThanks.  \n"}]}