{"thread":{"id":"57854","subject":"[PATCH v2 0/2] bisect: status improvements when bisect is not fully fleshed out","startedAt":"2022-05-06T00:52:48Z","lastAt":"2022-05-09T16:27:23Z","messageCount":19,"participants":["Chris Down","Taylor Blau","Junio C Hamano"],"isPatch":true,"patchVersion":2,"patchTotal":2},"messages":[{"id":"454893","messageId":"cover.1651796862.git.chris@chrisdown.name","threadId":"57854","inReplyTo":null,"subject":"[PATCH v2 0/2] bisect: status improvements when bisect is not fully fleshed out","fromName":"Chris Down","fromEmail":"chris@chrisdown.name","sentAt":"2022-05-06T00:52:37Z","receivedAt":"2022-05-06T00:52:48Z","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\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                    |  6 ++++\n builtin/bisect--helper.c    | 69 ++++++++++++++++++++++++++++++-------\n t/t6030-bisect-porcelain.sh | 23 +++++++++++++\n 3 files changed, 85 insertions(+), 13 deletions(-)\n\n\nbase-commit: f5aaf72f1b5aeb3b77bccabce014ea2590e823e2\n-- \n2.36.0\n\n"},{"id":"454894","messageId":"11edd3e4dbaac7fada8a3bcd43f4bbd353087637.1651796862.git.chris@chrisdown.name","threadId":"57854","inReplyTo":"cover.1651796862.git.chris@chrisdown.name","subject":"[PATCH v2 1/2] bisect: output state before we are ready to compute bisection","fromName":"Chris Down","fromEmail":"chris@chrisdown.name","sentAt":"2022-05-06T00:52:46Z","receivedAt":"2022-05-06T00:52:54Z","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                    |  6 +++++\n builtin/bisect--helper.c    | 54 ++++++++++++++++++++++++++++---------\n t/t6030-bisect-porcelain.sh | 18 +++++++++++++\n 3 files changed, 65 insertions(+), 13 deletions(-)\n\ndiff --git a/bisect.h b/bisect.h\nindex 1015aeb8ea..ccd9ad31f6 100644\n--- a/bisect.h\n+++ b/bisect.h\n@@ -62,6 +62,12 @@ enum bisect_error {\n \tBISECT_INTERNAL_SUCCESS_MERGE_BASE = -11\n };\n \n+struct bisect_state {\n+\tint nr_good;  /* How many good commits do we have? */\n+\tint nr_bad;   /* How many bad commits do we have? (Well, you can only\n+\t\t\t  have 0 or 1, but...) */\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..9d583f651c 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+\tint *nr = (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,49 @@ 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 struct bisect_state bisect_status(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+\tstruct bisect_state bs;\n+\n+\tmemset(&bs, 0, sizeof(bs));\n \n \tif (ref_exists(bad_ref))\n-\t\tmissing_bad = 0;\n+\t\tbs.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 *) &bs.nr_good);\n \n \tfree(good_glob);\n \tfree(bad_ref);\n \n-\treturn decide_next(terms, current_term, missing_good, missing_bad);\n+\treturn bs;\n+}\n+\n+static void bisect_print_status(const struct bisect_terms *terms)\n+{\n+\tconst struct bisect_state bs = bisect_status(terms);\n+\n+\t/* If we had both, we'd already be started, and shouldn't get here. */\n+\tif (bs.nr_good && bs.nr_bad)\n+\t\treturn;\n+\n+\tif (!bs.nr_good && !bs.nr_bad)\n+\t\tprintf(_(\"status: waiting for both good and bad commits\\n\"));\n+\telse if (bs.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  bs.nr_good), bs.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+\tconst struct bisect_state bs = bisect_status(terms);\n+\treturn decide_next(terms, current_term, !bs.nr_good, !bs.nr_bad);\n }\n \n static int get_terms(struct bisect_terms *terms)\n@@ -606,8 +632,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..a02587d1a7 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+       git bisect reset &&\n+       git bisect start >output &&\n+       grep \"waiting for both good and bad commits\" output &&\n+       git bisect good \"$HASH1\" >output &&\n+       grep \"waiting for bad commit, 1 good commit known\" output &&\n+       git bisect good \"$HASH2\" >output &&\n+       grep \"waiting for bad commit, 2 good commits known\" output\n+'\n+\n+test_expect_success 'bisect state output with bad commit' '\n+       git bisect reset &&\n+       git bisect start >output &&\n+       grep \"waiting for both good and bad commits\" output &&\n+       git bisect bad \"$HASH4\" >output &&\n+       grep -F \"waiting for good commit(s), bad commit known\" output\n+'\n+\n test_done\n-- \n2.36.0\n\n"},{"id":"454895","messageId":"acab8859d02c95750fdbc691ac672c17d5be0291.1651796862.git.chris@chrisdown.name","threadId":"57854","inReplyTo":"cover.1651796862.git.chris@chrisdown.name","subject":"[PATCH v2 2/2] bisect: output bisect setup status in bisect log","fromName":"Chris Down","fromEmail":"chris@chrisdown.name","sentAt":"2022-05-06T00:52:54Z","receivedAt":"2022-05-06T00:53:01Z","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    | 25 ++++++++++++++++++++-----\n t/t6030-bisect-porcelain.sh |  9 +++++++--\n 2 files changed, 27 insertions(+), 7 deletions(-)\n\ndiff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\nindex 9d583f651c..ef75f0a0ce 100644\n--- a/builtin/bisect--helper.c\n+++ b/builtin/bisect--helper.c\n@@ -404,6 +404,21 @@ static struct bisect_state bisect_status(const struct bisect_terms *terms)\n \treturn bs;\n }\n \n+__attribute__((format (printf, 1, 2)))\n+static void bisect_log_printf(const char *fmt, ...)\n+{\n+\tva_list ap;\n+\tchar buf[1024];\n+\n+\tva_start(ap, fmt);\n+\tif (vsnprintf(buf, sizeof(buf), fmt, ap) < 0)\n+\t\t*buf = '\\0';\n+\tva_end(ap);\n+\n+\tprintf(\"%s\", buf);\n+\tappend_to_file(git_path_bisect_log(), \"# %s\", buf);\n+}\n+\n static void bisect_print_status(const struct bisect_terms *terms)\n {\n \tconst struct bisect_state bs = bisect_status(terms);\n@@ -413,13 +428,13 @@ static void bisect_print_status(const struct bisect_terms *terms)\n \t\treturn;\n \n \tif (!bs.nr_good && !bs.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 (bs.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  bs.nr_good), bs.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     bs.nr_good), bs.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 a02587d1a7..d16730a2e2 100755\n--- a/t/t6030-bisect-porcelain.sh\n+++ b/t/t6030-bisect-porcelain.sh\n@@ -1029,18 +1029,23 @@ test_expect_success 'bisect state output with multiple good commits' '\n        git bisect reset &&\n        git bisect start >output &&\n        grep \"waiting for both good and bad commits\" output &&\n+       git bisect log | grep \"waiting for both good and bad commits\" &&\n        git bisect good \"$HASH1\" >output &&\n        grep \"waiting for bad commit, 1 good commit known\" output &&\n+       git bisect log | grep \"waiting for bad commit, 1 good commit known\" &&\n        git bisect good \"$HASH2\" >output &&\n-       grep \"waiting for bad commit, 2 good commits known\" output\n+       grep \"waiting for bad commit, 2 good commits known\" output &&\n+       git bisect log | grep \"waiting for bad commit, 2 good commits known\"\n '\n \n test_expect_success 'bisect state output with bad commit' '\n        git bisect reset &&\n        git bisect start >output &&\n        grep \"waiting for both good and bad commits\" output &&\n+       git bisect log | grep \"waiting for both good and bad commits\" &&\n        git bisect bad \"$HASH4\" >output &&\n-       grep -F \"waiting for good commit(s), bad commit known\" output\n+       grep -F \"waiting for good commit(s), bad commit known\" output &&\n+       git bisect log | grep -F \"waiting for good commit(s), bad commit known\"\n '\n \n test_done\n-- \n2.36.0\n\n"},{"id":"454896","messageId":"YnSNWMBRZA0S8X26@nand.local","threadId":"57854","inReplyTo":"11edd3e4dbaac7fada8a3bcd43f4bbd353087637.1651796862.git.chris@chrisdown.name","subject":"Re: [PATCH v2 1/2] bisect: output state before we are ready to compute bisection","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-05-06T02:52:08Z","receivedAt":"2022-05-06T02:52:46Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Fri, May 06, 2022 at 01:52:46AM +0100, Chris Down wrote:\n> In order to avoid situations like this, make it more clear what we're\n> waiting for:\n>\n>     $ git bisect start d93ff48803f0 v6.3\n>     status: waiting for good commit(s), bad commit known\n\nMakes sense. It would be kind of nice to realize that (in your example)\n\"v6.3\" likely wasn't a pathspec that matched any files, either, and\nafter trying to DWIM it into something sensible, printed an error and\nquit.\n\nBut I think the behavior here is slightly more subtle, since we really\ncare about whether or not a pathspec would match any revisions along the\nbisection, not just the tips or the currently checked-out revision.\n\nSo I think the approach here makes sense.\n\n> +static void bisect_print_status(const struct bisect_terms *terms)\n> +{\n> +\tconst struct bisect_state bs = bisect_status(terms);\n> +\n> +\t/* If we had both, we'd already be started, and shouldn't get here. */\n> +\tif (bs.nr_good && bs.nr_bad)\n> +\t\treturn;\n> +\n> +\tif (!bs.nr_good && !bs.nr_bad)\n> +\t\tprintf(_(\"status: waiting for both good and bad commits\\n\"));\n> +\telse if (bs.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  bs.nr_good), bs.nr_good);\n> +\telse\n> +\t\tprintf(_(\"status: waiting for good commit(s), bad commit known\\n\"));\n> +}\n\nCould or should these printf()'s be advise() calls instead? That would\nmake it easier for users to turn them off if they don't want the extra\noutput. At the very least, we should make sure that they are sent to\nstderr to discourage scripting around them.\n\nThanks,\nTaylor\n"},{"id":"454897","messageId":"YnSQDzhNjmu5ws4f@nand.local","threadId":"57854","inReplyTo":"acab8859d02c95750fdbc691ac672c17d5be0291.1651796862.git.chris@chrisdown.name","subject":"Re: [PATCH v2 2/2] bisect: output bisect setup status in bisect log","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-05-06T03:03:43Z","receivedAt":"2022-05-06T03:03:48Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Fri, May 06, 2022 at 01:52:54AM +0100, Chris Down wrote:\n> This allows seeing the current intermediate status without adding a new\n> good or bad commit:\n>\n>     $ git bisect log | tail -1\n>     # status: waiting for bad commit, 1 good commit known\n\nHmm. I was worried that this would make it harder to turn the output of\n\"git bisect log\" into something you can inject into \"git bisect replay\n<log>\". But it doesn't, because you prefix the status lines with a '#'\ncharacter.\n\nThat's good, and I think it's an improvement over what I'd currently\nrecommend, which would be something like:\n\n    git bisect log | grep '^# bad:'\n    git bisect log | grep '^# good:'\n\nto see the state of our good and bad endpoints.\n\nI'm not totally convinced it _needs_ to live in \"git bisect log\",\nthough, since it feels like additional information that is just added\nfor convenience. That's not the worst thing in the world, but I think\nit would be fine to just take the first patch (with my suggestions\napplied) as well.\n\n> Signed-off-by: Chris Down <chris@chrisdown.name>\n> ---\n>  builtin/bisect--helper.c    | 25 ++++++++++++++++++++-----\n>  t/t6030-bisect-porcelain.sh |  9 +++++++--\n>  2 files changed, 27 insertions(+), 7 deletions(-)\n>\n> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\n> index 9d583f651c..ef75f0a0ce 100644\n> --- a/builtin/bisect--helper.c\n> +++ b/builtin/bisect--helper.c\n> @@ -404,6 +404,21 @@ static struct bisect_state bisect_status(const struct bisect_terms *terms)\n>  \treturn bs;\n>  }\n>\n> +__attribute__((format (printf, 1, 2)))\n> +static void bisect_log_printf(const char *fmt, ...)\n> +{\n> +\tva_list ap;\n> +\tchar buf[1024];\n> +\n> +\tva_start(ap, fmt);\n> +\tif (vsnprintf(buf, sizeof(buf), fmt, ap) < 0)\n> +\t\t*buf = '\\0';\n> +\tva_end(ap);\n> +\n> +\tprintf(\"%s\", buf);\n> +\tappend_to_file(git_path_bisect_log(), \"# %s\", buf);\n> +}\n\nThis direct use of vsnprintf might be avoided by preparing the output in\nbisect_print_status() via a strbuf and then calling:\n\n    append_to_file(git_path_bisect_log(), \"# %s\", buf.buf).\n\n>  static void bisect_print_status(const struct bisect_terms *terms)\n>  {\n>  \tconst struct bisect_state bs = bisect_status(terms);\n> @@ -413,13 +428,13 @@ static void bisect_print_status(const struct bisect_terms *terms)\n>  \t\treturn;\n>\n>  \tif (!bs.nr_good && !bs.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 (bs.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  bs.nr_good), bs.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     bs.nr_good), bs.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\nInteresting; this patch removes the output that we were giving to users\nin the last patch. Should it go to both places?\n\nThanks,\nTaylor\n"},{"id":"454907","messageId":"YnTz6mbP2UDrsTaL@chrisdown.name","threadId":"57854","inReplyTo":"YnSQDzhNjmu5ws4f@nand.local","subject":"Re: [PATCH v2 2/2] bisect: output bisect setup status in bisect log","fromName":"Chris Down","fromEmail":"chris@chrisdown.name","sentAt":"2022-05-06T10:09:46Z","receivedAt":"2022-05-06T10:09:52Z","isPatch":true,"sender":{"key":"chris@chrisdown.name","avatar":"https://avatars.githubusercontent.com/u/660663?v=4"},"body":"Taylor Blau writes:\n>On Fri, May 06, 2022 at 01:52:54AM +0100, Chris Down wrote:\n>> This allows seeing the current intermediate status without adding a new\n>> good or bad commit:\n>>\n>>     $ git bisect log | tail -1\n>>     # status: waiting for bad commit, 1 good commit known\n>\n>Hmm. I was worried that this would make it harder to turn the output of\n>\"git bisect log\" into something you can inject into \"git bisect replay\n><log>\". But it doesn't, because you prefix the status lines with a '#'\n>character.\n>\n>That's good, and I think it's an improvement over what I'd currently\n>recommend, which would be something like:\n>\n>    git bisect log | grep '^# bad:'\n>    git bisect log | grep '^# good:'\n>\n>to see the state of our good and bad endpoints.\n\n>\n>I'm not totally convinced it _needs_ to live in \"git bisect log\",\n>though, since it feels like additional information that is just added\n>for convenience. That's not the worst thing in the world, but I think\n>it would be fine to just take the first patch (with my suggestions\n>applied) as well.\n\nThere was some discussion in the v1 thread (Message-ID: \n<xmqqv8uo1mk6.fsf@gitster.g>) about adding an additional `git bisect \nstatus' command, but while writing it my immediate thoughts were that it \ndoesn't make much sense to separate from the rest of the log. I'm curious what \nJunio's thoughts are on that, happy to do it whichever way is preferred. :-)\n\n>> Signed-off-by: Chris Down <chris@chrisdown.name>\n>> ---\n>>  builtin/bisect--helper.c    | 25 ++++++++++++++++++++-----\n>>  t/t6030-bisect-porcelain.sh |  9 +++++++--\n>>  2 files changed, 27 insertions(+), 7 deletions(-)\n>>\n>> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\n>> index 9d583f651c..ef75f0a0ce 100644\n>> --- a/builtin/bisect--helper.c\n>> +++ b/builtin/bisect--helper.c\n>> @@ -404,6 +404,21 @@ static struct bisect_state bisect_status(const struct bisect_terms *terms)\n>>  \treturn bs;\n>>  }\n>>\n>> +__attribute__((format (printf, 1, 2)))\n>> +static void bisect_log_printf(const char *fmt, ...)\n>> +{\n>> +\tva_list ap;\n>> +\tchar buf[1024];\n>> +\n>> +\tva_start(ap, fmt);\n>> +\tif (vsnprintf(buf, sizeof(buf), fmt, ap) < 0)\n>> +\t\t*buf = '\\0';\n>> +\tva_end(ap);\n>> +\n>> +\tprintf(\"%s\", buf);\n>> +\tappend_to_file(git_path_bisect_log(), \"# %s\", buf);\n>> +}\n>\n>This direct use of vsnprintf might be avoided by preparing the output in\n>bisect_print_status() via a strbuf and then calling:\n>\n>    append_to_file(git_path_bisect_log(), \"# %s\", buf.buf).\n\nI'm not intimately familiar with the codebase, so maybe I'm missing something, \nbut isn't it overkill to use a string buffer for something which is isn't going \nto then be used as a mutable buffer?\n\nHappy to do it whichever way works for you folks, but would be good to \nunderstand the rationale so that I can write better patches next time :-)\n\n>>  static void bisect_print_status(const struct bisect_terms *terms)\n>>  {\n>>  \tconst struct bisect_state bs = bisect_status(terms);\n>> @@ -413,13 +428,13 @@ static void bisect_print_status(const struct bisect_terms *terms)\n>>  \t\treturn;\n>>\n>>  \tif (!bs.nr_good && !bs.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 (bs.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  bs.nr_good), bs.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     bs.nr_good), bs.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>Interesting; this patch removes the output that we were giving to users\n>in the last patch. Should it go to both places?\n\nNot sure I understand, we printf() in bisect_log_printf, no?\n"},{"id":"454908","messageId":"YnT06hCm9zR287aY@chrisdown.name","threadId":"57854","inReplyTo":"YnSNWMBRZA0S8X26@nand.local","subject":"Re: [PATCH v2 1/2] bisect: output state before we are ready to compute bisection","fromName":"Chris Down","fromEmail":"chris@chrisdown.name","sentAt":"2022-05-06T10:14:02Z","receivedAt":"2022-05-06T10:14:10Z","isPatch":true,"sender":{"key":"chris@chrisdown.name","avatar":"https://avatars.githubusercontent.com/u/660663?v=4"},"body":"Taylor Blau writes:\n>Could or should these printf()'s be advise() calls instead? That would\n>make it easier for users to turn them off if they don't want the extra\n>output. At the very least, we should make sure that they are sent to\n>stderr to discourage scripting around them.\n\nAh, I didn't know about advise(); looks like it could be reasonable to use \nhere. If nobody objects, I'll change to advise() when the rest of the feedback \nhas come in and v3 is sent.\n\nThanks!\n"},{"id":"454920","messageId":"xmqqmtfuobno.fsf@gitster.g","threadId":"57854","inReplyTo":"YnSNWMBRZA0S8X26@nand.local","subject":"Re: [PATCH v2 1/2] bisect: output state before we are ready to compute bisection","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-05-06T16:42:35Z","receivedAt":"2022-05-06T16:42:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau <me@ttaylorr.com> writes:\n\n>> +\tif (!bs.nr_good && !bs.nr_bad)\n>> +\t\tprintf(_(\"status: waiting for both good and bad commits\\n\"));\n>> +\telse if (bs.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  bs.nr_good), bs.nr_good);\n>> +\telse\n>> +\t\tprintf(_(\"status: waiting for good commit(s), bad commit known\\n\"));\n>> +}\n>\n> Could or should these printf()'s be advise() calls instead?\n\nGiven that existing bisect_next_all() mesasge to give estimates come\nto the standard output, I do not think so.\n\n\t/*\n\t * TRANSLATORS: the last %s will be replaced with \"(roughly %d\n\t * steps)\" translation.\n\t */\n\tprintf(Q_(\"Bisecting: %d revision left to test after this %s\\n\",\n\t\t  \"Bisecting: %d revisions left to test after this %s\\n\",\n\t\t  nr), nr, steps_msg);\n\nI view these new messages as merely correcting the gap we used to\nhave.  We should have been giving feedback to the end-user when they\ndid something, but instead we were giving feedback only when we did\nsomething, which resulted in the original \"Huh?\" that motivated this\nseries.\n\nI actually wonder if we should do s/status:/Bisecting:/ to make the\nmessages even more uniform, but if we were to go in that direction\nin the longer term, we'd probably be downcasing \"Bisecting\" to match\nour error/warning/info messages.\n\nThanks.\n\n\n\n"},{"id":"454921","messageId":"xmqqh762obf4.fsf@gitster.g","threadId":"57854","inReplyTo":"acab8859d02c95750fdbc691ac672c17d5be0291.1651796862.git.chris@chrisdown.name","subject":"Re: [PATCH v2 2/2] bisect: output bisect setup status in bisect log","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-05-06T16:47:43Z","receivedAt":"2022-05-06T16:47:52Z","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> This allows seeing the current intermediate status without adding a new\n> good or bad commit:\n>\n>     $ git bisect log | tail -1\n>     # status: waiting for bad commit, 1 good commit known\n\nClever.  As we are keeping the \"log\" of what we have done so far,\nit is a good place to record this new piece of info, too.  I find\nit much better than my earlier suggestion to add \"bisect status\"\nsubcommand.\n\n> +__attribute__((format (printf, 1, 2)))\n> +static void bisect_log_printf(const char *fmt, ...)\n> +{\n> +\tva_list ap;\n> +\tchar buf[1024];\n> +\n> +\tva_start(ap, fmt);\n> +\tif (vsnprintf(buf, sizeof(buf), fmt, ap) < 0)\n> +\t\t*buf = '\\0';\n> +\tva_end(ap);\n\nWe probably should use strbuf_addf() or something so that we do not\neven have to wonder if 1024 is large enough for everybody.  That\nwould be in line with what is done in other parts of the codebase,\ne.g. bisect_next_all() uses xstrfmt() that is a thin wrapper around\nstrbuf to format the estimated steps.\n\nThanks.\n"},{"id":"454923","messageId":"xmqq7d6yoazk.fsf@gitster.g","threadId":"57854","inReplyTo":"YnSQDzhNjmu5ws4f@nand.local","subject":"Re: [PATCH v2 2/2] bisect: output bisect setup status in bisect log","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-05-06T16:57:03Z","receivedAt":"2022-05-06T16:57:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau <me@ttaylorr.com> writes:\n\n> On Fri, May 06, 2022 at 01:52:54AM +0100, Chris Down wrote:\n>> This allows seeing the current intermediate status without adding a new\n>> good or bad commit:\n>>\n>>     $ git bisect log | tail -1\n>>     # status: waiting for bad commit, 1 good commit known\n>\n> Hmm. I was worried that this would make it harder to turn the output of\n> \"git bisect log\" into something you can inject into \"git bisect replay\n> <log>\". But it doesn't, because you prefix the status lines with a '#'\n> character.\n\n;-) \n\nIt has always been the basic design of \"git bisect replay\" since the\nold days back when the command was scripted.  The log is designed to\nbe an executable shell script and we do not need to use \"replay\" at\nall.\n\n> I'm not totally convinced it _needs_ to live in \"git bisect log\",\n\nIt is a convenient place and other \"helpful\" comments are also\nstored there, so the existing users may already know to look into it\nto reorient where they were.\n\nThe original suggestion during the previous round of this series was\nto come up with a way to help those who get lost during a long\nsession of spelunking the history to start bisection, like so:\n\n * start a bisection session\n * find one end (perhaps bad end)\n * say \"git bisect bad $this\" and be told \"now go find a good one\"\n * ran around various versions with \"git reset --hard $that &&\n   make\" to see the other end (perhaps good end).\n * forget where they were---was it their turn to feed a good\n   revision or bad revision?\n * \"git bisect status\" can be used to remind them that they were\n   told \"now go find a good one\".\n\nAnd I actually find that \"go to log and see what progress you made\nso far\" as implemented in this series a far easier and scalable\nsolution than \"git bisect status\".  Next time we add more \"here is\nan extra piece of information to help reorient yourself\", we do not\nhave to worry about how to present it in \"git bisect status\" output\n(and we do not have to worry about teaching \"bisect status\"\nsubcommand about it in the first place).  As long as we make sure\nthat everybody uses the new \"bisect_log_printf()\" wrapper for\ninformational messages, we get it for free.\n\nThanks.\n"},{"id":"454933","messageId":"xmqqtua2lecl.fsf@gitster.g","threadId":"57854","inReplyTo":"11edd3e4dbaac7fada8a3bcd43f4bbd353087637.1651796862.git.chris@chrisdown.name","subject":"Re: [PATCH v2 1/2] bisect: output state before we are ready to compute bisection","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-05-06T18:12:42Z","receivedAt":"2022-05-06T18:12:53Z","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> diff --git a/bisect.h b/bisect.h\n> index 1015aeb8ea..ccd9ad31f6 100644\n> --- a/bisect.h\n> +++ b/bisect.h\n> @@ -62,6 +62,12 @@ enum bisect_error {\n>  \tBISECT_INTERNAL_SUCCESS_MERGE_BASE = -11\n>  };\n>  \n> +struct bisect_state {\n> +\tint nr_good;  /* How many good commits do we have? */\n> +\tint nr_bad;   /* How many bad commits do we have? (Well, you can only\n> +\t\t\t  have 0 or 1, but...) */\n> +};\n\n;-)  Multi-line comment style (cf. Documentation/CodingGuidelines)?\n\n> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\n> index 8b2b259ff0..9d583f651c 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> +\tint *nr = (int *)cb_data;\n> +\t(*nr)++;\n> +\treturn 0;\n>  }\n\n\"mark_good\" used to be a way to see if there is _any_ ref that match\nthe glob \"refs/bisect/good-*\"---the missing_good variable in the caller\nis initialized to \"true\" (pessimism) and this callback is called for\neach such ref.  Because the first such call is enough to say \"yes,\nwe do have such a ref\" without finding the second or subsequent \"good\"\nones, the function used to return 1 to stop for_each_glob_ref_in()\nearly.\n\nAll of that is now different here.  The helper is to \"count\" the\nrefs that match the glob.  So the caller will instead initialize the\ncounter to 0, and each time this callback is called, we increment it,\nand because we do not want to stop the iteration early, we return 0.\n\nAll makes sense.\n\n>  static const char need_bad_and_good_revision_warning[] =\n> @@ -384,23 +384,49 @@ 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 struct bisect_state bisect_status(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> +\tstruct bisect_state bs;\n> +\n> +\tmemset(&bs, 0, sizeof(bs));\n\nWe should just zero-initialize the struct by\n\n\tstruct bisect_state bs = { 0 };\n\nwithout memset().  But see below\n\n>  \tif (ref_exists(bad_ref))\n> -\t\tmissing_bad = 0;\n> +\t\tbs.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 *) &bs.nr_good);\n>  \tfree(good_glob);\n>  \tfree(bad_ref);\n>  \n> -\treturn decide_next(terms, current_term, missing_good, missing_bad);\n> +\treturn bs;\n> +}\n\nPassing struct by value?\n\nIt is more usual in this codebase for the caller to prepare a struct\nand give a pointer to it to a helper function like this one, i.e.\n\n    static void bisect_status(struct bisect_state *state,\n\t\t\t      const struct bisect_terms *terms)\n    {\n\t...\n\tfor_each_glob_ref_in(count_matches, good_glob, \"refs/bisect\",\n\t\t\t     (void *) &state->nr_good);\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\t...\n\nwould be more common.\n\n> diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh\n> index 5382e5d216..a02587d1a7 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> +       git bisect reset &&\n> +       git bisect start >output &&\n> +       grep \"waiting for both good and bad commits\" output &&\n> +       git bisect good \"$HASH1\" >output &&\n> +       grep \"waiting for bad commit, 1 good commit known\" output &&\n> +       git bisect good \"$HASH2\" >output &&\n> +       grep \"waiting for bad commit, 2 good commits known\" output\n> +'\n> +\n> +test_expect_success 'bisect state output with bad commit' '\n> +       git bisect reset &&\n> +       git bisect start >output &&\n> +       grep \"waiting for both good and bad commits\" output &&\n> +       git bisect bad \"$HASH4\" >output &&\n> +       grep -F \"waiting for good commit(s), bad commit known\" output\n> +'\n> +\n>  test_done\n\nLooking good.\n\nThanks.\n"},{"id":"454934","messageId":"xmqqh762le32.fsf@gitster.g","threadId":"57854","inReplyTo":"acab8859d02c95750fdbc691ac672c17d5be0291.1651796862.git.chris@chrisdown.name","subject":"Re: [PATCH v2 2/2] bisect: output bisect setup status in bisect log","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-05-06T18:18:25Z","receivedAt":"2022-05-06T18:18:32Z","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> +__attribute__((format (printf, 1, 2)))\n> +static void bisect_log_printf(const char *fmt, ...)\n> +{\n> +\tva_list ap;\n> +\tchar buf[1024];\n> +\n> +\tva_start(ap, fmt);\n> +\tif (vsnprintf(buf, sizeof(buf), fmt, ap) < 0)\n> +\t\t*buf = '\\0';\n> +\tva_end(ap);\n\nWe should just do\n\n\tstruct strbuf buf = STRBUF_INIT;\n\n\tva_start(ap, fmt);\n\tstrbuf_vaddf(&buf, fmt, ap);\n\tva_end(ap);\n\n> +\tprintf(\"%s\", buf);\n> +\tappend_to_file(git_path_bisect_log(), \"# %s\", buf);\n\nand free the resource with\n\n\tstrbuf_release(&buf);\n\nI think.\n\n> +}\n\n> diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh\n> index a02587d1a7..d16730a2e2 100755\n> --- a/t/t6030-bisect-porcelain.sh\n> +++ b/t/t6030-bisect-porcelain.sh\n> @@ -1029,18 +1029,23 @@ test_expect_success 'bisect state output with multiple good commits' '\n>         git bisect reset &&\n>         git bisect start >output &&\n>         grep \"waiting for both good and bad commits\" output &&\n> +       git bisect log | grep \"waiting for both good and bad commits\" &&\n\nHaving \"git\" command on the left hand side of pipe would hide a\nfailure signalled by its exit status from the command.  The \"but if\nthe command fails, how likely would we see the expected output to\nits standard ouput?\" argument aside, it is more common to write\n\n\t  git bisect log >output &&\n\t  grep \"...\" &&\n\ninstead.\n\nThanks.\n"},{"id":"454968","messageId":"YnZQvuyeu/WZLhXH@chrisdown.name","threadId":"57854","inReplyTo":"cover.1651796862.git.chris@chrisdown.name","subject":"Re: [PATCH v2 0/2] bisect: status improvements when bisect is not fully fleshed out","fromName":"Chris Down","fromEmail":"chris@chrisdown.name","sentAt":"2022-05-07T10:58:06Z","receivedAt":"2022-05-07T10:58:16Z","isPatch":true,"sender":{"key":"chris@chrisdown.name","avatar":"https://avatars.githubusercontent.com/u/660663?v=4"},"body":"Thanks Junio and Taylor for reviewing. I have the following action items for \nv3:\n\n# [1/3] bisect: lowercase \"Bisect:\" to \"bisect:\" prior to wider use\n\n- New patch\n\n# [2/3] bisect: output state before we are ready to compute bisection \n\n- Fix multiline comment style in bisect.h\n- Zero-initialise bisect_state directly, don't use memset()\n- Pass the bisect state struct as an argument into bisect_print_status\n- Change from \"status:\" to \"bisecting:\"\n\n# [3/3] bisect: output bisect setup status in bisect log\n\n- Use strbuf in bisect_log_printf\n- Change `git bisect log' use an output file in tests instead of piping\n\nI'll wait a bit to see if there's any further feedback and then will send v3.\n\nThanks!\n"},{"id":"454979","messageId":"xmqqk0axgpxl.fsf@gitster.g","threadId":"57854","inReplyTo":"YnZQvuyeu/WZLhXH@chrisdown.name","subject":"Re: [PATCH v2 0/2] bisect: status improvements when bisect is not fully fleshed out","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-05-07T18:25:58Z","receivedAt":"2022-05-07T18:26:10Z","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> Thanks Junio and Taylor for reviewing. I have the following action\n> items for v3:\n>\n> # [1/3] bisect: lowercase \"Bisect:\" to \"bisect:\" prior to wider use\n>\n> - New patch\n\nMy preference actually were to leave this change out of the topic.\nThat is, instead of using \"status:\" in newer messages, have them use\nthe same \"Bisecting:\", so that all the \"where we are in the bisect\nsession?\" messages from the command use that same prefix.\n\nI also wonder if the existing \"Bisecting:\" messages should also be\nsent as comment to the log file, using the same bisect_log_printf()\nhelper (with the v2 patches, they are still using printf() and sent\nonly to the standard output).\n\nBut this, just like \"status:\" -> \"Bisecting:\" -> \"bisecting:\" you\nreacted to, is just \"I wonder...\", and is not a suggestion to make\nchanges as part of this series.  Something to think about for a\npossible follow-up after we complete this topic.\n\nBut I do not mind if you want to go the extra mile to do all of the\nabove as part of the series.  It would make the series to require\nmore reviews, which is why I generally recommend against extending\nthe scope of the (initial) topic too much and instead leave as much\nadditional changes to follow-up series after the initial series is\ndone.\n\n> # [2/3] bisect: output state before we are ready to compute bisection \n> - Fix multiline comment style in bisect.h\n> - Zero-initialise bisect_state directly, don't use memset()\n> - Pass the bisect state struct as an argument into bisect_print_status\n> - Change from \"status:\" to \"bisecting:\"\n>\n> # [3/3] bisect: output bisect setup status in bisect log\n>\n> - Use strbuf in bisect_log_printf\n> - Change `git bisect log' use an output file in tests instead of piping\n>\n> I'll wait a bit to see if there's any further feedback and then will send v3.\n>\n> Thanks!\n"},{"id":"454985","messageId":"YnbjLFJBp70eCngT@chrisdown.name","threadId":"57854","inReplyTo":"xmqqk0axgpxl.fsf@gitster.g","subject":"Re: [PATCH v2 0/2] bisect: status improvements when bisect is not fully fleshed out","fromName":"Chris Down","fromEmail":"chris@chrisdown.name","sentAt":"2022-05-07T21:22:52Z","receivedAt":"2022-05-07T21:22:59Z","isPatch":true,"sender":{"key":"chris@chrisdown.name","avatar":"https://avatars.githubusercontent.com/u/660663?v=4"},"body":"Junio C Hamano writes:\n>Chris Down <chris@chrisdown.name> writes:\n>\n>> Thanks Junio and Taylor for reviewing. I have the following action\n>> items for v3:\n>>\n>> # [1/3] bisect: lowercase \"Bisect:\" to \"bisect:\" prior to wider use\n>>\n>> - New patch\n>\n>My preference actually were to leave this change out of the topic.\n>That is, instead of using \"status:\" in newer messages, have them use\n>the same \"Bisecting:\", so that all the \"where we are in the bisect\n>session?\" messages from the command use that same prefix.\n\nSure, that also works.\n\n>I also wonder if the existing \"Bisecting:\" messages should also be\n>sent as comment to the log file, using the same bisect_log_printf()\n>helper (with the v2 patches, they are still using printf() and sent\n>only to the standard output).\n>\n>But this, just like \"status:\" -> \"Bisecting:\" -> \"bisecting:\" you\n>reacted to, is just \"I wonder...\", and is not a suggestion to make\n>changes as part of this series.  Something to think about for a\n>possible follow-up after we complete this topic.\n>\n>But I do not mind if you want to go the extra mile to do all of the\n>above as part of the series.  It would make the series to require\n>more reviews, which is why I generally recommend against extending\n>the scope of the (initial) topic too much and instead leave as much\n>additional changes to follow-up series after the initial series is\n>done.\n\nSounds good then, I'll send another one after this goes in :-)\n\nThanks!\n"},{"id":"455011","messageId":"Ynk2LrMyHIKyK63i@nand.local","threadId":"57854","inReplyTo":"YnTz6mbP2UDrsTaL@chrisdown.name","subject":"Re: [PATCH v2 2/2] bisect: output bisect setup status in bisect log","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-05-09T15:41:34Z","receivedAt":"2022-05-09T15:41:43Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Fri, May 06, 2022 at 11:09:46AM +0100, Chris Down wrote:\n> > I'm not totally convinced it _needs_ to live in \"git bisect log\",\n> > though, since it feels like additional information that is just added\n> > for convenience. That's not the worst thing in the world, but I think\n> > it would be fine to just take the first patch (with my suggestions\n> > applied) as well.\n>\n> There was some discussion in the v1 thread (Message-ID:\n> <xmqqv8uo1mk6.fsf@gitster.g>) about adding an additional `git bisect status'\n> command, but while writing it my immediate thoughts were that it doesn't\n> make much sense to separate from the rest of the log. I'm curious what\n> Junio's thoughts are on that, happy to do it whichever way is preferred. :-)\n\nI don't have a strong feeling either way. The way that you have\nincorporated it into the output of \"git bisect log\" feels simple and\nsane to me.\n\nThe most important thing is that you didn't break the ability to pipe\nthe unmodified output of \"git bisect log\" into \"git bisect replay\", so I\nbeyond that I don't have any strong feelings about it.\n\n> > > Signed-off-by: Chris Down <chris@chrisdown.name>\n> > > ---\n> > >  builtin/bisect--helper.c    | 25 ++++++++++++++++++++-----\n> > >  t/t6030-bisect-porcelain.sh |  9 +++++++--\n> > >  2 files changed, 27 insertions(+), 7 deletions(-)\n> > >\n> > > diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\n> > > index 9d583f651c..ef75f0a0ce 100644\n> > > --- a/builtin/bisect--helper.c\n> > > +++ b/builtin/bisect--helper.c\n> > > @@ -404,6 +404,21 @@ static struct bisect_state bisect_status(const struct bisect_terms *terms)\n> > >  \treturn bs;\n> > >  }\n> > >\n> > > +__attribute__((format (printf, 1, 2)))\n> > > +static void bisect_log_printf(const char *fmt, ...)\n> > > +{\n> > > +\tva_list ap;\n> > > +\tchar buf[1024];\n> > > +\n> > > +\tva_start(ap, fmt);\n> > > +\tif (vsnprintf(buf, sizeof(buf), fmt, ap) < 0)\n> > > +\t\t*buf = '\\0';\n> > > +\tva_end(ap);\n> > > +\n> > > +\tprintf(\"%s\", buf);\n> > > +\tappend_to_file(git_path_bisect_log(), \"# %s\", buf);\n> > > +}\n> >\n> > This direct use of vsnprintf might be avoided by preparing the output in\n> > bisect_print_status() via a strbuf and then calling:\n> >\n> >    append_to_file(git_path_bisect_log(), \"# %s\", buf.buf).\n>\n> I'm not intimately familiar with the codebase, so maybe I'm missing\n> something, but isn't it overkill to use a string buffer for something which\n> is isn't going to then be used as a mutable buffer?\n>\n> Happy to do it whichever way works for you folks, but would be good to\n> understand the rationale so that I can write better patches next time :-)\n\nI'm trying to avoid having a vsnprintf() when one isn't strictly needed.\nThis part of bisect isn't performance critical code, so having a strbuf\naround doesn't bother me in the slightest.\n\nIf you really wanted to, you could treat the strbuf like a static buffer\nby declaring it as such and calling strbuf_reset() before or after using\nit. But allocating little bits of memory in bisect_print_status()\ndoesn't bother me much, either.\n\nAt the very least, it would dramatically simplify the implementation of\nthis patch with a negligible cost, so I'd recommend doing it.\n\n> > >  static void bisect_print_status(const struct bisect_terms *terms)\n> > >  {\n> > >  \tconst struct bisect_state bs = bisect_status(terms);\n> > > @@ -413,13 +428,13 @@ static void bisect_print_status(const struct bisect_terms *terms)\n> > >  \t\treturn;\n> > >\n> > >  \tif (!bs.nr_good && !bs.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 (bs.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  bs.nr_good), bs.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     bs.nr_good), bs.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> > Interesting; this patch removes the output that we were giving to users\n> > in the last patch. Should it go to both places?\n>\n> Not sure I understand, we printf() in bisect_log_printf, no?\n\nI missed that, thanks for pointing it out.\n\nThanks,\nTaylor\n"},{"id":"455012","messageId":"Ynk2q1XVGsLPvALn@nand.local","threadId":"57854","inReplyTo":"xmqqh762le32.fsf@gitster.g","subject":"Re: [PATCH v2 2/2] bisect: output bisect setup status in bisect log","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-05-09T15:43:39Z","receivedAt":"2022-05-09T15:43:48Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Fri, May 06, 2022 at 11:18:25AM -0700, Junio C Hamano wrote:\n> Chris Down <chris@chrisdown.name> writes:\n>\n> > +__attribute__((format (printf, 1, 2)))\n> > +static void bisect_log_printf(const char *fmt, ...)\n> > +{\n> > +\tva_list ap;\n> > +\tchar buf[1024];\n> > +\n> > +\tva_start(ap, fmt);\n> > +\tif (vsnprintf(buf, sizeof(buf), fmt, ap) < 0)\n> > +\t\t*buf = '\\0';\n> > +\tva_end(ap);\n>\n> We should just do\n>\n> \tstruct strbuf buf = STRBUF_INIT;\n>\n> \tva_start(ap, fmt);\n> \tstrbuf_vaddf(&buf, fmt, ap);\n> \tva_end(ap);\n>\n> > +\tprintf(\"%s\", buf);\n> > +\tappend_to_file(git_path_bisect_log(), \"# %s\", buf);\n>\n> and free the resource with\n>\n> \tstrbuf_release(&buf);\n>\n> I think.\n\nI don't think so. bisect_log_printf() has only one caller, which is\nbisect_print_status(). Couldn't the latter manage its own strbuf without\nthe need to introduce a new varargs function?\n\nThanks,\nTaylor\n"},{"id":"455017","messageId":"xmqqv8ueelip.fsf@gitster.g","threadId":"57854","inReplyTo":"Ynk2q1XVGsLPvALn@nand.local","subject":"Re: [PATCH v2 2/2] bisect: output bisect setup status in bisect log","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-05-09T16:08:46Z","receivedAt":"2022-05-09T16:08:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau <me@ttaylorr.com> writes:\n\n> On Fri, May 06, 2022 at 11:18:25AM -0700, Junio C Hamano wrote:\n>> Chris Down <chris@chrisdown.name> writes:\n>>\n>> > +__attribute__((format (printf, 1, 2)))\n>> > +static void bisect_log_printf(const char *fmt, ...)\n>> > +{\n>> > +\tva_list ap;\n>> > +\tchar buf[1024];\n>> > +\n>> > +\tva_start(ap, fmt);\n>> > +\tif (vsnprintf(buf, sizeof(buf), fmt, ap) < 0)\n>> > +\t\t*buf = '\\0';\n>> > +\tva_end(ap);\n>>\n>> We should just do\n>>\n>> \tstruct strbuf buf = STRBUF_INIT;\n>>\n>> \tva_start(ap, fmt);\n>> \tstrbuf_vaddf(&buf, fmt, ap);\n>> \tva_end(ap);\n>>\n>> > +\tprintf(\"%s\", buf);\n>> > +\tappend_to_file(git_path_bisect_log(), \"# %s\", buf);\n>>\n>> and free the resource with\n>>\n>> \tstrbuf_release(&buf);\n>>\n>> I think.\n>\n> I don't think so. bisect_log_printf() has only one caller, which is\n> bisect_print_status(). Couldn't the latter manage its own strbuf without\n> the need to introduce a new varargs function?\n\nI actually was hoping that other existing informational messages\nprefixed with \"Bisecting:\" (i.e. those that tells you how many steps\nare remaining) can go as similar comments to the log file; they are\ncurrently written with plain-vanilla printf(3), and could use this\none instead.\n"},{"id":"455020","messageId":"YnlA4NpNMuyVkQIR@nand.local","threadId":"57854","inReplyTo":"xmqqv8ueelip.fsf@gitster.g","subject":"Re: [PATCH v2 2/2] bisect: output bisect setup status in bisect log","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-05-09T16:27:12Z","receivedAt":"2022-05-09T16:27:23Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, May 09, 2022 at 09:08:46AM -0700, Junio C Hamano wrote:\n> > I don't think so. bisect_log_printf() has only one caller, which is\n> > bisect_print_status(). Couldn't the latter manage its own strbuf without\n> > the need to introduce a new varargs function?\n>\n> I actually was hoping that other existing informational messages\n> prefixed with \"Bisecting:\" (i.e. those that tells you how many steps\n> are remaining) can go as similar comments to the log file; they are\n> currently written with plain-vanilla printf(3), and could use this\n> one instead.\n\nI see. In that case I agree that we this function should take varargs,\nand it should pass that va_list to strbuf_vaddf().\n\nBut this patch shouldn't reimplement the wheel here with a bare\nvsnprintf() call that could be replaced with a strbuf.\n\nThanks,\nTaylor\n"}]}