{"thread":{"id":"57102","subject":"[PATCH v7 0/7] progress: test fixes / cleanup","startedAt":"2021-12-17T04:25:24Z","lastAt":"2022-01-03T23:48:06Z","messageCount":16,"participants":["Ævar Arnfjörð Bjarmason","Johannes Altmanninger","Junio C Hamano"],"isPatch":true,"patchVersion":7,"patchTotal":7},"messages":[{"id":"444316","messageId":"cover-v7-0.7-00000000000-20211217T041945Z-avarab@gmail.com","threadId":"57102","inReplyTo":null,"subject":"[PATCH v7 0/7] progress: test fixes / cleanup","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-12-17T04:24:55Z","receivedAt":"2021-12-17T04:25:24Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"A series that used to be about adding a \"global progress\" state, but\nstarted with some cleanups & adding the ability to test that mode. See\n[1] for the v6 and [2] for a discussion on the previous 8/8 patch.\n\nThis v7 ejects that 8/8 patch. I think it's safe to merge down, but\nper [3] etc. this topic has been stalled for a while in \"needs\nreview\". I think it's had enough thorough review. The sticking point\nhas been me & SZEDER disagreeing on whether adding that BUG() in the\nprevious 8/8 can be deemed safe or not.\n\nSo let's bypass that for now and hopefully queue these up. Once this\nlands we can re-visit that 8/8 and various other progress.c API & UX\nimprovements I have unsubmitted, which have been blocked by this\ntopic.\n\n1. https://lore.kernel.org/git/cover-v6-0.8-00000000000-20211102T122507Z-avarab@gmail.com/\n2. https://lore.kernel.org/git/211203.868rx2t0hv.gmgdl@evledraar.gmail.com/\n3. https://lore.kernel.org/git/211115.86wnl9qkfz.gmgdl@evledraar.gmail.com/\n\nÆvar Arnfjörð Bjarmason (7):\n  leak tests: fix a memory leaks in \"test-progress\" helper\n  progress.c test helper: add missing braces\n  progress.c tests: make start/stop commands on stdin\n  progress.c tests: test some invalid usage\n  progress.c: add temporary variable from progress struct\n  pack-bitmap-write.c: don't return without stop_progress()\n  various *.c: use isatty(0|2), not isatty(STDIN_FILENO|STDERR_FILENO)\n\n builtin/bisect--helper.c    |  2 +-\n builtin/bundle.c            |  2 +-\n compat/mingw.c              |  2 +-\n pack-bitmap-write.c         |  6 +--\n progress.c                  |  5 +-\n t/helper/test-progress.c    | 52 +++++++++++++++-----\n t/t0500-progress-display.sh | 94 ++++++++++++++++++++++++++++---------\n 7 files changed, 121 insertions(+), 42 deletions(-)\n\nRange-diff against v6:\n1:  88d89652831 = 1:  5367293ee84 leak tests: fix a memory leaks in \"test-progress\" helper\n2:  2fa92e7db83 = 2:  81788101763 progress.c test helper: add missing braces\n3:  400b491449e = 3:  d685c248686 progress.c tests: make start/stop commands on stdin\n4:  7557975a122 = 4:  40e446da277 progress.c tests: test some invalid usage\n5:  61c8da31aeb = 5:  c2303bfd130 progress.c: add temporary variable from progress struct\n6:  f6a76b80e91 = 6:  776362de897 pack-bitmap-write.c: don't return without stop_progress()\n7:  1a2eadf28d0 ! 7:  0670d1aa5f2 various *.c: use isatty(0|2), not isatty(STDIN_FILENO|STDERR_FILENO)\n    @@ Commit message\n         {STDIN_FILENO,STD{OUT,ERR}_FILENO} macros in \"stdlib.h\" to refer to\n         them.\n     \n    -    Let's change these for consistency, and because a subsequent commit's\n    -    commit message outlines a recipe to change all of these for ad-hoc\n    -    testing, not needing to match these with that ad-hoc regex will make\n    -    things easier to explain.\n    +    Let's change these for consistency, and because another commit that\n    +    would like to be based on top of this one[1] has a recipe to change\n    +    all of these for ad-hoc testing, not needing to match these with that\n    +    ad-hoc regex will make things easier to explain. Only one of these is\n    +    related to the \"struct progress\" code which it discusses, but let's\n    +    change all of these while we're at it.\n    +\n    +    1. https://lore.kernel.org/git/patch-v6-8.8-bff919994b5-20211102T122507Z-avarab@gmail.com/\n     \n         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n     \n8:  bff919994b5 < -:  ----------- progress.c: add & assert a \"global_progress\" variable\n-- \n2.34.1.1119.g7a3fc8778ee\n\n"},{"id":"444317","messageId":"patch-v7-1.7-5367293ee84-20211217T041945Z-avarab@gmail.com","threadId":"57102","inReplyTo":"cover-v7-0.7-00000000000-20211217T041945Z-avarab@gmail.com","subject":"[PATCH v7 1/7] leak tests: fix a memory leaks in \"test-progress\" helper","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-12-17T04:24:56Z","receivedAt":"2021-12-17T04:25:27Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Fix a memory leak in the test-progress helper, and mark the\ncorresponding \"t0500-progress-display.sh\" test as being leak-free\nunder SANITIZE=leak. This fixes a leak added in 2bb74b53a4 (Test the\nprogress display, 2019-09-16).\n\nMy 48f68715b14 (tr2: stop leaking \"thread_name\" memory, 2021-08-27)\nhad fixed another memory leak in this test (as it did some trace2\ntesting).\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n t/helper/test-progress.c    | 1 +\n t/t0500-progress-display.sh | 1 +\n 2 files changed, 2 insertions(+)\n\ndiff --git a/t/helper/test-progress.c b/t/helper/test-progress.c\nindex 5d05cbe7894..9265e6ab7cf 100644\n--- a/t/helper/test-progress.c\n+++ b/t/helper/test-progress.c\n@@ -69,6 +69,7 @@ int cmd__progress(int argc, const char **argv)\n \t\t\tdie(\"invalid input: '%s'\\n\", line.buf);\n \t}\n \tstop_progress(&progress);\n+\tstrbuf_release(&line);\n \n \treturn 0;\n }\ndiff --git a/t/t0500-progress-display.sh b/t/t0500-progress-display.sh\nindex 22058b503ac..f37cf2eb9c9 100755\n--- a/t/t0500-progress-display.sh\n+++ b/t/t0500-progress-display.sh\n@@ -2,6 +2,7 @@\n \n test_description='progress display'\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n show_cr () {\n-- \n2.34.1.1119.g7a3fc8778ee\n\n"},{"id":"444318","messageId":"patch-v7-2.7-81788101763-20211217T041945Z-avarab@gmail.com","threadId":"57102","inReplyTo":"cover-v7-0.7-00000000000-20211217T041945Z-avarab@gmail.com","subject":"[PATCH v7 2/7] progress.c test helper: add missing braces","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-12-17T04:24:57Z","receivedAt":"2021-12-17T04:25:29Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"If we have braces on one arm of an if/else all of them should have it,\nper the CodingGuidelines's \"When there are multiple arms to a\nconditional[...]\" advice. This formatting change makes a subsequent\ncommit smaller.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n t/helper/test-progress.c | 5 +++--\n 1 file changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/t/helper/test-progress.c b/t/helper/test-progress.c\nindex 9265e6ab7cf..50fd3be3dad 100644\n--- a/t/helper/test-progress.c\n+++ b/t/helper/test-progress.c\n@@ -63,10 +63,11 @@ int cmd__progress(int argc, const char **argv)\n \t\t\t\tdie(\"invalid input: '%s'\\n\", line.buf);\n \t\t\tprogress_test_ns = test_ms * 1000 * 1000;\n \t\t\tdisplay_throughput(progress, byte_count);\n-\t\t} else if (!strcmp(line.buf, \"update\"))\n+\t\t} else if (!strcmp(line.buf, \"update\")) {\n \t\t\tprogress_test_force_update();\n-\t\telse\n+\t\t} else {\n \t\t\tdie(\"invalid input: '%s'\\n\", line.buf);\n+\t\t}\n \t}\n \tstop_progress(&progress);\n \tstrbuf_release(&line);\n-- \n2.34.1.1119.g7a3fc8778ee\n\n"},{"id":"444319","messageId":"patch-v7-3.7-d685c248686-20211217T041945Z-avarab@gmail.com","threadId":"57102","inReplyTo":"cover-v7-0.7-00000000000-20211217T041945Z-avarab@gmail.com","subject":"[PATCH v7 3/7] progress.c tests: make start/stop commands on stdin","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-12-17T04:24:58Z","receivedAt":"2021-12-17T04:25:29Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change the usage of the \"test-tool progress\" introduced in\n2bb74b53a49 (Test the progress display, 2019-09-16) to take command\nlike \"start\" and \"stop\" on stdin, instead of running them implicitly.\n\nThis makes for tests that are easier to read, since the recipe will\nmirror the API usage, and allows for easily testing invalid usage that\nwould yield (or should yield) a BUG(), e.g. providing two \"start\"\ncalls in a row. A subsequent commit will add such tests.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n t/helper/test-progress.c    | 46 ++++++++++++++++++++++-------\n t/t0500-progress-display.sh | 58 +++++++++++++++++++++++--------------\n 2 files changed, 72 insertions(+), 32 deletions(-)\n\ndiff --git a/t/helper/test-progress.c b/t/helper/test-progress.c\nindex 50fd3be3dad..1435c28e950 100644\n--- a/t/helper/test-progress.c\n+++ b/t/helper/test-progress.c\n@@ -3,6 +3,9 @@\n  *\n  * Reads instructions from standard input, one instruction per line:\n  *\n+ *   \"start <total>[ <title>]\" - Call start_progress(title, total),\n+ *                               Uses the default title of \"Working hard\"\n+ *                               if the \" <title>\" is omitted.\n  *   \"progress <items>\" - Call display_progress() with the given item count\n  *                        as parameter.\n  *   \"throughput <bytes> <millis> - Call display_throughput() with the given\n@@ -10,6 +13,7 @@\n  *                                  specify the time elapsed since the\n  *                                  start_progress() call.\n  *   \"update\" - Set the 'progress_update' flag.\n+ *   \"stop\" - Call stop_progress().\n  *\n  * See 't0500-progress-display.sh' for examples.\n  */\n@@ -19,34 +23,52 @@\n #include \"parse-options.h\"\n #include \"progress.h\"\n #include \"strbuf.h\"\n+#include \"string-list.h\"\n+\n+/*\n+ * We can't use \"end + 1\" as an argument to start_progress() below, it\n+ * doesn't xstrdup() its \"title\" argument. We need to hold onto a\n+ * valid \"char *\" for it until the end.\n+ */\n+static char *dup_title(struct string_list *titles, const char *title)\n+{\n+\treturn string_list_insert(titles, title)->string;\n+}\n \n int cmd__progress(int argc, const char **argv)\n {\n-\tint total = 0;\n-\tconst char *title;\n+\tconst char *const default_title = \"Working hard\";\n+\tstruct string_list titles = STRING_LIST_INIT_DUP;\n \tstruct strbuf line = STRBUF_INIT;\n-\tstruct progress *progress;\n+\tstruct progress *progress = NULL;\n \n \tconst char *usage[] = {\n-\t\t\"test-tool progress [--total=<n>] <progress-title>\",\n+\t\t\"test-tool progress <stdin\",\n \t\tNULL\n \t};\n \tstruct option options[] = {\n-\t\tOPT_INTEGER(0, \"total\", &total, \"total number of items\"),\n \t\tOPT_END(),\n \t};\n \n \targc = parse_options(argc, argv, NULL, options, usage, 0);\n-\tif (argc != 1)\n-\t\tdie(\"need a title for the progress output\");\n-\ttitle = argv[0];\n+\tif (argc)\n+\t\tusage_with_options(usage, options);\n \n \tprogress_testing = 1;\n-\tprogress = start_progress(title, total);\n \twhile (strbuf_getline(&line, stdin) != EOF) {\n \t\tchar *end;\n \n-\t\tif (skip_prefix(line.buf, \"progress \", (const char **) &end)) {\n+\t\tif (skip_prefix(line.buf, \"start \", (const char **) &end)) {\n+\t\t\tuint64_t total = strtoull(end, &end, 10);\n+\t\t\tif (*end == '\\0')\n+\t\t\t\tprogress = start_progress(default_title, total);\n+\t\t\telse if (*end == ' ')\n+\t\t\t\tprogress = start_progress(dup_title(&titles,\n+\t\t\t\t\t\t\t\t    end + 1),\n+\t\t\t\t\t\t\t  total);\n+\t\t\telse\n+\t\t\t\tdie(\"invalid input: '%s'\\n\", line.buf);\n+\t\t} else if (skip_prefix(line.buf, \"progress \", (const char **) &end)) {\n \t\t\tuint64_t item_count = strtoull(end, &end, 10);\n \t\t\tif (*end != '\\0')\n \t\t\t\tdie(\"invalid input: '%s'\\n\", line.buf);\n@@ -65,12 +87,14 @@ int cmd__progress(int argc, const char **argv)\n \t\t\tdisplay_throughput(progress, byte_count);\n \t\t} else if (!strcmp(line.buf, \"update\")) {\n \t\t\tprogress_test_force_update();\n+\t\t} else if (!strcmp(line.buf, \"stop\")) {\n+\t\t\tstop_progress(&progress);\n \t\t} else {\n \t\t\tdie(\"invalid input: '%s'\\n\", line.buf);\n \t\t}\n \t}\n-\tstop_progress(&progress);\n \tstrbuf_release(&line);\n+\tstring_list_clear(&titles, 0);\n \n \treturn 0;\n }\ndiff --git a/t/t0500-progress-display.sh b/t/t0500-progress-display.sh\nindex f37cf2eb9c9..27ab4218b01 100755\n--- a/t/t0500-progress-display.sh\n+++ b/t/t0500-progress-display.sh\n@@ -18,6 +18,7 @@ test_expect_success 'simple progress display' '\n \tEOF\n \n \tcat >in <<-\\EOF &&\n+\tstart 0\n \tupdate\n \tprogress 1\n \tupdate\n@@ -26,8 +27,9 @@ test_expect_success 'simple progress display' '\n \tprogress 4\n \tupdate\n \tprogress 5\n+\tstop\n \tEOF\n-\ttest-tool progress \"Working hard\" <in 2>stderr &&\n+\ttest-tool progress <in 2>stderr &&\n \n \tshow_cr <stderr >out &&\n \ttest_cmp expect out\n@@ -42,11 +44,13 @@ test_expect_success 'progress display with total' '\n \tEOF\n \n \tcat >in <<-\\EOF &&\n+\tstart 3\n \tprogress 1\n \tprogress 2\n \tprogress 3\n+\tstop\n \tEOF\n-\ttest-tool progress --total=3 \"Working hard\" <in 2>stderr &&\n+\ttest-tool progress <in 2>stderr &&\n \n \tshow_cr <stderr >out &&\n \ttest_cmp expect out\n@@ -63,14 +67,14 @@ Working hard.......2.........3.........4.........5.........6:\n EOF\n \n \tcat >in <<-\\EOF &&\n+\tstart 100000 Working hard.......2.........3.........4.........5.........6\n \tprogress 100\n \tprogress 1000\n \tprogress 10000\n \tprogress 100000\n+\tstop\n \tEOF\n-\ttest-tool progress --total=100000 \\\n-\t\t\"Working hard.......2.........3.........4.........5.........6\" \\\n-\t\t<in 2>stderr &&\n+\ttest-tool progress <in 2>stderr &&\n \n \tshow_cr <stderr >out &&\n \ttest_cmp expect out\n@@ -89,16 +93,16 @@ Working hard.......2.........3.........4.........5.........6:\n EOF\n \n \tcat >in <<-\\EOF &&\n+\tstart 100000 Working hard.......2.........3.........4.........5.........6\n \tupdate\n \tprogress 1\n \tupdate\n \tprogress 2\n \tprogress 10000\n \tprogress 100000\n+\tstop\n \tEOF\n-\ttest-tool progress --total=100000 \\\n-\t\t\"Working hard.......2.........3.........4.........5.........6\" \\\n-\t\t<in 2>stderr &&\n+\ttest-tool progress <in 2>stderr &&\n \n \tshow_cr <stderr >out &&\n \ttest_cmp expect out\n@@ -117,14 +121,14 @@ Working hard.......2.........3.........4.........5.........6:\n EOF\n \n \tcat >in <<-\\EOF &&\n+\tstart 100000 Working hard.......2.........3.........4.........5.........6\n \tprogress 25000\n \tprogress 50000\n \tprogress 75000\n \tprogress 100000\n+\tstop\n \tEOF\n-\ttest-tool progress --total=100000 \\\n-\t\t\"Working hard.......2.........3.........4.........5.........6\" \\\n-\t\t<in 2>stderr &&\n+\ttest-tool progress <in 2>stderr &&\n \n \tshow_cr <stderr >out &&\n \ttest_cmp expect out\n@@ -141,14 +145,14 @@ Working hard.......2.........3.........4.........5.........6.........7.........:\n EOF\n \n \tcat >in <<-\\EOF &&\n+\tstart 100000 Working hard.......2.........3.........4.........5.........6.........7.........\n \tprogress 25000\n \tprogress 50000\n \tprogress 75000\n \tprogress 100000\n+\tstop\n \tEOF\n-\ttest-tool progress --total=100000 \\\n-\t\t\"Working hard.......2.........3.........4.........5.........6.........7.........\" \\\n-\t\t<in 2>stderr &&\n+\ttest-tool progress <in 2>stderr &&\n \n \tshow_cr <stderr >out &&\n \ttest_cmp expect out\n@@ -165,12 +169,14 @@ test_expect_success 'progress shortens - crazy caller' '\n \tEOF\n \n \tcat >in <<-\\EOF &&\n+\tstart 1000\n \tprogress 100\n \tprogress 200\n \tprogress 1\n \tprogress 1000\n+\tstop\n \tEOF\n-\ttest-tool progress --total=1000 \"Working hard\" <in 2>stderr &&\n+\ttest-tool progress <in 2>stderr &&\n \n \tshow_cr <stderr >out &&\n \ttest_cmp expect out\n@@ -186,6 +192,7 @@ test_expect_success 'progress display with throughput' '\n \tEOF\n \n \tcat >in <<-\\EOF &&\n+\tstart 0\n \tthroughput 102400 1000\n \tupdate\n \tprogress 10\n@@ -198,8 +205,9 @@ test_expect_success 'progress display with throughput' '\n \tthroughput 409600 4000\n \tupdate\n \tprogress 40\n+\tstop\n \tEOF\n-\ttest-tool progress \"Working hard\" <in 2>stderr &&\n+\ttest-tool progress <in 2>stderr &&\n \n \tshow_cr <stderr >out &&\n \ttest_cmp expect out\n@@ -215,6 +223,7 @@ test_expect_success 'progress display with throughput and total' '\n \tEOF\n \n \tcat >in <<-\\EOF &&\n+\tstart 40\n \tthroughput 102400 1000\n \tprogress 10\n \tthroughput 204800 2000\n@@ -223,8 +232,9 @@ test_expect_success 'progress display with throughput and total' '\n \tprogress 30\n \tthroughput 409600 4000\n \tprogress 40\n+\tstop\n \tEOF\n-\ttest-tool progress --total=40 \"Working hard\" <in 2>stderr &&\n+\ttest-tool progress <in 2>stderr &&\n \n \tshow_cr <stderr >out &&\n \ttest_cmp expect out\n@@ -240,6 +250,7 @@ test_expect_success 'cover up after throughput shortens' '\n \tEOF\n \n \tcat >in <<-\\EOF &&\n+\tstart 0\n \tthroughput 409600 1000\n \tupdate\n \tprogress 1\n@@ -252,8 +263,9 @@ test_expect_success 'cover up after throughput shortens' '\n \tthroughput 1638400 4000\n \tupdate\n \tprogress 4\n+\tstop\n \tEOF\n-\ttest-tool progress \"Working hard\" <in 2>stderr &&\n+\ttest-tool progress <in 2>stderr &&\n \n \tshow_cr <stderr >out &&\n \ttest_cmp expect out\n@@ -268,6 +280,7 @@ test_expect_success 'cover up after throughput shortens a lot' '\n \tEOF\n \n \tcat >in <<-\\EOF &&\n+\tstart 0\n \tthroughput 1 1000\n \tupdate\n \tprogress 1\n@@ -277,8 +290,9 @@ test_expect_success 'cover up after throughput shortens a lot' '\n \tthroughput 3145728 3000\n \tupdate\n \tprogress 3\n+\tstop\n \tEOF\n-\ttest-tool progress \"Working hard\" <in 2>stderr &&\n+\ttest-tool progress <in 2>stderr &&\n \n \tshow_cr <stderr >out &&\n \ttest_cmp expect out\n@@ -286,6 +300,7 @@ test_expect_success 'cover up after throughput shortens a lot' '\n \n test_expect_success 'progress generates traces' '\n \tcat >in <<-\\EOF &&\n+\tstart 40\n \tthroughput 102400 1000\n \tupdate\n \tprogress 10\n@@ -298,10 +313,11 @@ test_expect_success 'progress generates traces' '\n \tthroughput 409600 4000\n \tupdate\n \tprogress 40\n+\tstop\n \tEOF\n \n-\tGIT_TRACE2_EVENT=\"$(pwd)/trace.event\" test-tool progress --total=40 \\\n-\t\t\"Working hard\" <in 2>stderr &&\n+\tGIT_TRACE2_EVENT=\"$(pwd)/trace.event\" test-tool progress \\\n+\t\t<in 2>stderr &&\n \n \t# t0212/parse_events.perl intentionally omits regions and data.\n \ttest_region progress \"Working hard\" trace.event &&\n-- \n2.34.1.1119.g7a3fc8778ee\n\n"},{"id":"444320","messageId":"patch-v7-5.7-c2303bfd130-20211217T041945Z-avarab@gmail.com","threadId":"57102","inReplyTo":"cover-v7-0.7-00000000000-20211217T041945Z-avarab@gmail.com","subject":"[PATCH v7 5/7] progress.c: add temporary variable from progress struct","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-12-17T04:25:00Z","receivedAt":"2021-12-17T04:25:31Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Add a temporary \"progress\" variable for the dereferenced p_progress\npointer to a \"struct progress *\". Before 98a13647408 (trace2: log\nprogress time and throughput, 2020-05-12) we didn't dereference\n\"p_progress\" in this function, now that we do it's easier to read the\ncode if we work with a \"progress\" struct pointer like everywhere else,\ninstead of a pointer to a pointer.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n progress.c | 5 +++--\n 1 file changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/progress.c b/progress.c\nindex 680c6a8bf93..76a95cb7322 100644\n--- a/progress.c\n+++ b/progress.c\n@@ -325,15 +325,16 @@ void stop_progress(struct progress **p_progress)\n \tfinish_if_sparse(*p_progress);\n \n \tif (*p_progress) {\n+\t\tstruct progress *progress = *p_progress;\n \t\ttrace2_data_intmax(\"progress\", the_repository, \"total_objects\",\n \t\t\t\t   (*p_progress)->total);\n \n \t\tif ((*p_progress)->throughput)\n \t\t\ttrace2_data_intmax(\"progress\", the_repository,\n \t\t\t\t\t   \"total_bytes\",\n-\t\t\t\t\t   (*p_progress)->throughput->curr_total);\n+\t\t\t\t\t   progress->throughput->curr_total);\n \n-\t\ttrace2_region_leave(\"progress\", (*p_progress)->title, the_repository);\n+\t\ttrace2_region_leave(\"progress\", progress->title, the_repository);\n \t}\n \n \tstop_progress_msg(p_progress, _(\"done\"));\n-- \n2.34.1.1119.g7a3fc8778ee\n\n"},{"id":"444321","messageId":"patch-v7-4.7-40e446da277-20211217T041945Z-avarab@gmail.com","threadId":"57102","inReplyTo":"cover-v7-0.7-00000000000-20211217T041945Z-avarab@gmail.com","subject":"[PATCH v7 4/7] progress.c tests: test some invalid usage","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-12-17T04:24:59Z","receivedAt":"2021-12-17T04:25:32Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Test what happens when we \"stop\" without a \"start\", omit the \"stop\"\nafter a \"start\", or try to start two concurrent progress bars. This\nextends the trace2 tests added in 98a13647408 (trace2: log progress\ntime and throughput, 2020-05-12).\n\nThese tests are not merely testing the helper, but invalid API usage\nthat can happen if the progress.c API is misused.\n\nThe \"without stop\" test will leak under SANITIZE=leak, since this\nbuggy use of the API will leak memory. But let's not skip it entirely,\nor use the \"!SANITIZE_LEAK\" prerequisite check as we'd do with tests\nthat we're skipping due to leaks we haven't fixed yet. Instead\nannotate the specific command that should skip leak checking with\ncustom $LSAN_OPTIONS[1].\n\n1. https://github.com/google/sanitizers/wiki/AddressSanitizerLeakSanitizer\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n t/t0500-progress-display.sh | 35 +++++++++++++++++++++++++++++++++++\n 1 file changed, 35 insertions(+)\n\ndiff --git a/t/t0500-progress-display.sh b/t/t0500-progress-display.sh\nindex 27ab4218b01..59e9f226ea4 100755\n--- a/t/t0500-progress-display.sh\n+++ b/t/t0500-progress-display.sh\n@@ -325,4 +325,39 @@ test_expect_success 'progress generates traces' '\n \tgrep \"\\\"key\\\":\\\"total_bytes\\\",\\\"value\\\":\\\"409600\\\"\" trace.event\n '\n \n+test_expect_success 'progress generates traces: stop / start' '\n+\tcat >in <<-\\EOF &&\n+\tstart 0\n+\tstop\n+\tEOF\n+\n+\tGIT_TRACE2_EVENT=\"$(pwd)/trace-startstop.event\" test-tool progress \\\n+\t\t<in 2>stderr &&\n+\ttest_region progress \"Working hard\" trace-startstop.event\n+'\n+\n+test_expect_success 'progress generates traces: start without stop' '\n+\tcat >in <<-\\EOF &&\n+\tstart 0\n+\tEOF\n+\n+\tGIT_TRACE2_EVENT=\"$(pwd)/trace-start.event\" \\\n+\tLSAN_OPTIONS=detect_leaks=0 \\\n+\ttest-tool progress \\\n+\t\t<in 2>stderr &&\n+\tgrep region_enter.*progress trace-start.event &&\n+\t! grep region_leave.*progress trace-start.event\n+'\n+\n+test_expect_success 'progress generates traces: stop without start' '\n+\tcat >in <<-\\EOF &&\n+\tstop\n+\tEOF\n+\n+\tGIT_TRACE2_EVENT=\"$(pwd)/trace-stop.event\" test-tool progress \\\n+\t\t<in 2>stderr &&\n+\t! grep region_enter.*progress trace-stop.event &&\n+\t! grep region_leave.*progress trace-stop.event\n+'\n+\n test_done\n-- \n2.34.1.1119.g7a3fc8778ee\n\n"},{"id":"444322","messageId":"patch-v7-6.7-776362de897-20211217T041945Z-avarab@gmail.com","threadId":"57102","inReplyTo":"cover-v7-0.7-00000000000-20211217T041945Z-avarab@gmail.com","subject":"[PATCH v7 6/7] pack-bitmap-write.c: don't return without stop_progress()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-12-17T04:25:01Z","receivedAt":"2021-12-17T04:25:34Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Fix a bug that's been here since 7cc8f971085 (pack-objects: implement\nbitmap writing, 2013-12-21), we did not call stop_progress() if we\nreached the early exit in this function.\n\nWe could call stop_progress() before we return, but better yet is to\ndefer calling start_progress() until we need it.\n\nThis will matter in a subsequent commit where we BUG(...) out if this\nhappens, and matters now e.g. because we don't have a corresponding\n\"region_end\" for the progress trace2 event.\n\nSuggested-by: SZEDER Gábor <szeder.dev@gmail.com>\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n pack-bitmap-write.c | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/pack-bitmap-write.c b/pack-bitmap-write.c\nindex 9c55c1531e1..cab3eaa2acd 100644\n--- a/pack-bitmap-write.c\n+++ b/pack-bitmap-write.c\n@@ -575,15 +575,15 @@ void bitmap_writer_select_commits(struct commit **indexed_commits,\n \n \tQSORT(indexed_commits, indexed_commits_nr, date_compare);\n \n-\tif (writer.show_progress)\n-\t\twriter.progress = start_progress(\"Selecting bitmap commits\", 0);\n-\n \tif (indexed_commits_nr < 100) {\n \t\tfor (i = 0; i < indexed_commits_nr; ++i)\n \t\t\tpush_bitmapped_commit(indexed_commits[i]);\n \t\treturn;\n \t}\n \n+\tif (writer.show_progress)\n+\t\twriter.progress = start_progress(\"Selecting bitmap commits\", 0);\n+\n \tfor (;;) {\n \t\tstruct commit *chosen = NULL;\n \n-- \n2.34.1.1119.g7a3fc8778ee\n\n"},{"id":"444323","messageId":"patch-v7-7.7-0670d1aa5f2-20211217T041945Z-avarab@gmail.com","threadId":"57102","inReplyTo":"cover-v7-0.7-00000000000-20211217T041945Z-avarab@gmail.com","subject":"[PATCH v7 7/7] various *.c: use isatty(0|2), not isatty(STDIN_FILENO|STDERR_FILENO)","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-12-17T04:25:02Z","receivedAt":"2021-12-17T04:25:35Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"We have over 50 uses of \"isatty(1)\" and \"isatty(2)\" in the codebase,\nand around 10 \"isatty(0)\", but these used the\n{STDIN_FILENO,STD{OUT,ERR}_FILENO} macros in \"stdlib.h\" to refer to\nthem.\n\nLet's change these for consistency, and because another commit that\nwould like to be based on top of this one[1] has a recipe to change\nall of these for ad-hoc testing, not needing to match these with that\nad-hoc regex will make things easier to explain. Only one of these is\nrelated to the \"struct progress\" code which it discusses, but let's\nchange all of these while we're at it.\n\n1. https://lore.kernel.org/git/patch-v6-8.8-bff919994b5-20211102T122507Z-avarab@gmail.com/\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n builtin/bisect--helper.c | 2 +-\n builtin/bundle.c         | 2 +-\n compat/mingw.c           | 2 +-\n 3 files changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\nindex 28a2e6a5750..21360a4e70b 100644\n--- a/builtin/bisect--helper.c\n+++ b/builtin/bisect--helper.c\n@@ -830,7 +830,7 @@ static int bisect_autostart(struct bisect_terms *terms)\n \tfprintf_ln(stderr, _(\"You need to start by \\\"git bisect \"\n \t\t\t  \"start\\\"\\n\"));\n \n-\tif (!isatty(STDIN_FILENO))\n+\tif (!isatty(0))\n \t\treturn -1;\n \n \t/*\ndiff --git a/builtin/bundle.c b/builtin/bundle.c\nindex 5a85d7cd0fe..df69c651753 100644\n--- a/builtin/bundle.c\n+++ b/builtin/bundle.c\n@@ -56,7 +56,7 @@ static int parse_options_cmd_bundle(int argc,\n \n static int cmd_bundle_create(int argc, const char **argv, const char *prefix) {\n \tint all_progress_implied = 0;\n-\tint progress = isatty(STDERR_FILENO);\n+\tint progress = isatty(2);\n \tstruct strvec pack_opts;\n \tint version = -1;\n \tint ret;\ndiff --git a/compat/mingw.c b/compat/mingw.c\nindex e14f2d5f77c..7c55d0f0414 100644\n--- a/compat/mingw.c\n+++ b/compat/mingw.c\n@@ -2376,7 +2376,7 @@ int mingw_raise(int sig)\n \tswitch (sig) {\n \tcase SIGALRM:\n \t\tif (timer_fn == SIG_DFL) {\n-\t\t\tif (isatty(STDERR_FILENO))\n+\t\t\tif (isatty(2))\n \t\t\t\tfputs(\"Alarm clock\\n\", stderr);\n \t\t\texit(128 + SIGALRM);\n \t\t} else if (timer_fn != SIG_IGN)\n-- \n2.34.1.1119.g7a3fc8778ee\n\n"},{"id":"444985","messageId":"20211227010727.ak6hr5m3wc6kwblh@gmail.com","threadId":"57102","inReplyTo":"patch-v7-1.7-5367293ee84-20211217T041945Z-avarab@gmail.com","subject":"Re: [PATCH v7 1/7] leak tests: fix a memory leaks in \"test-progress\" helper","fromName":"Johannes Altmanninger","fromEmail":"aclopte@gmail.com","sentAt":"2021-12-27T01:07:27Z","receivedAt":"2021-12-27T01:07:34Z","isPatch":true,"sender":{"key":"aclopte@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6853872?v=4"},"body":"looks good; maybe change the subject: \"memory leaks\" -> \"memory leak\"\n"},{"id":"444986","messageId":"20211227011013.3ngeh57llxnknphf@gmail.com","threadId":"57102","inReplyTo":"patch-v7-3.7-d685c248686-20211217T041945Z-avarab@gmail.com","subject":"Re: [PATCH v7 3/7] progress.c tests: make start/stop commands on stdin","fromName":"Johannes Altmanninger","fromEmail":"aclopte@gmail.com","sentAt":"2021-12-27T01:10:13Z","receivedAt":"2021-12-27T01:10:19Z","isPatch":true,"sender":{"key":"aclopte@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6853872?v=4"},"body":"On Fri, Dec 17, 2021 at 05:24:58AM +0100, Ævar Arnfjörð Bjarmason wrote:\n> Change the usage of the \"test-tool progress\" introduced in\n> 2bb74b53a49 (Test the progress display, 2019-09-16) to take command\n> like \"start\" and \"stop\" on stdin, instead of running them implicitly.\n> \n> This makes for tests that are easier to read, since the recipe will\n> mirror the API usage, and allows for easily testing invalid usage that\n\n(Of course invalid API usage wasn't really a problem before, but it's good\nthat you mention the upcoming tests, to calm any concerns)\n\n> would yield (or should yield) a BUG(), e.g. providing two \"start\"\n> calls in a row. A subsequent commit will add such tests.\n> \n> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n> ---\n>  t/helper/test-progress.c    | 46 ++++++++++++++++++++++-------\n>  t/t0500-progress-display.sh | 58 +++++++++++++++++++++++--------------\n>  2 files changed, 72 insertions(+), 32 deletions(-)\n> \n> diff --git a/t/helper/test-progress.c b/t/helper/test-progress.c\n> index 50fd3be3dad..1435c28e950 100644\n> --- a/t/helper/test-progress.c\n> +++ b/t/helper/test-progress.c\n> @@ -3,6 +3,9 @@\n>   *\n>   * Reads instructions from standard input, one instruction per line:\n>   *\n> + *   \"start <total>[ <title>]\" - Call start_progress(title, total),\n> + *                               Uses the default title of \"Working hard\"\n> + *                               if the \" <title>\" is omitted.\n>   *   \"progress <items>\" - Call display_progress() with the given item count\n>   *                        as parameter.\n>   *   \"throughput <bytes> <millis> - Call display_throughput() with the given\n> @@ -10,6 +13,7 @@\n>   *                                  specify the time elapsed since the\n>   *                                  start_progress() call.\n>   *   \"update\" - Set the 'progress_update' flag.\n> + *   \"stop\" - Call stop_progress().\n>   *\n>   * See 't0500-progress-display.sh' for examples.\n>   */\n> @@ -19,34 +23,52 @@\n>  #include \"parse-options.h\"\n>  #include \"progress.h\"\n>  #include \"strbuf.h\"\n> +#include \"string-list.h\"\n> +\n> +/*\n> + * We can't use \"end + 1\" as an argument to start_progress() below, it\n> + * doesn't xstrdup() its \"title\" argument. We need to hold onto a\n> + * valid \"char *\" for it until the end.\n> + */\n> +static char *dup_title(struct string_list *titles, const char *title)\n> +{\n> +\treturn string_list_insert(titles, title)->string;\n> +}\n\nIt seems weird to reference someone else's local variables in \"end + 1\" here.\nHow about inlining this function instead?\n\n\t\t\t} else if (*end == ' ') {\n\t\t\t\tchar *title_duped = string_list_insert(&titles, end + 1)->string;\n\t\t\t\tprogress = start_progress(title_duped, total);\n\t\t\t} else {\n\nand maybe add a comment there, but I'm not sure if we still need it.\n\n>  \n>  int cmd__progress(int argc, const char **argv)\n>  {\n> -\tint total = 0;\n> -\tconst char *title;\n> +\tconst char *const default_title = \"Working hard\";\n> +\tstruct string_list titles = STRING_LIST_INIT_DUP;\n>  \tstruct strbuf line = STRBUF_INIT;\n> -\tstruct progress *progress;\n> +\tstruct progress *progress = NULL;\n>  \n>  \tconst char *usage[] = {\n> -\t\t\"test-tool progress [--total=<n>] <progress-title>\",\n> +\t\t\"test-tool progress <stdin\",\n>  \t\tNULL\n\n(unrelated: I'd always add a trailing comma if I can, even though in this case it won't ever matter)\n\n>  \t};\n>  \tstruct option options[] = {\n> -\t\tOPT_INTEGER(0, \"total\", &total, \"total number of items\"),\n>  \t\tOPT_END(),\n>  \t};\n>  \n>  \targc = parse_options(argc, argv, NULL, options, usage, 0);\n> -\tif (argc != 1)\n> -\t\tdie(\"need a title for the progress output\");\n> -\ttitle = argv[0];\n> +\tif (argc)\n> +\t\tusage_with_options(usage, options);\n>  \n>  \tprogress_testing = 1;\n> -\tprogress = start_progress(title, total);\n>  \twhile (strbuf_getline(&line, stdin) != EOF) {\n>  \t\tchar *end;\n>  \n> -\t\tif (skip_prefix(line.buf, \"progress \", (const char **) &end)) {\n> +\t\tif (skip_prefix(line.buf, \"start \", (const char **) &end)) {\n> +\t\t\tuint64_t total = strtoull(end, &end, 10);\n> +\t\t\tif (*end == '\\0')\n> +\t\t\t\tprogress = start_progress(default_title, total);\n> +\t\t\telse if (*end == ' ')\n> +\t\t\t\tprogress = start_progress(dup_title(&titles,\n> +\t\t\t\t\t\t\t\t    end + 1),\n> +\t\t\t\t\t\t\t  total);\n> +\t\t\telse\n> +\t\t\t\tdie(\"invalid input: '%s'\\n\", line.buf);\n> +\t\t} else if (skip_prefix(line.buf, \"progress \", (const char **) &end)) {\n>  \t\t\tuint64_t item_count = strtoull(end, &end, 10);\n>  \t\t\tif (*end != '\\0')\n>  \t\t\t\tdie(\"invalid input: '%s'\\n\", line.buf);\n> @@ -65,12 +87,14 @@ int cmd__progress(int argc, const char **argv)\n>  \t\t\tdisplay_throughput(progress, byte_count);\n>  \t\t} else if (!strcmp(line.buf, \"update\")) {\n>  \t\t\tprogress_test_force_update();\n> +\t\t} else if (!strcmp(line.buf, \"stop\")) {\n> +\t\t\tstop_progress(&progress);\n>  \t\t} else {\n>  \t\t\tdie(\"invalid input: '%s'\\n\", line.buf);\n>  \t\t}\n>  \t}\n> -\tstop_progress(&progress);\n>  \tstrbuf_release(&line);\n> +\tstring_list_clear(&titles, 0);\n>  \n>  \treturn 0;\n>  }\n> diff --git a/t/t0500-progress-display.sh b/t/t0500-progress-display.sh\n> index f37cf2eb9c9..27ab4218b01 100755\n> --- a/t/t0500-progress-display.sh\n> +++ b/t/t0500-progress-display.sh\n> @@ -18,6 +18,7 @@ test_expect_success 'simple progress display' '\n>  \tEOF\n>  \n>  \tcat >in <<-\\EOF &&\n> +\tstart 0\n>  \tupdate\n>  \tprogress 1\n>  \tupdate\n> @@ -26,8 +27,9 @@ test_expect_success 'simple progress display' '\n>  \tprogress 4\n>  \tupdate\n>  \tprogress 5\n> +\tstop\n>  \tEOF\n> -\ttest-tool progress \"Working hard\" <in 2>stderr &&\n> +\ttest-tool progress <in 2>stderr &&\n>  \n>  \tshow_cr <stderr >out &&\n>  \ttest_cmp expect out\n> @@ -42,11 +44,13 @@ test_expect_success 'progress display with total' '\n>  \tEOF\n>  \n>  \tcat >in <<-\\EOF &&\n> +\tstart 3\n>  \tprogress 1\n>  \tprogress 2\n>  \tprogress 3\n> +\tstop\n>  \tEOF\n> -\ttest-tool progress --total=3 \"Working hard\" <in 2>stderr &&\n> +\ttest-tool progress <in 2>stderr &&\n>  \n>  \tshow_cr <stderr >out &&\n>  \ttest_cmp expect out\n> @@ -63,14 +67,14 @@ Working hard.......2.........3.........4.........5.........6:\n>  EOF\n>  \n>  \tcat >in <<-\\EOF &&\n> +\tstart 100000 Working hard.......2.........3.........4.........5.........6\n>  \tprogress 100\n>  \tprogress 1000\n>  \tprogress 10000\n>  \tprogress 100000\n> +\tstop\n>  \tEOF\n> -\ttest-tool progress --total=100000 \\\n> -\t\t\"Working hard.......2.........3.........4.........5.........6\" \\\n\nI don't know enough about progress tests to judge if this is better.\nThe start invocation does look nicer, but it might feel weird to always\ninclude \"stop\". We could do that automatically but then we're no longer\nmirroring the API...\n\n> -\t\t<in 2>stderr &&\n> +\ttest-tool progress <in 2>stderr &&\n"},{"id":"444987","messageId":"20211227011100.g3pwc67onlc7qs7t@gmail.com","threadId":"57102","inReplyTo":"patch-v7-4.7-40e446da277-20211217T041945Z-avarab@gmail.com","subject":"Re: [PATCH v7 4/7] progress.c tests: test some invalid usage","fromName":"Johannes Altmanninger","fromEmail":"aclopte@gmail.com","sentAt":"2021-12-27T01:11:00Z","receivedAt":"2021-12-27T01:11:05Z","isPatch":true,"sender":{"key":"aclopte@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6853872?v=4"},"body":"On Fri, Dec 17, 2021 at 05:24:59AM +0100, Ævar Arnfjörð Bjarmason wrote:\n> Test what happens when we \"stop\" without a \"start\", omit the \"stop\"\n> after a \"start\", or try to start two concurrent progress bars.\n\nI think the concurrent case is missing (I guess that's not blocking)\n\n> This extends the trace2 tests added in 98a13647408 (trace2: log progress\n> time and throughput, 2020-05-12).\n> \n> These tests are not merely testing the helper, but invalid API usage\n> that can happen if the progress.c API is misused.\n> \n> The \"without stop\" test will leak under SANITIZE=leak, since this\n> buggy use of the API will leak memory. But let's not skip it entirely,\n> or use the \"!SANITIZE_LEAK\" prerequisite check as we'd do with tests\n> that we're skipping due to leaks we haven't fixed yet. Instead\n> annotate the specific command that should skip leak checking with\n> custom $LSAN_OPTIONS[1].\n> \n> 1. https://github.com/google/sanitizers/wiki/AddressSanitizerLeakSanitizer\n> \n> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n> ---\n>  t/t0500-progress-display.sh | 35 +++++++++++++++++++++++++++++++++++\n>  1 file changed, 35 insertions(+)\n> \n> diff --git a/t/t0500-progress-display.sh b/t/t0500-progress-display.sh\n> index 27ab4218b01..59e9f226ea4 100755\n> --- a/t/t0500-progress-display.sh\n> +++ b/t/t0500-progress-display.sh\n> @@ -325,4 +325,39 @@ test_expect_success 'progress generates traces' '\n>  \tgrep \"\\\"key\\\":\\\"total_bytes\\\",\\\"value\\\":\\\"409600\\\"\" trace.event\n>  '\n>  \n> +test_expect_success 'progress generates traces: stop / start' '\n> +\tcat >in <<-\\EOF &&\n> +\tstart 0\n> +\tstop\n> +\tEOF\n> +\n> +\tGIT_TRACE2_EVENT=\"$(pwd)/trace-startstop.event\" test-tool progress \\\n> +\t\t<in 2>stderr &&\n> +\ttest_region progress \"Working hard\" trace-startstop.event\n> +'\n> +\n> +test_expect_success 'progress generates traces: start without stop' '\n> +\tcat >in <<-\\EOF &&\n> +\tstart 0\n> +\tEOF\n> +\n> +\tGIT_TRACE2_EVENT=\"$(pwd)/trace-start.event\" \\\n> +\tLSAN_OPTIONS=detect_leaks=0 \\\n> +\ttest-tool progress \\\n> +\t\t<in 2>stderr &&\n\nnit: I personally prefer always indenting after continuation lines, like\n\n\tGIT_TRACE2_EVENT=\"$(pwd)/trace-start.event\" \\\n\t\tLSAN_OPTIONS=detect_leaks=0 \\\n\t\ttest-tool progress \\\n\t\t<in 2>stderr &&\n\nbut we don't have a uniform style, so it doesn't really matter.\n\n> +\tgrep region_enter.*progress trace-start.event &&\n> +\t! grep region_leave.*progress trace-start.event\n\nshould \"!\" be \"test_must_fail\"? Same below.\n\n> +'\n> +\n> +test_expect_success 'progress generates traces: stop without start' '\n> +\tcat >in <<-\\EOF &&\n> +\tstop\n> +\tEOF\n> +\n> +\tGIT_TRACE2_EVENT=\"$(pwd)/trace-stop.event\" test-tool progress \\\n> +\t\t<in 2>stderr &&\n> +\t! grep region_enter.*progress trace-stop.event &&\n> +\t! grep region_leave.*progress trace-stop.event\n> +'\n> +\n>  test_done\n> -- \n> 2.34.1.1119.g7a3fc8778ee\n> \n"},{"id":"444988","messageId":"20211227011125.bbczjt3wctfobfso@gmail.com","threadId":"57102","inReplyTo":"patch-v7-5.7-c2303bfd130-20211217T041945Z-avarab@gmail.com","subject":"Re: [PATCH v7 5/7] progress.c: add temporary variable from progress struct","fromName":"Johannes Altmanninger","fromEmail":"aclopte@gmail.com","sentAt":"2021-12-27T01:11:25Z","receivedAt":"2021-12-27T01:11:31Z","isPatch":true,"sender":{"key":"aclopte@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6853872?v=4"},"body":"On Fri, Dec 17, 2021 at 05:25:00AM +0100, Ævar Arnfjörð Bjarmason wrote:\n> Add a temporary \"progress\" variable for the dereferenced p_progress\n> pointer to a \"struct progress *\". Before 98a13647408 (trace2: log\n> progress time and throughput, 2020-05-12) we didn't dereference\n> \"p_progress\" in this function, now that we do it's easier to read the\n> code if we work with a \"progress\" struct pointer like everywhere else,\n> instead of a pointer to a pointer.\n\nThis message contains lots of nice details, but some of that is only really\nuseful after reading the diff. You mention the parameter's name but not the\nfunction name (other combinations would make more sense).\nI'd maybe write it like this, trying to follow our usual cadence of\ndiagnosis -> action:\n\n\tSince 98a13647408 (trace2: log progress time and throughput, 2020-05-12)\n\tstop_progress() dereferences a \"struct progress **\" parameter in several\n\tplaces. Extract a dereferenced variable (like in stop_progress_msg()) to\n\treduce clutter and make it clearer who needs to write to this parameter.\n\n> \n> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n> ---\n>  progress.c | 5 +++--\n>  1 file changed, 3 insertions(+), 2 deletions(-)\n> \n> diff --git a/progress.c b/progress.c\n> index 680c6a8bf93..76a95cb7322 100644\n> --- a/progress.c\n> +++ b/progress.c\n> @@ -325,15 +325,16 @@ void stop_progress(struct progress **p_progress)\n>  \tfinish_if_sparse(*p_progress);\n>  \n>  \tif (*p_progress) {\n> +\t\tstruct progress *progress = *p_progress;\n\nYeah, it's a good idea to reduce uses of the struct progress ** parameter.\nWhy not do this even earlier in this function:\n\n\tstruct progress *progress;\n\n\tif (!p_progress)\n\t\tBUG(\"don't provide NULL to stop_progress\");\n\tprogress = *p_progress;\n\n\tfinish_if_sparse(progress);\n\n\tif (progress) {\n\t\t...\n\t}\n\n\tstop_progress_msg(p_progress, _(\"done\"));\n\nthis way we only need to dereference once (right next to the null check)\nand it's clearer that stop_progress_msg() is the only one who wants to modify\nthe parameter.\n\n>  \t\ttrace2_data_intmax(\"progress\", the_repository, \"total_objects\",\n>  \t\t\t\t   (*p_progress)->total);\n\ns/(\\*p_progress)/progress/, same in the next line\n\n>  \n>  \t\tif ((*p_progress)->throughput)\n>  \t\t\ttrace2_data_intmax(\"progress\", the_repository,\n>  \t\t\t\t\t   \"total_bytes\",\n> -\t\t\t\t\t   (*p_progress)->throughput->curr_total);\n> +\t\t\t\t\t   progress->throughput->curr_total);\n>  \n> -\t\ttrace2_region_leave(\"progress\", (*p_progress)->title, the_repository);\n> +\t\ttrace2_region_leave(\"progress\", progress->title, the_repository);\n>  \t}\n>  \n>  \tstop_progress_msg(p_progress, _(\"done\"));\n> -- \n> 2.34.1.1119.g7a3fc8778ee\n> \n"},{"id":"444989","messageId":"20211227011148.rz57cgk3fovklz6i@gmail.com","threadId":"57102","inReplyTo":"patch-v7-6.7-776362de897-20211217T041945Z-avarab@gmail.com","subject":"Re: [PATCH v7 6/7] pack-bitmap-write.c: don't return without stop_progress()","fromName":"Johannes Altmanninger","fromEmail":"aclopte@gmail.com","sentAt":"2021-12-27T01:11:48Z","receivedAt":"2021-12-27T01:11:53Z","isPatch":true,"sender":{"key":"aclopte@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6853872?v=4"},"body":"On Fri, Dec 17, 2021 at 05:25:01AM +0100, Ævar Arnfjörð Bjarmason wrote:\n> Fix a bug that's been here since 7cc8f971085 (pack-objects: implement\n> bitmap writing, 2013-12-21), we did not call stop_progress() if we\n> reached the early exit in this function.\n> \n> We could call stop_progress() before we return, but better yet is to\n> defer calling start_progress() until we need it.\n\nLooks great.\n\n> \n> This will matter in a subsequent commit where we BUG(...) out if this happens,\n\n\"this\" = \"call start without stop\"? Let's maybe be explicit here\n\n(I guess the BUG() commit was postponed, but it doesn't hurt to mention it.)\n\n> and matters now e.g. because we don't have a corresponding\n> \"region_end\" for the progress trace2 event.\n\ns/region_end/region_leave/\n\n> \n> Suggested-by: SZEDER Gábor <szeder.dev@gmail.com>\n> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n> ---\n>  pack-bitmap-write.c | 6 +++---\n>  1 file changed, 3 insertions(+), 3 deletions(-)\n> \n> diff --git a/pack-bitmap-write.c b/pack-bitmap-write.c\n> index 9c55c1531e1..cab3eaa2acd 100644\n> --- a/pack-bitmap-write.c\n> +++ b/pack-bitmap-write.c\n> @@ -575,15 +575,15 @@ void bitmap_writer_select_commits(struct commit **indexed_commits,\n>  \n>  \tQSORT(indexed_commits, indexed_commits_nr, date_compare);\n>  \n> -\tif (writer.show_progress)\n> -\t\twriter.progress = start_progress(\"Selecting bitmap commits\", 0);\n> -\n>  \tif (indexed_commits_nr < 100) {\n>  \t\tfor (i = 0; i < indexed_commits_nr; ++i)\n>  \t\t\tpush_bitmapped_commit(indexed_commits[i]);\n>  \t\treturn;\n>  \t}\n>  \n> +\tif (writer.show_progress)\n> +\t\twriter.progress = start_progress(\"Selecting bitmap commits\", 0);\n> +\n>  \tfor (;;) {\n>  \t\tstruct commit *chosen = NULL;\n>  \n> -- \n> 2.34.1.1119.g7a3fc8778ee\n> \n"},{"id":"444990","messageId":"20211227011736.jea5qvn3mffusxnc@gmail.com","threadId":"57102","inReplyTo":"patch-v7-7.7-0670d1aa5f2-20211217T041945Z-avarab@gmail.com","subject":"Re: [PATCH v7 7/7] various *.c: use isatty(0|2), not isatty(STDIN_FILENO|STDERR_FILENO)","fromName":"Johannes Altmanninger","fromEmail":"aclopte@gmail.com","sentAt":"2021-12-27T01:17:36Z","receivedAt":"2021-12-27T01:17:41Z","isPatch":true,"sender":{"key":"aclopte@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6853872?v=4"},"body":"nit: I'd maybe drop the \"various *.c: \" subject prefix. It's already\nimplied that the change is across the entire tree.\n\nOn Fri, Dec 17, 2021 at 05:25:02AM +0100, Ævar Arnfjörð Bjarmason wrote:\n> We have over 50 uses of \"isatty(1)\" and \"isatty(2)\" in the codebase,\n> and around 10 \"isatty(0)\", but these used the\n\nnit: ambiguous pronouns like \"these\" here can trip up the reader for a\nsecond. Maybe \"three places\" is clearer.\n\n> {STDIN_FILENO,STD{OUT,ERR}_FILENO} macros in \"stdlib.h\" to refer to\n> them.\n> \n> Let's change these for consistency, and because another commit that\n> would like to be based on top of this one[1] has a recipe to change\n> all of these for ad-hoc testing, not needing to match these with that\n> ad-hoc regex will make things easier to explain.\n\nthis sentence is quite long, and it also doesn't help that \"these\" refers\nto something other than it did in the previous sentence.\nHere's my attempt:\n\n\tLet's change these for consistency.  This makes it easier to change all\n\tcalls to isatty() at a whim, which is useful to test some scenarios[1].\n\n> Only one of these is\n> related to the \"struct progress\" code which it discusses, but let's\n> change all of these while we're at it.\n> \n> 1. https://lore.kernel.org/git/patch-v6-8.8-bff919994b5-20211102T122507Z-avarab@gmail.com/\n> \n> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n> ---\n>  builtin/bisect--helper.c | 2 +-\n>  builtin/bundle.c         | 2 +-\n>  compat/mingw.c           | 2 +-\n>  3 files changed, 3 insertions(+), 3 deletions(-)\n> \n> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\n> index 28a2e6a5750..21360a4e70b 100644\n> --- a/builtin/bisect--helper.c\n> +++ b/builtin/bisect--helper.c\n> @@ -830,7 +830,7 @@ static int bisect_autostart(struct bisect_terms *terms)\n>  \tfprintf_ln(stderr, _(\"You need to start by \\\"git bisect \"\n>  \t\t\t  \"start\\\"\\n\"));\n>  \n> -\tif (!isatty(STDIN_FILENO))\n> +\tif (!isatty(0))\n>  \t\treturn -1;\n>  \n>  \t/*\n> diff --git a/builtin/bundle.c b/builtin/bundle.c\n> index 5a85d7cd0fe..df69c651753 100644\n> --- a/builtin/bundle.c\n> +++ b/builtin/bundle.c\n> @@ -56,7 +56,7 @@ static int parse_options_cmd_bundle(int argc,\n>  \n>  static int cmd_bundle_create(int argc, const char **argv, const char *prefix) {\n>  \tint all_progress_implied = 0;\n> -\tint progress = isatty(STDERR_FILENO);\n> +\tint progress = isatty(2);\n>  \tstruct strvec pack_opts;\n>  \tint version = -1;\n>  \tint ret;\n> diff --git a/compat/mingw.c b/compat/mingw.c\n> index e14f2d5f77c..7c55d0f0414 100644\n> --- a/compat/mingw.c\n> +++ b/compat/mingw.c\n> @@ -2376,7 +2376,7 @@ int mingw_raise(int sig)\n>  \tswitch (sig) {\n>  \tcase SIGALRM:\n>  \t\tif (timer_fn == SIG_DFL) {\n> -\t\t\tif (isatty(STDERR_FILENO))\n> +\t\t\tif (isatty(2))\n>  \t\t\t\tfputs(\"Alarm clock\\n\", stderr);\n>  \t\t\texit(128 + SIGALRM);\n>  \t\t} else if (timer_fn != SIG_IGN)\n> -- \n> 2.34.1.1119.g7a3fc8778ee\n> \n"},{"id":"444991","messageId":"211227.864k6ug7bx.gmgdl@evledraar.gmail.com","threadId":"57102","inReplyTo":"20211227011013.3ngeh57llxnknphf@gmail.com","subject":"Re: [PATCH v7 3/7] progress.c tests: make start/stop commands on stdin","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-12-27T01:31:59Z","receivedAt":"2021-12-27T01:33:27Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Dec 27 2021, Johannes Altmanninger wrote:\n\n[Will reply to the rest later, thanks for the review...]\n\n> On Fri, Dec 17, 2021 at 05:24:58AM +0100, Ævar Arnfjörð Bjarmason wrote:\n> [...]\n>>  int cmd__progress(int argc, const char **argv)\n>>  {\n>> -\tint total = 0;\n>> -\tconst char *title;\n>> +\tconst char *const default_title = \"Working hard\";\n>> +\tstruct string_list titles = STRING_LIST_INIT_DUP;\n>>  \tstruct strbuf line = STRBUF_INIT;\n>> -\tstruct progress *progress;\n>> +\tstruct progress *progress = NULL;\n>>  \n>>  \tconst char *usage[] = {\n>> -\t\t\"test-tool progress [--total=<n>] <progress-title>\",\n>> +\t\t\"test-tool progress <stdin\",\n>>  \t\tNULL\n>\n> (unrelated: I'd always add a trailing comma if I can, even though in this case it won't ever matter)\n\nFWIW this bit is intentional coding style in git.git, see Junio's\nhttps://lore.kernel.org/git/xmqqk0g5656r.fsf@gitster.g/:\n    \n    It is a good idea to leave a comma even after the last element,\n    _unless_ there is a strong reason why the element that currently is\n    at the last MUST stay to be last when new elements are added[...]\n\nWell, in that case he's talking about enums, but the same applies even\nmore to these sorts of lists here the NULL must remain the last element.\n    \n"},{"id":"445371","messageId":"xmqqy23wgz4f.fsf@gitster.g","threadId":"57102","inReplyTo":"20211227011100.g3pwc67onlc7qs7t@gmail.com","subject":"Re: [PATCH v7 4/7] progress.c tests: test some invalid usage","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-01-03T23:48:00Z","receivedAt":"2022-01-03T23:48:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Altmanninger <aclopte@gmail.com> writes:\n\n>> +\tgrep region_enter.*progress trace-start.event &&\n>> +\t! grep region_leave.*progress trace-start.event\n>\n> should \"!\" be \"test_must_fail\"? Same below.\n\nThe answer is no.  The main difference between test_must_fail and\n\"!\" is that the former is designed to catch ungraceful death (e.g.\nby segfault) and to be used to expect controlled failure from OUR\nprograms (i.e. \"git\" and \"test-tool\" invocations).  We are not in\nthe business of catching segfaulting bug in system's \"grep\", so it\nis perfectly fine to use \"!\" here.\n"}]}